fix: complete visible thinking items on the streaming Responses surface - #5150
Conversation
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'dev' into fix-responses-st..." | Re-trigger Greptile |
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAnthropic Responses streaming now buffers visible-thinking text per output index, preserves signatures, completes reasoning output items, includes them in ChangesVisible-thinking streaming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Anthropic as Anthropic SSE
participant Converter as ToBifrostResponsesStream
participant State as AnthropicResponsesStreamState
participant Client as Responses client
Anthropic->>Converter: thinking block start and text deltas
Converter->>State: persist item and buffer reasoning text
Anthropic->>Converter: thinking block stop and signature
Converter->>State: fold text and signature into reasoning item
Converter->>Client: completed reasoning events
Converter->>Client: response.completed with reasoning output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
The merge-base changed after approval.
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
6ee13ff to
ef380f9
Compare
The merge-base changed after approval.
44564de to
493bff0
Compare
|
|
…ce (maximhq#5150) * add support for clickhouse for enterprise logstore tables * Revert "add support for clickhouse for enterprise logstore tables" (maximhq#5225) * Delete 1 Signed-off-by: Akshay Deo <akshay@akshaydeo.com> * fix: complete visible thinking items on the streaming Responses surface --------- Signed-off-by: Akshay Deo <akshay@akshaydeo.com> Co-authored-by: Sahil Choudhary <sahil.codes.it@gmail.com> Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
Summary
Fixes #5149.
On streamed
/v1/responsesfrom Anthropic-family backends with extended thinking enabled, visible thinking blocks stream their deltas but are never completed:reasoning_summary_text.donecarries an empty text,output_item.donefor the reasoning index degrades to an empty"message"shell that contradicts its ownoutput_item.added, andresponse.completedomits the reasoning item (and the signature) entirely. Clients that treatresponse.completedas the authoritative final snapshot (the openai-python streaming helper does, and so does the openai-node version Bifrost's integrations pin) lose the reasoning block, and a tool loop that echoes the output back silently drops the model's thinking from its own conversation history.The cause is that the completion side of the visible-thinking lifecycle was never implemented: the
thinkingstart case doesn't persist its item intostate.OutputItems(every other item type does), thinking deltas aren't buffered anywhere, andstate.ReasoningSignatureshas been write-only since #895. #5094 fixed the same gap forredacted_thinkingonly.Changes
core/providers/anthropic/responses.goReasoningTextBuffersto the stream state (mirror ofTextBuffers), covered in the pool New/acquire/flush resetsstate.OutputItemsatcontent_block_start, mirroring the redacted_thinking casethinking_deltatext per output index (with a defensive lazy map init, same pattern as the input-JSON buffers)content_block_stop, fold the accumulated text and the stored signature into the item using the exact shape the non-streaming converter produces (reasoning_textcontent block with text + signature), emitreasoning_summary_text.donewith the full text andcontent_part.donewith the completed part (mirroring the text-block path); the generic done path then emits a correctoutput_item.doneandresponse.completedcarries the itemcore/providers/anthropic/visiblethinkingresponses_test.go: four behavior tests covering full lifecycle completion, JSON round-trip replay of the streamed done item throughToAnthropicResponsesRequest(thinking with signature must precede tool_use), interleaved multi-block isolation, and signature-less thinkingcore/changelog.md: changelog entryAll four tests fail on current dev with the exact reported symptoms and pass with the fix. The redacted_thinking suite, the passthrough suite, and the full anthropic package are green;
go vetclean; golangci-lint findings identical before/after.Type of change
Affected areas
How to test
To see the wire difference live, stream a Responses request with
reasoningenabled and a tool against an Anthropic key and inspectreasoning_summary_text.done, the reasoningoutput_item.done, andresponse.completedbefore and after this change; then echoresponse.completed.outputback with afunction_call_outputas the next turn: before the fix the echoed history carries no reasoning item, after it the thinking block and its signature are preserved.Breaking changes
None. Additive on the streaming completion path; the added/delta events are unchanged, and the completed item uses the same shape the non-streaming converter already produces (which the request converter already replays).
Related issues
Fixes #5149. Sibling of #5094, which covered the redacted_thinking variant of the same lifecycle gap.
Security considerations
None. No new inputs are parsed; the change only carries already-received stream content into the completion events.
Checklist