diff --git a/devlog/_plan/260805_bug_fix_stack/000_plan.md b/devlog/_plan/260805_bug_fix_stack/000_plan.md new file mode 100644 index 000000000..93ef6d70b --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/000_plan.md @@ -0,0 +1,112 @@ +# 000 — Plan: bug-fix stack from the 260805 triage + +## Objective + +Fix the defects the 2026-08-05 triage proved at code level, one PR per defect, +each with a regression test that fails without the fix. + +Source of candidates: `devlog/_plan/260805_issue_pr_triage/010_issue_verdicts.md`. +That unit produced seventeen verdicts; five were real open defects with no owner. +Four of them are fixable here. The fifth (#1059, the Windows suite) needs a +Windows runner this machine does not have, and stays out. + +## Base + +| Fact | Value | +|------|-------| +| Base | `origin/dev` = `aaa71967a` | +| Branch | `codex/260805-bug-fix-stack` | +| Dirty files preserved | `src/usage/log.ts`, `tests/usage-log.test.ts` (user-owned, untouched) | + +## Layer map + +| Layer | Issue | Files | +|-------|-------|-------| +| 010 | #1057 DeepSeek ladder | `src/providers/registry.ts:349-360` **and `:1668-1669`**, `src/config.ts`, 3 test files | +| 020 | #1043 (+ live half of #1024) | `src/providers/registry.ts:1652` **and `:1671`**, `tests/vision-sidecar-e2e.test.ts` | +| 030 | #1061 test harness | `tests/native-profile-crash-boundaries.test.ts`, `tests/helpers/native-profile-startup-child.ts` | +| 040 | #1046 startup app-server | `src/codex/app-server-processes.ts`, `src/codex/desired-state.ts`, `src/server/index.ts` | + +### Stack shape: 010→020 stacked, 030 and 040 independent + +An earlier draft claimed 010 and 020 touch `registry.ts` "~1300 lines apart with +no semantic overlap" and then made all four layers a linear chain. The audit +found both halves wrong. + +**The overlap is adjacent, not distant.** 010 rewrites `opencode-free`'s +reasoning maps at `:1668-1669`; 020 rewrites that same provider object's +`noVisionModels` at `:1671`. Two lines apart, same literal. They genuinely need +ordering — just not for the reason the draft gave. + +**030 and 040 share nothing with anything.** Chaining them behind 020 would buy +nothing and cost two retargets after the parents land. They go straight to `dev` +as independent PRs, which `AGENTS.md` permits alongside stacked children. + +``` +dev ──┬── 010 (#1057) ── 020 (#1043) stacked: adjacent registry edits + ├── 030 (#1061) independent + └── 040 (#1046) independent +``` + +Order within the stack: 010 first, because 020's `noVisionModels` widening at +`:1671` reads more clearly once 010 has already reshaped the same object's +reasoning maps. + +## What the design research changed + +Three read-only `gpt-5.6-terra`/`sol` lanes and two `gpt-5.6-luna` search lanes ran +before any code was written. Two findings materially changed the plan, and both +would have produced a wrong patch if we had gone straight from the triage anchors +to an edit. + +**#1043: the reporter's own suggested fix is the wrong one to ship first.** The +issue proposes stripping images whenever `inputModalities` lacks `"image"`. The +control-flow lane found that modality metadata is *not reliably populated* — live +`GET /v1/models` returns `undefined` when the provider omits a recognized modality +field (`src/codex/catalog/provider-fetch.ts:719-743`), and live-discovered +modalities are never copied into the request-time provider config +(`src/router.ts:84-110`). So a modality-keyed fix would silently do nothing for +exactly the provider that motivated the issue. It also found a deliberate +regression guard asserting that unlisted models keep forwarding images +(`tests/vision-sidecar-e2e.test.ts:163-193`), which a default-on strip would +break. The narrow fix — classify the zen models explicitly — ships now; the +modality-driven default is a follow-up that needs the metadata to become canonical +first. + +**#1057: the shared mapping table may be wrong per model.** DeepSeek's official +thinking-mode docs give a native ladder of `low / high / max`, which matches the +reporter. But the same table maps requested `xhigh` differently per model — +`xhigh -> max` for `deepseek-v4-pro` and `xhigh -> high` for `deepseek-v4-flash`. +The code currently applies one shared map to both. A confirmation lane is running +against the official table before this layer is written; if the per-model +difference holds, the fix is not a one-line constant change. + +**#1046: the obvious fix is unsafe at boot.** The existing +`afterCatalogWriteHandleAppServers()` has a `restart: true` branch that SIGTERMs +long-lived app-servers and explicitly warns that active turns may be interrupted +(`src/codex/app-server-processes.ts:738-742`). Wiring that into unattended startup +would kill a user's in-flight turn on every service start. Only the warning path +is startup-safe. + +## Scope boundary + +**IN:** `src/providers/registry.ts`, `src/config.ts`, `src/codex/app-server-processes.ts`, +`src/codex/desired-state.ts`, the named test files, and this devlog unit. + +**OUT:** #1059 (needs a Windows runner); any change to the vision default for +unlisted models (follow-up, not this stack); process termination at startup; the +user's dirty `src/usage/log.ts` and `tests/usage-log.test.ts`; merging any PR; +closing any issue by hand. + +## Accept criteria, all layers + +1. The regression test fails on the pre-fix tree and passes after — ablation output recorded in the layer's decade doc. +2. `bun run typecheck` exits 0. +3. The affected test files pass. +4. No existing test is rewritten to accommodate the change unless that test was locking the defect itself, and the decade doc says which and why. +5. Each PR fills `.github/PULL_REQUEST_TEMPLATE.md` and links its issue. + +Criterion 4 is the one with history: `devlog/_plan/260804_overnight_triage/000_dispositions.md` +records a PR rejected for rewriting a regression contract to make a broader change +pass. Two layers here legitimately update tests (#1057's ladder assertions, #1061's +harness) — both are tests that encode the defect, and both are named in advance. diff --git a/devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md b/devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md new file mode 100644 index 000000000..d1b658b20 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md @@ -0,0 +1,92 @@ +# 001 — Upstream evidence gathered before writing any patch + +Two `gpt-5.6-luna` search lanes ran against primary vendor sources. Both changed +the plan, and one of them stopped a layer outright. + +## DeepSeek reasoning ladder (#1057) — CONFIRMED, and worse than reported + +Source: [api-docs.deepseek.com/guides/thinking_mode](https://api-docs.deepseek.com/guides/thinking_mode/), +observed 2026-08-06. The Chinese mirror agrees verbatim. + +| requested | `deepseek-v4-flash` | `deepseek-v4-pro` | +|---|---|---| +| `low` | `low` | `high` | +| `high` | `high` | `high` | +| `xhigh` | `high` | `max` | +| `max` | `max` | `max` | + +The page carries a footnote: *"We will update the actual mapped effort of +deepseek-v4-pro in early August 2026."* The Chinese page says the same. + +Three things follow, none of which were visible from the issue alone. + +**The two models do not share a mapping.** `xhigh` resolves to `high` on Flash and +`max` on Pro; `low` resolves to `low` on Flash and `high` on Pro. Our code applies +one shared `DEEPSEEK_THINKING_REASONING_MAP` to both +(`src/providers/registry.ts:353-360`). A single corrected constant would fix Pro +and break Flash, or the reverse. The fix has to split the map per model. + +**The reporter's requested mapping is right for Pro and wrong for Flash.** #1057 +asks for `low -> low`. That is Flash's documented behavior. On Pro, DeepSeek +itself maps `low -> high`, so advertising `low` as a native Pro tier would promise +a level the vendor does not honor. + +**The vendor is about to change it.** The footnote says Pro's mapping updates in +early August 2026 — which is now. Pinning Pro's map today means pinning a value +the vendor has announced it will move. That is a reason to be conservative about +Pro, not a reason to wait: the advertised *ladder* (`low/high/max`) is stable in +both columns; only Pro's internal resolution of `low` and `xhigh` is in flux. + +`medium` has no row in either table. Our map currently sends `medium -> high` +for both models. That is a local compatibility choice, not a documented vendor +behavior, and the decade doc must say so rather than implying the vendor blessed it. + +## OpenCode Zen free models (#1043) — NOT verifiable, layer blocked + +Source: [opencode.ai/docs/zen](https://opencode.ai/docs/zen), observed 2026-08-06. + +The official page lists eight free model IDs: + +``` +big-pickle mimo-v2.5-free laguna-s-2.1-free +ling-3.0-flash-free longcat-2.0-free north-mini-code-free +nemotron-3-ultra-free deepseek-v4-flash-free +``` + +It does **not** publish input modality for any of them. The lane checked the live +`/v1/models` endpoint, a community lesson page, and a third-party catalog; none +produced an authoritative per-ID modality. Its verdict was `unknown` for all +eight, with one community report that the free MiMo model refuses images — +suggestive, not sufficient. + +This blocks the narrow fix as designed. The narrow fix means adding zen model IDs +to `noVisionModels`, and a model on that list gets its images replaced with a +caption or an omission marker before the request goes upstream +(`src/vision/index.ts:447-471`). Guessing wrong in the text-only direction +silently degrades a working vision model — the exact failure the registry comments +warn about at `src/providers/registry.ts:542-554`. + +What is *not* blocked: `deepseek-v4-flash-free` is already classified text-only +through `OPENCODE_FREE_DEEPSEEK_MODELS`, and the sibling `opencode-free` provider +at `src/providers/registry.ts:1655-1671` already carries a `noVisionModels` list +against the same base URL. The registry's own DeepSeek modality table also states +`"deepseek-v4-flash": ["text"]` at `src/providers/registry.ts:470`. + +**Resolved by measurement.** Rather than ship the narrow-but-partial version, the +eight models were probed directly against the live endpoint. See `002` — six are +text-only, two accept images, and the reporter's exact error was reproduced on +`big-pickle`. The layer is no longer blocked and no ID is guessed. + +## What this changes in the layer map + +| Layer | Before this research | After | +|-------|---------------------|-------| +| #1057 | one-line constant change | per-model map split + config migration | +| #1043 | add zen IDs to `noVisionModels` | 6 measured text-only IDs; 2 measured vision-capable and deliberately excluded (`002`) | +| #1061 | unchanged | unchanged | +| #1046 | call the existing handler | warning-only variant; the `restart` branch is not startup-safe | + +The #1043 downgrade is the one worth stating plainly. The triage called it a real +open defect and it is; what the search lane established is that we cannot close it +correctly today without evidence nobody has published. Shipping a guess would +trade a loud 400 for a silent capability loss. diff --git a/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md b/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md new file mode 100644 index 000000000..fd25eda0e --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md @@ -0,0 +1,112 @@ +# 002 — Live modality probe of the OpenCode Zen free models + +The search lane could not classify these models: the official docs publish no +modality, and `GET /v1/models` returns only `id`, `object`, `created`, +`owned_by` — no capability field at all. That absence *is* the root cause of +#1043, so it could not also serve as its evidence. + +So the classification was measured instead of inferred. Probed 2026-08-05 +against `https://opencode.ai/zen/v1/chat/completions`, no credential (the public +desktop tier the `opencode-free` provider already uses), one 1x1 PNG data URL per +request. + +## Method + +```bash +IMG='{"type":"image_url","image_url":{"url":"data:image/png;base64,iVBORw0KGgoAAA...ErkJggg=="}}' +curl -s https://opencode.ai/zen/v1/chat/completions \ + -H "content-type: application/json" -H "x-opencode-client: desktop" \ + -d "{\"model\":\"$m\",\"max_tokens\":8,\"messages\":[{\"role\":\"user\", + \"content\":[{\"type\":\"text\",\"text\":\"what is in this image\"},$IMG]}]}" +``` + +A text-only control (`"content":"hi"`) was run first and returned `200` with a +completion, which is what makes a subsequent image failure attributable to the +image part rather than to auth, quota, or the endpoint being down. + +Rate limiting is real on this tier: a tight loop returned `Internal server error` +for every model, and the same requests succeeded with a 12-14 second gap. An +early batch run was discarded for exactly this reason — the uniform failure was +the limiter, not the models. + +## Result: 8 of 8 classified, no guesses + +| model | image request | verdict | +|---|---|---| +| `big-pickle` | `invalid_request_error … Failed to deserialize the JSON body into the target type` | **text-only** | +| `nemotron-3-ultra-free` | `[404] No endpoints found that support image input` | **text-only** | +| `ling-3.0-flash-free` | `[404] No endpoints found that support image input` | **text-only** | +| `north-mini-code-free` | `[404] No endpoints found that support image input` | **text-only** | +| `laguna-s-2.1-free` | `[404] No endpoints found that support image input` | **text-only** | +| `deepseek-v4-flash-free` | `[400] Model only supports text input; received unsupported content type 'image_url'` | **text-only** | +| `mimo-v2.5-free` | `200`, completion returned | **vision-capable** | +| `longcat-2.0-free` | `200`, completion returned | **vision-capable** | + +## Two findings that change the patch + +**`big-pickle` reproduces the reported error verbatim.** The issue quotes +`Failed to deserialize the JSON body into the target type: messages[65]: unknown +variant 'image_url'`. That is `big-pickle`'s exact failure shape, and it is the +*only* one of the eight that fails this way — the others return a clean 404 or +400. So the reporter was almost certainly on `big-pickle`, and the defect is +confirmed end to end rather than by analogy. + +**Two of the eight accept images, and a blanket classification would have broken +them.** `mimo-v2.5-free` and `longcat-2.0-free` both returned completions for an +image request. The search lane's honest `unknown` verdict, plus one community +report that "the free MiMo model refuses images", would have led straight to a +wrong entry: MiMo is exactly the model that *does* work. Listing it in +`noVisionModels` would silently replace a user's image with a caption on a model +that never needed it. + +This is why the probe was worth the ten minutes. The narrow fix is now +evidence-backed rather than blocked, and it covers six models instead of the two +the repository already knew about. + +## Caveat recorded honestly + +This is a point-in-time measurement of a live free tier, taken once per model. +Zen's catalog is discovered live (`liveModels: true` on the sibling +`opencode-free` entry), so the free roster can change under a static registry +list. The patch should therefore classify what is measured today and leave the +list easy to amend, not claim permanence. + +`#1024`'s remaining `TR` / `moonshotai/kimi-k3-free` half is untouched by this — +`TR` is not a built-in registry provider and depends on reporter configuration. + +## Follow-up: does this evidence transfer to the key-auth provider? + +The audit raised a real objection: the probe used `x-opencode-client: desktop`, +which is the *free* tier's header, while layer 020 modifies the **key-auth** +`opencode-zen` entry. Evidence from one access tier applied to another is exactly +the kind of reasoning that silently destroys a user's image. + +Re-probed 2026-08-05 with the header removed entirely: + +| request | result | +|---|---| +| `big-pickle` text, no header | `200`, completion returned | +| `big-pickle` + image, no header | `invalid_request_error … unknown variant 'image_url', expected 'text'` | +| `mimo-v2.5-free` + image, no header | `200`, reasoning begins *"The user has provided an image and…"* | +| `big-pickle` text, `authorization: Bearer sk-invalid-probe` | `AuthError: Invalid API key.` | + +Three things follow. + +**The desktop header was never what made the probe work.** The same models answer +with no header at all, so the modality behavior is a property of the model +routing, not of the free tier's client identity. + +**Model capability is enforced upstream of authentication.** `big-pickle` rejects +`image_url` identically with and without the header, and the rejection text names +the upstream provider's deserializer. A key does not change which content types a +text-only model accepts. + +**`mimo-v2.5-free` is now doubly confirmed as vision-capable** — it does not merely +return 200, it narrates the image. The reviewer's own spot-check hit a transient +502 on this model and recorded it as inconclusive rather than contrary; this run +resolves it. + +What remains genuinely unproven is whether an *authenticated* Zen account is +served a different roster or different routing for the same IDs. That cannot be +settled without a key. The layer handles it by scope rather than by assumption: +see `020`. diff --git a/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md new file mode 100644 index 000000000..b9692d01d --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md @@ -0,0 +1,176 @@ +# 010 — Layer 1: DeepSeek reasoning ladder (#1057) + +## The defect + +`src/providers/registry.ts:349-360` advertises `["high","xhigh","max"]` for both +DeepSeek V4 models and maps `low -> high`. Native `low` is therefore unreachable, +and `xhigh` is presented as a native tier while aliasing something else. + +## What the vendor actually documents + +From `001`, confirmed in both the English and Chinese official tables: + +| requested | `deepseek-v4-flash` | `deepseek-v4-pro` | +|---|---|---| +| `low` | `low` | `high` | +| `high` | `high` | `high` | +| `xhigh` | `high` | `max` | +| `max` | `max` | `max` | + +**The two models do not share a mapping.** The current code applies one +`DEEPSEEK_THINKING_REASONING_MAP` to both, so no single corrected constant is +right for both models. + +## Change map + +### `src/providers/registry.ts:349-360` — MODIFY + +Replace the one shared map with the advertised ladder plus two per-model maps. + +```ts +// BEFORE +const DEEPSEEK_THINKING_EFFORTS = ["high", "xhigh", "max"]; +const DEEPSEEK_THINKING_REASONING_MAP: Record = { + low: "high", medium: "high", high: "high", xhigh: "max", max: "max", +}; + +// AFTER +// DeepSeek's Codex ladder is low/high/max (api-docs.deepseek.com/guides/thinking_mode, +// verified 2026-08-06). `xhigh` is a compatibility alias, not a native tier, and it +// resolves DIFFERENTLY per model: xhigh->max on Pro, xhigh->high on Flash. `medium` +// has no documented row; mapping it to `high` is our own compatibility choice. +// Flash honors low natively. Pro does not (low->high today), so Pro must not +// advertise a tier the vendor silently upgrades. +const DEEPSEEK_FLASH_EFFORTS = ["low", "high", "max"]; +const DEEPSEEK_PRO_EFFORTS = ["high", "max"]; +const DEEPSEEK_PRO_REASONING_MAP: Record = { + low: "high", medium: "high", high: "high", xhigh: "max", max: "max", +}; +const DEEPSEEK_FLASH_REASONING_MAP: Record = { + low: "low", medium: "high", high: "high", xhigh: "high", max: "max", +}; +``` + +### The advertised ladder is per model too, and this is the audit's finding + +An earlier draft advertised `["low","high","max"]` for both models. The reviewer +rejected that, correctly: on Pro, DeepSeek resolves `low -> high`. Advertising +`low` there would put a tier in the picker that the vendor silently upgrades — the +user selects "low", pays for "high", and nothing tells them. That is not a smaller +version of the reported defect, it is the same defect pointed at a different value. + +So Pro advertises `["high","max"]` — the two levels it actually distinguishes — +and Flash advertises `["low","high","max"]`. The reporter asked for `low/high/max` +everywhere; that is right for Flash and wrong for Pro, and following the vendor +table beats following the request. + +`xhigh` disappears from both advertised ladders while staying in both wire maps. +That is the issue's actual ask: aliases stay, they just stop pretending to be +native tiers. + +**Re-verify before implementing.** The vendor page carries a footnote that Pro's +mapping updates in early August 2026, which is now. If Pro starts honoring `low` +natively, `DEEPSEEK_PRO_EFFORTS` gains it and the map entry changes with it. The +layer's first action is to re-read the table, not to trust this document. + +### The seven consumers — MODIFY each to pick the right map + +`DEEPSEEK_THINKING_REASONING_MAP` is consumed by seven provider entries. Each +becomes a per-model selection instead of one shared object: + +| provider | line | models | +|---|---|---| +| `opencode-go` | 934-950 | both | +| `orcarouter` | 1088-1093 | `deepseek/deepseek-v4-pro` | +| `deepseek` | 1185-1186 | both | +| `volcengine-coding-plan` | 1460-1478 | both | +| `alibaba-token-plan` | 1519-1524 | pro only | +| `alibaba-token-plan-intl` | 1552-1562 | both | +| `opencode-free` | 1668-1669 | `deepseek-v4-flash-free` → Flash map | + +A small helper keeps this from becoming seven hand-written objects: + +```ts +const isDeepseekFlash = (modelId: string): boolean => + modelId.toLowerCase().includes("flash"); +const deepseekEffortsFor = (modelId: string): string[] => + isDeepseekFlash(modelId) ? DEEPSEEK_FLASH_EFFORTS : DEEPSEEK_PRO_EFFORTS; +const deepseekReasoningMapFor = (modelId: string): Record => + isDeepseekFlash(modelId) ? DEEPSEEK_FLASH_REASONING_MAP : DEEPSEEK_PRO_REASONING_MAP; +``` + +The audit verified this substring test against every ID in the seven entries, +including the prefixed `deepseek/deepseek-v4-pro` and the suffixed +`deepseek-v4-flash-free`, and found it correct today. It is still a substring +test, so the layer adds an assertion enumerating the exact IDs and their expected +classification — a future `deepseek-v5-flashlite-pro` would otherwise misroute +silently. + +### `src/config.ts` — MODIFY (saved-config migration) + +A constants-only patch fixes fresh installs only. CLI-created built-in providers +persist the full registry seed (`src/cli/provider.ts:168-179`, +`src/providers/derive.ts:135-140`), and a persisted per-model ladder *replaces* +the registry ladder at routing (`src/router.ts:162-170`). An existing user keeps +advertising `xhigh` and mapping `low -> high` forever. + +Narrow in-memory normalizer during `loadConfig`, no write from the read path +(matching `src/config.ts:1509-1516`). + +**The migration is per model, exactly like the registry.** An earlier draft +normalized every legacy ladder to `["low","high","max"]`, which would have handed +Pro back the `low` tier the registry change just removed — the same defect, +reintroduced through the upgrade path for existing users only. The audit caught it. + +- Legacy ladder `["high","xhigh","max"]` on a **Flash** model → `["low","high","max"]`. +- Legacy ladder `["high","xhigh","max"]` on a **Pro** model → `["high","max"]`. +- Legacy map: set `low` to `low` on Flash, leave `low: "high"` on Pro, and set + `xhigh` to `high` on Flash / `max` on Pro. +- Replace the exact legacy shapes only; leave any non-exact user override untouched. +- Apply only where provider name and transport match the registry entry. + +After migration a saved config and a fresh install must produce identical +metadata for the same model. That equality is the migration test. + +### Tests + +**Update (these lock the defect):** + +- `tests/provider-registry-parity.test.ts:110-113` — advertised ladder. +- `tests/volcengine-providers.test.ts:71-78` — advertised ladder + `low` mapping. + +Both now expect **different** values per model, not one shared array. + +**Do not touch (these lock compatibility aliases, not the defect):** + +- `tests/volcengine-providers.test.ts:261-264` — `medium -> high`, `xhigh -> max`. +- `tests/opencode-go-deepseek.test.ts:102-110` — same. +- `tests/umans-provider.test.ts:81-82` — Umans GLM, unrelated literal match. +- `tests/alibaba-intl-token-plan.test.ts:57-64` — Qwen 3.8, unrelated. +- `tests/reasoning-effort.test.ts:723-734` — generic self-heal fixture. + +**Add**, in `tests/provider-registry-parity.test.ts` (registry) and +`tests/config.test.ts` (migration): + +- Flash maps `xhigh -> high`; Pro maps `xhigh -> max`. +- Flash advertises `["low","high","max"]`; Pro advertises `["high","max"]`. +- An enumerated ID-to-classification table covering all seven entries, so a future + id like `deepseek-v5-flashlite-pro` cannot misroute through the substring test. +- Migration: a legacy Pro config normalizes to `["high","max"]` and a legacy Flash + config to `["low","high","max"]`; both then equal a fresh install. +- Migration: a user-customized ladder is left untouched. + +## Red-green + +The new per-model test fails on the pre-fix tree because both models currently +share one map. Ablating the Flash map alone flips that single assertion. + +## Accept criteria + +- Flash advertises `["low","high","max"]`; Pro advertises `["high","max"]`. No + entry advertises `xhigh`. +- Flash and Pro carry different wire maps, matching the vendor table verified + 2026-08-06 (`001`, re-confirmed immediately before implementation). +- A migrated legacy config equals a fresh install for the same model, per model. +- A user-customized ladder is not rewritten. +- `bun run typecheck` clean; the affected test files pass. diff --git a/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md b/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md new file mode 100644 index 000000000..4c2c0ab68 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md @@ -0,0 +1,169 @@ +# 020 — Layer 2: text-only Zen models reject images (#1043, live half of #1024) + +## The defect + +`opencode-zen` (`src/providers/registry.ts:1652`) declares neither +`noVisionModels` nor modality metadata. Image stripping and sidecar activation +both key on `noVisionModels` membership (`src/vision/index.ts:192-211`, +`src/server/responses/core.ts:1689-1692`), so an image part is forwarded verbatim +to a text-only model and the upstream rejects the whole request with a 400. + +Reproduced live — see `002`. `big-pickle` returns exactly the error quoted in the +issue. + +## Why not the reporter's suggested fix + +The issue proposes stripping whenever `inputModalities` lacks `"image"`. Three +findings from the control-flow lane make that the wrong first move: + +1. Zen's `GET /v1/models` returns **no capability field at all** — only `id`, + `object`, `created`, `owned_by` (measured in `002`). So `inputModalities` is + `undefined` for exactly these models (`src/codex/catalog/provider-fetch.ts:719-743`). +2. Live-discovered modalities are never copied into the request-time provider + config (`src/router.ts:84-110`), so vision planning cannot see them anyway. +3. A deliberate regression guard asserts unlisted models keep forwarding images + (`tests/vision-sidecar-e2e.test.ts:163-193`). Flipping the default breaks a + contract someone wrote on purpose. + +A modality-keyed default would therefore change behavior everywhere *except* the +provider that motivated the issue. The narrow classification fixes the actual +report; the modality-driven default is a follow-up that needs the metadata to +become canonical first. + +## Change map + +### `src/providers/registry.ts` — MODIFY + +Add the measured constant beside the existing OpenCode lists (near `:350`): + +```ts +// Measured against https://opencode.ai/zen/v1 on 2026-08-05, one image request per +// model (devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md). Zen publishes +// no modality field, so this list is empirical, not derived. mimo-v2.5-free and +// longcat-2.0-free ACCEPT images and are deliberately absent. +const OPENCODE_ZEN_TEXT_ONLY_MODELS = [ + "big-pickle", + "nemotron-3-ultra-free", + "ling-3.0-flash-free", + "north-mini-code-free", + "laguna-s-2.1-free", + "deepseek-v4-flash-free", +]; +``` + +Then attach it to the `opencode-zen` entry at `:1652`, which currently has no +`noVisionModels`: + +```ts +// BEFORE +{ id: "opencode-zen", label: "opencode zen", baseUrl: "https://opencode.ai/zen/v1", + adapter: "openai-chat", authKind: "key", dashboardUrl: "https://opencode.ai/auth" }, + +// AFTER +{ id: "opencode-zen", label: "opencode zen", baseUrl: "https://opencode.ai/zen/v1", + adapter: "openai-chat", authKind: "key", dashboardUrl: "https://opencode.ai/auth", + noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS }, + +// and widen the sibling free entry from the DeepSeek-only list +// :1671 noVisionModels: OPENCODE_FREE_DEEPSEEK_MODELS, +// to +// :1671 noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS, +``` + +The sibling `opencode-free` entry at `:1655-1671` already does exactly this +against the same base URL, so this is the established shape for this vendor, not +a new mechanism. + +### Does free-tier evidence justify touching the key-auth entry? + +The audit's sharpest objection: the probe ran unauthenticated, and `opencode-zen` +is `authKind: "key"`. Applying one tier's evidence to another is how a user's +image gets silently destroyed. + +Re-probed with the desktop header removed entirely (`002`, follow-up section): +`big-pickle` rejects `image_url` identically with no header, `mimo-v2.5-free` +narrates the image with no header, and a bogus bearer token returns `AuthError` +before any model logic runs. So capability is enforced upstream of authentication +and the header was never load-bearing — the evidence does transfer for these IDs. + +What stays unproven is whether an authenticated account is served a *different +roster* under the same names. Two things bound that risk. The list is +fail-**closed** only for IDs measured to reject images, so a wrong entry costs a +caption instead of a hard 400; and `#1024` already reports real users hitting the +text-only failure through this provider, so the population being protected is not +hypothetical. + +If a maintainer with a Zen key disagrees, the correct narrowing is to move the +list to `opencode-free` alone. That is recorded as the fallback, not chosen, +because it would leave the reporter's own provider unfixed. + +**Accepted residual risk, stated for the PR.** Two audit rounds narrowed this and +neither closed it fully. Authenticated-tier equivalence is unproven for all eight +IDs, and the reviewer's own spaced re-probes of `mimo-v2.5-free` returned a 400 +and a 502 where ours returned 200 twice — the free route is intermittently +unstable, which is a reason to distrust any single measurement including our own. + +What survives that instability: `big-pickle`'s rejection reproduced identically +across four attempts, with and without the header, and its error text matches the +issue verbatim. The six listed IDs are the ones that failed consistently; the two +excluded ones are the only two that ever returned a completion for an image. + +The PR must carry this residual in its description rather than presenting the +list as settled. A maintainer with a key can close it in one command. + +### Drift policy — the list is dated, not permanent + +Zen's roster is discovered live (`liveModels: true` on the sibling) while this +list is static. Two failure directions, unequal in cost: + +| drift | consequence | severity | +|---|---|---| +| Zen adds a text-only model we do not list | user gets the loud upstream 400 — today's behavior | tolerable, visible | +| A listed model gains vision | images silently replaced by a caption | **worse, invisible** | + +The second is why the constant carries its measurement date in a comment and why +`mimo-v2.5-free` / `longcat-2.0-free` get an explicit *negative* assertion: the +guard has to survive a future well-meaning "just classify all the free models" +patch. This is a dated, measured exception list — not a capability model, and it +should be replaced by one once live modality metadata becomes canonical (the +follow-up named at the top of this doc). + +### Tests + +**Add:** + +- a registry assertion that all six measured IDs are in `opencode-zen.noVisionModels`; +- an explicit assertion that `mimo-v2.5-free` and `longcat-2.0-free` are **not** — + this is the guard against a future well-meaning "classify all free models" patch; +- an e2e case alongside `tests/vision-sidecar-e2e.test.ts:91-161` proving a listed + Zen model gets its image replaced before the upstream request. + +**Do not touch:** `tests/vision-sidecar-e2e.test.ts:163-193` (the unlisted-model +contract stays intact — this layer adds listings, it does not change the default). + +## Red-green + +The e2e assertion fails on the pre-fix tree: with no `noVisionModels`, the +upstream body contains the image bytes. After the fix it contains the omission +marker. Ablating just the registry line flips it back. + +## Activation evidence (C-ACTIVATION-GROUNDING-01) + +The strip path is a conditional branch, so "tests pass" is not enough. The e2e +test must assert the *observable effect* — the marker present and the image bytes +absent in the captured upstream body — not merely that the request succeeded. + +## Scope note + +This closes #1043. For #1024 it closes the Zen half; the `TR` / +`moonshotai/kimi-k3-free` half depends on reporter configuration for a provider +that is not in the registry, and stays open. + +## Accept criteria + +- Six measured IDs listed; the two vision-capable ones explicitly not. +- A test in `tests/vision-sidecar-e2e.test.ts` that fails if someone later adds + `mimo-v2.5-free` or `longcat-2.0-free` to the list. +- The registry assertion lives in `tests/provider-registry-parity.test.ts`. +- The unlisted-model forwarding contract still green. +- `bun run typecheck` clean; vision test files pass. diff --git a/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md b/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md new file mode 100644 index 000000000..ddfb0d572 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md @@ -0,0 +1,151 @@ +# 030 — Layer 3: native-profile harness hang and JSON race (#1061) + +## The defect, two halves + +**Unbounded teardown.** `tests/native-profile-crash-boundaries.test.ts:194-198`: + +```ts +} finally { + writeFileSync(p.release, "recover"); + writeFileSync(p.stop, "stop"); + expect(await restart.exited).toBe(0); +} +``` + +No deadline, no kill fallback. The child can stall in `server.stop(true)` +(`tests/helpers/native-profile-startup-child.ts:84`), and the run hangs until CI +kills the job — the 30-minute hang the issue reports. + +**Existence-not-content race.** `waitFor()` at `:80-84` proves only that the file +exists; `:182-183` then parses it immediately. The child writes that JSON +non-atomically (`tests/helpers/native-profile-startup-child.ts:71-81`), so a +partially written file yields `Unexpected EOF`. + +## Change map — reuse, do not invent + +### `tests/helpers/native-profile-startup-child.ts` — MODIFY + +The repository already has an atomic writer with an explicit no-half-written +guarantee (`src/config.ts:195-217`, documented at `:102-105`): + +```ts +// BEFORE +import { loadConfig } from "../../src/config"; +... +writeFileSync(settledPath, JSON.stringify(...)); + +// AFTER +import { atomicWriteFile, loadConfig } from "../../src/config"; +... +atomicWriteFile(settledPath, JSON.stringify(...)); +``` + +Both write sites (`:72-80`, success and failure) change. Marker files that are +only checked for existence stay as they are. + +### `tests/native-profile-crash-boundaries.test.ts` — MODIFY + +Add a parse-aware wait beside the existing `waitFor`, and use it where a parse +follows: + +```ts +async function waitForJson(path: string, timeout = 10_000): Promise { + const deadline = Date.now() + timeout; + let lastError: unknown; + while (Date.now() < deadline) { + if (existsSync(path)) { + try { return JSON.parse(readFileSync(path, "utf8")) as T; } + catch (error) { lastError = error; } + } + await Bun.sleep(10); + } + throw new Error(`timed out waiting for parseable JSON in ${path}`, { cause: lastError }); +} +``` + +Bound the teardown following the pattern the sibling test already uses +(`tests/native-profile-startup.test.ts:259-269`), with the shared +`INTERNAL_DEADLINE_MS` from `tests/helpers/test-budget.ts:49-57`. + +The audit caught two defects in the earlier sketch: it referenced an undefined +`timeoutMs`, and it left an unbounded `await child.exited` *after* the kill while +claiming no unbounded waits remained. A SIGTERM that the child ignores would hang +in exactly the same way as the bug being fixed. Both are corrected here: + +```ts +import { INTERNAL_DEADLINE_MS } from "./helpers/test-budget"; + +const KILL_GRACE_MS = 2_000; + +async function stopStartup( + child: ReturnType, + paths: ReturnType, + timeoutMs: number = INTERNAL_DEADLINE_MS, +): Promise { + writeFileSync(paths.release, "recover"); + writeFileSync(paths.stop, "stop"); + const exit = await Promise.race([child.exited, Bun.sleep(timeoutMs).then(() => null)]); + if (exit === null) { + child.kill(); + // bounded here too: an ignored SIGTERM must not reproduce the original hang + const killed = await Promise.race([ + child.exited, + Bun.sleep(KILL_GRACE_MS).then(() => null), + ]); + if (killed === null) { + child.kill("SIGKILL"); + // and observe the escalation, so the child is reaped before we throw — + // otherwise the caller's `child.exitCode !== null` assertion races + await Promise.race([child.exited, Bun.sleep(KILL_GRACE_MS).then(() => null)]); + } + throw new Error("startup child did not stop"); + } + if (exit !== 0) throw new Error(await new Response(child.stderr).text()); +} +``` + +Call `await stopStartup(restart, p)` from `finally`, replacing +`expect(await restart.exited).toBe(0)` at `:194-198`. + +Both halves reuse mechanisms already in the tree. Neither is new machinery. + +## Red-green, and where it is honest about its limits + +**The JSON race is deterministically testable.** Write partial JSON (`"{"`), start +`waitForJson`, then replace it atomically; assert it returns the parsed object +instead of throwing `Unexpected EOF`. That test fails against the old +existence-only `waitFor` by construction. + +**The hang needs injection to be provable.** Exact shape, since the audit asked +for it rather than a description: + +- In `tests/helpers/native-profile-startup-child.ts`, immediately after the stop + marker is observed and **before** `server.stop(true)` at `:84`: + ```ts + if (process.env.OCX_TEST_STALL_ON_STOP === "1") { await new Promise(() => {}); } + ``` + Test-only, opt-in, and inert unless the variable is set. The audit judged this + acceptable in `tests/helpers/`. +- A new case in `tests/native-profile-crash-boundaries.test.ts` spawns the child + with that variable, calls `stopStartup(child, paths, 1_000)`, and asserts the + rejection message is `startup child did not stop` **and** that + `child.exitCode !== null` afterwards — proving cleanup happened, not merely that + a timeout was detected. + The `exitCode` assertion is only sound because the SIGKILL escalation is now + awaited before the throw; the audit caught that race in the previous draft. + A child that ignores SIGKILL is not testable and not a real case — the escalation + branch itself stays unproven, and the PR should say so rather than imply otherwise. +- Deadline values: `1_000` ms in the injected test (fast), `INTERNAL_DEADLINE_MS` + in production teardown. + +Without that case the bounded wait would be present but never shown firing, and +"it no longer hangs" would be unfalsifiable in a green run. + +## Accept criteria + +- Teardown bounded with a kill fallback; no unbounded `await child.exited`. +- The settled-file read proves parseable JSON, not existence. +- The child publishes that file atomically. +- A deterministic test for the parse race; an injected-stall test for the timeout, + or an explicit statement that the branch is unexercised. +- `bun test tests/native-profile-crash-boundaries.test.ts` green on macOS. diff --git a/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md new file mode 100644 index 000000000..e621f8ebd --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md @@ -0,0 +1,210 @@ +# 040 — Layer 4: startup catalog write leaves stale app-servers unhandled (#1046) + +## The defect + +Service startup rewrites the Codex catalog and `models_cache.json`, then does +nothing about app-servers already running against the old catalog. +`afterCatalogWriteHandleAppServers()` is called only from the explicit CLI `sync` +and `sync-cache` paths (`src/cli/index.ts:858-880`). + +The reporter's two-host comparison pinned it precisely: *"Host A's catalog was +rewritten 4m27s after its app-server booted, so the app-server serves an in-memory +list that no longer exists on disk. Every check we ran reads the file; the picker +renders memory."* + +That is confirmed upstream. For a configured catalog, Codex builds a +`StaticModelsManager` holding an in-process `Vec`; its list operations +clone that vector and its refresh is a no-op. Rewriting either disk file cannot +move it. The gap appears twice on the startup path: +`src/server/index.ts:403-412` (cache invalidation) and +`src/cli/index.ts:319-320` → `src/codex/desired-state.ts:148-160` (catalog sync). + +## The obvious fix is unsafe, and this is the important part + +`afterCatalogWriteHandleAppServers()` has two branches +(`src/codex/app-server-processes.ts:725-756`): + +- `restart: false` — logs a warning. No signal, no prompt, no wait. +- `restart: true` — **SIGTERMs matching app-servers** + (`:738-742` → `:656-710`), with its own log line admitting active turns may be + interrupted. It does not drain, and it never escalates to SIGKILL. + +Wiring the `restart` branch into unattended startup would kill a user's in-flight +turn every time the service starts on login, repair, or update. A human typing +`ocx sync --restart-codex` is consenting to that; a boot is not. + +There is a second trap. The existing handler warns whenever *any* matching +app-server exists — it does not check staleness. The repository already has an +mtime-based classifier that does (`:538-642`, `stale` iff +`startedAtMs <= catalogMtimeMs`). Using the blunt handler at startup would warn on +every boot with Codex open, including the common case where the app-server is +newer than the catalog and perfectly correct. + +## Change map — warn only, stale only + +### `src/codex/app-server-processes.ts` — ADD + +A startup-safe helper beside the existing handler: + +```ts +export function warnIfStaleCodexAppServersAfterStartupWrite( + opts: { + log?: Pick; + io?: CodexAppServerProcessIo; // the real seam, src/codex/app-server-processes.ts:79-89 + } = {}, +): { warned: boolean } { + try { + // Pass `io` through: tests inject listSnapshots/readStartMs/catalogMtimeMs/now, + // and supplying any field also bypasses the 5s memo (`fullyDefault` at :580), + // which is what stops a pre-write `fresh` reading from masking this check. + const status = collectCodexAppServerCatalogState(opts.io ?? {}); + if (status.state !== "stale") return { warned: false }; + (opts.log ?? console).error(formatStaleCodexAppServerWarning(status.processes)); + return { warned: true }; + } catch { + return { warned: false }; // startup sync is best-effort; never fail boot + } +} +``` + +Three corrections the audit forced, all verified against the real source: + +- `CodexAppServerProcessIo` (`:79-89`) already carries `listSnapshots`, + `readStartMs`, `catalogMtimeMs`, `now`, **and `kill`** — every seam the test list + below needs. The earlier draft promised injection while showing a helper with + nothing to inject into. +- `collectCodexAppServerCatalogState` is **synchronous** and takes the io object + positionally; the earlier draft `await`ed a no-arg call. +- Supplying any io field makes `fullyDefault` false, so the 5 s memo is skipped. + The cache bypass therefore needs no new mechanism — but production must pass a + non-empty io (or an explicit invalidation) after a confirmed write, or it + inherits the stale-masking bug. + +`CodexAppServerCatalogStatus` (`:540-544`) is +`{ state: CodexAppServerCatalogState; processes: Array<{ pid: number; startedAtMs: number | null }>; catalogMtimeMs: number | null }`. +The discriminant field is `state`, and `processes` is already a PID-bearing shape, +so `formatStaleCodexAppServerWarning` needs only its parameter widened from the +full `CodexAppServerProcess` to that shape — it reads `.pid` alone (`:410-417`). + +It never reaches `restartCodexAppServers()`. That is the whole point. + +`formatStaleCodexAppServerWarning` currently takes full `CodexAppServerProcess` +objects but reads only `.pid` (`:410-417`); widening its parameter to a +PID-bearing shape avoids a needless cast. + +### `src/codex/desired-state.ts:148-160` — MODIFY (two problems, both found at audit) + +**The sync result is currently thrown away.** `defaultStartupSync` returns +`syncModelsToCodex(port)` as `Promise`, and `syncCodexOnStartIfEnabled` +does `await sync(port).catch(() => {})` then returns a bare `boolean` meaning +"the integration was enabled", not "a write happened". So the `catalogWritten || +cacheSynced` gate this layer depends on **cannot be evaluated** as the code +stands. + +Fix the seam first: type `CodexStartupSync` to return the sync result, keep the +`.catch` (startup must stay best-effort), and let the caller distinguish +*skipped* / *ran and wrote* / *ran and failed*. Only then is the write gate real. + +**The catalog-state cache can mask the very staleness we are checking for.** +`collectCodexAppServerCatalogState` memoizes for 5 s +(`src/codex/app-server-processes.ts:557-585`, `CATALOG_STATE_TTL_MS`). Startup +plausibly calls it before the write — some other guidance path already does — gets +`fresh`, and the post-write call inside that window returns the cached `fresh` +and stays silent. The helper must bypass or invalidate the cache after a +confirmed write. The function already accepts a `CodexAppServerProcessIo` seam +and only uses the cache when every field is defaulted, so an explicit bypass is +available without new machinery. + +### `src/server/index.ts:403-412` — MODIFY (no longer out of scope) + +An earlier draft excluded this as "a different write with a different lifecycle". +The audit rejected that, correctly: this path invalidates `models_cache.json` on +**every** server startup, independently of the later optional sync. With Codex +integration disabled, or when the sync writes nothing, startup still leaves a +running app-server stale and this layer would have shipped silence. + +### Who owns the warning, and how it warns once + +The audit's last blocker: "both writes route through one decision" is a +requirement, not a design. Two independent helper calls would warn twice. + +The two write sites are ordered and both live in the same process on the CLI +start path: + +1. `src/server/index.ts:406-412` — `invalidateCodexModelsCacheWithPermit`, inside + `startServer()`, wrapped in `try/catch` because `getCodexHome()` throws when + there is no Codex home. +2. `src/cli/index.ts:320` — `await syncCodexOnStartIfEnabled(port, config)`, after + the server is already bound. + +**`handleStart` owns the single warning.** Neither write site warns on its own: + +- `startServer()` returns whether its cache invalidation actually wrote + (currently the `try` block discards that). It stays silent. +- `syncCodexOnStartIfEnabled` returns the typed sync result (the seam fix above). + It stays silent. +- `handleStart` ORs the two write flags and calls + `warnIfStaleCodexAppServersAfterStartupWrite()` at most once, after both. + +That ordering matters for correctness, not just tidiness: warning after the +*first* write would read a catalog mtime that the *second* write is about to +move, so the one call has to come last. + +`startServer()` is also reachable without `handleStart` (tests, embedded use). In +that path nothing warns — deliberate. A caller that does not own a startup +lifecycle should not emit lifecycle diagnostics, and the alternative is a module +global that a test can leak across cases. + +**Test:** `tests/codex-desired-state.test.ts` — a startup where both the cache +invalidation and the sync report a write emits exactly one warning; asserted on +the injected `log.error` call count, not on the message. + +## Tests + +No test currently combines startup sync with app-server handling — verified by an +exhaustive read-only scan across `origin/dev`'s `tests/` for files mentioning both +`syncCodexOnStartIfEnabled|handleStart` and +`afterCatalogWriteHandleAppServers|collectCodexAppServerCatalogState`. Result: +empty. + +**Add**, faking both boundaries through the existing `CodexAppServerProcessIo` +seam (`io.listSnapshots`, `io.readStartMs`, `io.catalogMtimeMs`, `io.now`) so no +new injection point is invented: + +1. the warning runs only when the sync reports a write — requires the typed + result from the seam fix above; +2. stale warns; `fresh`, `not_running`, and `unknown` do not; +3. **pre-write `fresh` then post-write `stale` still warns** — this is the + cache-masking regression, and it is the test that would have caught the bug the + audit found; +4. **an injected `kill` that fails the test if called**, proving no startup path + can reach `restartCodexAppServers()`; +5. discovery throwing still resolves startup successfully; +6. a startup hitting both write paths warns exactly once. + Asserted through `handleStart`'s injected log, per the ownership rule above. + +For (4), inject `io.kill` and fail the test if it is ever called. The seam exists +at `src/codex/app-server-processes.ts:84`. + +Existing coverage to leave intact: `tests/codex-app-server-processes.test.ts:19-106` +(classification), `:309-338` (warn vs SIGTERM), `tests/codex-desired-state.test.ts:167-205` +(startup enable/disable). + +## Red-green + +Test 2 fails on the pre-fix tree: startup emits no warning at all for a stale +app-server. Test 3 fails against a naive implementation that reuses the cached +state — it is the specific regression proof for this layer. Test 4 passes before +and after by construction; it is a guard against future refactors, not a +regression proof, and is not counted as one. + +## Accept criteria + +- Startup warns only when the classifier says `stale`. +- The startup sync seam returns a typed result, so "a write happened" is observable. +- A pre-write `fresh` reading cannot suppress the post-write warning. +- Both startup write paths are covered, warning once. +- No code path from startup can reach `restartCodexAppServers()`, proven by an + injected `kill` that fails the test if invoked. +- Discovery failure never fails boot. +- `bun run typecheck` clean; both app-server test files pass. diff --git a/src/providers/registry.ts b/src/providers/registry.ts index ae26b6964..ae4907b80 100644 --- a/src/providers/registry.ts +++ b/src/providers/registry.ts @@ -348,16 +348,61 @@ const THINKING_BUDGET_MODELS = [ const OPENCODE_GO_THINKING_BUDGET_MODELS = ["qwen3.5-plus", "qwen3.6-plus", "qwen3.7-max", "qwen3.7-plus"]; const DEEPSEEK_THINKING_MODELS = ["deepseek-v4-pro", "deepseek-v4-flash"]; const OPENCODE_FREE_DEEPSEEK_MODELS = ["deepseek-v4-flash-free"]; -// "max" is advertised too: the wire map routes xhigh->max and max->max, so the picker -// should surface the max tier instead of hiding it behind xhigh. -const DEEPSEEK_THINKING_EFFORTS = ["high", "xhigh", "max"]; -const DEEPSEEK_THINKING_REASONING_MAP: Record = { +/* + * DeepSeek's Codex ladder is low/high/max, and the two V4 models resolve it + * DIFFERENTLY. From the official thinking-mode table (api-docs.deepseek.com, + * EN and zh-cn agree, re-verified 2026-08-06): + * + * requested | v4-flash | v4-pro + * low | low | high + * high | high | high + * xhigh | high | max + * max | max | max + * + * Two consequences (#1057): + * + * - `xhigh` is a COMPATIBILITY ALIAS, not a native tier. It stays in the wire maps + * so existing requests and saved configs keep working, but it is not advertised. + * - Pro does NOT honor `low` — the vendor silently upgrades it to `high`. So Pro + * advertises only the two levels it actually distinguishes. Advertising `low` + * there would put a tier in the picker that costs `high`, which is the same + * defect this fixes wearing a different value. + * + * The vendor page footnotes that Pro's mapping updates in early August 2026; as of + * the re-verification above it had not changed. When it does, Pro gains `low` here. + * + * `medium` has no row in the vendor table — mapping it to `high` is OUR + * compatibility choice for clients that only speak the OpenAI ladder. + */ +const DEEPSEEK_FLASH_THINKING_EFFORTS = ["low", "high", "max"]; +const DEEPSEEK_PRO_THINKING_EFFORTS = ["high", "max"]; +const DEEPSEEK_PRO_REASONING_MAP: Record = { low: "high", medium: "high", high: "high", xhigh: "max", max: "max", }; +const DEEPSEEK_FLASH_REASONING_MAP: Record = { + low: "low", + medium: "high", + high: "high", + xhigh: "high", + max: "max", +}; +/** + * Flash-versus-Pro classification for DeepSeek V4 model ids, including prefixed + * (`deepseek/deepseek-v4-pro`) and suffixed (`deepseek-v4-flash-free`) forms. + * `tests/provider-registry-parity.test.ts` enumerates every id the registry + * actually passes here, so a future id this substring test would misread cannot + * land silently. + */ +const isDeepseekFlashModel = (modelId: string): boolean => + modelId.toLowerCase().includes("flash"); +const deepseekThinkingEffortsFor = (modelId: string): string[] => + isDeepseekFlashModel(modelId) ? DEEPSEEK_FLASH_THINKING_EFFORTS : DEEPSEEK_PRO_THINKING_EFFORTS; +const deepseekReasoningMapFor = (modelId: string): Record => + isDeepseekFlashModel(modelId) ? DEEPSEEK_FLASH_REASONING_MAP : DEEPSEEK_PRO_REASONING_MAP; // 260719 Alibaba Token Plan Personal Edition (China/Beijing). Keep it distinct from // Coding Plan: the products use different exact allowlists and different base URLs. // Evidence: https://help.aliyun.com/en/model-studio/token-plan-personal-overview @@ -938,7 +983,7 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ "kimi-k2.7-code-highspeed": [], ...Object.fromEntries(OPENCODE_GO_THINKING_TOGGLE_MODELS.map(id => [id, THINKING_TOGGLE_EFFORTS])), ...Object.fromEntries(OPENCODE_GO_THINKING_BUDGET_MODELS.map(id => [id, THINKING_BUDGET_EFFORTS])), - ...Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_EFFORTS])), + ...Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)])), }, modelDefaultReasoningEfforts: { "kimi-k3": "max" }, // glm-5.2 uses identity labels now that `max` is a native Codex level (no alias map); @@ -946,7 +991,7 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ modelReasoningEffortMap: { "kimi-k3": KIMI_CODING_K3_REASONING_EFFORT_MAP, ...Object.fromEntries(OPENCODE_GO_THINKING_TOGGLE_MODELS.map(id => [id, THINKING_TOGGLE_MAP])), - ...Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_REASONING_MAP])), + ...Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekReasoningMapFor(id)])), }, thinkingToggleModels: OPENCODE_GO_THINKING_TOGGLE_MODELS, thinkingBudgetModels: THINKING_BUDGET_MODELS, @@ -1087,9 +1132,9 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ // returned 200), so no noTemperatureModels entry is warranted here. modelReasoningEfforts: { "openai/gpt-5.5": ["low", "medium", "high", "xhigh"], - "deepseek/deepseek-v4-pro": DEEPSEEK_THINKING_EFFORTS, + "deepseek/deepseek-v4-pro": deepseekThinkingEffortsFor("deepseek/deepseek-v4-pro"), }, - modelReasoningEffortMap: { "deepseek/deepseek-v4-pro": DEEPSEEK_THINKING_REASONING_MAP }, + modelReasoningEffortMap: { "deepseek/deepseek-v4-pro": deepseekReasoningMapFor("deepseek/deepseek-v4-pro") }, preserveReasoningContentModels: ["deepseek/deepseek-v4-pro"], note: "OpenAI-compatible adaptive router. Default is a tool-capable model; orcarouter/auto (adaptive routing) is also selectable. Full catalog: https://www.orcarouter.ai/models", }, @@ -1182,8 +1227,8 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ - 대안 분석: Globally preserve reasoning_content for all OpenAI-compatible models; preserve it for legacy deepseek-reasoner too; mark only V4 thinking models in registry metadata. - 선택 근거: DeepSeek V4 thinking mode requires history replay, while older DeepSeek reasoner has different compatibility rules. A model-scoped registry flag fixes built-in and stale saved configs without broad provider regressions. */ - modelReasoningEfforts: Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_EFFORTS])), - modelReasoningEffortMap: Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_REASONING_MAP])), + modelReasoningEfforts: Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)])), + modelReasoningEffortMap: Object.fromEntries(DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekReasoningMapFor(id)])), preserveReasoningContentModels: DEEPSEEK_THINKING_MODELS, // Issue #88: every DeepSeek API model is text-only input (no image support upstream) — the // vision sidecar describes attached images for them, and the catalog advertises image input @@ -1470,10 +1515,10 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ modelInputModalities: VOLCENGINE_PLAN_INPUT_MODALITIES, noVisionModels: VOLCENGINE_PLAN_TEXT_ONLY_MODELS, modelReasoningEfforts: Object.fromEntries( - DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_EFFORTS]), + DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)]), ), modelReasoningEffortMap: Object.fromEntries( - DEEPSEEK_THINKING_MODELS.map(id => [id, DEEPSEEK_THINKING_REASONING_MAP]), + DEEPSEEK_THINKING_MODELS.map(id => [id, deepseekReasoningMapFor(id)]), ), preserveReasoningContentModels: DEEPSEEK_THINKING_MODELS, note: "Coding tools only. Volcengine restricts Coding Plan quota to supported AI coding tools and warns that using this key for general API calls may suspend the subscription or ban the account. Use the plan key issued by the Ark console.", @@ -1519,9 +1564,9 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ modelReasoningEfforts: { ...Object.fromEntries(ALIBABA_TOKEN_PLAN_QWEN_MODELS.map(id => [id, THINKING_BUDGET_EFFORTS])), "glm-5.2": ZAI_GLM_52_REASONING_EFFORTS, - "deepseek-v4-pro": DEEPSEEK_THINKING_EFFORTS, + "deepseek-v4-pro": deepseekThinkingEffortsFor("deepseek-v4-pro"), }, - modelReasoningEffortMap: { "deepseek-v4-pro": DEEPSEEK_THINKING_REASONING_MAP }, + modelReasoningEffortMap: { "deepseek-v4-pro": deepseekReasoningMapFor("deepseek-v4-pro") }, thinkingBudgetModels: ALIBABA_TOKEN_PLAN_QWEN_MODELS, preserveReasoningContentModels: ["glm-5.2", "deepseek-v4-pro", "qwen3.8-max", "qwen3.7-max", "qwen3.7-plus", "qwen3.6-flash"], noVisionModels: ["glm-5.2", "deepseek-v4-pro"], @@ -1553,12 +1598,12 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ ...Object.fromEntries(ALIBABA_INTL_TOKEN_PLAN_QWEN_MODELS.map(id => [id, THINKING_BUDGET_EFFORTS])), "qwen3.8-max": ["low", "high", "xhigh"], "glm-5.2": ZAI_GLM_52_REASONING_EFFORTS, - "deepseek-v4-pro": DEEPSEEK_THINKING_EFFORTS, - "deepseek-v4-flash": DEEPSEEK_THINKING_EFFORTS, + "deepseek-v4-pro": deepseekThinkingEffortsFor("deepseek-v4-pro"), + "deepseek-v4-flash": deepseekThinkingEffortsFor("deepseek-v4-flash"), }, modelReasoningEffortMap: { - "deepseek-v4-pro": DEEPSEEK_THINKING_REASONING_MAP, - "deepseek-v4-flash": DEEPSEEK_THINKING_REASONING_MAP, + "deepseek-v4-pro": deepseekReasoningMapFor("deepseek-v4-pro"), + "deepseek-v4-flash": deepseekReasoningMapFor("deepseek-v4-flash"), }, thinkingBudgetModels: ALIBABA_INTL_TOKEN_PLAN_QWEN_MODELS, preserveReasoningContentModels: ["glm-5.2", "deepseek-v4-pro", "deepseek-v4-flash", "qwen3.8-max", "qwen3.7-max", "qwen3.7-plus", "qwen3.6-plus", "qwen3.6-flash"], @@ -1665,8 +1710,8 @@ export const PROVIDER_REGISTRY: readonly ProviderRegistryEntry[] = [ staticHeaders: { "x-opencode-client": "desktop", }, - modelReasoningEfforts: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, DEEPSEEK_THINKING_EFFORTS])), - modelReasoningEffortMap: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, DEEPSEEK_THINKING_REASONING_MAP])), + modelReasoningEfforts: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, deepseekThinkingEffortsFor(id)])), + modelReasoningEffortMap: Object.fromEntries(OPENCODE_FREE_DEEPSEEK_MODELS.map(id => [id, deepseekReasoningMapFor(id)])), preserveReasoningContentModels: OPENCODE_FREE_DEEPSEEK_MODELS, noVisionModels: OPENCODE_FREE_DEEPSEEK_MODELS, }, diff --git a/tests/opencode-go-deepseek.test.ts b/tests/opencode-go-deepseek.test.ts index 018405e1e..595acf175 100644 --- a/tests/opencode-go-deepseek.test.ts +++ b/tests/opencode-go-deepseek.test.ts @@ -105,7 +105,9 @@ describe("opencode-go DeepSeek V4 thinking mode", () => { const xhighBody = buildToolCallBody(modelId, "xhigh"); const mediumBody = buildToolCallBody(modelId, "medium"); - expect(xhighBody.reasoning_effort).toBe("max"); + // #1057: `xhigh` is a vendor alias that resolves per model — max on Pro, + // high on Flash (api-docs.deepseek.com/guides/thinking_mode, 2026-08-06). + expect(xhighBody.reasoning_effort).toBe(modelId === "deepseek-v4-flash" ? "high" : "max"); expect(mediumBody.reasoning_effort).toBe("high"); expect(xhighBody.messages[1].reasoning_content).toBe("I need to inspect files before answering."); expect(xhighBody.messages[1]).toMatchObject({ diff --git a/tests/provider-registry-parity.test.ts b/tests/provider-registry-parity.test.ts index cb332e24f..14b4fb1ac 100644 --- a/tests/provider-registry-parity.test.ts +++ b/tests/provider-registry-parity.test.ts @@ -108,9 +108,18 @@ describe("provider registry parity", () => { expect(KEY_LOGIN_PROVIDERS.openrouter.modelContextWindows?.["openai/gpt-5.6-terra"]).toBe(1_050_000); expect(KEY_LOGIN_PROVIDERS.openrouter.modelContextWindows?.["openai/gpt-5.6-luna"]).toBe(1_050_000); expect(KEY_LOGIN_PROVIDERS.deepseek.models).toContain("deepseek-v4-pro"); - expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEfforts?.["deepseek-v4-pro"]).toEqual(["high", "xhigh", "max"]); + // #1057: DeepSeek's ladder is low/high/max and the two V4 models resolve it + // differently (api-docs.deepseek.com/guides/thinking_mode, verified 2026-08-06). + // `xhigh` is an alias, so it stays in the wire map but is not advertised. Pro + // does not honor `low` (the vendor maps it to `high`), so Pro must not offer it. + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEfforts?.["deepseek-v4-pro"]).toEqual(["high", "max"]); + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEfforts?.["deepseek-v4-flash"]).toEqual(["low", "high", "max"]); + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-pro"]?.low).toBe("high"); expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-pro"]?.xhigh).toBe("max"); expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-pro"]?.max).toBe("max"); + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-flash"]?.low).toBe("low"); + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-flash"]?.xhigh).toBe("high"); + expect(KEY_LOGIN_PROVIDERS.deepseek.modelReasoningEffortMap?.["deepseek-v4-flash"]?.max).toBe("max"); expect(KEY_LOGIN_PROVIDERS.deepseek.preserveReasoningContentModels).toEqual(["deepseek-v4-pro", "deepseek-v4-flash"]); // Issue #88: every DeepSeek API model is text-only input — the vision sidecar covers them. expect(KEY_LOGIN_PROVIDERS.deepseek.noVisionModels).toEqual([ @@ -925,4 +934,51 @@ describe("free-provider directory isolation", () => { } } }); + + /* + * #1057. The registry classifies DeepSeek V4 ids Flash-versus-Pro with a substring + * test, which is correct for every id shipped today but is exactly the kind of + * thing that misfires on a future name. This enumerates every provider/model pair + * that actually receives the shared metadata, so a misclassification cannot land + * silently — it fails here with the offending id named. + * + * Ladders: Flash gets low/high/max; Pro gets high/max, because DeepSeek upgrades a + * requested `low` to `high` on Pro and advertising it would sell a tier the vendor + * does not deliver. Neither advertises `xhigh` — it is an alias, kept in the wire + * map only (api-docs.deepseek.com/guides/thinking_mode, verified 2026-08-06). + */ + test("every DeepSeek V4 entry advertises its own ladder and alias mapping", () => { + const flashLadder = ["low", "high", "max"]; + const proLadder = ["high", "max"]; + const cases: Array<{ provider: string; model: string; flash: boolean }> = [ + { provider: "deepseek", model: "deepseek-v4-pro", flash: false }, + { provider: "deepseek", model: "deepseek-v4-flash", flash: true }, + { provider: "opencode-go", model: "deepseek-v4-pro", flash: false }, + { provider: "opencode-go", model: "deepseek-v4-flash", flash: true }, + { provider: "orcarouter", model: "deepseek/deepseek-v4-pro", flash: false }, + { provider: "volcengine-coding-plan", model: "deepseek-v4-pro", flash: false }, + { provider: "volcengine-coding-plan", model: "deepseek-v4-flash", flash: true }, + { provider: "alibaba-token-plan", model: "deepseek-v4-pro", flash: false }, + { provider: "alibaba-token-plan-intl", model: "deepseek-v4-pro", flash: false }, + { provider: "alibaba-token-plan-intl", model: "deepseek-v4-flash", flash: true }, + { provider: "opencode-free", model: "deepseek-v4-flash-free", flash: true }, + ]; + + for (const { provider, model, flash } of cases) { + const entry = PROVIDER_REGISTRY.find(p => p.id === provider); + expect(entry, `registry entry ${provider} is missing`).toBeTruthy(); + + const ladder = entry?.modelReasoningEfforts?.[model]; + expect(ladder, `${provider}/${model} advertises no ladder`).toBeTruthy(); + expect(ladder, `${provider}/${model} ladder`).toEqual(flash ? flashLadder : proLadder); + expect(ladder, `${provider}/${model} must not advertise the xhigh alias`) + .not.toContain("xhigh"); + + const map = entry?.modelReasoningEffortMap?.[model]; + expect(map, `${provider}/${model} has no effort map`).toBeTruthy(); + expect(map?.xhigh, `${provider}/${model} xhigh alias`).toBe(flash ? "high" : "max"); + expect(map?.low, `${provider}/${model} low resolution`).toBe(flash ? "low" : "high"); + expect(map?.max, `${provider}/${model} max`).toBe("max"); + } + }); }); diff --git a/tests/volcengine-providers.test.ts b/tests/volcengine-providers.test.ts index f127a9647..93c7bf415 100644 --- a/tests/volcengine-providers.test.ts +++ b/tests/volcengine-providers.test.ts @@ -68,13 +68,15 @@ describe("Volcengine Ark providers", () => { "kimi-k2.6": ["text", "image"], "minimax-m3": ["text", "image"], }, + // #1057: per-model ladders. Pro omits `low` because the vendor upgrades it to + // `high`; `xhigh` is an alias in both maps and advertised in neither. modelReasoningEfforts: { - "deepseek-v4-pro": ["high", "xhigh", "max"], - "deepseek-v4-flash": ["high", "xhigh", "max"], + "deepseek-v4-pro": ["high", "max"], + "deepseek-v4-flash": ["low", "high", "max"], }, modelReasoningEffortMap: { "deepseek-v4-pro": { low: "high", medium: "high", high: "high", xhigh: "max", max: "max" }, - "deepseek-v4-flash": { low: "high", medium: "high", high: "high", xhigh: "max", max: "max" }, + "deepseek-v4-flash": { low: "low", medium: "high", high: "high", xhigh: "high", max: "max" }, }, preserveReasoningContentModels: ["deepseek-v4-pro", "deepseek-v4-flash"], }); @@ -258,11 +260,17 @@ describe("Volcengine Ark providers", () => { }; }; + // #1057: `medium` is our own compatibility alias (no vendor row) and maps to + // `high` on both. `xhigh` is a vendor-documented alias that resolves + // DIFFERENTLY per model — max on Pro, high on Flash — so this cannot be a + // shared expectation. expect(buildBody("medium").reasoning_effort).toBe("high"); const xhighBody = buildBody("xhigh"); - expect(xhighBody.reasoning_effort).toBe("max"); + expect(xhighBody.reasoning_effort).toBe(modelId === "deepseek-v4-flash" ? "high" : "max"); expect(xhighBody.messages[1]?.reasoning_content).toBe("I need to inspect files first."); expect(xhighBody.messages[1]).toHaveProperty("tool_calls"); + // Flash honors native `low`; Pro's `low` is upgraded to `high` upstream. + expect(buildBody("low").reasoning_effort).toBe(modelId === "deepseek-v4-flash" ? "low" : "high"); }, );