diff --git a/src/harness/providers/openai/convert.rs b/src/harness/providers/openai/convert.rs index 93254bf..6fd71a8 100644 --- a/src/harness/providers/openai/convert.rs +++ b/src/harness/providers/openai/convert.rs @@ -255,11 +255,102 @@ pub(super) fn parse_tool_arguments( if raw.trim().is_empty() { return Ok(Value::Object(Map::new())); } - serde_json::from_str(raw).map_err(|err| { - TinyAgentsError::Model(format!( - "{context} contained invalid JSON arguments for tool call `{call_id}` (`{name}`): {err}; raw arguments: {raw:?}" - )) - }) + match serde_json::from_str(raw) { + Ok(value) => Ok(value), + Err(err) => { + // Some OpenAI-compatible gateways fail to strip a model's + // chat-template tool-call delimiters (e.g. a trailing ``) + // before placing the call in `function.arguments`, turning + // otherwise-valid JSON into an unparseable blob. Seen in the wild + // leaking a trailing `` into a Composio `composio_execute` + // call, which then never parses — orphaning the sub-agent's reduce. + // Attempt one conservative repair before failing. `recover_tool_arguments` + // runs only after `raw` has already failed to parse, so well-formed + // arguments are never rewritten. + if let Some(value) = recover_tool_arguments(raw) { + return Ok(value); + } + // Still unparseable after repair: fail fast with a clear, + // non-retryable error. Surfacing the failure keeps the malformed + // call from becoming a never-resolving tool call that stalls the + // agent loop (and the parent's join/reduce) indefinitely. + Err(TinyAgentsError::Model(format!( + "{context} contained invalid JSON arguments for tool call `{call_id}` (`{name}`): {err}; raw arguments: {raw:?}" + ))) + } + } +} + +/// Chat-template tool-call delimiters that some OpenAI-compatible gateways fail +/// to strip before placing a call in `function.arguments`. Different model +/// families (Hermes/Qwen/Kimi/…) wrap tool calls in their own markers; a +/// leaked one turns valid argument JSON unparseable. +const TOOL_CALL_TEMPLATE_MARKERS: &[&str] = &[ + "<|tool_calls_section_end|>", + "<|tool_call_begin|>", + "<|tool_call_end|>", + "<|tool_call|>", + "<|tool_sep|>", + "", + "", + "", +]; + +/// Attempts to recover a usable arguments object from a tool-call arguments +/// string that failed to parse as JSON. +/// +/// Two conservative strategies, tried in order; the caller only invokes this +/// *after* the raw string has already failed `serde_json::from_str`, so this can +/// never rewrite arguments that were already valid: +/// +/// 1. Strip leaked chat-template tool-call delimiters (see +/// [`TOOL_CALL_TEMPLATE_MARKERS`]) and re-parse — recovers a valid call whose +/// only corruption is a leaked marker (e.g. `{"a":1}`). +/// 2. Take the first complete JSON *object* from the front of the (marker-stripped) +/// string — recovers a valid leading call followed by trailing template noise +/// or a second concatenated fragment (e.g. `{"a":1}{"b":2}`). +/// +/// Restricting strategy 2 to a leading `Value::Object` keeps it from accepting a +/// bare number/string scraped out of surrounding noise as if it were the call's +/// arguments. Returns `None` when neither strategy yields valid object-shaped JSON, +/// so the caller still fails fast on genuinely malformed input. +fn recover_tool_arguments(raw: &str) -> Option { + let stripped = strip_tool_call_markers(raw); + let candidate = stripped.as_deref().unwrap_or(raw); + + // Strategy 1: the marker-stripped string parses cleanly on its own. + if stripped.is_some() + && let Ok(value) = serde_json::from_str::(candidate) + { + return Some(value); + } + + // Strategy 2: recover the first complete JSON value if it is an object. + let mut values = + serde_json::Deserializer::from_str(candidate.trim_start()).into_iter::(); + match values.next() { + Some(Ok(value @ Value::Object(_))) => Some(value), + _ => None, + } +} + +/// Removes any [`TOOL_CALL_TEMPLATE_MARKERS`] found in `raw` and trims the +/// result. Returns `Some(cleaned)` only when a marker was actually present and +/// the trimmed result is non-empty; otherwise `None` (nothing to strip). +fn strip_tool_call_markers(raw: &str) -> Option { + let mut cleaned = raw.to_string(); + let mut changed = false; + for &marker in TOOL_CALL_TEMPLATE_MARKERS { + if cleaned.contains(marker) { + cleaned = cleaned.replace(marker, ""); + changed = true; + } + } + if !changed { + return None; + } + let trimmed = cleaned.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) } /// Converts an OpenAI [`UsageWire`] into the harness-neutral [`Usage`]. diff --git a/src/harness/providers/openai/test.rs b/src/harness/providers/openai/test.rs index 3157f87..64e9d70 100644 --- a/src/harness/providers/openai/test.rs +++ b/src/harness/providers/openai/test.rs @@ -1263,3 +1263,122 @@ fn merge_system_is_noop_without_system_messages() { let input = vec![Message::user("just user")]; assert_eq!(merge_system_into_user(&input), input); } + +/// Builds an OpenAI-shaped non-streaming response body carrying a single tool +/// call whose `function.arguments` string is `raw` (verbatim, including any +/// corruption). Used to exercise `parse_tool_arguments` recovery/fail-fast. +fn tool_call_body(raw: &str) -> serde_json::Value { + json!({ + "id": "chatcmpl-toolargs", + "choices": [ + { + "message": { + "role": "assistant", + "tool_calls": [ + { + "id": "call-1", + "type": "function", + "function": { "name": "composio_execute", "arguments": raw } + } + ] + }, + "finish_reason": "tool_calls" + } + ] + }) +} + +#[test] +fn recovers_tool_args_with_leaked_trailing_template_marker() { + // Regression (openhuman#4766): an OpenAI-compatible gateway leaked a + // model's trailing `` chat-template delimiter into + // `function.arguments`. The JSON is otherwise valid, so stripping the + // marker must recover the call instead of failing the turn. + let response = parse_response(tool_call_body( + r#"{"tool":"GMAIL_CREATE_EMAIL_DRAFT","x":1}"#, + )) + .expect("leaked marker must be recovered"); + let calls = response.tool_calls(); + assert_eq!(calls.len(), 1); + assert_eq!( + calls[0].arguments, + json!({ "tool": "GMAIL_CREATE_EMAIL_DRAFT", "x": 1 }) + ); +} + +#[test] +fn recovers_tool_args_wrapped_in_tool_call_tags() { + // Some templates wrap the whole call in ``; both + // delimiters must be stripped before parsing. + let response = parse_response(tool_call_body(r#"{"q":"hi"}"#)) + .expect("wrapped args must be recovered"); + assert_eq!(response.tool_calls()[0].arguments, json!({ "q": "hi" })); +} + +#[test] +fn recovers_first_call_when_fragments_are_concatenated() { + // A leaked delimiter can also sit *between* two concatenated argument + // objects (e.g. an accumulator over-merge). After stripping the marker the + // string is `{...}{...}`; recovery keeps the first complete object. + let response = parse_response(tool_call_body( + r#"{"tool":"gmail"}{"tool":"other"}"#, + )) + .expect("leading call must be recovered"); + assert_eq!( + response.tool_calls()[0].arguments, + json!({ "tool": "gmail" }) + ); +} + +#[test] +fn still_fails_fast_on_genuinely_malformed_tool_args() { + // The exact corruption seen in the wild carries a stray `]` *inside* the + // JSON — not just a leaked delimiter — so it cannot be safely repaired. It + // must fail fast (a clear, non-retryable model error) rather than hang. + let err = parse_response(tool_call_body( + r#"{"arguments":{"body":"hi"]}}"#, + )) + .expect_err("unrepairable args must fail closed"); + assert!(matches!(err, TinyAgentsError::Model(_)), "got {err:?}"); + let message = err.to_string(); + assert!(message.contains("call-1"), "{message}"); + assert!(message.contains("raw arguments"), "{message}"); +} + +#[test] +fn valid_tool_args_containing_marker_substring_are_left_intact() { + // A legitimate arguments object whose *string value* contains the marker + // text parses cleanly on the first attempt, so the repair path never runs + // and the value is preserved verbatim. + let response = + parse_response(tool_call_body(r#"{"body":"use literally"}"#)).unwrap(); + assert_eq!( + response.tool_calls()[0].arguments, + json!({ "body": "use literally" }) + ); +} + +#[tokio::test] +async fn sse_stream_recovers_tool_args_with_leaked_template_marker() { + // The streaming reduce (`OpenAiStreamAcc::into_response`) shares + // `parse_tool_arguments`, so a leaked trailing marker across the terminal + // must reconstruct a usable call instead of finishing as a model error + // (which is what orphaned the parent's join/reduce in openhuman#4766). + let raw: Vec> = vec![ + b"data: {\"choices\":[{\"delta\":{\"tool_calls\":[{\"index\":0,\"id\":\"call-x\",\"function\":{\"name\":\"composio_execute\",\"arguments\":\"{\\\"q\\\":1}\"}}]},\"finish_reason\":\"tool_calls\"}]}\n\n".to_vec(), + b"data: [DONE]\n\n".to_vec(), + ]; + + let items = collect_sse(raw).await; + let mut merged = StreamAccumulator::new(); + for item in &items { + merged.push(item); + } + let response = merged + .finish() + .expect("stream must not fail on a recoverable marker"); + let calls = response.tool_calls(); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].name, "composio_execute"); + assert_eq!(calls[0].arguments, json!({ "q": 1 })); +}