fix(shell): route sandbox execution through platform_shell so Windows spawns cmd.exe (#4705) #4723
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| //! 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 (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) { | ||
| ("cmd", ["/C".to_string(), command.to_string()]) | ||
| } else if let Some(bash) = bash_path() { | ||
| ( | ||
| bash, | ||
| ["-lc".to_string(), format!("set -o pipefail\n{command}")], | ||
|
Collaborator
There was a problem hiding this comment. 💡 Question / minor — behavior change on the sandbox path: Previously
If parity with the old non-login sandbox behavior is desired, consider a non-login variant ( |
||
| ) | ||
| } else { | ||
| ("sh", ["-lc".to_string(), command.to_string()]) | ||
| } | ||
| } | ||
|
|
||
| /// 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!( | ||
|
Collaborator
There was a problem hiding this comment.
|
||
| "{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() | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| #[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'" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd /Creceives the command viaCommand::arg(), whose MSVCRT-style quoting mangles embedded double-quotes thatcmd.exedoes not understand.Rust's
std/tokioCommand::argon Windows quotes any arg containing whitespace and escapes interior"as\"(theCommandLineToArgvWconvention).cmd.exedoes not parse\"— it only toggles quote state on bare". So a command handed tocmd /Cas a single arg is re-parsed by cmd with the wrong quote boundaries.Two consequences:
wrap_with_output_redirection(below) always produces... > "out" 2> "err". Afterarg()escaping the line becomescmd /C "echo hi > \"out\" 2> \"err\"". cmd sees the>inside a quoted span, treats it as a literal, and performs no redirection — the.sandbox_stdout/.sandbox_stderrfiles stay empty andexecute_local_jailreturns empty output. This is exactly theSandboxMode::Sandboxed→ Local path the tinyflowscodenode uses on Windows, so the PR's headline Windows fix silently returns no output there."breaks even on the non-redirect path (echo "hello world"→ cmd runsecho \hello world\).Fix: pass the payload to
cmdverbatim viaraw_argon Windows:Since
select_shell_program_and_argsreturns(program, [String;2])andraw_argis Windows-only, this likely means branching the twobuild_*fns oncfg!(windows)rather than sharing the tuple helper. Please verify on real Windows — the current unit tests only assert the arg vector, never spawn cmd with a quoted/redirected command.