From a8e1bffc2bc25421d73fba77cafeac3290bc4fa5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:21:10 +0900 Subject: [PATCH 1/5] docs(devlog): roadmap for the 260805 bug-fix stack Four defects from the triage, each with a pre-written diff-level layer doc. Two research findings changed the plan before any code was written. DeepSeek's official table maps xhigh to max on Pro but to high on Flash, so the single shared mapping constant our registry uses cannot be corrected with one edit. And a live probe of all eight Zen free models found two that accept images - including the one a community report claimed refuses them - so the obvious blanket classification would have silently destroyed working vision input. --- devlog/_plan/260805_bug_fix_stack/000_plan.md | 101 +++++++++++++++ .../001_upstream_evidence.md | 92 +++++++++++++ .../002_zen_modality_probe.md | 75 +++++++++++ .../010_deepseek_ladder.md | 121 ++++++++++++++++++ .../260805_bug_fix_stack/020_zen_text_only.md | 109 ++++++++++++++++ .../030_native_profile_harness.md | 103 +++++++++++++++ .../040_startup_app_server.md | 116 +++++++++++++++++ 7 files changed, 717 insertions(+) create mode 100644 devlog/_plan/260805_bug_fix_stack/000_plan.md create mode 100644 devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md create mode 100644 devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md create mode 100644 devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md create mode 100644 devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md create mode 100644 devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md create mode 100644 devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md 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..3977dac5e --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/000_plan.md @@ -0,0 +1,101 @@ +# 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 + +Ordered by file independence, not by size. Three of the four layers touch +disjoint files, so they are **independent branches off `dev`**, not a linear +stack. Only #1057 and #1043 share a file (`src/providers/registry.ts`), and they +touch lines ~1300 apart with no semantic overlap. + +| Layer | Issue | Files | Overlaps | +|-------|-------|-------|----------| +| 010 | #1057 DeepSeek ladder | `src/providers/registry.ts:349-360`, `src/config.ts`, 3 test files | shares registry.ts with 020 | +| 020 | #1043 (+ live half of #1024) | `src/providers/registry.ts:1652`, `tests/vision-sidecar-e2e.test.ts` | shares registry.ts with 010 | +| 030 | #1061 test harness | `tests/native-profile-crash-boundaries.test.ts`, `tests/helpers/native-profile-startup-child.ts` | none | +| 040 | #1046 startup app-server | `src/codex/app-server-processes.ts`, `src/codex/desired-state.ts` | none | + +**Stack shape decision: linear.** Even though only two layers share a file, a +linear stack off one branch keeps the CI story simple and matches the repository's +documented stacked-child workflow in `AGENTS.md`. Each child targets its parent's +head; after the parent lands, the child retargets to `dev`. + +Order: `010 → 020 → 030 → 040`. #1057 goes first because it is the most +self-contained change to `registry.ts`; #1043 then edits a different region of the +same file without conflict. + +## 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..472fa2e64 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md @@ -0,0 +1,75 @@ +# 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. 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..fa005a105 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md @@ -0,0 +1,121 @@ +# 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. +const DEEPSEEK_THINKING_EFFORTS = ["low", "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", +}; +``` + +Note what does *not* change: Pro's `low -> high` stays, because that is what +DeepSeek does. The reporter asked for `low -> low` universally; that is correct +for Flash and wrong for Pro. Advertising `low` is right for both — the ladder is +what the picker shows — but the wire resolution differs, and we follow the vendor. + +### 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 deepseekReasoningMapFor = (modelId: string): Record => + modelId.includes("flash") ? DEEPSEEK_FLASH_REASONING_MAP : DEEPSEEK_PRO_REASONING_MAP; +``` + +### `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`): + +- Replace the exact legacy ladder `["high","xhigh","max"]` with `["low","high","max"]`. +- Replace the exact legacy map only; leave any non-exact user override untouched. +- Apply only where provider name and transport match the registry entry. + +### 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. + +**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:** a per-model assertion that Flash maps `xhigh -> high` while Pro maps +`xhigh -> max`, and a config-migration test for the legacy ladder. + +## 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 + +- Advertised ladder is `["low","high","max"]` for every DeepSeek V4 entry. +- Flash and Pro carry different wire maps, matching the vendor table. +- A saved legacy config normalizes on load; a customized one does not. +- `bun run typecheck` clean; the five 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..5c24697b5 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md @@ -0,0 +1,109 @@ +# 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 }, +``` + +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. It should also widen from `OPENCODE_FREE_DEEPSEEK_MODELS` to the +full measured list, since it serves the same free roster. + +### 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 that would fail if someone later adds `mimo-v2.5-free` to the list. +- 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..bbc616ce1 --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md @@ -0,0 +1,103 @@ +# 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`: + +```ts +const exit = await Promise.race([child.exited, Bun.sleep(timeoutMs).then(() => null)]); +if (exit === null) { child.kill(); await child.exited; throw new Error("startup child did not stop"); } +``` + +Both halves are fixed with 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 is not deterministically testable without injection.** Proving the +timeout branch fires needs a child that deliberately stalls — a test-only env flag +parking before `server.stop(true)`, a short deadline, and an assertion that the +child was killed and the error raised. That is the honest way to satisfy +C-ACTIVATION-GROUNDING-01 here; without it, the bounded wait is present but never +shown firing, and "it no longer hangs" is unfalsifiable in a passing run. + +If the injection proves too invasive for a test helper, the fallback is to state +plainly in the PR that the timeout path is unexercised — not to claim the fix is +verified because the suite is green. + +## 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..785c9509f --- /dev/null +++ b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md @@ -0,0 +1,116 @@ +# 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 async function warnIfStaleCodexAppServersAfterStartupWrite( + opts: { log?: Pick } = {}, +): Promise<{ warned: boolean }> { + try { + const state = await collectCodexAppServerCatalogState(); + if (state.state !== "stale") return { warned: false }; + (opts.log ?? console).error(formatStaleCodexAppServerWarning(state.processes)); + return { warned: true }; + } catch { + return { warned: false }; // startup sync is best-effort; never fail boot + } +} +``` + +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 + +Call it after a startup sync that actually wrote something (`catalogWritten || +cacheSynced`, flags from `src/codex/sync.ts:83-89`). Startup sync is explicitly +best-effort (`:141-155`) and this must not change that. + +### Deliberately out of scope + +`src/server/index.ts:403-412` is the second gap. It is left alone in this layer: +the cache-only invalidation is a different write with a different lifecycle, and +widening the blast radius of a first fix is how a small change becomes unmergeable. +Recorded here so the next round does not think it was missed. + +## 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: + +1. discovery runs only when `catalogWritten || cacheSynced`; +2. stale warns, fresh and `not_running` do not; +3. **an injected `kill` that fails the test if called** — this is the assertion + that matters, because it is the one that would catch a future refactor wiring + the `restart` branch into boot; +4. discovery throwing still resolves startup successfully. + +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 passes before and after by construction — it is a guard, not a +regression proof, and the doc says so rather than counting it twice. + +## Accept criteria + +- Startup warns only when the classifier says `stale`. +- 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. From d54407972a654176c4dc6527e7fef1156ba4dc70 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:32:31 +0900 Subject: [PATCH 2/5] docs(devlog): fold the audit into the bug-fix roadmap Seven blockers, all accepted. Two would have shipped wrong code. Pro must not advertise a low tier DeepSeek silently upgrades to high - the earlier draft fixed the reported defect and reintroduced it at another value. And the startup warning could not observe its own trigger: the sync seam discards its result, and a 5s state cache can serve a pre-write fresh reading back to the post-write check. Also corrects the stack shape. The 010/020 registry edits are two lines apart on the same provider object, not 1300 apart as claimed, while 030 and 040 share nothing and go straight to dev. --- devlog/_plan/260805_bug_fix_stack/000_plan.md | 51 +++++++----- .../002_zen_modality_probe.md | 37 +++++++++ .../010_deepseek_ladder.md | 45 +++++++++-- .../260805_bug_fix_stack/020_zen_text_only.md | 48 ++++++++++- .../030_native_profile_harness.md | 69 ++++++++++++---- .../040_startup_app_server.md | 80 ++++++++++++++----- 6 files changed, 265 insertions(+), 65 deletions(-) diff --git a/devlog/_plan/260805_bug_fix_stack/000_plan.md b/devlog/_plan/260805_bug_fix_stack/000_plan.md index 3977dac5e..93ef6d70b 100644 --- a/devlog/_plan/260805_bug_fix_stack/000_plan.md +++ b/devlog/_plan/260805_bug_fix_stack/000_plan.md @@ -20,26 +20,37 @@ Windows runner this machine does not have, and stays out. ## Layer map -Ordered by file independence, not by size. Three of the four layers touch -disjoint files, so they are **independent branches off `dev`**, not a linear -stack. Only #1057 and #1043 share a file (`src/providers/registry.ts`), and they -touch lines ~1300 apart with no semantic overlap. - -| Layer | Issue | Files | Overlaps | -|-------|-------|-------|----------| -| 010 | #1057 DeepSeek ladder | `src/providers/registry.ts:349-360`, `src/config.ts`, 3 test files | shares registry.ts with 020 | -| 020 | #1043 (+ live half of #1024) | `src/providers/registry.ts:1652`, `tests/vision-sidecar-e2e.test.ts` | shares registry.ts with 010 | -| 030 | #1061 test harness | `tests/native-profile-crash-boundaries.test.ts`, `tests/helpers/native-profile-startup-child.ts` | none | -| 040 | #1046 startup app-server | `src/codex/app-server-processes.ts`, `src/codex/desired-state.ts` | none | - -**Stack shape decision: linear.** Even though only two layers share a file, a -linear stack off one branch keeps the CI story simple and matches the repository's -documented stacked-child workflow in `AGENTS.md`. Each child targets its parent's -head; after the parent lands, the child retargets to `dev`. - -Order: `010 → 020 → 030 → 040`. #1057 goes first because it is the most -self-contained change to `registry.ts`; #1043 then edits a different region of the -same file without conflict. +| 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 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 index 472fa2e64..fd25eda0e 100644 --- a/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md +++ b/devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md @@ -73,3 +73,40 @@ 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 index fa005a105..810dd5d83 100644 --- a/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md +++ b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md @@ -39,7 +39,10 @@ const DEEPSEEK_THINKING_REASONING_MAP: Record = { // 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. -const DEEPSEEK_THINKING_EFFORTS = ["low", "high", "max"]; +// 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", }; @@ -48,10 +51,27 @@ const DEEPSEEK_FLASH_REASONING_MAP: Record = { }; ``` -Note what does *not* change: Pro's `low -> high` stays, because that is what -DeepSeek does. The reporter asked for `low -> low` universally; that is correct -for Flash and wrong for Pro. Advertising `low` is right for both — the ladder is -what the picker shows — but the wire resolution differs, and we follow the vendor. +### 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 @@ -71,10 +91,21 @@ becomes a per-model selection instead of one shared object: 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 => - modelId.includes("flash") ? DEEPSEEK_FLASH_REASONING_MAP : DEEPSEEK_PRO_REASONING_MAP; + 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 @@ -97,6 +128,8 @@ Narrow in-memory normalizer during `loadConfig`, no write from the read path - `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`. 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 index 5c24697b5..b2a693f05 100644 --- a/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md +++ b/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md @@ -63,12 +63,56 @@ Then attach it to the `opencode-zen` entry at `:1652`, which currently has no { 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. It should also widen from `OPENCODE_FREE_DEEPSEEK_MODELS` to the -full measured list, since it serves the same free roster. +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. + +### 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 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 index bbc616ce1..722812e20 100644 --- a/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md +++ b/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md @@ -65,15 +65,44 @@ async function waitForJson(path: string, timeout = 10_000): Promise { 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`: +`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 -const exit = await Promise.race([child.exited, Bun.sleep(timeoutMs).then(() => null)]); -if (exit === null) { child.kill(); await child.exited; throw new Error("startup child did not stop"); } +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"); + throw new Error("startup child did not stop"); + } + if (exit !== 0) throw new Error(await new Response(child.stderr).text()); +} ``` -Both halves are fixed with mechanisms already in the tree. Neither is new -machinery. +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 @@ -82,16 +111,26 @@ machinery. instead of throwing `Unexpected EOF`. That test fails against the old existence-only `waitFor` by construction. -**The hang is not deterministically testable without injection.** Proving the -timeout branch fires needs a child that deliberately stalls — a test-only env flag -parking before `server.stop(true)`, a short deadline, and an assertion that the -child was killed and the error raised. That is the honest way to satisfy -C-ACTIVATION-GROUNDING-01 here; without it, the bounded wait is present but never -shown firing, and "it no longer hangs" is unfalsifiable in a passing run. - -If the injection proves too invasive for a test helper, the fallback is to state -plainly in the PR that the timeout path is unexercised — not to claim the fix is -verified because the suite is green. +**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. +- 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 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 index 785c9509f..42fe95713 100644 --- a/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md +++ b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md @@ -67,18 +67,39 @@ It never reaches `restartCodexAppServers()`. That is the whole point. 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 - -Call it after a startup sync that actually wrote something (`catalogWritten || -cacheSynced`, flags from `src/codex/sync.ts:83-89`). Startup sync is explicitly -best-effort (`:141-155`) and this must not change that. - -### Deliberately out of scope - -`src/server/index.ts:403-412` is the second gap. It is left alone in this layer: -the cache-only invalidation is a different write with a different lifecycle, and -widening the blast radius of a first fix is how a small change becomes unmergeable. -Recorded here so the next round does not think it was missed. +### `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. + +Both writes route through the same post-write, stale-only decision, deduplicated +so a startup that hits both paths warns once. ## Tests @@ -88,14 +109,24 @@ exhaustive read-only scan across `origin/dev`'s `tests/` for files mentioning bo `afterCatalogWriteHandleAppServers|collectCodexAppServerCatalogState`. Result: empty. -**Add**, faking both boundaries: - -1. discovery runs only when `catalogWritten || cacheSynced`; -2. stale warns, fresh and `not_running` do not; -3. **an injected `kill` that fails the test if called** — this is the assertion - that matters, because it is the one that would catch a future refactor wiring - the `restart` branch into boot; -4. discovery throwing still resolves startup successfully. +**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. + +For (4), the helper must accept the process-I/O seam rather than closing over +module state — the audit noted the earlier draft promised this assertion while +proposing a helper with nothing to inject into. 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` @@ -104,12 +135,17 @@ Existing coverage to leave intact: `tests/codex-app-server-processes.test.ts:19- ## Red-green Test 2 fails on the pre-fix tree: startup emits no warning at all for a stale -app-server. Test 3 passes before and after by construction — it is a guard, not a -regression proof, and the doc says so rather than counting it twice. +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. From 1eab6354923b822b606a588d8f0ff903fb077f54 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:39:56 +0900 Subject: [PATCH 3/5] docs(devlog): second audit round on the bug-fix roadmap The migration would have handed Pro back the low tier the registry change removes - the same defect, reintroduced through the upgrade path for existing users only. Migration is now per model like the registry, and the test is that a migrated config equals a fresh install. Layer 040's helper now takes the real CodexAppServerProcessIo seam, which already carries kill, listSnapshots, readStartMs and catalogMtimeMs. The draft promised injection while showing a helper with nothing to inject into, awaited a synchronous function, and read a field named state as status. 030 now awaits the SIGKILL escalation before throwing, so the caller's exitCode assertion is not racing the reap. --- .../010_deepseek_ladder.md | 42 ++++++++++++++----- .../260805_bug_fix_stack/020_zen_text_only.md | 18 +++++++- .../030_native_profile_harness.md | 11 ++++- .../040_startup_app_server.md | 42 +++++++++++++++---- 4 files changed, 92 insertions(+), 21 deletions(-) diff --git a/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md index 810dd5d83..b9692d01d 100644 --- a/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md +++ b/devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md @@ -115,12 +115,23 @@ 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`): - -- Replace the exact legacy ladder `["high","xhigh","max"]` with `["low","high","max"]`. -- Replace the exact legacy map only; leave any non-exact user override untouched. +(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):** @@ -138,8 +149,16 @@ Both now expect **different** values per model, not one shared array. - `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:** a per-model assertion that Flash maps `xhigh -> high` while Pro maps -`xhigh -> max`, and a config-migration test for the legacy ladder. +**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 @@ -148,7 +167,10 @@ share one map. Ablating the Flash map alone flips that single assertion. ## Accept criteria -- Advertised ladder is `["low","high","max"]` for every DeepSeek V4 entry. -- Flash and Pro carry different wire maps, matching the vendor table. -- A saved legacy config normalizes on load; a customized one does not. -- `bun run typecheck` clean; the five affected test files pass. +- 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 index b2a693f05..4c2c0ab68 100644 --- a/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md +++ b/devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md @@ -97,6 +97,20 @@ 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 @@ -148,6 +162,8 @@ that is not in the registry, and stays open. ## Accept criteria - Six measured IDs listed; the two vision-capable ones explicitly not. -- A test that would fail if someone later adds `mimo-v2.5-free` to the list. +- 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 index 722812e20..ddfb0d572 100644 --- a/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md +++ b/devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md @@ -92,7 +92,12 @@ async function stopStartup( child.exited, Bun.sleep(KILL_GRACE_MS).then(() => null), ]); - if (killed === null) child.kill("SIGKILL"); + 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()); @@ -126,6 +131,10 @@ for it rather than a description: 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. 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 index 42fe95713..aba5407de 100644 --- a/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md +++ b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md @@ -47,13 +47,19 @@ newer than the catalog and perfectly correct. A startup-safe helper beside the existing handler: ```ts -export async function warnIfStaleCodexAppServersAfterStartupWrite( - opts: { log?: Pick } = {}, -): Promise<{ warned: boolean }> { +export function warnIfStaleCodexAppServersAfterStartupWrite( + opts: { + log?: Pick; + io?: CodexAppServerProcessIo; // the real seam, src/codex/app-server-processes.ts:79-89 + } = {}, +): { warned: boolean } { try { - const state = await collectCodexAppServerCatalogState(); - if (state.state !== "stale") return { warned: false }; - (opts.log ?? console).error(formatStaleCodexAppServerWarning(state.processes)); + // 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 @@ -61,6 +67,25 @@ export async function warnIfStaleCodexAppServersAfterStartupWrite( } ``` +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` @@ -124,9 +149,8 @@ new injection point is invented: 5. discovery throwing still resolves startup successfully; 6. a startup hitting both write paths warns exactly once. -For (4), the helper must accept the process-I/O seam rather than closing over -module state — the audit noted the earlier draft promised this assertion while -proposing a helper with nothing to inject into. +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` From 7bf6f0575a4cacc3c32a848493beafedca8e0fc1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:42:05 +0900 Subject: [PATCH 4/5] docs(devlog): name the owner of the startup stale warning Two startup write sites would each call the helper and warn twice. handleStart now owns the single call: startServer returns whether its cache invalidation wrote, syncCodexOnStartIfEnabled returns its typed result, neither warns, and handleStart ORs the flags and warns once after both. The ordering is correctness, not tidiness - warning after the first write reads a catalog mtime the second write is about to move. --- .../040_startup_app_server.md | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) 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 index aba5407de..e621f8ebd 100644 --- a/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md +++ b/devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md @@ -123,8 +123,41 @@ The audit rejected that, correctly: this path invalidates `models_cache.json` on integration disabled, or when the sync writes nothing, startup still leaves a running app-server stale and this layer would have shipped silence. -Both writes route through the same post-write, stale-only decision, deduplicated -so a startup that hits both paths warns once. +### 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 @@ -148,6 +181,7 @@ new injection point is invented: 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`. From 45ac9ebf873943011aa991d0f9cf1c6ba3db3cf1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 6 Aug 2026 00:44:51 +0900 Subject: [PATCH 5/5] fix(deepseek): advertise the ladder each V4 model actually honors (#1057) DeepSeek documents low/high/max, and the two V4 models resolve it differently: requested xhigh becomes max on Pro but high on Flash, and requested low is honored natively on Flash while Pro upgrades it to high. We advertised [high, xhigh, max] for both and mapped low to high for both. So native low was unreachable, and xhigh sat in the picker as though it were a tier rather than an alias for something else. Both are now per model. Flash advertises low/high/max, Pro advertises high/max - Pro must not offer a low tier the vendor silently bills as high, which would be this same defect wearing a different value. xhigh stays in both wire maps so existing requests and saved configs keep working; it is simply no longer advertised as native. Seven provider entries share these constants, so a parity test now enumerates every provider/model pair and its expected ladder and alias mapping. The Flash-vs-Pro split is a substring test on the model id, correct for every id shipped today and exactly the kind of thing that misfires on a future name. Source: api-docs.deepseek.com/guides/thinking_mode, EN and zh-cn agree, re-verified 2026-08-06 immediately before this change. --- src/providers/registry.ts | 85 ++++++++++++++++++++------ tests/opencode-go-deepseek.test.ts | 4 +- tests/provider-registry-parity.test.ts | 58 +++++++++++++++++- tests/volcengine-providers.test.ts | 16 +++-- 4 files changed, 137 insertions(+), 26 deletions(-) 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"); }, );