fix: give reasoning deltas their own output_item.added so Anthropic streams stay valid - #5221
Shaik-Sirajuddin wants to merge 1 commit into
Conversation
Live e2e verification (real anthropic-python SDK, real Ollama, real Bifrost server)Screenshots below are of a live terminal session (captured via Before fix (
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChat-to-Responses streaming now gives reasoning output items stable identifiers and lifecycle events, prevents empty text deltas, closes reasoning before tool calls, and includes reasoning in deterministic terminal output aggregation. ChangesReasoning stream lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ChatCompletionStream
participant ToBifrostResponsesStreamResponse
participant ResponsesClient
ChatCompletionStream->>ToBifrostResponsesStreamResponse: reasoning delta
ToBifrostResponsesStreamResponse->>ResponsesClient: output_item.added with ItemID
ToBifrostResponsesStreamResponse->>ResponsesClient: reasoning_summary_text.delta
ToBifrostResponsesStreamResponse->>ResponsesClient: output_item.done
ToBifrostResponsesStreamResponse->>ResponsesClient: tool-call output_item.added
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
…treams stay valid ToBifrostResponsesStreamResponse emitted reasoning_summary_text.delta events without a preceding output_item.added and without Item/ItemID, so the Anthropic reverse-converter (keyed on Item.ID > ItemID > "oi:<OutputIndex>") registered no block for them - a content_block_delta for a block whose content_block_start was never sent. This crashes the official anthropic-python SDK (IndexError: list index out of range) and explains Claude Code's duplicate stream+non-stream requests for reasoning-capable models behind Chat-Completions-fallback providers. Give the reasoning item its own output_item.added/output_item.done (mirroring the text/tool-call paths), close it before any tool call opens, fold it into the terminal Output-array sort, and drop the now unnecessary phantom empty text item that used to stand in for it. Fixes maximhq#5169.
4680f06 to
57f75e3
Compare
Confidence Score: 4/5Mixed and resumed reasoning streams can still produce invalid content-block lifecycles.
core/schemas/mux.go Important Files Changed
Reviews (1): Last reviewed commit: "fix: give reasoning deltas their own out..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // otherwise downstream Anthropic-format consumers see a | ||
| // content_block_delta for a block whose content_block_start was | ||
| // never sent, which strict SSE clients reject. | ||
| if !state.ReasoningItemAdded { | ||
| outputIndex := state.CurrentOutputIndex | ||
| if outputIndex == 0 { | ||
| outputIndex = 1 // Skip 0 if text is using it | ||
| } | ||
| state.CurrentOutputIndex = outputIndex + 1 | ||
| state.ReasoningOutputIndex = outputIndex | ||
|
|
||
| var itemID string | ||
| if state.MessageID == nil { | ||
| itemID = fmt.Sprintf("rs_item_%d", outputIndex) | ||
| } else { | ||
| itemID = fmt.Sprintf("rs_%s_item_%d", *state.MessageID, outputIndex) | ||
| } | ||
| state.ItemIDs["reasoning"] = itemID | ||
|
|
||
| reasoningType := ResponsesMessageTypeReasoning | ||
| role := ResponsesInputMessageRoleAssistant | ||
| item := &ResponsesMessage{ | ||
| ID: &itemID, | ||
| Type: &reasoningType, | ||
| Role: &role, | ||
| } | ||
|
|
||
| responses = append(responses, &BifrostResponsesStreamResponse{ | ||
| Type: ResponsesStreamResponseTypeOutputItemAdded, | ||
| SequenceNumber: state.SequenceNumber, | ||
| OutputIndex: Ptr(outputIndex), | ||
| Item: item, | ||
| ExtraFields: cr.ExtraFields, | ||
| }) | ||
| state.SequenceNumber++ | ||
| state.ReasoningItemAdded = true |
There was a problem hiding this comment.
When a stream emits reasoning, then a tool call, then more reasoning, the tool-call path closes the reasoning item but leaves ReasoningItemAdded true. The later reasoning chunk therefore skips output_item.added and emits a delta for an item that already received output_item.done, producing an invalid Anthropic content-block lifecycle.
Context Used: Review Bifrost PRs for correctness, regressions, c... (source)
| if delta.Reasoning != nil && *delta.Reasoning != "" { | ||
| // Reasoning/thought content delta (for models that support reasoning) | ||
| // Reasoning/thought content delta (for models that support reasoning). | ||
| // Give the reasoning item its own output_item.added (with a stable | ||
| // Item.ID) before the first delta, mirroring the text item above - | ||
| // otherwise downstream Anthropic-format consumers see a | ||
| // content_block_delta for a block whose content_block_start was | ||
| // never sent, which strict SSE clients reject. | ||
| if !state.ReasoningItemAdded { | ||
| outputIndex := state.CurrentOutputIndex | ||
| if outputIndex == 0 { | ||
| outputIndex = 1 // Skip 0 if text is using it | ||
| } | ||
| state.CurrentOutputIndex = outputIndex + 1 | ||
| state.ReasoningOutputIndex = outputIndex | ||
|
|
||
| var itemID string | ||
| if state.MessageID == nil { | ||
| itemID = fmt.Sprintf("rs_item_%d", outputIndex) | ||
| } else { | ||
| itemID = fmt.Sprintf("rs_%s_item_%d", *state.MessageID, outputIndex) | ||
| } | ||
| state.ItemIDs["reasoning"] = itemID | ||
|
|
||
| reasoningType := ResponsesMessageTypeReasoning | ||
| role := ResponsesInputMessageRoleAssistant | ||
| item := &ResponsesMessage{ | ||
| ID: &itemID, | ||
| Type: &reasoningType, | ||
| Role: &role, | ||
| } | ||
|
|
||
| responses = append(responses, &BifrostResponsesStreamResponse{ | ||
| Type: ResponsesStreamResponseTypeOutputItemAdded, | ||
| SequenceNumber: state.SequenceNumber, | ||
| OutputIndex: Ptr(outputIndex), | ||
| Item: item, | ||
| ExtraFields: cr.ExtraFields, | ||
| }) | ||
| state.SequenceNumber++ | ||
| state.ReasoningItemAdded = true |
There was a problem hiding this comment.
When a provider emits text before reasoning, or includes both fields in one chunk, the text path opens output index 0 and this path opens the reasoning item without closing the text item. Both remain active until a tool call or terminal event, so the Anthropic reverse converter can emit interleaved content blocks instead of a valid start/delta/stop sequence.
Context Used: Review Bifrost PRs for correctness, regressions, c... (source)
|
Duplicate of #5170, which was opened first (2026-07-14) and already has review feedback addressed. Closing this one in favor of that. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
core/schemas/mux.go (1)
1768-1855: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftClose the sibling item before opening text or reasoning.
core/schemas/mux.go:1775andcore/schemas/mux.go:2032open a new block without closing the other one first, so reasoning-first responses that later emitcontentcan leave two content blocks open until the terminal/tool-call path. The current tests cover reasoning→tool-call, not reasoning→text.🤖 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 1768 - 1855, Update the text-content emission path and the corresponding reasoning emission path so any currently open sibling content block is closed before opening the new text or reasoning block. Ensure reasoning-first responses that later emit content close reasoning before creating text, while preserving existing terminal and tool-call lifecycle behavior.
🤖 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 `@core/schemas/mux.go`:
- Around line 1768-1855: Update the text-content emission path and the
corresponding reasoning emission path so any currently open sibling content
block is closed before opening the new text or reasoning block. Ensure
reasoning-first responses that later emit content close reasoning before
creating text, while preserving existing terminal and tool-call lifecycle
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bffd4da-508d-46ec-9b6e-bafa815f868e
⛔ Files ignored due to path filters (2)
.github/pr-evidence/issue-5169-after-fix.pngis excluded by!**/*.png.github/pr-evidence/issue-5169-before-fix.pngis excluded by!**/*.png
📒 Files selected for processing (2)
core/schemas/mux.gocore/schemas/mux_test.go
1 participant
Fixes #5169.
ToBifrostResponsesStreamResponse(used wheneverResponsesStreamfalls back to Chat Completions - Ollama, Groq, Cerebras, DeepSeek, Mistral, Nebius, Parasail, SGL, vLLM, Perplexitysonar-*) emittedreasoning_summary_text.deltafor a model'sreasoningfield without ever sending a precedingoutput_item.added. Since the Anthropic reverse-converter keys content-block indices onItem.ID/ItemID, the orphaned delta resolved to nothing - acontent_block_deltafor a block never started. That's an SSE protocol violation: crashes the officialanthropic-pythonSDK (IndexError: list index out of range), and independently explains Claude Code's duplicate stream+non-stream requests (#5128) for reasoning-capable models.Fix: give the reasoning item its own
output_item.added/output_item.done(mirroring the existing text/tool-call paths), close it before any tool call opens, fold it into the terminal Output-array sort, and drop the now-redundant phantom empty text item.Verified: new unit tests (fail pre-fix, pass post-fix), full
core/schemas+core/providers/anthropicsuites pass, and a live e2e round-trip with the realanthropicPython SDK (0.116.0) against real Bifrost + Ollama - crashes before the fix, passes cleanly after (screenshots in comment below).