Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/tool_parser/src/parsers/deepseek_dsml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ impl DeepSeekDsmlParser {
}
}

serde_json::to_string(&Value::Object(params)).unwrap_or_else(|_| "{}".to_string())
helpers::args_to_json_string(&Value::Object(params)).unwrap_or_else(|_| "{}".to_string())
}

/// Parse a single complete invoke block into a ToolCall
Expand Down
48 changes: 48 additions & 0 deletions crates/tool_parser/src/parsers/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,54 @@ pub(crate) fn handle_json_tool_streaming(
Ok(result)
}

/// serde_json formatter with Python `json.dumps` separators (`", "` / `": "`).
struct SpacedFormatter;

impl serde_json::ser::Formatter for SpacedFormatter {
fn begin_array_value<W: ?Sized + std::io::Write>(
&mut self,
writer: &mut W,
first: bool,
) -> std::io::Result<()> {
if first {
Ok(())
} else {
writer.write_all(b", ")
}
}

fn begin_object_key<W: ?Sized + std::io::Write>(
&mut self,
writer: &mut W,
first: bool,
) -> std::io::Result<()> {
if first {
Ok(())
} else {
writer.write_all(b", ")
}
}

fn begin_object_value<W: ?Sized + std::io::Write>(
&mut self,
writer: &mut W,
) -> std::io::Result<()> {
writer.write_all(b": ")
}
}

/// 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> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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?

let mut buf = Vec::new();
let mut ser = serde_json::Serializer::with_formatter(&mut buf, SpacedFormatter);
value
.serialize(&mut ser)
.map_err(|e| ParserError::ParsingFailed(e.to_string()))?;
Ok(String::from_utf8_lossy(&buf).into_owned())
Comment on lines +439 to +442

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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()))

}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion crates/tool_parser/src/parsers/minimax_m2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ impl MinimaxM2Parser {
let param_types = helpers::param_types_for_function(tools, func_name);
let parameters = self.parse_parameters(params_text, &param_types);

match serde_json::to_string(&parameters) {
match helpers::args_to_json_string(&parameters) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Ok(arguments_str) => calls.push(ToolCall {
function: FunctionCall {
name: func_name.to_string(),
Expand Down
3 changes: 1 addition & 2 deletions crates/tool_parser/src/parsers/qwen_xml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ impl QwenXmlParser {
}
}

let arguments = serde_json::to_string(&parameters)
.map_err(|e| ParserError::ParsingFailed(e.to_string()))?;
let arguments = helpers::args_to_json_string(&parameters)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


Ok(Some(ToolCall {
function: FunctionCall {
Expand Down
20 changes: 20 additions & 0 deletions crates/tool_parser/tests/tool_parser_minimax_m2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -965,3 +965,23 @@ async fn test_minimax_invalid_json_in_parameters() {

assert_eq!(normal_text, "");
}

#[tokio::test]
async fn test_minimax_arguments_use_python_json_spacing() {
// Spaced separators (", " / ": "), matching vLLM's json.dumps.
let parser = MinimaxM2Parser::new();
let input = r#"<minimax:tool_call>
<invoke name="get_weather">
<parameter name="city">Tokyo</parameter>
<parameter name="units">celsius</parameter>
</invoke>
</minimax:tool_call>"#;
let (_n, tools) = parser.parse_complete(input).await.unwrap();
assert_eq!(tools.len(), 1);
let a = &tools[0].function.arguments;
assert!(
a.contains("\": \""),
"expected spaced key/value separators, got {a}"
);
assert!(!a.contains("\":\""), "must not be compact, got {a}");
Comment on lines +982 to +986

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

}
Loading