fix(code_executor): surface command exit code + both streams so the agent stops looping (#4095) (#4128)

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();

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
 

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<i32>, 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<i32>, stdout: &str, stderr: &str) -> ToolResult {
+    ToolResult::error(render_command_failure(exit_code, stdout, stderr))
+}
+
+/// Normalise a sandbox backend's `exit_code` into the `Option<i32>` 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<i32> {
+    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));
+    }
+}

src/openhuman/tools/impl/system/mod.rs

@@ -1,3 +1,4 @@
+mod command_output;
 mod current_time;
 mod detect_tools;
 mod insert_sql_record;

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}")),

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}")),

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<SecurityPolicy> {
         Arc::new(SecurityPolicy {
             autonomy: AutonomyLevel::Supervised,