fix(bedrock): omit toolChoice.tool on Llama for synthetic structured-output tool#3196
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds Llama-aware tool-choice handling in the Bedrock provider: a new ChangesBedrock Llama Tool-Choice Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 6/8 reviews remaining, refill in 7 minutes and 35 seconds.Comment |
Confidence Score: 5/5Safe to merge — the fix is narrowly scoped to Llama models, all previously failing paths now have gates, non-Llama behavior is unchanged and regression-guarded by tests. No P0/P1 findings. The prior P1 comment (missing Llama gate for explicit tool_choice on the Responses API path) is fully addressed in this version. The No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(bedrock): omit toolChoice.tool on Ll..." | Re-trigger Greptile |
…output tool
Bedrock Converse rejects toolConfig.toolChoice.tool on Meta Llama variants
with HTTP 400 ("This model doesn't support the toolConfig.toolChoice.tool
field. Remove toolConfig.toolChoice.tool and try again."). The synthetic
bf_so_* tool path used for structured output unconditionally pinned the
synthetic tool via toolChoice.tool, which broke every with_structured_output
caller against Llama 4 Maverick / Scout / Llama 3.x on Bedrock.
This patch adds an IsLlamaModel helper next to the existing IsNovaModel /
IsAnthropicModel family and gates the forced-tool emission off it in four
places (covering both API surfaces and both the synthetic and explicit
tool_choice paths on each):
1. core/providers/bedrock/utils.go: ChatCompletions synthetic-tool injection
(response_format=json_schema -> bf_so_* tool path).
2. core/providers/bedrock/responses.go: OpenAI Responses API mirror of maximhq#1
(text.format=json_schema -> bf_so_* tool path).
3. core/providers/bedrock/utils.go: convertToolConfigFromFiltered's explicit
tool_choice path (defense-in-depth for ChatCompletions callers using
bind_tools(tool_choice={"type": "function", "function": {"name": "X"}})
directly).
4. core/providers/bedrock/responses.go: Responses API mirror of maximhq#3 — the
explicit tool_choice path runs convertResponsesToolChoice and writes
BedrockToolChoice{Tool: ...} straight onto the request, which trips the
same Llama 400 without the gate.
With one synthetic tool bound and Bedrock's default "auto" behavior, omitting
tool_choice yields the same outcome on Llama because there's exactly one tool
the model can call. This mirrors the gate langchain-aws ships
(supports_tool_choice_values for llama4 / llama3-1 / llama3-3 sets only
"auto", causing with_structured_output to emit no tool_choice at all).
Tests added (mirroring PR maximhq#3184's TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool
naming and structure):
- TestBedrockLlamaChatStructuredOutputOmitsForcedToolChoice — synthetic tool
present, ToolChoice nil for Llama on the ChatCompletions path.
- TestBedrockNonLlamaChatStructuredOutputForcesToolChoice — regression guard:
Nova / Anthropic still get the forced tool_choice (gate doesn't over-fire).
- TestToBedrockResponsesRequest_LlamaStructuredOutputOmitsForcedToolChoice —
same negative+positive on the Responses API synthetic-tool path.
- TestBedrockLlamaConvertToolConfigOmitsForcedToolChoice — defense-in-depth
coverage for ChatCompletions explicit tool_choice struct callers.
- TestToBedrockResponsesRequest_LlamaConvertResponsesToolChoiceOmitsForcedToolChoice
— defense-in-depth coverage for Responses API explicit tool_choice
(function-typed) struct callers — the gap surfaced in code review.
- TestToBedrockResponsesRequest_NonLlamaConvertResponsesToolChoiceForcesToolChoice
— Anthropic regression guard for maximhq#5 so the Responses-side Llama gate
doesn't over-fire.
Refs:
- https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolChoice.html
- https://github.com/langchain-ai/langchain-aws (bedrock_converse.py
supports_tool_choice_values family-detection lines 678-711).
- PR maximhq#3184 (Anthropic structured-output fallback to synthetic tool path) —
immediate predecessor; this PR completes the per-family gating story.
ryan-orphic
force-pushed
the
upstream/llama-toolchoice-gate
branch
from
bf85e94 to
a49a95f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/responses.go (1)
1959-1993:⚠️ Potential issue | 🔴 CriticalAdd Llama guard for explicit user tool_choice in Responses path.
The chat completions path (
convertToolConfigFromFiltered, utils.go:1575–1580) drops.Tool-type choices on Llama models to prevent the HTTP 400 rejection. The Responses path lacks this defense for explicit user-providedtool_choice.When a caller passes both
text.format=json_schemaand an explicittool_choice: {type:"function", name:"some_fn"}against Llama:
- Lines 1960–1968 convert the explicit choice →
bedrockReq.ToolConfig.ToolChoice = {Tool: {Name:"some_fn"}}- Line 1986 guard only protects the synthetic tool injection, not the user choice from step 1
- Result: Bedrock still receives
toolChoice.tool→ HTTP 400 on LlamaAdd a Llama gate immediately after line 1968 to strip
.Toolchoices on Llama models, mirroring the chat path logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/responses.go` around lines 1959 - 1993, After converting an explicit user tool choice via convertResponsesToolChoice (when bifrostReq.Params.ToolChoice != nil) ensure you strip any .Tool entries for Llama models: after obtaining bedrockToolChoice and before assigning it to bedrockReq.ToolConfig.ToolChoice, check schemas.IsLlamaModel(bifrostReq.Model) and if true remove/null out the Tool field on bedrockToolChoice (so only non-Tool choices remain) then continue assigning; this mirrors the chat-path guard and prevents sending toolChoice.tool to Bedrock for Llama models.
🤖 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/responses.go`:
- Around line 1959-1993: After converting an explicit user tool choice via
convertResponsesToolChoice (when bifrostReq.Params.ToolChoice != nil) ensure you
strip any .Tool entries for Llama models: after obtaining bedrockToolChoice and
before assigning it to bedrockReq.ToolConfig.ToolChoice, check
schemas.IsLlamaModel(bifrostReq.Model) and if true remove/null out the Tool
field on bedrockToolChoice (so only non-Tool choices remain) then continue
assigning; this mirrors the chat-path guard and prevents sending toolChoice.tool
to Bedrock for Llama models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cdb07cf-68cf-4c4c-9aa7-02f332422771
📒 Files selected for processing (4)
core/providers/bedrock/bedrock_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/utils.go
|
Thanks @greptile-apps — good catch on the Responses API explicit `tool_choice` gap. The Chat Completions path had defense-in-depth via `convertToolConfigFromFiltered`, but the analogous gate on `responses.go` lines 1962–1971 was missing. Just pushed an amend that closes that gap:
Commit message updated to call out four gate sites instead of three (two API surfaces × two paths each: synthetic-tool injection + explicit `tool_choice`). |
|
The diff confirms all four gate sites are now covered:
The two gates in One small note on Six tests covering both positive (tool survives) and negative (forced pin dropped) assertions across all four sites — that's thorough. LGTM. Tip: You can customize Greptile's behavior for this repo with |
|
@akshaydeo Thank you. How long does it usually take for changes to be included in your enterprise builds? |
3 participants
Summary
Bedrock Converse rejects
toolConfig.toolChoice.toolon Meta Llama variants with HTTP 400:The synthetic
bf_so_*tool path used for structured output (introduced in #3184) unconditionally pinned the synthetic tool viatoolChoice.tool, which broke everywith_structured_output(...)/response_format=json_schemacaller against Llama 4 Maverick / Scout / Llama 3.x on Bedrock.This PR completes the per-family gating story #3184 started for Anthropic.
Changes
Adds an
IsLlamaModelhelper next to the existingIsNovaModel/IsAnthropicModelfamily incore/schemas/utils.go, then gates the forced-tool emission off it in three places:core/providers/bedrock/utils.go— ChatCompletions synthetic-tool injection (response_format=json_schema→bf_so_*tool path). Synthetic tool is still injected; only the forcedtoolChoice.toolis suppressed.core/providers/bedrock/responses.go— OpenAI Responses API mirror path (text.format=json_schema→bf_so_*tool path). Same shape as Refactor Bifrost for plugin support and multi-provider architecture along with tests #1.core/providers/bedrock/utils.go—convertToolConfigFromFiltered's explicittool_choicepath (defense-in-depth for callers usingbind_tools(tool_choice={\"type\": \"function\", \"function\": {\"name\": \"X\"}})directly).With one synthetic tool bound and Bedrock's default "auto" behavior, omitting
tool_choiceyields the same outcome on Llama because there's exactly one tool the model can call. This mirrors the gatelangchain-awsships (supports_tool_choice_valuesonly allows "auto" forllama4,llama3-1,llama3-3model families, causingwith_structured_outputto emit notool_choiceat all).Type of change
Affected areas
How to test
Tests added (mirroring #3184's `TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool` naming + structure):
```sh
go test -count=1 -run "TestBedrockLlama|TestBedrockNonLlama|TestToBedrockResponsesRequest_Llama" ./core/providers/bedrock/...
```
Live verification: ran a forked build of this patch behind production CRS traffic (LangChain `with_structured_output(strict=True)` against `us.meta.llama4-maverick-17b-instruct-v1:0` on Bedrock) — previously 400 ValidationException on every planner request; with this gate, structured output succeeds end-to-end.
Breaking changes
The gate only narrows behavior for Llama (where the prior code path was already failing 100% of requests with HTTP 400). Nova / Anthropic / non-Llama paths are unchanged and covered by the negative regression test.
Related issues
Companion to #3184 — that PR added the synthetic-tool fallback for Bedrock Converse `response_format=json_schema`, which made the forced-tool emission load-bearing for all structured-output callers. This PR completes that work for the Llama family, which has a stricter `toolChoice` support matrix than Anthropic / Nova.
Security considerations
No new auth surfaces, secrets, PII, or sandboxing changes. Internal request shaping only.
Checklist
References