fix: emit contentBlockStop events on the Bedrock ConverseStream egress - #4923
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBedrock ConverseStream conversion now explicitly emits ChangesContentBlockStop Emission
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Stream as BifrostResponsesStreamResponse
participant Converter as ToBedrockConverseStreamResponse
participant Event as BedrockStreamEvent
participant Encoder as ToEncodedEvents
Stream->>Converter: output_item.done
Converter->>Event: set ContentBlockStop=true, ContentBlockIndex
Event->>Encoder: ToEncodedEvents()
Encoder->>Encoder: append contentBlockStop event with ContentBlockIndex
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 |
Merge activity
|
## Summary Adds end-to-end test coverage for a set of provider egress streaming and truncation correctness bugs. The new tests assert that Bedrock `converse-stream` properly closes content blocks before terminating, that Anthropic normalized and Claude Code passthrough streams emit contiguous `content_block_start` indices starting from 0, and that Bedrock Responses API truncated responses correctly signal `status=incomplete` with `reason=max_output_tokens` in both streaming and non-streaming modes. ## Changes - Added a `contentBlockStop`-before-`messageStop` assertion to the existing Bedrock `converse-stream` basic test to catch #4923. - Added a `content_block_start` index contiguity check to the existing Anthropic normalized streaming test to catch gaps introduced by server-tool rewrites (#4890 / #4932). - Added a new **section 17 – Provider Egress Streaming/Truncation Guards** with four requests: - Bedrock forced-tool `converse-stream` verifies `toolUse`, `contentBlockStop`, and `messageStop` ordering (#4923). - Anthropic normalized `web_fetch` streaming verifies contiguous `content_block_start` indices (#4932). - Bedrock Responses non-streaming truncation verifies `status=incomplete` and `incomplete_details.reason=max_output_tokens` (#4680). - Bedrock Responses streaming truncation verifies `response.incomplete` is emitted and `response.completed` is absent (#4680). - Added a new **section 18 – Claude Code Passthrough server-tool streaming index contiguity** with three requests covering `web_search` (normal results), `web_search` (zero results), and `web_fetch` via the `claude-cli` User-Agent passthrough path (#4890). ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the Postman/Newman collection against a live Bifrost instance: ```sh newman run tests/e2e/api/collections/provider-harness.json \ --env-var baseUrl=<BIFROST_URL> \ --env-var bedrockModel=<BEDROCK_MODEL_ID> \ --env-var anthropicKey=<ANTHROPIC_API_KEY> ``` All tests in sections 17 and 18 should pass. Specifically: - Bedrock `converse-stream` responses must contain `contentBlockStop` before `messageStop`. - All Anthropic streaming responses must have `content_block_start` indices `[0, 1, 2, …]` with no gaps. - Bedrock Responses truncated (non-streaming) must return `status=incomplete` with `incomplete_details.reason=max_output_tokens`. - Bedrock Responses truncated (streaming) must emit `response.incomplete` and must **not** emit `response.completed`. ## Breaking changes - [x] No ## Related issues Closes #4923, #4932, #4890, #4680 ## Security considerations None. These are read-only test assertions against existing API endpoints; no new credentials or secrets are introduced beyond those already required by the collection. ## 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 #4262. The Bedrock ConverseStream egress (
/bedrock/model/{modelId}/converse-stream) never emittedcontentBlockStopevents: content blocks were opened and filled with deltas, but never closed beforemessageStop. The AWS contract terminates every content block with acontentBlockStopevent carrying the block'scontentBlockIndex(a required field), and consumers that assemble the final message on block boundaries (the AWS SDK stream unions, frameworks like strands) only finalize a block when they receive it. Through Bifrost they received all the deltas and still ended up with an empty assembled message, so the break is silent: live delta readers see output, assembling consumers get nothing.The invoke egress (
invoke-with-response-stream, Anthropic Messages shape) already mapsOutputItemDonetocontent_block_stop. The Converse path dropped the same event with a comment saying Bedrock has no done events, which holds for the part-level done events but not at the block level.Changes
ToBedrockConverseStreamResponsenow mapsOutputItemDoneto acontentBlockStopevent carrying the same content block index the block's start and delta events use, mirroring the invoke path.OutputTextDone,ContentPartDoneandReasoningSummaryTextDonestay skipped, so each block is closed exactly once, on its item's done event.BedrockStreamEventgets an explicitContentBlockStopmarker: the wire payload ofcontentBlockStopis just{"contentBlockIndex": n}, so the flat event union had no way to represent it. The marker is never serialized;ToEncodedEventsbuilds the payload fromContentBlockIndex.ToEncodedEventsencodes the new event after deltas and beforemessageStop, so a text stream now encodes asmessageStart,contentBlockDelta(xN),contentBlockStop,messageStop,metadata.Two notes on scope:
contentBlockStartfor text blocks. Real ConverseStream responses do not do that: theContentBlockStartunion only hasimage,toolResultandtoolUsemembers, so text blocks start implicitly with their first delta. The current behavior already matches the contract there, so this PR leaves the start events unchanged.invoke-with-response-streamdelegate to the same Converse converter, so that path picks up the block terminator as well, which also matches Nova's native stream shape.Type of change
Affected areas
How to test
All three tests fail on
devwithout this change, with the exact event sequence from the issue report (messageStart, contentBlockDelta, contentBlockDelta, messageStop, metadataand nocontentBlockStop), and pass with it.go build ./...and the full./providers/bedrock/package suite are green.Screenshots/Recordings
N/A
Breaking changes
Consumers that only read
contentBlockDeltaare unaffected. The new event is part of the documentedConverseStreamOutputunion, so Bedrock clients already handle it.Related issues
Closes #4262
Security considerations
None. This only adds a lifecycle event to the outgoing stream encoding.
Checklist
docs/contributing/README.mdand followed the guidelines