fix(tools): broad tool calling across every in-process engine - #879
fix(tools): broad tool calling across every in-process engine#879ttupper92618 wants to merge 29 commits into
Conversation
…age boundary
Llama 3.1+ writes a tool call as a bare JSON object with no opening marker and
ends the message with <|eom_id|>, so neither half of the marker mechanism that
drives MLX tool parsing applies. Two things went wrong as a result. The model
declares only <|eot_id|> as a stop token, so generation ran straight past the
end of its call and produced the next turn's header, putting control tokens in
the answer text. And with no marker to open on, the call was never recognized,
so a caller asking for a tool got JSON in the content and no tool call at all.
Add <|eom_id|> to the stop tokens whenever the vocabulary has it, keyed on the
vocabulary rather than on the chat template mentioning the token, because
Llama's template never writes it literally. Open the tool block on '{' for
those families and read the whole block with the cross-family text dialects,
which recognize both the bare object and the <|python_tag|> variant. A block
that opens on '{' can also be a model answering in JSON, so an unmarked block
that does not parse as a call is delivered as content rather than reported as
a parse failure.
Also let a marked block closed by the end of generation parse before falling
through to the error path: several families close a call by ending the message
rather than by emitting a closing marker.
Broaden the shared text parser to the dialects the other engines' models
speak: Llama <|python_tag|> calls, Mistral [TOOL_CALLS] arrays, GLM
<arg_key>/<arg_value> pairs, and, strictly, an unmarked call object that is
the entire message.
…s nobody offered
Two defects found running the tool surface against a Llama model on the MLX
engine.
Llama prefixes <|python_tag|> when it reaches for a tool by name, so a block
that opened only on '{' left that marker behind as content, and the caller
received a stray control token alongside the call. A parser can now name
further markers that also open a block.
Llama also answers some plain questions with <|python_tag|>print("hello"),
which parses as a call to 'print'. That is one of its built-ins, not something
the caller offered, and handing back a tool name the caller has no
implementation for is worse than showing them the text. A block whose calls
name no offered tool is now delivered as content. Nothing is filtered when the
request declares no tools, since there is no list to check against.
…path too The llama.cpp runner calls the shared text parser directly rather than through the MLX marker state machine, so it did not get the filter added for the MLX engine and would still hand a caller a call to a built-in they never offered. Filtering inside the shared parser covers both engines from one place, and a block left with no offered tool reads as prose there, which is the fallback that path already takes. Document the dialects, the offered-tools rule, and the message-boundary stop token in the architecture narrative, the fact sheet, and the API guide, and record the caller-visible behavior in the changelog and release notes.
A generation chunk is whatever the streaming detokenizer could resolve that step, not a token. An opening marker that is a single token id still reaches the parser in pieces, observed live from the MLX engine as <tool, _, c, all>, so testing each chunk on its own meant the block never opened for most models and the caller received the raw markup as content with a stop finish reason. A Qwen model emitting a perfectly well formed call looked like a model that refused to call anything. The opening decision is now made against the text accumulated from the start of the message, and the closing marker is matched against the accumulated block for the same reason. Only the leading chunks are held back, and only until the text either matches a marker or can no longer become one, so ordinary answers stream as before; a bound on that buffer keeps a whitespace-only stream from being held indefinitely. The decision resets when a block closes, so a message carrying more than one call still opens each of them.
tool_choice reached only the served engines, which forward it to a server that acts on it. The in-process engines render whatever tools they are handed and parse whatever the model writes, so the option did nothing there: a Llama model on the MLX engine returned the tool call on all four attempts of a request that sent "none" and asked for the tool by name. Apply the choice at the API boundary instead, the same place the logprobs request is resolved, so it means one thing on every engine. "none" removes the tools, which is the only way to guarantee that no call comes back; a model handed a tool and asked to use it will call it whatever the request said. The choice is dropped alongside them so a served engine is not handed a tool_choice with nothing to choose from. Naming a single function narrows the offered tools to that one, so the model cannot call a tool the caller did not ask for, while the choice still travels for server-side enforcement. A name matching no offered tool is left for the engine to report rather than silently becoming a no-tools request. "required" passes through untouched and remains best-effort in-process, since forcing a call would need constrained decoding.
![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: 7dacb2cd07
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR fixes and standardizes tool-calling behavior across Skulk’s in-process engines (MLX + llama.cpp) by hardening the stream-time marker parser, adding cross-family tool-call dialect parsing (including Llama’s unmarked / <|python_tag|> forms), and applying tool_choice consistently at the API boundary so it behaves the same regardless of engine.
Changes:
- Make tool-call parsing robust to markers split across streamed chunks and to dialect differences (Llama
<|eom_id|>boundary, Mistral[TOOL_CALLS], GLM<arg_key>/<arg_value>, strict unmarked JSON-call messages). - Resolve
tool_choicebefore dispatch so"none"and forced-function behavior are consistent on in-process and served engines. - Update docs, changelog/release notes, tests, and model cards to reflect restored tool-calling truth and the clarified caller-visible rules.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/release-notes/next.md | Adds public “next” release note entry describing the new tool-calling behavior and guarantees. |
| website/docs/architecture.md | Documents tool-call dialect differences and the `< |
| website/docs/architecture-reference.md | Adds a reference section enumerating supported in-process tool-call dialects and the marker state machine properties. |
| website/docs/api-guide.md | Documents caller-visible semantics: offered-tools-only, tool_choice normalization, and JSON-vs-tool-call ambiguity rules. |
| src/skulk/worker/tests/unittests/test_runner/test_parse_tool_calls.py | Adds a unit test ensuring calls naming only non-offered tools fall back to content. |
| src/skulk/worker/runner/llm_inference/tool_text_parser.py | Expands text-based tool-call parsing to additional dialects and adds offered-tool filtering. |
| src/skulk/worker/runner/llm_inference/tool_parsers.py | Introduces unmarked-dialect sentinel + whole-block dialect parser builder + offered-tool filter helper. |
| src/skulk/worker/runner/llm_inference/tests/test_unmarked_tool_dialect.py | New coverage for the unmarked Llama dialect and “unparsed block is content” behavior. |
| src/skulk/worker/runner/llm_inference/tests/test_tool_text_parser_dialects.py | New unit tests pin one example per supported dialect plus false-positive guards. |
| src/skulk/worker/runner/llm_inference/tests/test_split_tool_markers.py | New tests for tool markers split across chunks and for streaming not being overly buffered. |
| src/skulk/worker/runner/llm_inference/runner.py | Selects the whole-block dialect parser when tokenizer uses the unmarked sentinel. |
| src/skulk/worker/runner/llm_inference/model_output_parsers.py | Implements chunk-splitting-safe open/close detection, end-of-generation closure handling, and offered-tool filtering. |
| src/skulk/worker/engines/mlx/utils_mlx.py | Adds `< |
| src/skulk/api/tests/test_tool_choice_resolution.py | New tests for tool_choice resolution behavior before dispatch. |
| src/skulk/api/adapters/chat_completions.py | Adds resolve_tool_choice() and wires it into request→task conversion. |
| resources/inference_model_cards/Qwen--Qwen3.6-35B-A3B-FP8.toml | Restores model-truth tool calling support declaration. |
| resources/inference_model_cards/Qwen--Qwen3.6-27B-FP8.toml | Restores model-truth tool calling support declaration. |
| resources/inference_model_cards/mlx-community--Qwen3.6-35B-A3B-nvfp4.toml | Adds [tooling] section declaring tool-calling support for the quantized sibling card. |
| resources/inference_model_cards/mlx-community--Qwen3.6-27B-4bit.toml | Adds [tooling] section declaring tool-calling support for the quantized sibling card. |
| resources/inference_model_cards/google--gemma-4-31B-it-qat-w4a16-ct.toml | Restores model-truth tool calling support declaration for Gemma 4 vLLM cards. |
| resources/inference_model_cards/google--gemma-4-26B-A4B-it.toml | Restores model-truth tool calling support declaration for Gemma 4 vLLM cards. |
| resources/inference_model_cards/google--gemma-4-12B-it-qat-w4a16-ct.toml | Restores model-truth tool calling support declaration for Gemma 4 vLLM cards. |
| CHANGELOG.md | Records behavior changes in the unreleased changelog. |
Suppressed comments (2)
CHANGELOG.md:38
- Another repeated
### Fixedheading under[Unreleased]. Keeping a single### Fixedsection and continuing the bullet list avoids duplicate headers and makes the changelog easier to scan.
### Fixed
CHANGELOG.md:64
- This introduces yet another
### Fixedheader within the same[Unreleased]section. Consider keeping one### Fixedheader and appending additional bullets beneath it.
### Fixed
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review found two holes. The tool parser is wired from the tokenizer, which cannot see what the request asked for, so a request carrying no tools was still parsed for calls. A model that spontaneously writes something call-shaped, which is exactly what a request asking for JSON output invites, then returned tool_calls to a caller who offered none. It also meant tool_choice "none" did not fully hold, since resolving that choice works by removing the tools from the request. Both in-process engines now skip tool parsing when the request declared no tools; on the MLX path that also stops the parser holding back leading chunks for a request that can never produce a call. The filtering helper keeps its own semantics: an absent tools list means the caller had no list to check against, not that nothing may be called. The steward parses its own turns through the same dialects without passing one, so deciding that in the helper would have silently disabled the steward's tools. Revert the five vLLM-only card flips. Those models do support tool calling, but the vLLM runner resolves a parser only from an explicit runtime pin and rejects every tools request without one, so advertising the capability without pinning a parser would make it error at request time. Pinning one is not something this change can validate: the documented rule is to pin only names proven on hardware, and no GPU node was available here. Card truth for the signed registry is tracked separately in any case, since bundled cards only load when the registry load fails.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/skulk/worker/runner/llm_inference/model_output_parsers.py:762
- The end-marker detection joins the entire accumulated tool-call text on every streamed chunk (
"".join(tool_call_text_parts)), which makes parsing a long tool-call block potentially O(n^2) in total bytes processed. You only need a tail window to detect whether the closing marker has arrived; join the full block only once when the marker is actually detected.
if not just_opened:
tool_call_text_parts.append(response.text)
# The closing marker splits across chunks for the same reason the
# opening one does, so match it against the accumulated block.
if "".join(tool_call_text_parts).rstrip().endswith(tool_parser.end_parsing):
# parse the actual tool calls from the tool call text
combined = "".join(tool_call_text_parts)
parsed = tool_parser.parse(combined.strip(), tools=tools)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 226ae123b9
ℹ️ 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".
…tually does The API guide claimed the engine reports an unmatched forced tool name. Only the served engines do; the in-process engines never consume tool_choice, so they answer from the full list instead. Describe the real behavior rather than the intended one, and record rejecting it at the boundary as the follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c668fd025
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/skulk/worker/runner/llm_inference/model_output_parsers.py:760
- The closing-marker check currently does
"".join(tool_call_text_parts)on every streamed chunk. For long tool-call blocks this turns into O(n^2) work (re-joining the entire accumulated text each step) even though we only need to know whether the tail ends with the end marker.
You can avoid the repeated full join by only joining a bounded slice of the most recent parts sized to the end marker length (plus a small cushion for chunk-splitting).
# The closing marker splits across chunks for the same reason the
# opening one does, so match it against the accumulated block.
if "".join(tool_call_text_parts).rstrip().endswith(tool_parser.end_parsing):
# parse the actual tool calls from the tool call text
Tool parsing runs downstream of the thinking parser, so a thinking model that reasons before calling a tool sends its reasoning through this parser first. Accumulating that text into the opening decision made the message look like an ordinary answer, permanently, so the marker that followed was never examined and the caller received raw tool markup as content. That is the exact failure this change set exists to remove, reintroduced for the family most likely to call a tool. Reasoning chunks now pass straight through without taking part in the decision. That also stops a call the model only contemplated inside its reasoning from being executed, which the llama.cpp engine already avoided deliberately and the MLX path did not.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/skulk/worker/runner/llm_inference/model_output_parsers.py:772
parse_tool_calls()currently does"".join(tool_call_text_parts)on every streamed chunk to detect a split closing marker. For long tool-call payloads this turns the close check into O(n²) work over the tool-call length, even though you only need to inspect a small suffix window until you actually see the end marker.
# The closing marker splits across chunks for the same reason the
# opening one does, so match it against the accumulated block.
if "".join(tool_call_text_parts).rstrip().endswith(tool_parser.end_parsing):
# parse the actual tool calls from the tool call text
combined = "".join(tool_call_text_parts)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c1f5f7b04
ℹ️ 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".
…eased
Review found that settling the opening decision once was too coarse in the
other direction: a model that writes a sentence before calling ("I'll check
that." and then the call) settled the decision on that sentence, so every
later marker went unexamined and the call came back as raw markup. Models
announce what they are about to do routinely, so this is the common shape
rather than an edge case.
Replace the settle with a rolling scan. Only the trailing run of text that
could still become a marker is carried across chunk boundaries, which is
shorter than the longest marker, so ordinary answers stream with at most a few
characters of latency, nothing is held for a message that turns out to contain
no call, and the scan never stops looking. This also removes the arbitrary
buffer budget the settle needed.
Distinctive markers may open a block anywhere. The unmarked dialect's marker is
a brace, which appears in ordinary prose and in JSON answers, so it opens one
only at the start of a message; the families writing that dialect put the call
in the whole message, so anchoring it loses nothing, and their distinctive
marker still opens a call after a preamble.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/skulk/api/adapters/chat_completions.py:138
resolve_tool_choicereturns early onnot tools, so a request withtools=[]andtool_choice="none"never reaches the branch that drops both fields. That leavestool_choicepopulated even though there are no tools, which can get forwarded to served engines (and contradicts the comment about avoidingtool_choicewith nothing to choose from).
if tool_choice is None or not tools:
return tools, tool_choice
src/skulk/worker/runner/llm_inference/model_output_parsers.py:820
- Inside the tool-call block, the code joins
tool_call_text_partstwice per chunk (once for theendswithcheck and again to buildcombined). Hoisting the join avoids redundant work while keeping the same behavior.
if "".join(tool_call_text_parts).rstrip().endswith(tool_parser.end_parsing):
# parse the actual tool calls from the tool call text
combined = "".join(tool_call_text_parts)
parsed = tool_parser.parse(combined.strip(), tools=tools)
There was a problem hiding this comment.
💡 Codex Review
Skulk/src/skulk/worker/runner/llm_inference/model_output_parsers.py
Lines 1007 to 1009 in 52de05b
Preserve earlier calls when a closed block fails parsing
When a valid block has already populated accumulated_calls and a later, fully closed block is malformed, this emits an error finish reason and breaks; the calls yielded by the post-loop fallback then occur after the terminal chunk, so API._token_chunk_stream never delivers them. Fresh evidence beyond the earlier unclosed/truncated-block thread is that this complete-closing-marker path also makes behavior depend on chunk boundaries: _scan_remaining_blocks preserves valid surrounding calls when the same suffix is in the first terminal chunk, while splitting before the malformed block loses them. Route this exit through the same accumulated-call terminal policy.
ℹ️ 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".
Found by running the tool surface against gpt-oss on the MLX engine, which is Skulk's own harmony parser rather than a server's. A request sending tool_choice "none", which resolves by removing the tools, came back with a call on every attempt, and the call name carried the model's own namespace prefix so a caller matching on the offered name would not have matched it. Those two families decode their calls from the token stream and are selected ahead of the marker path, so the filter the marker path applies never saw them. Their output now passes through the same rule, and a rejected call is re-serialized as content, which is what every other path here does, so the caller sees what the model did instead of a blank answer. I had scored this exact gap a 3 when review raised it earlier in this branch, on the reasoning that these dialects name a function from a namespace that only appears when tools are rendered. That reasoning was wrong: it reproduced on the first live attempt with no tools rendered at all.
There was a problem hiding this comment.
💡 Codex Review
Skulk/src/skulk/worker/runner/llm_inference/model_output_parsers.py
Lines 1058 to 1061 in 1010951
Preserve accepted calls when a later closed block is malformed
When an earlier block has populated accumulated_calls and a subsequent marker-delimited block reaches its closing marker but fails parsing, this emits a terminal error and breaks before the accumulated valid calls are returned; the API stops on that finish reason, so the caller loses calls it could have executed. This differs from the fixed EOF/truncation path and is also chunk-dependent: when the malformed suffix is already in held_text, _scan_remaining_blocks preserves the earlier calls. Route this exit through the same message-finishing logic while keeping the malformed block nonterminal.
ℹ️ 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".
A rejected call was re-serialized with a finish reason of its own, so a stream that carries a terminal chunk after the call, which these two families usually do, ended at the consumer before that chunk arrived. The text now rides the next chunk instead, and is released with a finish reason only if the stream ends without one, so there is always exactly one terminal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37191a5de3
ℹ️ 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".
Two small things in the rejection fallback. Several rejected calls in a row ran together into text a caller could not read back, and the fabricated fallback response threw away the usage and stats the rejected response carried, so a caller was told the message cost nothing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
CHANGELOG.md:91
- This changelog entry describes the unmarked tool-call dialect as only working when the call object is “the entire message”, but the implementation/tests accept a leading call object with trailing text (e.g.
{"name": ...}Done). Updating this wording will keep the release notes consistent with the actual parsing rules.
`<|python_tag|>` calls, Mistral `[TOOL_CALLS]` arrays, GLM
`<arg_key>`/`<arg_value>` pairs, and an unmarked call object that is the
entire message, alongside the harmony channels and `<tool_call>` blocks
already supported.
Whether a model can call tools, and which dialect it writes, are properties of the model, so two cards for the same base model cannot both be right when they disagree. The contradiction is not academic: it decides what /v1/models advertises and whether a served engine rejects a request carrying tools, so a client picking a quantization can be told the same model does and does not support tools depending which one it picked. A survey of the bundled cards found six base models whose cards disagree. Two are flat contradictions, an explicit true beside an explicit false, and both are the vLLM-only cards that under-declare: those models do call tools, but the vLLM runner resolves a parser only from an explicit pin and rejects a tools request without one, so flipping the flag alone would advertise a capability that fails at request time, and pinning a parser has to be validated on GPU hardware first. Those two are listed as known debt so the guard catches anything new, and a third test fails if a listed entry is fixed, so the list shrinks rather than quietly widening what is allowed. The remaining disagreements are an unstated value beside a stated one, which is under-specification rather than contradiction, and are not flagged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/skulk/api/adapters/chat_completions.py:131
resolve_tool_choicetreats an emptytoolslist as “no tools” due tonot tools, sotool_choice="none"won’t be applied and will be forwarded along withtools=[]. That contradicts the function’s own intent (“none removes the tools entirely”) and can also hand served engines atool_choicewith nothing to choose from.
Use a tools is None check instead so an empty list still flows through the "none" branch and gets normalized to (None, None).
if tool_choice is None or not tools:
return tools, tool_choice
src/skulk/worker/runner/llm_inference/model_output_parsers.py:722
- In
reject_unoffered_tool_calls, if a rejected tool call has already populatedpending_textand a subsequentToolCallResponseis kept (i.e., names an offered tool),pending_textis never cleared. That can cause the rejected-call text to be emitted later (or synthesized at generator end), potentially creating an extra terminal chunk after a valid tool call or leaking rejected-call text into an otherwise-valid tool-call response.
Clear any pending rejected-call text when a kept tool call is emitted.
if isinstance(response, ToolCallResponse):
kept = declared_tool_calls(response.tool_calls, tools) if tools else []
if kept:
yield response.model_copy(update={"tool_calls": kept})
continue
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 251c727785
ℹ️ 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".
Found by the harness's tool-contract suite rather than by hand, which is the point of having it: a named tool_choice narrowed the offered tools, a Llama model called the tool that had been narrowed away, the call was correctly rejected, and the caller received <|python_tag|> in the answer text. A rejected or unparsed block is delivered as content so the caller can see what the model did. That is only useful if it is readable, so the dialect's markers are stripped. A response already flagged as an error keeps the raw block, since there the markup is the evidence of what was malformed rather than an answer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80bf27c8bd
ℹ️ 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".
| else: | ||
| # Not a call the caller can run, so it is text like any other. | ||
| leftover.append(remaining[:end_of_block]) |
There was a problem hiding this comment.
When a terminal message contains an accepted tool block followed by a block naming an unoffered tool, _finish_message sends the suffix through this path, which appends the rejected block verbatim. Unlike the direct rejection path, it does not call _block_as_content, so control markup such as <tool_call>...</tool_call> is exposed in assistant content before the accepted ToolCallResponse. Apply the same marker stripping when preserving rejected suffix blocks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/skulk/worker/runner/llm_inference/tool_text_parser.py:156
- In
_call_from_json_object, theargsstring branch treats any string containing a JSON object prefix as valid (_first_json_object(args) is not None) and then passes the original string through. If the string has trailing characters after the object (e.g."{...}Done"), downstreamjson.loads(tool_call.arguments)will fail and the call will be emitted with malformedarguments. Instead, re-serialize the parsed object (or require that the entire string decodes to an object).
args = payload.get("arguments", payload.get("parameters", {}))
if isinstance(args, dict):
args_str = json.dumps(args)
elif isinstance(args, str) and _first_json_object(args) is not None:
args_str = args
else:
args_str = "{}"
…ools Found by the harness's tool-contract and tool-recovery suites on Gemma 4: a request sending tool_choice "none" came back with <|tool_call> and <tool_call|> in the answer text. This was self-inflicted. Skipping the scan when no tools are offered is what makes "none" hold, but it also meant nothing recognized a block the model wrote anyway, so the markers were delivered verbatim. Separating the two decisions fixes it: the block is always recognized, and emit_calls carries whether the request permits it to become a call. A rejected block goes down the content path, which strips the markers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97df3000bf
ℹ️ 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".
| in_tool_call = True | ||
| just_opened = True | ||
| held_text = "" | ||
| at_message_start = False | ||
| tool_call_text_parts.append(scanned[start:]) |
There was a problem hiding this comment.
When an MLX Llama response begins with { but is ordinary JSON rather than a tool call, this opens an anchored unmarked block and holds every subsequent chunk while waiting for <|eom_id|>. Because that token is configured as the generation stop and is not delivered in response text, the parser cannot classify the block as content until the terminal chunk, so streamed JSON responses lose all incremental output and time-to-first-token can grow to the full generation time. Delay opening until a call-shaped name plus arguments/parameters signature is distinguishable, or otherwise release non-call JSON incrementally; the current behavior also contradicts the updated architecture fact sheet's bounded-latency claim.
AGENTS.md reference: AGENTS.md:L262-L262
Useful? React with 👍 / 👎.
| if held_text: | ||
| more_calls, leftover = _scan_remaining_blocks( | ||
| held_text, tool_parser, tools | ||
| ) |
There was a problem hiding this comment.
When a request offers no tools (including tool_choice: "none") and one terminal generation chunk contains two complete marked blocks, the first block is correctly converted to content via emit_calls=False, but _finish_message passes the suffix here without that flag. _scan_remaining_blocks then calls declared_tool_calls(..., tools=None), whose deliberate shared-helper semantics retain every call, so the second block is emitted as a terminal ToolCallResponse despite the caller explicitly prohibiting calls. Pass the emission policy into the suffix scan so all blocks in the same message follow the same no-call rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/skulk/worker/runner/llm_inference/model_output_parsers.py:1045
parse_tool_calls()currently doesblock_so_far = "".join(tool_call_text_parts)on every streamed chunk while inside a tool-call block, solely tofind()the closing marker. For long tool-call payloads (large JSON args) streamed in many chunks, this becomes quadratic (re-allocating and scanning the entire accumulated block each step) and can regress streaming latency under load.
Consider tracking a rolling suffix (length len(end_parsing)-1) to detect when the end marker could have appeared, and only join() the full tool_call_text_parts once—when the end marker is detected or when generation ends.
# message is still found.
block_so_far = "".join(tool_call_text_parts)
end_index = block_so_far.find(tool_parser.end_parsing)
| half of the marker mechanism applies. These tests pin the two behaviors that | ||
| makes possible: a call that opens on ``{`` and is closed by the end of | ||
| generation is parsed, and a block that turns out not to be a call is delivered | ||
| as content instead of being reported as a failure. |
What this fixes
Tool calling was broken or unreliable on the in-process engines, and the
failures were quiet: a model that emitted a perfectly well formed call came
back to the caller as a
stopresponse with the raw markup incontentand notool_callsat all. Four separate defects, found by running the tool surfaceagainst live models rather than by reading code.
1. Tool-call markers split across streamed chunks were never matched. A
generation chunk is whatever the streaming detokenizer could resolve that step,
not a token, so an opening marker that is a single token id still arrives in
pieces. Observed live from the MLX engine as
<tool,_,c,all>. Theparser tested each chunk on its own, so for most models the block never opened.
The opening decision is now made against the text accumulated from the start of
the message, and the closing marker is matched the same way. Only the leading
chunks are held back, and only until the text either matches a marker or can no
longer become one, so ordinary answers stream as before.
2. Llama models could not call a tool at all on the MLX engine. Llama ends
a message that hands off to a tool with
<|eom_id|>but declares only<|eot_id|>as a stop token, so generation ran past the end of its own call andwrote the next turn's header into the answer. It also writes the call as a bare
JSON object with no opening marker, which nothing recognized. Skulk now stops at
the message boundary for any model whose vocabulary carries that token, keyed on
the vocabulary rather than on the chat template mentioning it (Llama's template
never writes it literally, it routes tool results through the
ipythonrole),and reads the whole block with a set of cross-family dialects.
3. A model's own built-ins were surfaced as tool calls. Llama answers some
plain questions with
<|python_tag|>print("hello"), and gpt-oss haspythonand
browser. A caller has no implementation for those names, so a block whosecalls name no offered tool is now delivered as content. Nothing is filtered when
the request declares no tools.
4.
tool_choicedid nothing on the in-process engines. Only the servedengines forward it to a server that acts on it. A request sending
"none"andasking for the tool by name returned the tool call on all four attempts. The
choice is now applied at the API boundary, the same place the logprobs request
is resolved:
"none"removes the tools, and naming a single function narrowsthe offered tools to that one.
"required"passes through and remainsbest-effort in-process, since forcing a call would need constrained decoding.
The shared text parser also gained the dialects other families write: Llama
<|python_tag|>calls, Mistral[TOOL_CALLS]arrays, GLM<arg_key>/<arg_value>pairs, and, strictly, an unmarked call object that isthe entire message.
Seven model cards are restored to model truth: five vLLM cards that declared no
tool support for models that support it, and two that carried no
[tooling]section at all. Card truth for the signed registry is tracked separately, since
bundled cards only load when the registry load fails.
Validation
Live, on a two-node cluster running this branch, across every engine that can
serve a text model today. Each engine ran the same matrix: a basic call and its
arguments, no control tokens leaking into content, the tool-result round trip,
picking the right tool of two,
tool_choiceofnone/ a named function /required, the streaming call, and the streaming chunk discriminator.mlx(unmarked dialect)mlx(marker dialect)llama_serverllama_cpp(in-process)Every behavior change carries a test that was verified to fail when the fix is
reverted. Full battery green:
basedpyright0 errors,ruff checkclean, 3725tests pass.
Known follow-up, not in this PR
enable_thinkingis ignored by the in-processllama_cppengine. Thellama_serverrunner maps it intochat_template_kwargs; the in-process runnerhas no reference to it, and
llama_cpp_python'screate_chat_completionhas nokeyword passthrough to reach the template. With tools re-offered, a thinking
model then reasons until it hits
max_tokensand returns an empty answer.Fixing it means changing how that engine renders prompts, which is a different
change from this one.
Docs
website/docs/api-guide.mddocuments the caller-visible rules (only offeredtools come back, a JSON answer stays an answer, and what each
tool_choicevalue guarantees).
website/docs/architecture.mdandarchitecture-reference.mdcover the dialects, the marker mechanism's newproperties, and the message-boundary stop token.
CHANGELOG.mdandwebsite/docs/release-notes/next.mdrecord the behavior changes.