From 9198444ffd710b3105baeeacd84fbc9738b97100 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Tue, 23 Jun 2026 00:46:59 +0530 Subject: [PATCH] fix(agent): halt on first permanent inference failure to stop agent cascade (#3104) (#3779) Co-authored-by: Steven Enamakel --- src/openhuman/agent/harness/engine/core.rs | 70 ++- .../agent/harness/engine/core_tests.rs | 349 +++++++++++++ src/openhuman/agent/harness/tool_loop.rs | 149 ++++++ .../agent/harness/tool_loop_tests.rs | 460 ++++++++++++++++++ 4 files changed, 1025 insertions(+), 3 deletions(-) diff --git a/src/openhuman/agent/harness/engine/core.rs b/src/openhuman/agent/harness/engine/core.rs index 0309ef077..b413348e4 100644 --- a/src/openhuman/agent/harness/engine/core.rs +++ b/src/openhuman/agent/harness/engine/core.rs @@ -824,6 +824,17 @@ pub(crate) async fn run_turn_engine( "[agent_loop] circuit breaker tripped — halting with root cause" ); halt_reason = Some(reason); + // Stop executing the rest of this assistant message's tool-call + // batch (#3104). Native-tool providers can emit multiple tool + // calls in one message; without this break the loop would drain + // the remaining calls — and on a permanent inference failure + // (out of budget / provider-config) that means launching the + // *next* paid sub-agent delegation right after the first one + // proved the wall is unrecoverable. Breaking here makes the + // "halt on the first occurrence" guarantee hold for batched + // calls too. The tool results recorded so far are still threaded + // into history below, so the caller keeps full context. + break; } // Early-exit when a sub-agent calls ask_user_clarification: @@ -840,6 +851,55 @@ pub(crate) async fn run_turn_engine( } } + // A circuit-breaker / early-exit `break` can stop the batch before every + // tool call ran, so `individual_results` (one entry per EXECUTED call) + // may be shorter than `native_tool_calls` (every call the model emitted). + // The persisted assistant message must reference ONLY the executed calls: + // a native-mode assistant turn carrying N `tool_call` ids followed by + // fewer than N `role: tool` results is rejected by OpenAI-compatible + // providers ("an assistant message with tool_calls must be followed by + // tool messages responding to each tool_call_id") on the next request — + // exactly the raw `ChatMessage` histories used by run_tool_call_loop / + // the sub-agent paths (Codex review #3779). Trim the persisted tool-call + // list to the executed prefix so call-ids and tool-results stay in + // lockstep. `tool_calls` is a 1:1, same-order map of `native_tool_calls` + // (see `parse_structured_tool_calls`), so the executed prefix is simply + // the first `individual_results.len()` native calls. + let executed = individual_results.len(); + let executed_native_calls = &native_tool_calls[..executed.min(native_tool_calls.len())]; + // The parsed list is a 1:1, same-order map of the native list, so the + // executed prefix lines up. Trim it too: the typed-history observer + // (`turn_engine_adapter::persisted_tool_calls`) builds the `Agent::turn` + // `AssistantToolCalls` entry from these, and would otherwise persist a + // tool-call for every emitted call while only collecting results for the + // executed prefix — the same orphaned-id mismatch in the raw-ChatMessage + // path (Codex review #3779). + let executed_parsed_calls = &tool_calls[..executed.min(tool_calls.len())]; + let assistant_history_content = if executed < native_tool_calls.len() { + tracing::debug!( + iteration, + emitted = native_tool_calls.len(), + executed, + "[agent_loop] batch truncated before all tool calls ran — trimming \ + persisted assistant tool-calls to the executed prefix so tool_call_ids \ + match tool-results (no orphaned id)" + ); + // Rebuild from the executed prefix. Empty prefix (a break before the + // first call could ever produce one) degrades to the plain + // response-text assistant message, mirroring the no-tool-call path. + if executed_native_calls.is_empty() { + response_text.clone() + } else { + build_native_assistant_history( + &response_text, + reasoning_content.as_deref(), + executed_native_calls, + ) + } + } else { + assistant_history_content + }; + // Add assistant message with tool calls + tool results to history. // Native mode: JSON-structured messages so convert_messages() can // reconstruct OpenAI-format tool_calls + tool result messages. Prompt @@ -850,8 +910,8 @@ pub(crate) async fn run_turn_engine( &display_text, &response_text, reasoning_content.as_deref(), - &native_tool_calls, - &tool_calls, + executed_native_calls, + executed_parsed_calls, iteration, false, ) @@ -861,7 +921,11 @@ pub(crate) async fn run_turn_engine( observer.on_results_batch(&content, iteration); history.push(ChatMessage::user(content)); } else { - for (native_call, result) in native_tool_calls.iter().zip(individual_results.iter()) { + // Zip over the executed prefix only — one `role: tool` result per + // executed `tool_call_id`, matching the trimmed assistant message + // above so the next provider request has no orphaned tool-call id. + for (native_call, result) in executed_native_calls.iter().zip(individual_results.iter()) + { let tool_msg = serde_json::json!({ "tool_call_id": native_call.id, "content": result, diff --git a/src/openhuman/agent/harness/engine/core_tests.rs b/src/openhuman/agent/harness/engine/core_tests.rs index d4a5b4116..ada3e56d4 100644 --- a/src/openhuman/agent/harness/engine/core_tests.rs +++ b/src/openhuman/agent/harness/engine/core_tests.rs @@ -262,6 +262,355 @@ async fn engine_autocompacts_history_when_guard_trips() { ); } +// -- #3104 / Codex #3779 Finding B: batch-break leaves no orphaned tool-call id - + +/// Provider that emits a single assistant response carrying MULTIPLE native tool +/// calls in one message (the native-mode batch the reviewer flagged), then a +/// plain final text on any later call so the loop can terminate. +struct BatchToolCallProvider { + served: Mutex, + calls: Vec, +} + +impl BatchToolCallProvider { + fn new(calls: Vec) -> Arc { + Arc::new(Self { + served: Mutex::new(false), + calls, + }) + } +} + +#[async_trait] +impl Provider for BatchToolCallProvider { + async fn chat_with_system( + &self, + _system: Option<&str>, + _message: &str, + _model: &str, + _temperature: f64, + ) -> anyhow::Result { + Ok("noop".into()) + } + + async fn chat( + &self, + _request: ChatRequest<'_>, + _model: &str, + _temperature: f64, + ) -> anyhow::Result { + let mut served = self.served.lock().unwrap(); + if *served { + // Loop should have already halted on the first batch; this is only a + // safety net so the engine can never block waiting for more input. + return Ok(ChatResponse { + text: Some("FINAL".into()), + tool_calls: vec![], + usage: None, + reasoning_content: None, + }); + } + *served = true; + Ok(ChatResponse { + text: Some(String::new()), + tool_calls: self.calls.clone(), + usage: None, + reasoning_content: None, + }) + } + + fn supports_native_tools(&self) -> bool { + true + } + + async fn effective_context_window(&self, _model: &str) -> Option { + None + } +} + +/// Tool source whose FIRST executed call returns a terminal (budget-exhausted) +/// delegated-inference failure carrying the sub-agent dispatch wrapper, so the +/// shared `RepeatFailureGuard` halts on the first occurrence and the batch +/// breaks before the remaining call(s) run. Any later call would succeed — but +/// must never be reached. +struct FailFirstToolSource { + specs: Vec, + executed: Vec, +} + +#[async_trait] +impl ToolSource for FailFirstToolSource { + fn request_specs(&self) -> &[crate::openhuman::tools::ToolSpec] { + &self.specs + } + + async fn execute_call( + &mut self, + call: &ParsedToolCall, + _iteration: usize, + _progress: &dyn super::ProgressReporter, + _progress_call_id: &str, + ) -> ToolRunResult { + self.executed.push(call.name.clone()); + if self.executed.len() == 1 { + // Sub-agent dispatch wrapper (`failed and did not complete`) + a + // budget body → terminal inference failure → halt on first failure. + ToolRunResult { + text: "run_code failed and did not complete — no work was performed. \ + Error: {\"error\":\"insufficient balance — add credits\"}" + .into(), + success: false, + } + } else { + ToolRunResult { + text: "ok".into(), + success: true, + } + } + } +} + +#[allow(clippy::too_many_arguments)] +async fn run_with_source( + provider: &dyn Provider, + history: &mut Vec, + tool_source: &mut dyn ToolSource, +) -> TurnEngineOutcome { + let progress = NullProgress; + let mut observer = NullObserver; + let checkpoint = ErrorCheckpoint; + let parser = DefaultParser; + let multimodal = MultimodalConfig::default(); + let multimodal_files = MultimodalFileConfig::default(); + + run_turn_engine( + provider, + history, + tool_source, + &progress, + &mut observer, + &checkpoint, + &parser, + "test-provider", + "ctx-test-model-xyz", + 0.0, + true, + &multimodal, + &multimodal_files, + 8, + None, + &[], + None, + None, + ) + .await + .expect("turn engine should complete") +} + +/// Collect the `tool_call` ids referenced by an assistant message's native-mode +/// JSON content (`{"content":…,"tool_calls":[{"id":…}, …]}`). Returns empty for +/// non-native / non-tool-call assistant messages. +fn assistant_tool_call_ids(content: &str) -> Vec { + serde_json::from_str::(content) + .ok() + .and_then(|v| { + v.get("tool_calls").and_then(|tc| tc.as_array()).map(|arr| { + arr.iter() + .filter_map(|c| c.get("id").and_then(|i| i.as_str()).map(str::to_string)) + .collect() + }) + }) + .unwrap_or_default() +} + +/// Collect the `tool_call_id`s of every `role: tool` result message in history. +fn tool_result_ids(history: &[ChatMessage]) -> Vec { + history + .iter() + .filter(|m| m.role == "tool") + .filter_map(|m| { + serde_json::from_str::(&m.content) + .ok() + .and_then(|v| { + v.get("tool_call_id") + .and_then(|i| i.as_str()) + .map(str::to_string) + }) + }) + .collect() +} + +#[tokio::test] +async fn batch_break_trims_assistant_tool_calls_to_executed_no_orphan_id() { + // The model emits THREE native tool calls in one message. The first returns a + // terminal budget failure → the shared guard halts on the first occurrence + // and the batch breaks, so calls #2 and #3 never run. The persisted assistant + // message must reference ONLY the executed call id, and there must be exactly + // one matching `role: tool` result — no orphaned tool-call id that an + // OpenAI-compatible provider would reject on the next request. + let provider = BatchToolCallProvider::new(vec![ + ToolCall { + id: "call-A".into(), + name: "run_code".into(), + arguments: "{\"prompt\":\"a\"}".into(), + extra_content: None, + }, + ToolCall { + id: "call-B".into(), + name: "run_code".into(), + arguments: "{\"prompt\":\"b\"}".into(), + extra_content: None, + }, + ToolCall { + id: "call-C".into(), + name: "run_code".into(), + arguments: "{\"prompt\":\"c\"}".into(), + extra_content: None, + }, + ]); + let mut tool_source = FailFirstToolSource { + specs: Vec::new(), + executed: Vec::new(), + }; + let mut history = vec![ChatMessage::system("SYSTEM"), ChatMessage::user("TASK")]; + + let outcome = run_with_source(provider.as_ref(), &mut history, &mut tool_source).await; + + // Only the first call ran; the batch broke before #2/#3. + assert_eq!( + tool_source.executed.as_slice(), + ["run_code"], + "only the first tool call should have executed before the terminal halt" + ); + // The turn returned the root-cause halt summary (budget), not a generic stop. + assert!( + outcome.text.contains("out of inference budget"), + "expected the budget root-cause halt summary, got: {}", + outcome.text + ); + + // The persisted assistant message references ONLY the executed call id. + let assistant = history + .iter() + .find(|m| m.role == "assistant" && m.content.contains("tool_calls")) + .expect("an assistant message carrying tool_calls must be in history"); + let asst_ids = assistant_tool_call_ids(&assistant.content); + assert_eq!( + asst_ids, + vec!["call-A".to_string()], + "assistant tool-call list must be trimmed to the executed prefix \ + (no orphaned call-B/call-C): {asst_ids:?}" + ); + + // Exactly one tool result, matching that id — perfect id ↔ result pairing. + let result_ids = tool_result_ids(&history); + assert_eq!( + result_ids, + vec!["call-A".to_string()], + "exactly one tool-result, matching the single executed tool-call id: {result_ids:?}" + ); + + // The invariant the provider enforces: every persisted assistant tool-call id + // has a corresponding tool-result, and vice-versa (no orphans either way). + assert_eq!( + asst_ids, result_ids, + "tool-call ids and tool-result ids must be in lockstep after a batch break" + ); +} + +#[tokio::test] +async fn single_failing_call_pairs_id_with_result_boundary() { + // Boundary: a SINGLE native tool call that fails terminally. There is no + // un-executed suffix to trim, but the executed-prefix path must still produce + // exactly one assistant tool-call id paired with one tool-result (the + // degenerate case must not regress to zero results or an orphan). + let provider = BatchToolCallProvider::new(vec![ToolCall { + id: "only-1".into(), + name: "run_code".into(), + arguments: "{}".into(), + extra_content: None, + }]); + let mut tool_source = FailFirstToolSource { + specs: Vec::new(), + executed: Vec::new(), + }; + let mut history = vec![ChatMessage::system("SYSTEM"), ChatMessage::user("TASK")]; + + run_with_source(provider.as_ref(), &mut history, &mut tool_source).await; + + let assistant = history + .iter() + .find(|m| m.role == "assistant" && m.content.contains("tool_calls")) + .expect("assistant message with tool_calls"); + assert_eq!(assistant_tool_call_ids(&assistant.content), vec!["only-1"]); + assert_eq!(tool_result_ids(&history), vec!["only-1"]); +} + +#[tokio::test] +async fn full_batch_success_keeps_all_ids_paired() { + // Success path: when NO break happens (all calls run), every emitted tool-call + // id must be preserved and paired 1:1 with its result — proving the trim is + // additive (only fires on truncation) and never drops calls on the happy path. + struct AllOkToolSource { + specs: Vec, + } + #[async_trait] + impl ToolSource for AllOkToolSource { + fn request_specs(&self) -> &[crate::openhuman::tools::ToolSpec] { + &self.specs + } + async fn execute_call( + &mut self, + _call: &ParsedToolCall, + _iteration: usize, + _progress: &dyn super::ProgressReporter, + _progress_call_id: &str, + ) -> ToolRunResult { + ToolRunResult { + text: "ok".into(), + success: true, + } + } + } + + let provider = BatchToolCallProvider::new(vec![ + ToolCall { + id: "x1".into(), + name: "noop".into(), + arguments: "{}".into(), + extra_content: None, + }, + ToolCall { + id: "x2".into(), + name: "noop".into(), + arguments: "{}".into(), + extra_content: None, + }, + ]); + let mut tool_source = AllOkToolSource { specs: Vec::new() }; + let mut history = vec![ChatMessage::system("SYSTEM"), ChatMessage::user("TASK")]; + + run_with_source(provider.as_ref(), &mut history, &mut tool_source).await; + + let assistant = history + .iter() + .find(|m| m.role == "assistant" && m.content.contains("tool_calls")) + .expect("assistant message with tool_calls"); + let asst_ids = assistant_tool_call_ids(&assistant.content); + let result_ids = tool_result_ids(&history); + assert_eq!( + asst_ids, + vec!["x1".to_string(), "x2".to_string()], + "both emitted tool-call ids must be preserved on the all-success path" + ); + assert_eq!( + result_ids, + vec!["x1".to_string(), "x2".to_string()], + "both tool results must be present and ordered on the all-success path" + ); + assert_eq!(asst_ids, result_ids); +} + #[tokio::test] async fn engine_does_not_autocompact_when_opted_out() { // Same guard-tripping scenario, but `autocompact = None` (the main-agent / diff --git a/src/openhuman/agent/harness/tool_loop.rs b/src/openhuman/agent/harness/tool_loop.rs index f26962a4a..9a0f264f9 100644 --- a/src/openhuman/agent/harness/tool_loop.rs +++ b/src/openhuman/agent/harness/tool_loop.rs @@ -68,6 +68,124 @@ pub(crate) fn hard_reject_kind(result: &str) -> Option { } } +/// A permanent, non-retryable inference failure surfaced by a tool result — +/// typically a delegated sub-agent (`run_code` / `tools_agent` / `plan`) whose +/// provider call hit a user-state wall. Unlike a transient error, re-issuing the +/// call cannot succeed even under a *different* delegation tool or varied args: +/// the budget is account-wide and the model/provider configuration is shared by +/// every sub-agent. See [`terminal_inference_failure_kind`]. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum TerminalInferenceFailure { + /// Out of inference budget / credits — every retry hits the same wall. + /// Detected via + /// [`is_budget_exhausted_message`](crate::openhuman::inference::provider::is_budget_exhausted_message). + BudgetExhausted, + /// The configured model/provider rejected the request for a reason the user + /// must fix (unknown model, non-chat/embedding model, missing credential, + /// region block, …). Detected via + /// [`is_provider_config_rejection_message`](crate::openhuman::inference::provider::is_provider_config_rejection_message). + ProviderConfig, +} + +/// Inference/delegation **envelope** markers that prove a tool result came from +/// a delegated inference call (a sub-agent / provider round-trip) rather than +/// from arbitrary tool stderr. +/// +/// The two provider classifiers ([`is_budget_exhausted_message`] / +/// [`is_provider_config_rejection_message`]) match on short message substrings +/// (`"insufficient balance"`, `"invalid temperature"`, `"model field is +/// required"`, …) that can legitimately appear in a *recoverable* tool's output +/// — e.g. a `shell`/`run_code` script printing `ValueError: invalid temperature` +/// or a test asserting on `"model field is required"`. Applying the terminal +/// halt to those would misreport a fixable script failure as "fix the model or +/// API key" and stop after a single attempt. +/// +/// Gating on these envelope markers scopes the classifier to genuinely +/// delegated inference failures. Every marker here is **harness-generated** — +/// produced by our own reliable-chain rollup or sub-agent dispatch wrapper, NOT +/// by a provider HTTP body that arbitrary tool stderr could forge: +/// * the reliable-chain exhaustion rollup (`"All providers/models failed"` / +/// `"may not be available on your provider"`, reliable.rs), and +/// * the sub-agent dispatch wrapper (`"failed and did not complete"`, see +/// [`crate::openhuman::agent_orchestration::tools::dispatch::format_subagent_failure`]). +/// +/// **Why the bare provider envelope is NOT a marker (Codex review #3779):** the +/// raw provider-HTTP shape (`" API error (…)"`, `" Responses +/// API error: …"`) is reproducible verbatim by a *recoverable* tool that is +/// debugging its own API client — e.g. a `shell`/`run_code` script printing +/// `OpenAI API error (400): invalid temperature` or `… model field is required`. +/// Matching the bare `"api error"` substring there would let that script trip +/// the broad provider-config classifier and HALT the whole turn after a single +/// failed command with a misleading "fix your model in Settings → AI" message, +/// even though the agent should just recover. Every *genuine* delegated +/// inference failure additionally surfaces through one of the harness wrappers +/// above (a sub-agent provider error reaches the orchestrator only via +/// `dispatch::format_subagent_failure`; a direct reliable-chain exhaustion via +/// the rollup), so dropping the bare provider envelope loses no real detection +/// while closing the false-positive on tool stderr. +/// +/// [`is_budget_exhausted_message`]: crate::openhuman::inference::provider::is_budget_exhausted_message +/// [`is_provider_config_rejection_message`]: crate::openhuman::inference::provider::is_provider_config_rejection_message +const INFERENCE_FAILURE_ENVELOPE_MARKERS: &[&str] = &[ + // Reliable-chain exhaustion rollup (reliable.rs::format_failure_aggregate). + "all providers/models failed", + "may not be available on your provider", + // Sub-agent delegation failure wrapper (dispatch.rs::format_subagent_failure). + "failed and did not complete", +]; + +/// True if `result` carries one of the inference/delegation envelope markers — +/// i.e. the failure demonstrably came from a delegated provider round-trip, not +/// from an arbitrary tool's stderr. See [`INFERENCE_FAILURE_ENVELOPE_MARKERS`]. +fn has_inference_failure_envelope(result: &str) -> bool { + let lower = result.to_ascii_lowercase(); + INFERENCE_FAILURE_ENVELOPE_MARKERS + .iter() + .any(|marker| lower.contains(marker)) +} + +/// Recognize a permanent (non-retryable) inference failure from a tool result. +/// +/// Two-stage gate so a *recoverable* tool failure can't be misclassified: +/// 1. The result must carry a delegated-inference **envelope** +/// ([`has_inference_failure_envelope`]) — proving it came from a sub-agent +/// / provider round-trip and not from arbitrary tool stderr that merely +/// happens to contain a classifier substring (e.g. a `shell` script +/// printing `ValueError: invalid temperature` or a test asserting on +/// `"model field is required"`). Without this guard a fixable script/test +/// failure would be misreported as "fix the model or API key" and stopped +/// after a single attempt (Codex review #3779). +/// 2. The (then-trusted) body is matched against the two deliberately-tight +/// provider classifiers, which stay in lockstep with the Sentry-demotion +/// phrase sets: a transient / 5xx / generic 4xx body matches NEITHER, so +/// genuinely retryable failures still get the normal consecutive-failure +/// grace ([`NO_PROGRESS_FAILURE_THRESHOLD`]) and are never halted early. +/// Budget takes precedence if both somehow match. +/// +/// The orchestrator otherwise re-emits a failed delegation under *varied* tool +/// names (Plan → `run_code` → `tools_agent`), so the identical-`(tool,args)` +/// [`REPEAT_FAILURE_THRESHOLD`] never trips and the chain grinds through ~6-8 +/// doomed, paid delegations before [`NO_PROGRESS_FAILURE_THRESHOLD`] finally +/// halts with an opaque "Something went wrong" (#3104). Tripping on the FIRST +/// permanent failure stops that cascade and surfaces the root cause. +pub(crate) fn terminal_inference_failure_kind(result: &str) -> Option { + use crate::openhuman::inference::provider::{ + is_budget_exhausted_message, is_provider_config_rejection_message, + }; + // Require the delegated-inference envelope first: the message-only + // classifiers are too broad to apply to arbitrary tool stderr. + if !has_inference_failure_envelope(result) { + return None; + } + if is_budget_exhausted_message(result) { + Some(TerminalInferenceFailure::BudgetExhausted) + } else if is_provider_config_rejection_message(result) { + Some(TerminalInferenceFailure::ProviderConfig) + } else { + None + } +} + /// Shared repeated-failure circuit breaker, used by BOTH agent loops /// (`run_tool_call_loop` here and `run_inner_loop` in `subagent_runner`) so they /// can't drift. Tracks per-`(tool,args)`-signature failure counts and a @@ -108,6 +226,37 @@ impl RepeatFailureGuard { *c += 1; *c }; + // Permanent inference failures (out of budget / provider-config rejection) + // cannot be recovered by retrying — the budget is account-wide and the + // model/provider config is shared by every (sub-)agent. Halt on the FIRST + // occurrence with an actionable root cause instead of letting the + // orchestrator re-emit the step under varied delegation-tool names until + // NO_PROGRESS_FAILURE_THRESHOLD (#3104: Plan → Run Code ×6 → Tools Agent + // ×2). Checked before the count-based thresholds precisely because those + // never trip in time when the tool name keeps changing. + if let Some(kind) = terminal_inference_failure_kind(result) { + tracing::warn!( + tool, + kind = ?kind, + "[agent_loop] permanent inference failure — halting on first occurrence with root cause" + ); + return Some(match kind { + TerminalInferenceFailure::BudgetExhausted => format!( + "Stopping: the `{tool}` step failed because the account is out of inference \ + budget/credits — every retry hits the same wall. Add credits to your account \ + (or, when using a custom/BYO provider, top up that provider's own account) \ + and try again. Details:\n{}", + truncate_for_halt(result), + ), + TerminalInferenceFailure::ProviderConfig => format!( + "Stopping: the `{tool}` step failed because the configured model/provider \ + rejected the request (e.g. an unknown model, a non-chat/embedding model, a \ + missing credential, or a region block) — retrying will not help. Fix the model \ + or API key in Settings → AI. Details:\n{}", + truncate_for_halt(result), + ), + }); + } // Hard policy rejections trip on the first verbatim repeat; everything // else uses the generic identical-retry threshold. let hard = hard_reject_kind(result); diff --git a/src/openhuman/agent/harness/tool_loop_tests.rs b/src/openhuman/agent/harness/tool_loop_tests.rs index 18f2b9e0a..60a1b8bca 100644 --- a/src/openhuman/agent/harness/tool_loop_tests.rs +++ b/src/openhuman/agent/harness/tool_loop_tests.rs @@ -109,6 +109,62 @@ impl Tool for ErrorResultTool { } } +/// Simulates a delegated sub-agent (`run_code` / `tools_agent`) whose provider +/// call hit a permanent, non-retryable wall: `dispatch_subagent` converts that +/// sub-agent `Err` into a `ToolResult::error` carrying the budget-exhaustion +/// body. Used to prove the #3104 cascade halts on the FIRST such failure. +struct BudgetExhaustedDelegationTool; + +#[async_trait] +impl Tool for BudgetExhaustedDelegationTool { + fn name(&self) -> &str { + "run_code" + } + + fn description(&self) -> &str { + "delegate to the code executor (always 400s on budget here)" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({"type":"object"}) + } + + async fn execute(&self, _args: serde_json::Value) -> Result { + // Mirrors dispatch.rs::format_subagent_failure wrapping the upstream body. + Ok(ToolResult::error( + "run_code failed and did not complete — no work was performed. \ + Error: OpenHuman API error (400): {\"error\":\"Insufficient budget\"}", + )) + } +} + +/// Records whether it ever executed. Used to prove that a tool placed AFTER a +/// terminal-inference failure in the SAME assistant-message batch never runs +/// (#3104 — Codex review #3779, batched tool calls). +struct RanTrackerTool { + ran: std::sync::Arc, +} + +#[async_trait] +impl Tool for RanTrackerTool { + fn name(&self) -> &str { + "ran_tracker" + } + + fn description(&self) -> &str { + "records that it executed" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({"type":"object"}) + } + + async fn execute(&self, _args: serde_json::Value) -> Result { + self.ran.store(true, std::sync::atomic::Ordering::SeqCst); + Ok(ToolResult::success("ran-tracker-out")) + } +} + struct FailingTool; #[async_trait] @@ -1071,6 +1127,148 @@ async fn run_tool_call_loop_halts_when_no_progress() { ); } +/// #3104 end-to-end repro through the real engine: a delegation that fails with +/// a PERMANENT inference condition (out of budget) must halt the orchestrator on +/// the FIRST failure with an actionable root cause — NOT grind to the +/// 6-consecutive no-progress backstop (the Plan → Run Code ×6 → Tools Agent ×2 +/// cascade the user saw). Before the fix this loop consumed 6 LLM turns and +/// ended in a generic message; after it, exactly 1. +#[tokio::test] +async fn run_tool_call_loop_halts_on_first_budget_exhausted_delegation() { + let mut responses = Vec::new(); + for i in 0..10 { + // Varied args so the per-signature repeat guard can NEVER trip — only the + // terminal-inference check (or, pre-fix, the 6-consecutive backstop) can. + responses.push(Ok(ChatResponse { + text: Some(format!( + "{{\"name\":\"run_code\",\"arguments\":{{\"step\":{i}}}}}" + )), + tool_calls: vec![], + usage: None, + reasoning_content: None, + })); + } + let provider = ScriptedProvider { + responses: Mutex::new(responses), + native_tools: false, + vision: false, + }; + let mut history = vec![ChatMessage::user("build me a dashboard")]; + let tools: Vec> = vec![Box::new(BudgetExhaustedDelegationTool)]; + + let result = run_tool_call_loop( + &provider, + &mut history, + &tools, + "test-provider", + "model", + 0.0, + true, + "channel", + &crate::openhuman::config::MultimodalConfig::default(), + &crate::openhuman::config::MultimodalFileConfig::default(), + 10, // max_iterations — must NOT be reached; terminal halt fires at 1 + None, + None, + &[], + None, + None, + &crate::openhuman::tools::policy::DefaultToolPolicy, + ) + .await + .expect("budget-exhausted halt returns Ok with an actionable summary"); + + assert!( + result.contains("Stopping") && result.contains("out of inference budget"), + "expected an actionable budget halt, got: {result}" + ); + // The decisive assertion: only the FIRST scripted turn was consumed (9 of 10 + // remain). Proves the cascade is killed on the first permanent failure + // instead of burning 6 doomed, paid delegations. + assert_eq!( + provider.responses.lock().len(), + 9, + "loop must halt after exactly 1 turn on a permanent inference failure" + ); +} + +/// #3104 batched-tool-call guarantee (Codex review #3779): when a single +/// assistant message emits MULTIPLE tool calls and the FIRST records a terminal +/// inference failure (out of budget / provider-config), the loop must STOP +/// executing the rest of the batch — so a second delegated call in the same +/// message can never launch a paid sub-agent after the first proved the wall is +/// unrecoverable. Pre-fix the loop set `halt_reason` but drained the batch first. +#[tokio::test] +async fn run_tool_call_loop_stops_batch_after_first_terminal_failure() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + + // One assistant message with TWO tool calls: the budget-exhausted delegation + // first, a tracker second. Native mode emits both as `tool_calls` in a single + // ChatResponse so they share one batch iteration. + let provider = ScriptedProvider { + responses: Mutex::new(vec![Ok(ChatResponse { + text: Some(String::new()), + tool_calls: vec![ + crate::openhuman::inference::provider::ToolCall { + id: "call-budget".into(), + name: "run_code".into(), + arguments: "{}".into(), + extra_content: None, + }, + crate::openhuman::inference::provider::ToolCall { + id: "call-tracker".into(), + name: "ran_tracker".into(), + arguments: "{}".into(), + extra_content: None, + }, + ], + usage: None, + reasoning_content: None, + })]), + native_tools: true, + vision: false, + }; + let mut history = vec![ChatMessage::user("build me a dashboard")]; + let ran = Arc::new(AtomicBool::new(false)); + let tools: Vec> = vec![ + Box::new(BudgetExhaustedDelegationTool), + Box::new(RanTrackerTool { ran: ran.clone() }), + ]; + + let result = run_tool_call_loop( + &provider, + &mut history, + &tools, + "test-provider", + "model", + 0.0, + true, + "channel", + &crate::openhuman::config::MultimodalConfig::default(), + &crate::openhuman::config::MultimodalFileConfig::default(), + 5, + None, + None, + &[], + None, + None, + &crate::openhuman::tools::policy::DefaultToolPolicy, + ) + .await + .expect("budget-exhausted halt returns Ok with an actionable summary"); + + assert!( + result.contains("Stopping") && result.contains("out of inference budget"), + "expected an actionable budget halt, got: {result}" + ); + // The decisive assertion: the SECOND call in the batch must NOT have run. + assert!( + !ran.load(Ordering::SeqCst), + "a tool placed after a terminal inference failure in the same batch must NOT execute" + ); +} + // -- RepeatFailureGuard (shared by run_tool_call_loop + run_inner_loop) -------- #[test] @@ -1197,6 +1395,268 @@ fn hard_reject_distinct_args_do_not_trip_repeat() { ); } +// -- Terminal inference failures (#3104: budget / provider-config cascade) ------- + +#[test] +fn terminal_inference_failure_kind_classifies_budget_and_config() { + // Budget wins precedence; provider-config covers the user-state model/key + // rejections; transient / 5xx / generic-4xx match NEITHER (so retryable + // failures keep their normal consecutive-failure grace). + assert_eq!( + terminal_inference_failure_kind( + "run_code failed and did not complete. Error: OpenHuman API error (400): \ + {\"error\":\"insufficient budget — add credits\"}" + ), + Some(TerminalInferenceFailure::BudgetExhausted) + ); + assert_eq!( + terminal_inference_failure_kind( + "tools_agent failed and did not complete. Error: ollama API error (400 Bad Request): \ + {\"error\":{\"message\":\"\\\"bge-m3:latest\\\" does not support chat\"}}" + ), + Some(TerminalInferenceFailure::ProviderConfig) + ); + for transient in [ + "Error: connection reset by peer", + "Error: 503 Service Unavailable", + "Error: rate limit exceeded, retry after 1s", + "run_code failed and did not complete. Error: timed out", + ] { + assert_eq!( + terminal_inference_failure_kind(transient), + None, + "{transient:?} must NOT classify as a terminal inference failure" + ); + } +} + +#[test] +fn terminal_inference_failure_requires_delegated_inference_envelope() { + // Codex review #3779: the message-only provider classifiers match short + // substrings (`invalid temperature`, `model field is required`, + // `insufficient balance`, …) that can legitimately appear in a RECOVERABLE + // tool's stderr. Those must NOT trip the terminal halt — only a result that + // carries a delegated-inference/provider envelope may. + + // 1) Recoverable `shell`/`run_code` SCRIPT stderr that merely *contains* a + // classifier substring — no provider envelope → must NOT classify, so the + // normal consecutive-failure grace still applies and the script can be + // retried/fixed. + for recoverable in [ + // A Python test raising on a `temperature` arg — not an inference call. + "ValueError: invalid temperature: only 1 is allowed for this model", + // A user script validating config and printing the provider-ish phrase. + "AssertionError: model field is required", + // A finance script echoing an account-balance string. + "RuntimeError: insufficient balance in wallet 0xabc", + // A test asserting on the literal remediation copy. + "FAILED test_models.py::test_unknown - assert 'model_not_found' in resp", + ] { + assert_eq!( + terminal_inference_failure_kind(recoverable), + None, + "recoverable tool stderr without an inference envelope must NOT classify \ + as a terminal inference failure: {recoverable:?}" + ); + } + + // 2) The SAME phrases, but now wrapped in a genuine delegated-inference + // envelope (provider HTTP error / reliable rollup / sub-agent dispatch + // wrapper) → MUST classify, because these only come from a delegated + // provider round-trip. + assert_eq!( + terminal_inference_failure_kind( + "run_code failed and did not complete. Error: custom_openai API error \ + (400 Bad Request): {\"error\":{\"message\":\"invalid temperature: only 1 is \ + allowed for this model\"}}" + ), + Some(TerminalInferenceFailure::ProviderConfig), + "a delegated provider config-rejection (with envelope) must classify" + ); + assert_eq!( + terminal_inference_failure_kind( + "tools_agent failed and did not complete. Error: OpenHuman API error (402): \ + {\"error\":\"Insufficient balance\"}" + ), + Some(TerminalInferenceFailure::BudgetExhausted), + "a delegated budget-exhaustion (with envelope) must classify" + ); + // The reliable-chain rollup envelope (no `API error` token) also qualifies. + assert_eq!( + terminal_inference_failure_kind( + "All providers/models failed. Attempts:\n provider=custom_openai model=gpt-5.5 \ + attempt 1/3: non_retryable; error=insufficient balance" + ), + Some(TerminalInferenceFailure::BudgetExhausted), + "a reliable-chain exhaustion rollup carrying a budget body must classify" + ); +} + +#[test] +fn bare_provider_api_error_in_tool_stderr_does_not_classify() { + // Codex review #3779 (follow-up): a recoverable `shell`/`run_code` task that + // is debugging its OWN API client can print the verbatim provider-HTTP + // envelope shape (` API error (status): …`). Without a + // harness-generated wrapper (`failed and did not complete` / + // `all providers/models failed` / `may not be available on your provider`) + // that output must NOT be treated as a delegated inference failure — else the + // whole turn halts after a single failed command with a misleading "fix your + // model in Settings → AI" message. + for tool_stderr in [ + // The exact shape called out in the review: a config-rejection body that + // a debugging script could print to stdout/stderr. + "OpenAI API error (400): invalid temperature", + "OpenAI API error (400): model field is required", + // Budget-flavoured body, same forged-envelope risk. + "custom_openai API error (402 Payment Required): {\"error\":\"Insufficient balance\"}", + // Responses/streaming envelope variants, still bare (no harness wrapper). + "custom_openai Responses API error: {\"error\":{\"message\":\"model 'x' not found\"}}", + "custom_openai streaming API error (404 Not Found): model 'llama3.3' not found", + ] { + assert_eq!( + terminal_inference_failure_kind(tool_stderr), + None, + "a bare provider-HTTP envelope without a harness wrapper must NOT \ + classify as a terminal inference failure: {tool_stderr:?}" + ); + } +} + +#[test] +fn bare_provider_api_error_keeps_consecutive_failure_grace() { + // The behavioural consequence of the above at the guard level: a `run_code` + // task printing a bare provider API-error body on each attempt must keep its + // normal consecutive-failure grace (it is recoverable user code, not a + // delegated wall), and only trip the 6-in-a-row no-progress backstop — never + // halt on the FIRST failure. + let mut g = RepeatFailureGuard::new(); + let bare = "OpenAI API error (400): invalid temperature"; + for i in 0..(NO_PROGRESS_FAILURE_THRESHOLD - 1) { + assert!( + g.record("run_code", &format!("debug-attempt{i}"), false, bare) + .is_none(), + "a bare provider API-error in tool stderr must NOT halt on failure #{}", + i + 1 + ); + } + // The Nth consecutive failure trips the generic no-progress backstop, as for + // any other recoverable failure — proving the grace was preserved, not that + // the terminal classifier fired. + let halt = g.record( + "run_code", + &format!("debug-attempt{}", NO_PROGRESS_FAILURE_THRESHOLD - 1), + false, + bare, + ); + let msg = halt.expect("the consecutive no-progress backstop still trips"); + assert!( + msg.contains("in a row failed with no progress"), + "must halt via the generic no-progress path, not the terminal-inference \ + path: {msg}" + ); + assert!( + !msg.contains("Settings → AI") && !msg.contains("out of inference budget"), + "must NOT use the terminal-inference remediation copy: {msg}" + ); +} + +#[test] +fn delegated_provider_failure_still_halts_first_after_narrowing() { + // Boundary guard for the #3779 narrowing: a GENUINE delegated provider + // failure — the same config-rejection body, but wrapped by the sub-agent + // dispatch (`failed and did not complete`) — MUST still halt on the first + // occurrence. Narrowing the envelope must not regress the #3104 fix. + let mut g = RepeatFailureGuard::new(); + let delegated = "run_code failed and did not complete — no work was performed. \ + Error: OpenAI API error (400): invalid temperature"; + let halt = g.record("run_code", "{\"prompt\":\"x\"}", false, delegated); + let msg = halt.expect("a wrapped delegated provider-config rejection must halt first"); + assert!(msg.contains("rejected the request"), "got: {msg}"); + assert!( + msg.contains("Settings → AI"), + "actionable remediation: {msg}" + ); +} + +#[test] +fn terminal_budget_failure_halts_on_first_occurrence() { + let mut g = RepeatFailureGuard::new(); + let budget = "run_code failed and did not complete — no work was performed. \ + Error: OpenHuman API error (400): {\"error\":\"Insufficient budget\"}"; + let halt = g.record("run_code", "{\"prompt\":\"write a file\"}", false, budget); + assert!( + halt.is_some(), + "a budget-exhausted delegation must halt on the FIRST failure, not grind to 6" + ); + let msg = halt.unwrap(); + assert!(msg.contains("out of inference budget"), "got: {msg}"); + assert!(msg.contains("`run_code`"), "names the failing step: {msg}"); +} + +#[test] +fn terminal_config_rejection_halts_on_first_occurrence() { + let mut g = RepeatFailureGuard::new(); + let cfg = "tools_agent failed and did not complete. Error: OpenHuman API error \ + (400 Bad Request): Model 'gpt-5.5' is not available. Use GET \ + /openai/v1/models to list available models."; + let halt = g.record("tools_agent", "{\"prompt\":\"do the thing\"}", false, cfg); + assert!( + halt.is_some(), + "a provider-config rejection must halt on the FIRST failure" + ); + let msg = halt.unwrap(); + assert!(msg.contains("rejected the request"), "got: {msg}"); + assert!( + msg.contains("Settings → AI"), + "actionable remediation: {msg}" + ); +} + +#[test] +fn terminal_failure_halts_first_even_across_varied_delegation_tools() { + // The #3104 cascade reproduction at the guard level: the orchestrator + // re-emits a doomed step under VARIED tool names (plan → run_code → + // tools_agent), so neither the identical-(tool,args) repeat guard nor the + // 6-consecutive no-progress guard would catch it in time. The terminal + // classifier must halt on the very first permanent failure regardless of + // which delegation tool surfaced it. + let mut g = RepeatFailureGuard::new(); + // Carries the sub-agent dispatch envelope (`failed and did not complete`) + // so it is recognised as a *delegated* inference failure, not arbitrary + // tool stderr — see `has_inference_failure_envelope`. + let budget = "plan failed and did not complete. Error: {\"error\":\"insufficient balance\"}"; + let halt = g.record("plan", "{\"goal\":\"x\"}", false, budget); + assert!( + halt.is_some(), + "first permanent failure under ANY delegation tool must halt immediately" + ); +} + +#[test] +fn terminal_classifier_does_not_short_circuit_transient_grace() { + // A genuinely transient (retryable) failure must still flow through the + // existing no-progress backstop — proving the terminal halt is additive, + // not a blanket "halt on first failure" regression. + let mut g = RepeatFailureGuard::new(); + for i in 0..5 { + assert!( + g.record( + "run_code", + &format!("attempt{i}"), + false, + "Error: timed out" + ) + .is_none(), + "transient failures must NOT halt before the 6-consecutive threshold" + ); + } + assert!( + g.record("run_code", "attempt5", false, "Error: timed out") + .is_some(), + "6 consecutive transient failures still trip the no-progress guard" + ); +} + /// Provider that records the tool-spec names of every `chat()` request /// it sees, then returns the next scripted response. struct CapturingProvider {