diff --git a/src/openhuman/agent/harness/parse.rs b/src/openhuman/agent/harness/parse.rs index 52b34afbc..9daf8b39c 100644 --- a/src/openhuman/agent/harness/parse.rs +++ b/src/openhuman/agent/harness/parse.rs @@ -26,7 +26,49 @@ pub(crate) fn parse_arguments_value(raw: Option<&serde_json::Value>) -> serde_js } } +/// Object keys that may carry the tool **arguments**, in priority order. +/// Models drift from the canonical `arguments` to `args`/`parameters`/etc.; +/// accepting these recovers an otherwise well-formed call (with a correct +/// `name`) instead of dropping it and burning an agent iteration +/// (bug-report-2026-05-26 A3). The tool **name** is deliberately left +/// strict — widening it would risk misreading a plain JSON answer as a +/// tool call in the whole-response parse path. +const TOOL_ARG_KEYS: &[&str] = &["arguments", "args", "parameters", "params", "input"]; + +/// Normalized arguments for the first present key among [`TOOL_ARG_KEYS`] +/// (via [`parse_arguments_value`], which tolerates both stringified and +/// object JSON). Empty-object default when none are present. +fn first_args_by_keys(obj: &serde_json::Value) -> serde_json::Value { + for key in TOOL_ARG_KEYS { + if let Some(v) = obj.get(*key) { + return parse_arguments_value(Some(v)); + } + } + parse_arguments_value(None) +} + pub(crate) fn parse_tool_call_value(value: &serde_json::Value) -> Option { + // Default to the permissive (tagged) behaviour: callers that reach a + // value through an explicit tool-call marker (`tool_calls` array, + // `` tags, ```tool_call blocks) accept the arg-key aliases. + parse_tool_call_value_aliased(value, true) +} + +/// Parse a single JSON value as a tool call. +/// +/// `allow_arg_aliases` controls whether the generic argument-key aliases in +/// [`TOOL_ARG_KEYS`] (notably the very generic `input`) are honoured for a +/// **bare** `{ "name": .., .. }` object. The whole-response fallback path +/// (`parse_tool_calls` on a top-level JSON object) passes `false`: there, a +/// normal model reply such as `{"name":"Alice","input":"hi"}` must not have +/// its `input` slurped into tool arguments and routed to execution +/// (bug-report-2026-05-26 A3 follow-up). The `function`-wrapped shape stays +/// permissive regardless — the `function` key is an unambiguous tool-call +/// marker. +fn parse_tool_call_value_aliased( + value: &serde_json::Value, + allow_arg_aliases: bool, +) -> Option { if let Some(function) = value.get("function") { let name = function .get("name") @@ -35,7 +77,7 @@ pub(crate) fn parse_tool_call_value(value: &serde_json::Value) -> Option Option`/``, `tool_calls` array, `function` + // wrapper) reach this fn with `allow_arg_aliases = true` and keep the + // permissive behaviour. + match value.get("arguments") { + Some(args) => parse_arguments_value(Some(args)), + None => return None, + } + }; Some(ParsedToolCall { name, arguments, @@ -64,11 +121,25 @@ pub(crate) fn parse_tool_call_value(value: &serde_json::Value) -> Option Vec { + // Tagged contexts (callers reach here via an explicit tool-call marker) + // accept the argument-key aliases. + parse_tool_calls_from_json_value_aliased(value, true) +} + +/// Like [`parse_tool_calls_from_json_value`], but lets the caller forbid +/// generic arg-key aliases on a **bare** singleton/array object. The +/// `tool_calls`-keyed envelope always stays permissive — that key is an +/// unambiguous tool-call marker even on the whole-response path. +pub(crate) fn parse_tool_calls_from_json_value_aliased( + value: &serde_json::Value, + allow_arg_aliases: bool, +) -> Vec { let mut calls = Vec::new(); if let Some(tool_calls) = value.get("tool_calls").and_then(|v| v.as_array()) { for call in tool_calls { - if let Some(parsed) = parse_tool_call_value(call) { + // `tool_calls` entries are explicitly tool-call shaped → widen. + if let Some(parsed) = parse_tool_call_value_aliased(call, true) { calls.push(parsed); } } @@ -80,14 +151,14 @@ pub(crate) fn parse_tool_calls_from_json_value(value: &serde_json::Value) -> Vec if let Some(array) = value.as_array() { for item in array { - if let Some(parsed) = parse_tool_call_value(item) { + if let Some(parsed) = parse_tool_call_value_aliased(item, allow_arg_aliases) { calls.push(parsed); } } return calls; } - if let Some(parsed) = parse_tool_call_value(value) { + if let Some(parsed) = parse_tool_call_value_aliased(value, allow_arg_aliases) { calls.push(parsed); } @@ -350,7 +421,12 @@ pub(crate) fn parse_tool_calls(response: &str) -> (String, Vec) // First, try to parse as OpenAI-style JSON response with tool_calls array // This handles providers like Minimax that return tool_calls in native JSON format if let Ok(json_value) = serde_json::from_str::(response.trim()) { - calls = parse_tool_calls_from_json_value(&json_value); + // Whole-response parse: a bare top-level object/array is NOT an + // explicit tool-call marker, so forbid the generic arg-key aliases + // here (a plain `{"name":..,"input":..}` answer must stay text). + // The `tool_calls`-keyed envelope is still honoured (it carries its + // own marker) — handled inside the `_aliased` helper. + calls = parse_tool_calls_from_json_value_aliased(&json_value, false); if !calls.is_empty() { // If we found tool_calls, extract any content field as text if let Some(content) = json_value.get("content").and_then(|v| v.as_str()) { @@ -388,7 +464,15 @@ pub(crate) fn parse_tool_calls(response: &str) -> (String, Vec) } if !parsed_any { - tracing::warn!("Malformed JSON: expected tool-call object in tag body"); + // body_chars only (never the body itself — it may carry tool + // arguments with user data). Stable `[agent_parse]` prefix so + // it aggregates with the other harness log families. Surfaces + // how often the model emits an unparseable tool-call tag + // (bug-report-2026-05-26 A3). + tracing::warn!( + body_chars = inner.chars().count(), + "[agent_parse] malformed JSON: expected tool-call object in tag body" + ); } remaining = &after_open[close_idx + close_tag.len()..]; diff --git a/src/openhuman/agent/harness/parse_tests.rs b/src/openhuman/agent/harness/parse_tests.rs index 4fd16f68d..c694b4fd6 100644 --- a/src/openhuman/agent/harness/parse_tests.rs +++ b/src/openhuman/agent/harness/parse_tests.rs @@ -69,6 +69,65 @@ fn parse_tool_call_value_supports_function_shape_flat_shape_and_invalid_names() assert!(parse_tool_call_value(&serde_json::json!({ "function": {} })).is_none()); } +#[test] +fn parse_tool_call_value_accepts_argument_key_aliases() { + // Correct name but the model used `args`/`parameters` instead of the + // canonical `arguments` — recover the call rather than drop it and burn + // an agent iteration (bug-report-2026-05-26 A3). + let with_args = serde_json::json!({ "name": "echo", "args": { "value": "hi" } }); + let parsed = parse_tool_call_value(&with_args).expect("args alias should parse"); + assert_eq!(parsed.name, "echo"); + assert_eq!(parsed.arguments, serde_json::json!({ "value": "hi" })); + + let with_parameters = serde_json::json!({ + "function": { "name": "shell", "parameters": "{\"command\":\"ls\"}" } + }); + let parsed = parse_tool_call_value(&with_parameters).expect("parameters alias should parse"); + assert_eq!(parsed.name, "shell"); + assert_eq!(parsed.arguments, serde_json::json!({ "command": "ls" })); + + // Name stays strict: an arg alias without a recognized name key is not + // a tool call (guards the whole-response JSON parse path). + assert!(parse_tool_call_value(&serde_json::json!({ "tool": "echo", "args": {} })).is_none()); +} + +#[test] +fn whole_response_singleton_ignores_generic_arg_aliases() { + // A plain JSON answer that happens to carry a `name` plus a generic, + // object-valued `input`. Tagged contexts widen `input` into arguments… + let answer = serde_json::json!({ "name": "Alice", "input": { "value": "hi" } }); + let tagged = parse_tool_calls_from_json_value(&answer); + assert_eq!(tagged.len(), 1); + assert_eq!(tagged[0].arguments, serde_json::json!({ "value": "hi" })); + + // …but the whole-response (bare singleton) path must treat this as plain + // text, not a tool call: it carries no canonical `arguments` marker, only + // a `name` that happens to match a tool (CodeRabbit, #2683). + let whole = parse_tool_calls_from_json_value_aliased(&answer, false); + assert!( + whole.is_empty(), + "bare whole-response object without canonical `arguments` must not dispatch a tool call" + ); + + // A bare object WITH the canonical `arguments` key is still recognized on + // the whole-response path — `arguments` is the explicit tool-call marker. + let bare_call = serde_json::json!({ "name": "echo", "arguments": { "value": "hi" } }); + let calls = parse_tool_calls_from_json_value_aliased(&bare_call, false); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].name, "echo"); + assert_eq!(calls[0].arguments, serde_json::json!({ "value": "hi" })); + + // The `tool_calls`-keyed envelope is an explicit marker and stays + // permissive even when aliases are forbidden for bare objects. + let envelope = serde_json::json!({ + "tool_calls": [ { "name": "echo", "input": { "value": "hi" } } ] + }); + let calls = parse_tool_calls_from_json_value_aliased(&envelope, false); + assert_eq!(calls.len(), 1); + assert_eq!(calls[0].name, "echo"); + assert_eq!(calls[0].arguments, serde_json::json!({ "value": "hi" })); +} + #[test] fn parse_tool_calls_from_json_value_handles_tool_calls_array_arrays_and_singletons() { let wrapped = serde_json::json!({ diff --git a/src/openhuman/agent/harness/session/turn.rs b/src/openhuman/agent/harness/session/turn.rs index 60d1b20d5..be21039c6 100644 --- a/src/openhuman/agent/harness/session/turn.rs +++ b/src/openhuman/agent/harness/session/turn.rs @@ -36,7 +36,7 @@ use crate::openhuman::context::prompt::{LearnedContextData, PromptContext, Promp use crate::openhuman::context::{ReductionOutcome, ARCHIVIST_EXTRACTION_PROMPT}; use crate::openhuman::inference::model_context::context_window_for_model; use crate::openhuman::inference::provider::{ - ChatMessage, ChatRequest, ConversationMessage, ProviderDelta, + ChatMessage, ChatRequest, ConversationMessage, ProviderDelta, UsageInfo, }; use crate::openhuman::memory::MemoryCategory; use crate::openhuman::tools::traits::ToolCallOptions; @@ -50,6 +50,41 @@ use anyhow::Result; use std::hash::{Hash, Hasher}; use std::sync::Arc; +/// Instruction appended (as a synthetic user turn) to the provider +/// messages when a turn hits the tool-call iteration cap. Asks the model +/// to wrap up with a resumable checkpoint instead of letting the turn die. +/// Native tools are disabled for this call so the model produces prose, +/// not yet another tool call. See bug-report-2026-05-26 A1. +const MAX_ITER_CHECKPOINT_INSTRUCTION: &str = "\ +You have reached the maximum number of tool calls allowed for this single turn, so you cannot call any more tools right now. \ +Do not attempt another tool call. Instead, write a short progress checkpoint for the user with two clearly labelled parts:\n\ +1. **Done so far** — what you have accomplished in this turn, grounded in the tool results above.\n\ +2. **Next steps** — exactly what you plan to do next.\n\ +Write it so you can pick up seamlessly where you left off when the user replies. Be concise."; + +/// Build a deterministic checkpoint summary from this turn's tool-call +/// records. Used only as a safety net when the model-written checkpoint +/// call fails or returns empty, so a capped turn can never be left without +/// a well-formed assistant message — which is what silently wedged the +/// thread before (bug-report-2026-05-26 A1). +fn build_deterministic_checkpoint(records: &[ToolCallRecord], max_iterations: usize) -> String { + let mut out = format!( + "I reached the tool-call limit for this turn ({max_iterations} steps), so I paused here.\n\n**Done so far:**\n" + ); + if records.is_empty() { + out.push_str("- (no tools completed yet)\n"); + } else { + for r in records { + let status = if r.success { "ok" } else { "failed" }; + out.push_str(&format!("- `{}` — {}\n", r.name, status)); + } + } + out.push_str( + "\n**Next steps:** I'll continue from here — just reply (e.g. \"continue\") and I'll pick up where I left off.", + ); + out +} + impl Agent { /// Executes a single interaction "turn" with the agent. /// @@ -443,6 +478,17 @@ impl Agent { // Collect tool call records across all iterations for post-turn hooks let mut all_tool_records: Vec = Vec::new(); + // Trim-robust digest of THIS turn's tool calls + results, compiled as + // the loop runs. Used as the *only* context for the max-iteration + // checkpoint summary, so it compiles "what I did this turn" without + // the prior conversation or system prompt bleeding in — and it's + // immune to history trimming (which drops/reorders from the front). + // The persisted transcript is unaffected (bug-report-2026-05-26 A1). + // Bounded: each entry truncates the result to 800 chars, so at the + // default 10-iteration cap the digest is ~8 KB — revisit if + // `max_tool_iterations` is raised substantially. + let mut turn_tool_digest = String::new(); + // Capture the last `Vec` sent to the provider so we // can persist it as a session transcript after the turn completes. let mut last_provider_messages: Option> = None; @@ -745,6 +791,21 @@ impl Agent { } else { text }; + // Defense-in-depth (bug-report-2026-05-26 A1): a + // completion with no text *and* no tool calls is never a + // valid final answer — it's a degenerate/poisoned + // response. Surfacing it as an error is visible; the old + // behaviour returned `Ok("")`, which rendered as a blank + // reply and silently wedged the thread. + if final_text.trim().is_empty() { + log::warn!( + "[agent_loop] provider returned an empty final response (i={}, no text, no tool calls) — surfacing as error instead of a silent blank reply", + iteration + 1 + ); + return Err(anyhow::anyhow!( + "The model returned an empty response. Please try again." + )); + } log::info!( "[agent] no tool calls — returning final response after {} iteration(s)", iteration + 1 @@ -910,6 +971,14 @@ impl Agent { r.name, truncate_with_ellipsis(&r.output, 300) ); + // Record this call in the turn digest (output truncated to + // bound size) for a possible max-iteration checkpoint. + turn_tool_digest.push_str(&format!( + "- {} [{}]: {}\n", + r.name, + if r.success { "ok" } else { "failed" }, + truncate_with_ellipsis(&r.output, 800) + )); } log::info!( "[agent] all tools complete for iteration {} — looping back to provider", @@ -940,26 +1009,136 @@ impl Agent { ); } + // Tool-call iteration cap reached. Instead of aborting the turn + // — which left the persisted transcript on an unterminated tool + // cycle and silently wedged the thread on the next message + // (bug-report-2026-05-26 A1) — emit a *resumable checkpoint*: + // ask the model (tools disabled) to summarize what it did and + // what comes next, persist that as the final assistant message, + // and return it. The full tool-call history stays in the + // transcript, so the user's next message naturally resumes the + // task — no heuristic "continue" detection needed. log::warn!( - "[agent] exceeded max tool iterations ({}) — aborting turn", + "[agent_loop] reached max tool iterations max={} — emitting resumable checkpoint instead of aborting", self.config.max_tool_iterations ); - log::warn!( - "[agent_loop] exceeded maximum tool iterations max={}", - self.config.max_tool_iterations + + let base_messages = last_provider_messages + .clone() + .unwrap_or_else(|| self.tool_dispatcher.to_provider_messages(&self.history)); + // Summarize ONLY this turn's work: feed the compiled tool-call + // digest (no system prompt, no prior conversation), not the full + // conversation. `base_messages` above is still used for the + // transcript persist below, so the saved transcript is unchanged + // (bug-report-2026-05-26 A1). `user_message` below is the + // `turn(&mut self, message: &str)` parameter (the turn's request). + let turn_summary_input = vec![ChatMessage::user(format!( + "You were working on this user request:\n{user_message}\n\nHere are the tool calls you made this turn and their results — compile your checkpoint from these:\n{}", + if turn_tool_digest.is_empty() { + "(no tool calls recorded)" + } else { + turn_tool_digest.as_str() + } + ))]; + let checkpoint_iteration = (self.config.max_tool_iterations + 1) as u32; + let (mut checkpoint, checkpoint_usage) = self + .summarize_iteration_checkpoint( + &turn_summary_input, + &effective_model, + checkpoint_iteration, + ) + .await; + + // Fold the checkpoint call's usage into the turn's cumulative + // accounting. The provider call happens regardless of whether we + // keep its prose, so dropping its tokens would undercount the + // turn and mis-attribute the prior iteration's usage to the + // checkpoint message (mirrors the normal final-response path). + if let Some(ref usage) = checkpoint_usage { + self.context.record_usage(usage); + cumulative_input_tokens += usage.input_tokens; + cumulative_output_tokens += usage.output_tokens; + cumulative_cached_input_tokens += usage.cached_input_tokens; + cumulative_charged_usd += usage.charged_amount_usd; + last_turn_usage = Some(transcript::TurnUsage { + model: effective_model.clone(), + usage: transcript::MessageUsage { + input: usage.input_tokens, + output: usage.output_tokens, + cached_input: usage.cached_input_tokens, + cost_usd: usage.charged_amount_usd, + }, + ts: chrono::Utc::now().to_rfc3339(), + }); + } else { + // No usage on the checkpoint call: don't attribute a stale + // prior-iteration snapshot to the checkpoint assistant message. + last_turn_usage = None; + } + + if checkpoint.trim().is_empty() { + log::warn!("[agent_loop] checkpoint summary empty — using deterministic fallback"); + checkpoint = build_deterministic_checkpoint( + &all_tool_records, + self.config.max_tool_iterations, + ); + } + log::info!( + "[agent_loop] max-iter checkpoint emitted chars={}", + checkpoint.chars().count() ); - // Return the typed `AgentError::MaxIterationsExceeded` variant - // (boxed through `anyhow::Error`) so `Agent::run_single` can - // downcast at the Sentry funnel and suppress emission — this is - // a deterministic agent-state outcome, not a bug (OPENHUMAN- - // TAURI-99 / -98). The `Display` text is preserved verbatim so - // the user-visible chat-rendered "Error: Agent exceeded - // maximum tool iterations" string is unchanged. - Err(anyhow::Error::new( - crate::openhuman::agent::error::AgentError::MaxIterationsExceeded { - max: self.config.max_tool_iterations, - }, - )) + + self.emit_progress(AgentProgress::TurnCompleted { + iterations: self.config.max_tool_iterations as u32, + }) + .await; + + self.history + .push(ConversationMessage::Chat(ChatMessage::assistant( + checkpoint.clone(), + ))); + self.trim_history(); + + // Persist the checkpoint so the transcript ends on a + // well-formed assistant message (never a dangling tool cycle). + // Note: `base_messages` ends before the final (capped) iteration's + // tool results — those landed after the last `last_provider_messages` + // snapshot — so the persisted transcript omits them. That's fine: + // the checkpoint prose covers the work done, and the transcript + // stays structurally correct (ends on an assistant message). + let mut checkpoint_messages = base_messages; + checkpoint_messages.push(ChatMessage::assistant(checkpoint.clone())); + self.persist_session_transcript( + &checkpoint_messages, + cumulative_input_tokens, + cumulative_output_tokens, + cumulative_cached_input_tokens, + cumulative_charged_usd, + last_turn_usage.as_ref(), + ); + + self.context.record_tool_calls(all_tool_records.len()); + + // Fire post-turn hooks with the checkpoint as the assistant + // response (mirrors the normal final-response path). + if !self.post_turn_hooks.is_empty() { + let ctx = TurnContext { + user_message: user_message.to_string(), + assistant_response: checkpoint.clone(), + tool_calls: all_tool_records, + turn_duration_ms: turn_started.elapsed().as_millis() as u64, + session_id: Some(self.event_session_id.clone()) + .filter(|session_id| !session_id.trim().is_empty()), + agent_id: Some(self.agent_definition_id.clone()) + .filter(|agent_id| !agent_id.trim().is_empty()), + entrypoint: Some(self.event_channel.clone()) + .filter(|entrypoint| !entrypoint.trim().is_empty()), + iteration_count: self.config.max_tool_iterations, + }; + hooks::fire_hooks(&self.post_turn_hooks, ctx); + } + + Ok(checkpoint) }; // end of `turn_body` async block // Run the turn body inside the parent-execution-context scope so @@ -1875,6 +2054,104 @@ impl Agent { } } + /// Ask the provider for a resumable checkpoint summary when a turn + /// hits the tool-call iteration cap, with native tools **disabled** so + /// the model returns prose rather than another tool call. Streams text + /// deltas to the progress sink (when attached) so the checkpoint + /// appears in the UI like any other reply. + /// + /// Returns the summary text (empty when the provider call fails or + /// yields nothing — the caller then falls back to + /// [`build_deterministic_checkpoint`] so the thread is never left on an + /// unterminated tool cycle, bug-report-2026-05-26 A1) **paired with the + /// provider usage** for this extra call, so the caller can fold it into + /// the turn's cumulative token/cost accounting instead of silently + /// dropping it. + async fn summarize_iteration_checkpoint( + &self, + base_messages: &[ChatMessage], + effective_model: &str, + iteration_for_stream: u32, + ) -> (String, Option) { + let mut messages = base_messages.to_vec(); + messages.push(ChatMessage::user(MAX_ITER_CHECKPOINT_INSTRUCTION)); + + // Mirror the main loop's streaming sink so the checkpoint renders + // incrementally. Only text deltas are relevant here (tools are + // disabled for this call). + let (delta_tx_opt, delta_forwarder) = if self.on_progress.is_some() { + let (tx, mut rx) = tokio::sync::mpsc::channel::(128); + let progress_tx = self.on_progress.clone(); + let forwarder = tokio::spawn(async move { + while let Some(event) = rx.recv().await { + let Some(ref sink) = progress_tx else { + continue; + }; + if let ProviderDelta::TextDelta { delta } = event { + if sink + .send(AgentProgress::TextDelta { + delta, + iteration: iteration_for_stream, + }) + .await + .is_err() + { + break; + } + } + } + }); + (Some(tx), Some(forwarder)) + } else { + (None, None) + }; + + let result = self + .provider + .chat( + ChatRequest { + messages: &messages, + tools: None, + stream: delta_tx_opt.as_ref(), + }, + effective_model, + self.temperature, + ) + .await; + drop(delta_tx_opt); + if let Some(handle) = delta_forwarder { + let _ = handle.await; + } + + match result { + Ok(resp) => { + let usage = resp.usage.clone(); + // Strip any stray tool-call XML a text-mode model may have + // emitted; keep only the prose. + let (text, calls) = self.tool_dispatcher.parse_response(&resp); + let checkpoint = if !text.trim().is_empty() { + text + } else if calls.is_empty() { + // No tool-call markup was present, so the raw text (if + // any) is genuine prose — safe to use. + resp.text.unwrap_or_default() + } else { + // `parse_response` stripped tool-call markup and left no + // prose. Do NOT re-emit `resp.text` here: it would persist + // the raw `…` markup verbatim as the checkpoint. + // Return empty so the caller uses the deterministic + // fallback instead (bug-report-2026-05-26 A1). + String::new() + }; + (checkpoint, usage) + } + Err(e) => { + log::warn!("[agent_loop] checkpoint summary call failed: {e:#}"); + (String::new(), None) + } + } + } + /// Persist the exact provider messages as a session transcript. /// /// Writes JSONL as source of truth and re-renders the companion `.md` diff --git a/src/openhuman/agent/harness/session/turn_tests.rs b/src/openhuman/agent/harness/session/turn_tests.rs index b3e81dd17..5ce997174 100644 --- a/src/openhuman/agent/harness/session/turn_tests.rs +++ b/src/openhuman/agent/harness/session/turn_tests.rs @@ -7,7 +7,7 @@ use crate::openhuman::agent::tool_policy::{ GeneratedToolRuntimeContext, GeneratedToolRuntimeRisk, ToolPolicy, ToolPolicyDecision, ToolPolicyRequest, }; -use crate::openhuman::inference::provider::{ChatRequest, ChatResponse, Provider}; +use crate::openhuman::inference::provider::{ChatRequest, ChatResponse, Provider, UsageInfo}; use crate::openhuman::memory::Memory; use crate::openhuman::tools::ToolResult; use crate::openhuman::tools::{PermissionLevel, Tool}; @@ -829,13 +829,28 @@ async fn turn_uses_cached_transcript_prefix_on_first_iteration() { } #[tokio::test] -async fn turn_errors_when_max_tool_iterations_are_exceeded() { +async fn turn_emits_checkpoint_when_max_tool_iterations_are_exceeded() { + // First response forces a tool call (consuming the single allowed + // iteration); the second is the model-written checkpoint the harness + // requests (tools disabled) once the cap is hit. The turn must NOT + // error anymore — it returns a resumable checkpoint so the thread stays + // well-formed and the user can continue on their next message + // (bug-report-2026-05-26 A1). let provider: Arc = Arc::new(SequenceProvider { - responses: AsyncMutex::new(vec![Ok(ChatResponse { - text: Some("{\"name\":\"echo\",\"arguments\":{}}".into()), - tool_calls: vec![], - usage: None, - })]), + responses: AsyncMutex::new(vec![ + Ok(ChatResponse { + text: Some("{\"name\":\"echo\",\"arguments\":{}}".into()), + tool_calls: vec![], + usage: None, + }), + Ok(ChatResponse { + text: Some( + "**Done so far:** ran echo.\n**Next steps:** I'll continue from here.".into(), + ), + tool_calls: vec![], + usage: None, + }), + ]), requests: AsyncMutex::new(Vec::new()), }); let mut agent = make_agent_with_builder( @@ -852,17 +867,179 @@ async fn turn_errors_when_max_tool_iterations_are_exceeded() { crate::openhuman::config::ContextConfig::default(), ); - let err = agent + let reply = agent .turn("hello") .await - .expect_err("turn should stop at configured iteration budget"); - assert!(err - .to_string() - .contains("Agent exceeded maximum tool iterations (1)")); + .expect("turn should emit a checkpoint at the iteration cap, not error"); + assert!( + reply.contains("Next steps"), + "checkpoint should summarize next steps, got: {reply}" + ); + // The tool-call history from the capped iteration is preserved... assert!(agent.history.iter().any(|message| matches!( message, ConversationMessage::AssistantToolCalls { tool_calls, .. } if tool_calls.len() == 1 ))); + // ...and the transcript ends on a well-formed assistant message (the + // checkpoint), never a dangling tool cycle — this is what stops the + // next message from silently wedging the thread. + assert!( + matches!( + agent.history.last(), + Some(ConversationMessage::Chat(msg)) + if msg.role == "assistant" && msg.content.contains("Next steps") + ), + "history should end on the assistant checkpoint, got: {:?}", + agent.history.last() + ); +} + +#[tokio::test] +async fn turn_errors_on_empty_provider_response() { + // A completion with no text and no tool calls is never a valid final + // answer — surface it as an error instead of accepting a blank reply, + // which previously rendered as silence and wedged the thread + // (bug-report-2026-05-26 A1, defect B). + let provider: Arc = Arc::new(SequenceProvider { + responses: AsyncMutex::new(vec![Ok(ChatResponse { + text: Some(String::new()), + tool_calls: vec![], + usage: None, + })]), + requests: AsyncMutex::new(Vec::new()), + }); + let mut agent = make_agent_with_builder( + provider, + vec![], + Box::new(FixedMemoryLoader { + context: String::new(), + }), + vec![], + crate::openhuman::config::AgentConfig::default(), + crate::openhuman::config::ContextConfig::default(), + ); + + let err = agent + .turn("hello") + .await + .expect_err("an empty provider response should surface as an error"); + assert!( + err.to_string().contains("empty response"), + "expected an empty-response error, got: {err}" + ); +} + +#[tokio::test] +async fn turn_checkpoint_falls_back_to_deterministic_summary_when_model_summary_empty() { + // Tool call consumes the single iteration; the checkpoint request then + // comes back empty. The harness must fall back to a deterministic + // done/next summary so the turn never returns blank — the safety net + // that guarantees the thread can't re-wedge (bug-report-2026-05-26 A1). + let provider: Arc = Arc::new(SequenceProvider { + responses: AsyncMutex::new(vec![ + Ok(ChatResponse { + text: Some("{\"name\":\"echo\",\"arguments\":{}}".into()), + tool_calls: vec![], + usage: None, + }), + Ok(ChatResponse { + text: Some(String::new()), + tool_calls: vec![], + usage: None, + }), + ]), + requests: AsyncMutex::new(Vec::new()), + }); + let mut agent = make_agent_with_builder( + provider, + vec![Box::new(EchoTool)], + Box::new(FixedMemoryLoader { + context: String::new(), + }), + vec![], + crate::openhuman::config::AgentConfig { + max_tool_iterations: 1, + ..crate::openhuman::config::AgentConfig::default() + }, + crate::openhuman::config::ContextConfig::default(), + ); + + let reply = agent + .turn("hello") + .await + .expect("empty model checkpoint should fall back, not error"); + assert!( + reply.contains("tool-call limit"), + "deterministic fallback summary expected, got: {reply}" + ); + assert!( + reply.contains("echo"), + "fallback should list the tool that ran, got: {reply}" + ); +} + +#[tokio::test] +async fn turn_checkpoint_usage_is_folded_into_transcript_accounting() { + // The extra checkpoint provider call costs tokens; those must land in + // the persisted transcript's cumulative accounting rather than being + // silently dropped (CodeRabbit review on bug-report-2026-05-26 A1). + let provider: Arc = Arc::new(SequenceProvider { + responses: AsyncMutex::new(vec![ + // Tool iteration — provider reports no usage. + Ok(ChatResponse { + text: Some("{\"name\":\"echo\",\"arguments\":{}}".into()), + tool_calls: vec![], + usage: None, + }), + // Checkpoint call — reports usage that must be accounted for. + Ok(ChatResponse { + text: Some("**Done so far:** ran echo.\n**Next steps:** continue.".into()), + tool_calls: vec![], + usage: Some(UsageInfo { + input_tokens: 11, + output_tokens: 4, + cached_input_tokens: 2, + charged_amount_usd: 0.05, + ..UsageInfo::default() + }), + }), + ]), + requests: AsyncMutex::new(Vec::new()), + }); + let mut agent = make_agent_with_builder( + provider, + vec![Box::new(EchoTool)], + Box::new(FixedMemoryLoader { + context: String::new(), + }), + vec![], + crate::openhuman::config::AgentConfig { + max_tool_iterations: 1, + ..crate::openhuman::config::AgentConfig::default() + }, + crate::openhuman::config::ContextConfig::default(), + ); + + agent + .turn("hello") + .await + .expect("turn should emit a checkpoint at the iteration cap"); + + let transcript = transcript::read_transcript( + agent + .session_transcript_path + .as_ref() + .expect("checkpoint turn should persist a transcript"), + ) + .expect("transcript should be readable"); + // Only the checkpoint call reported usage, so the turn totals must equal + // exactly its numbers — proof the extra call is accounted for, not lost. + assert_eq!( + transcript.meta.input_tokens, 11, + "checkpoint input tokens should be folded into the turn total" + ); + assert_eq!(transcript.meta.output_tokens, 4); + assert_eq!(transcript.meta.cached_input_tokens, 2); } #[tokio::test] diff --git a/src/openhuman/agent/harness/subagent_runner/ops.rs b/src/openhuman/agent/harness/subagent_runner/ops.rs index ff1a1781f..5614c1743 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops.rs @@ -34,6 +34,7 @@ use crate::openhuman::context::prompt::{ use crate::openhuman::inference::provider::{ChatMessage, ChatRequest, Provider, ToolCall}; use crate::openhuman::memory_conversations::ConversationMessage; use crate::openhuman::tools::{Tool, ToolCategory, ToolSpec}; +use crate::openhuman::util::truncate_with_ellipsis; /// Prompt suffix injected into every typed sub-agent run. /// @@ -199,7 +200,7 @@ struct LazyToolkitResolver { impl LazyToolkitResolver { fn resolve(&self, name: &str) -> Option> { - let action = self.actions.iter().find(|a| a.name == name)?; + let action = self.find_action(name)?; Some(Box::new( crate::openhuman::composio::ComposioActionTool::new( self.config.clone(), @@ -210,6 +211,63 @@ impl LazyToolkitResolver { )) } + /// Match a model-supplied tool name to a real toolkit action, tolerant + /// of the near-miss slugs models routinely emit — case differences and + /// separator/prefix drift (bug-report-2026-05-26 A2). Tries, in order: + /// exact, case-insensitive, then a normalized alphanumeric match + /// (accepted only when **unique**, so a fabricated slug can't silently + /// resolve to the wrong action — those still fall through to the + /// "tool not available" error, which lists `known_slugs` for the model + /// to self-correct). + fn find_action( + &self, + name: &str, + ) -> Option<&crate::openhuman::context::prompt::ConnectedIntegrationTool> { + if let Some(action) = self.actions.iter().find(|a| a.name == name) { + return Some(action); + } + if let Some(action) = self + .actions + .iter() + .find(|a| a.name.eq_ignore_ascii_case(name)) + { + tracing::debug!( + requested = %name, + matched = %action.name, + "[subagent_runner] resolved tool by case-insensitive match" + ); + return Some(action); + } + let norm = normalize_slug(name); + if !norm.is_empty() { + let mut matches = self + .actions + .iter() + .filter(|a| normalize_slug(&a.name) == norm); + if let Some(action) = matches.next() { + if matches.next().is_none() { + tracing::info!( + requested = %name, + matched = %action.name, + "[subagent_runner] resolved tool by normalized-slug match" + ); + return Some(action); + } + // Ambiguous: 2+ actions normalize to the same slug (e.g. + // `read_file` and `ReadFile` → `readfile`). We deliberately + // refuse to guess. Warn (not debug): a slug collision is a + // toolkit configuration anomaly that should surface in normal + // operator logs, not stay hidden behind debug filtering. + tracing::warn!( + requested = %name, + norm = %norm, + "[subagent_runner] ambiguous normalized-slug match — multiple actions resolve to the same slug; not resolving" + ); + } + } + None + } + /// Slugs from the bound toolkit, for inclusion in unknown-tool /// errors so the model can self-correct without burning a turn. fn known_slugs(&self) -> Vec<&str> { @@ -217,6 +275,17 @@ impl LazyToolkitResolver { } } +/// Lowercased, non-alphanumerics stripped — collapses separator/prefix +/// drift (`GOOGLESLIDES_BATCH_UPDATE` vs `googleslides_batch_update`) so +/// near-miss tool slugs still resolve, while genuinely different slugs +/// (e.g. a hallucinated `GMAIL_GET_LAST_3_MESSAGES`) stay distinct. +fn normalize_slug(s: &str) -> String { + s.chars() + .filter(|c| c.is_ascii_alphanumeric()) + .map(|c| c.to_ascii_lowercase()) + .collect() +} + /// Run a sub-agent based on its definition and a task prompt. /// /// This is the primary entry point for agent delegation. It performs the following: @@ -1161,6 +1230,12 @@ async fn run_inner_loop( ) -> Result<(String, usize, AggregatedUsage), SubagentRunError> { let max_iterations = max_iterations.max(1); + // Compiled digest of this sub-agent run's tool calls + results, for a + // graceful checkpoint if it hits the iteration cap (mirrors the main + // agent — bug-report-2026-05-26 A1). Accumulated as the loop runs so it's + // robust to history trimming. + let mut run_tool_digest = String::new(); + // Sub-agent transcript stem — mirrors what // `persist_subagent_transcript` used to compute on one-shot // post-loop writes. We compute it once up front so **every @@ -1694,6 +1769,15 @@ async fn run_inner_loop( let call_output_chars = result_text.chars().count(); let call_elapsed_ms = call_started.elapsed().as_millis() as u64; + // Record this call in the run digest (output truncated to bound + // size) for a possible max-iteration checkpoint. + run_tool_digest.push_str(&format!( + "- {} [{}]: {}\n", + call.name, + if call_success { "ok" } else { "failed" }, + truncate_with_ellipsis(&result_text, 800) + )); + // Repeated-failure circuit breaker (shared guard). `call.arguments` // is the stable signature; on a trip we stash the root-cause summary // and bail after this iteration's tool results are recorded. @@ -1785,7 +1869,70 @@ async fn run_inner_loop( } } - Err(SubagentRunError::MaxIterationsExceeded(max_iterations)) + // Iteration cap reached. Instead of erroring — which discards all of the + // sub-agent's partial work (the parent just sees "delegate failed") — + // compile a graceful checkpoint of what it accomplished and return it as + // the result, so the calling agent can continue from the partial progress + // (mirrors the main-agent checkpoint — bug-report-2026-05-26 A1). + let digest = if run_tool_digest.is_empty() { + "(no tool calls completed)" + } else { + run_tool_digest.as_str() + }; + let deterministic = format!( + "I reached my tool-call limit ({max_iterations} steps) before finishing this task. \ + Progress so far (tool calls + results):\n{digest}\n\nThe task is incomplete — the above is \ + what I accomplished; continue from here." + ); + let summary_input = vec![ChatMessage::user(format!( + "You are sub-agent `{agent_id}` and reached your tool-call limit before finishing. Here are \ + the tool calls you made and their results — compile a brief progress checkpoint (what you \ + accomplished, what still remains) for the agent that delegated to you. Do not call tools.\n\n{digest}" + ))]; + let checkpoint = match provider + .chat( + ChatRequest { + messages: &summary_input, + tools: None, + stream: None, + }, + model, + temperature, + ) + .await + { + Ok(resp) => { + if let Some(ref u) = resp.usage { + usage.input_tokens += u.input_tokens; + usage.output_tokens += u.output_tokens; + usage.cached_input_tokens += u.cached_input_tokens; + usage.charged_amount_usd += u.charged_amount_usd; + } + // Strip any stray tool-call markup a text-mode model emits; if no + // prose survives, fall back to the deterministic digest. + let raw = resp.text.unwrap_or_default(); + let (prose, _) = super::super::parse::parse_tool_calls(&raw); + if prose.trim().is_empty() { + deterministic + } else { + prose + } + } + Err(e) => { + tracing::warn!( + agent_id = %agent_id, + task_id = %task_id, + error = %e, + "[subagent_runner] checkpoint summary call failed — using deterministic fallback" + ); + deterministic + } + }; + // NB: unlike the main-agent path, this checkpoint is intentionally NOT + // written to a sub-agent transcript — the calling agent's transcript + // captures the delegated result, so there's no data loss. Don't "fix" + // this by adding a `persist_subagent_transcript` call. + Ok((checkpoint, max_iterations, usage)) } fn parse_tool_arguments(arguments: &str) -> serde_json::Value { diff --git a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs index 809cfd244..cdef0a455 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs @@ -1,6 +1,44 @@ use super::*; use crate::openhuman::agent::harness::definition::{ModelSpec, ToolScope}; +#[test] +fn lazy_resolver_tolerates_near_miss_slugs() { + use crate::openhuman::context::prompt::ConnectedIntegrationTool; + let mk = |name: &str| ConnectedIntegrationTool { + name: name.into(), + description: "d".into(), + parameters: None, + }; + let resolver = LazyToolkitResolver { + config: std::sync::Arc::new(crate::openhuman::config::Config::default()), + actions: vec![mk("GOOGLESLIDES_BATCH_UPDATE"), mk("GMAIL_LIST_MESSAGES")], + }; + // Exact, case-insensitive, and separator/prefix drift all resolve + // (bug-report-2026-05-26 A2). + assert!(resolver.resolve("GMAIL_LIST_MESSAGES").is_some()); + assert!(resolver.resolve("gmail_list_messages").is_some()); + assert!(resolver.resolve("googleslides_batch_update").is_some()); + // A fabricated slug stays unresolved → routed to the "available tools" + // error so the model self-corrects, not silently mis-dispatched. + assert!(resolver.resolve("GMAIL_GET_LAST_3_MESSAGES").is_none()); +} + +#[test] +fn normalize_slug_collapses_separators_and_case() { + assert_eq!( + normalize_slug("GOOGLESLIDES_BATCH_UPDATE"), + "googleslidesbatchupdate" + ); + assert_eq!( + normalize_slug("googleslides_batch_update"), + "googleslidesbatchupdate" + ); + assert_ne!( + normalize_slug("GMAIL_GET_LAST_3_MESSAGES"), + normalize_slug("GMAIL_LIST_MESSAGES") + ); +} + fn make_def_named_tools(names: &[&str]) -> AgentDefinition { AgentDefinition { id: "test".into(), @@ -597,6 +635,68 @@ async fn runner_errors_outside_parent_context() { assert!(matches!(result, Err(SubagentRunError::NoParentContext))); } +#[tokio::test] +async fn subagent_emits_checkpoint_at_iteration_cap_instead_of_erroring() { + // A sub-agent that keeps calling tools and never finishes must hit its + // cap and return a graceful partial-progress checkpoint (Ok), not a bare + // MaxIterationsExceeded that discards its work — so the delegating agent + // can continue from what it got (bug-report-2026-05-26 A1, mirrors the + // main agent). Two tool rounds (max_iterations=2), then the summarize + // call returns prose which becomes the checkpoint. + let provider = ScriptedProvider::new(vec![ + tool_response("file_read", "{}"), + tool_response("file_read", "{}"), + text_response("Progress so far: read the file. Remaining: keep going."), + ]); + let parent = make_parent(provider.clone(), vec![stub("file_read")]); + let mut def = make_def_named_tools(&["file_read"]); + def.max_iterations = 2; + + let outcome = with_parent_context(parent, async { + run_subagent(&def, "keep reading forever", SubagentRunOptions::default()).await + }) + .await + .expect("hitting the iteration cap should return a checkpoint, not error"); + + assert!( + outcome.output.contains("Progress so far"), + "expected the model-written checkpoint, got: {}", + outcome.output + ); +} + +#[tokio::test] +async fn subagent_checkpoint_falls_back_to_deterministic_when_summary_empty() { + // Same cap, but the summarize call yields nothing (response queue + // exhausted → empty). The runner must fall back to a deterministic + // partial-progress digest so the parent still gets a usable result + // (bug-report-2026-05-26 A1). + let provider = ScriptedProvider::new(vec![ + tool_response("file_read", "{}"), + tool_response("file_read", "{}"), + ]); + let parent = make_parent(provider.clone(), vec![stub("file_read")]); + let mut def = make_def_named_tools(&["file_read"]); + def.max_iterations = 2; + + let outcome = with_parent_context(parent, async { + run_subagent(&def, "keep reading forever", SubagentRunOptions::default()).await + }) + .await + .expect("empty summary should fall back, not error"); + + assert!( + outcome.output.contains("tool-call limit"), + "expected the deterministic fallback checkpoint, got: {}", + outcome.output + ); + assert!( + outcome.output.contains("file_read"), + "deterministic checkpoint should list the tool work done, got: {}", + outcome.output + ); +} + #[tokio::test] async fn runner_allows_spawn_at_max_depth() { let provider = ScriptedProvider::new(vec![text_response("ok")]); diff --git a/src/openhuman/agent/tests.rs b/src/openhuman/agent/tests.rs index 8e0970d00..c83570fb3 100644 --- a/src/openhuman/agent/tests.rs +++ b/src/openhuman/agent/tests.rs @@ -437,12 +437,18 @@ async fn turn_handles_multi_step_tool_chain() { } // ═══════════════════════════════════════════════════════════════════════════ -// 4. Max-iteration bailout +// 4. Max-iteration checkpoint (resumable, not a hard bailout) // ═══════════════════════════════════════════════════════════════════════════ #[tokio::test] -async fn turn_bails_out_at_max_iterations() { - // Create more tool calls than max_tool_iterations allows. +async fn turn_emits_checkpoint_at_max_iterations() { + // Create more tool calls than max_tool_iterations allows. Hitting the + // cap must NOT error anymore: the harness emits a resumable checkpoint + // and returns it Ok, so the transcript ends on a well-formed assistant + // message instead of a dangling tool cycle that wedges the next turn + // (bug-report-2026-05-26 A1). Every scripted response here is a tool + // call, so the checkpoint summary call also yields no prose and the + // deterministic fallback summary is used. let max_iters = 3; let mut responses = Vec::new(); for i in 0..max_iters + 5 { @@ -462,12 +468,24 @@ async fn turn_bails_out_at_max_iterations() { let (mut agent, _tmp) = build_agent_with_config(provider, vec![Box::new(EchoTool)], config); - let result = agent.turn("infinite loop").await; - assert!(result.is_err()); - let err = result.unwrap_err().to_string(); + let reply = agent + .turn("infinite loop") + .await + .expect("hitting the iteration cap should return a checkpoint, not error"); assert!( - err.contains("maximum tool iterations"), - "Expected max iterations error, got: {err}" + reply.contains("tool-call limit") && reply.contains("Next steps"), + "Expected a resumable checkpoint summary, got: {reply}" + ); + // The transcript ends on the assistant checkpoint (well-formed), which + // is what lets the user's next message resume the task cleanly. + assert!( + matches!( + agent.history().last(), + Some(ConversationMessage::Chat(msg)) + if msg.role == "assistant" && msg.content.contains("Next steps") + ), + "history should end on the assistant checkpoint, got: {:?}", + agent.history().last() ); } @@ -714,7 +732,11 @@ async fn xml_dispatcher_does_not_send_tool_specs() { // ═══════════════════════════════════════════════════════════════════════════ #[tokio::test] -async fn turn_handles_empty_text_response() { +async fn turn_errors_on_empty_text_response() { + // A completion with no text *and* no tool calls is never a valid final + // answer. The old behaviour returned `Ok("")`, which rendered as a blank + // reply and silently wedged the thread; now it surfaces as a visible + // error the user can retry on (bug-report-2026-05-26 A1). let provider = Box::new(ScriptedProvider::new(vec![ChatResponse { text: Some(String::new()), tool_calls: vec![], @@ -723,12 +745,18 @@ async fn turn_handles_empty_text_response() { let (mut agent, _tmp) = build_agent_with(provider, vec![], Box::new(NativeToolDispatcher)); - let response = agent.turn("hi").await.unwrap(); - assert!(response.is_empty()); + let err = agent + .turn("hi") + .await + .expect_err("an empty provider response should surface as an error"); + assert!( + err.to_string().contains("empty response"), + "expected an empty-response error, got: {err}" + ); } #[tokio::test] -async fn turn_handles_none_text_response() { +async fn turn_errors_on_none_text_response() { let provider = Box::new(ScriptedProvider::new(vec![ChatResponse { text: None, tool_calls: vec![], @@ -737,9 +765,14 @@ async fn turn_handles_none_text_response() { let (mut agent, _tmp) = build_agent_with(provider, vec![], Box::new(NativeToolDispatcher)); - // Should not panic — falls back to empty string - let response = agent.turn("hi").await.unwrap(); - assert!(response.is_empty()); + let err = agent + .turn("hi") + .await + .expect_err("a null-text provider response should surface as an error"); + assert!( + err.to_string().contains("empty response"), + "expected an empty-response error, got: {err}" + ); } // ═══════════════════════════════════════════════════════════════════════════