feat: Enhance textual tool call parsing, pre-validate arguments, and refine tool failure detection to improve agent robustness.

vinhnx · vinhnx · commit 4e52efeb7251 · 2025-12-26T01:41:53.000+07:00
diff --git a/src/agent/runloop/text_tools.rs b/src/agent/runloop/text_tools.rs
@@ -624,6 +624,7 @@ fn convert_harmony_args_to_tool_format(tool_name: &str, parsed: Value) -> Result
 
 fn parse_tagged_tool_call(text: &str) -> Option<(String, Value)> {
     const TOOL_TAG: &str = "<tool_call>";
+    const TOOL_TAG_CLOSE: &str = "</tool_call>";
     const ARG_KEY_TAG: &str = "<arg_key>";
     const ARG_VALUE_TAG: &str = "<arg_value>";
     const ARG_KEY_CLOSE: &str = "</arg_key>";
@@ -640,7 +641,10 @@ fn parse_tagged_tool_call(text: &str) -> Option<(String, Value)> {
     let mut indexed_values: BTreeMap<String, BTreeMap<usize, Value>> = BTreeMap::new();
     rest = after_name;
 
+    // First, try standard <arg_key>/<arg_value> parsing
+    let mut found_arg_tags = false;
     while let Some(key_index) = rest.find(ARG_KEY_TAG) {
+        found_arg_tags = true;
         rest = &rest[key_index + ARG_KEY_TAG.len()..];
         let (raw_key, mut after_key) = read_tag_text(rest);
         if raw_key.is_empty() {
@@ -674,6 +678,60 @@ fn parse_tagged_tool_call(text: &str) -> Option<(String, Value)> {
         }
     }
 
+    // If no arg tags found, try fallback parsing for malformed output
+    // e.g., <tool_call>list_files<tool_call>{"path": "/tmp"} or <tool_call>read_file path="/tmp"
+    if !found_arg_tags && object.is_empty() {
+        // Determine the content boundary (next <tool_call>, </tool_call>, or end)
+        let content_end = after_name
+            .find(TOOL_TAG)
+            .or_else(|| after_name.find(TOOL_TAG_CLOSE))
+            .unwrap_or(after_name.len());
+        let content = after_name[..content_end].trim();
+
+        if !content.is_empty() {
+            // Try parsing as JSON first
+            if let Some(json_start) = content.find('{') {
+                let json_content = &content[json_start..];
+                // Find matching closing brace
+                let mut depth = 0;
+                let mut json_end = None;
+                for (idx, ch) in json_content.char_indices() {
+                    match ch {
+                        '{' => depth += 1,
+                        '}' => {
+                            depth -= 1;
+                            if depth == 0 {
+                                json_end = Some(idx + 1);
+                                break;
+                            }
+                        }
+                        _ => {}
+                    }
+                }
+                if let Some(end) = json_end {
+                    if let Ok(parsed) = serde_json::from_str::<Value>(&json_content[..end]) {
+                        if let Some(obj) = parsed.as_object() {
+                            for (k, v) in obj {
+                                object.insert(k.clone(), v.clone());
+                            }
+                        }
+                    }
+                }
+            }
+
+            // If JSON parsing didn't work, try key=value or key:value pairs
+            if object.is_empty() {
+                if let Some(parsed) = parse_key_value_arguments(content) {
+                    if let Some(obj) = parsed.as_object() {
+                        for (k, v) in obj {
+                            object.insert(k.clone(), v.clone());
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     for (base, entries) in indexed_values {
         let offset = if entries.contains_key(&0) {
             0usize
@@ -1593,4 +1651,63 @@ mode: overwrite
             "Should reject Harmony format with whitespace-only command"
         );
     }
+
+    // ==================== Tests for malformed XML handling (GLM models) ====================
+
+    #[test]
+    fn test_parse_tagged_tool_call_handles_double_tag_malformed_xml() {
+        // GLM models sometimes output: <tool_call>list_files<tool_call>list
+        // Should extract tool name but with empty args
+        let message = "<tool_call>list_files<tool_call>list";
+        let result = parse_tagged_tool_call(message);
+        assert!(result.is_some(), "Should parse malformed double-tag XML");
+        let (name, args) = result.unwrap();
+        assert_eq!(name, "list_files");
+        // Args should be empty object since no valid args were found
+        assert!(args.as_object().map_or(true, |o| o.is_empty()));
+    }
+
+    #[test]
+    fn test_parse_tagged_tool_call_extracts_json_after_name() {
+        // When JSON appears after the tool name
+        let message = r#"<tool_call>read_file{"path": "/tmp/test.txt"}</tool_call>"#;
+        let result = parse_tagged_tool_call(message);
+        assert!(result.is_some(), "Should parse JSON after tool name");
+        let (name, args) = result.unwrap();
+        assert_eq!(name, "read_file");
+        assert_eq!(args.get("path").and_then(|v| v.as_str()), Some("/tmp/test.txt"));
+    }
+
+    #[test]
+    fn test_parse_tagged_tool_call_extracts_json_with_space() {
+        // When JSON appears after tool name with space
+        let message = r#"<tool_call>read_file {"path": "/tmp/test.txt"}</tool_call>"#;
+        let result = parse_tagged_tool_call(message);
+        assert!(result.is_some(), "Should parse JSON with space after tool name");
+        let (name, args) = result.unwrap();
+        assert_eq!(name, "read_file");
+        assert_eq!(args.get("path").and_then(|v| v.as_str()), Some("/tmp/test.txt"));
+    }
+
+    #[test]
+    fn test_parse_tagged_tool_call_handles_nested_json() {
+        // Nested JSON should be parsed correctly
+        let message = r#"<tool_call>run_pty_cmd{"command": "echo", "env": {"PATH": "/usr/bin"}}</tool_call>"#;
+        let result = parse_tagged_tool_call(message);
+        assert!(result.is_some(), "Should parse nested JSON");
+        let (name, args) = result.unwrap();
+        assert_eq!(name, "run_pty_cmd");
+        assert_eq!(args.get("command").and_then(|v| v.as_str()), Some("echo"));
+        assert!(args.get("env").and_then(|v| v.as_object()).is_some());
+    }
+
+    #[test]
+    fn test_parse_tagged_tool_call_stops_at_next_tool_call_tag() {
+        // Content boundary should be the next <tool_call> tag
+        let message = "<tool_call>list_files<tool_call>read_file";
+        let result = parse_tagged_tool_call(message);
+        assert!(result.is_some());
+        let (name, _) = result.unwrap();
+        assert_eq!(name, "list_files");
+    }
 }
diff --git a/src/agent/runloop/unified/turn/run_loop.rs b/src/agent/runloop/unified/turn/run_loop.rs
@@ -2843,14 +2843,26 @@ pub(crate) async fn run_single_agent_loop_unified(
                                         tool_spinner.finish();
                                         autonomous_executor.record_execution(name, false);
 
-                                        // Increment failure counter for this tool signature
-                                        let failed_attempts = repeated_tool_attempts
-                                            .entry(signature_key.clone())
-                                            .or_insert(0);
-                                        *failed_attempts += 1;
+                                        // Check if this is a "missing required params" error
+                                        // These should not count toward loop detection since:
+                                        // 1. They all have the same signature (empty args)
+                                        // 2. The model needs guidance, not a loop detection abort
+                                        let error_str = format!("{:?}", error);
+                                        let is_missing_params_error = error_str.contains("Missing required")
+                                            || error_str.contains("Invalid") && error_str.contains("arguments");
+
+                                        if !is_missing_params_error {
+                                            // Only increment failure counter for non-param-validation errors
+                                            let failed_attempts = repeated_tool_attempts
+                                                .entry(signature_key.clone())
+                                                .or_insert(0);
+                                            *failed_attempts += 1;
+                                        }
+                                        // If it IS a missing params error, we don't increment the counter
+                                        // The enhanced error message will guide the model to correct format
 
                                         // Convert the tool error into anyhow for the helper
-                                        let any_err = anyhow::anyhow!(format!("{:?}", error));
+                                        let any_err = anyhow::anyhow!(error_str);
                                         // Call the centralized failure handler
                                         run_turn_handle_tool_failure(
                                             name,
diff --git a/src/agent/runloop/unified/turn/turn_processing.rs b/src/agent/runloop/unified/turn/turn_processing.rs
@@ -20,6 +20,7 @@ use vtcode_core::core::decision_tracker::DecisionTracker;
 use vtcode_core::core::pruning_decisions::PruningDecisionLedger;
 use vtcode_core::core::token_budget::TokenBudgetManager;
 use vtcode_core::llm::TokenCounter;
+use vtcode_core::config::constants::tools as tool_names;
 use vtcode_core::llm::provider::{self as uni, ParallelToolConfig};
 use vtcode_core::tools::ToolRegistry;
 use vtcode_core::tools::result_cache::ToolResultCache;
@@ -301,31 +302,48 @@ pub(crate) fn process_llm_response(
         && let Some((name, args)) =
             crate::agent::runloop::text_tools::detect_textual_tool_call(&text)
     {
-        let args_json = serde_json::to_string(&args).unwrap_or_else(|_| "{}".to_string());
-        let code_blocks = crate::agent::runloop::text_tools::extract_code_fence_blocks(&text);
-        if !code_blocks.is_empty() {
-            crate::agent::runloop::tool_output::render_code_fence_blocks(renderer, &code_blocks)?;
-            renderer.line(MessageStyle::Output, "")?;
-        }
-        let (headline, _) =
-            crate::agent::runloop::unified::tool_summary::describe_tool_action(&name, &args);
-        let notice = if headline.is_empty() {
-            format!(
-                "Detected {} request",
-                crate::agent::runloop::unified::tool_summary::humanize_tool_name(&name)
-            )
+        // Validate required arguments before adding the tool call.
+        // This prevents executing tools with empty args that will fail and trigger loop detection.
+        if let Some(missing_params) = validate_required_tool_args(&name, &args) {
+            // Show warning about missing parameters but don't add the tool call.
+            // This allows the model to continue naturally instead of failing execution.
+            let tool_display = crate::agent::runloop::unified::tool_summary::humanize_tool_name(&name);
+            let missing_list = missing_params.join(", ");
+            renderer.line(
+                MessageStyle::Info,
+                &format!(
+                    "Detected {} but missing required params: {}",
+                    tool_display, missing_list
+                ),
+            )?;
+            // Don't set interpreted_textual_call = true, let it fall through to TextResponse
         } else {
-            format!("Detected {headline}")
-        };
-        renderer.line(MessageStyle::Info, &notice)?;
-        let call_id = format!("call_textual_{}", conversation_len);
-        tool_calls.push(uni::ToolCall::function(
-            call_id.clone(),
-            name.clone(),
-            args_json.clone(),
-        ));
-        interpreted_textual_call = true;
-        final_text = None;
+            let args_json = serde_json::to_string(&args).unwrap_or_else(|_| "{}".to_string());
+            let code_blocks = crate::agent::runloop::text_tools::extract_code_fence_blocks(&text);
+            if !code_blocks.is_empty() {
+                crate::agent::runloop::tool_output::render_code_fence_blocks(renderer, &code_blocks)?;
+                renderer.line(MessageStyle::Output, "")?;
+            }
+            let (headline, _) =
+                crate::agent::runloop::unified::tool_summary::describe_tool_action(&name, &args);
+            let notice = if headline.is_empty() {
+                format!(
+                    "Detected {} request",
+                    crate::agent::runloop::unified::tool_summary::humanize_tool_name(&name)
+                )
+            } else {
+                format!("Detected {headline}")
+            };
+            renderer.line(MessageStyle::Info, &notice)?;
+            let call_id = format!("call_textual_{}", conversation_len);
+            tool_calls.push(uni::ToolCall::function(
+                call_id.clone(),
+                name.clone(),
+                args_json.clone(),
+            ));
+            interpreted_textual_call = true;
+            final_text = None;
+        }
     }
 
     // Build result
@@ -353,3 +371,41 @@ pub(crate) fn process_llm_response(
 
     Ok(TurnProcessingResult::Empty)
 }
+
+/// Validates that a textual tool call has required arguments before execution.
+/// Returns `None` if valid, or `Some(missing_params)` if validation fails.
+///
+/// This prevents executing tools with empty args that will just fail,
+/// allowing the model to continue naturally instead of hitting loop detection.
+fn validate_required_tool_args(name: &str, args: &serde_json::Value) -> Option<Vec<&'static str>> {
+    let required: &[&str] = match name {
+        n if n == tool_names::READ_FILE => &["path"],
+        n if n == tool_names::WRITE_FILE => &["path", "content"],
+        n if n == tool_names::EDIT_FILE => &["path", "old_string", "new_string"],
+        n if n == tool_names::LIST_FILES => &["path"],
+        n if n == tool_names::GREP_FILE => &["pattern"],
+        n if n == tool_names::RUN_PTY_CMD => &["command"],
+        n if n == tool_names::APPLY_PATCH => &["patch"],
+        _ => &[],
+    };
+
+    if required.is_empty() {
+        return None;
+    }
+
+    let missing: Vec<&'static str> = required
+        .iter()
+        .filter(|key| {
+            args.get(*key)
+                .map(|v| v.is_null() || (v.is_string() && v.as_str().unwrap_or("").is_empty()))
+                .unwrap_or(true)
+        })
+        .copied()
+        .collect();
+
+    if missing.is_empty() {
+        None
+    } else {
+        Some(missing)
+    }
+}
diff --git a/vtcode-core/src/tools/file_ops.rs b/vtcode-core/src/tools/file_ops.rs
@@ -895,7 +895,14 @@ impl FileOpsTool {
             .or_else(|| args.get("file_path"))
             .and_then(|v| v.as_str())
             .ok_or_else(|| {
-                anyhow!("Error: Invalid 'read_file' arguments. Expected JSON object with: path (required, string). Optional: offset, limit, mode, indentation.")
+                let received = serde_json::to_string(&args).unwrap_or_else(|_| "{}".to_string());
+                anyhow!(
+                    "Error: Invalid 'read_file' arguments. Missing required 'path' parameter.\n\
+                    Received: {}\n\
+                    Expected: {{\"path\": \"/path/to/file\"}}\n\
+                    Optional params: offset, limit, mode, indentation",
+                    received
+                )
             })?;
 
         // Try to resolve the file path
