Repository navigation
fix: repair qwen3_coder, Mistral and pythonic tool-call parser gaps - #2487
Merged
Merged
Conversation
qwen3_coder took the function and parameter name as everything up to the next '>', so a dropped '>' swallowed the following tag into the name. Names now stop at the first whitespace, '<' or '>' (upstream mlx-lm rule) and an unmatchable parameter name is skipped.
Mistral bracket parsing now loops over `NAME[ARGS]` headers (hyphenated names allowed) and reads one JSON value per header, returning the calls collected before a truncated one. ParsedToolCall gains `id: Option<String>`, set from the Mistral Nemo list element and preferred over a minted id in the Chat (streaming and non-streaming) and Responses (streaming and non-streaming) builders.
Pythonic parsing replaces the lazy regex with a scanner that tracks quotes and ()[]{} depth, so multiple calls, nested calls, and dict/list values survive. Dict literals coerce to JSON objects.
Validated with the touched parser code compiled in an isolated harness (206 formats tests pass); the full crate could not be built here because cmake is missing.
Refs #2392
With the closing `>` now optional, a truncated opener such as `<function=read_file` at end of input matched as a complete name and produced a spurious second call, failing qwen3_coder_malformed_missing_closing_tags_keeps_prior_calls. A name that reaches end of input with no terminator is now treated as truncated: a function name is rejected and a parameter is skipped. Also collapses a nested `if` in the pythonic dict coercion flagged by clippy. Tests: tool_calls:: 446 passed; clippy -D warnings on lib and tests clean. Refs #2392
The quoted-name branch of match_qwen_name searched for `>` to the end of input, so a run of malformed `<parameter="x` openers (now skipped instead of breaking the loop) rescanned the whole tail each time: about 300 ms for 0.5 MB, quadratic in input size. The scan now stops at the next `<` and falls back to the unquoted rule, which also stops a quoted name with a dropped `>` from swallowing the next tag. The pythonic scanner accepted a call list that never closed with `]`, so prose such as `see [note(x) for details` or a list comprehension produced a call even with no tools to filter it. The list must now close right after its last call, matching the brief and the old `)]` requirement. Also refresh the stale pythonic and Mistral doc comments, and add a parse_tool_calls dispatch test for the three #2392 inputs. tool_calls tests (450), clippy and fmt pass. Refs #2392
Member
Author
Implementation Review SummaryIntent
Findings Addressed
Remaining Items
Verification
|
Member
Author
Security and Performance ReviewNo CRITICAL or HIGH findings, so no commits. Fuzzing about 300k random inputs (including multibyte characters) through Remaining Items
Verification
|
The pythonic scanner spent one of its 64 retries on every `[`, so 65 non-call brackets like `[1]` hid a later valid call that main found. Only candidates that reach `name(` and scan for the closing `)` now count.
try_mistral_nemo now keeps a model-supplied id only when it matches `^[A-Za-z0-9_-]{1,64}$` and is unique within the result; otherwise a fresh id is minted. try_mistral_bracket stops after 1024 calls like the sibling parsers.
Tests: tool_calls:: 453 passed; clippy -D warnings on lib and tests clean; cargo fmt --check clean.
Refs #2392
Chat Completions and Responses now return a tool-call id the model emitted instead of always minting one. Document that in server-features.md and responses-api.md, and add an Unreleased Fixed entry covering the qwen3_coder, Mistral and pythonic parser fixes. No Korean docs tree exists in this repository, so no translation was added. Validation: - cargo test --profile test-fast -p mlxcel tool_calls:: (453 passed) - cargo clippy -p mlxcel --lib --tests -- -D warnings and cargo fmt --check clean Refs #2392
…all-parser-gaps # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three tool-call parser gaps in
src/server/tool_calls/formats.rsand passes a model-emitted tool-call id through to clients.\s*([^\s<>]+)>?, so a dropped>no longer folds the next tag into the name. A parameter name that cannot be matched is skipped, not the whole call. A name that runs to end of input (truncated generation) is rejected. The quoted-name search stops at the next<, which keeps the scan linear on adversarial input.[ARGS]:try_mistral_bracketloops over([\w-]+)\s*\[ARGS\]\s*headers and reads one JSON value after each one, the equivalent ofraw_decode. Parallel and hyphenated calls now parse. A truncated trailing call is dropped. Parsing stops atMISTRAL_MAX_CALLS(1024).ParsedToolCallgainsid: Option<String>.try_mistral_nemosets it from the list element only when it matches^[A-Za-z0-9_-]{1,64}$and is unique in the result. Chat (build_tool_call_responses, streamingroutes/chat.rs) and Responses (responses_translator.rs, streamingroutes/responses.rs) use it as the call id when present and mint one otherwise. The Responsesfc_item id is still minted.()[]{}depth replaces the regex. It emits one call pername(...), requires the closing], and converts JSON-style dict literals to objects.split_pythonic_commastracks all three bracket kinds. The 64-candidate retry cap counts only realname(candidates, so[1]or markdown links before a call do not use up retries. The "whole body is valid JSON, decline" guard is unchanged.docs/server-features.md,docs/responses-api.mdand a CHANGELOG entry describe the id passthrough.Not in scope
anthropic_translator.rs,routes/anthropic.rs) still mintstoolu_ids and ignorescall.id.{'k': True},None) stay raw strings. Only JSON-syntax dicts become objects.Test plan
MLXCEL_BUILD_METAL=0 cargo test --profile test-fast -p mlxcel tool_calls::: 453 passed, 0 failed (CPU-only MLX build on macOS)MLXCEL_BUILD_METAL=0 cargo clippy -p mlxcel --lib --tests -- -D warnings: cleancargo fmt --check: cleanparse_tool_callsdispatchResolves #2392