From 736666ceb1a62e2a766f6e227c6f06cda32daead Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 9 Jul 2026 12:23:33 +0530 Subject: [PATCH 1/5] fix(shell): route sandbox execution through platform_shell so Windows spawns cmd.exe (#4705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `execute_unsandboxed` and `execute_local_jail` in `src/openhuman/sandbox/ops.rs` hardcoded `Command::new("sh")`, which fails at `CreateProcessW` on Windows in ~30ms because `sh` is not in `PATH`. Any Sandboxed-mode agent (and the `tinyflows` `code` node, which always runs Sandboxed) hit this and reported "Failed 34ms" as soon as the shell tool was invoked. Consolidate all three shell-spawning sites (`NativeRuntime`, `execute_unsandboxed`, `execute_local_jail`) behind a new `openhuman::agent::platform_shell` module: - `build_tokio_command` / `build_std_command` — `cmd.exe /C` on Windows, `bash -lc "set -o pipefail\n"` when bash is present on Unix, `sh -lc` fallback otherwise. - `wrap_with_output_redirection` — POSIX `{ ...; } > 'out' 2> 'err'` on Unix, plain trailing ` > "out" 2> "err"` on Windows (cmd.exe has no brace grouping). `NativeRuntime` on Windows moves from PowerShell to `cmd.exe`, per the reporter's acceptance criteria: this restores `%VAR%` expansion (`echo %USERPROFILE%`), gives byte-transparent `>`/`2>` for the sandbox output-capture path, and matches the POSIX `sh -c` contract users expect from the shell tool. Existing tests that assumed Unix (`/tmp`, `false` builtin) are `#[cfg(unix)]` and paired with cross-platform equivalents using `tempfile::tempdir()` and shell-builtin `echo hello`/`exit 1` so the Windows CI lane exercises the real spawn path. Closes #4705 --- src/openhuman/agent/host_runtime.rs | 79 ++++------- src/openhuman/agent/mod.rs | 5 + src/openhuman/agent/platform_shell.rs | 197 ++++++++++++++++++++++++++ src/openhuman/sandbox/ops.rs | 86 +++++++++-- 4 files changed, 301 insertions(+), 66 deletions(-) create mode 100644 src/openhuman/agent/platform_shell.rs diff --git a/src/openhuman/agent/host_runtime.rs b/src/openhuman/agent/host_runtime.rs index 956b6e2b34..ee617c56f4 100644 --- a/src/openhuman/agent/host_runtime.rs +++ b/src/openhuman/agent/host_runtime.rs @@ -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 { - // 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> = 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(); diff --git a/src/openhuman/agent/mod.rs b/src/openhuman/agent/mod.rs index 1621f46dde..07341a6dfb 100644 --- a/src/openhuman/agent/mod.rs +++ b/src/openhuman/agent/mod.rs @@ -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/ diff --git a/src/openhuman/agent/platform_shell.rs b/src/openhuman/agent/platform_shell.rs new file mode 100644 index 0000000000..cb3f0b72d0 --- /dev/null +++ b/src/openhuman/agent/platform_shell.rs @@ -0,0 +1,197 @@ +//! 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 `. 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"` when bash is +//! available at `/usr/bin/bash` or `/bin/bash`, otherwise `sh -lc +//! `. `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 { + if cfg!(windows) { + let mut c = tokio::process::Command::new("cmd"); + c.arg("/C").arg(command); + c + } else if let Some(bash) = bash_path() { + 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 + } +} + +/// [`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 { + if cfg!(windows) { + let mut c = std::process::Command::new("cmd"); + c.arg("/C").arg(command); + c + } else if let Some(bash) = bash_path() { + let mut c = std::process::Command::new(bash); + c.arg("-lc").arg(format!("set -o pipefail\n{command}")); + c + } else { + let mut c = std::process::Command::new("sh"); + c.arg("-lc").arg(command); + c + } +} + +/// 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> = 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 = 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 = 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'" + ); + } + } +} diff --git a/src/openhuman/sandbox/ops.rs b/src/openhuman/sandbox/ops.rs index 2dd89eeaf3..b4b8f87aeb 100644 --- a/src/openhuman/sandbox/ops.rs +++ b/src/openhuman/sandbox/ops.rs @@ -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, timeout: Duration, ) -> anyhow::Result { - 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")); From 2b6aaf7df9e1de66e284395d47f6c39399a43d3c Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 9 Jul 2026 12:42:34 +0530 Subject: [PATCH 2/5] refactor(platform_shell): extract select_shell_program_and_args helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the PR #4723 review nitpick — the Windows/bash/sh branching was duplicated between `build_tokio_command` and `build_std_command`. Fold it into a private `select_shell_program_and_args` helper so both builders only differ in the `Command` type they construct, and future shell-policy changes (pwsh support, pipefail tweaks) touch one place. --- src/openhuman/agent/platform_shell.rs | 43 +++++++++++++-------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/openhuman/agent/platform_shell.rs b/src/openhuman/agent/platform_shell.rs index cb3f0b72d0..5a6aadb50d 100644 --- a/src/openhuman/agent/platform_shell.rs +++ b/src/openhuman/agent/platform_shell.rs @@ -33,37 +33,36 @@ use std::path::Path; /// platform's default shell. Callers are responsible for setting /// `current_dir`, environment, and stdio. pub fn build_tokio_command(command: &str) -> tokio::process::Command { - if cfg!(windows) { - let mut c = tokio::process::Command::new("cmd"); - c.arg("/C").arg(command); - c - } else if let Some(bash) = bash_path() { - 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 - } + let (program, args) = select_shell_program_and_args(command); + let mut cmd = tokio::process::Command::new(program); + cmd.args(args); + 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 (program, args) = select_shell_program_and_args(command); + let mut cmd = std::process::Command::new(program); + cmd.args(args); + cmd +} + +/// Single source of truth for the shell-selection policy shared by +/// [`build_tokio_command`] and [`build_std_command`] — future changes +/// to the platform matrix (adding pwsh, changing pipefail semantics) +/// belong here, so both `Command` flavours stay in lockstep. +fn select_shell_program_and_args(command: &str) -> (&'static str, [String; 2]) { if cfg!(windows) { - let mut c = std::process::Command::new("cmd"); - c.arg("/C").arg(command); - c + ("cmd", ["/C".to_string(), command.to_string()]) } else if let Some(bash) = bash_path() { - let mut c = std::process::Command::new(bash); - c.arg("-lc").arg(format!("set -o pipefail\n{command}")); - c + ( + bash, + ["-lc".to_string(), format!("set -o pipefail\n{command}")], + ) } else { - let mut c = std::process::Command::new("sh"); - c.arg("-lc").arg(command); - c + ("sh", ["-lc".to_string(), command.to_string()]) } } From f953026f7ca6449bf1cccbf6a5bf830fbc2432a1 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 9 Jul 2026 13:04:24 +0530 Subject: [PATCH 3/5] fix(cwd_jail): report Windows AppContainer unavailable until Child bridge lands (PR #4723 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AppContainerBackend::spawn_in_container` calls `CreateProcessW` successfully, then returns `Err(io::ErrorKind::Unsupported)` because it can't yet bridge `OwnedHandle` to `std::process::Child` (see the TODO at ~line 279). Prior to this PR the sandbox Windows path spawned non-existent `sh`, so `CreateProcessW` failed early and no orphan was created — the AppContainer wart was masked. After the platform_shell fix, `cmd.exe` spawns for real, the Err surfaces to `execute_local_jail`, and the running `cmd.exe` gets stranded (writing to redirected stdout/stderr files while the caller reports "spawn failed" and skips wait/timeout/output). Flip `AppContainerBackend::is_available()` to `false` so `pick_backend()` (and `execute_local_jail`'s fallback branch) route through `NoopBackend`, which returns a real waitable `Child`. Reverting to `true` should land in the same commit that adds the `OwnedHandle -> std::process::Child` bridge. --- src/openhuman/cwd_jail/windows.rs | 36 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/src/openhuman/cwd_jail/windows.rs b/src/openhuman/cwd_jail/windows.rs index 9f6989f6b6..e529b7e251 100644 --- a/src/openhuman/cwd_jail/windows.rs +++ b/src/openhuman/cwd_jail/windows.rs @@ -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 TODO at + // `spawn_in_container`, ~line 279). 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 { @@ -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" + ); + } } From 62e86ae6fb58debefa1643bcdb8d77d9b9f29693 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 9 Jul 2026 17:06:14 +0530 Subject: [PATCH 4/5] fix(voice): restore hotkey imports removed by #4720 that broke Linux clippy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `voice/dictation_listener.rs` still references `hotkey::parse_hotkey`, `hotkey::start_listener`, `ActivationMode::{Push,Tap}`, and `HotkeyEvent::{Pressed,Released}` inside the `#[cfg(not(target_os = "macos"))]` `start_rdev_listener` path, but #4720 removed the top-level `use crate::openhuman::voice::hotkey::{ self, ActivationMode, HotkeyEvent};` as "unused" — a bad rustc read that only shows up when the non-macOS branch is compiled. The Linux `Rust Quality (fmt, clippy)` job on `ci-lite.yml` has been red on every merge to main for the last five runs as a result, blocking all PRs (including #4723 this ships in). Restore the import behind a matching `#[cfg(not(target_os = "macos"))]` gate so rustc doesn't emit an `unused_imports` warning on macOS, keeping the previous macOS-clean behaviour while unblocking the Linux clippy lane. Unblocks #4723 / #4705. --- src/openhuman/voice/dictation_listener.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/openhuman/voice/dictation_listener.rs b/src/openhuman/voice/dictation_listener.rs index a3334b47d5..a09133e084 100644 --- a/src/openhuman/voice/dictation_listener.rs +++ b/src/openhuman/voice/dictation_listener.rs @@ -13,6 +13,13 @@ use tokio::sync::broadcast; use tokio::task::JoinHandle; use crate::openhuman::config::Config; +// `hotkey::{self, ActivationMode, HotkeyEvent}` is only referenced from the +// non-macOS `start_rdev_listener` path; gate the `use` on the same cfg so +// rustc doesn't complain about an unused import on macOS. Restoring the +// import unblocks Linux clippy after #4720 removed it as "unused" while +// leaving the actual usage in place. +#[cfg(not(target_os = "macos"))] +use crate::openhuman::voice::hotkey::{self, ActivationMode, HotkeyEvent}; const LOG_PREFIX: &str = "[dictation_listener]"; From 7ca176677de1f0ddb2e25afd8bc72c752d47691c Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 14 Jul 2026 19:57:35 +0530 Subject: [PATCH 5/5] fix(shell): pass cmd.exe payload via raw_arg so Windows redirects work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (PR #4723): on Windows, cmd /C was built with Command::arg(), whose MSVCRT (CommandLineToArgvW) quoting escapes interior double-quotes as \" — which cmd.exe does not understand (it toggles quote state only on a bare "). The redirect wrap ' > "out" 2> "err"' was therefore re-parsed with the >/2> operators inside a cmd quote-span, so no redirection happened and the .sandbox_stdout/.sandbox_stderr capture files were never written on the local- jail path. Route the payload through raw_arg (verbatim, no escaping); the Unix arm is byte-identical. Also drop a rotting line-number reference in the cwd_jail AppContainer is_available() comment. --- src/openhuman/agent/platform_shell.rs | 59 +++++++++++++++++++-------- src/openhuman/cwd_jail/windows.rs | 4 +- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/openhuman/agent/platform_shell.rs b/src/openhuman/agent/platform_shell.rs index 5a6aadb50d..fae040fb30 100644 --- a/src/openhuman/agent/platform_shell.rs +++ b/src/openhuman/agent/platform_shell.rs @@ -33,9 +33,10 @@ use std::path::Path; /// 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 (program, args) = select_shell_program_and_args(command); - let mut cmd = tokio::process::Command::new(program); - cmd.args(args); + 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 } @@ -43,26 +44,48 @@ pub fn build_tokio_command(command: &str) -> tokio::process::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 (program, args) = select_shell_program_and_args(command); - let mut cmd = std::process::Command::new(program); - cmd.args(args); + let mut cmd = std::process::Command::new(shell_program()); + configure_shell_args(&mut cmd, command); cmd } -/// Single source of truth for the shell-selection policy shared by -/// [`build_tokio_command`] and [`build_std_command`] — future changes -/// to the platform matrix (adding pwsh, changing pipefail semantics) -/// belong here, so both `Command` flavours stay in lockstep. -fn select_shell_program_and_args(command: &str) -> (&'static str, [String; 2]) { +/// 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", ["/C".to_string(), command.to_string()]) - } else if let Some(bash) = bash_path() { - ( - bash, - ["-lc".to_string(), format!("set -o pipefail\n{command}")], - ) + "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"` 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 { - ("sh", ["-lc".to_string(), command.to_string()]) + cmd.arg("-lc").arg(command); } } diff --git a/src/openhuman/cwd_jail/windows.rs b/src/openhuman/cwd_jail/windows.rs index e529b7e251..743b67a003 100644 --- a/src/openhuman/cwd_jail/windows.rs +++ b/src/openhuman/cwd_jail/windows.rs @@ -116,8 +116,8 @@ impl JailBackend for AppContainerBackend { // 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 TODO at - // `spawn_in_container`, ~line 279). Reporting the backend as + // 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