diff --git a/crates/goose/src/providers/formats/databricks.rs b/crates/goose/src/providers/formats/databricks.rs index 0d19f834..68092926 100644 --- a/crates/goose/src/providers/formats/databricks.rs +++ b/crates/goose/src/providers/formats/databricks.rs @@ -1,7 +1,7 @@ use crate::message::{Message, MessageContent}; use crate::model::ModelConfig; use crate::providers::utils::{ - convert_image, detect_image_path, is_valid_function_name, load_image_file, + convert_image, detect_image_path, is_valid_function_name, load_image_file, safely_parse_json, sanitize_function_name, ImageFormat, }; use anyhow::{anyhow, Error}; @@ -324,14 +324,19 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { .as_str() .unwrap_or_default() .to_string(); - let mut arguments = tool_call["function"]["arguments"] + + // Get the raw arguments string from the LLM. + let arguments_str = tool_call["function"]["arguments"] .as_str() .unwrap_or_default() .to_string(); - // If arguments is empty, we will have invalid json parsing error later. - if arguments.is_empty() { - arguments = "{}".to_string(); - } + + // If arguments_str is empty, default to an empty JSON object string. + let arguments_str = if arguments_str.is_empty() { + "{}".to_string() + } else { + arguments_str + }; if !is_valid_function_name(&function_name) { let error = ToolError::NotFound(format!( @@ -340,7 +345,7 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { )); content.push(MessageContent::tool_request(id, Err(error))); } else { - match serde_json::from_str::(&arguments) { + match safely_parse_json(&arguments_str) { Ok(params) => { content.push(MessageContent::tool_request( id, @@ -349,8 +354,8 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { } Err(e) => { let error = ToolError::InvalidParameters(format!( - "Could not interpret tool use parameters for id {}: {}", - id, e + "Could not interpret tool use parameters for id {}: {}. Raw arguments: '{}'", + id, e, arguments_str )); content.push(MessageContent::tool_request(id, Err(error))); } diff --git a/crates/goose/src/providers/formats/openai.rs b/crates/goose/src/providers/formats/openai.rs index 2a02ae6e..a0e57447 100644 --- a/crates/goose/src/providers/formats/openai.rs +++ b/crates/goose/src/providers/formats/openai.rs @@ -2,7 +2,7 @@ use crate::message::{Message, MessageContent}; use crate::model::ModelConfig; use crate::providers::base::{ProviderUsage, Usage}; use crate::providers::utils::{ - convert_image, detect_image_path, is_valid_function_name, load_image_file, + convert_image, detect_image_path, is_valid_function_name, load_image_file, safely_parse_json, sanitize_function_name, ImageFormat, }; use anyhow::{anyhow, Error}; @@ -284,14 +284,19 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { .as_str() .unwrap_or_default() .to_string(); - let mut arguments = tool_call["function"]["arguments"] + + // Get the raw arguments string from the LLM. + let arguments_str = tool_call["function"]["arguments"] .as_str() .unwrap_or_default() .to_string(); - // If arguments is empty, we will have invalid json parsing error later. - if arguments.is_empty() { - arguments = "{}".to_string(); - } + + // If arguments_str is empty, default to an empty JSON object string. + let arguments_str = if arguments_str.is_empty() { + "{}".to_string() + } else { + arguments_str + }; if !is_valid_function_name(&function_name) { let error = ToolError::NotFound(format!( @@ -300,7 +305,7 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { )); content.push(MessageContent::tool_request(id, Err(error))); } else { - match serde_json::from_str::(&arguments) { + match safely_parse_json(&arguments_str) { Ok(params) => { content.push(MessageContent::tool_request( id, @@ -309,8 +314,8 @@ pub fn response_to_message(response: &Value) -> anyhow::Result { } Err(e) => { let error = ToolError::InvalidParameters(format!( - "Could not interpret tool use parameters for id {}: {}", - id, e + "Could not interpret tool use parameters for id {}: {}. Raw arguments: '{}'", + id, e, arguments_str )); content.push(MessageContent::tool_request(id, Err(error))); } diff --git a/crates/goose/src/providers/utils.rs b/crates/goose/src/providers/utils.rs index c76cdfcb..abfa9f5a 100644 --- a/crates/goose/src/providers/utils.rs +++ b/crates/goose/src/providers/utils.rs @@ -347,6 +347,72 @@ pub fn emit_debug_trace( ); } +/// Safely parse a JSON string that may contain doubly-encoded or malformed JSON. +/// This function first attempts to parse the input string as-is. If that fails, +/// it applies control character escaping and tries again. +/// +/// This approach preserves valid JSON like `{"key1": "value1",\n"key2": "value"}` +/// (which contains a literal \n but is perfectly valid JSON) while still fixing +/// broken JSON like `{"key1": "value1\n","key2": "value"}` (which contains an +/// unescaped newline character). +pub fn safely_parse_json(s: &str) -> Result { + // First, try parsing the string as-is + match serde_json::from_str(s) { + Ok(value) => Ok(value), + Err(_) => { + // If that fails, try with control character escaping + let escaped = json_escape_control_chars_in_string(s); + serde_json::from_str(&escaped) + } + } +} + +/// Helper to escape control characters in a string that is supposed to be a JSON document. +/// This function iterates through the input string `s` and replaces any literal +/// control characters (U+0000 to U+001F) with their JSON-escaped equivalents +/// (e.g., '\n' becomes "\\n", '\u0001' becomes "\\u0001"). +/// +/// It does NOT escape quotes (") or backslashes (\) because it assumes `s` is a +/// full JSON document, and these characters might be structural (e.g., object delimiters, +/// existing valid escape sequences). The goal is to fix common LLM errors where +/// control characters are emitted raw into what should be JSON string values, +/// making the overall JSON structure unparsable. +/// +/// If the input string `s` has other JSON syntax errors (e.g., an unescaped quote +/// *within* a string value like `{"key": "string with " quote"}`), this function +/// will not fix them. It specifically targets unescaped control characters. +pub fn json_escape_control_chars_in_string(s: &str) -> String { + let mut r = String::with_capacity(s.len()); // Pre-allocate for efficiency + for c in s.chars() { + match c { + // ASCII Control characters (U+0000 to U+001F) + '\u{0000}'..='\u{001F}' => { + match c { + '\u{0008}' => r.push_str("\\b"), // Backspace + '\u{000C}' => r.push_str("\\f"), // Form feed + '\n' => r.push_str("\\n"), // Line feed + '\r' => r.push_str("\\r"), // Carriage return + '\t' => r.push_str("\\t"), // Tab + // Other control characters (e.g., NUL, SOH, VT, etc.) + // that don't have a specific short escape sequence. + _ => { + r.push_str(&format!("\\u{:04x}", c as u32)); + } + } + } + // Other characters are passed through. + // This includes quotes (") and backslashes (\). If these are part of the + // JSON structure (e.g. {"key": "value"}) or part of an already correctly + // escaped sequence within a string value (e.g. "string with \\\" quote"), + // they are preserved as is. This function does not attempt to fix + // malformed quote or backslash usage *within* string values if the LLM + // generates them incorrectly (e.g. {"key": "unescaped " quote in string"}). + _ => r.push(c), + } + } + r +} + #[cfg(test)] mod tests { use super::*; @@ -572,4 +638,89 @@ mod tests { assert_eq!(result, expected_status); } } + + #[test] + fn test_safely_parse_json() { + // Test valid JSON that should parse without escaping (contains proper escape sequence) + let valid_json = r#"{"key1": "value1","key2": "value2"}"#; + let result = safely_parse_json(valid_json).unwrap(); + assert_eq!(result["key1"], "value1"); + assert_eq!(result["key2"], "value2"); + + // Test JSON with actual unescaped newlines that needs escaping + let invalid_json = "{\"key1\": \"value1\n\",\"key2\": \"value2\"}"; + let result = safely_parse_json(invalid_json).unwrap(); + assert_eq!(result["key1"], "value1\n"); + assert_eq!(result["key2"], "value2"); + + // Test already valid JSON - should parse on first try + let good_json = r#"{"test": "value"}"#; + let result = safely_parse_json(good_json).unwrap(); + assert_eq!(result["test"], "value"); + + // Test completely invalid JSON that can't be fixed + let broken_json = r#"{"key": "unclosed_string"#; + assert!(safely_parse_json(broken_json).is_err()); + + // Test empty object + let empty_json = "{}"; + let result = safely_parse_json(empty_json).unwrap(); + assert!(result.as_object().unwrap().is_empty()); + + // Test JSON with escaped newlines (valid JSON) - should parse on first try + let escaped_json = r#"{"key": "value with\nnewline"}"#; + let result = safely_parse_json(escaped_json).unwrap(); + assert_eq!(result["key"], "value with\nnewline"); + } + + #[test] + fn test_json_escape_control_chars_in_string() { + // Test basic control character escaping + assert_eq!( + json_escape_control_chars_in_string("Hello\nWorld"), + "Hello\\nWorld" + ); + assert_eq!( + json_escape_control_chars_in_string("Hello\tWorld"), + "Hello\\tWorld" + ); + assert_eq!( + json_escape_control_chars_in_string("Hello\rWorld"), + "Hello\\rWorld" + ); + + // Test multiple control characters + assert_eq!( + json_escape_control_chars_in_string("Hello\n\tWorld\r"), + "Hello\\n\\tWorld\\r" + ); + + // Test that quotes and backslashes are preserved (not escaped) + assert_eq!( + json_escape_control_chars_in_string("Hello \"World\""), + "Hello \"World\"" + ); + assert_eq!( + json_escape_control_chars_in_string("Hello\\World"), + "Hello\\World" + ); + + // Test JSON-like string with control characters + assert_eq!( + json_escape_control_chars_in_string("{\"message\": \"Hello\nWorld\"}"), + "{\"message\": \"Hello\\nWorld\"}" + ); + + // Test no changes for normal strings + assert_eq!( + json_escape_control_chars_in_string("Hello World"), + "Hello World" + ); + + // Test other control characters get unicode escapes + assert_eq!( + json_escape_control_chars_in_string("Hello\u{0001}World"), + "Hello\\u0001World" + ); + } }