[fix]: Bedrock provider - emit message_stop event for Anthropic invoke stream#2429
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges restructure the Bedrock provider's stream event handling to support multiple raw chunks per event, enabling the Changes
Sequence DiagramsequenceDiagram
participant Client
participant BedrockProvider as Bedrock Provider
participant AnthropicAPI as Anthropic SSE
Client->>BedrockProvider: Invoke with response stream
BedrockProvider->>AnthropicAPI: Forward request
AnthropicAPI-->>BedrockProvider: Response event: Completed
Note over BedrockProvider: Completed → multiple chunks
BedrockProvider->>BedrockProvider: Marshal message_delta payload
BedrockProvider->>BedrockProvider: Marshal message_stop payload
BedrockProvider->>BedrockProvider: Collect as [][]byte chunks
BedrockProvider->>BedrockProvider: ToEncodedEvents()
loop For each chunk in InvokeModelRawChunks
BedrockProvider-->>Client: Emit "chunk" event with raw bytes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Confidence Score: 4/5Safe to merge after fixing the broken test and adding the missing message_stop coverage test. The production logic change in invoke.go and responses.go is correct and well-scoped. However, an existing test will now always fail due to a stale field name in its local unmarshaling struct, and the promised new test covering the core fix is absent — both are concrete test failures that need resolving before CI passes. core/providers/bedrock/bedrock_test.go — stale field name in NoDuplicateContentBlockStop test and missing MessageStopEmitted test. Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/providers/bedrock/types.go (1)
600-603: Consider a transitional decode shim for the old JSON field name.Line 603 is a breaking wire-format rename; if any external/fixture JSON still sends
invokeModelRawChunk, payloads will be silently dropped. A one-release dual-read fallback can smooth migration.Proposed compatibility patch
type BedrockStreamEvent struct { @@ // For InvokeModelWithResponseStream (Legacy API) // InvokeModelRawChunks holds one or more raw byte payloads for legacy invoke stream. // Multiple chunks are needed when a single Bifrost event maps to multiple Anthropic SSE events // (e.g., Completed → message_delta + message_stop). InvokeModelRawChunks [][]byte `json:"invokeModelRawChunks,omitempty"` } + +// UnmarshalJSON supports transitional compatibility for the legacy singular field. +func (event *BedrockStreamEvent) UnmarshalJSON(data []byte) error { + type Alias BedrockStreamEvent + aux := &struct { + InvokeModelRawChunk []byte `json:"invokeModelRawChunk,omitempty"` + InvokeModelRawChunks [][]byte `json:"invokeModelRawChunks,omitempty"` + *Alias + }{ + Alias: (*Alias)(event), + } + + if err := sonic.Unmarshal(data, aux); err != nil { + return err + } + if len(aux.InvokeModelRawChunks) > 0 { + event.InvokeModelRawChunks = aux.InvokeModelRawChunks + } else if len(aux.InvokeModelRawChunk) > 0 { + event.InvokeModelRawChunks = [][]byte{aux.InvokeModelRawChunk} + } + return nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/types.go` around lines 600 - 603, The Rename of the JSON field to InvokeModelRawChunks is breaking for payloads that still send the old "invokeModelRawChunk" key — add a transitional decode shim: implement a custom UnmarshalJSON on the struct that owns the InvokeModelRawChunks field to accept either "invokeModelRawChunks" (new array form) or the legacy "invokeModelRawChunk" (single chunk) and populate InvokeModelRawChunks accordingly (wrap the single value into a slice when the legacy key is present); keep the existing json tag/output using the new "invokeModelRawChunks" name so only decoding is dual-read for one release.core/providers/bedrock/bedrock_test.go (1)
17-20:mustMarshalJSONignores the error fromjson.Marshal.Based on retrieved learnings,
must*helper functions in bedrock tests should handlejson.Marshalerrors by panicking or failing fast, not by discarding them with_.♻️ Proposed fix
func mustMarshalJSON(v interface{}) json.RawMessage { - b, _ := json.Marshal(v) + b, err := json.Marshal(v) + if err != nil { + panic(fmt.Sprintf("mustMarshalJSON: %v", err)) + } return json.RawMessage(b) }Note: A similar fix should be applied to
mustMarshalToolParamsat lines 42-45.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock_test.go` around lines 17 - 20, mustMarshalJSON currently discards the error from json.Marshal; change it to check the error and fail fast (panic) when Marshal returns an error, returning the json.RawMessage only on success; include the error text in the panic message for debugging. Apply the same pattern to mustMarshalToolParams so both helpers validate the json.Marshal error instead of ignoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/providers/bedrock/bedrock_test.go`:
- Around line 17-20: mustMarshalJSON currently discards the error from
json.Marshal; change it to check the error and fail fast (panic) when Marshal
returns an error, returning the json.RawMessage only on success; include the
error text in the panic message for debugging. Apply the same pattern to
mustMarshalToolParams so both helpers validate the json.Marshal error instead of
ignoring it.
In `@core/providers/bedrock/types.go`:
- Around line 600-603: The Rename of the JSON field to InvokeModelRawChunks is
breaking for payloads that still send the old "invokeModelRawChunk" key — add a
transitional decode shim: implement a custom UnmarshalJSON on the struct that
owns the InvokeModelRawChunks field to accept either "invokeModelRawChunks" (new
array form) or the legacy "invokeModelRawChunk" (single chunk) and populate
InvokeModelRawChunks accordingly (wrap the single value into a slice when the
legacy key is present); keep the existing json tag/output using the new
"invokeModelRawChunks" name so only decoding is dual-read for one release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf8b955-d896-4914-a2d9-4e4511e1c3da
📒 Files selected for processing (7)
core/changelog.mdcore/providers/bedrock/bedrock_test.gocore/providers/bedrock/invoke.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.gotransports/bifrost-http/integrations/bedrock.gotransports/changelog.md
tefimov
force-pushed
the
fix/bedrock-invoke-message-stop-conversion
branch
from
e762f53 to
1a5f377
Compare
The Bedrock invoke-with-response-stream endpoint converts Bifrost internal format to Anthropic Messages API SSE events. Previously, the Completed event only emitted message_delta but not message_stop, causing clients like Claude Code to hang indefinitely waiting for the stream termination signal. This change: - Renames InvokeModelRawChunk to InvokeModelRawChunks ([][]byte) to support multiple events from a single Bifrost event - Updates toAnthropicInvokeStreamBytes to return [][]byte - For Completed events, emits both message_delta and message_stop - Updates ToEncodedEvents to iterate over multiple chunks - Adds test verifying both events are emitted This fixes cross-provider routing (e.g., OpenAI → Bedrock) where the conversion layer is required. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tefimov
force-pushed
the
fix/bedrock-invoke-message-stop-conversion
branch
from
1a5f377 to
51d4bde
Compare
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/providers/bedrock/invoke.go (1)
835-849:⚠️ Potential issue | 🟠 MajorPreserve
tool_usein the finalmessage_delta.Line 837 falls back to
end_turnwheneverIncompleteDetailsis nil, buttoBedrockInvokeAnthropicResponse()already inferstool_usefrom the final output in that case. A streamed invoke completion that ends in a tool call will now emit both terminal events but still advertise a different stop reason than the non-streaming path.Suggested fix
stopReason := "end_turn" if resp.Response != nil && resp.Response.IncompleteDetails != nil { stopReason = resp.Response.IncompleteDetails.Reason + } else if resp.Response != nil { + for _, item := range resp.Response.Output { + if item.ResponsesToolMessage != nil { + stopReason = "tool_use" + break + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/invoke.go` around lines 835 - 849, The final streamed completion currently always sets stop_reason to "end_turn" when resp.Response.IncompleteDetails is nil, which loses the inferred tool-use info produced by toBedrockInvokeAnthropicResponse; update the message_delta construction in the ResponsesStreamResponseTypeCompleted case to preserve any tool_use captured on the response (e.g., use resp.Response.ToolUse or the field populated by toBedrockInvokeAnthropicResponse) by adding a "tool_use" entry to the delta when present (fall back to omitting it only if no tool_use is available). Locate the ResponsesStreamResponseTypeCompleted case in invoke.go (variable resp) and add the tool_use key into the messageDelta["delta"] map so the streamed completion emits the same tool_use as the non-streaming path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/bedrock/invoke.go`:
- Around line 835-849: The final streamed completion currently always sets
stop_reason to "end_turn" when resp.Response.IncompleteDetails is nil, which
loses the inferred tool-use info produced by toBedrockInvokeAnthropicResponse;
update the message_delta construction in the
ResponsesStreamResponseTypeCompleted case to preserve any tool_use captured on
the response (e.g., use resp.Response.ToolUse or the field populated by
toBedrockInvokeAnthropicResponse) by adding a "tool_use" entry to the delta when
present (fall back to omitting it only if no tool_use is available). Locate the
ResponsesStreamResponseTypeCompleted case in invoke.go (variable resp) and add
the tool_use key into the messageDelta["delta"] map so the streamed completion
emits the same tool_use as the non-streaming path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bedee72-bf87-4817-a2e0-1617089bc071
📒 Files selected for processing (10)
core/changelog.mdcore/providers/bedrock/batch.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/invoke.gocore/providers/bedrock/responses.gocore/providers/bedrock/types.godocs/openapi/openapi.jsondocs/openapi/schemas/integrations/bedrock/converse.yamltransports/bifrost-http/integrations/bedrock.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (4)
- core/changelog.md
- transports/changelog.md
- core/providers/bedrock/batch.go
- core/providers/bedrock/responses.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/integrations/bedrock.go
tefimov
force-pushed
the
fix/bedrock-invoke-message-stop-conversion
branch
from
51d4bde to
98ed28a
Compare
- Add error handling to mustMarshalJSON and mustMarshalToolParams test helpers - Update OpenAPI schema: rename invokeModelRawChunk to invokeModelRawChunks (array type) in converse.yaml and openapi.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tefimov
force-pushed
the
fix/bedrock-invoke-message-stop-conversion
branch
from
98ed28a to
064f0ad
Compare
|
@greptile |
akshaydeo
dismissed stale reviews from coderabbitai[bot] and themself
via
f662c87
5 participants
Summary
Fixes the Bedrock invoke-with-response-stream conversion to emit both
message_deltaandmessage_stopevents when converting from Bifrost internal format to Anthropic Messages API SSE format. Previously onlymessage_deltawas emitted, causing clients like Claude Code to hang indefinitely waiting for the stream termination signal.Changes
InvokeModelRawChunktoInvokeModelRawChunks([][]byte) to support multiple events from a single Bifrost eventtoAnthropicInvokeStreamBytesto return[][]byteinstead of[]byteCompletedevents, emit bothmessage_deltaandmessage_stopas separate eventsToEncodedEventsto iterate over multiple chunksAffected packages:
core/providers/bedrock/types.go- Field renamecore/providers/bedrock/invoke.go- Multi-event emission logiccore/providers/bedrock/responses.go- Iterate over chunkscore/providers/bedrock/bedrock_test.go- New test + error handling in helperstransports/bifrost-http/integrations/bedrock.go- Use new field namedocs/openapi/schemas/integrations/bedrock/converse.yaml- Schema updatedocs/openapi/openapi.json- Schema updateType of change
Affected areas
How to test
Breaking changes
Impact: The
BedrockStreamEvent.InvokeModelRawChunkfield has been renamed toInvokeModelRawChunksand changed from[]byteto[][]byte. Any code directly accessing this field will need to be updated.Migration: Change
event.InvokeModelRawChunktoevent.InvokeModelRawChunks[0]for single-event access, or iterate over the slice for multi-event handling.Related issues
tool_use/tool_resultcontent blocks #2311 (tool_use/tool_result blocks dropped) - that issue covers additional bugs in the same code path that are NOT fixed by this PRSecurity considerations
None - this is a bug fix for event emission.
Checklist
docs/contributing/README.mdand followed the guidelines🤖 Generated with Claude Code