Forward declared tools to the DiffusionGemma visual server - #864
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for forwarding tools from the chat request payload to the visual server. The feedback suggests changing the truthiness check if tools: to if tools is not None: to prevent empty tool lists (e.g., []) from being silently dropped.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@codex review |
1 similar comment
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8f3c31838
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async def chat(req: Request): | ||
| body = await req.json() | ||
| messages = body.get("messages", []) | ||
| tools = body.get("tools") # forwarded to the visual server (rendered once the binary reads it) |
There was a problem hiding this comment.
When callers include a tool registry but set tool_choice: "none" (or force a single function), the shim now still forwards the entire registry into the visual server. Once the server renders inputs.tools, those turns advertise tools to the model despite the client disabling or constraining tool selection, so an OpenAI-compatible client can receive a tool call it explicitly disallowed or a call to the wrong tool. Please apply tool_choice to tools here before passing it to generate_visual.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 12d3153: _tools_for_choice narrows the registry before forwarding, so tool_choice "none" drops all tools and a forced function is filtered to just that tool.
| with _LOCK: | ||
| return V.generate_visual(srv, messages, seed=seed, max_blocks=max_blocks, | ||
| on_stats=stats_box.update) | ||
| on_stats=stats_box.update, tools=tools) |
There was a problem hiding this comment.
For direct /v1/chat/completions callers that send tools, this forwarding can make the visual server generate Gemma's native <|tool_call>... text, but the shim still returns the generated text only as assistant content with finish_reason: "stop" (and streams it as content deltas on the streaming path). OpenAI-compatible clients hitting this endpoint directly will not see message.tool_calls / finish_reason: "tool_calls", so they cannot execute the requested tool; please parse the native tool-call text here before returning or avoid enabling tool rendering in this shim until that response healing is local.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
By design: this shim is an internal loopback bridge that Studio spawns, and the Studio backend heals the native tool-call text into structured tool_calls (unslothai/unsloth#6851). On the supported Studio path the call is surfaced as tool_calls; a direct client on the loopback port is not a supported entry point, and emitting tool_calls in the shim would double-process against that healing.
|
Pushed a follow-up so the shim honors Previously the full On the empty-list note above: an empty |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d31539d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if tools: | ||
| req["tools"] = tools |
There was a problem hiding this comment.
When a request uses tool_choice: "required" or forces a specific function, the shim may narrow the registry, but the request file built here still only contains tools and drops the choice itself before the visual server renders the chat template. In those scenarios the server sees optional tools, so an OpenAI-compatible caller that explicitly required a tool call can still receive ordinary assistant content. Please forward a required/forced choice alongside the filtered registry, or reject unsupported choices, instead of only writing tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The diffusion visual server renders only the prompt and samples with no grammar (data.grammar is never applied) and never reads tool_choice, so required/forced enforcement is not achievable on this path regardless of forwarding. _tools_for_choice still narrows to the forced function for advertising, so forwarding the choice would be a no-op here.
| body = await req.json() | ||
| messages = body.get("messages", []) | ||
| # forwarded to the visual server (rendered once the binary reads it) | ||
| tools = _tools_for_choice(body.get("tools"), body.get("tool_choice")) |
There was a problem hiding this comment.
When an OpenAI-compatible client sends a valid function tool that only includes the required name and omits description or parameters, this path forwards the raw tool to the visual server, whose common_chat_tools_parse_oaicompat reads those fields unconditionally. Those requests now fail with ERR parse (or stream an engine error) instead of completing; please add defaults for omitted optional fields before forwarding or reject them with a clean 400 response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
common_chat_tools_parse_oaicompat uses function.value("description", "") and function.value("parameters", {}) at chat.cpp:551-555; only name uses .at(). A name-only tool parses without error, so there is no ERR parse to guard against.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6334addc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| req["tools"] = tools | ||
| with open(self.req, "w") as f: | ||
| json.dump({"seed": int(seed), "n_blocks": int(n_blocks), "messages": messages}, f, | ||
| ensure_ascii=False) | ||
| json.dump(req, f, ensure_ascii=False) |
There was a problem hiding this comment.
On Windows or any non-UTF-8 locale, this new forwarding path serializes tool schemas through json.dump(..., ensure_ascii=False) into a file opened with the platform default encoding. An otherwise ASCII chat can now fail before reaching the visual server when a tool description contains characters such as °C or emoji, because those tool strings were not written at all before this change; open the request file with UTF-8 or escape non-ASCII when dumping the request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 741b83f: the request file is now opened with encoding="utf-8", so non-ASCII tool descriptions and message content no longer depend on the platform default codec (this also covered the pre-existing message path). Confirmed the same content raises UnicodeEncodeError under cp1252.
| return [t for t in tools or [] | ||
| if isinstance(t, dict) and (t.get("function") or {}).get("name") == name] or None | ||
| return tools |
There was a problem hiding this comment.
When a client passes a forced tool_choice for one function, this only narrows the advertised schemas and then discards the actual choice before calling the visual server. The downstream request is therefore indistinguishable from auto with one tool, so the model can still return plain text instead of the required function call; OpenAI-compatible clients that force a tool rely on that call being enforced or rejected, not treated as optional.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the earlier forced-tool_choice item: the diffusion visual server samples with no grammar (data.grammar is never applied) and never reads tool_choice, so required/forced enforcement is not achievable on this path regardless of forwarding. _tools_for_choice still narrows to the forced function for advertising.
| with _LOCK: | ||
| return V.generate_visual(srv, messages, seed=seed, max_blocks=max_blocks, | ||
| on_stats=stats_box.update) | ||
| on_stats=stats_box.update, tools=tools) |
There was a problem hiding this comment.
When tools are supplied, this new argument lets the visual server render schemas and the model can respond with a tool-call marker, but chat() still funnels the returned text through _split_thought_channels and emits it only as message.content/content deltas with finish_reason: "stop" in the non-streaming and streaming branches. Non-Studio OpenAI clients will never see message.tool_calls/finish_reason: "tool_calls", so they won't execute the function even though the request advertised tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the earlier item: this shim is an internal loopback bridge Studio spawns, and the Studio backend heals the native tool-call text into structured tool_calls (unslothai/unsloth#6851). A direct client on the loopback port is not a supported entry point, and emitting tool_calls here would double-process against that healing.
|
@codex review |
1 similar comment
|
@codex review |
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741b83f186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body = await req.json() | ||
| messages = body.get("messages", []) | ||
| # forwarded to the visual server, honoring tool_choice | ||
| tools = _tools_for_choice(body.get("tools"), body.get("tool_choice")) |
There was a problem hiding this comment.
When an OpenAI-compatible request includes tools (especially with a forced tool_choice), this now advertises those schemas to the visual server, but the shim still returns the committed generation only as plain content and always finishes with "stop"; there is no conversion of generated native tool-call text into message.tool_calls or streamed delta.tool_calls. Tool-aware clients will therefore not execute the selected tool and may display the raw call instead, so the shim needs to parse and emit the Chat Completions tool-call fields before forwarding tool schemas.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the earlier items on this file: this shim is an internal loopback bridge that Studio spawns, not a public entry point. The Studio backend parses the native tool-call text into structured tool_calls (unslothai/unsloth#6851), so emitting them here as well would double-process the same generation. Forwarding the schemas is what makes the visual server render them into the chat template; the conversion to Chat Completions tool-call fields happens one layer up, not in the shim.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741b83f186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body = await req.json() | ||
| messages = body.get("messages", []) | ||
| # forwarded to the visual server, honoring tool_choice | ||
| tools = _tools_for_choice(body.get("tools"), body.get("tool_choice")) |
There was a problem hiding this comment.
When a client sends a forced tool choice (for example {"type":"function","function":{"name":"foo"}}) or tool_choice: "required", this line collapses the request to just a filtered tools list; _send then serializes only tools, so the child cannot distinguish an optional single tool from a required/forced call. Agent clients that rely on forced tool use can receive a normal assistant message instead of a tool call even though they explicitly required one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as the earlier forced-tool_choice item: the diffusion visual server samples with no grammar and never reads tool_choice, so required/forced enforcement is not achievable on this path even if the field were forwarded. _tools_for_choice already narrows to the forced function so only that one schema is advertised, which is the most the server can act on here.
3 participants
The DiffusionGemma shim drops the caller's
tools, so the model never sees the tool definitions and can't produce schema-correct tool calls.Part of unslothai/unsloth#6732.
Problem
The shim rebuilds a minimal request for the visual server,
{seed, n_blocks, messages}, and never readstoolsfrom the body. So even when a client declares tools, the chat template renders none of them, and the model guesses argument names (or declines) instead of following the schema.Fix
Thread
toolsfrom the request body throughgenerate_visualinto the request the visual server reads. Only added when present, so plain chat is unchanged.Note
One link in a three-part fix: Studio heals the model's text tool call into a structured
tool_callsarray (unslothai/unsloth#6851), and the visual server still has to readtoolsintoinputs.tools(llama.cpp#24423) to actually render them. So this is a no-op until that binary support lands, and plain chat and thinking are unaffected either way.