Repository navigation
fix(openai): preserve allowed_tools tool_choice across Chat Completions routes, refs #2504 - #2519
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe OpenAI transformer now supports ChangesOpenAI allowed_tools support
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant OpenAIRequest
participant ToLLMRequest
participant LLMRequest
participant RequestFromLLM
OpenAIRequest->>ToLLMRequest: Convert allowed_tools choice
ToLLMRequest->>LLMRequest: Store mode and allowed tool options
LLMRequest->>RequestFromLLM: Convert tool choice
RequestFromLLM->>OpenAIRequest: Restore nested allowed_tools shape
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 |
) (#2639) Model Anthropic's `allowed_callers` tool field on the wire Tool struct and on the unified llm.Function, and copy it in both conversion directions so it is no longer dropped. This is the last remaining sub-item of #2504: the other five dropped tool fields were already fixed by #2519, #2523 and #2539 and are left untouched. Only function/custom tools are mapped; server tools (web_search) have no documented allowed_callers equivalent in this code path and stay unchanged. An absent value is preserved as absent (never `[]` or `null`).
Refs #2504 (case 1).
Problem
On Chat Completions routes, a
tool_choiceof typeallowed_toolswas forwarded upstream as{"type":"allowed_tools","function":{"name":""}}— an empty function name, with the caller's mode and tool subset silently gone.Chat Completions nests the choice as
{"type":"allowed_tools","allowed_tools":{"mode":"required","tools":[{"type":"function","function":{"name":"tool_a"}}]}}, butToolChoice.UnmarshalJSONparsed any object as the plain named shape{type, function}; the nestedallowed_toolskey was dropped at decode time, and the outbound converter then re-emitted the named shape with an emptyfunction.name. A restriction the caller set was lifted without any error.Fix
model.go:ToolChoicegains anAllowedToolsfield; unmarshal decodes the nestedallowed_toolsobject (mode + tool subset) before the named fallback, and marshal emits the nested shape for anallowed_toolschoice — it can no longer produce an empty function name for it.inbound_convert.go: the parsed mode and subset now land in the unifiedllm.ToolChoice(mode inToolChoice, subset inTools), the same representation the Responses transformer already uses since feat(llm): response namespace transfomer #2468.outbound_convert.go: aToolChoiceFromLLMhelper rebuilds the nested Chat Completions form forallowed_tools.String choices (
auto/none/required) and named single-function choices are unchanged, andllm/tools.gois untouched — the unified model already round-trips the subset for request persistence.Cross-route note: for an
allowed_toolsinput the inbound converter now also fills the unifiedllm.ToolChoice.ToolChoicemode, which subtly improves other routes' behavior rather than degrading it — Anthropic outbound now receives{"type":"any"}for moderequiredand{"type":"auto"}forauto(previously nothing was forwarded), Responses outbound now emits the full{type,mode,tools}object instead of a bare{"type":"allowed_tools"}, and Gemini maps modeautotoAUTOinstead of falling into the named branch. The tool subset remains dropped on Anthropic/Gemini — that is cases 2-6, out of scope here.Scope note
#2504 also reports dropped
strict,allowed_callers, andparallel_tool_callsacross Anthropic/Gemini routes (cases 2-6). Those are separate transformers with different trade-offs (e.g. whether to reject unmappable restrictions), so this PR stays on the malformed-request case to keep the change reviewable. The open #2220 Responses→Chat compatibility feature is complementary and does not cover this path (its plain Chat codec copies the named choice verbatim).Tests
TestToolChoice_AllowedToolsMarshalUnmarshalJSONandTestRoundTrip_AllowedToolsToolChoice(model_test.go): the issue's exact payload now round-trips through inbound → unified → outbound byte-faithfully (before:{"type":"allowed_tools","function":{"name":""}}).TestRequestFromLLM_AllowedToolsToolChoice/TestRequestFromLLM_NamedToolChoiceUnchanged(outbound_convert_test.go): nested form emitted; named choice keeps its existing shape.cd llm && go test ./...passes (one pre-existingwebsocket_executortest failure also fails on a clean checkout of the base on Windows);go vet ./...andgofmtclean on changed files.Summary by CodeRabbit
New Features
allowed_toolstool-choice option.Bug Fixes
allowed_toolschoices.