fix(deepseek): keep parallel reasoning replay on continuation - #1499
fix(deepseek): keep parallel reasoning replay on continuation#1499Wibias wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughDeepSeek V4 Flash now normalizes complete, unambiguous parallel Responses tool-call batches. The adapter preserves ambiguous histories unchanged. Documentation and inbound-wire tests cover ordering, context placement, separate rounds, and invalid matches. ChangesDeepSeek Responses normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InboundRequest
participant normalizeResponsesToolResultAdjacency
participant DeepSeekResponsesAPI
InboundRequest->>normalizeResponsesToolResultAdjacency: Responses history with tool calls and results
normalizeResponsesToolResultAdjacency->>normalizeResponsesToolResultAdjacency: Validate call/result order and uniqueness
normalizeResponsesToolResultAdjacency->>DeepSeekResponsesAPI: Normalized complete batch or unchanged ambiguous history
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@src/adapters/openai-responses.ts`:
- Around line 591-601: Update the normalization logic around the calls/outputs
validation and batch construction to reject orphan outputs by requiring every
entry in outputs to map to exactly one call. Before rewriting each batch,
require matched output indices to be strictly increasing in call order; preserve
body unchanged for missing, duplicate, reversed, or otherwise out-of-order
histories. Add regression coverage for an orphan output in an otherwise valid
batch and reversed parallel outputs.
In `@tests/deepseek-inbound-wire.test.ts`:
- Around line 102-104: Update deepseekReasoningProvider() to return the
unmodified deepseekProvider() result instead of overriding
preserveResponsesReasoningContent. Add an assertion that the returned
configuration has preserveResponsesReasoningContent set to true before the tests
use it, so the tests validate the registered DeepSeek preset behavior.
🪄 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: 8e5ea2d3-4319-48bf-958d-4ed1e2635737
📒 Files selected for processing (4)
docs-site/src/content/docs/reference/adapters.mdsrc/adapters/openai-responses.tsstructure/04_transports-and-sidecars.mdtests/deepseek-inbound-wire.test.ts
| // Fail closed: only normalize when every collected call has exactly one matching | ||
| // result and that result appears after its call. Missing, duplicate, or backward | ||
| // histories are ambiguous and must be left untouched for the upstream to reject. | ||
| const pairs: Array<{ callIndex: number; outputIndex: number }> = []; | ||
| for (const [key, callIndices] of calls) { | ||
| const outputIndices = outputs.get(key); | ||
| if (callIndices.length !== 1 || outputIndices?.length !== 1) 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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unmatched and result-reversed tool histories.
Lines 595-601 validate calls, but they do not reject an output whose call_id has no matching call. Lines 609-622 also accept callA, callB, outputB, outputA and rewrite it to callA, callB, outputA, outputB. Both cases are ambiguous histories. The adapter must return body unchanged.
Validate every entry in outputs against exactly one call. Before building each batch, require output indices to be strictly increasing in call order. Add regression cases for an orphan output beside an otherwise valid batch and for reversed parallel outputs.
Proposed fix
+ for (const [key, outputIndices] of outputs) {
+ if (calls.get(key)?.length !== 1 || outputIndices.length !== 1) return body;
+ }
+
const pairs: Array<{ callIndex: number; outputIndex: number }> = [];
for (const [key, callIndices] of calls) {
const outputIndices = outputs.get(key);
if (callIndices.length !== 1 || outputIndices?.length !== 1) return body; while (next < pairs.length && pairs[next]!.callIndex < firstOutputIndex) {
group.push(pairs[next]!);
firstOutputIndex = Math.min(firstOutputIndex, pairs[next]!.outputIndex);
next += 1;
}
+ if (group.some((pair, index) =>
+ index > 0 && pair.outputIndex <= group[index - 1]!.outputIndex,
+ )) return body;As per path instructions, “Preserve histories with missing, duplicate, reversed, or otherwise out-of-order calls/results unchanged.”
Also applies to: 609-622
🤖 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 591 - 601, Update the
normalization logic around the calls/outputs validation and batch construction
to reject orphan outputs by requiring every entry in outputs to map to exactly
one call. Before rewriting each batch, require matched output indices to be
strictly increasing in call order; preserve body unchanged for missing,
duplicate, reversed, or otherwise out-of-order histories. Add regression
coverage for an orphan output in an otherwise valid batch and reversed parallel
outputs.
Source: Path instructions
| function deepseekReasoningProvider(): OcxProviderConfig { | ||
| return { ...deepseekProvider(), preserveResponsesReasoningContent: true }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the registered DeepSeek reasoning configuration.
deepseekReasoningProvider() forces preserveResponsesReasoningContent: true. The new tests therefore pass even if the DeepSeek registry entry stops enabling this required provider behavior. In that failure case, buildRequest can remove replayed plaintext reasoning before the request reaches DeepSeek.
Return the unmodified deepseekProvider() result. Assert that preserveResponsesReasoningContent is true before using it.
Proposed fix
function deepseekReasoningProvider(): OcxProviderConfig {
- return { ...deepseekProvider(), preserveResponsesReasoningContent: true };
+ const provider = deepseekProvider();
+ expect(provider.preserveResponsesReasoningContent).toBe(true);
+ return provider;
}As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.” The PR objective requires the built-in DeepSeek preset to preserve reasoning content.
📝 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.
| function deepseekReasoningProvider(): OcxProviderConfig { | |
| return { ...deepseekProvider(), preserveResponsesReasoningContent: true }; | |
| } | |
| function deepseekReasoningProvider(): OcxProviderConfig { | |
| const provider = deepseekProvider(); | |
| expect(provider.preserveResponsesReasoningContent).toBe(true); | |
| return provider; | |
| } |
🤖 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 `@tests/deepseek-inbound-wire.test.ts` around lines 102 - 104, Update
deepseekReasoningProvider() to return the unmodified deepseekProvider() result
instead of overriding preserveResponsesReasoningContent. Add an assertion that
the returned configuration has preserveResponsesReasoningContent set to true
before the tests use it, so the tests validate the registered DeepSeek preset
behavior.
Source: Path instructions
1 participant
Summary
reasoning_text in the thinking mode must be passed back400 ondeepseek/deepseek-v4-flash.Validation
bun test tests/deepseek-inbound-wire.test.ts— 37 pass (5 new regression tests, 99 expect calls)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— 294 pass; 6 pre-existing symlink EPERM failures in config.test.ts only (Windows lacks symlink privilege); none related to this diffbun run typecheck— passbun run privacy:scan— passcd docs-site && bun install --frozen-lockfile && bun run build— 265 pagesgit diff --check— passReview notes
scripts/pre-open-gate.mjs— ready (all required bug lenses and security surfaces covered; test-honesty probe verified clean).Fixes #1477
Summary by CodeRabbit