From 6232dffe03cb1c0fda080714f3ee1b46f2b007d6 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Wed, 4 Nov 2026 16:53:26 +0100 Subject: [PATCH] fix: apply the review items for #752 and #757 at merge The launch-id check is pure, so it now runs before the context warning and the swap confirmation: nobody agrees to evict a model only to get the 400. Also the search-box and in-use comments, the model-plain-colon wording, and the wiki's order of the picker's marks. Co-Authored-By: Claude Opus 5.5 --- docs/architecture-invariants.md | 2 +- docs/wiki/Custom-Model-Endpoints.md | 13 ++++--- src/web/public/index.html | 11 +++--- src/web/routes/session-routes.ts | 47 ++++++++++++------------ test/routes/session-custom-model.test.ts | 22 +++++++++++ 5 files changed, 61 insertions(+), 34 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 3b827d91..7ca12971 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -632,7 +632,7 @@ Adding the header changed the preamble, so `CODEMAN_PREAMBLE` was bumped (1.22.0 ⚠️ The actual per-CLI redirect is `capabilities.customModelInjection` on the CLI registry (four kinds: `env` for claude/gemini/copilot/deepseek, `configContentEnv` reusing opencode's existing `OPENCODE_CONFIG_CONTENT`, `configDir` for codex/pi/grok/omp — writes an isolated per-session config file, NEVER the user's real `~/.codex`/`~/.pi`/`~/.omp`/grok config — and `unsupported` for antigravity, which has no known mechanism), computed by the pure `custom-model-injection.ts` (mirrors `session-cli-builder.ts`'s no-IO discipline). ⚠️ `PI_CONFIG_DIR` does NOTHING for pi or omp (grepped pi's entire bundled JS source — the string appears nowhere); both hardcode `~/.pi/agent/models.json` / `~/.omp/agent/models.yml` with no dedicated override, so the real redirect for both is the child process's own **`HOME`**, and both need `models` as an ARRAY of `{id}` objects (an object keyed by id silently loads zero models). Grok's real mechanism turned out to be a `config.toml` `[model.]` block redirected via `GROK_HOME` — its original env-var-based recipe was flat-out wrong (produced "Not signed in" against a real binary), not just unverified. -⚠️ **Two launch paths, chosen by mechanism, not preference**: opencode/codex/gemini/pi/grok/deepseek/omp apply the selection ONE-SHOT, before the session/process ever exists, with no restart at all; claude alone still applies a selection by **restarting the session's CLI process in place** via `Session.restartCli()` — a de-restricted `reattachRemote()` reusing the same `respawn-pane -k` primitive local/remote respawns already share — because every one of these harnesses reads its endpoint config at process start, never per-turn, so there is no live hot-swap; `Session.setCustomModel()` undoes the PREVIOUS selection's env keys (and deletes its old `configDir`) before merging the new ones in, so switching endpoints or clearing back to native cloud never leaves a stale key behind. ⚠️ Deleting a key from `_envOverrides` is NOT enough on its own: `tmux setenv` persists at the tmux-session level and is inherited by `respawn-pane` (measured: `setenv FOO bar` survived two successive `respawn-pane -k`), so the retired keys are queued (`_pendingEnvUnsets`) and ride `RespawnPaneOptions.unsetEnvKeys` into `applyEnvOverrides()`, which `setenv -u`s them BEFORE re-applying the live overrides. ⚠️ `restartCli()` relaunches a CLI in an existing pane, so a CLI whose launch declares a `fallback` chain (claude) gets a conversation pinned as `resumeSessionId` for that one respawn: `--session-id ` refuses an id that already has a transcript (`Session ID ... is already in use`), and without the `--resume || --session-id ` shape the docker/remote pane commands already use, applying a model killed the pane and lost the session. The dead-pane respawn shares this pin, see [Dead-pane respawn: the resume pin](#dead-pane-respawn-the-resume-pin). ⚠️ pi, omp and grok need the config file AND a `model` launch param (`custom/` for pi/omp, grok's `[model.codeman-custom]` block name), and codex gets one too (`--model `, its pattern widened to `model-plain-colon` for qwen3-style ids): that is the registry's `customModelInjection.launchModel` template, applied onto the respawn options through `legacyConfigField` by `_withCustomModelLaunchModel()`, never by id, and a model id the CLI's `model` token pattern cannot carry is refused with a 400 rather than silently dropped by the argv engine. ⚠️ Remote (SSH) and Docker sessions are REFUSED (400): their `restartCli()` reattaches a durable tmux rather than restarting the agent and the env lands on the local pane, so they used to report `restarted:true` and change nothing. The selection survives a Codeman restart as the disk-only `__customModel` (bookkeeping: env KEYS, config dir, launch model; never the values, which carry the API key and are re-derived from the endpoint store on recovery), the config dir is removed with the session, and every secret-bearing file (`custom-model-hosts.json`, the per-session config dir) is written 0600. +⚠️ **Two launch paths, chosen by mechanism, not preference**: opencode/codex/gemini/pi/grok/deepseek/omp apply the selection ONE-SHOT, before the session/process ever exists, with no restart at all; claude alone still applies a selection by **restarting the session's CLI process in place** via `Session.restartCli()` — a de-restricted `reattachRemote()` reusing the same `respawn-pane -k` primitive local/remote respawns already share — because every one of these harnesses reads its endpoint config at process start, never per-turn, so there is no live hot-swap; `Session.setCustomModel()` undoes the PREVIOUS selection's env keys (and deletes its old `configDir`) before merging the new ones in, so switching endpoints or clearing back to native cloud never leaves a stale key behind. ⚠️ Deleting a key from `_envOverrides` is NOT enough on its own: `tmux setenv` persists at the tmux-session level and is inherited by `respawn-pane` (measured: `setenv FOO bar` survived two successive `respawn-pane -k`), so the retired keys are queued (`_pendingEnvUnsets`) and ride `RespawnPaneOptions.unsetEnvKeys` into `applyEnvOverrides()`, which `setenv -u`s them BEFORE re-applying the live overrides. ⚠️ `restartCli()` relaunches a CLI in an existing pane, so a CLI whose launch declares a `fallback` chain (claude) gets a conversation pinned as `resumeSessionId` for that one respawn: `--session-id ` refuses an id that already has a transcript (`Session ID ... is already in use`), and without the `--resume || --session-id ` shape the docker/remote pane commands already use, applying a model killed the pane and lost the session. The dead-pane respawn shares this pin, see [Dead-pane respawn: the resume pin](#dead-pane-respawn-the-resume-pin). ⚠️ pi, omp and grok need the config file AND a `model` launch param (`custom/` for pi/omp, grok's `[model.codeman-custom]` block name), and codex gets one too (`--model `, its pattern changed to `model-plain-colon`, which admits qwen3-style colon ids and requires a letter or digit first, for the native App Settings Codex model too): that is the registry's `customModelInjection.launchModel` template, applied onto the respawn options through `legacyConfigField` by `_withCustomModelLaunchModel()`, never by id, and a model id the CLI's `model` token pattern cannot carry is refused with a 400 rather than silently dropped by the argv engine. ⚠️ Remote (SSH) and Docker sessions are REFUSED (400): their `restartCli()` reattaches a durable tmux rather than restarting the agent and the env lands on the local pane, so they used to report `restarted:true` and change nothing. The selection survives a Codeman restart as the disk-only `__customModel` (bookkeeping: env KEYS, config dir, launch model; never the values, which carry the API key and are re-derived from the endpoint store on recovery), the config dir is removed with the session, and every secret-bearing file (`custom-model-hosts.json`, the per-session config dir) is written 0600. ⚠️ **Security**: every env var this feature can redirect (`ANTHROPIC_BASE_URL`, `GOOGLE_GEMINI_BASE_URL`, `CODEX_HOME`, `GROK_HOME`, `HOME` for pi/omp, `OPENCODE_CONFIG_CONTENT`, etc.) is in that CLI's `privilegedEnvKeys` — several of these were reachable via the generic `envOverrides` field's prefix allowlist BEFORE this feature existed (the env allowlist is global and prefix-based, not per-CLI-scoped), so building this surfaced and closed a pre-existing gap rather than opening a new one. `ANTHROPIC_*` is deliberately NOT in claude's `allowedPrefixes` at all — Anthropic-traffic redirection can only happen through this feature's own admin-configured, SSRF-guarded route, never a plain client-supplied `envOverrides`. diff --git a/docs/wiki/Custom-Model-Endpoints.md b/docs/wiki/Custom-Model-Endpoints.md index 6ec5bac9..8f927463 100644 --- a/docs/wiki/Custom-Model-Endpoints.md +++ b/docs/wiki/Custom-Model-Endpoints.md @@ -60,11 +60,14 @@ hundreds of models stays usable. The endpoint's default model, if set, is marked but not auto-picked, so a launch can deliberately use a different one without changing the saved default. The list is not raw discovery order either: the model llama-swap reports loaded and ready is moved to the top and tagged -**Currently loaded**, and when nothing is loaded, the model you last launched on this harness -and endpoint pair is moved up instead and tagged **Last used** (a per-device browser value, so -another device starts from its own history). The default model keeps its own **Default** pill -in both cases, and nothing is ever auto-chosen: the promoted row is simply the one under your -thumb. +**Currently loaded**. When nothing is reported loaded, the model one of your own running +sessions is using on that endpoint is moved up instead and tagged **In use by a session** (it +beats **Last used**, never **Currently loaded**), and failing that the model you last launched +on this harness and endpoint pair is moved up and tagged **Last used** (a per-device browser +value, so another device starts from its own history). With none of those, a line under the +search box says nothing is marked, so the plain list is not mistaken for an answer. The default +model keeps its own **Default** pill in every case, and nothing is ever auto-chosen: the +promoted row is simply the one under your thumb. **For opencode, Codex, Gemini, Pi, Grok, DeepSeek, OMP and GitHub Copilot, picking an entry launches straight onto the endpoint** — no restart, because the endpoint is applied before the diff --git a/src/web/public/index.html b/src/web/public/index.html index 26eca72a..5e0d4741 100644 --- a/src/web/public/index.html +++ b/src/web/public/index.html @@ -1034,16 +1034,17 @@

+ a display layer only, the rows themselves are never re-rendered. Reset on every + open and focused only on non-touch devices, same in-memory rule as the rail's + search box. --> -
diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index ba483cb9..1314f094 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -1548,6 +1548,30 @@ export function registerSessionRoutes( const contextLength = resolveCustomModelContextLength(endpoint, body.modelId); const slotCount = endpoint.modelSlotCounts?.[body.modelId]; + // A CLI whose config alone cannot select the model also gets its `model` launch param + // forced (pi/omp `custom/`, grok's block name, and codex now carries its bare + // selected id too). The argv engine DROPS a token that + // fails its pattern rather than quoting it, which would silently launch the CLI on its + // own default provider again, so refuse an id the pattern cannot carry up front. + // The check runs on the PURE build result, before applyCustomModelInjection below: + // the forced launch id only comes out of that build, and writing first would mean a + // refused id already replaced the config dir a live codex session uses as CODEX_HOME. + // It also runs before the two confirmations, so nobody agrees to evict a model only + // to be told the id was never usable. + const modelSpec = entry.launch.params.model; + const injection = buildCustomModelInjection(entry, endpoint, body.modelId, contextLength); + const forcedLaunchModel = injection.kind === 'unsupported' ? undefined : injection.launchModel; + if ( + forcedLaunchModel !== undefined && + modelSpec?.type === 'token' && + !matchesPattern(modelSpec.pattern, forcedLaunchModel) + ) { + return createErrorResponse( + ApiErrorCode.INVALID_INPUT, + `Model id ${JSON.stringify(body.modelId)} cannot be passed to ${session.mode} on its command line` + ); + } + // Some CLIs (today: only claude) carry enough of their own fixed system-prompt/tool- // schema overhead that a small enough real context guarantees a first-message failure // no matter what CLAUDE_CODE_MAX_CONTEXT_TOKENS says — confirmed live at ~36.4K tokens @@ -1611,29 +1635,6 @@ export function registerSessionRoutes( } } - // A CLI whose config alone cannot select the model also gets its `model` launch param - // forced (pi/omp `custom/`, grok's block name, and codex now carries its bare - // selected id too). The argv engine DROPS a token that - // fails its pattern rather than quoting it, which would silently launch the CLI on its - // own default provider again, so refuse an id the pattern cannot carry up front. - // The check runs on the PURE build result, before applyCustomModelInjection below: - // the forced launch id only comes out of that build, and writing first would mean a - // refused id already replaced the config dir a live codex session uses as CODEX_HOME. - const modelSpec = entry.launch.params.model; - const injection = buildCustomModelInjection(entry, endpoint, body.modelId, contextLength); - if (injection.kind === 'unsupported') { - return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `${session.mode} has no known custom-model mechanism`); - } - if ( - injection.launchModel !== undefined && - modelSpec?.type === 'token' && - !matchesPattern(modelSpec.pattern, injection.launchModel) - ) { - return createErrorResponse( - ApiErrorCode.INVALID_INPUT, - `Model id ${JSON.stringify(body.modelId)} cannot be passed to ${session.mode} on its command line` - ); - } const applied = applyCustomModelInjection( entry, endpoint, diff --git a/test/routes/session-custom-model.test.ts b/test/routes/session-custom-model.test.ts index 71c6c4a8..686b95db 100644 --- a/test/routes/session-custom-model.test.ts +++ b/test/routes/session-custom-model.test.ts @@ -339,6 +339,28 @@ describe('POST /api/sessions/:id/custom-model', () => { expect(session.restartCli).not.toHaveBeenCalled(); }); + it('refuses a model id the CLI cannot launch before asking anyone to evict a model', async () => { + const { app, ctx } = await setup(); + const session = ctx.sessions.get('test-session-1')!; + session.mode = 'codex'; + const other = createMockSession('other-session'); + other.name = 'w2-otherbox'; + other.customModel = { endpointId: 'ep1', modelId: 'llama3' }; + ctx.sessions.set('other-session', other); + mockRunning([{ model: 'llama3', state: 'ready' }]); + + const res = await app.inject({ + method: 'POST', + url: '/api/sessions/test-session-1/custom-model', + payload: { endpointId: 'ep1', modelId: 'org/model@q4_k_m' }, + }); + + // Agreeing to the swap first would only lead to this same 400. + expect(res.json().requiresConfirmation).toBeUndefined(); + expect(res.json().errorCode).toBe('INVALID_INPUT'); + expect(session.setCustomModel).not.toHaveBeenCalled(); + }); + describe('multi-user: the confirm dialog must not name a session the caller cannot access', () => { const saved: Record = {};