From f10819d340f6cdf62c8040a9cf7818eff38db749 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Fri, 3 Jul 2026 22:34:08 +0000 Subject: [PATCH] =?UTF-8?q?wip(agent):=20#4451=20partial=20=E2=80=94=20fat?= =?UTF-8?q?al=20tool-arg=20validation=20->=20model-visible=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvaged from the parity workflow worktree after the subagent hit the monthly spend limit before verifying/opening a PR. Seam changes (middleware.rs, mod.rs) + e2e test scaffold in tests/agent_harness_e2e.rs, plus a vendored tinyagents ValidationPolicy addition (submodule bump). NOT build-verified. Preserved so the work is not lost. Ref: tinyhumansai/openhuman#4451 Claude-Session: https://claude.ai/code/session_019j5TLsRLHsM3kqAYFyH4hR --- src/openhuman/tinyagents/middleware.rs | 204 +++++++++++++++++++++++-- src/openhuman/tinyagents/mod.rs | 29 +++- tests/agent_harness_e2e.rs | 174 +++++++++++++++++++++ vendor/tinyagents | 2 +- 4 files changed, 392 insertions(+), 17 deletions(-) diff --git a/src/openhuman/tinyagents/middleware.rs b/src/openhuman/tinyagents/middleware.rs index b0a0537067..4f37749cdd 100644 --- a/src/openhuman/tinyagents/middleware.rs +++ b/src/openhuman/tinyagents/middleware.rs @@ -1439,14 +1439,70 @@ impl Middleware<()> for ToolOutcomeCaptureMiddleware { } } -/// `before_tool`: coerce a tool call's arguments to an empty object when they -/// are not a JSON object (issue #4249). A model can emit malformed native -/// arguments (invalid JSON, or a bare scalar/array); the model adapter parses -/// those to `Value::Null`, which the harness then rejects against an object -/// schema and aborts the whole turn. The in-house engine recovered such a call by -/// running the tool with `{}`; restore that so a single bad tool call is -/// recoverable rather than fatal. -pub(crate) struct ArgRecoveryMiddleware; +/// `before_tool`: best-effort recovery of malformed tool-call arguments before +/// the harness schema-validates them (issues #4249, #4451). +/// +/// A model routinely emits arguments the native adapter can't parse cleanly: +/// invalid JSON (parsed to `Value::Null`), a JSON *string* that itself encodes +/// the real object (double-encoding), or that object wrapped in a Markdown code +/// fence. This middleware recovers what it can **without fabricating data**: +/// +/// 1. If the arguments are a string, strip a surrounding ```` ```json ```` fence +/// and re-parse — recovering the intended object when the model +/// double-encoded or fenced it. +/// 2. If the arguments are still not an object *and the tool's schema declares +/// no required fields*, coerce to `{}` (the legacy convenience that lets a +/// no-argument tool run when the model sends `null`). +/// 3. Otherwise leave the arguments untouched. Under +/// [`ValidationPolicy::ReturnToolError`](tinyagents::harness::runtime::ValidationPolicy) +/// the harness turns the schema violation into a descriptive, model-visible +/// tool error and continues the loop, so the model self-corrects on the next +/// turn — mirroring the legacy engine, which surfaced argument errors as +/// recoverable tool results rather than fatal aborts. +/// +/// The prior behavior — blindly coercing *every* non-object to `{}` — actively +/// destroyed recoverable intent and, for the common required-field tool, +/// guaranteed a `" is required"` result the model then had to repair +/// blind. Recovering the real object (step 1) usually passes validation outright +/// with no extra round-trip. +pub(crate) struct ArgRecoveryMiddleware { + /// Snapshot of each registered tool's parameters JSON-Schema, keyed by tool + /// name. Captured at harness-assembly time so the `before_tool` hook (which + /// only receives the call) can consult `required` without the registry. + schemas: HashMap, +} + +impl ArgRecoveryMiddleware { + /// Builds the middleware from a name → parameters-schema snapshot. + pub(crate) fn new(schemas: HashMap) -> Self { + Self { schemas } + } + + /// Returns `true` when the tool's schema declares a non-empty `required` + /// array. Absent schema / no `required` → `false` (treat as no-required, so + /// a `null`-arg call to an optional-arg tool still recovers to `{}`). + fn schema_has_required(&self, tool: &str) -> bool { + self.schemas + .get(tool) + .and_then(|schema| schema.get("required")) + .and_then(serde_json::Value::as_array) + .is_some_and(|required| !required.is_empty()) + } +} + +/// Attempts to recover a JSON value the model encoded as a *string*: strips a +/// surrounding Markdown code fence (```` ```json … ``` ````) and parses the +/// remainder. Returns `Some(value)` only when the string parses to valid JSON. +fn recover_json_encoded_arguments(raw: &str) -> Option { + let mut text = raw.trim(); + if let Some(stripped) = text.strip_prefix("```") { + // Drop an optional language tag on the opening fence line, then the + // closing fence. + let after_tag = stripped.split_once('\n').map(|(_, rest)| rest).unwrap_or(""); + text = after_tag.trim().strip_suffix("```").unwrap_or(after_tag).trim(); + } + serde_json::from_str::(text).ok() +} #[async_trait] impl Middleware<()> for ArgRecoveryMiddleware { @@ -1460,10 +1516,40 @@ impl Middleware<()> for ArgRecoveryMiddleware { _state: &(), call: &mut TaToolCall, ) -> TaResult<()> { - if !call.arguments.is_object() { + if call.arguments.is_object() { + return Ok(()); + } + + // Step 1: recover a JSON-encoded-string / fenced payload into its real value. + if let Some(raw) = call.arguments.as_str() { + if let Some(recovered) = recover_json_encoded_arguments(raw) { + tracing::debug!( + tool = call.name.as_str(), + recovered_object = recovered.is_object(), + "[tinyagents::mw::arg_recovery] recovered JSON-encoded string tool arguments" + ); + call.arguments = recovered; + if call.arguments.is_object() { + return Ok(()); + } + } + } + + // Step 2: still not an object. Only coerce to `{}` when the tool declares + // no required fields — otherwise `{}` would mask the real error and the + // model would repair blind. Leave required-field tools to the harness' + // `ValidationPolicy::ReturnToolError` recoverable-error path (step 3). + if self.schema_has_required(&call.name) { tracing::debug!( tool = call.name.as_str(), - "[tinyagents::mw] recovering non-object tool arguments to {{}}" + "[tinyagents::mw::arg_recovery] non-object arguments left intact for required-field \ + tool; deferring to schema-validation tool-error recovery" + ); + } else { + tracing::debug!( + tool = call.name.as_str(), + "[tinyagents::mw::arg_recovery] coercing non-object arguments to {{}} for \ + no-required-field tool" ); call.arguments = serde_json::json!({}); } @@ -2445,4 +2531,102 @@ mod tests { second.content ); } + + // ── ArgRecoveryMiddleware (#4451) ───────────────────────────────────────── + + fn required_schema(tool: &str) -> HashMap { + let mut m = HashMap::new(); + m.insert( + tool.to_string(), + json!({ + "type": "object", + "required": ["query"], + "properties": { "query": { "type": "string" } } + }), + ); + m + } + + fn optional_schema(tool: &str) -> HashMap { + let mut m = HashMap::new(); + m.insert( + tool.to_string(), + json!({ "type": "object", "properties": { "q": { "type": "string" } } }), + ); + m + } + + async fn recover( + schemas: HashMap, + args: serde_json::Value, + ) -> serde_json::Value { + let mw = ArgRecoveryMiddleware::new(schemas); + let mut call = TaToolCall { + id: "c1".into(), + name: "query_memory".into(), + arguments: args, + }; + mw.before_tool(&mut ctx(), &(), &mut call).await.unwrap(); + call.arguments + } + + #[test] + fn recover_json_encoded_string_parses_object() { + let v = recover_json_encoded_arguments(r#"{"query":"hi"}"#).expect("parses"); + assert_eq!(v, json!({ "query": "hi" })); + } + + #[test] + fn recover_json_encoded_string_strips_markdown_fence() { + let fenced = "```json\n{\"query\":\"hi\"}\n```"; + let v = recover_json_encoded_arguments(fenced).expect("parses fenced"); + assert_eq!(v, json!({ "query": "hi" })); + } + + #[test] + fn recover_json_encoded_string_rejects_non_json() { + assert!(recover_json_encoded_arguments("not json at all").is_none()); + } + + #[tokio::test] + async fn arg_recovery_parses_double_encoded_object_args() { + // The model emitted the whole arguments object as a JSON *string*. + let out = recover(required_schema("query_memory"), json!("{\"query\":\"hi\"}")).await; + assert_eq!( + out, + json!({ "query": "hi" }), + "a JSON-encoded-string payload should be recovered to its real object" + ); + } + + #[tokio::test] + async fn arg_recovery_leaves_unrecoverable_non_object_intact_for_required_tool() { + // A bare scalar with a required-field schema must NOT be coerced to `{}` + // (that would mask the real error); it is left for the harness' + // ValidationPolicy::ReturnToolError recoverable-error path. + let out = recover(required_schema("query_memory"), json!(null)).await; + assert_eq!( + out, + json!(null), + "non-object args for a required-field tool must be left intact" + ); + } + + #[tokio::test] + async fn arg_recovery_coerces_non_object_to_empty_for_optional_tool() { + // A no-required-field tool keeps the legacy convenience: `null` → `{}` + // so the tool still runs without an extra self-correction round-trip. + let out = recover(optional_schema("query_memory"), json!(null)).await; + assert_eq!( + out, + json!({}), + "non-object args for a no-required tool should coerce to an empty object" + ); + } + + #[tokio::test] + async fn arg_recovery_is_noop_for_valid_object_args() { + let out = recover(required_schema("query_memory"), json!({ "query": "hi" })).await; + assert_eq!(out, json!({ "query": "hi" })); + } } diff --git a/src/openhuman/tinyagents/mod.rs b/src/openhuman/tinyagents/mod.rs index 474bf8d697..7189c5ce98 100644 --- a/src/openhuman/tinyagents/mod.rs +++ b/src/openhuman/tinyagents/mod.rs @@ -50,7 +50,7 @@ use tinyagents::harness::middleware::{ ToolPolicyMiddleware as TaToolPolicyMiddleware, }; use tinyagents::harness::model::CapabilitySet; -use tinyagents::harness::runtime::{AgentHarness, RunPolicy, UnknownToolPolicy}; +use tinyagents::harness::runtime::{AgentHarness, RunPolicy, UnknownToolPolicy, ValidationPolicy}; use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; use tinyagents::harness::store::StoreRegistry; use tinyagents::harness::summarization::TrimStrategy; @@ -157,6 +157,12 @@ fn run_policy_for(max_iterations: usize, response_cache_enabled: bool) -> RunPol policy.limits.max_depth = MAX_SPAWN_DEPTH; policy.retry.max_attempts = 1; policy.unknown_tool = UnknownToolPolicy::ReturnToolError; + // Schema-validation failures on tool arguments (missing required field, + // wrong type, bad enum) become recoverable, model-visible tool errors rather + // than fatal turn aborts — the legacy engine surfaced argument errors as + // recoverable tool results and self-corrected on the next iteration (#4451). + // Bounded by `max_tool_calls` above, exactly like the unknown-tool path. + policy.validation = ValidationPolicy::ReturnToolError; // Prompt-prefix protection is always on (issue #4249, 03.2): the // `PromptCacheGuardMiddleware` records a `CacheLayoutEvent` whenever volatile // content busts the provider KV-cache prefix. Purely diagnostic — never @@ -1389,11 +1395,22 @@ fn assemble_turn_harness( ))); } - // Malformed-argument recovery (`before_tool`): coerce a call's non-object - // arguments (invalid JSON parses to Null) to `{}` so a single bad tool call is - // recoverable — the harness would otherwise reject it against an object schema - // and abort the whole turn. Engine parity. - harness.push_middleware(Arc::new(middleware::ArgRecoveryMiddleware)); + // Malformed-argument recovery (`before_tool`): recover a call's non-object + // arguments where possible (JSON-encoded-string / Markdown-fenced payloads), + // and coerce to `{}` only for no-required-field tools. Required-field tools + // with unrecoverable arguments fall through to the harness' + // `ValidationPolicy::ReturnToolError` path (set in `run_policy_for`), which + // surfaces a descriptive tool error the model self-corrects on — instead of + // aborting the whole turn (#4451). Needs the tool schemas to read `required`. + let arg_recovery_schemas: std::collections::HashMap = harness + .tools() + .schemas() + .into_iter() + .map(|schema| (schema.name, schema.parameters)) + .collect(); + harness.push_middleware(Arc::new(middleware::ArgRecoveryMiddleware::new( + arg_recovery_schemas, + ))); AssembledTurnHarness { harness, diff --git a/tests/agent_harness_e2e.rs b/tests/agent_harness_e2e.rs index 31faf17ae3..e93c66f0ff 100644 --- a/tests/agent_harness_e2e.rs +++ b/tests/agent_harness_e2e.rs @@ -3070,3 +3070,177 @@ async fn provider_sse_tool_args_accumulation() { server.abort(); } + +// ─── #4451: malformed tool-arg schema validation is recoverable, not fatal ──── +// +// The tinyagents harness schema-validates a model-supplied tool call before +// execution. Historically a violation (missing required field, wrong type, bad +// enum) returned `TinyAgentsError::Validation` out of the loop and failed the +// *entire* turn with a `chat_error` — a routine model behavior the legacy engine +// absorbed by surfacing argument errors as recoverable tool results. +// +// The fix wires `ValidationPolicy::ReturnToolError` in `run_policy_for` +// (src/openhuman/tinyagents/mod.rs): a schema violation is now injected into the +// transcript as a descriptive, model-visible tool error and the loop continues, +// so the model self-corrects. These two tests exercise the acceptance criteria +// end-to-end over the real RPC/SSE stack: a scripted provider emits one +// malformed tool call then a corrected response; the turn must reach `chat_done` +// and never emit `chat_error`. + +/// Chat path: the orchestrator emits a `resolve_time` call missing its required +/// `expr` field (wrong field name), then a corrected text reply. The malformed +/// call must NOT abort the turn — it becomes a recoverable tool error and the +/// turn reaches `chat_done`. +#[test] +fn malformed_tool_call_recovers_on_chat_path() { + run_on_agent_stack( + "malformed_tool_call_recovers_on_chat_path", + malformed_tool_call_recovers_on_chat_path_inner, + ); +} + +async fn malformed_tool_call_recovers_on_chat_path_inner() { + let _lock = env_lock(); + reset_script(vec![ + // request[0]: orchestrator calls resolve_time with the WRONG field name + // (`expression` instead of the required `expr`). Under the old fatal gate + // this aborted the whole turn; now it is a recoverable tool error. + tool_call_completion("resolve_time", json!({ "expression": "24h ago" })), + // request[1]: having read the schema-error tool result, the model corrects + // itself and returns a final answer. + text_completion("RECOVERED_ARG_CANARY: corrected after the schema error."), + ]); + let stack = boot_stack().await; + + let mut events = spawn_sse_collector(format!( + "{}/events?client_id=harness-argrecover", + stack.rpc_base + )); + send_web_chat( + &stack.rpc_base, + 900, + "harness-argrecover", + "thread-argrecover", + "what time was it a day ago?", + ) + .await; + + let done = wait_for_terminal(&mut events, Duration::from_secs(60)).await; + // Acceptance: the malformed call did not convert into a turn-level chat_error. + assert_eq!( + done.get("event").and_then(Value::as_str), + Some("chat_done"), + "malformed tool call must be recoverable, not a chat_error: {done}" + ); + let full_response = done + .get("full_response") + .and_then(Value::as_str) + .unwrap_or_else(|| panic!("chat_done missing 'full_response': {done}")); + assert!( + full_response.contains("RECOVERED_ARG_CANARY"), + "final response missing recovery canary; full_response: {full_response}\nevent: {done}" + ); + + // The second upstream request must carry a descriptive schema-error tool + // result so the model could self-correct — the exact recoverable-error text + // the fix injects (agent_loop `ValidationPolicy::ReturnToolError`). + let requests = with_captured(|c| c.clone()); + assert!( + requests.len() >= 2, + "expected ≥2 upstream requests (malformed call + corrected turn), got {};\nrequests: {}", + requests.len(), + serde_json::to_string_pretty(&requests).unwrap_or_default() + ); + let second_serialized = serde_json::to_string(requests.get(1).unwrap()).unwrap_or_default(); + assert!( + second_serialized.contains("invalid arguments for `resolve_time`"), + "corrected turn's upstream request must include the injected schema-error tool result; \ + request[1]: {}", + serde_json::to_string_pretty(requests.get(1).unwrap()).unwrap_or_default() + ); + + stack.shutdown(); +} + +/// Subagent path: the researcher subagent's inner loop emits a `web_search_tool` +/// call with `{"q": …}` instead of the required `query` (the exact scenario from +/// the issue), then corrected canary text. The malformed inner call must not +/// kill the subagent run; delegation completes and the turn reaches `chat_done`. +#[test] +fn malformed_tool_call_recovers_on_subagent_path() { + run_on_agent_stack( + "malformed_tool_call_recovers_on_subagent_path", + malformed_tool_call_recovers_on_subagent_path_inner, + ); +} + +async fn malformed_tool_call_recovers_on_subagent_path_inner() { + let _lock = env_lock(); + reset_script(vec![ + // request[0]: orchestrator delegates to the researcher via `research`. + tool_call_completion("research", json!({ "prompt": "find the marker" })), + // request[1]: researcher inner loop emits web_search_tool with the WRONG + // field name (`q` instead of the required `query`). Under the old fatal + // gate this killed the whole subagent run; now it is recoverable. + tool_call_completion("web_search_tool", json!({ "q": "marker phrase" })), + // request[2]: researcher reads the schema-error tool result and returns + // its corrected canary as text. + text_completion("SUBAGENT_RECOVERED_CANARY is the marker."), + // request[3]: orchestrator synthesizes the researcher result. + text_completion("Done: SUBAGENT_RECOVERED_CANARY"), + ]); + let stack = boot_stack().await; + + let mut events = spawn_sse_collector(format!( + "{}/events?client_id=harness-subargrecover", + stack.rpc_base + )); + send_web_chat( + &stack.rpc_base, + 910, + "harness-subargrecover", + "thread-subargrecover", + "research the marker", + ) + .await; + + let done = wait_for_terminal(&mut events, Duration::from_secs(120)).await; + // Acceptance: the malformed inner call did not convert into a chat_error on + // the subagent path. + assert_eq!( + done.get("event").and_then(Value::as_str), + Some("chat_done"), + "malformed subagent tool call must be recoverable, not a chat_error: {done}" + ); + let full_response = done + .get("full_response") + .and_then(Value::as_str) + .unwrap_or_else(|| panic!("chat_done missing 'full_response': {done}")); + assert!( + full_response.contains("SUBAGENT_RECOVERED_CANARY"), + "final response missing subagent recovery canary; full_response: {full_response}\n\ + event: {done}" + ); + + // ≥4 upstream requests prove the subagent inner loop recovered rather than + // aborting: orchestrator → researcher (malformed) → researcher (corrected) → + // orchestrator synthesis. The researcher's recovered turn must carry the + // injected schema-error tool result. + let requests = with_captured(|c| c.clone()); + assert!( + requests.len() >= 4, + "expected ≥4 upstream requests (orchestrator + researcher x2 + synthesis), got {};\ + \nrequests: {}", + requests.len(), + serde_json::to_string_pretty(&requests).unwrap_or_default() + ); + let all_serialized = serde_json::to_string(&requests).unwrap_or_default(); + assert!( + all_serialized.contains("invalid arguments for `web_search_tool`"), + "a recovered researcher turn must include the injected schema-error tool result; \ + requests: {}", + serde_json::to_string_pretty(&requests).unwrap_or_default() + ); + + stack.shutdown(); +} diff --git a/vendor/tinyagents b/vendor/tinyagents index a9500184b3..8f43d885ce 160000 --- a/vendor/tinyagents +++ b/vendor/tinyagents @@ -1 +1 @@ -Subproject commit a9500184b3d6e87e43019e757d4ca622a418b9d9 +Subproject commit 8f43d885ceddc92ce09b722257f46b06096e74eb