Skip to content

fix(middleware): parse OpenAI-spec tool_choice in /v1/chat/completions#9559

Open
Anai-Guo wants to merge 1 commit intomudler:masterfrom
Anai-Guo:fix-toolchoice-chatcompletions
Open

fix(middleware): parse OpenAI-spec tool_choice in /v1/chat/completions#9559
Anai-Guo wants to merge 1 commit intomudler:masterfrom
Anai-Guo:fix-toolchoice-chatcompletions

Conversation

@Anai-Guo
Copy link
Copy Markdown
Contributor

Summary

Closes the fourth-site clause in #9508. This is the natural follow-up to #9526 (merged): #9526 fixed the wrong-setter pattern at three endpoints, and walcz-de's #9509 (merged) fixed the parallel /v1/responses parsing. The remaining piece — tool_choice parsing in /v1/chat/completions (SetModelAndConfig middleware) — silently mishandled both the string-mode and the OpenAI-spec map shape.

The bug

core/http/middleware/request.go:322-335 (before this PR) unmarshalled every tool_choice shape through json.Unmarshal(..., &functions.Tool{}):

switch content := input.ToolsChoice.(type) {
case string:
    _ = json.Unmarshal([]byte(content), &toolChoice)
case map[string]any:
    dat, _ := json.Marshal(content)
    _ = json.Unmarshal(dat, &toolChoice)
}
input.FunctionCall = map[string]any{
    "name": toolChoice.Function.Name,
}

Two failure modes, both silenced via _ =:

  1. String mode (tool_choice: "required" | "auto" | "none") — json.Unmarshal([]byte("required"), &functions.Tool{}) fails. toolChoice.Function.Name == "". The downstream dispatch (lines 487+ in this same file) then runs SetFunctionCallNameString(""), so the requested mode never engages.
  2. OpenAI-spec map shape ({"type":"function", "function":{"name":"..."}}) — the JSON keys do not match functions.Tool's field tags (Tool nests function differently), so name again ends up empty.

The fix

Mirrors the parsing pattern walcz-de established in #9509 for MergeOpenResponsesConfig, dispatching by the actual incoming shape rather than reinterpreting through the wrong struct:

  • String case: if tool_choice is "required" / "none", hand it to input.FunctionCall as a string. The existing string-case branch downstream (line 487) routes that through SetFunctionCallString, which writes the mode field. "auto" is the default and skipped. Empty string is skipped.
  • Map case: accept both the OpenAI-spec nested shape ({"type":"function", "function":{"name":"..."}}) and the legacy/Anthropic-compat flat shape ({"type":"function", "name":"..."}). Nested is preferred; flat is the fallback. Set input.FunctionCall = {"name": <name>} only when a non-empty name was actually found, so the downstream map-case branch writes the name via SetFunctionCallNameString.

This way the existing dispatch on input.FunctionCall (lines 487-502) keeps doing what it already does correctly — the bug was strictly in the upstream parsing that fed it bad data.

The pkg/functions import drops with this change (it was only used for the now-removed var toolChoice functions.Tool).

Net effect (matches what #9509 + #9526 already restored elsewhere)

Request Before After
tool_choice: "required" SetFunctionCallNameString("") (silent no-op) SetFunctionCallString("required") engages mode
tool_choice: {type:"function", function:{name:"X"}} SetFunctionCallNameString("") (silent no-op) SetFunctionCallNameString("X") engages grammar forcing
tool_choice: {type:"function", name:"X"} (legacy) already worked unchanged
tool_choice: "auto" / "none" reached downstream as "" (no-op) skipped at parse time (no-op) — same observable behavior

Why no new tests in this PR

The parsing logic here is intentionally identical (modulo the string-vs-map dispatch difference dictated by the surrounding code) to MergeOpenResponsesConfig in #9509, which already has nine Ginkgo specs (MergeOpenResponsesConfig tool_choice parsing in request_test.go) covering all four shapes plus malformed inputs. Adding a parallel set for SetModelAndConfig would require a heavier scaffold (the existing tests there go through Echo + a real model dir) for coverage that's structurally redundant with the merged tests. Happy to add a unit-level test scaffold for SetModelAndConfig's parsing if a maintainer prefers — let me know.

Test plan

  • go build ./...
  • go test ./core/http/middleware/ -count=1 -run TestMiddleware
  • Send tool_choice: "required" to /v1/chat/completions with a tools array → grammar forcing engages, output arrives as tool_calls not free-text.
  • Send tool_choice: {"type":"function", "function":{"name":"my_tool"}}my_tool is forced.
  • Send tool_choice: {"type":"function", "name":"my_tool"} (legacy) → still works.

🤖 Generated with Claude Code

Follows up on mudler#9526 (the 3-site setter fix) by addressing the remaining
clause in mudler#9508 — string mode and OpenAI-spec specific-function shape both
silently failed in the /v1/chat/completions parsing path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants