fix(shell): route sandbox execution through platform_shell so Windows spawns cmd.exe (#4705) (#4723)

Co-authored-by: M3gA-Mind <elvin@tinyhumans.ai>
This commit is contained in:
CodeGhost21
2026-07-14 20:38:42 +05:30
committed by GitHub
co-authored by M3gA-Mind
parent 574c22196f
commit cb9d5e6fa5
5 changed files with 354 additions and 71 deletions
+24 -55
View File
@@ -1,5 +1,6 @@
//! Native and Docker shell runtime adapters (`RuntimeAdapter` implementations).
use crate::openhuman::agent::platform_shell;
use crate::openhuman::config::RuntimeConfig;
use std::path::{Path, PathBuf};
@@ -77,30 +78,10 @@ impl RuntimeAdapter for NativeRuntime {
command: &str,
workspace_dir: &Path,
) -> anyhow::Result<tokio::process::Command> {
// On Windows hosts there is no POSIX `sh`; drive PowerShell instead.
// `-NoProfile` keeps startup fast and avoids user profile side effects.
let mut cmd = if cfg!(windows) {
let mut c = tokio::process::Command::new("powershell");
c.arg("-NoProfile").arg("-Command").arg(command);
c
} else if let Some(bash) = bash_path() {
// Prefer bash with `pipefail` so a failed stage in a pipeline (e.g.
// `pip install … | tail`) surfaces as a non-zero exit instead of
// being masked by the last stage's success. Without it the harness
// records the call as successful and the repeated-failure circuit
// breaker (`RepeatedToolFailureMiddleware`, tinyagents/middleware.rs)
// never trips, so the agent loops on a
// command that is silently failing. `/bin/sh` is dash on
// Debian/Ubuntu and rejects `set -o pipefail`, so this is gated on
// bash actually being present; otherwise we fall back to plain sh.
let mut c = tokio::process::Command::new(bash);
c.arg("-lc").arg(format!("set -o pipefail\n{command}"));
c
} else {
let mut c = tokio::process::Command::new("sh");
c.arg("-lc").arg(command);
c
};
// Shell selection is shared with the sandboxed execution paths in
// `sandbox::ops` so all three sites (native, unsandboxed, jailed)
// pick the same platform-appropriate shell (#4705).
let mut cmd = platform_shell::build_tokio_command(command);
// Validate the CWD up front so a missing/bad action_dir produces an
// actionable message naming the path, instead of an opaque OS error 267
// (ERROR_DIRECTORY) from CreateProcessW on Windows / a raw ENOENT on
@@ -138,21 +119,6 @@ fn maybe_hide_window(cmd: &mut tokio::process::Command, hide: bool) {
);
}
/// Locate a `bash` binary once (cached — this is hit on every shell call) for
/// the `pipefail` wrapper in [`NativeRuntime::build_shell_command`]. Returns
/// `None` on hosts without bash (e.g. minimal containers), where we fall back
/// to plain `sh` without pipefail.
fn bash_path() -> Option<&'static str> {
static BASH: std::sync::OnceLock<Option<String>> = std::sync::OnceLock::new();
BASH.get_or_init(|| {
["/usr/bin/bash", "/bin/bash"]
.into_iter()
.find(|p| Path::new(p).exists())
.map(str::to_string)
})
.as_deref()
}
pub struct DockerRuntime {
config: crate::openhuman::config::DockerRuntimeConfig,
}
@@ -257,8 +223,11 @@ mod tests {
assert_eq!(runtime.memory_budget(), 0);
assert!(runtime.storage_path().ends_with("openhuman/runtime"));
// Use a tempdir so `ensure_usable_cwd` accepts the path on every
// OS (`/tmp` does not exist on Windows).
let tempdir = tempfile::tempdir().unwrap();
let command = runtime
.build_shell_command("echo hi", Path::new("/tmp"))
.build_shell_command("echo hi", tempdir.path())
.unwrap();
let prog = command
.as_std()
@@ -270,19 +239,17 @@ mod tests {
.get_args()
.map(|arg| arg.to_string_lossy().into_owned())
.collect();
// NativeRuntime prefers bash with `set -o pipefail` when bash is present
// (so masked pipe failures surface), and falls back to plain `sh`.
if let Some(bash) = bash_path() {
assert_eq!(prog, bash);
assert_eq!(
args,
vec!["-lc".to_string(), "set -o pipefail\necho hi".to_string()]
);
// Shell selection is delegated to `platform_shell::build_tokio_command`
// — this test just asserts NativeRuntime is wired into it. The full
// per-platform matrix is covered by `platform_shell::tests`.
if cfg!(windows) {
assert_eq!(prog, "cmd");
assert_eq!(args, vec!["/C".to_string(), "echo hi".to_string()]);
} else {
assert_eq!(prog, "sh");
assert_eq!(args, vec!["-lc".to_string(), "echo hi".to_string()]);
assert!(prog.ends_with("bash") || prog == "sh");
assert_eq!(args.first().map(String::as_str), Some("-lc"));
}
assert_eq!(command.as_std().get_current_dir(), Some(Path::new("/tmp")));
assert_eq!(command.as_std().get_current_dir(), Some(tempdir.path()));
}
#[test]
@@ -359,16 +326,18 @@ mod tests {
let native = create_runtime(&RuntimeConfig::default(), true).unwrap();
assert_eq!(native.name(), "native");
// Tempdir so `ensure_usable_cwd` accepts it on Windows CI too.
let tempdir = tempfile::tempdir().unwrap();
let runtime = NativeRuntime::with_hide_window(true);
let command = runtime
.build_shell_command("echo hi", Path::new("/tmp"))
.build_shell_command("echo hi", tempdir.path())
.expect("hide_window should not break command construction");
assert_eq!(command.as_std().get_current_dir(), Some(Path::new("/tmp")));
assert_eq!(command.as_std().get_current_dir(), Some(tempdir.path()));
// The program/args are identical with and without the flag — hiding the
// window must not alter what is executed.
let plain = NativeRuntime::with_hide_window(false)
.build_shell_command("echo hi", Path::new("/tmp"))
.build_shell_command("echo hi", tempdir.path())
.unwrap();
assert_eq!(command.as_std().get_program(), plain.as_std().get_program());
}
@@ -394,7 +363,7 @@ mod tests {
#[cfg(unix)]
#[tokio::test]
async fn native_shell_pipefail_surfaces_failed_pipe_stage() {
if bash_path().is_none() {
if platform_shell::bash_path().is_none() {
return; // no bash → plain sh, pipefail unavailable
}
let rt = NativeRuntime::new();
+5
View File
@@ -29,6 +29,11 @@ pub mod host_runtime;
pub mod library;
pub mod multimodal;
pub mod pformat;
/// Cross-platform shell selection shared by [`host_runtime::NativeRuntime`]
/// and [`crate::openhuman::sandbox::ops`] so all three shell-spawning sites
/// agree on `cmd.exe` (Windows) vs `bash`/`sh` (Unix). Fixes #4705 where
/// the sandbox paths hardcoded `sh` and failed at spawn on Windows.
pub mod platform_shell;
pub mod progress;
/// Structured tracing export off the [`progress`] channel: turns the
/// real-time [`progress::AgentProgress`] stream into OpenTelemetry/
+219
View File
@@ -0,0 +1,219 @@
//! Cross-platform shell selection for spawning agent shell commands.
//!
//! Consolidates the "which shell binary do we spawn?" decision so
//! [`NativeRuntime::build_shell_command`](super::host_runtime::NativeRuntime)
//! and the sandbox execution paths in
//! [`crate::openhuman::sandbox::ops`] can share one Windows-aware
//! implementation. Prior to this module the sandbox paths hardcoded
//! `Command::new("sh")`, which fails at `CreateProcessW` on Windows in
//! ~30ms because `sh` is not in `PATH` (#4705).
//!
//! **Shell choice per platform:**
//!
//! - **Windows** → `cmd.exe /C <command>`. Chosen over PowerShell because
//! Windows users expect `%VAR%` expansion (`echo %USERPROFILE%`) and
//! byte-transparent `>` / `2>` redirection for the sandboxed output-
//! capture path in
//! [`crate::openhuman::sandbox::ops::execute_local_jail`]. PowerShell
//! 5.1's `>` writes UTF-16LE and does not expand `%VAR%`.
//! - **Unix** → `bash -lc "set -o pipefail\n<command>"` when bash is
//! available at `/usr/bin/bash` or `/bin/bash`, otherwise `sh -lc
//! <command>`. `set -o pipefail` surfaces a failed stage in a pipeline
//! (e.g. `pip install … | tail`) as a non-zero exit instead of being
//! masked by the last stage — without it the harness records the call
//! as successful and the repeated-failure circuit breaker
//! (`RepeatedToolFailureMiddleware`) never trips, so the agent loops
//! on a silently-failing command. `/bin/sh` is dash on Debian/Ubuntu
//! and rejects `set -o pipefail`, so this is gated on bash actually
//! being present; otherwise we fall back to plain sh.
use std::path::Path;
/// Build a [`tokio::process::Command`] that runs `command` under the
/// platform's default shell. Callers are responsible for setting
/// `current_dir`, environment, and stdio.
pub fn build_tokio_command(command: &str) -> tokio::process::Command {
let mut cmd = tokio::process::Command::new(shell_program());
// `as_std_mut()` so the Windows arm can reach `raw_arg` (only defined on
// `std::process::Command`); the tokio wrapper forwards the raw arg.
configure_shell_args(cmd.as_std_mut(), command);
cmd
}
/// [`std::process::Command`] variant for callers that hand the command
/// to [`crate::openhuman::cwd_jail::spawn`], which is built around
/// `std::process::Command` (not the tokio variant).
pub fn build_std_command(command: &str) -> std::process::Command {
let mut cmd = std::process::Command::new(shell_program());
configure_shell_args(&mut cmd, command);
cmd
}
/// Shell binary for the current platform. Single source of truth shared by
/// [`build_tokio_command`] and [`build_std_command`] — future changes to the
/// platform matrix (adding pwsh, changing pipefail semantics) belong here plus
/// [`configure_shell_args`], so both `Command` flavours stay in lockstep.
fn shell_program() -> &'static str {
if cfg!(windows) {
"cmd"
} else {
bash_path().unwrap_or("sh")
}
}
/// Append the shell flag + command payload to `cmd`.
///
/// On Windows the payload MUST go through `raw_arg`, not `arg`: Rust's `arg`
/// applies MSVCRT (`CommandLineToArgvW`) quoting, escaping any interior `"` as
/// `\"`. But `cmd.exe` does not understand `\"` — it only toggles quote state
/// on a bare `"`. Handed to `cmd /C` via `arg`, the `>` / `2>` operators in a
/// redirect wrap (see [`wrap_with_output_redirection`]) land inside a cmd
/// quote-span, so no redirection happens and the `.sandbox_stdout` /
/// `.sandbox_stderr` capture files are never written. `raw_arg` passes the
/// string to cmd verbatim, which is exactly the byte-transparent contract this
/// module promises. `/C` itself has no special characters.
#[cfg(windows)]
fn configure_shell_args(cmd: &mut std::process::Command, command: &str) {
use std::os::windows::process::CommandExt;
cmd.arg("/C").raw_arg(command);
}
/// Unix arm: `bash -lc "set -o pipefail\n<command>"` when bash is present
/// (so a masked pipe-stage failure still surfaces), else plain `sh -lc`.
#[cfg(not(windows))]
fn configure_shell_args(cmd: &mut std::process::Command, command: &str) {
if bash_path().is_some() {
cmd.arg("-lc").arg(format!("set -o pipefail\n{command}"));
} else {
cmd.arg("-lc").arg(command);
}
}
/// Wrap `command` so that stdout and stderr redirect to the given file
/// paths, using shell syntax compatible with the platform's default
/// shell as selected by [`build_tokio_command`] / [`build_std_command`].
///
/// Used by [`crate::openhuman::sandbox::ops::execute_local_jail`] to
/// capture output on backends (macOS Seatbelt) that rebuild the command
/// internally and don't forward piped stdio settings.
pub fn wrap_with_output_redirection(
command: &str,
stdout_path: &Path,
stderr_path: &Path,
) -> String {
if cfg!(windows) {
// cmd.exe has no `{ … }` command grouping, but `>`/`2>` bind to
// the whole /C payload when placed at the end, so a plain
// trailing redirect captures the full output for both single
// commands and pipelines. Double-quote paths so backslashes,
// spaces, and `(` / `)` inside typical Windows workspace paths
// (e.g. `C:\Program Files (x86)\…`) don't break parsing.
format!(
"{command} > \"{}\" 2> \"{}\"",
stdout_path.display(),
stderr_path.display()
)
} else {
// sh/bash need `{ … ; }` grouping so a semicolon- or pipe-
// separated multi-stage `command` routes *all* stages' output
// to the temp files. Without the group `a; b > out` would only
// redirect `b`. Single-quote paths so shell metacharacters in
// the workspace path stay literal.
format!(
"{{ {command} ; }} > '{}' 2> '{}'",
stdout_path.display(),
stderr_path.display()
)
}
}
/// Locate a `bash` binary once (cached — hit on every shell call) for
/// the `pipefail` wrapper. Returns `None` on hosts without bash at a
/// standard path (Windows, minimal containers), where we fall back to
/// plain `sh` without pipefail. Exposed `pub(crate)` so regression
/// tests in [`super::host_runtime`] can skip the pipefail assertions
/// on bash-less hosts.
pub(crate) fn bash_path() -> Option<&'static str> {
static BASH: std::sync::OnceLock<Option<String>> = std::sync::OnceLock::new();
BASH.get_or_init(|| {
["/usr/bin/bash", "/bin/bash"]
.into_iter()
.find(|p| Path::new(p).exists())
.map(str::to_string)
})
.as_deref()
}
#[cfg(test)]
mod tests {
use super::*;
use std::path::PathBuf;
#[test]
fn tokio_command_selects_platform_shell() {
let cmd = build_tokio_command("echo hi");
let prog = cmd.as_std().get_program().to_string_lossy().into_owned();
let args: Vec<String> = cmd
.as_std()
.get_args()
.map(|a| a.to_string_lossy().into_owned())
.collect();
if cfg!(windows) {
assert_eq!(prog, "cmd");
assert_eq!(args, vec!["/C".to_string(), "echo hi".to_string()]);
} else if let Some(bash) = bash_path() {
assert_eq!(prog, bash);
assert_eq!(
args,
vec!["-lc".to_string(), "set -o pipefail\necho hi".to_string()]
);
} else {
assert_eq!(prog, "sh");
assert_eq!(args, vec!["-lc".to_string(), "echo hi".to_string()]);
}
}
#[test]
fn std_command_selects_platform_shell() {
let cmd = build_std_command("echo hi");
let prog = cmd.get_program().to_string_lossy().into_owned();
let args: Vec<String> = cmd
.get_args()
.map(|a| a.to_string_lossy().into_owned())
.collect();
if cfg!(windows) {
assert_eq!(prog, "cmd");
assert_eq!(args, vec!["/C".to_string(), "echo hi".to_string()]);
} else if let Some(bash) = bash_path() {
assert_eq!(prog, bash);
assert_eq!(
args,
vec!["-lc".to_string(), "set -o pipefail\necho hi".to_string()]
);
} else {
assert_eq!(prog, "sh");
assert_eq!(args, vec!["-lc".to_string(), "echo hi".to_string()]);
}
}
#[test]
fn output_redirection_wraps_per_platform() {
let stdout = PathBuf::from("/tmp/openhuman/out.log");
let stderr = PathBuf::from("/tmp/openhuman/err.log");
let wrapped = wrap_with_output_redirection("echo hi", &stdout, &stderr);
if cfg!(windows) {
assert_eq!(
wrapped,
r#"echo hi > "/tmp/openhuman/out.log" 2> "/tmp/openhuman/err.log""#
);
} else {
assert_eq!(
wrapped,
"{ echo hi ; } > '/tmp/openhuman/out.log' 2> '/tmp/openhuman/err.log'"
);
}
}
}
+31 -5
View File
@@ -113,11 +113,19 @@ impl JailBackend for AppContainerBackend {
}
fn is_available(&self) -> bool {
// AppContainer is available on Windows 8+ and Server 2012+.
// We could probe `CreateAppContainerProfile`'s availability via
// GetProcAddress, but treating the target_os = "windows" cfg as
// good enough — supported Windows versions are all 10+.
true
// AppContainer is *reachable* on every supported Windows (8+ / Server
// 2012+), but the spawn path in `spawn_in_container` can't yet return
// a `std::process::Child` — after `CreateProcessW` succeeds it must
// reply `Err(io::ErrorKind::Unsupported)` (see the TODO / `Unsupported`
// return at the end of `spawn_in_container`). Reporting the backend as
// available anyway strands the successfully-spawned process on the
// caller side: `execute_local_jail` bubbles the `Err` up as a spawn
// failure and never `wait`s on the running `cmd.exe`, so it
// orphan-runs against the redirected stdout/stderr files. Report
// unavailable until `Child` bridging lands so `pick_backend()` /
// `execute_local_jail` route through `NoopBackend`, which returns a
// real waitable `Child`. (PR #4723 review — #4705.)
false
}
fn spawn(&self, jail: &Jail, cmd: Command) -> io::Result<Child> {
@@ -584,4 +592,22 @@ mod tests {
assert!(s.len() <= 70);
assert!(s.starts_with("openhuman."));
}
/// PR #4723 review — `AppContainerBackend::is_available()` must
/// stay `false` until the spawn path can return a
/// `std::process::Child`. Reporting available strands a
/// successfully-spawned `cmd.exe` process because `spawn_in_container`
/// currently answers `Err(Unsupported)` after `CreateProcessW`, and
/// callers (e.g. `execute_local_jail`) drop the child on the floor.
/// Flip back to `true` in the same commit that lands the
/// `OwnedHandle -> Child` bridge.
#[test]
fn appcontainer_backend_reports_unavailable_until_child_bridge_lands() {
assert!(
!AppContainerBackend::new().is_available(),
"AppContainer must report unavailable while its spawn path \
cannot yield a waitable std::process::Child — see #4705 / \
PR #4723 for the orphan-spawn hazard"
);
}
}
+75 -11
View File
@@ -7,11 +7,11 @@ use super::types::{
SandboxPolicy, SandboxStatus, ELEVATED_TOOLS,
};
use crate::openhuman::agent::harness::definition::SandboxMode;
use crate::openhuman::agent::platform_shell;
use crate::openhuman::config::RuntimeConfig;
use crate::openhuman::cwd_jail::{self, Jail, NoopBackend};
use std::collections::HashMap;
use std::path::Path;
use std::process::Command;
use std::time::Duration;
/// Safe environment variables forwarded into sandboxed execution.
@@ -162,8 +162,10 @@ async fn execute_unsandboxed(
extra_env: &HashMap<String, String>,
timeout: Duration,
) -> anyhow::Result<SandboxExecResult> {
let mut cmd = tokio::process::Command::new("sh");
cmd.arg("-c").arg(command);
// Shell selection routed through `platform_shell` so this path picks
// `cmd.exe /C` on Windows instead of the non-existent `sh` binary
// (#4705 — Windows Shell tool spawn-failed at ~30ms).
let mut cmd = platform_shell::build_tokio_command(command);
cmd.current_dir(working_dir);
cmd.env_clear();
for var in SANDBOX_ENV_PASSTHROUGH {
@@ -216,14 +218,11 @@ async fn execute_local_jail(
let stdout_file = policy.workspace_root.join(".sandbox_stdout");
let stderr_file = policy.workspace_root.join(".sandbox_stderr");
let wrapped = format!(
"{{ {command} ; }} > '{}' 2> '{}'",
stdout_file.display(),
stderr_file.display()
);
let mut cmd = Command::new("sh");
cmd.arg("-c").arg(&wrapped);
// Platform-aware output-capture wrap: `{ … ; } > … 2> …` on sh/bash,
// trailing `> … 2> …` on cmd.exe (no brace grouping). Shell binary is
// picked by `platform_shell` so this path is Windows-safe (#4705).
let wrapped = platform_shell::wrap_with_output_redirection(command, &stdout_file, &stderr_file);
let mut cmd = platform_shell::build_std_command(&wrapped);
cmd.current_dir(working_dir);
cmd.env_clear();
for var in SANDBOX_ENV_PASSTHROUGH {
@@ -423,6 +422,12 @@ mod tests {
assert_eq!(handle.status, SandboxStatus::Ready);
}
// The `/tmp` path and Unix builtins (`false`) are Unix-only, so these
// integration-style tests are gated to Unix. A cross-platform
// `execute_unsandboxed_echo_runs_on_every_os` below exercises the same
// code path on Windows CI (#4705) — that is the primary regression
// guard for the `sh` → platform-aware shell fix.
#[cfg(unix)]
#[tokio::test]
async fn execute_unsandboxed_echo() {
let result = execute_unsandboxed(
@@ -438,6 +443,7 @@ mod tests {
assert!(!result.timed_out);
}
#[cfg(unix)]
#[tokio::test]
async fn execute_unsandboxed_failure() {
let result = execute_unsandboxed(
@@ -451,6 +457,38 @@ mod tests {
assert_ne!(result.exit_code, 0);
}
/// #4705 regression — every OS. `execute_unsandboxed` used to
/// `Command::new("sh")`, which fails at `CreateProcessW` on Windows
/// in ~30ms because `sh` is not in PATH. `echo hello` and `exit 1`
/// are shell builtins on both `cmd.exe` and `sh`/`bash`, so this
/// exercises the real code path on Windows CI as well as Unix.
#[tokio::test]
async fn execute_unsandboxed_echo_runs_on_every_os() {
let tempdir = tempfile::tempdir().unwrap();
let result = execute_unsandboxed(
"echo hello",
tempdir.path(),
&HashMap::new(),
Duration::from_secs(10),
)
.await
.unwrap();
assert_eq!(result.exit_code, 0, "stderr: {}", result.stderr);
assert!(result.stdout.contains("hello"));
assert!(!result.timed_out);
let failing = execute_unsandboxed(
"exit 1",
tempdir.path(),
&HashMap::new(),
Duration::from_secs(10),
)
.await
.unwrap();
assert_ne!(failing.exit_code, 0);
}
#[cfg(unix)]
#[tokio::test]
async fn execute_in_sandbox_none_backend() {
let policy = resolve_sandbox_policy(
@@ -472,6 +510,32 @@ mod tests {
assert!(result.stdout.contains("sandbox-test"));
}
/// #4705 regression — `execute_in_sandbox` with the `None` backend
/// now delegates to `execute_unsandboxed`, which used to fail on
/// Windows with a ~30ms `sh`-not-found spawn error. Cross-platform
/// so both Unix and Windows CI catch a shell-selection regression.
#[tokio::test]
async fn execute_in_sandbox_none_backend_runs_on_every_os() {
let tempdir = tempfile::tempdir().unwrap();
let policy = resolve_sandbox_policy(
SandboxMode::None,
tempdir.path(),
&RuntimeConfig::default(),
false,
);
let result = execute_in_sandbox(
&policy,
"echo sandbox-test",
tempdir.path(),
HashMap::new(),
Duration::from_secs(10),
)
.await
.unwrap();
assert!(result.success(), "stderr: {}", result.stderr);
assert!(result.stdout.contains("sandbox-test"));
}
#[test]
fn env_passthrough_includes_safe_vars() {
assert!(SANDBOX_ENV_PASSTHROUGH.contains(&"PATH"));