fix(responses): keep DeepSeek reasoning_content on tool-call continuations (#950) - #971
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesReasoning replay preservation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ModelStream
participant ResponsesBridge
participant ReplayCache
participant OpenAIChatAdapter
ModelStream->>ResponsesBridge: emit raw reasoning and tool call
ResponsesBridge->>ReplayCache: rememberReasoningForCall(callId, reasoning, scope)
OpenAIChatAdapter->>ReplayCache: peekReasoningForCall(callId, scope)
ReplayCache-->>OpenAIChatAdapter: cached reasoning
OpenAIChatAdapter-->>ModelStream: assistant tool call with reasoning_content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-chat.ts`:
- Around line 405-412: Update the orphan repair block around
peekReasoningForCall to emit reasoning_content only when
modelInList(provider.preserveReasoningContentModels, parsed.modelId) is true,
matching the normal assistant path. Keep the cached reasoning lookup and
assistant tool-call replay unchanged otherwise.
In `@src/responses/reasoning-replay-cache.ts`:
- Around line 66-75: The reasoning cache currently expires entries only during
peek operations and uses an exclusive TTL boundary. Update the cache state
around rememberReasoningForCall to schedule cleanup for the next entry expiry,
have the timer delete every due entry and reschedule for the next expiry, and
clear that timer in clearReasoningReplayCacheForTests. Change the expiration
check in peekReasoningForCall to use >= TTL_MS while preserving totalBytes
accounting.
In `@tests/deepseek-reasoning-replay-gaps.test.ts`:
- Around line 124-133: Update the test covering orphan tool-result repair,
identified by “GAP C: orphan tool result (lost assistant turn) is repaired WITH
the recorded reasoning,” to call wireFor() twice without clearing the remembered
reasoning cache. Extract the synthesized assistant message from each result and
assert both contain REASONING in reasoning_content, preserving the existing call
ID and input setup.
- Around line 177-194: Strengthen the tests around rememberReasoningForCall:
verify the exact MAX_ENTRIES boundary by asserting call_5 is retained and call_6
is evicted after inserting 70 entries, and add several individually valid
reasoning entries whose combined size exceeds the 256 KiB aggregate limit,
asserting the oldest entries are evicted while newer entries remain available.
🪄 Autofix (Beta)
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: 221f3a65-7ca2-4c65-b4d3-85a864a2b65b
📒 Files selected for processing (8)
src/adapters/openai-chat.tssrc/bridge.tssrc/images/loop.tssrc/responses/parser.tssrc/responses/reasoning-replay-cache.tstests/bridge-raw-reasoning-hidden.test.tstests/deepseek-reasoning-replay-gaps.test.tstests/images/loop-reasoning-replay.test.ts
![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: e2d9421882
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bridge.ts (1)
854-854: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear pending raw reasoning in the hidden-thinking path.
When
hideThinkingSummaryis true, the branch at Lines 842-849 returns before Line 854 clearsrawReasoningForNextToolCall. A priorreasoning_raw_deltacan then be stored for a later tool call even though athinking_deltaoccurred between them.Flush the hidden raw item, then clear the pending cache candidate before appending hidden thinking. Add a regression case in
tests/bridge-raw-reasoning-hidden.test.tsforreasoning_raw_delta → thinking_delta → tool_call_start.Proposed fix
case "thinking_delta": { if (options?.hideThinkingSummary) { + flushHiddenRawReasoning(); + rawReasoningForNextToolCall = ""; ({ value: hiddenThinkingText, bytes: hiddenThinkingBytes } = appendString( hiddenThinkingText, hiddenThinkingBytes,As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 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/bridge.ts` at line 854, Update the hidden-thinking branch in the bridge flow around rawReasoningForNextToolCall so it flushes any pending hidden raw item and clears the pending raw-reasoning cache before appending hidden thinking, including when the branch returns early. Add a focused regression case in the existing hidden raw-reasoning tests covering reasoning_raw_delta → thinking_delta → tool_call_start and verify the stale raw reasoning is not attached to the later tool call.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/bridge.ts`:
- Line 854: Update the hidden-thinking branch in the bridge flow around
rawReasoningForNextToolCall so it flushes any pending hidden raw item and clears
the pending raw-reasoning cache before appending hidden thinking, including when
the branch returns early. Add a focused regression case in the existing hidden
raw-reasoning tests covering reasoning_raw_delta → thinking_delta →
tool_call_start and verify the stale raw reasoning is not attached to the later
tool call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8974f182-b8d3-48c5-8995-e5b7a681e52a
📒 Files selected for processing (7)
src/adapters/openai-chat.tssrc/bridge.tssrc/images/loop.tssrc/responses/reasoning-replay-cache.tssrc/server/responses/core.tssrc/web-search/loop.tstests/deepseek-reasoning-replay-gaps.test.ts
Collaborator
Author
|
[GD] Addressed feedback feedbacks:
commit: 0b73d5a |
Collaborator
Author
[GD] Merge readyPR: #971 — fix(responses): keep DeepSeek reasoning_content on tool-call continuations (#950) Reviews
Tip freshness
Checks
Residual
Ready to merge. |
Collaborator
Author
[GD] MergingWhy it helps: this restores DeepSeek thinking-mode |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reasoning_contenton tool-call continuations soopencode-go/deepseek-v4-flash(and otherpreserveReasoningContentModelsproviders) no longer receives bare assistanttool_callsafter history transformations — fixing the intermittent upstream HTTP 400 from issue [Bug] OpenCode Go DeepSeek V4 Flash intermittently drops reasoning_content on tool-call continuation #950.reasoningitem that arrives after itsfunction_callto the owning assistant turn instead of discarding it, and the image bridge preservesreasoning_raw_deltain replayed assistant turns (mirroring the web-search loop from DeepSeek V4 web-search continuation drops reasoning_raw_delta and returns 502 #688).Validation
bun run typecheck— passbun test tests/deepseek-reasoning-replay-gaps.test.ts tests/bridge-raw-reasoning-hidden.test.ts tests/images/loop-reasoning-replay.test.ts tests/images/loop.test.ts tests/web-search.test.ts tests/opencode-go-deepseek.test.ts tests/deepseek-reasoning-replay.test.ts— pass (93 tests)bun run privacy:scan— passbun run test— not completed locally (Windows runner: suite exceeded ~50 min and the background worker was terminated; CI runs the full matrix on this head)Review notes
_clientThreadId) plus call id so colliding provider ids (call_1) cannot leak reasoning across threads; entries are bounded (64 / 256 KiB / 1h TTL), swept on insert, and never logged or serialized. Parallel tool calls share one recorded reasoning block and are deduplicated at serialization. Adjacent but distinct: open PR fix(kiro): round-trip the redactedContent reasoning blob #948 covers Kiro's redacted reasoning blob, not this wire.Limitations
Fixes #950
Summary by CodeRabbit