fix: Bedrock parallel tool calls #1165
Conversation
When using parallel tool calls with Bedrock, messages need to be grouped into a single message having multiple toolResult blocks. Without grouping messages like this, parallel tool calls, at least when using Anthropic's models, will cause following error: Expected toolResult blocks at messages.N.content for the following Ids: tooluse_xyz This commit groups parallel tool calls into a single user message.
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR fixes parallel tool calls for Bedrock by refactoring tool message conversion to aggregate consecutive tool messages into a single user message with multiple tool result blocks, matching Bedrock's API requirement for grouped parallel tool calls. A test case validates the conversion behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/providers/bedrock/bedrock_test.gocore/providers/bedrock/utils.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/bedrock/bedrock_test.gocore/providers/bedrock/utils.go
🧠 Learnings (4)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/bedrock/bedrock_test.gocore/providers/bedrock/utils.go
📚 Learning: 2025-12-11T07:38:31.413Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/bedrock/bedrock_test.go:1374-1390
Timestamp: 2025-12-11T07:38:31.413Z
Learning: In core/providers/bedrock tests, follow a layered testing approach: - Unit tests (e.g., TestBifrostToBedrockResponseConversion) should perform structural comparisons and type/field checks to avoid brittleness from dynamic fields. - Separate scenario-based and integration tests should validate the full end-to-end conversion logic, including content block internals. Ensure unit tests avoid brittle string/field matching and that integration tests cover end-to-end behavior with realistic data.
Applied to files:
core/providers/bedrock/bedrock_test.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/bedrock/bedrock_test.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/bedrock/bedrock_test.gocore/providers/bedrock/utils.go
🧬 Code graph analysis (1)
core/providers/bedrock/utils.go (7)
core/schemas/chatcompletions.go (3)
ChatMessage(478-487)ChatMessageRoleTool(473-473)ChatToolMessage(644-646)ui/lib/types/logs.ts (1)
ChatMessage(108-118)core/providers/gemini/types.go (3)
Role(17-17)Content(981-989)Type(782-782)core/providers/bedrock/types.go (5)
BedrockMessage(150-153)BedrockContentBlock(163-190)BedrockCachePoint(199-201)BedrockCachePointTypeDefault(195-195)BedrockToolResult(234-238)core/providers/bedrock/responses.go (1)
ToolResult(1952-1957)ui/lib/constants/logs.ts (1)
Status(163-163)core/schemas/utils.go (1)
Ptr(16-18)
🔇 Additional comments (4)
core/providers/bedrock/utils.go (3)
274-309: LGTM: Loop control flow correctly handles consecutive tool message grouping.The look-ahead logic properly collects consecutive tool messages and advances the index to skip already-processed messages. The outer loop index update ensures each message is processed exactly once.
390-442: LGTM: Content handling correctly processes multiple formats.The logic properly handles JSON parsing with text fallback, image conversion, and cache control blocks. The approach matches the pattern used in
convertContentBlockand correctly builds nested content structures for tool results.
376-380: LGTM: Function signature correctly reflects multi-message semantics.The rename from
convertToolMessagetoconvertToolMessagesand the parameter change to a slice accurately represent the new aggregation behavior. Error messages are consistently updated throughout.core/providers/bedrock/bedrock_test.go (1)
451-600: LGTM: Comprehensive test validates parallel tool call grouping.The test case directly validates the fix for issue #1164 by verifying that consecutive tool messages are converted into a single Bedrock user message with multiple tool result blocks. The test data is well-formed with consistent tool IDs and names, and the expected output correctly matches Bedrock's required format.
| toolResultBlock := BedrockContentBlock{ | ||
| ToolResult: &BedrockToolResult{ | ||
| ToolUseID: *msg.ChatToolMessage.ToolCallID, | ||
| Content: toolResultContent, | ||
| Status: schemas.Ptr("success"), // Default to success | ||
| }, | ||
| } | ||
|
|
||
| contentBlocks = append(contentBlocks, toolResultBlock) |
There was a problem hiding this comment.
Add nil check for ToolCallID to prevent potential panic.
Line 447 dereferences msg.ChatToolMessage.ToolCallID without checking if it's nil. While the schema design suggests this should always be present for tool messages, defensive programming is recommended to prevent runtime panics on malformed input.
🔎 Proposed fix
+ // Validate required field
+ if msg.ChatToolMessage.ToolCallID == nil {
+ return BedrockMessage{}, fmt.Errorf("tool message missing required ToolCallID")
+ }
+
// Create tool result content block for this tool message
toolResultBlock := BedrockContentBlock{
ToolResult: &BedrockToolResult{
ToolUseID: *msg.ChatToolMessage.ToolCallID,
Content: toolResultContent,
Status: schemas.Ptr("success"), // Default to success
},
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/providers/bedrock/utils.go around lines 445 to 453, the code
dereferences msg.ChatToolMessage.ToolCallID without checking for nil; add a nil
check before using *msg.ChatToolMessage.ToolCallID and handle the missing ID
defensively — e.g., if ToolCallID is nil, log a warning (using the existing
logger in scope or fmt.Printf if none) and skip creating/appending the
toolResultBlock (or create it with a safe fallback ID and an error status),
otherwise proceed to dereference and append as before.
|
hi @elahti thanks for the PR ❤️ - could you resolve the coderabbit comments. We will do a final review once its ready |
2 participants
Summary
This PR fixes the bug described in #1164 : parallel tool calls don't work when using Bedrock. In this PR parallel tool calls are grouped into one user message, which is the behavior that Bedrock expects.
The problem is a Bedrock-specific one and doesn't affect eg. models provided via Azure. Different LLMs behaving a bit differently, without testing all models via Bedrock it is impossible to say if the problem is limited to specific set of models. I've verified that the problem exists with Anthropic's models at least.
Type of change
Affected areas
How to test
See the
cURLcommand I used from #1164.Breaking changes
This should not be a breaking change. As stated above, as different LLMs might not behave the same way even within same provider, it might be a good idea to run tests for other LLMs via beedrock than Anthropic's. Unfortunately I only have access to Anthropic's models via Bedrock.
This change was directly to Bedrock specific code in the
coremodule, so it doesn't affect other providers.Related issues
Closes #1164
Checklist
docs/contributing/README.mdand followed the guidelines