fix(providers): replay DeepSeek reasoning for opencode-zen (#994) - #1068
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesOpenCode Zen DeepSeek reasoning support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🤖 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 `@tests/opencode-zen-deepseek-reasoning.test.ts`:
- Around line 75-80: Add focused image-request regression coverage near the
existing tests in tests/opencode-zen-deepseek-reasoning.test.ts for
deepseek-v4-pro, deepseek-v4-flash, and deepseek-v4-flash-free. Exercise the
noVisionModels configuration from the provider registry and assert each model
follows its expected no-vision or vision-sidecar path, while preserving the
existing reasoning replay tests.
🪄 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: 97e90f8b-4585-4bad-8c33-eb36766bf9fa
📒 Files selected for processing (2)
src/providers/registry.tstests/opencode-zen-deepseek-reasoning.test.ts
|
The registry slice looks right: the missing |
|
The replay bug you are fixing is real, and the diagnosis is right. There is one line I have to block on, and it is easy to miss because it looks like an addition rather than a deletion.
// #1043: without this the proxy forwards image parts to text-only Zen models and
// the upstream rejects the whole request with a 400.
noVisionModels: OPENCODE_ZEN_TEXT_ONLY_MODELS,Your diff sets Five models stop being marked text-only, so the proxy resumes forwarding image parts to them and the gateway 400s the whole request. That is #1043 reopening. The list is not a guess either — it came out of a modality probe recorded in Fix is a one-liner: union the two rather than replace. noVisionModels: [
...OPENCODE_ZEN_TEXT_ONLY_MODELS,
...DEEPSEEK_THINKING_MODELS,
...OPENCODE_FREE_DEEPSEEK_MODELS,
],Your new test asserting Second item: the effort constants moved under you. The branch uses generic DeepSeek effort constants, but On the failing check: the So: keep the replay metadata and the test, union the vision list, rebase for the per-model effort helpers. The replay half of this is work we want, and #994 is waiting on it. |
f377088 to
ac63b73
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/opencode-zen-deepseek-reasoning.test.ts`:
- Around line 82-95: Add a positive test alongside the existing noVisionModels
coverage using routeModel(configFor("big-pickle"), "opencode-zen/big-pickle"),
and assert that the resulting provider.noVisionModels contains "big-pickle".
🪄 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: b6d1f07b-e32a-422d-a4af-2f6b09c0c376
📒 Files selected for processing (2)
src/providers/registry.tstests/opencode-zen-deepseek-reasoning.test.ts
8efa6cd to
cb5641d
Compare
cb5641d to
957990a
Compare
[GD] Verdict: gatedTLDR
Full verdictSemantic propagation
Linked: #994 (this PR's issue), #950/#971 (replay history), #1043 (Zen text-only list), #1057 (per-model DeepSeek ladder). UsefulnessReal, user-visible bug. opencode-zen is the same gateway as opencode-free but lacked the DeepSeek reasoning metadata, so a tool-call continuation in DeepSeek thinking mode dropped Bugs / correctness
oVisionModels replaced the base OPENCODE_ZEN_TEXT_ONLY_MODELS with only the DeepSeek set — this would have reopened #1043 (five measured text-only models lose image stripping). Corrected to the union [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS].
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Simplify was explicitly requested, but this PR is foreign, so nothing was edited or pushed beyond the rebase/fix commits above. Bounded candidates for the owner to consider (optional; not blockers):
GateDraft (GitHub isDraft: true, set by the opencodex-pr-gate bot on the new head). Not WIP/do-not-merge by title or label. Ship-gate result: �locked — Bottom lineThe fix is useful, correct, and now rebased with all maintainer-requested code changes landed and verified (union vision list, per-model effort helpers, �ig-pickle coverage). The PR is draft-gated: it cannot be merge-ready until the owner (1) adds the end-to-end Claude /v1/messages continuation regression the maintainer asked for, and (2) clears the draft via the readiness checklist. No security or spec blockers. |
|
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata =="
gh pr view 1068 --repo lidge-jun/opencodex --json headRefOid,isDraft,mergeStateStatus,statusCheckRollup \
--jq '{headRefOid, isDraft, mergeStateStatus, checks: [.statusCheckRollup[] | {name, conclusion, status}]}'
echo
echo "== Candidate test files =="
fd -t f . tests | rg '(claude|messages|reasoning|replay|zen).*\.test\.(ts|tsx)$' || true
echo
echo "== Test structure =="
for f in tests/opencode-zen-deepseek-reasoning.test.ts tests/deepseek-reasoning-replay.test.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all --type function --view expanded || true
fi
done
echo
echo "== Claude Messages continuation / thinking replay references =="
rg -n -i -C 3 '(\/v1\/messages|claude.*continuation|continuation.*claude|thinking.*replay|replay.*thinking)' tests src || trueLength of output: 65653
I will review the current ready-for-review head.
|
![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: 957990a91c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| [...DEEPSEEK_THINKING_MODELS, ...OPENCODE_FREE_DEEPSEEK_MODELS].map(id => [id, deepseekThinkingEffortsFor(id)]), | ||
| ), | ||
| modelReasoningEffortMap: Object.fromEntries( | ||
| [...DEEPSEEK_THINKING_MODELS, ...OPENCODE_FREE_DEEPSEEK_MODELS].map(id => [id, deepseekReasoningMapFor(id)]), |
There was a problem hiding this comment.
These new opencode-zen calls to deepseekThinkingEffortsFor / deepseekReasoningMapFor put three more model ids under the shared Flash-vs-Pro substring classifier, but tests/provider-registry-parity.test.ts’s “every DeepSeek V4 entry…” matrix still omits this provider. That matrix is the existing guard for every registry id that uses the classifier; without the Zen cases, a future Zen-specific id typo/name change could be mis-advertised without failing the parity test, so please add the three opencode-zen models there.
AGENTS.md reference: src/AGENTS.md:L22-L26
Useful? React with 👍 / 👎.
Second loop of the bug campaign, scoped to author corrected replacements for
four contributor PRs and close theirs as absorbed. Three adversarial audit
rounds refuted the premise, and the plan now records that instead of the
outcome it was written to produce.
The decisive finding: I judged "has the author responded to our review" by
`updatedAt`, which moves when WE comment and therefore can never show author
activity. Comparing last-commit time against review time gives the real
picture:
#1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes
#1068 commits 08:52:23Z review 09:13:15Z -> predates the review
#1036 commits 08-05 review 09:12:51Z -> no response yet
#997 commits 02:51Z review 09:16:02Z -> no response yet
Under the wrong reading I was about to close #1092 as absorbed — taking
credit for work its author did in direct response to my own request — and to
credit #1068 with agreeing to a review it had not yet read. Neither is a
process nit; both would have been visible to the contributor.
So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997
get a stated 72-hour response window with a mandatory head re-check before
anything is authored or closed; "has not replied within an hour" is not
abandonment, and #997's author was active at 02:51Z. This loop therefore
authors no absorbed layer at all, and says so.
#1068's re-review carries a finding neither side has: its new test asserts
all three DeepSeek ids are in `noVisionModels`, but routing merges the
registry list, which holds only the `-free` one. Reproduced with the test's
own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of
three cases fail. Latent because no check currently runs that suite.
Also corrected: the layers were called dependency-ordered when they share no
files, so s3/s4 are independent heads off `origin/dev` and the
`--update-refs` cascade is reserved for genuine chains; and #978's exclusion
no longer claims it is "already correct" when it needs an author-side docs
change.
Terminal outcome is deferred, not DONE. Shrinking the criteria to match what
finished would have hidden exactly the thing worth recording.
|
Re-reviewed your current head ( First, an apology for the last review's framing. I told you the Second: the new test.each(["deepseek-v4-flash-free", "deepseek-v4-flash", "deepseek-v4-pro"])(
"%s is listed in opencode-zen noVisionModels for the vision sidecar",
modelId => {
const route = routeModel(configFor(modelId), `opencode-zen/${modelId}`);
expect(route.provider.noVisionModels).toContain(modelId);
},
);The entry keeps So two of the three cases fail. It has not surfaced because no check currently runs that suite on this PR — which is why I am telling you rather than letting CI do it later. The underlying question is genuinely open, and it is not one I can answer for you.
Either is fine by me. The first unblocks you today; the second is more valuable and you have the Zen access to do it properly. The replay half — mirroring |
|
Correction — my previous comment was stale, and the merged result is right. I posted that eight minutes after this merged, against the head I had fetched earlier, and it does not describe what actually landed. The merged entry is: noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS],That is a union, which is exactly what the review asked for and what I claimed was missing. I ran the merged suite against current So both of my objections are resolved in the merged code: the six probed text-only models keep their guard, and the DeepSeek thinking models are added rather than substituted. Whoever tightened it before merge got it right. What remains true and worth carrying forward: whether Apologies for the noise. The lesson on my side is not subtle: I read a head, wrote a finding, and posted it after the PR had moved. Re-fetching before commenting is cheap and I did not do it. Thanks for the fix — #994 has been waiting on the replay half, and the shared-gateway insight between |
Records what the stack-and-absorb campaign actually produced once live data replaced its assumptions. #1068 merged at 10:45:57Z while I was writing its review, and the merged code is not what I reviewed: `noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS]` — the union the review asked for. Running the merged suite against `origin/dev` gives 9 pass / 0 fail, including a new test pinning the six probed text-only models. My 10:53Z comment claiming a failing test was true of the head I had fetched and false of what landed, so it got a public correction eight minutes later. That is the second process error in this unit, and both are now written down rather than quietly fixed. First: judging author activity by `updatedAt`, which moves when WE comment. Second: commenting against a stale fetched head. Each produced a wrong public statement to a contributor. The rules are re-fetch immediately before commenting, and compare last-commit time against review time. #1092's author restored the fail-closed guard within 30 minutes of the review, with a comment keeping `unknown` distinct in debug. Credited explicitly; asked only to split the unrelated `imageInput` scope. #1036 and #997 are deferred, not absorbed. Their last commits (08-05 07:18Z, 08-06 02:51Z) predate our reviews and neither carries the requested change, but roughly two hours have passed and #997's author was active that morning. 050 states a 72-hour response window with a mandatory head re-check before anything is authored or closed. The window exists because this unit demonstrated the alternative: two of four absorb targets were being actively fixed by their authors while we drafted replacements. #994 updated with the dev-only landing (merge 7d0c02d, ancestor of `origin/dev`) and deliberately left open pending the reporter's provider confirmation and a release. Zero contributor PRs closed by this loop.
Two loops circled a problem the campaign had invented. The user named the answer in one line: their PRs report maintainerCanModify=true, so push our completion commit onto their branch. Their commits stay, blame stays accurate, their PR merges, and attribution is the commit graph rather than a paragraph. No replacement PR, no close, no "absorbed with credit to" prose. #1036 landed that way. Four Agent59353 commits, then ours on top, pushed with a lease pinned to their head after re-verifying the remote. The change is bounded: live-transport records the bare names we actually advertised this request, the event state carries them, and both translate call sites convert only for names in that set. Their conversion logic is byte- unchanged. The stateless fallback passes through, since it has no state to consult and an unconverted call is recoverable while a wrongly converted one edits a file — disclosed in the PR comment as a judgment call open to disagreement. Ablation: restoring the name-only gate gives 21 pass / 1 fail, red on exactly the new collision test. #997 was not ours to push. The lease rejected it as stale: the author landed their own fix mid-flight, and theirs is stronger than what we staged — we pinned getConfigDir(), they assert the resolved usage.jsonl receives the row and the default location does not. Verified instead of overwritten, 10 pass with a 9/1 ablation on their own assertion. That is the third time in this campaign that acting on a stale head produced wasted or wrong work: updatedAt as an activity signal, a "failing test" comment posted eight minutes after #1068 merged with the fix, and a commit built against a head the author had already improved. The lease caught the third; the first two reached a contributor as a wrong public statement. The rule is now written down — re-fetch immediately before preparing a commit, not before pushing.
3 participants
Summary
opencode-zenwas the only OpenCode provider missing DeepSeek reasoning metadata. The fix(responses): keep DeepSeek reasoning_content on tool-call continuations (#950) #971 replay fix only fires for models inpreserveReasoningContentModels, andopencode-zendeclared none — soopencode-zen/deepseek-v4-flash-freetool-call continuations went upstream without the originalreasoning_content, and the gateway rejected them with HTTP 400.preserveReasoningContentModels, reasoning-effort map,noVisionModels) thatopencode-goandopencode-freealready carry ontoopencode-zen, coveringdeepseek-v4-pro,deepseek-v4-flash, anddeepseek-v4-flash-free.Verification
bun run typecheck— passbun test tests/opencode-zen-deepseek-reasoning.test.ts— pass (regression; red before fix, green after; also asserts effort mapping and that non-listed models are untouched)bun test tests/provider-registry-parity.test.ts tests/opencode-go-deepseek.test.ts tests/reasoning-effort.test.ts tests/deepseek-reasoning-replay.test.ts— passbun run test— 9018 pass, 0 failbun run privacy:scan— passChecklist
Related
reasoning_contenton tool-call continuation for DeepSeek thinking modeopencode-go/opencode-freebut notopencode-zenopencode-zen)Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
xhighmapped to the maximum level.Bug Fixes