fix: reasoning item streaming in mux - #5286
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change gives reasoning its own Responses output item, enforces ordering and closure before text or tool output, places reasoning first in terminal responses, and ensures Anthropic thinking blocks always include signatures. Tests cover stream event integrity, ordering, indices, and signature presence. ChangesReasoning stream handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatStream
participant ToBifrostResponsesStreamResponse
participant AnthropicSSEConversion
ChatStream->>ToBifrostResponsesStreamResponse: reasoning delta
ToBifrostResponsesStreamResponse->>AnthropicSSEConversion: reasoning output item and delta
ChatStream->>ToBifrostResponsesStreamResponse: text or tool delta
ToBifrostResponsesStreamResponse->>AnthropicSSEConversion: close reasoning item
ToBifrostResponsesStreamResponse->>AnthropicSSEConversion: emit text or tool output
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
|
Confidence Score: 5/5No additional blocking issue qualifies for this follow-up review.
Important Files Changed
Reviews (3): Last reviewed commit: "fix: reasoning item streaming in mux" | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/schemas/mux.go (1)
2225-2245: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReasoning output items never carry a
Status, unlike text/tool items.The reasoning
doneItembuilt incloseReasoningItem(Lines 1727-1735) and the terminal reasoning message built here both omitStatus, whereas the equivalent text (Status: &statusFinal) and tool-call done items always set it. If downstream consumers of the Responses stream key offstatus(e.g. "completed") to know a reasoning item finished, this asymmetry could be surfaced as another spec-compliance gap for strict clients — the same category of issue this PR is otherwise fixing.♻️ Suggested fix
+ statusCompleted := "completed" doneItem := &ResponsesMessage{ Type: &reasoningType, Role: &role, + Status: &statusCompleted, Content: &ResponsesMessageContent{🤖 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 `@core/schemas/mux.go` around lines 2225 - 2245, Set the completed status on reasoning output items in both closeReasoningItem and the terminal reasoning-message construction shown here. Reuse the existing statusFinal symbol and ensure each reasoning doneItem or ResponsesMessage includes Status alongside its type, role, content, and optional ID, matching text and tool-call completion behavior.core/providers/anthropic/reasoningstream_test.go (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
p()pointer helper duplicates the existingschemas.Ptr().
schemas.Ptris already used for this exact purpose elsewhere in this same PR (e.g.core/schemas/mux_test.go). Reusing it here avoids a redundant local helper.Based on learnings, in maximhq/bifrost prefer using bifrost.Ptr()/schemas.Ptr() to create pointers instead of ad-hoc address-of helpers, for consistency across the codebase.♻️ Suggested fix
- p := func(s string) *string { return &s } chunks := []*schemas.BifrostChatResponse{ - reasoningStreamChunk("c1", &schemas.ChatStreamResponseChoiceDelta{Role: p("assistant"), Reasoning: p("")}, nil), + reasoningStreamChunk("c1", &schemas.ChatStreamResponseChoiceDelta{Role: schemas.Ptr("assistant"), Reasoning: schemas.Ptr("")}, nil),🤖 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 `@core/providers/anthropic/reasoningstream_test.go` at line 39, Remove the local p pointer helper in the reasoning stream tests and replace its usages with the existing schemas.Ptr helper, preserving the current pointer values and test behavior.Source: Learnings
🤖 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.
Nitpick comments:
In `@core/providers/anthropic/reasoningstream_test.go`:
- Line 39: Remove the local p pointer helper in the reasoning stream tests and
replace its usages with the existing schemas.Ptr helper, preserving the current
pointer values and test behavior.
In `@core/schemas/mux.go`:
- Around line 2225-2245: Set the completed status on reasoning output items in
both closeReasoningItem and the terminal reasoning-message construction shown
here. Reuse the existing statusFinal symbol and ensure each reasoning doneItem
or ResponsesMessage includes Status alongside its type, role, content, and
optional ID, matching text and tool-call completion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: abd99a0a-1045-44df-b459-d26b34f471a9
📒 Files selected for processing (4)
core/providers/anthropic/reasoningstream_test.gocore/providers/anthropic/responses.gocore/schemas/mux.gocore/schemas/mux_test.go
952c020 to
36391e1
Compare
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 `@core/providers/anthropic/reasoningstream_test.go`:
- Around line 147-164: Update the stream test around the resumed reasoning chunk
in the chunks loop to capture the events emitted specifically for that chunk
after text begins, and assert that it contains no thinking content-block start
or delta events. Keep the existing open-block validation, while ensuring both a
newly opened thinking block and a completely missing expected event sequence are
detected.
- Around line 127-129: Update the assertion for blocks[0].Signature in the
reasoning stream test to verify both that the pointer is non-nil and that its
dereferenced value is the empty string when the fixture has no signature.
Preserve the existing failure context while ensuring synthesized non-empty
values fail.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4b8e17e9-2672-4fb8-b3de-a222fbe5cad4
📒 Files selected for processing (4)
core/providers/anthropic/reasoningstream_test.gocore/providers/anthropic/responses.gocore/schemas/mux.gocore/schemas/mux_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/mux_test.go
- core/providers/anthropic/responses.go
- core/schemas/mux.go
Merge activity
|
The base branch was changed.
b00153a to
a5dbd36
Compare
a5dbd36 to
31d9c6a
Compare
## Summary Fixes a bug where reasoning models (e.g. DeepSeek Reasoner) streaming through the Chat → Responses → Anthropic SSE pipeline produced orphan thinking blocks — `content_block_delta` events referencing an index that no prior `content_block_start` had opened. Strict clients like Claude Code reject this with "Content block not found", abort the stream, and report a client disconnect. Additionally, fixes the Agent SDK's non-streaming parser failing with "Missing required field in assistant message: 'signature'" when converted (non-Anthropic) reasoning blocks omit the `signature` field. ## Changes - **Reasoning item lifecycle**: Reasoning content now opens a dedicated `output_item.added` (type=reasoning) before emitting any reasoning deltas, and closes it with `output_item.done` before the text or tool-call item opens. This ensures `content_block_start type=thinking` always precedes any `thinking_delta` in the Anthropic SSE layer, and blocks never overlap. - **Output index ordering**: Reasoning is assigned output index 0, with the text item receiving a higher index. Previously both reasoning and text deltas were hardcoded to index 0, causing collisions. - **Reasoning item state tracking**: Added `ReasoningItemAdded`, `ReasoningItemClosed`, `ReasoningOutputIndex`, `ReasoningBuffer`, and `TextOutputIndex` fields to `ChatToResponsesStreamState` so the reasoning block's lifecycle is tracked independently from the text block. - **Signature defaulting**: `convertBifrostReasoningToAnthropicThinking` now defaults `Signature` to an empty string pointer instead of `nil` for converted (non-Anthropic) reasoning blocks, satisfying the Agent SDK's required-field check. - **Removed empty delta workaround**: The previous workaround that emitted an empty text delta for reasoning-only responses is removed; reasoning models now emit proper reasoning items instead. - **Tests**: Added `TestToBifrostResponsesStreamResponse_ReasoningOpensThinkingBlock` (schema layer) and `TestReasoningStream_NoOrphanThinkingBlock` / `TestConvertBifrostReasoning_SignaturePresent` (Anthropic provider layer) to assert correct block ordering, ItemID propagation, and signature presence. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... -run TestToBifrostResponsesStreamResponse_ReasoningOpensThinkingBlock -v go test ./core/providers/anthropic/... -run TestReasoningStream_NoOrphanThinkingBlock -v go test ./core/providers/anthropic/... -run TestConvertBifrostReasoning_SignaturePresent -v go test ./... ``` Expected: all three new tests pass, and no existing tests regress. When streaming a reasoning model (e.g. DeepSeek Reasoner) through the Anthropic SSE endpoint, Claude Code no longer disconnects with "Content block not found". ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. Changes are limited to stream event ordering and field defaulting in the reasoning/thinking block conversion path. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Fixes a bug where reasoning models (e.g. DeepSeek Reasoner) streaming through the Chat → Responses → Anthropic SSE pipeline produced orphan thinking blocks — `content_block_delta` events referencing an index that no prior `content_block_start` had opened. Strict clients like Claude Code reject this with "Content block not found", abort the stream, and report a client disconnect. Additionally, fixes the Agent SDK's non-streaming parser failing with "Missing required field in assistant message: 'signature'" when converted (non-Anthropic) reasoning blocks omit the `signature` field. ## Changes - **Reasoning item lifecycle**: Reasoning content now opens a dedicated `output_item.added` (type=reasoning) before emitting any reasoning deltas, and closes it with `output_item.done` before the text or tool-call item opens. This ensures `content_block_start type=thinking` always precedes any `thinking_delta` in the Anthropic SSE layer, and blocks never overlap. - **Output index ordering**: Reasoning is assigned output index 0, with the text item receiving a higher index. Previously both reasoning and text deltas were hardcoded to index 0, causing collisions. - **Reasoning item state tracking**: Added `ReasoningItemAdded`, `ReasoningItemClosed`, `ReasoningOutputIndex`, `ReasoningBuffer`, and `TextOutputIndex` fields to `ChatToResponsesStreamState` so the reasoning block's lifecycle is tracked independently from the text block. - **Signature defaulting**: `convertBifrostReasoningToAnthropicThinking` now defaults `Signature` to an empty string pointer instead of `nil` for converted (non-Anthropic) reasoning blocks, satisfying the Agent SDK's required-field check. - **Removed empty delta workaround**: The previous workaround that emitted an empty text delta for reasoning-only responses is removed; reasoning models now emit proper reasoning items instead. - **Tests**: Added `TestToBifrostResponsesStreamResponse_ReasoningOpensThinkingBlock` (schema layer) and `TestReasoningStream_NoOrphanThinkingBlock` / `TestConvertBifrostReasoning_SignaturePresent` (Anthropic provider layer) to assert correct block ordering, ItemID propagation, and signature presence. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... -run TestToBifrostResponsesStreamResponse_ReasoningOpensThinkingBlock -v go test ./core/providers/anthropic/... -run TestReasoningStream_NoOrphanThinkingBlock -v go test ./core/providers/anthropic/... -run TestConvertBifrostReasoning_SignaturePresent -v go test ./... ``` Expected: all three new tests pass, and no existing tests regress. When streaming a reasoning model (e.g. DeepSeek Reasoner) through the Anthropic SSE endpoint, Claude Code no longer disconnects with "Content block not found". ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. Changes are limited to stream event ordering and field defaulting in the reasoning/thinking block conversion path. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Fixes a bug where reasoning models (e.g. DeepSeek Reasoner) streaming through the Chat → Responses → Anthropic SSE pipeline produced orphan thinking blocks —
content_block_deltaevents referencing an index that no priorcontent_block_starthad opened. Strict clients like Claude Code reject this with "Content block not found", abort the stream, and report a client disconnect. Additionally, fixes the Agent SDK's non-streaming parser failing with "Missing required field in assistant message: 'signature'" when converted (non-Anthropic) reasoning blocks omit thesignaturefield.Changes
output_item.added(type=reasoning) before emitting any reasoning deltas, and closes it withoutput_item.donebefore the text or tool-call item opens. This ensurescontent_block_start type=thinkingalways precedes anythinking_deltain the Anthropic SSE layer, and blocks never overlap.ReasoningItemAdded,ReasoningItemClosed,ReasoningOutputIndex,ReasoningBuffer, andTextOutputIndexfields toChatToResponsesStreamStateso the reasoning block's lifecycle is tracked independently from the text block.convertBifrostReasoningToAnthropicThinkingnow defaultsSignatureto an empty string pointer instead ofnilfor converted (non-Anthropic) reasoning blocks, satisfying the Agent SDK's required-field check.TestToBifrostResponsesStreamResponse_ReasoningOpensThinkingBlock(schema layer) andTestReasoningStream_NoOrphanThinkingBlock/TestConvertBifrostReasoning_SignaturePresent(Anthropic provider layer) to assert correct block ordering, ItemID propagation, and signature presence.Type of change
Affected areas
How to test
Expected: all three new tests pass, and no existing tests regress. When streaming a reasoning model (e.g. DeepSeek Reasoner) through the Anthropic SSE endpoint, Claude Code no longer disconnects with "Content block not found".
Screenshots/Recordings
N/A
Breaking changes
Related issues
Security considerations
No security implications. Changes are limited to stream event ordering and field defaulting in the reasoning/thinking block conversion path.
Checklist
docs/contributing/README.mdand followed the guidelines