Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughBedrock Responses streaming now buffers reasoning text and signatures, preserves citations, tracks output items for ChangesBedrock streaming terminal payloads
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change completes Bedrock streaming terminal payloads and reasoning, citation, and structured-output events; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BedrockStreamEvent
participant BedrockResponsesStreamState
participant FinalizeBedrockStream
participant response.completed
BedrockStreamEvent->>BedrockResponsesStreamState: buffer reasoning, citations, and output fragments
BedrockResponsesStreamState->>FinalizeBedrockStream: provide cached items and late metadata
FinalizeBedrockStream->>response.completed: emit ordered output items
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/providers/bedrock/streamterminalevents_test.go (1)
467-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the test's manual interception mirrors the handler's ordering.
This test reproduces the handler logic by hand (
beginStructuredOutputTextItemthenappendStructuredOutputText), so it can drift fromBedrockProvider.ResponsesStreamincore/providers/bedrock/bedrock.go(Lines 1885-1934) without failing. In particular the handler also setsisAccumulatingStructuredOutputand gates the delta branch on it, and it suppresses non-tool text/reasoning deltas — neither is exercised here. Consider adding a case that feeds the suppression path (a text delta between tool-arg deltas) so a regression in the handler's gating is caught.🤖 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/bedrock/streamterminalevents_test.go` around lines 467 - 527, Extend TestBedrockResponsesStreamStructuredOutputSnapshot to exercise BedrockProvider.ResponsesStream’s structured-output gating, including setting isAccumulatingStructuredOutput and inserting a non-tool text or reasoning delta between tool-argument deltas. Assert that the suppressed delta produces no output while the tool fragments remain ordered and the final structured-output snapshot is unchanged.core/providers/bedrock/bedrock.go (1)
1894-1907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the chunk-emit loop; it is now duplicated three times.
Lines 1894-1907, 1921-1934, and the generic path at Lines 1957-1972 are the same body: stamp
ExtraFields, bumpchunkIndex/lastChunkTime, optionally attach the raw payload, send. The two new copies also drop theresponse != nilguard the generic loop has. A small closure overctx/chunkIndex/lastChunkTime/message.Payloadkeeps them from drifting.♻️ Suggested closure
// Declare once, after lastChunkTime is initialized: emit := func(responses []*schemas.BifrostResponsesStreamResponse, rawPayload []byte) { for _, response := range responses { if response == nil { continue } response.ExtraFields = schemas.BifrostResponseExtraFields{ ChunkIndex: chunkIndex, Latency: time.Since(lastChunkTime).Milliseconds(), } chunkIndex++ lastChunkTime = time.Now() if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { response.ExtraFields.RawResponse = string(rawPayload) } providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil, nil), responseChan, postHookSpanFinalizer) } }Then both new blocks collapse to:
- for _, response := range streamState.appendStructuredOutputText(streamEvent.Delta.ToolUse.Input, contentBlockIndex, chunkIndex) { - response.ExtraFields = schemas.BifrostResponseExtraFields{ - ChunkIndex: chunkIndex, - Latency: time.Since(lastChunkTime).Milliseconds(), - } - chunkIndex++ - lastChunkTime = time.Now() - - if providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse) { - response.ExtraFields.RawResponse = string(message.Payload) - } - - providerUtils.ProcessAndSendResponse(ctx, postHookRunner, providerUtils.GetBifrostResponseForStreamResponse(nil, nil, response, nil, nil, nil), responseChan, postHookSpanFinalizer) - } + emit(streamState.appendStructuredOutputText(streamEvent.Delta.ToolUse.Input, contentBlockIndex, chunkIndex), message.Payload)Also applies to: 1921-1934
🤖 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/bedrock/bedrock.go` around lines 1894 - 1907, Extract the duplicated response-processing loop into a single closure near the initialization of lastChunkTime, using captured ctx, chunkIndex, lastChunkTime, and the existing send dependencies. Have the closure skip nil responses, stamp ExtraFields, update chunk timing, attach the optional raw payload, and send each response; replace the loops at the structured-output paths and generic path with calls to this closure.
🤖 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/bedrock/responses.go`:
- Around line 1237-1239: Keep the assignment in the reasoningContentDelta
handling path as a direct replacement of state.ReasoningSignatures[outputIndex]
with the single reasoningDelta.Signature value. Do not append or accumulate
signature data, since each source delta carries only one signature payload;
update the surrounding comment only if the source contract changes.
---
Nitpick comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1894-1907: Extract the duplicated response-processing loop into a
single closure near the initialization of lastChunkTime, using captured ctx,
chunkIndex, lastChunkTime, and the existing send dependencies. Have the closure
skip nil responses, stamp ExtraFields, update chunk timing, attach the optional
raw payload, and send each response; replace the loops at the structured-output
paths and generic path with calls to this closure.
In `@core/providers/bedrock/streamterminalevents_test.go`:
- Around line 467-527: Extend TestBedrockResponsesStreamStructuredOutputSnapshot
to exercise BedrockProvider.ResponsesStream’s structured-output gating,
including setting isAccumulatingStructuredOutput and inserting a non-tool text
or reasoning delta between tool-argument deltas. Assert that the suppressed
delta produces no output while the tool fragments remain ordered and the final
structured-output snapshot is unchanged.
🪄 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: b45241da-c5eb-4c53-a248-942a09558093
📒 Files selected for processing (4)
core/changelog.mdcore/providers/bedrock/bedrock.gocore/providers/bedrock/responses.gocore/providers/bedrock/streamterminalevents_test.go
41828b9 to
b622043
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
b622043 to
cda61a5
Compare
The merge-base changed after approval.
cda61a5 to
7691011
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The merge-base changed after approval.
244a01d to
ce1b2a6
Compare
7691011 to
828d787
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The merge-base changed after approval.
|
|
…tput array and reasoning payloads
828d787 to
d465b76
Compare
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
Streaming
/v1/responseson Bedrock never completed its terminal events:response.completedcarried no output array ("output":null), reasoning done events were empty shells at all three lazy close sites (thinking deltas were never buffered, unlike text deltas since #3838), the thinking signature only existed as an in-flight delta, and citation annotations never reached the done events or the snapshot.Structured-output streams were the worst case: the interception emitted bare text deltas with no item, no indices, no item id, so those streams produced no output_item events at all.
The non-streaming path returns all of this correctly. The streamed reasoning item was not even replayable: it had neither the content blocks the request converter reads nor a filled summary, so the next Converse request rebuilt zero
reasoningContentblocks and silently lost the signed thinking chain.Fourth and last member of the terminal-events series, after Anthropic (#5150), Gemini (#5260), and Cohere (#5386). Fixes #5667.
Changes
BedrockResponsesStreamState: anOutputItemsmap fed by a smalltrackOutputItemshelper whereveroutput_item.added/output_item.doneare emitted, with a guard so an added shell never overwrites a completed item. The terminal snapshot'soutputis filled from it, sorted by output index, on bothresponse.completedandresponse.incomplete, mirroring the Anthropic ingress.ReasoningTextBuffers) and the signature delta (ReasoningSignatures), and replace the three duplicated reasoning close sites with onecloseReasoningItemhelper that emits the done trio with the accumulated text and signature. The done item carries the payload as a reasoning content block next to the empty summary, the shape the non-streaming converter produces and the request converters replay, signature included. Reasoning text delta and done events now carrysummary_index(0), which the OpenAI event types require.output_item.addedpluscontent_part.added), argument fragments are buffered and emitted asoutput_text.deltawith output_index/content_index/item_id (previously bare), and the end-of-stream text close completes it. The tool_calls-to-stop stop-reason downgrade is preserved.acquireandflush, same idiom as the existing ones, and add a changelog entry.Type of change
Affected areas
How to test
The ten new tests in
streamterminalevents_test.goreplay Converse event sequences throughToBifrostResponsesStreamandFinalizeBedrockStream: the done trio at each of the three close sites, the completed output array and its payloads, annotations on the done events when they arrive before the close and in the snapshot either way, two concurrent reasoning blocks, the structured-output item, pooled-state recycling, and a full replay chain (the streamed reasoning and function-call items echoed back throughToBedrockResponsesRequestrebuild the ConversereasoningContentblock with its text and signature before thetoolUseblock). Nine of them fail on dev without the fix with these exact symptoms; the structured-output test drives the interception helpers added here, since dev kept no state on that path and there was nothing to pin.Screenshots/Recordings
Not a UI change.
Breaking changes
Terminal events that carried empty payloads now carry the accumulated ones, and the snapshot now includes the
outputarray, matching the non-streaming path. The framework accumulator folds deltas and ignores done events, so logged messages are unchanged (verified). The Bedrock-format, invoke, and Anthropic-format egress serializers never read the output array from the completed event, so they are unaffected (verified).Related issues
Closes #5667
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