Skip to content
Merged
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
63 changes: 33 additions & 30 deletions crates/goose-providers/src/formats/openai.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::conversation::message::{Message, MessageContent, ProviderMetadata};
use crate::conversation::token_usage::{ProviderUsage, Usage};
use crate::errors::ProviderError;
use crate::images::{convert_image, detect_image_path, load_image_file, ImageFormat};
use crate::json::safely_parse_json;
use crate::json::{parse_tool_arguments, truncation_error_message};
use crate::mcp_utils::extract_text_from_resource;
use crate::model::ModelConfig;
use crate::thinking::{
Expand Down Expand Up @@ -653,22 +653,23 @@ pub fn response_to_message(response: &Value) -> anyhow::Result<Message> {
metadata.as_ref(),
));
} else {
match safely_parse_json(&arguments_str) {
Ok(params) => {
match parse_tool_arguments(&arguments_str) {
Some(params) => {
content.push(MessageContent::tool_request_with_metadata(
id,
Ok(CallToolRequestParams::new(function_name)
.with_arguments(object(params))),
metadata.as_ref(),
));
}
Err(e) => {
None => {
let message_text = truncation_error_message(&arguments_str)
.unwrap_or_else(|| {
format!("Could not interpret tool use parameters for id {id}")
});
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(format!(
"Could not interpret tool use parameters for id {}: {}. Raw arguments: '{}'",
id, e, arguments_str
)),
message: Cow::from(message_text),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
Expand Down Expand Up @@ -1085,12 +1086,6 @@ where

for index in sorted_indices {
if let Some((id, function_name, arguments, extra_fields)) = tool_call_data.get(&index) {
let parsed = if arguments.is_empty() {
Ok(json!({}))
} else {
safely_parse_json(arguments)
};

let metadata = if let Some(sig) = &last_signature {
let mut combined = extra_fields.clone().unwrap_or_default();
combined.insert(
Expand All @@ -1102,26 +1097,34 @@ where
extra_fields.as_ref().filter(|m| !m.is_empty()).cloned()
};

let content = match parsed {
Ok(params) => {
MessageContent::tool_request_with_metadata(
let content = if arguments.is_empty() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Don't bypass truncation guard for empty streamed args

When an OpenAI-compatible stream ends with finish_reason: "length" before any argument deltas arrive, arguments is still empty and this branch turns the truncated call into a successful {} tool request. The collector above stops on any finish reason but doesn't retain that it was length, so a required-argument tool can still be executed with empty parameters instead of surfacing the new truncation error. Please track the final finish reason and treat empty arguments from a length-limited tool call as a parse error rather than a valid no-arg call.

Useful? React with 👍 / 👎.

MessageContent::tool_request_with_metadata(
id.clone(),
Ok(CallToolRequestParams::new(function_name.clone()).with_arguments(object(json!({})))),
metadata.as_ref(),
)
} else {
match parse_tool_arguments(arguments) {
Some(params) => MessageContent::tool_request_with_metadata(
id.clone(),
Ok(CallToolRequestParams::new(function_name.clone()).with_arguments(object(params))),
metadata.as_ref(),
)
},
Err(e) => {
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(format!(
"Could not interpret tool use parameters for id {}: {}",
id, e
)),
data: None,
};
MessageContent::tool_request_with_metadata(id.clone(), Err(error), metadata.as_ref())
),
None => {
let message_text = truncation_error_message(arguments)
.unwrap_or_else(|| {
format!("Could not interpret tool use parameters for id {id}")
});
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(message_text),
data: None,
};
MessageContent::tool_request_with_metadata(id.clone(), Err(error), metadata.as_ref())
}
}
};

contents.push(content);
}
}
Expand Down Expand Up @@ -1934,7 +1937,7 @@ mod tests {
message: msg,
data: None,
}) => {
assert!(msg.starts_with("Could not interpret tool use parameters"));
assert!(msg.contains("tool arguments") || msg.contains("truncated"));
}
_ => panic!("Expected InvalidParameters error"),
}
Expand Down
207 changes: 207 additions & 0 deletions crates/goose-providers/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,121 @@ pub fn json_escape_control_chars_in_string(s: &str) -> String {
r
}

/// Detect whether a raw tool-arguments string looks truncated (the model hit
/// its output-token limit mid-JSON). Returns true when the string has
/// unbalanced or unclosed structural delimiters — whether the cut-off happened
/// mid-value (e.g. `{"path":"/a` with no closing quote) or after a nested
/// closer but before the outer object closed (e.g. `{"items":[1,2]` where the
/// outer `{` is still open).
pub fn looks_truncated(args: &str) -> bool {
let trimmed = args.trim_end();
if trimmed.is_empty() {
return false;
}

let mut in_string = false;
let mut escape_next = false;
let mut depth = Vec::new();

for c in trimmed.chars() {
if in_string {
if escape_next {
escape_next = false;
} else if c == '\\' {
escape_next = true;
} else if c == '"' {
in_string = false;
}
continue;
}

match c {
'"' => in_string = true,
'{' => depth.push('}'),
'[' => depth.push(']'),
'}' | ']' => {
if depth.last() == Some(&c) {
depth.pop();
} else {
return true;
}
}
_ => {}
}
}

in_string || escape_next || !depth.is_empty()
}

/// Build an actionable error message for tool arguments that could not be
/// parsed. `args` is the raw, accumulated arguments string from the provider.
///
/// The message distinguishes truncation (likely from the output token limit)
/// from other malformation, and includes a snippet of where parsing broke.
pub fn truncation_error_message(args: &str) -> Option<String> {
if args.is_empty() {
return None;
}

if serde_json::from_str::<serde_json::Value>(args).is_ok() {
return None;
}

let trimmed = args.trim_end();
let is_truncated = looks_truncated(trimmed);

let snippet = {
let len = trimmed.chars().count();
if len > 80 {
let s: String = trimmed
.chars()
.rev()
.take(80)
.collect::<Vec<_>>()
.into_iter()
.rev()
.collect();
format!("…{s}")
} else {
trimmed.to_string()
}
};

let guidance = if is_truncated {
"The model's response was truncated — it hit the output token limit while generating this tool call. \
Try increasing max_tokens for this provider or breaking the task into smaller steps."
} else {
"The model produced malformed tool arguments. Try resending your message or breaking the task into smaller steps."
};

Some(format!(
"{guidance}\nReceived {} characters; cut off at: {snippet}",
trimmed.chars().count()
))
}

/// Parse tool-call arguments, returning `None` when the input looks truncated
/// so callers can surface an actionable error rather than invoking a tool with
/// incomplete arguments. Non-truncated malformation (e.g. unescaped control
/// characters some models emit) is still repaired via [`safely_parse_json`].
pub fn parse_tool_arguments(args: &str) -> Option<serde_json::Value> {
if args.is_empty() {
return Some(serde_json::Value::Object(serde_json::Map::new()));
}

if let Ok(value) = serde_json::from_str::<serde_json::Value>(args) {
return Some(value);
}

if !looks_truncated(args) {
if let Ok(value) = safely_parse_json(args) {
Comment thread
vincenzopalazzo marked this conversation as resolved.
return Some(value);
}
}

None
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -218,4 +333,96 @@ mod tests {
"Hello\\u0001World"
);
}

#[test]
fn test_truncation_error_message_valid_json() {
assert!(truncation_error_message(r#"{"key":"value"}"#).is_none());
assert!(truncation_error_message(r#"{}"#).is_none());
Comment thread
vincenzopalazzo marked this conversation as resolved.
assert!(truncation_error_message(r#"{"a":[1,2],"b":{"c":3}}"#).is_none());
assert!(truncation_error_message(r#"[1,2,3]"#).is_none());
assert!(truncation_error_message(r#"{"a":{"b":"c"}}"#).is_none());
assert!(truncation_error_message("").is_none());
}

#[test]
fn test_looks_truncated_nested_closers() {
// Truncated after inner array closes, but outer object still open.
assert!(looks_truncated(r#"{"items":[1,2]"#));
// Truncated after inner object closes, but outer object still open.
assert!(looks_truncated(r#"{"patch":{"path":"x"}"#));
// Truncated mid-string.
assert!(looks_truncated(
r##"{"path":"/report.md","content":"# cut"##
));
// Truncated mid-key.
assert!(looks_truncated(r#"{"key":"val"#));

// Well-formed JSON is NOT truncated.
assert!(!looks_truncated(r#"{"key":"value"}"#));
assert!(!looks_truncated(r#"{"a":[1,2],"b":{"c":3}}"#));
assert!(!looks_truncated(r#"[1,2,3]"#));
assert!(!looks_truncated(r#"{"a":{"b":"c"}}"#));
assert!(!looks_truncated(r#"{}"#));
assert!(!looks_truncated(""));
}

#[test]
fn test_parse_tool_arguments_nested_closers_truncated() {
// These end with ] or } so the old check passed, but the outer object
// is still open — silently repairing these would invoke tools with
// incomplete arguments.
let case1 = r#"{"items":[1,2]"#;
assert!(parse_tool_arguments(case1).is_none());

let case2 = r#"{"patch":{"path":"x"}"#;
assert!(parse_tool_arguments(case2).is_none());
}

#[test]
fn test_parse_tool_arguments_control_char_recovery() {
// Unescaped control chars (raw newline) inside a string value should
// still parse successfully via safely_parse_json fallback.
let args = "{\"key\": \"value\nwith newline\"}";
let parsed = parse_tool_arguments(args).expect("control-char JSON should parse");
assert_eq!(parsed["key"], "value\nwith newline");
}

#[test]
fn test_parse_tool_arguments_truncated_fails() {
let truncated = r##"{"path":"/report.md","content":"# Big report that got cut"##;
assert!(
parse_tool_arguments(truncated).is_none(),
"truncated JSON should NOT parse (would silently invoke tool with truncated content)"
);
}

#[test]
fn test_parse_tool_arguments_strict_json() {
let valid = r#"{"key":"value"}"#;
assert!(parse_tool_arguments(valid).is_some());
assert!(parse_tool_arguments("").is_some());
}

#[test]
fn test_truncation_error_message_truncated() {
let truncated = r##"{"path":"/report.md","content":"# Big report that got cut"##;
let msg =
truncation_error_message(truncated).expect("truncated args should produce an error");
assert!(msg.contains("truncated"), "msg: {msg}");
assert!(
msg.contains("max_tokens") || msg.contains("smaller steps"),
"msg: {msg}"
);
assert!(msg.contains("cut off at:"), "msg: {msg}");
}

#[test]
fn test_truncation_error_message_malformed() {
// Malformed JSON that ends with } (not truncated, just broken).
// safely_parse_json should fail too, so truncation_error_message fires.
let malformed = r##"{"key": }"##;
let msg =
truncation_error_message(malformed).expect("malformed args should produce an error");
assert!(msg.contains("malformed"), "msg: {msg}");
}
}
22 changes: 9 additions & 13 deletions crates/goose/src/agents/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2194,7 +2194,15 @@ impl Agent {
.collect();

for request in frontend_requests.iter().chain(remaining_requests.iter()) {
if request.tool_call.is_ok() {
if let Err(err) = &request.tool_call {
let err_msg = err.message.to_string();
error!("Tool call could not be parsed: {}", err_msg);
yield AgentEvent::Message(
Message::assistant().with_text(err_msg)
);
exit_chat = true;
break;
} else {
let mut request_msg = Message::assistant()
.with_id(format!("msg_{}", Uuid::new_v4()));

Expand All @@ -2221,18 +2229,6 @@ impl Agent {
messages_to_add.push(request_msg);
yield AgentEvent::Message(final_response.clone());
messages_to_add.push(final_response);
} else {
error!(
"Tool call could not be parsed: {}",
request.tool_call.as_ref().unwrap_err(),
);
yield AgentEvent::Message(
Message::assistant().with_text(
"A tool call could not be parsed — the response may have been truncated. Try breaking the task into smaller steps or resending your message."
)
);
exit_chat = true;
break;
}
}

Expand Down
Loading
Loading