fix(deepseek): advertise the ladder each V4 model actually honors (#1057) - #1069
Conversation
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.
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.
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.
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.
) 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.
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughChangesBug-fix stack
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260805_bug_fix_stack/000_plan.md`:
- Line 45: Add the text language tag to the opening fenced code blocks at
devlog/_plan/260805_bug_fix_stack/000_plan.md lines 45-45 and
devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md lines 50-50 by
changing each fence to ```text.
- Around line 76-82: Update the DeepSeek requirement in the plan to state the
definitive contract: Flash advertises low/high/max, while Pro advertises
high/max, with compatibility mappings kept model-specific for both low and
xhigh. Remove the provisional confirmation language and ensure the requirement
prevents Pro from advertising or mapping low as supported.
In `@devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md`:
- Around line 8-9: Correct the observation date in the source evidence entry to
the actual collection date, or explicitly mark it as planned rather than
confirmed. Update the timing language in the corresponding present-tense
statements at lines 34-38 so it consistently reflects the corrected evidence
status.
In `@devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md`:
- Around line 15-20: Make the probe command runnable by defining the model
variable or adding the intended model loop before the curl request, and replace
the abbreviated PNG data URL with the complete valid 1x1 payload. If the snippet
is intentionally pseudocode, label it accordingly and provide a separate command
with a defined model and complete image data.
- Line 6: Update the paragraph in 002_zen_modality_probe.md so the reference is
written as “issue `#1043`” or otherwise escapes the hash, keeping it as paragraph
text and preventing Markdown from interpreting it as an ATX heading.
In `@devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md`:
- Around line 38-43: Replace the future verification date with the actual date
the vendor check was completed: update
devlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.md lines 38-43, 71-74, and
170-174; the DeepSeek registry entry in src/providers/registry.ts lines 352-375;
the date assertion in tests/opencode-go-deepseek.test.ts lines 108-110; and both
parity references in tests/provider-registry-parity.test.ts lines 111-114 and
945-948. Keep the surrounding ladder documentation, registry metadata, and test
expectations unchanged.
In `@devlog/_plan/260805_bug_fix_stack/020_zen_text_only.md`:
- Around line 131-142: Extend the acceptance tests to cover both opencode-zen
and opencode-free entries: assert all six measured IDs are present and
mimo-v2.5-free plus longcat-2.0-free are absent for each registry entry. Add
activation coverage for both provider routes, or explicitly verify they consume
the same derived configuration; do not rely on separate key-provider derivation
as coverage for opencode-free.
In `@devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md`:
- Around line 114-117: Update the waitForJson partial-write test to synchronize
on an observed parse failure before replacing the file atomically. Add a barrier
or injectable read hook that confirms the waiter has attempted to parse the
partial "{" content and failed, then perform the replacement and assert the
parsed object is returned; do not use a fixed sleep for synchronization.
- Around line 144-150: Update the “Accept criteria” section to require an
injected-stall test proving the teardown timeout fires; remove the option to
substitute an unexercised-branch statement. Allow that statement only for the
final post-SIGKILL timeout branch.
- Around line 124-126: Add blank lines immediately before and after the nested
TypeScript code fence in the documented OCX_TEST_STALL_ON_STOP snippet,
preserving the snippet’s contents and formatting otherwise.
- Around line 131-135: Update the cleanup assertion in the startup-child
termination flow to verify the subprocess has exited and that either
child.exitCode or child.signalCode is non-null, allowing intentional SIGKILL
termination to satisfy the check. Preserve the existing awaited SIGKILL cleanup
and rejection-message assertions.
In `@devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md`:
- Around line 50-67: Update handleStart and its production warning call to pass
a non-default CodexAppServerProcessIo seam into
warnIfStaleCodexAppServersAfterStartupWrite, ensuring
collectCodexAppServerCatalogState bypasses the five-second memo after the
confirmed startup write. Reuse the existing process I/O seam rather than
invalidating the cache or changing unrelated startup behavior.
- Around line 158-184: Expand the startup test coverage around handleStart to
exercise the cacheWritten || syncWritten gate: add separate cases for cache-only
stale startup, sync-only stale startup, both writes, and neither write. Assert
exactly one warning for each case where either path writes, and zero warnings
when neither path writes, using the injected log and existing
CodexAppServerProcessIo seam.
- Around line 97-106: Update CodexStartupSync and defaultStartupSync to return
the structured CodexSyncResult from syncModelsToCodex instead of a boolean. In
syncCodexOnStartIfEnabled, preserve best-effort startup behavior and the
existing catch, but return a distinct skipped, successful-write, or failed
result so callers can evaluate the catalogWritten || cacheSynced gate.
- Around line 170-187: Update handleStart and the startup sync path so the
test-seam lifetime/dependency object is propagated into
afterCatalogWriteHandleAppServers. For the warning-only startup run, pass
restart: false and the injected CodexAppServerProcessIo implementation,
including listSnapshots, readStartMs, catalogMtimeMs, now, and a kill function
that fails if called; preserve the existing sync and sync-cache handling while
ensuring no path invokes restartCodexAppServers or process.kill.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83fa74b3-5edf-42f6-8b55-9a6e4c2174c5
📒 Files selected for processing (11)
devlog/_plan/260805_bug_fix_stack/000_plan.mddevlog/_plan/260805_bug_fix_stack/001_upstream_evidence.mddevlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.mddevlog/_plan/260805_bug_fix_stack/010_deepseek_ladder.mddevlog/_plan/260805_bug_fix_stack/020_zen_text_only.mddevlog/_plan/260805_bug_fix_stack/030_native_profile_harness.mddevlog/_plan/260805_bug_fix_stack/040_startup_app_server.mdsrc/providers/registry.tstests/opencode-go-deepseek.test.tstests/provider-registry-parity.test.tstests/volcengine-providers.test.ts
| nothing and cost two retargets after the parents land. They go straight to `dev` | ||
| as independent PRs, which `AGENTS.md` permits alongside stacked children. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add language tags to both fenced blocks.
markdownlint-cli2 reports MD040 for both blocks. Add text to each opening fence.
devlog/_plan/260805_bug_fix_stack/000_plan.md#L45-L45: change the opening fence to```text.devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md#L50-L50: change the opening fence to```text.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
devlog/_plan/260805_bug_fix_stack/000_plan.md#L45-L45(this comment)devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md#L50-L50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/000_plan.md` at line 45, Add the text
language tag to the opening fenced code blocks at
devlog/_plan/260805_bug_fix_stack/000_plan.md lines 45-45 and
devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md lines 50-50 by
changing each fence to ```text.
Source: Linters/SAST tools
| **#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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the DeepSeek requirement definitive and complete.
Lines 80-82 still say that confirmation is running and identify only an xhigh split. devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md Lines 23-32 also confirm a low split: Flash maps low to low, while Pro maps low to high.
State the final contract here: Flash advertises low/high/max; Pro advertises high/max; compatibility mappings remain model-specific. Otherwise, a later patch can fix only xhigh and still advertise low for Pro. The official table shows these model-specific mappings. (api-docs.deepseek.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/000_plan.md` around lines 76 - 82, Update
the DeepSeek requirement in the plan to state the definitive contract: Flash
advertises low/high/max, while Pro advertises high/max, with compatibility
mappings kept model-specific for both low and xhigh. Remove the provisional
confirmation language and ensure the requirement prevents Pro from advertising
or mapping low as supported.
| 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the evidence date before treating this source as confirmed.
Line 9 records August 6, 2026, but the current review date is August 5, 2026. The document cannot report an observation from tomorrow. Lines 34-38 also use present-tense timing based on that record.
Replace the date with the actual collection date, or mark the observation as planned. Align the timing language before using this document as upstream evidence.
Also applies to: 34-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/001_upstream_evidence.md` around lines 8 -
9, Correct the observation date in the source evidence entry to the actual
collection date, or explicitly mark it as planned rather than confirmed. Update
the timing language in the corresponding present-tense statements at lines 34-38
so it consistently reflects the corrected evidence status.
| 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep #1043 inside the paragraph.
Line [6] starts with #1043. Markdownlint reports MD018 for this malformed ATX heading. Write issue #1043`` or escape the hash so the text remains part of the paragraph.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 6-6: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md` at line 6,
Update the paragraph in 002_zen_modality_probe.md so the reference is written as
“issue `#1043`” or otherwise escapes the hash, keeping it as paragraph text and
preventing Markdown from interpreting it as an ATX heading.
Source: Linters/SAST tools
| ```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]}]}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the probe command reproducible.
The command uses an undefined $m, and the PNG data URL contains .... As written, it sends an empty model name and invalid image data. Define the model before the request or add the model loop. Replace the abbreviated data URL with the complete 1x1 PNG payload. If this is only pseudocode, label it as such and provide a separate runnable command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/002_zen_modality_probe.md` around lines 15
- 20, Make the probe command runnable by defining the model variable or adding
the intended model loop before the curl request, and replace the abbreviated PNG
data URL with the complete valid 1x1 payload. If the snippet is intentionally
pseudocode, label it accordingly and provide a separate command with a defined
model and complete image data.
| ## 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Require the injected timeout test in the acceptance criteria.
Lines 141-142 state that the bounded wait is unfalsifiable without the injected-stall case, but line 150 permits an explicit statement instead. That exception allows the hang fix to merge without proof that the deadline fires. Keep the injected-stall test mandatory. Limit the “unexercised” note to the final post-SIGKILL timeout branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/030_native_profile_harness.md` around lines
144 - 150, Update the “Accept criteria” section to require an injected-stall
test proving the teardown timeout fires; remove the option to substitute an
unexercised-branch statement. Allow that statement only for the final
post-SIGKILL timeout branch.
| export function warnIfStaleCodexAppServersAfterStartupWrite( | ||
| opts: { | ||
| log?: Pick<Console, "error">; | ||
| io?: CodexAppServerProcessIo; // the real seam, src/codex/app-server-processes.ts:79-89 | ||
| } = {}, | ||
| ): { warned: boolean } { | ||
| try { | ||
| // Pass `io` through: tests inject listSnapshots/readStartMs/catalogMtimeMs/now, | ||
| // and supplying any field also bypasses the 5s memo (`fullyDefault` at :580), | ||
| // which is what stops a pre-write `fresh` reading from masking this check. | ||
| const status = collectCodexAppServerCatalogState(opts.io ?? {}); | ||
| if (status.state !== "stale") return { warned: false }; | ||
| (opts.log ?? console).error(formatStaleCodexAppServerWarning(status.processes)); | ||
| return { warned: true }; | ||
| } catch { | ||
| return { warned: false }; // startup sync is best-effort; never fail boot | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Pass a non-default process I/O object to the production warning call.
collectCodexAppServerCatalogState uses the memo when every CodexAppServerProcessIo field is defaulted, as shown in src/codex/app-server-processes.ts, Lines [576-585]. The production call in Lines [146-147] invokes warnIfStaleCodexAppServersAfterStartupWrite() without options. It can therefore reuse a pre-write fresh result for five seconds.
This violates the cache regression requirement in Lines [177-179] and acceptance criterion in Line [205]. Thread the existing I/O seam into handleStart and pass a non-empty production seam, or explicitly invalidate the cache after the final confirmed write.
Also applies to: 140-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md` around lines 50
- 67, Update handleStart and its production warning call to pass a non-default
CodexAppServerProcessIo seam into warnIfStaleCodexAppServersAfterStartupWrite,
ensuring collectCodexAppServerCatalogState bypasses the five-second memo after
the confirmed startup write. Reuse the existing process I/O seam rather than
invalidating the cache or changing unrelated startup behavior.
| **The sync result is currently thrown away.** `defaultStartupSync` returns | ||
| `syncModelsToCodex(port)` as `Promise<unknown>`, 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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\b(syncModelsToCodex|CodexStartupSync|syncCodexOnStartIfEnabled)\b' src testsRepository: lidge-jun/opencodex
Length of output: 50375
No change needed. syncModelsToCodex already returns a structured CodexSyncResult; CodexStartupSync can be typed to return that result, preserving the existing .catch(() => {}) behavior while making write/write-failure states available to callers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md` around lines 97
- 106, Update CodexStartupSync and defaultStartupSync to return the structured
CodexSyncResult from syncModelsToCodex instead of a boolean. In
syncCodexOnStartIfEnabled, preserve best-effort startup behavior and the
existing catch, but return a distinct skipped, successful-write, or failed
result so callers can evaluate the catalogWritten || cacheSynced gate.
| **Test:** `tests/codex-desired-state.test.ts` — a startup where both the cache | ||
| invalidation and the sync report a write emits exactly one warning; asserted on | ||
| the injected `log.error` call count, not on the message. | ||
|
|
||
| ## Tests | ||
|
|
||
| No test currently combines startup sync with app-server handling — verified by an | ||
| exhaustive read-only scan across `origin/dev`'s `tests/` for files mentioning both | ||
| `syncCodexOnStartIfEnabled|handleStart` and | ||
| `afterCatalogWriteHandleAppServers|collectCodexAppServerCatalogState`. Result: | ||
| empty. | ||
|
|
||
| **Add**, faking both boundaries through the existing `CodexAppServerProcessIo` | ||
| seam (`io.listSnapshots`, `io.readStartMs`, `io.catalogMtimeMs`, `io.now`) so no | ||
| new injection point is invented: | ||
|
|
||
| 1. the warning runs only when the sync reports a write — requires the typed | ||
| result from the seam fix above; | ||
| 2. stale warns; `fresh`, `not_running`, and `unknown` do not; | ||
| 3. **pre-write `fresh` then post-write `stale` still warns** — this is the | ||
| cache-masking regression, and it is the test that would have caught the bug the | ||
| audit found; | ||
| 4. **an injected `kill` that fails the test if called**, proving no startup path | ||
| can reach `restartCodexAppServers()`; | ||
| 5. discovery throwing still resolves startup successfully; | ||
| 6. a startup hitting both write paths warns exactly once. | ||
| Asserted through `handleStart`'s injected log, per the ownership rule above. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test both sides of the startup write gate.
Line [174] says the warning runs only when the sync reports a write. Lines [146-147] define the gate as cacheWritten || syncWritten, and Lines [118-125] include cache-only startup writes in the defect scope.
Add separate cases for cache-only stale startup, sync-only stale startup, both writes, and neither write. Assert one warning for each write case and zero warnings when neither path writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md` around lines 158
- 184, Expand the startup test coverage around handleStart to exercise the
cacheWritten || syncWritten gate: add separate cases for cache-only stale
startup, sync-only stale startup, both writes, and neither write. Assert exactly
one warning for each case where either path writes, and zero warnings when
neither path writes, using the injected log and existing CodexAppServerProcessIo
seam.
| **Add**, faking both boundaries through the existing `CodexAppServerProcessIo` | ||
| seam (`io.listSnapshots`, `io.readStartMs`, `io.catalogMtimeMs`, `io.now`) so no | ||
| new injection point is invented: | ||
|
|
||
| 1. the warning runs only when the sync reports a write — requires the typed | ||
| result from the seam fix above; | ||
| 2. stale warns; `fresh`, `not_running`, and `unknown` do not; | ||
| 3. **pre-write `fresh` then post-write `stale` still warns** — this is the | ||
| cache-masking regression, and it is the test that would have caught the bug the | ||
| audit found; | ||
| 4. **an injected `kill` that fails the test if called**, proving no startup path | ||
| can reach `restartCodexAppServers()`; | ||
| 5. discovery throwing still resolves startup successfully; | ||
| 6. a startup hitting both write paths warns exactly once. | ||
| Asserted through `handleStart`'s injected log, per the ownership rule above. | ||
|
|
||
| For (4), inject `io.kill` and fail the test if it is ever called. The seam exists | ||
| at `src/codex/app-server-processes.ts:84`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 '\b(handleStart|warnIfStaleCodexAppServersAfterStartupWrite|CodexAppServerProcessIo)\b' src testsRepository: lidge-jun/opencodex
Length of output: 23641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning syncCodexOnStartIfEnabled / afterCatalogWrite / handleStart =="
rg -n -C 8 '\bsyncCodexOnStartIfEnabled\b|\bafterCatalogWrite|warnIfStale|handleStart\(' src tests devlog || true
echo
echo "== src/cli/index.ts handleStart relevant section =="
sed -n '170,280p' src/cli/index.ts
echo
echo "== targeted static call graph for collectCodexAppServerCatalogState/restartCodexAppServers/AfterCatalogWrite =="
python3 - <<'PY'
from pathlib import Path
for p in Path('src').rglob('*.ts'):
s=p.read_text(errors='ignore')
if 'collectCodexAppServerCatalogState' in s or 'warnIfStaleCodexAppServers' in s or 'AfterCatalogWriteAppServerOptions' in s or 'restartCodexAppServers' in s:
print(f'--- {p}')
lines=s.splitlines()
for i,l in enumerate(lines,1):
if any(x in l for x in ['collectCodexAppServerCatalogState', 'warnIfStaleCodexAppServersAfterStartupWrite', 'function warnIfStale', 'AfterCatalogWriteAppServerOptions', 'restartCodexAppServers']) or any(x in l for x in ['from "../codex/app-server-processes"','function warn', 'await warn', 'collectCodexAppServerCatalogState', 'warnIfStaleCodexAppServers']):
start=max(1,i-4); end=min(len(lines),i+6)
for j in range(start,end+1):
print(f'{j:4}: {lines[j-1]}')
PYRepository: lidge-jun/opencodex
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md tail =="
sed -n '80,210p' devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md
echo
echo "== afterCatalogWriteHandleAppServers implementation =="
sed -n '712,770p' src/codex/app-server-processes.ts
echo
echo "== handleStart calls involving syncCodexOnStartIfEnabled / catalog writes / afterCatalogWrite =="
sed -n '300,330p' src/cli/index.ts
sed -n '848,862p' src/cli/index.ts
sed -n '877,881p' src/cli/index.ts
echo
echo "== source-level contract check =="
python3 - <<'PY'
from pathlib import Path
cli = Path("src/cli/index.ts").read_text()
plan = Path("devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md").read_text()
for needle in ["syncCodexOnStartIfEnabled", "handleStart({", "syncCodexOnStartIfEnabled("]:
print(f"{needle}: {cli.index(needle) if needle in cli else 'ABSSENT'}")
handler_start=cli.index("afterCatalogWriteHandleAppServers({ restart: restartCodex, log: console });")
handler_start2=cli.index("afterCatalogWriteHandleAppServers({ restart: restartCodex, log: console });", handler_start+1)
print(f"handler option keys near sync: {cli[handler_start-80:handler_start+60]}")
print(f"handler option keys near sync-cache: {cli[handler_start2-80:handler_start2+60]}")
print(f"plan mentions startSafe handler/option: {'startup-safe' in plan and 'startup: restart: false' in plan}")
print(f"plan mentions start hook call: {'afterCatalogWriteHandleAppServers({ restart: false, log: console, io:' in plan or 'handleStart' in plan[plan.index('# Add'):plan.index('For (4)')] if '# Add' in plan else 'NO_ADD_HEAD'}")
PYRepository: lidge-jun/opencodex
Length of output: 12167
Wire handleStart to the stale-app-server test seams.
handleStart() calls syncCodexOnStartIfEnabled(port, config) and then the existing sync / sync-cache case calls afterCatalogWriteHandleAppServers({ restart: restartCodex, log: console }) with no injectable process I/O. As written, afterCatalogWriteHandleAppServers() can still call process.kill if restartCodex becomes true, so the injected kill guard cannot prove the startup path is safe. Add a test-seam lifetime/dependency object so the warning-only startup run passes restart: false plus a CodexAppServerProcessIo with injected listSnapshots, readStartMs, catalogMtimeMs, now, and a failing kill.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260805_bug_fix_stack/040_startup_app_server.md` around lines 170
- 187, Update handleStart and the startup sync path so the test-seam
lifetime/dependency object is propagated into afterCatalogWriteHandleAppServers.
For the warning-only startup run, pass restart: false and the injected
CodexAppServerProcessIo implementation, including listSnapshots, readStartMs,
catalogMtimeMs, now, and a kill function that fails if called; preserve the
existing sync and sync-cache handling while ensuring no path invokes
restartCodexAppServers or process.kill.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45ac9ebf87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| 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)])), |
There was a problem hiding this comment.
This updates only the registry defaults, but existing users who added deepseek, opencode-go, Volcengine Coding Plan, etc. before this release have the old modelReasoningEfforts and modelReasoningEffortMap persisted in config.json from providerConfigSeed() / key login. routedProviderConfig() then lets those saved per-model arrays replace the new registry arrays, and the saved nested map keys override the new map keys, so an upgraded deepseek-v4-flash provider still advertises xhigh and still sends low -> high / xhigh -> max. Please add a config/backfill migration or make these built-in provider facts canonical so the fix applies to already-installed providers, not only freshly added ones.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| */ | ||
| 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)])), |
There was a problem hiding this comment.
This updates only the registry defaults, but existing users who added deepseek, opencode-go, Volcengine Coding Plan, etc. before this release have the old modelReasoningEfforts and modelReasoningEffortMap persisted in config.json from providerConfigSeed() / key login. routedProviderConfig() then lets those saved per-model arrays replace the new registry arrays, and the saved nested map keys override the new map keys, so an upgraded deepseek-v4-flash provider still advertises xhigh and still sends low -> high / xhigh -> max. Please add a config/backfill migration or make these built-in provider facts canonical so the fix applies to already-installed providers, not only freshly added ones.
Useful? React with 👍 / 👎.
1 participant
Summary
DeepSeek documents a
low / high / maxladder, and the two V4 models resolve it differently. From the official thinking-mode table (EN and zh-cn agree, re-verified 2026-08-06 immediately before this change):deepseek-v4-flashdeepseek-v4-prolowlowhighhighhighhighxhighhighmaxmaxmaxmaxWe advertised
["high","xhigh","max"]for both and mappedlow -> highfor both. So nativelowwas unreachable, andxhighsat in the picker as though it were a tier rather than an alias for something else.Both the ladder and the map are now per model:
low/high/max— it honors all three.high/max. It must not offer alowtier the vendor silently upgrades tohigh; that would be this same defect wearing a different value, and the reporter's requestedlow -> lowis correct for Flash but wrong for Pro.xhighstays in both wire maps so existing requests and saved configs keep working. It is simply no longer advertised as native, which is what the issue actually asks for.mediumhas no row in the vendor table. Mapping it tohighis our own compatibility choice for clients that only speak the OpenAI ladder, and the code now says so.Seven provider entries share these constants (
opencode-go,orcarouter,deepseek,volcengine-coding-plan, both Alibaba token plans,opencode-free), so all seven move together. The Flash-vs-Pro split is a substring test on the model id — correct for every id shipped today, includingdeepseek/deepseek-v4-proanddeepseek-v4-flash-free, and exactly the kind of thing that misfires on a future name. A parity test therefore enumerates every provider/model pair with its expected ladder and alias mapping, so a misclassification fails loudly with the offending id named.Three existing tests changed. All three asserted the defect itself (the advertised
["high","xhigh","max"]and a sharedxhigh -> max); the alias assertions that were testing compatibility rather than the defect are untouched.Closes #1057.
Verification
Red-green: disabling the Flash classification (
isDeepseekFlashModelforced tofalse) fails 2 tests, including the new enumeration; restoring it returns 34 pass / 0 fail.Full suite on this branch: 9015 pass / 1 fail. The one failure is
jawcode-metadata-sync, which fails identically onorigin/devand is unrelated to this change.Source: https://api-docs.deepseek.com/guides/thinking_mode/
Checklist
Summary by CodeRabbit
New Features
lowandxhigh.Bug Fixes
Documentation
Tests