From 1b8ac40baff35cacc35b98439fa6087c1776e9bf Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Sun, 1 Nov 2026 21:26:54 +0800 Subject: [PATCH] fix(custom-model): core review fixes for context-length discovery and injection - share one context-length resolution (discovered figure, endpoint default fallback) across the routes and boot recovery, with a recovery test - record listed per-slot context figures with a slot count of 1 and force the slot-count fallback once per rediscovery - open up the codex model launch token for qwen3-style ids (model-plain-colon), pinned by a spawn-golden case - pass the full discovered window to opencode as limit.context - translate the picker search strings and document the discovery precedence --- docs/api-reference.md | 27 +++++- docs/architecture-invariants.md | 2 +- docs/custom-model-endpoints.md | 28 ++++-- src/config/cli-registry/patterns.ts | 5 + src/config/cli-registry/stock.ts | 9 +- src/config/cli-registry/types.ts | 20 ++-- src/custom-model-hosts.ts | 11 +++ src/custom-model-injection.ts | 11 ++- src/web/public/i18n.js | 3 + src/web/routes/custom-model-routes.ts | 21 ++++- src/web/routes/session-routes.ts | 6 +- src/web/server.ts | 4 +- test/cli-registry-spawn-golden.test.ts | 9 ++ .../custom-model-endpoint-rediscovery.test.ts | 46 ++++++++++ test/custom-model-injection.test.ts | 10 +- test/custom-model-recovery.test.ts | 91 +++++++++++++++++++ 16 files changed, 256 insertions(+), 47 deletions(-) create mode 100644 test/custom-model-recovery.test.ts diff --git a/docs/api-reference.md b/docs/api-reference.md index 4a0cb6b7..1a7e8844 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1205,13 +1205,18 @@ data}` envelope on the wire — unwrap it the same way). Answers `[]` for a boolean` reports whether one is stored, so a client can render "unchanged if left blank" without ever holding the real value. - `POST /api/v1/model-endpoints` with `{ id, label, baseUrl, apiKey?, -authStyle?, defaultModelId? }` creates one. `id` must match +authStyle?, defaultModelId?, defaultContextLength? }` creates one. `id` + must match `^[a-zA-Z0-9_-]+$`; `authStyle` is `bearer` (default) or `api-key`, never both (a real server hung indefinitely when sent both headers on one - request); `baseUrl` must be `http(s)`, carry no embedded credentials, and + request); `defaultContextLength` is a user-set fallback used when + assigning a model that discovery has no context length for (discovery + itself never writes it); `baseUrl` must be `http(s)`, carry no embedded + credentials, and is refused if it points at (or resolves to) a link-local or cloud-metadata address. `409 ALREADY_EXISTS` on a duplicate id. -- `PUT /api/v1/model-endpoints/:id` updates one. An **absent** `apiKey` +- `PUT /api/v1/model-endpoints/:id` updates one, also accepting + `defaultContextLength`. An **absent** `apiKey` keeps the stored one rather than clearing it — the client never receives the real value to resend deliberately unchanged, so omission is the only way to say "leave it alone"; there is no way to clear a key back to unset @@ -1221,8 +1226,20 @@ authStyle?, defaultModelId? }` creates one. `id` must match - `POST /api/v1/model-endpoints/:id/discover-models` fetches the endpoint's own `GET /v1/models` and stores the result as `models`, updating `lastDiscoveredAt`, plus (best-effort, only for a model llama-swap's own - response already reports loaded) `modelContextLengths` and `modelSizesGB`. - A `defaultModelId` that no longer appears in the fresh list is dropped + response already reports loaded) `modelContextLengths`, + `modelSlotCounts` and `modelSizesGB`. A loaded model's context figure + comes from llama-swap's own launch command (`--fit-ctx` / `-c` / + `--ctx-size`), falling back to `/props` (`n_ctx`, `total_slots`) for + whatever the command did not answer; the model's own listing + (`context_length`, then `context_window`) only ever fills the gap when + neither probe does, and is recorded as a single-slot figure — + llama-swap lists the PER-SLOT window, so whenever the listed figure is + the one taken the stored slot count is 1. A server with no `status` + field on any entry (plain llama.cpp, a non-llama-swap gateway) gets no + context/size enrichment at all. `modelContextLengths` falls back to the + endpoint's `defaultContextLength` when a session is assigned a model + discovery never learned a window for. A `defaultModelId` that no longer + appears in the fresh list is dropped rather than carried forward invalid. Failures answer `422 OPERATION_FAILED` with the underlying connection error, or a named egress refusal if the resolved address turned out to be blocked. The same refresh also runs diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index ebba7973..4b5413ff 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -628,7 +628,7 @@ Adding the header changed the preamble, so `CODEMAN_PREAMBLE` was bumped (1.22.0 **Custom Model Endpoint Profiles** (opt-in, `customModelEndpointsEnabled`, SYNCED, default OFF; `docs/custom-model-endpoints.md`, design doc `docs/custom-model-endpoints-plan.md`; full stack — settings-panel CRUD + the Run-menu picker, on top of the backend below): points a session at a user-configured custom OpenAI-compatible endpoint — local (llama.cpp, DGX Spark, Strix Halo) or cloud (Azure AI Foundry, OpenRouter) — instead of its harness's native cloud backend. Endpoints are a read/write-array store (`custom-model-hosts.ts`, `~/.codeman/custom-model-hosts.json`) discovered via `GET /v1/models`; `CustomModelHost.authStyle` is `'bearer'` (default, `Authorization: Bearer`) or `'api-key'` (Azure's convention) — **never both**, live-tested against a real server: sending both headers on one request reliably hangs it indefinitely, reproduced 3×. -⚠️ The actual per-CLI redirect is `capabilities.customModelInjection` on the CLI registry (four kinds: `env` for claude/gemini/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. +⚠️ 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): 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. diff --git a/docs/custom-model-endpoints.md b/docs/custom-model-endpoints.md index f76c21a1..7ae21222 100644 --- a/docs/custom-model-endpoints.md +++ b/docs/custom-model-endpoints.md @@ -67,15 +67,23 @@ configured, `PUT`/`DELETE /api/model-endpoints/:id` update or remove one. Endpoint management is admin-only in multi-user mode, same as remote/docker hosts — these are machine-level infra, not per-user settings. -**Context length is discovered too, opportunistically and safely.** The plain -`GET /v1/models` response has no context-window field. Discovery only ever -looks for one for a model llama-swap's own response already reports +**Context length is discovered too, opportunistically and safely.** Older +servers state no context-window field in the plain `GET /v1/models` +response at all; current llama-swap states one per model (both +`context_length` and `context_window`) and gateways like Bifrost pass +`context_window` through — that listed figure is the *lowest*-precedence +source, used only when neither probe below answers, and because llama-swap +documents it as the PER-SLOT window, a model whose figure comes from the +listing is recorded with a slot count of 1 so the slot check never divides a +per-slot figure a second time. The probes are only ever looked for on a model +llama-swap's own response already reports `status.value === "loaded"` for — never for an unloaded one, because llama-swap treats `?model=` as a routing hint and asking about a model that isn't loaded risks triggering an actual (slow, GPU-swapping) load as a side effect of what should be read-only discovery. A server with no `status` field -on any entry at all (not llama-swap) gets no context-length enrichment, -rather than guessing. A model's previously-learned context length survives a +on any entry at all (not llama-swap) gets no launch-command/`/props` +enrichment, only whatever its listings state — rather than guessing. A +model's previously-learned context length survives a later cycle where it wasn't the loaded one; it's dropped only once the model disappears from the endpoint's list entirely. Stored per model in `modelContextLengths` and applied automatically (see "Applying a model to a @@ -293,14 +301,16 @@ agent, so the selection would report success and change nothing. **Claude gets two more env vars when known/applicable, both declared on its registry entry (`contextLengthVar`/`configDirVar`), not hardcoded here:** -- `CLAUDE_CODE_MAX_CONTEXT_TOKENS` is set to `modelId`'s discovered context - length (see the discovery section above) whenever one is known. Without +- `CLAUDE_CODE_MAX_CONTEXT_TOKENS` is set to `modelId`'s context length — + the discovered figure (see the discovery section above), falling back to + the endpoint's user-set `defaultContextLength` — whenever one is known. + Without it, Claude Code assumes a large (200k) window for any unrecognized custom model id and never compacts, which reliably overflows a much smaller real local context — confirmed live: a stock ~33.7K-token system prompt against a 16384-token llama-swap model failed with `exceeds the available context -size`. No entry for the model in `modelContextLengths` means the var is - simply omitted, never a guess. ⚠️ **This var only affects when Claude +size`. Neither a `modelContextLengths` entry nor a `defaultContextLength` + for the model means the var is simply omitted, never a guess. ⚠️ **This var only affects when Claude Code compacts conversation _history_ — it cannot fix a model whose real context is smaller than Claude Code's own fixed per-turn overhead** (system prompt + tool schemas, empirically ~36.4K tokens, confirmed live diff --git a/src/config/cli-registry/patterns.ts b/src/config/cli-registry/patterns.ts index 9cd8f303..c73381ee 100644 --- a/src/config/cli-registry/patterns.ts +++ b/src/config/cli-registry/patterns.ts @@ -21,6 +21,7 @@ export type TokenPattern = | 'model' | 'model-claude' | 'model-plain' + | 'model-plain-colon' | 'model-pi' | 'id' | 'id-dotted' @@ -47,6 +48,10 @@ const PATTERNS: Record = { // copilot --model. Same characters as `model`, but a leading alphanumeric: the value follows a // flag, so `--yolo` must not pass as a model id and become its own flag (the allowAll clamp bypass). 'model-plain': /^[a-zA-Z0-9][a-zA-Z0-9._\-/]*$/, + // codex --model, and the value a custom-model `launchModel` is forced onto it. Same + // shape as `model-plain` (leading alphanumeric, so a flag cannot ride the flag) plus + // `:` — Ollama-style ids like `qwen3:32b` must survive onto the command line. + 'model-plain-colon': /^[a-zA-Z0-9][a-zA-Z0-9._\-/:]*$/, // buildPiCommand — `:` for a thinking suffix (`sonnet:high`), `/` for `provider/id` 'model-pi': /^[a-zA-Z0-9._\-/:]+$/, // opencode --session, codex resume diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 4ac5b29d..9c674cb0 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -628,7 +628,10 @@ const CODEX: CliEntry = { params: { bypassApprovals: { type: 'bool' }, animations: { type: 'bool' }, - model: { type: 'token', pattern: 'model' }, + // `model-plain-colon`, not `model`: the custom-model feature forces the selected id + // onto this param (`launchModel: '{modelId}'` below), and Ollama-style ids carry a + // `:` (`qwen3:32b`). A leading alphanumeric still keeps `--yolo` from riding a flag. + model: { type: 'token', pattern: 'model-plain-colon' }, reasoningEffort: { type: 'enum', values: [...CODEX_REASONING_EFFORTS] }, resumeId: { type: 'token', pattern: 'id' }, }, @@ -1922,8 +1925,8 @@ const COPILOT: CliEntry = { // custom provider and drops the GitHub sign-in requirement; COPILOT_PROVIDER_TYPE defaults to // "openai", which is what a Codeman custom endpoint is (llama.cpp, vLLM, Ollama). // COPILOT_MODEL names the model and COPILOT_PROVIDER_MODEL_ID / _WIRE_MODEL default to it. - // The OpenAI-style route is `/v1/chat/completions`, so the base gets the `/v1` suffix - // the same way deepseek's does. All four already match the COPILOT_ prefix allowlist, so + // The OpenAI-style route is `/v1/chat/completions`, so the base gets the `/v1` + // suffix via appendV1Suffix below. All four already match the COPILOT_ prefix allowlist, so // they are privileged env keys below: a non-granted multi-user owner must not be able to // redirect the session's endpoint or credentials through plain envOverrides. customModelInjection: { diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index 6707d4aa..db460813 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -856,19 +856,13 @@ export interface CliCapabilities { * reuses that file. * * `appendV1Suffix` (env kind only): the raw `endpoint.baseUrl` gets `withV1Suffix()` - * applied before being written to `baseUrlVar`, instead of being used verbatim. - * DeepSeek needs this and claude/gemini must NOT get it — a per-CLI asymmetry confirmed - * by reading each SDK's own request-building source, not assumed: DeepSeek Harness's - * bundled `@deepseek-ai/dsh-llm-deepseek` concatenates `${connection.baseURL}/chat/ - * completions` with no `/v1` insertion of its own (its real public API base, - * `https://api.deepseek.com`, expects the caller's base URL to already carry any - * needed prefix), while llama-swap/llama.cpp only ever serves the OpenAI-conventional - * `/v1/chat/completions` — confirmed live: a bare `POST /chat/completions` - * 404s, `POST /v1/chat/completions` succeeds, and the harness's own error - * message template (`DeepSeek API error (HTTP ${status})`) reproduces the exact - * `HTTP_404` this feature originally shipped with unexplained. Claude Code's own SDK, - * by contrast, was already confirmed working end-to-end against the RAW `baseUrl` with - * no suffix — appending one there would be wrong, not just redundant. + * applied before being written to `baseUrlVar`, instead of being used verbatim. Only + * deepseek and copilot declare it — both speak to `/v1/chat/completions` and a + * gateway saves the bare root. claude/gemini must NOT get it: Claude Code's own SDK was + * confirmed working end-to-end against the RAW `baseUrl`, so appending a suffix there + * would be wrong, not just redundant. The per-session config-file recipes do not need + * this flag at all — `renderConfigFile()` applies the suffix inside every template, so + * no configDir entry carries a raw base URL. */ customModelInjection: | { diff --git a/src/custom-model-hosts.ts b/src/custom-model-hosts.ts index 46dc7271..e4b7176e 100644 --- a/src/custom-model-hosts.ts +++ b/src/custom-model-hosts.ts @@ -102,6 +102,17 @@ export interface CustomModelHost { defaultContextLength?: number; } +/** + * The context window to apply for `modelId` on `host`: the discovered per-model figure when + * discovery has one, else the endpoint's user-set `defaultContextLength`, else undefined + * (no override applied, never a guess). ONE helper for every read site — the swap route, + * the quickstart route and boot recovery each re-write the CLI's config files, and a site + * that forgot the fallback silently loses the endpoint default on the next respawn. + */ +export function resolveCustomModelContextLength(host: CustomModelHost, modelId: string): number | undefined { + return host.modelContextLengths?.[modelId] ?? host.defaultContextLength; +} + export function customModelHostsPath(configDir: string): string { return join(configDir, CUSTOM_MODEL_HOSTS_FILE); } diff --git a/src/custom-model-injection.ts b/src/custom-model-injection.ts index 83613878..d80c54a4 100644 --- a/src/custom-model-injection.ts +++ b/src/custom-model-injection.ts @@ -175,7 +175,9 @@ function opencodeModelLimit(contextLength: number | undefined): { context: numbe const total = Math.trunc(contextLength); if (total <= 0) return undefined; const output = Math.max(1, Math.min(8192, Math.floor(total / 4))); - return { context: total - output, output }; + // `limit.context` is the FULL window as far as opencode is concerned — it reserves for + // the reply itself — so the discovered total is passed through, never a shrunk figure. + return { context: total, output }; } function renderConfigContent( @@ -189,10 +191,9 @@ function renderConfigContent( case 'opencode-json': { // `limit` carries the discovered context window (issue #749): without it opencode assumes // a large default window for an unrecognized model id and eventually overflows a much - // smaller local server. `context` is what opencode caps the PROMPT at, so the response - // headroom from `opencodeModelLimit` is subtracted up front — `output` is opencode's own - // cap on the reply, so setting them from the same discovered total keeps a single turn - // inside the real window. + // smaller local server. `context` is the FULL window and opencode reserves for the reply + // inside it itself, so `opencodeModelLimit` passes the real total through and only + // `output` gets the capped reservation. const limit = opencodeModelLimit(contextLength); return JSON.stringify({ $schema: 'https://opencode.ai/config.json', diff --git a/src/web/public/i18n.js b/src/web/public/i18n.js index dceab1ac..7525ebdd 100644 --- a/src/web/public/i18n.js +++ b/src/web/public/i18n.js @@ -630,6 +630,9 @@ 'Last used': '上次使用', 'That endpoint no longer exists': '该端点已不存在', 'No models discovered for this endpoint yet': '此端点尚未发现任何模型', + // Model picker search box (index.html placeholder/aria-label and the no-matches line) + 'Search models': '搜索模型', + 'No models match': '没有匹配的模型', 'Subagent Options': '子智能体选项', 'Enable Tracking': '启用跟踪', 'Active Tab Only': '仅活动标签页', diff --git a/src/web/routes/custom-model-routes.ts b/src/web/routes/custom-model-routes.ts index 5cf24df5..a48368de 100644 --- a/src/web/routes/custom-model-routes.ts +++ b/src/web/routes/custom-model-routes.ts @@ -371,6 +371,9 @@ async function discoverModels( // itself calls loaded. Listed figures above are unaffected: reading them triggers nothing. const hasStatusField = entries.some((m) => m && typeof m === 'object' && 'status' in m); const slotCounts: Record = {}; + // Ids whose figure THIS round's loaded-model probe answered — the listed figure remains + // the source only for the ids NOT in this set (see the per-slot correction below). + const ctxFromProbe = new Set(); if (hasStatusField) { const loadedIds = entries .filter((m) => m.status && typeof m.status === 'object' && (m.status as { value?: unknown }).value === 'loaded') @@ -407,11 +410,27 @@ async function discoverModels( // Assigning only when a probe answered keeps the listed figure (if any) when // neither the launch command nor /props stated one, and replaces it with the // stronger source whenever they did: cmd > /props > listed, never the reverse. - if (ctx !== undefined) contextLengths[id] = ctx; + if (ctx !== undefined) { + contextLengths[id] = ctx; + ctxFromProbe.add(id); + } if (slots !== undefined) slotCounts[id] = slots; } } } + // llama-swap documents the LISTED `context_length` as the PER-SLOT window, so whenever + // the listed figure is the one taken this round — the model was not loaded, or the + // probe answered nothing — its slot count is 1 by definition: the floor check must NOT + // divide a per-slot figure a second time. Without recording 1 here, the count a loaded + // round stored survives the merge (applyDiscoveredModels keeps what a later round does + // not overwrite): 262144 @ 4 slots stored, model unloaded, listed 65536 → 65536 with 4 + // slots → a false 16384-per-slot warning. Even a count this round's launch command + // parsed must yield — pairing a per-slot listed figure with the cmd's --parallel is the + // same double division. Same rule the /props branch above applies. Ids whose figure the + // probe DID answer keep their probe-derived count untouched. + for (const id of Object.keys(contextLengths)) { + if (!ctxFromProbe.has(id)) slotCounts[id] = 1; + } return { models, contextLengths, slotCounts, sizesGB }; } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 424de49b..3628a53a 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -70,7 +70,7 @@ import { SessionWaitOutputQuerySchema, CustomModelSelectionSchema, } from '../schemas.js'; -import { readCustomModelHosts } from '../../custom-model-hosts.js'; +import { readCustomModelHosts, resolveCustomModelContextLength } from '../../custom-model-hosts.js'; import { applyCustomModelInjection, removeConfigDir } from '../../custom-model-injection-apply.js'; import { getLlamaSwapStatus, @@ -1544,7 +1544,7 @@ export function registerSessionRoutes( // Discovery's per-model figure wins; the endpoint's user-set default covers models // whose size discovery could not determine (a gateway that states none, say). With // neither, the existing "no override, CLI assumes its own default" behaviour stands. - const contextLength = endpoint.modelContextLengths?.[body.modelId] ?? endpoint.defaultContextLength; + const contextLength = resolveCustomModelContextLength(endpoint, body.modelId); const slotCount = endpoint.modelSlotCounts?.[body.modelId]; // Some CLIs (today: only claude) carry enough of their own fixed system-prompt/tool- @@ -4452,7 +4452,7 @@ export function registerSessionRoutes( if (!cmEndpoint) return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Model endpoint not found'); // Per-model discovery figure first, user-set endpoint default second, nothing third // (see the dedicated route's own comment above). - const cmContextLength = cmEndpoint.modelContextLengths?.[customModel.modelId] ?? cmEndpoint.defaultContextLength; + const cmContextLength = resolveCustomModelContextLength(cmEndpoint, customModel.modelId); const cmSlotCount = cmEndpoint.modelSlotCounts?.[customModel.modelId]; // See the dedicated route's own comment for the full reasoning: some CLIs' own fixed diff --git a/src/web/server.ts b/src/web/server.ts index ddfc296d..172bc9f8 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -80,7 +80,7 @@ import { attachmentRegistry, buildFileThumbnailRoute, registerExternalAttachment import { folderGrants } from '../folder-listing.js'; import { getCli, enabledClis, listClis } from '../config/cli-registry/registry.js'; import { isCliEntryInstalled, probeStockCliAvailability } from '../utils/cli-installed-probes.js'; -import { readCustomModelHosts } from '../custom-model-hosts.js'; +import { readCustomModelHosts, resolveCustomModelContextLength } from '../custom-model-hosts.js'; import { applyCustomModelInjection, customModelConfigDir, removeConfigDir } from '../custom-model-injection-apply.js'; import type { CustomModelBookkeeping } from '../types/session.js'; import { registerGeneratedArtifactAttachment } from '../generated-artifact-attachments.js'; @@ -1470,7 +1470,7 @@ export class WebServer extends EventEmitter { endpoint, saved.modelId, session.id, - endpoint.modelContextLengths?.[saved.modelId] + resolveCustomModelContextLength(endpoint, saved.modelId) )?.envOverrides; } catch (err) { console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err); diff --git a/test/cli-registry-spawn-golden.test.ts b/test/cli-registry-spawn-golden.test.ts index c4fe6032..464fe6fd 100644 --- a/test/cli-registry-spawn-golden.test.ts +++ b/test/cli-registry-spawn-golden.test.ts @@ -171,6 +171,15 @@ describe('codex', () => { expect(cx({ model: 'gpt-5', resumeSessionId: 'roll_42' })).toBe('codex --model gpt-5 resume roll_42'); }); + it('carries an Ollama-style `qwen3:32b` model id, and still drops a leading-flag shape', () => { + // `model-plain-colon`: the custom-model feature forces the selected id onto this param, + // and ids like `qwen3:32b` carry a colon. The leading-alphanumeric rule is what keeps a + // flag from riding the flag, so `:x` and `-x` must still drop out. + expect(cx({ model: 'qwen3:32b' })).toBe('codex --model qwen3:32b'); + expect(cx({ model: ':leading' })).toBe('codex'); + expect(cx({ model: '-yolo' })).toBe('codex'); + }); + it('sends reasoning effort as one model_reasoning_effort config value, for every level', () => { for (const level of CODEX_REASONING_EFFORTS) { expect(cx({ reasoningEffort: level })).toBe(`codex --config model_reasoning_effort=${level}`); diff --git a/test/custom-model-endpoint-rediscovery.test.ts b/test/custom-model-endpoint-rediscovery.test.ts index fd60f845..d7ab8f68 100644 --- a/test/custom-model-endpoint-rediscovery.test.ts +++ b/test/custom-model-endpoint-rediscovery.test.ts @@ -481,6 +481,52 @@ describe('refreshAllCustomModelHosts: parallel-slot enrichment (llama.cpp splits const [updated] = await readCustomModelHosts(dir); expect(updated.modelSlotCounts).toEqual({ a: 4 }); }); + + it('a round where only the listed figure answers resets the slot count a loaded round learned', async () => { + // The false-warning scenario, measured on a real llama-swap: round 1 has the model + // loaded under `--ctx-size 262144 --parallel 4` — a TOTAL window shared across slots, + // correctly stored with its count. Round 2 finds it unloaded and takes the LISTED + // `context_length`, which llama-swap documents as the PER-SLOT window. If the stored 4 + // survived, the floor check would divide the per-slot figure a second time + // (65536/4 = 16384) and warn about a model that serves 65536 to every request. + const dir = getDataDir(); + await writeCustomModelHosts(dir, [host({ id: 'ep', baseUrl: 'http://localhost:8080' })]); + fetchMock.mockImplementation(async (url: URL) => { + if (url.pathname === '/v1/models') { + return new Response(JSON.stringify({ data: [{ id: 'm', status: { value: 'loaded' } }] }), { status: 200 }); + } + if (url.pathname === '/running') { + return new Response( + JSON.stringify({ + running: [{ model: 'm', state: 'ready', cmd: 'llama-server -m /models/m.gguf --ctx-size 262144 --parallel 4' }], + }), + { status: 200 } + ); + } + throw new Error(`unexpected request: ${url.href}`); // the command answers both questions + }); + + await refreshAllCustomModelHosts(); + + let [round1] = await readCustomModelHosts(dir); + expect(round1.modelContextLengths).toEqual({ m: 262144 }); + expect(round1.modelSlotCounts).toEqual({ m: 4 }); + + // Round 2: unloaded, no /running or /props probe in order, listing its per-slot figure. + fetchMock.mockResolvedValue( + new Response(JSON.stringify({ data: [{ id: 'm', status: { value: 'unloaded' }, context_length: 65536 }] }), { + status: 200, + }) + ); + + await refreshAllCustomModelHosts(); + + const [round2] = await readCustomModelHosts(dir); + expect(round2.modelContextLengths).toEqual({ m: 65536 }); + // 1 slot, not the stale 4: 65536 per slot with 1 recorded slot is the honest pair, so + // the floor check reads 65536/request — exactly what the server gives a single request. + expect(round2.modelSlotCounts).toEqual({ m: 1 }); + }); }); describe('refreshAllCustomModelHosts: model-size enrichment (parsed from /v1/models description)', () => { diff --git a/test/custom-model-injection.test.ts b/test/custom-model-injection.test.ts index 19c524ce..a407ba53 100644 --- a/test/custom-model-injection.test.ts +++ b/test/custom-model-injection.test.ts @@ -112,20 +112,20 @@ describe('buildCustomModelInjection', () => { expect(parsed.provider.custom.models.qwen3).toEqual({}); }); - it('opencode: threads a discovered context length into the model limit with response headroom (issue #749)', () => { + it('opencode: threads a discovered context length into the model limit as the full window (issue #749)', () => { const result = buildCustomModelInjection(entryOrThrow('opencode'), endpoint, 'qwen3', 16384); if (result.kind !== 'env') throw new Error('unreachable'); const parsed = JSON.parse(result.envOverrides.OPENCODE_CONFIG_CONTENT); - // One quarter held back for the reply: context 16384-4096, output 4096 — the sum is the - // real window, so a full-size prompt plus a max-length reply never exceeds the server. - expect(parsed.provider.custom.models.qwen3).toEqual({ limit: { context: 12288, output: 4096 } }); + // `limit.context` IS the full window to opencode — it reserves for the reply itself — + // so the discovered total passes through; `output` only caps the reply at a quarter. + expect(parsed.provider.custom.models.qwen3).toEqual({ limit: { context: 16384, output: 4096 } }); }); it('opencode: caps the response headroom reservation at 8192 for big windows', () => { const result = buildCustomModelInjection(entryOrThrow('opencode'), endpoint, 'qwen3', 262144); if (result.kind !== 'env') throw new Error('unreachable'); const parsed = JSON.parse(result.envOverrides.OPENCODE_CONFIG_CONTENT); - expect(parsed.provider.custom.models.qwen3).toEqual({ limit: { context: 253952, output: 8192 } }); + expect(parsed.provider.custom.models.qwen3).toEqual({ limit: { context: 262144, output: 8192 } }); }); it('opencode: never invents a limit for an unknown or non-finite context length (issue #749 regression)', () => { diff --git a/test/custom-model-recovery.test.ts b/test/custom-model-recovery.test.ts new file mode 100644 index 00000000..5ef64a18 --- /dev/null +++ b/test/custom-model-recovery.test.ts @@ -0,0 +1,91 @@ +/** + * @fileoverview Boot recovery rebuilds a saved custom-model selection through the SAME + * context-length resolution the routes use: the model's discovered figure, falling back to + * the endpoint's `defaultContextLength` (`resolveCustomModelContextLength`). Before the + * shared helper, `_rebuildCustomModelEnv` read `modelContextLengths` alone — so a session + * on a model that only the endpoint default covers came back from a Codeman restart with + * `CLAUDE_CODE_MAX_CONTEXT_TOKENS` (or a config file's context figure) silently gone, + * re-arming exactly the window overflow the var exists to prevent. + * + * `_rebuildCustomModelEnv` reads no `this` state (it re-derives everything from the endpoint + * store and the Session it is handed), so it is driven straight off the prototype with an + * empty receiver — `restoreMuxSessions()` itself cannot be reached under vitest, the same + * constraint recorded in test/session-model-recovery.test.ts. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { getDataDir } from '../src/config/instance.js'; +import { writeCustomModelHosts, type CustomModelHost } from '../src/custom-model-hosts.js'; +import { customModelConfigDir, removeConfigDir } from '../src/custom-model-injection-apply.js'; +import { Session } from '../src/session.js'; +import { WebServer } from '../src/web/server.js'; +import type { CustomModelBookkeeping } from '../src/types/session.js'; + +type RebuildFn = (session: Session, saved: CustomModelBookkeeping) => Promise | undefined>; + +/** The real method, with an empty receiver — see the file header for why that is sound. */ +function rebuildCustomModelEnv(session: Session, saved: CustomModelBookkeeping) { + const fn = (WebServer.prototype as unknown as { _rebuildCustomModelEnv: RebuildFn })._rebuildCustomModelEnv; + expect(typeof fn, '_rebuildCustomModelEnv vanished from WebServer.prototype').toBe('function'); + return fn.call({}, session, saved); +} + +function endpoint(overrides: Partial = {}): CustomModelHost { + return { + id: 'ep', + label: 'Ep', + baseUrl: 'http://localhost:8080', + apiKey: 'recovery-probe-key', + models: ['qwen3'], + ...overrides, + }; +} + +describe('_rebuildCustomModelEnv: context-length fallback on boot recovery', () => { + const configs: string[] = []; + + afterEach(() => { + for (const id of configs.splice(0)) removeConfigDir(customModelConfigDir(id)); + }); + + it('falls back to the endpoint defaultContextLength when discovery recorded no per-model figure', async () => { + await writeCustomModelHosts(getDataDir(), [endpoint({ defaultContextLength: 32768 })]); + const session = new Session({ workingDir: '/tmp', mode: 'claude' }); + configs.push(session.id); + + const env = await rebuildCustomModelEnv(session, { endpointId: 'ep', modelId: 'qwen3', envKeys: [] }); + + expect(env?.CLAUDE_CODE_MAX_CONTEXT_TOKENS).toBe('32768'); + }); + + it('a discovered per-model figure still wins over the endpoint default', async () => { + await writeCustomModelHosts(getDataDir(), [ + endpoint({ modelContextLengths: { qwen3: 131072 }, defaultContextLength: 32768 }), + ]); + const session = new Session({ workingDir: '/tmp', mode: 'claude' }); + configs.push(session.id); + + const env = await rebuildCustomModelEnv(session, { endpointId: 'ep', modelId: 'qwen3', envKeys: [] }); + + expect(env?.CLAUDE_CODE_MAX_CONTEXT_TOKENS).toBe('131072'); + }); + + it('states nothing rather than guessing when neither source knows a window', async () => { + await writeCustomModelHosts(getDataDir(), [endpoint()]); + const session = new Session({ workingDir: '/tmp', mode: 'claude' }); + configs.push(session.id); + + const env = await rebuildCustomModelEnv(session, { endpointId: 'ep', modelId: 'qwen3', envKeys: [] }); + + expect(env?.CLAUDE_CODE_MAX_CONTEXT_TOKENS).toBeUndefined(); + }); + + it('drops the selection to undefined (kept for clearing) when the endpoint no longer exists', async () => { + await writeCustomModelHosts(getDataDir(), []); + const session = new Session({ workingDir: '/tmp', mode: 'claude' }); + configs.push(session.id); + + await expect(rebuildCustomModelEnv(session, { endpointId: 'gone', modelId: 'qwen3', envKeys: [] })).resolves.toBe( + undefined + ); + }); +});