From b6f75b87f59c3e989542f472f935a0c8061ee885 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:05:09 +0800 Subject: [PATCH] fix(custom-model): don't clamp DEEPSEEK_API_KEY as a privileged env key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a real regression: DEEPSEEK_API_KEY was added to deepseek's privilegedEnvKeys alongside DEEPSEEK_BASE_URL on the theory that "the pair travels together," but that contradicts the documented and tested design (clampEnvOverridesForOwner()'s own docstring in session-routes.ts) — a non-granted owner supplying their OWN DeepSeek key removes privilege rather than granting it, since the exfiltration vector is the BASE URL (which redirects the server's own forwarded key to a foreign host), not the key itself. Removed it from the list; test/deepseek-mode.test.ts's existing two clamp tests now pass again. Also swapped that test's "unrelated override" example off CODEX_HOME, which the earlier commit in this same PR legitimately made privileged (closing a real pre-existing gap, documented in PR.md) — so it stopped being a valid "unrelated" example the moment that fix landed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017HqNWfmtBU2KN29SvSVWB3 --- deployment_plan.md | 2 +- src/config/cli-registry/stock.ts | 10 +++++++--- test/deepseek-mode.test.ts | 5 ++++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/deployment_plan.md b/deployment_plan.md index e00e6daf..a99010ab 100644 --- a/deployment_plan.md +++ b/deployment_plan.md @@ -112,7 +112,7 @@ declared capability, never an `if (mode === 'claude')` branch. | `gemini` | Env vars `GOOGLE_GEMINI_BASE_URL` + `GEMINI_API_KEY` + `GEMINI_MODEL`; CLI needs a restart to pick them up | **Confirmed BROKEN against llama.cpp/llama-swap, unresolved after real investigation.** Setting `GOOGLE_GEMINI_BASE_URL` makes gemini-cli internally select an `AuthType.GATEWAY` auth path (undocumented — inferred from behaviour) with validation requirements distinct from every normal auth mode; a real run against llama-swap fails with `Invalid auth method selected` regardless of what key/format is supplied. Tried and all failed: a Google-format dummy API key, `GOOGLE_GENAI_USE_VERTEXAI=false`, a `GEMINI_DEFAULT_AUTH_TYPE` override, and hand-writing `settings.json` directly. `--skip-trust` was a real, separate fix (without it a trust-folder check silently overrides `--approval-mode yolo` back to `default`) but does not touch this auth failure. Documented as an open gap, not shipped as working — the registry entry and injection code exist and are exercised by the test script, but end-to-end gemini support needs upstream investigation of `GATEWAY` AuthType before it can be called done | | `pi` | Config file `~/.pi/agent/models.json` with a custom provider whose `models` is an **array** of `{id}` objects (not an object keyed by id) plus `authHeader: true`. Redirected via the child process's own `HOME` env var, isolated per test/session — **not** `PI_CONFIG_DIR`, which does nothing for pi (grepped pi's entire bundled JS source: the string appears nowhere) | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back. Two real bugs found and fixed before this worked: (1) `PI_CONFIG_DIR` is not read by pi at all — pi hardcodes `~/.pi/agent/models.json` with no dedicated override, so the actual redirect has to be the child process's `HOME`; (2) `models` must be an array of `{id}` objects per pi's own bundled `docs/models.md`, not an object keyed by model id (silently loaded zero models). Also requires an explicit `--model custom/` on invocation — without it pi falls back to its own default provider and fails with "No API key found for the selected model" | | `grok` | TOML `config.toml`: a fixed `[model.codeman-custom]` block (`base_url`, `env_key` naming an env var the key rides in, never a literal TOML field) written to an isolated dir via `GROK_HOME`. Invoked with `-m codeman-custom` | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back. The ORIGINAL recipe in this table (env vars `GROK_BASE_URL`/`XAI_API_KEY`/`GROK_MODEL`) was flat-out **wrong**, not just unverified: it produced "Not signed in" against a real binary. Grok's real mechanism, confirmed against xAI's own docs and a live binary, is a `config.toml` with a `[model.]` block, redirected via `GROK_HOME`; the key still rides as an env var (`XAI_API_KEY` via `env_key`), just referenced from the TOML rather than read directly | -| `deepseek` | Reuse the **existing** `DEEPSEEK_BASE_URL` + `DEEPSEEK_API_KEY` keys (already declared in `stock.ts`, already in `privilegedEnvKeys`). No model-selection var — dsh model is a profile composition entry, not a flag/env var | **Confirmed reaching the server, but failing — unresolved.** A real run against llama-swap returns `dsh: HTTP_404: DeepSeek API error (HTTP 404)` consistently (confirmed the env vars are read: the request reaches the network rather than failing locally). Root cause not identified — plausible explanation by analogy with codex's Responses-API gap is that `dsh --profile headless` expects DeepSeek's official API response shape/path structure rather than a generic OpenAI-compatible `/v1/chat/completions` endpoint, but this was not confirmed by reading dsh's own bundled source (unlike pi/grok, where that grep resolved the question directly). Documented as best-effort/unknown, not shipped as verified working | +| `deepseek` | Reuse the **existing** `DEEPSEEK_BASE_URL` + `DEEPSEEK_API_KEY` keys (already declared in `stock.ts`). Only `DEEPSEEK_BASE_URL` is in `privilegedEnvKeys` — `DEEPSEEK_API_KEY` deliberately stays clamp-exempt, since a non-granted owner supplying their OWN key removes privilege rather than granting it (adding it to the clamp list was a real regression, caught by `test/deepseek-mode.test.ts` and fixed before merge). No model-selection var — dsh model is a profile composition entry, not a flag/env var | **Confirmed reaching the server, but failing — unresolved.** A real run against llama-swap returns `dsh: HTTP_404: DeepSeek API error (HTTP 404)` consistently (confirmed the env vars are read: the request reaches the network rather than failing locally). Root cause not identified — plausible explanation by analogy with codex's Responses-API gap is that `dsh --profile headless` expects DeepSeek's official API response shape/path structure rather than a generic OpenAI-compatible `/v1/chat/completions` endpoint, but this was not confirmed by reading dsh's own bundled source (unlike pi/grok, where that grep resolved the question directly). Documented as best-effort/unknown, not shipped as verified working | | `omp` | Config file `~/.omp/agent/models.yml` with the same array-shaped `models` + `authHeader: true` fix as pi. Redirected via `HOME`, same reasoning as pi (`PI_CONFIG_DIR` does not relocate omp's config either, despite an earlier CLAUDE.md note claiming it does) | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back, after applying the same two fixes as pi (array-shaped `models`, `HOME`-redirect instead of `PI_CONFIG_DIR`) plus an explicit `--model custom/` on invocation. Unverified against omp's own official docs (none are bundled in the install), but empirically confirmed working live | | `antigravity` | No CLI/env/config mechanism found — Antigravity's docs describe only a GUI settings panel, and explicitly say a custom endpoint "cannot currently" become the core reasoning model. **Not implemented**; toolbar entry stays disabled for this mode with an explanatory tooltip | No known mechanism | diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 180a31f0..25f3d73c 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1037,9 +1037,13 @@ const DEEPSEEK: CliEntry = { // The half no other CLI needs. `DSH_*` is an allowlisted envOverrides prefix and // applyEnvOverrides() runs LAST, so without this a non-granted owner could send // DSH_PERMISSION_MODE on the same request and land after the config clamp. - // DEEPSEEK_API_KEY added alongside DEEPSEEK_BASE_URL for the custom-model-injection.ts - // recipe (deployment_plan.md) — the pair travels together, same reasoning as base URL. - privilegedEnvKeys: ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL', 'DEEPSEEK_API_KEY'], + // ⚠️ DEEPSEEK_API_KEY deliberately stays OUT of this list (see the docstring on + // clampEnvOverridesForOwner() in session-routes.ts): _configureCliEnv() forwards the + // SERVER's own key into every dsh pane, so DEEPSEEK_BASE_URL is the exfiltration + // vector, not the key itself — a non-granted owner supplying THEIR OWN key removes + // privilege rather than granting it, and clamping it here was a real regression + // (test/deepseek-mode.test.ts) fixed before this shipped. + privilegedEnvKeys: ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL'], // Web-researched, unverified, partial: reuses the already-existing DEEPSEEK_BASE_URL/ // DEEPSEEK_API_KEY keys above. No modelVars — dsh's model is a profile-composition // entry (see `model: { source: 'none' }` above), not an env var, so forcing a specific diff --git a/test/deepseek-mode.test.ts b/test/deepseek-mode.test.ts index b5322d77..6f3d4666 100644 --- a/test/deepseek-mode.test.ts +++ b/test/deepseek-mode.test.ts @@ -372,7 +372,10 @@ describe('DeepSeek multi-user clamp: the env-var half', () => { }); it('leaves unrelated overrides alone, and returns the same object when there is nothing to strip', async () => { - const input = { DEEPSEEK_API_KEY: 'sk-test', CODEX_HOME: '/tmp/cx' }; + // CODEX_HOME is a poor "unrelated" example here — it is itself a privileged key + // (codex's own registry entry), so a genuinely non-privileged one is needed to + // prove the identity-return fast path, not just that DEEPSEEK_API_KEY is exempt. + const input = { DEEPSEEK_API_KEY: 'sk-test', OPENCODE_LOG_LEVEL: 'debug' }; const out = await _clampEnvOverridesForOwner('nobody', input); expect(out).toBe(input); expect(await _clampEnvOverridesForOwner('nobody', undefined)).toBeUndefined();