Repository navigation
Conversation
vLLM serializes tool_calls[].arguments with Python json.dumps (spaced ", "/": "), while SMG's XML-style parsers used serde_json::to_string (compact). The Qwen3 chat template emits arguments verbatim, so on every multi-turn step that feeds back a prior tool call, the SMG arm rendered different prompt tokens than pure vLLM — diverging generations that compound across turns (confirmed: a byte-for-byte template-parity test matches HF except for this arg spacing). Add helpers::args_to_json_string (spaced separators, UTF-8 escaping unchanged) and use it in the XML-derived parsers that build args from <parameter> tags: qwen_xml, minimax_m2, deepseek_dsml. kimik2 already passes the model's raw arguments string through, so it's unchanged. Type coercion is preserved (only spacing changes). Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
github-actions
Bot
added
tests
📝 WalkthroughWalkthroughA new ChangesSpaced JSON serialization for tool-call arguments
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 introduces a custom JSON serialization helper, args_to_json_string, which formats JSON with Python-like spacing (", " and ": ") to match vLLM's json.dumps behavior. This helper is integrated into the DeepSeek DSML, Minimax M2, and Qwen XML parsers. The review feedback points out that the streaming/incremental parameter parsing implementations in both the Qwen XML and Minimax M2 parsers still use compact serialization for nested objects, which could cause a mismatch with the final parsed arguments. Additionally, it is recommended to use String::from_utf8 instead of String::from_utf8_lossy in the helper function to avoid unnecessary scanning and allocation overhead.
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.
|
|
||
| let arguments = serde_json::to_string(¶meters) | ||
| .map_err(|e| ParserError::ParsingFailed(e.to_string()))?; | ||
| let arguments = helpers::args_to_json_string(¶meters)?; |
There was a problem hiding this comment.
While using helpers::args_to_json_string here ensures spaced serialization for complete parsing, the incremental/streaming implementation in parse_and_stream_parameters (specifically on lines 255 and 279) still uses serde_json::to_string(value) to serialize individual parameter values. If a parameter value is a nested object or array, it will be serialized compactly (e.g., {"nested":"value"}), leading to a mismatch/divergence between the streamed arguments and the final parsed arguments. Consider updating parse_and_stream_parameters to also use helpers::args_to_json_string(value) for serializing parameter values.
| let parameters = self.parse_parameters(params_text, ¶m_types); | ||
|
|
||
| match serde_json::to_string(¶meters) { | ||
| match helpers::args_to_json_string(¶meters) { |
There was a problem hiding this comment.
While using helpers::args_to_json_string here ensures spaced serialization for complete parsing, the incremental/streaming implementation in parse_and_stream_parameters (specifically on lines 273 and 300) still uses serde_json::to_string(value) to serialize individual parameter values. If a parameter value is a nested object or array, it will be serialized compactly (e.g., {"nested":"value"}), leading to a mismatch/divergence between the streamed arguments and the final parsed arguments. Consider updating parse_and_stream_parameters to also use helpers::args_to_json_string(value) for serializing parameter values.
| value | ||
| .serialize(&mut ser) | ||
| .map_err(|e| ParserError::ParsingFailed(e.to_string()))?; | ||
| Ok(String::from_utf8_lossy(&buf).into_owned()) |
There was a problem hiding this comment.
Using String::from_utf8_lossy scans the entire buffer for invalid UTF-8 sequences and may allocate/copy. Since serde_json serialization is guaranteed to produce valid UTF-8, we can use String::from_utf8 to perform a zero-copy conversion of the Vec<u8> buffer into a String and avoid this overhead.
| value | |
| .serialize(&mut ser) | |
| .map_err(|e| ParserError::ParsingFailed(e.to_string()))?; | |
| Ok(String::from_utf8_lossy(&buf).into_owned()) | |
| value | |
| .serialize(&mut ser) | |
| .map_err(|e| ParserError::ParsingFailed(e.to_string()))?; | |
| String::from_utf8(buf).map_err(|e| ParserError::ParsingFailed(e.to_string())) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Serialize tool-call arguments like Python `json.dumps` (spaced separators), so | ||
| /// they match vLLM — chat templates emit `arguments` verbatim, so compact vs spaced | ||
| /// would diverge the prompt across multi-turn tool calls. | ||
| pub fn args_to_json_string<T: serde::Serialize>(value: &T) -> ParserResult<String> { |
There was a problem hiding this comment.
🟡 Nit: The JSON-based parsers (llama.rs:82, json.rs:146, qwen.rs:93, deepseek.rs:123, deepseek31.rs:110, glm4_moe.rs:142, step3.rs:396, pythonic.rs:284, mistral.rs:191) also parse model output and re-serialize with serde_json::to_string (compact). If vLLM re-serializes those with json.dumps (spaced) too, they'd have the same spacing divergence in multi-turn prompts.
Intentionally out of scope for this PR, or worth a follow-up?
There was a problem hiding this comment.
Clean, well-scoped change. The SpacedFormatter correctly replicates Python json.dumps separators, the three XML parser call sites are straightforward substitutions, and the test validates the output format.
One nit posted: the JSON-based parsers also re-serialize with compact spacing — worth considering whether they need the same treatment in a follow-up.
0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tool_parser/tests/tool_parser_minimax_m2.rs`:
- Around line 982-986: The assertions in the test starting at line 982 only
verify spaced key-value separators (`: `) but do not check item separators in
the JSON output. Expand the test assertions to also verify that item separators
are properly spaced by adding checks that confirm the presence of spaced item
separators (`, `) and the absence of compact item separators (`,`) in the
variable `a`, similar to the existing key-value separator assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e34d7a95-afce-42a4-bab7-add84f3d2526
📒 Files selected for processing (5)
crates/tool_parser/src/parsers/deepseek_dsml.rscrates/tool_parser/src/parsers/helpers.rscrates/tool_parser/src/parsers/minimax_m2.rscrates/tool_parser/src/parsers/qwen_xml.rscrates/tool_parser/tests/tool_parser_minimax_m2.rs
| assert!( | ||
| a.contains("\": \""), | ||
| "expected spaced key/value separators, got {a}" | ||
| ); | ||
| assert!(!a.contains("\":\""), "must not be compact, got {a}"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Expand the spacing assertion to cover item separators too.
The test currently checks ": " only; compact item separators (",") could regress without failing this test.
Suggested test assertion update
assert!(
a.contains("\": \""),
"expected spaced key/value separators, got {a}"
);
assert!(!a.contains("\":\""), "must not be compact, got {a}");
+ assert!(a.contains(", "), "expected spaced item separators, got {a}");
+ assert!(!a.contains(",\""), "must not use compact item separators, got {a}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tool_parser/tests/tool_parser_minimax_m2.rs` around lines 982 - 986,
The assertions in the test starting at line 982 only verify spaced key-value
separators (`: `) but do not check item separators in the JSON output. Expand
the test assertions to also verify that item separators are properly spaced by
adding checks that confirm the presence of spaced item separators (`, `) and the
absence of compact item separators (`,`) in the variable `a`, similar to the
existing key-value separator assertions.
1 participant
Description
Problem
vLLM serializes
tool_calls[].argumentswith Pythonjson.dumps— spaced separators ({"folder": "tmp"}). SMG's XML-derived parsers usedserde_json::to_string— compact ({"folder":"tmp"}).A byte-for-byte parity test (SMG's
ChatTemplateProcessorvs HFapply_chat_templatefor a Qwen3 multi-turn conversation) matches except for this spacing. Because the chat template emitsargumentsverbatim, every multi-turn step that feeds back a prior tool call renders to different prompt tokens on the SMG arm than on pure vLLM — diverging generations that compound across turns (one driver of the BFCLmulti_turngap; the other, sampling nondeterminism, is addressed by the greedy A/B change).Solution
Add
helpers::args_to_json_string(a serde_jsonFormatterwith Pythonjson.dumpsseparators) and use it in the XML-style parsers that build args from<parameter>tags —qwen_xml,minimax_m2,deepseek_dsml. Type coercion is unchanged; only the output spacing changes.kimik2already passes the model's raw arguments string through, so it's untouched. (String escaping stays serde's UTF-8 default =ensure_ascii=False.)Changes
crates/tool_parser/src/parsers/helpers.rs—args_to_json_string+SpacedFormatter.qwen_xml.rs,minimax_m2.rs,deepseek_dsml.rs— use it for the final argument serialization.tool_parser_minimax_m2.rs— assert spaced output.Test Plan
cargo test -p tool-parser— all pass (existing parser tests compare parsedValue, so they're spacing-agnostic; added an explicit spaced-output assertion).cargo +nightly fmt+cargo clippy -p tool-parserclean.Context
From the multi_turn deep-dive: SMG's chat-template rendering is byte-identical to HF/vLLM; the only systematic frontend difference was this argument spacing.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Refactor
Tests