diff --git a/crates/tinyagents-harness/src/agent_loop/mod_tests.rs b/crates/tinyagents-harness/src/agent_loop/mod_tests.rs index 61f801b0a..d8dc41532 100644 --- a/crates/tinyagents-harness/src/agent_loop/mod_tests.rs +++ b/crates/tinyagents-harness/src/agent_loop/mod_tests.rs @@ -3693,6 +3693,76 @@ async fn provider_invalid_arguments_recoverable_by_relaxed_json_are_repaired_and ); } +/// The host stack installs `ArgRecoveryMiddleware`, whose `before_tool` runs +/// before admission's relaxed-JSON repair. It used to coerce a provider-invalid +/// call's raw string to `{}` (the tool's schema is unknown to it here, so it +/// counts as permissive), which left repair nothing to fix and answered a +/// repairable call with the bare parse error. +#[tokio::test] +async fn arg_recovery_middleware_leaves_provider_invalid_arguments_for_repair() { + let tool = Arc::new(crate::testkit::FakeTool::returning("lookup", "found it")); + let mut harness: AgentHarness<()> = AgentHarness::new(); + harness.with_model_resolver(Arc::new(crate::host::FixedModelResolver::named( + "mock", + Arc::new(MockModel::with_responses(vec![ + invalid_tool_call_response("call-x", "lookup", "{query:\"weather\"}"), + text_response("found it", 1, 1), + ])), + ))); + harness.register_tool(tool.clone()); + harness.push_middleware(Arc::new(crate::middleware::ArgRecoveryMiddleware::new( + Vec::new(), + ))); + + let run = harness + .invoke_in_context( + &(), + RunContext::new(RunConfig::new("arg-recovery-repair"), ()), + vec![Message::user("lookup the weather")], + ) + .await + .expect("repaired arguments let the call execute"); + + assert_eq!(run.text().as_deref(), Some("found it")); + assert_eq!(tool.calls(), vec![json!({"query": "weather"})]); +} + +/// An unrepairable call is answered with the parse detail plus a corrective +/// the model can act on, not the bare serde error alone. +#[tokio::test] +async fn unrepairable_arguments_are_answered_with_an_actionable_error() { + let tool = Arc::new(crate::testkit::FakeTool::returning("lookup", "found it")); + let mut harness: AgentHarness<()> = AgentHarness::new(); + harness.with_model_resolver(Arc::new(crate::host::FixedModelResolver::named( + "mock", + Arc::new(MockModel::with_responses(vec![ + invalid_tool_call_response("call-x", "lookup", "{\"query\": [1 2]}"), + text_response("gave up", 1, 1), + ])), + ))); + harness.register_tool(tool.clone()); + + let run = harness + .invoke_in_context( + &(), + RunContext::new(RunConfig::new("unrepairable-args"), ()), + vec![Message::user("lookup")], + ) + .await + .expect("unparseable arguments do not fail the run"); + + assert!(tool.calls().is_empty()); + let transcript = format!("{:?}", run.messages); + assert!( + transcript.contains("invalid JSON arguments"), + "{transcript}" + ); + assert!( + transcript.contains("matches the tool's argument schema"), + "{transcript}" + ); +} + #[tokio::test] async fn structured_output_is_extracted() { let mut harness: AgentHarness<()> = AgentHarness::new(); diff --git a/crates/tinyagents-harness/src/agent_loop/tools.rs b/crates/tinyagents-harness/src/agent_loop/tools.rs index a34177577..4fae1acc6 100644 --- a/crates/tinyagents-harness/src/agent_loop/tools.rs +++ b/crates/tinyagents-harness/src/agent_loop/tools.rs @@ -869,7 +869,7 @@ impl AgentHarness { }); status.set_last_event(record.id); return Ok(ResolvedToolCall::Answered(tinytools::ToolResult::error( - detail, + unparseable_tool_call_message(&call.name, &detail), ))); } @@ -2342,6 +2342,21 @@ fn repeated_truncated_tool_call_message(tool_name: &str) -> String { ) } +/// The corrective for a call whose arguments the provider could not parse and +/// relaxed-JSON repair could not fix. A bare parse error ("expected `,` or `]` +/// at line 1 column 911") gave the model nothing to act on: production traces +/// show it re-sending the same large inline document until the failure breaker +/// halted the run. +fn unparseable_tool_call_message(tool_name: &str, detail: &str) -> String { + format!( + "Tool call `{tool_name}` was not run: its arguments are not valid JSON ({detail}). \ + Re-send it as well-formed JSON that matches the tool's argument schema. If the \ + arguments carry a large document, do not inline it again: build it in smaller \ + incremental calls (for example with an editing tool that returns a handle or id) and \ + pass that reference instead." + ) +} + /// Appends a batch's follow-up user messages (B2) after its last tool row, /// in the calls' original order. /// diff --git a/crates/tinyagents-harness/src/middleware/library/arg_recovery.rs b/crates/tinyagents-harness/src/middleware/library/arg_recovery.rs index 85e44a276..9850e2117 100644 --- a/crates/tinyagents-harness/src/middleware/library/arg_recovery.rs +++ b/crates/tinyagents-harness/src/middleware/library/arg_recovery.rs @@ -24,10 +24,12 @@ use tinytools::Tool; /// 1. Already a JSON object → leave it (the common, valid case). /// 2. A JSON-encoded string (optionally fenced) that decodes to an object → /// decode and use it. -/// 3. Otherwise a non-object whose tool schema declares **no** required fields → +/// 3. A call the provider marked invalid (`ToolCall::invalid`) keeps its raw +/// string, so admission can run its relaxed-JSON repair on it. +/// 4. Otherwise a non-object whose tool schema declares **no** required fields → /// coerce to `{}` (legacy-engine parity: the tool runs and produces its own /// recoverable error). -/// 4. Otherwise (non-object + schema has required fields) → leave the arguments +/// 5. Otherwise (non-object + schema has required fields) → leave the arguments /// untouched so the crate's `InvalidArgsPolicy::ReturnToolError` path reports /// the original validation failure. Coercing to `{}` would discard the raw /// malformed value without improving recovery. @@ -66,7 +68,9 @@ impl Middleware<(), C> for ArgRecoveryMiddleware { } // (2) JSON-encoded-string arguments (optionally markdown-fenced): decode - // and adopt the inner object. + // and adopt the inner object. A provider marks a fenced object invalid, + // so the decode also clears that mark; otherwise admission would answer + // the now-valid call with the stale parse error. if let Some(raw) = call.arguments.as_str() && let Some(obj) = recover_object_from_json_string(raw) { @@ -75,10 +79,23 @@ impl Middleware<(), C> for ArgRecoveryMiddleware { "[tinyagents::mw] arg_recovery: decoded JSON-encoded-string tool arguments to object" ); call.arguments = obj; + call.invalid = None; return Ok(()); } - // (3) Non-object with a permissive schema (no required fields): coerce to + // (3) A call the provider marked unparseable keeps its raw string: admission + // runs the relaxed-JSON repair on it next, and otherwise answers with the + // parse error. Coercing it to `{}` here destroyed the input that repair + // needs while the call was still answered as invalid. + if call.invalid.is_some() { + tracing::debug!( + tool = call.name.as_str(), + "[tinyagents::mw] arg_recovery: leaving provider-invalid tool arguments for admission repair" + ); + return Ok(()); + } + + // (4) Non-object with a permissive schema (no required fields): coerce to // `{}` so the tool runs and produces its own recoverable error — engine // parity for tools that predate the schema gate. let has_required = self @@ -94,7 +111,7 @@ impl Middleware<(), C> for ArgRecoveryMiddleware { return Ok(()); } - // (4) Non-object + schema has required fields: leave untouched. The + // (5) Non-object + schema has required fields: leave untouched. The // crate admission policy surfaces a descriptive, recoverable tool // result without executing the real tool. tracing::debug!( diff --git a/crates/tinyagents-harness/src/middleware/library/arg_recovery_tests.rs b/crates/tinyagents-harness/src/middleware/library/arg_recovery_tests.rs index 0d8df8306..a7b0a1f4d 100644 --- a/crates/tinyagents-harness/src/middleware/library/arg_recovery_tests.rs +++ b/crates/tinyagents-harness/src/middleware/library/arg_recovery_tests.rs @@ -97,3 +97,23 @@ async fn a_non_object_for_a_schema_with_required_fields_is_left_for_the_crate_ga async fn an_unknown_tool_is_treated_as_permissive() { assert_eq!(recover("nope", json!(3)).await, json!({})); } + +#[tokio::test] +async fn provider_invalid_arguments_keep_their_raw_string_for_admission_repair() { + let mut ctx: RunContext = RunContext::new(RunConfig::new("mw-test"), ()); + let raw = "{query:\"weather\"}"; + let mut call = ToolCall::invalid("c1", "loose", raw, "key must be a string"); + mw().before_tool(&mut ctx, &(), &mut call).await.unwrap(); + assert_eq!(call.arguments, json!(raw)); + assert!(call.invalid.is_some()); +} + +#[tokio::test] +async fn a_provider_invalid_fenced_object_is_decoded_and_cleared() { + let mut ctx: RunContext = RunContext::new(RunConfig::new("mw-test"), ()); + let raw = "```json\n{\"q\": \"x\"}\n```"; + let mut call = ToolCall::invalid("c1", "strict", raw, "expected value"); + mw().before_tool(&mut ctx, &(), &mut call).await.unwrap(); + assert_eq!(call.arguments, json!({"q": "x"})); + assert!(call.invalid.is_none()); +}