From fbf86aba646cfc4e98347b4df07da2d37b34d412 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Fri, 26 Jun 2026 02:37:26 +0530 Subject: [PATCH] fix(code_executor): surface command exit code + both streams so the agent stops looping (#4095) (#4128) --- .../agent/harness/tool_loop_tests.rs | 29 +++ .../agents/code_executor/prompt.md | 1 + .../tools/impl/system/command_output.rs | 167 ++++++++++++++++++ src/openhuman/tools/impl/system/mod.rs | 1 + src/openhuman/tools/impl/system/node_exec.rs | 20 ++- src/openhuman/tools/impl/system/npm_exec.rs | 20 ++- src/openhuman/tools/impl/system/shell.rs | 74 +++++++- 7 files changed, 288 insertions(+), 24 deletions(-) create mode 100644 src/openhuman/tools/impl/system/command_output.rs diff --git a/src/openhuman/agent/harness/tool_loop_tests.rs b/src/openhuman/agent/harness/tool_loop_tests.rs index 57a6b632f..2a688cf5a 100644 --- a/src/openhuman/agent/harness/tool_loop_tests.rs +++ b/src/openhuman/agent/harness/tool_loop_tests.rs @@ -1290,6 +1290,35 @@ fn repeat_failure_guard_halts_on_3_identical() { assert!(halt.unwrap().contains("externally-managed-environment")); } +/// End-to-end intent for the code_executor manifestation (#4095): once the +/// shell-family tools surface the exit code (the `command_output` formatter), a +/// dependency wall (`exit code 127 — command not found …`) re-issued with +/// IDENTICAL args trips the shared breaker within `REPEAT_FAILURE_THRESHOLD` and +/// returns an actionable halt summary — so the turn ends with a surfaced result, +/// not a silent ~10× retry loop. The string here mirrors what +/// `system::command_output::render_command_failure(Some(127), …)` now produces. +#[test] +fn repeat_failure_guard_halts_on_surfaced_dependency_wall() { + let surfaced = "Error: Command failed (exit code 127 — command not found: a required \ + executable or dependency is missing or not on PATH. Install/declare it, \ + use an available alternative, or report the blocker — do NOT re-run the \ + same command)\n[stderr]\npytest: command not found"; + let mut g = RepeatFailureGuard::new(); + assert!(g.record("shell", "pytest -q", false, surfaced).is_none()); + assert!(g.record("shell", "pytest -q", false, surfaced).is_none()); + let halt = g.record("shell", "pytest -q", false, surfaced); + assert!( + halt.is_some(), + "an identical dependency-wall failure must trip the breaker within \ + REPEAT_FAILURE_THRESHOLD instead of looping" + ); + let summary = halt.unwrap(); + assert!( + summary.contains("127") || summary.contains("command not found"), + "halt summary should carry the surfaced root cause: {summary}" + ); +} + #[test] fn repeat_failure_guard_halts_on_6_consecutive_varied() { let mut g = RepeatFailureGuard::new(); diff --git a/src/openhuman/agent_registry/agents/code_executor/prompt.md b/src/openhuman/agent_registry/agents/code_executor/prompt.md index 4ec2a3fc8..1c2039637 100644 --- a/src/openhuman/agent_registry/agents/code_executor/prompt.md +++ b/src/openhuman/agent_registry/agents/code_executor/prompt.md @@ -49,6 +49,7 @@ Shell commands run through an approval gate under the user's access policy. Keep - **No `sudo` / system package installs** unless the user explicitly granted it. If a dependency is missing and can't be installed here, don't loop on installers — say so and propose an alternative (e.g. a stdlib-only approach). - **If you create a virtualenv, use it.** After `python3 -m venv .venv`, install and run with `.venv/bin/pip` and `.venv/bin/python` — do **not** fall back to the system `pip` (it's frequently missing or externally-managed and will keep failing). - **Only stdout/stderr comes back to you.** `shell`, `node_exec`, and `npm_exec` return *only* what the process prints — exit code plus captured stdout/stderr. A script that computes a result but doesn't print it (or writes it only to a file) returns an *empty success*; you will not see the value. Always make scripts `print(...)` / `console.log(...)` the result you need, or follow up by reading the file they wrote. Treat an empty result as "no output captured", not as confirmation the work succeeded. +- **Read the exit code on failure.** A failed command comes back as `Command failed (exit code N …)` followed by its `[stdout]` and `[stderr]`. Use the code to pick your next move instead of re-running: **127** = command/dependency not found (install or declare it, use a stdlib/available alternative, or report the blocker — never re-run the same command); **126** = permission denied / not executable (usually a sandbox restriction — it will not succeed on retry, so report it or request escalation); other non-zero codes are ordinary failures — read the streams for the root cause. Re-issuing a command that already failed the same way is the loop that gets a run cut off with nothing shipped. ## Rules diff --git a/src/openhuman/tools/impl/system/command_output.rs b/src/openhuman/tools/impl/system/command_output.rs new file mode 100644 index 000000000..c50c45cd8 --- /dev/null +++ b/src/openhuman/tools/impl/system/command_output.rs @@ -0,0 +1,167 @@ +//! Shared output formatting for the shell-family command tools (`shell`, +//! `node_exec`, `npm_exec`). +//! +//! # Why this exists (code_executor no-progress loop, #4095) +//! +//! Each shell-family tool previously formatted a FAILED command as +//! `if stderr.is_empty() { stdout } else { stderr }` with no exit code. That +//! lost the two signals the agent most needs to recover: +//! +//! 1. **stdout was dropped whenever stderr was non-empty.** Compilers, test +//! runners and linters routinely write diagnostics to stdout; the model +//! saw only stderr and lost half the failure context. +//! 2. **the exit code was never surfaced.** The model could not tell a `127` +//! (command / dependency not found) or a `126` (permission denied — often a +//! sandbox restriction) from a generic `1`, so it could not recognise an +//! un-retryable wall and re-ran the identical command. The harness +//! repeated-failure circuit breaker (`RepeatFailureGuard`, see +//! `agent/harness/tool_loop.rs`) still bounds that loop, but only after a +//! few wasted iterations and with a generic halt message, because the +//! root-cause signal had already been thrown away. +//! +//! This module surfaces the exit code AND both streams on failure, and appends a +//! short hint for the well-known dependency/sandbox exit codes so the agent can +//! adapt (install/declare the dependency, request escalation, or report the +//! blocker) instead of retrying blindly. The success path is intentionally left +//! as raw stdout so existing callers that parse a command's output (e.g. `pwd`) +//! keep working unchanged. + +use crate::openhuman::tools::traits::ToolResult; + +/// Hint appended after the exit code for the exit statuses that almost always +/// mean "this exact command cannot succeed on retry here". Empty for every other +/// code so an ordinary application failure is never editorialised. +fn exit_code_hint(code: i32) -> &'static str { + match code { + 127 => { + " — command not found: a required executable or dependency is \ + missing or not on PATH. Install/declare it, use an available \ + alternative, or report the blocker — do NOT re-run the same command" + } + 126 => { + " — permission denied or not executable: often a sandbox \ + restriction. This will not succeed on retry — report the blocker \ + or request escalation instead of repeating the command" + } + _ => "", + } +} + +/// Render a finished command's exit status + captured streams into the text the +/// model sees on FAILURE. Never drops a non-empty stream; always states the exit +/// code (or that the process was terminated by a signal). +pub(crate) fn render_command_failure(exit_code: Option, stdout: &str, stderr: &str) -> String { + let mut out = match exit_code { + Some(code) => format!("Command failed (exit code {code}{})", exit_code_hint(code)), + None => "Command failed (terminated by a signal — no exit code)".to_string(), + }; + let stdout = stdout.trim_end(); + let stderr = stderr.trim_end(); + if !stdout.is_empty() { + out.push_str("\n[stdout]\n"); + out.push_str(stdout); + } + if !stderr.is_empty() { + out.push_str("\n[stderr]\n"); + out.push_str(stderr); + } + if stdout.is_empty() && stderr.is_empty() { + out.push_str("\n(no output was captured on stdout or stderr)"); + } + out +} + +/// A `ToolResult::error` carrying [`render_command_failure`]. The single failure +/// constructor shared by every shell-family tool, on both the native and the +/// sandboxed execution path, so the surfaced shape can't drift between them. +pub(crate) fn command_failure(exit_code: Option, stdout: &str, stderr: &str) -> ToolResult { + ToolResult::error(render_command_failure(exit_code, stdout, stderr)) +} + +/// Normalise a sandbox backend's `exit_code` into the `Option` the +/// formatter expects. The sandbox layer uses `-1` as its sentinel for +/// "terminated / no real exit code" (see `sandbox::types::SandboxExecResult`); +/// map any negative value to `None` so it renders as a signal termination rather +/// than the literal `exit code -1`. +pub(crate) fn sandbox_exit_code(code: i32) -> Option { + if code < 0 { + None + } else { + Some(code) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn failure_surfaces_exit_code_and_both_streams() { + let rendered = render_command_failure(Some(7), "the-stdout-line", "the-stderr-line"); + assert!( + rendered.contains("exit code 7"), + "exit code missing: {rendered}" + ); + assert!( + rendered.contains("the-stdout-line"), + "stdout dropped on failure: {rendered}" + ); + assert!( + rendered.contains("the-stderr-line"), + "stderr dropped on failure: {rendered}" + ); + } + + #[test] + fn failure_keeps_stdout_even_when_stderr_present() { + // The exact regression: the old `if stderr.is_empty() { stdout } else + // { stderr }` formatting threw stdout away whenever stderr existed. + let rendered = render_command_failure(Some(1), "diagnostic-on-stdout", "error-on-stderr"); + assert!(rendered.contains("diagnostic-on-stdout")); + assert!(rendered.contains("error-on-stderr")); + } + + #[test] + fn exit_127_hints_missing_command_or_dependency() { + let rendered = render_command_failure(Some(127), "", "pytest: command not found"); + assert!(rendered.contains("exit code 127")); + assert!( + rendered.to_lowercase().contains("command not found"), + "127 should hint at a missing command/dependency: {rendered}" + ); + } + + #[test] + fn exit_126_hints_permission_or_sandbox() { + let rendered = render_command_failure(Some(126), "", "permission denied"); + assert!(rendered.contains("exit code 126")); + assert!( + rendered.to_lowercase().contains("sandbox") + || rendered.to_lowercase().contains("permission denied"), + "126 should hint at a permission/sandbox wall: {rendered}" + ); + } + + #[test] + fn ordinary_failure_code_gets_no_hint() { + let rendered = render_command_failure(Some(1), "", "boom"); + // No editorialising for a generic application failure. + assert!(rendered.contains("exit code 1")); + assert!(!rendered.contains("command not found")); + assert!(!rendered.contains("sandbox")); + } + + #[test] + fn signal_termination_has_no_exit_code() { + let rendered = render_command_failure(None, "", ""); + assert!(rendered.contains("terminated by a signal")); + assert!(rendered.contains("no output was captured")); + } + + #[test] + fn sandbox_negative_exit_code_maps_to_signal() { + assert_eq!(sandbox_exit_code(-1), None); + assert_eq!(sandbox_exit_code(0), Some(0)); + assert_eq!(sandbox_exit_code(7), Some(7)); + } +} diff --git a/src/openhuman/tools/impl/system/mod.rs b/src/openhuman/tools/impl/system/mod.rs index cb22d0c72..14c00a0a9 100644 --- a/src/openhuman/tools/impl/system/mod.rs +++ b/src/openhuman/tools/impl/system/mod.rs @@ -1,3 +1,4 @@ +mod command_output; mod current_time; mod detect_tools; mod insert_sql_record; diff --git a/src/openhuman/tools/impl/system/node_exec.rs b/src/openhuman/tools/impl/system/node_exec.rs index da551562a..ac146bdfd 100644 --- a/src/openhuman/tools/impl/system/node_exec.rs +++ b/src/openhuman/tools/impl/system/node_exec.rs @@ -315,8 +315,13 @@ impl Tool for NodeExecTool { Ok(ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}"))) } } else { - let err_msg = if stderr.is_empty() { stdout } else { stderr }; - Ok(ToolResult::error(err_msg)) + // Surface exit code + both streams so the agent can diagnose + // the failure instead of re-running it (#4095). + Ok(super::command_output::command_failure( + output.status.code(), + &stdout, + &stderr, + )) } } Ok(Err(e)) => Ok(ToolResult::error(format!("Failed to execute node: {e}"))), @@ -426,12 +431,11 @@ impl NodeExecTool { )) } } else { - let err_msg = if result.stderr.is_empty() { - result.stdout - } else { - result.stderr - }; - ToolResult::error(err_msg) + super::command_output::command_failure( + super::command_output::sandbox_exit_code(result.exit_code), + &result.stdout, + &result.stderr, + ) } } Err(e) => ToolResult::error(format!("Sandbox execution failed: {e}")), diff --git a/src/openhuman/tools/impl/system/npm_exec.rs b/src/openhuman/tools/impl/system/npm_exec.rs index 945c94226..83e4bd16c 100644 --- a/src/openhuman/tools/impl/system/npm_exec.rs +++ b/src/openhuman/tools/impl/system/npm_exec.rs @@ -324,8 +324,13 @@ impl Tool for NpmExecTool { Ok(ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}"))) } } else { - let err_msg = if stderr.is_empty() { stdout } else { stderr }; - Ok(ToolResult::error(err_msg)) + // Surface exit code + both streams so the agent can diagnose + // the failure instead of re-running it (#4095). + Ok(super::command_output::command_failure( + output.status.code(), + &stdout, + &stderr, + )) } } Ok(Err(e)) => Ok(ToolResult::error(format!("Failed to execute npm: {e}"))), @@ -428,12 +433,11 @@ impl NpmExecTool { )) } } else { - let err_msg = if result.stderr.is_empty() { - result.stdout - } else { - result.stderr - }; - ToolResult::error(err_msg) + super::command_output::command_failure( + super::command_output::sandbox_exit_code(result.exit_code), + &result.stdout, + &result.stderr, + ) } } Err(e) => ToolResult::error(format!("Sandbox execution failed: {e}")), diff --git a/src/openhuman/tools/impl/system/shell.rs b/src/openhuman/tools/impl/system/shell.rs index f0546362a..2f07c6c9f 100644 --- a/src/openhuman/tools/impl/system/shell.rs +++ b/src/openhuman/tools/impl/system/shell.rs @@ -428,8 +428,10 @@ impl ShellTool { ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}")) } } else { - let err_msg = if stderr.is_empty() { stdout } else { stderr }; - ToolResult::error(err_msg) + // Surface the exit code AND both streams so the agent can + // diagnose the failure (e.g. 127 missing dependency, 126 + // sandbox/permission wall) instead of looping on it (#4095). + super::command_output::command_failure(output.status.code(), &stdout, &stderr) } } Ok(Err(e)) => ToolResult::error(format!("Failed to execute command: {e}")), @@ -517,12 +519,13 @@ impl ShellTool { )) } } else { - let err_msg = if result.stderr.is_empty() { - result.stdout - } else { - result.stderr - }; - ToolResult::error(err_msg) + // Same exit-code + both-streams surfacing as the native path + // (#4095); the sandbox `-1` sentinel renders as a signal. + super::command_output::command_failure( + super::command_output::sandbox_exit_code(result.exit_code), + &result.stdout, + &result.stderr, + ) }; (true, tool_result) } @@ -963,6 +966,61 @@ mod tests { assert!(result.is_error); } + /// Regression for the code_executor no-progress loop (#4095): a FAILED + /// command must surface its exit code AND both streams — never drop stdout + /// when stderr is present — so the agent can read *why* it failed instead of + /// re-running it blindly. + #[cfg(not(windows))] + #[tokio::test] + async fn shell_failure_surfaces_exit_code_and_both_streams() { + let tool = ShellTool::new( + test_security(AutonomyLevel::Full), + test_runtime(), + test_audit(), + ); + let result = tool + .execute(json!({ + "command": "echo stdout-marker; echo stderr-marker 1>&2; exit 7" + })) + .await + .unwrap(); + assert!(result.is_error, "non-zero exit must be an error result"); + let out = result.output(); + assert!(out.contains("exit code 7"), "exit code not surfaced: {out}"); + assert!( + out.contains("stdout-marker"), + "stdout dropped on failure: {out}" + ); + assert!( + out.contains("stderr-marker"), + "stderr dropped on failure: {out}" + ); + } + + /// A missing executable exits 127; the surfaced result must carry the code + /// and the actionable "command not found / missing dependency" hint so the + /// agent recognises the dependency wall and adapts instead of looping. + #[cfg(not(windows))] + #[tokio::test] + async fn shell_missing_command_surfaces_127_with_dependency_hint() { + let tool = ShellTool::new( + test_security(AutonomyLevel::Full), + test_runtime(), + test_audit(), + ); + let result = tool + .execute(json!({"command": "this_binary_does_not_exist_xyz --version"})) + .await + .unwrap(); + assert!(result.is_error); + let out = result.output().to_lowercase(); + assert!(out.contains("127"), "exit code 127 not surfaced: {out}"); + assert!( + out.contains("command not found"), + "missing-dependency hint absent: {out}" + ); + } + fn test_security_with_env_cmd() -> Arc { Arc::new(SecurityPolicy { autonomy: AutonomyLevel::Supervised,