fix(tool_parser): coerce XML tool-call args by declared schema type (qwen_xml, glm4_moe)

crates/tool_parser/src/parsers/glm4_moe.rs

@@ -1,3 +1,5 @@
+use std::collections::HashMap;
+
 use async_trait::async_trait;
 use openai_protocol::common::Tool;
 use regex::Regex;
@@ -89,37 +91,22 @@ impl Glm4MoeParser {
         Self::new(r"(?s)<tool_call>\s*([^<\s]+)\s*(.*?)</tool_call>")
     }
 
-    /// Parse arguments from key-value pairs
-    fn parse_arguments(&self, args_text: &str) -> serde_json::Map<String, Value> {
+    /// Parse arguments, coercing each value by its declared schema type and
+    /// falling back to [`infer_value`] when the type is unknown.
+    fn parse_arguments(
+        &self,
+        args_text: &str,
+        param_types: &HashMap<String, String>,
+    ) -> serde_json::Map<String, Value> {
         let mut arguments = serde_json::Map::new();
 
         for capture in self.arg_extractor.captures_iter(args_text) {
             let key = capture.get(1).map_or("", |m| m.as_str()).trim();
             let value_str = capture.get(2).map_or("", |m| m.as_str()).trim();
 
-            // Try to parse the value as JSON first, fallback to string
-            let value = if let Ok(json_val) = serde_json::from_str::<Value>(value_str) {
-                json_val
-            } else {
-                // Try parsing as Python literal (similar to Python's ast.literal_eval)
-                if value_str == "true" || value_str == "True" {
-                    Value::Bool(true)
-                } else if value_str == "false" || value_str == "False" {
-                    Value::Bool(false)
-                } else if value_str == "null" || value_str == "None" {
-                    Value::Null
-                } else if let Ok(num) = value_str.parse::<i64>() {
-                    Value::Number(num.into())
-                } else if let Ok(num) = value_str.parse::<f64>() {
-                    if let Some(n) = serde_json::Number::from_f64(num) {
-                        Value::Number(n)
-                    } else {
-                        Value::String(value_str.to_string())
-                    }
-                } else {
-                    Value::String(value_str.to_string())
-                }
-            };
+            let value =
+                helpers::coerce_by_schema_type(value_str, param_types.get(key).map(String::as_str))
+                    .unwrap_or_else(|| infer_value(value_str));
 
             arguments.insert(key.to_string(), value);
         }
@@ -128,16 +115,16 @@ impl Glm4MoeParser {
     }
 
     /// Parse a single tool call block
-    fn parse_tool_call(&self, block: &str) -> ParserResult<Option<ToolCall>> {
+    fn parse_tool_call(&self, block: &str, tools: &[Tool]) -> ParserResult<Option<ToolCall>> {
         if let Some(captures) = self.func_detail_extractor.captures(block) {
             // Get function name
             let func_name = captures.get(1).map_or("", |m| m.as_str()).trim();
 
             // Get arguments text
             let args_text = captures.get(2).map_or("", |m| m.as_str());
 
-            // Parse arguments
-            let arguments = self.parse_arguments(args_text);
+            let param_types = helpers::param_types_for_function(tools, func_name);
+            let arguments = self.parse_arguments(args_text, &param_types);
 
             let arguments_str = serde_json::to_string(&arguments)
                 .map_err(|e| ParserError::ParsingFailed(e.to_string()))?;
@@ -154,12 +141,12 @@ impl Glm4MoeParser {
     }
 
     /// Parse all tool calls from text (shared logic for complete and incremental parsing)
-    fn parse_tool_calls_from_text(&self, text: &str) -> Vec<ToolCall> {
-        let mut tools = Vec::new();
+    fn parse_tool_calls_from_text(&self, text: &str, tools: &[Tool]) -> Vec<ToolCall> {
+        let mut parsed = Vec::new();
 
         for mat in self.tool_call_extractor.find_iter(text) {
-            match self.parse_tool_call(mat.as_str()) {
-                Ok(Some(tool)) => tools.push(tool),
+            match self.parse_tool_call(mat.as_str(), tools) {
+                Ok(Some(tool)) => parsed.push(tool),
                 Ok(None) => continue,
                 Err(e) => {
                     tracing::debug!("Failed to parse tool call: {}", e);
@@ -168,20 +155,17 @@ impl Glm4MoeParser {
             }
         }
 
-        tools
+        parsed
     }
 }
 
-impl Default for Glm4MoeParser {
-    fn default() -> Self {
-        Self::glm45()
-    }
-}
-
-#[async_trait]
-impl ToolParser for Glm4MoeParser {
-    async fn parse_complete(&self, text: &str) -> ParserResult<(String, Vec<ToolCall>)> {
-        // Check if text contains GLM-4 MoE format
+impl Glm4MoeParser {
+    /// Shared non-streaming parse, schema-aware when `tools` are provided.
+    fn parse_complete_inner(
+        &self,
+        text: &str,
+        tools: &[Tool],
+    ) -> ParserResult<(String, Vec<ToolCall>)> {
         if !self.has_tool_markers(text) {
             return Ok((text.to_string(), vec![]));
         }
@@ -193,15 +177,58 @@ impl ToolParser for Glm4MoeParser {
             .ok_or_else(|| ParserError::ParsingFailed("tool call marker not found".to_string()))?;
         let normal_text = text[..idx].to_string();
 
-        // Parse all tool calls using shared helper
-        let tools = self.parse_tool_calls_from_text(text);
+        let parsed = self.parse_tool_calls_from_text(text, tools);
 
         // If no tools were successfully parsed despite having markers, return entire text as fallback
-        if tools.is_empty() {
+        if parsed.is_empty() {
             return Ok((text.to_string(), vec![]));
         }
 
-        Ok((normal_text, tools))
+        Ok((normal_text, parsed))
+    }
+}
+
+/// Infer a JSON value from raw text when the schema type is unknown: JSON
+/// (numbers/bools/null/objects/arrays), then Python-style literals, then string.
+fn infer_value(value_str: &str) -> Value {
+    if let Ok(json_val) = serde_json::from_str::<Value>(value_str) {
+        return json_val;
+    }
+    match value_str {
+        "true" | "True" => Value::Bool(true),
+        "false" | "False" => Value::Bool(false),
+        "null" | "None" => Value::Null,
+        _ => {
+            if let Ok(num) = value_str.parse::<i64>() {
+                Value::Number(num.into())
+            } else if let Ok(num) = value_str.parse::<f64>() {
+                serde_json::Number::from_f64(num)
+                    .map_or_else(|| Value::String(value_str.to_string()), Value::Number)
+            } else {
+                Value::String(value_str.to_string())
+            }
+        }
+    }
+}
+
+impl Default for Glm4MoeParser {
+    fn default() -> Self {
+        Self::glm45()
+    }
+}
+
+#[async_trait]
+impl ToolParser for Glm4MoeParser {
+    async fn parse_complete(&self, text: &str) -> ParserResult<(String, Vec<ToolCall>)> {
+        self.parse_complete_inner(text, &[])
+    }
+
+    async fn parse_complete_with_tools(
+        &self,
+        text: &str,
+        tools: &[Tool],
+    ) -> ParserResult<(String, Vec<ToolCall>)> {
+        self.parse_complete_inner(text, tools)
     }
 
     async fn parse_incremental(
@@ -250,7 +277,7 @@ impl ToolParser for Glm4MoeParser {
 
             // Parse the complete block using shared helper
             let block_end = end_pos + self.eot_token.len();
-            let parsed_tools = self.parse_tool_calls_from_text(&current_text[..block_end]);
+            let parsed_tools = self.parse_tool_calls_from_text(&current_text[..block_end], tools);
 
             // Extract normal text before tool calls
             let idx = current_text.find(self.bot_token);
@@ -346,3 +373,69 @@ impl ToolParser for Glm4MoeParser {
         self.streamed_args_for_tool.clear();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use openai_protocol::common::Function;
+
+    use super::*;
+
+    fn tool_with_props(props: Value) -> Vec<Tool> {
+        vec![Tool {
+            tool_type: "function".to_string(),
+            function: Function {
+                name: "f".to_string(),
+                description: None,
+                parameters: serde_json::json!({"type": "object", "properties": props}),
+                strict: None,
+            },
+        }]
+    }
+
+    // String-typed params stay strings even when they look numeric/bool/array.
+    #[tokio::test]
+    async fn test_schema_aware_coercion_keeps_strings() {
+        let tools = tool_with_props(serde_json::json!({
+            "limit": {"type": "string"},
+            "flag": {"type": "string"},
+            "coords": {"type": "string"},
+            "count": {"type": "integer"},
+        }));
+        let text = "<tool_call>f\n\
+            <arg_key>limit</arg_key>\n<arg_value>4</arg_value>\n\
+            <arg_key>flag</arg_key>\n<arg_value>true</arg_value>\n\
+            <arg_key>coords</arg_key>\n<arg_value>[60,30]</arg_value>\n\
+            <arg_key>count</arg_key>\n<arg_value>5</arg_value>\n\
+            </tool_call>";
+        let (_, calls) = Glm4MoeParser::glm45()
+            .parse_complete_with_tools(text, &tools)
+            .await
+            .unwrap();
+        assert_eq!(calls.len(), 1);
+        let args: Value = serde_json::from_str(&calls[0].function.arguments).unwrap();
+        assert_eq!(args["limit"], Value::String("4".to_string()));
+        assert_eq!(args["flag"], Value::String("true".to_string()));
+        assert_eq!(args["coords"], Value::String("[60,30]".to_string()));
+        assert_eq!(args["count"], Value::Number(5.into()));
+    }
+
+    // The streaming path threads `tools` separately, so cover it too.
+    #[tokio::test]
+    async fn test_streaming_schema_aware_coercion() {
+        let tools = tool_with_props(serde_json::json!({
+            "limit": {"type": "string"},
+            "count": {"type": "integer"},
+        }));
+        let text = "<tool_call>f\n\
+            <arg_key>limit</arg_key>\n<arg_value>4</arg_value>\n\
+            <arg_key>count</arg_key>\n<arg_value>5</arg_value>\n\
+            </tool_call>";
+        let result = Glm4MoeParser::glm45()
+            .parse_incremental(text, &tools)
+            .await
+            .unwrap();
+        let args: Value = serde_json::from_str(&result.calls[0].parameters).unwrap();
+        assert_eq!(args["limit"], Value::String("4".to_string()));
+        assert_eq!(args["count"], Value::Number(5.into()));
+    }
+}

crates/tool_parser/src/parsers/helpers.rs

@@ -12,7 +12,8 @@ use crate::{
 /// `param_name -> declared JSON-schema type` for the named function (empty if the
 /// function or its `properties` are absent). Lets XML-style parsers coerce by the
 /// declared type instead of guessing from text (e.g. keep a numeric-looking
-/// `string` as a string).
+/// `string` as a string). Only scalar `type` is read; unions/nullable schemas
+/// have no single type, so they fall back to the caller's inference.
 pub fn param_types_for_function(tools: &[Tool], func_name: &str) -> HashMap<String, String> {
     let mut types = HashMap::new();
     let Some(tool) = tools.iter().find(|t| t.function.name == func_name) else {
@@ -33,23 +34,25 @@ pub fn param_types_for_function(tools: &[Tool], func_name: &str) -> HashMap<Stri
     types
 }
 
-/// Coerce a raw value by its declared JSON-schema type. `string` is kept verbatim;
-/// numeric/boolean/structured types are parsed. `None` (unknown type or parse
-/// failure) means the caller should fall back to its own inference.
+/// Coerce a raw value by its declared JSON-schema type. For `string`, a JSON
+/// string literal (`"4"`) is unwrapped while bare text (`4`, `true`) is kept
+/// verbatim; other types are parsed. `None` (unknown type or parse failure)
+/// means the caller should infer.
 pub fn coerce_by_schema_type(text: &str, declared_type: Option<&str>) -> Option<Value> {
     match declared_type? {
-        "string" => Some(Value::String(text.to_string())),
+        "string" => Some(Value::String(
+            serde_json::from_str::<String>(text).unwrap_or_else(|_| text.to_string()),
+        )),
         "integer" => text
             .trim()
             .parse::<i64>()
             .ok()
             .map(|n| Value::Number(n.into())),
-        "number" => text
-            .trim()
-            .parse::<f64>()
+        // Parse as JSON so large integers keep their precision (a plain f64 parse
+        // would round e.g. 9007199254740993).
+        "number" => serde_json::from_str::<Value>(text.trim())
             .ok()
-            .and_then(serde_json::Number::from_f64)
-            .map(Value::Number),
+            .filter(Value::is_number),
         "boolean" => match text.trim() {
             "true" | "True" => Some(Value::Bool(true)),
             "false" | "False" => Some(Value::Bool(false)),
@@ -622,4 +625,43 @@ mod tests {
             &serde_json::json!({"key": "value"})
         );
     }
+
+    #[test]
+    fn test_coerce_by_schema_type() {
+        use serde_json::json;
+        // string: numeric/bool/array-looking text stays a string; a JSON string
+        // literal is unwrapped.
+        assert_eq!(coerce_by_schema_type("4", Some("string")), Some(json!("4")));
+        assert_eq!(
+            coerce_by_schema_type("true", Some("string")),
+            Some(json!("true"))
+        );
+        assert_eq!(
+            coerce_by_schema_type("[60,30]", Some("string")),
+            Some(json!("[60,30]"))
+        );
+        assert_eq!(
+            coerce_by_schema_type("\"4\"", Some("string")),
+            Some(json!("4"))
+        );
+
+        assert_eq!(coerce_by_schema_type("4", Some("integer")), Some(json!(4)));
+        assert_eq!(
+            coerce_by_schema_type("true", Some("boolean")),
+            Some(json!(true))
+        );
+        assert_eq!(
+            coerce_by_schema_type("[60,30]", Some("array")),
+            Some(json!([60, 30]))
+        );
+        // number keeps integer precision (no f64 rounding).
+        assert_eq!(
+            coerce_by_schema_type("9007199254740993", Some("number")),
+            Some(json!(9007199254740993i64))
+        );
+
+        // unknown type / value that fails to parse -> None (caller infers)
+        assert_eq!(coerce_by_schema_type("4", None), None);
+        assert_eq!(coerce_by_schema_type("abc", Some("integer")), None);
+    }
 }