fix: complete Cohere streaming Responses terminal events with the output array and reasoning payloads - #5386
Conversation
…put array and reasoning payloads
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCohere Responses terminal events now preserve reasoning text, citations, output items, tool-plan completion, usage, and stop reasons. Non-streaming responses map stop reasons, with tests covering terminal snapshots, ordering, recycling, and replayability. ChangesCohere Responses terminal event handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CohereStreamEvent
participant CohereResponsesStreamState
participant response_completed
CohereStreamEvent->>CohereResponsesStreamState: process streamed content and citations
CohereStreamEvent->>CohereResponsesStreamState: process MessageEnd and FinishReason
CohereResponsesStreamState->>CohereResponsesStreamState: close tool plan and fold annotations
CohereResponsesStreamState->>response_completed: emit ordered output and stop reason
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: allocate the tool-plan output index..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/cohere/streamterminalevents_test.go (1)
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a pointer helper over the address operator (
&) for variables.Based on learnings, prefer using a pointer helper function (like
schemas.Ptr, which is heavily used in this file) instead of the address operator (&) when taking the address of a local variable. This improves readability and keeps the code consistent with the repository's preferred pattern.
core/providers/cohere/streamterminalevents_test.go#L162-L162: Replace&documentwithschemas.Ptr(document).core/providers/cohere/streamterminalevents_test.go#L179-L179: Replace&finishReasonwithschemas.Ptr(finishReason).core/providers/cohere/streamterminalevents_test.go#L533-L533: Replace&finishReasonwithschemas.Ptr(finishReason).🤖 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/cohere/streamterminalevents_test.go` at line 162, Replace direct address operators with schemas.Ptr for the local variables at core/providers/cohere/streamterminalevents_test.go lines 162, 179, and 533: use schemas.Ptr(document) and schemas.Ptr(finishReason) at the respective sites, preserving the existing test data and 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/cohere/streamterminalevents_test.go`:
- Line 162: Replace direct address operators with schemas.Ptr for the local
variables at core/providers/cohere/streamterminalevents_test.go lines 162, 179,
and 533: use schemas.Ptr(document) and schemas.Ptr(finishReason) at the
respective sites, preserving the existing test data and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 97437bd2-e1ce-4e52-a167-6a524a5f9198
📒 Files selected for processing (3)
core/changelog.mdcore/providers/cohere/responses.gocore/providers/cohere/streamterminalevents_test.go
… items cannot overwrite it
Summary
Streaming
/v1/responseson Cohere never completed its terminal events:response.completedcarried no output array at all ("output":null, every item type missing from the final snapshot) and nostop_reason, reasoning done events were empty shells (thinking deltas were never buffered, unlike text deltas since #3838), and citation annotation events carried noitem_id. The non-streaming path returns the output array and the reasoning content correctly, and the Anthropic stream ingress already populatesOutputfrom per-item bookkeeping, so these were Cohere-only gaps. The streamed reasoning shape was also not replayable: it had neither the content blocks the request converter reads nor a filled summary, so conversations coming from streaming silently lost their thinking blocks on the next turn. Fixes #5385.Changes
CohereResponsesStreamState(OutputItemsmap fed by a smalltrackOutputItemshelper whereveroutput_item.added/output_item.doneare emitted) and populateresponse.completed.response.outputfrom it, sorted by output index, mirroring the Anthropic ingress.TextBuffersthe way text deltas already are, and emitreasoning_summary_text.done, the reasoningcontent_part.done, and the reasoningoutput_item.donewith the accumulated text. The done item carries the text as a reasoning content block, the exact shape the non-streaming converter produces, which is whatconvertBifrostReasoningToCohereThinkingreplays. The reasoning delta and done events also carrysummary_index(0; Cohere thinking has a single summary part per item), which the official OpenAI event types require.item_idonoutput_text.annotation.added/done, include the already-streamed annotations in the text item's done events, and defensively fold annotations into the completed snapshot regardless of whether they arrive before or aftercontent-end, with clone-on-write so already-emitted events are never mutated.delta.finish_reasononmessage-end(andfinish_reasonon the non-streaming Responses conversion) through the existingConvertCohereFinishReasonToBifrost, soresponse.completedand the non-streaming response carrystop_reasonthe way the chat surface already does. This also fixes the Anthropic-format egress synthesizingend_turnfor Cohere tool-use turns.content-startandtool-call-start) into acloseToolPlanItemhelper and also call it onmessage-end, so a stream ending right after a tool plan completes the item beforeresponse.completed.Type of change
Affected areas
How to test
The nine new tests in
streamterminalevents_test.goconstruct Cohere v2 stream event sequences (thinking deltas, text, tool plan, tool calls, citations arriving both during and after their content block) and drive them throughToBifrostResponsesStream, pinning: done events carrying the accumulated thinking text,response.completedcarrying the full output array with the actual payloads and the mappedstop_reason,item_idon annotation events, citations present in the done events and the final snapshot, the tool-plan item completed at message-end, no state leakage across pooled-state recycling, a full replay chain (completed reasoning item JSON-echoed back throughConvertBifrostMessagesToCohereMessagesrecovers the thinking block), and the non-streamingstop_reasonmapping. All nine fail on dev without the fix with the exact symptoms above.Screenshots/Recordings
Not a UI change.
Breaking changes
Terminal events that previously carried empty payloads now carry the accumulated ones, and
response.completednow includes theoutputarray andstop_reason, matching the non-streaming path. The framework stream accumulator folds deltas and ignores done events, so logged messages are unchanged (verified). The Anthropic-format egress ignores the enriched done payloads and only gains the correct terminalstop_reason.Related issues
Closes #5385
Security considerations
None. No new inputs are parsed; the change only carries already-received stream content through to the terminal events.
Checklist
docs/contributing/README.mdand followed the guidelines