fix(deepseek): preserve parallel reasoning replay - #1479
Conversation
📝 WalkthroughWalkthroughDeepSeek Responses normalization now handles unambiguous parallel tool-call batches. It preserves reasoning context, moves injected messages after complete batches, and leaves ambiguous or invalid histories unchanged. Documentation and inbound-wire tests describe and validate the behavior. ChangesDeepSeek Responses normalization
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 💡 1📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
|
Exact-head CI note for
No code or workflow change is justified by this runtime crash. I will keep the PR draft and rerun only the failed macOS job once GitHub finalizes the parent run. |
|
Exact-head CI update for Run
The only red job is the macOS full-suite job, across three infrastructure/flaky outcomes on the same code:
Neither failing path is in this PR diff, and the DeepSeek focused coverage plus every Linux shard passed. I am stopping repeated macOS reruns after the same runtime crash reproduced twice. The PR remains draft and unmerged pending independent human review; this comment records CI evidence rather than treating infrastructure failure as approval. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Reviewed the DeepSeek batch normalizer. I did not find an additional blocking defect in this head. The change preserves same-turn parallel calls as one call batch followed by results, retains the existing single-call repair, and returns ambiguous duplicate/backwards histories unchanged rather than guessing.
I am not approving this head because it is currently not mergeable against the latest dev. Please rebase/resolve conflicts and request a short re-review of the final normalizer diff.
ec5b995 to
daea201
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs-site/src/content/docs/reference/adapters.md`:
- Around line 61-64: Update docs-site/src/content/docs/reference/adapters.md
lines 61-64 to state that the openai-responses adapter preserves original order
for duplicate, missing, and out-of-order call IDs. Update
structure/04_transports-and-sidecars.md lines 359-364 to include missing
call/result pairs in the fail-closed history cases alongside duplicate and
backward/out-of-order pairs.
In `@src/adapters/openai-responses.ts`:
- Around line 580-588: Update the call/output pairing logic around the calls
iteration to fail closed whenever any collected tool call lacks exactly one
matching result, rather than skipping unmatched calls. Validate that every call
has one later result and that no result is backward; return the original body
unchanged for duplicate, missing, or out-of-order IDs, and add regression
coverage for a missing earlier call result and a backward result.
🪄 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: fa2807df-f832-403b-86da-4831a2108a63
📒 Files selected for processing (6)
docs-site/src/content/docs/reference/adapters.mdsrc/adapters/openai-responses.tssrc/providers/registry.tssrc/types.tsstructure/04_transports-and-sidecars.mdtests/deepseek-inbound-wire.test.ts
| - DeepSeek's stateless Responses parser receives provider-scoped history normalization: hook-injected | ||
| context moves after an unambiguous tool-call/result batch. Parallel calls remain grouped before | ||
| their matching outputs so every call stays in the reasoning-bearing assistant turn. Tolerant | ||
| providers and ambiguous duplicate call IDs keep their original input order. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document all fail-closed history cases.
The documentation must state that missing and out-of-order call/result histories retain their original order. The public adapter reference currently names only duplicate IDs. The design document names duplicate and backward pairs but omits missing pairs.
docs-site/src/content/docs/reference/adapters.md#L61-L64: State that duplicate, missing, and out-of-order call IDs remain unchanged.structure/04_transports-and-sidecars.md#L359-L364: Add missing call/result pairs to the fail-closed list.
As per path instructions, the openai-responses reference must document duplicate, missing, and out-of-order IDs as unchanged.
📍 Affects 2 files
docs-site/src/content/docs/reference/adapters.md#L61-L64(this comment)structure/04_transports-and-sidecars.md#L359-L364
🤖 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 `@docs-site/src/content/docs/reference/adapters.md` around lines 61 - 64,
Update docs-site/src/content/docs/reference/adapters.md lines 61-64 to state
that the openai-responses adapter preserves original order for duplicate,
missing, and out-of-order call IDs. Update
structure/04_transports-and-sidecars.md lines 359-364 to include missing
call/result pairs in the fail-closed history cases alongside duplicate and
backward/out-of-order pairs.
Source: Path instructions
| for (const [key, callIndices] of calls) { | ||
| const outputIndices = outputs.get(key); | ||
| if (callIndices.length !== 1 || outputIndices?.length !== 1) continue; | ||
| if (!outputIndices) continue; | ||
| if (callIndices.length !== 1 || outputIndices.length !== 1) return body; | ||
| const callIndex = callIndices[0]!; | ||
| const outputIndex = outputIndices[0]!; | ||
| if (outputIndex === callIndex + 1) continue; | ||
| movedOutputIndices.add(outputIndex); | ||
| outputAfterCall.set(callIndex, input[outputIndex]); | ||
| if (outputIndex <= callIndex) return body; | ||
| pairs.push({ callIndex, outputIndex }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject a history when any tool call has no matching result.
Line 582 skips a call with no result. The function can then reorder a later pair in the same ambiguous history.
For example, [callA, callB, injected, outputB] becomes [callA, callB, outputB, injected], even though callA has no result. Return body when any collected call lacks exactly one later matching result. Add a regression test for this case and for a backward result.
Proposed fix
for (const [key, callIndices] of calls) {
const outputIndices = outputs.get(key);
- if (!outputIndices) continue;
+ if (!outputIndices) return body;
if (callIndices.length !== 1 || outputIndices.length !== 1) return body;As per path instructions, normalization must leave “duplicate, missing, or out-of-order call IDs unchanged (fail closed).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const [key, callIndices] of calls) { | |
| const outputIndices = outputs.get(key); | |
| if (callIndices.length !== 1 || outputIndices?.length !== 1) continue; | |
| if (!outputIndices) continue; | |
| if (callIndices.length !== 1 || outputIndices.length !== 1) return body; | |
| const callIndex = callIndices[0]!; | |
| const outputIndex = outputIndices[0]!; | |
| if (outputIndex === callIndex + 1) continue; | |
| movedOutputIndices.add(outputIndex); | |
| outputAfterCall.set(callIndex, input[outputIndex]); | |
| if (outputIndex <= callIndex) return body; | |
| pairs.push({ callIndex, outputIndex }); | |
| } | |
| for (const [key, callIndices] of calls) { | |
| const outputIndices = outputs.get(key); | |
| if (!outputIndices) return body; | |
| if (callIndices.length !== 1 || outputIndices.length !== 1) return body; | |
| const callIndex = callIndices[0]!; | |
| const outputIndex = outputIndices[0]!; | |
| if (outputIndex <= callIndex) return body; | |
| pairs.push({ callIndex, outputIndex }); | |
| } |
🤖 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 `@src/adapters/openai-responses.ts` around lines 580 - 588, Update the
call/output pairing logic around the calls iteration to fail closed whenever any
collected tool call lacks exactly one matching result, rather than skipping
unmatched calls. Validate that every call has one later result and that no
result is backward; return the original body unchanged for duplicate, missing,
or out-of-order IDs, and add regression coverage for a missing earlier call
result and a backward result.
Source: Path instructions
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review against current dev@e8db4e03: one current blocking correctness issue remains. normalizeResponsesToolResultAdjacency() skips calls with no matching output, so a partially matched history can still be reordered. For example [callA, callB, injected, outputB] can move outputB ahead of injected even though callA is unresolved. This violates the documented fail-closed boundary for missing/duplicate/out-of-order call-result histories. Please return the original body when any collected call lacks exactly one later matching output, add missing/backward regression coverage, and update the public/design docs to name missing histories explicitly. The branch is also 9 commits behind current dev and currently not mergeable, so please rebase after the fix.
2 participants
Summary
Root cause
The provider-scoped #1292 normalizer repaired every call/result pair independently. A valid parallel history such as
reasoning, call A, call B, output A, output Btherefore becamereasoning, call A, output A, call B, output B. DeepSeek merges adjacent reasoning and function calls into one assistant message and always enables parallel tool calling, so the second call lost the reasoning block that belonged to its original turn and the continuation failed withreasoning_textmissing.The fix groups calls that occur before the first matched output, emits all calls followed by their outputs in call order, and moves intervening non-tool context after the complete batch. Duplicate, backwards, or otherwise ambiguous call/result histories return unchanged rather than being guessed.
Fixes #1477.
Verification
bun test tests/deepseek-inbound-wire.test.ts tests/openai-responses-passthrough.test.ts tests/deepseek-reasoning-replay.test.ts tests/deepseek-reasoning-replay-gaps.test.ts tests/config.test.ts tests/provider-registry-parity.test.ts tests/config-save-boundary.test.ts— 296 passed, 0 failedbun run typecheck— passedbun run privacy:scan— passedcd docs-site && bun install --frozen-lockfile && bun run build— 265 pages builtgit diff --check— passedorigin/dev@849ab5e35because this host has a live service-token environment; no shim files are changed hereAll local builds and tests ran with
taskset -c 0-1 nice -n 10.Decision Log
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation