fix(shell): route sandbox execution through platform_shell so Windows spawns cmd.exe (#4705)#4723
Conversation
… spawns cmd.exe (tinyhumansai#4705) 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<cmd>"` when bash is present on Unix, `sh -lc` fallback otherwise. - `wrap_with_output_redirection` — POSIX `{ ...; } > 'out' 2> 'err'` on Unix, plain trailing `<cmd> > "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 tinyhumansai#4705
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA shared ChangesPlatform shell centralization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/agent/platform_shell.rs (1)
35-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate shell-selection branching between the tokio and std builders.
build_tokio_commandandbuild_std_commandrepeat identical Windows/bash/sh selection logic, differing only in theCommandtype constructed. If the shell-selection policy changes in the future (e.g. new fallback), it's easy to update one and forget the other.♻️ Proposed refactor to share selection logic
+fn select_shell(command: &str) -> (&'static str, Vec<String>) { + if cfg!(windows) { + ("cmd", vec!["/C".to_string(), command.to_string()]) + } else if let Some(bash) = bash_path() { + (bash, vec!["-lc".to_string(), format!("set -o pipefail\n{command}")]) + } else { + ("sh", vec!["-lc".to_string(), command.to_string()]) + } +} + 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 (prog, args) = select_shell(command); + let mut c = tokio::process::Command::new(prog); + c.args(args); + c } 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 - } + let (prog, args) = select_shell(command); + let mut c = std::process::Command::new(prog); + c.args(args); + c }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/platform_shell.rs` around lines 35 - 68, The shell-selection logic is duplicated in build_tokio_command and build_std_command, so refactor the shared Windows/bash/sh branching into a single helper or shared path-selection function and have both builders reuse it while only differing in the Command type they construct. Keep the current behavior of cfg!(windows), bash_path(), and the pipefail command wrapping identical in both code paths, and ensure any future shell policy changes only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/agent/platform_shell.rs`:
- Around line 1-123: This new platform_shell module violates the project’s Rust
module layout rule by living as a root-level .rs file under openhuman/agent.
Move the functionality into a dedicated openhuman/agent/platform_shell/
subdirectory with a mod.rs entry point and sibling module(s), and keep mod.rs as
a thin re-export layer. Preserve the existing symbols build_tokio_command,
build_std_command, wrap_with_output_redirection, and bash_path in the new module
structure so callers in NativeRuntime and sandbox::ops continue to resolve them
unchanged.
---
Nitpick comments:
In `@src/openhuman/agent/platform_shell.rs`:
- Around line 35-68: The shell-selection logic is duplicated in
build_tokio_command and build_std_command, so refactor the shared
Windows/bash/sh branching into a single helper or shared path-selection function
and have both builders reuse it while only differing in the Command type they
construct. Keep the current behavior of cfg!(windows), bash_path(), and the
pipefail command wrapping identical in both code paths, and ensure any future
shell policy changes only need to be made in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e2d678e-a4a4-4aa2-852c-2db7a52dd62e
📒 Files selected for processing (4)
src/openhuman/agent/host_runtime.rssrc/openhuman/agent/mod.rssrc/openhuman/agent/platform_shell.rssrc/openhuman/sandbox/ops.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 736666ceb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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); |
There was a problem hiding this comment.
When a Windows desktop agent runs with SandboxMode::Sandboxed, resolve_sandbox_policy selects the Local backend, and this new cmd.exe command is spawnable by cwd_jail's AppContainer backend. But src/openhuman/cwd_jail/windows.rs:283-287 still returns Unsupported after CreateProcessW succeeds because it cannot produce a std::process::Child, so execute_local_jail reports a spawn failure and skips timeout/output handling even though the command may already be running and writing the redirected files. This turns the Windows fix into side-effecting false failures/retries; fallback to NoopBackend for this backend or keep AppContainer unavailable until it can return a waitable child.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in commit f953026.
You're right — before this PR, Command::new("sh") failed inside spawn_in_container's CreateProcessW call (sh not in PATH), so the Err was clean and no process leaked. After the platform_shell fix, cmd.exe actually spawns, the Err(Unsupported) from the missing OwnedHandle → Child bridge surfaces to execute_local_jail, and the caller reports "spawn failed" while a real cmd.exe keeps running against the redirected stdout/stderr files.
Fix: flipped AppContainerBackend::is_available() to false (with a long comment referencing the TODO at spawn_in_container:~279 and this PR). pick_backend() on Windows now falls through to NoopBackend, which returns a real waitable Child, so execute_local_jail's wait/timeout/output-capture path works. Regression test appcontainer_backend_reports_unavailable_until_child_bridge_lands in windows.rs pins the invariant — the same commit that adds the Child bridge should flip both the flag and the test in one go.
Went with your second option ("keep AppContainer unavailable until it can return a waitable child") rather than a targeted fallback in execute_local_jail — the underlying issue is a backend advertising a capability it can't actually deliver, so fixing it at the backend keeps every future caller of pick_backend() / default_backend() (registry, potential other callers) honest without needing per-caller workarounds.
There was a problem hiding this comment.
Confirming this P1 is correctly addressed by commit f953026. Flipping AppContainerBackend::is_available() to false makes pick_backend() fall through to NoopBackend, whose spawn returns a real waitable std::process::Child, so execute_local_jail's wait/timeout/output path no longer strands a running cmd.exe. The regression test appcontainer_backend_reports_unavailable_until_child_bridge_lands pins the invariant. Good fix at the backend level rather than a per-caller workaround.
One remaining Windows hazard on this same execute_local_jail path (separate from the orphan issue): the redirect wrap is passed to cmd /C via Command::arg(), whose quote-escaping breaks cmd's redirection parsing so the captured output files end up empty — see the inline comment on platform_shell.rs.
Address the PR tinyhumansai#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.
…idge lands (PR tinyhumansai#4723 P1) `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.
|
Fixed in 2b6aaf7. Extracted |
…broke Linux clippy
`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
tinyhumansai#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 tinyhumansai#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 tinyhumansai#4723 / tinyhumansai#4705.
| /// 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()]) |
There was a problem hiding this comment.
cmd /C receives the command via Command::arg(), whose MSVCRT-style quoting mangles embedded double-quotes that cmd.exe does not understand.
Rust's std/tokio Command::arg on Windows quotes any arg containing whitespace and escapes interior " as \" (the CommandLineToArgvW convention). cmd.exe does not parse \" — it only toggles quote state on bare ". So a command handed to cmd /C as a single arg is re-parsed by cmd with the wrong quote boundaries.
Two consequences:
- Redirect wrap always breaks.
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. - Any command containing
"breaks even on the non-redirect path (echo "hello world"→ cmd runsecho \hello world\).
Fix: pass the payload to cmd verbatim via raw_arg on Windows:
// build_tokio_command / build_std_command, Windows arm
use std::os::windows::process::CommandExt;
let mut cmd = Command::new("cmd");
cmd.raw_arg("/C").raw_arg(command); // no MSVCRT re-quotingSince select_shell_program_and_args returns (program, [String;2]) and raw_arg is Windows-only, this likely means branching the two build_* fns on cfg!(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.
| // 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!( |
There was a problem hiding this comment.
raw_arg comment above). This wrap is the concrete place the quoting bug bites: the produced string always contains double-quoted paths (> "…" 2> "…"), and it is spawned via build_std_command → cmd /C <arg> whose Command::arg() escaping puts the >/2> inside a cmd quote-span, so no redirection happens and the stdout/stderr temp files are never written. On Windows this path is reached for SandboxMode::Sandboxed local agents (and the tinyflows code node). Blocking factor is the same raw_arg fix on the Windows arm; additionally, the Windows branch here formats the redirect paths with .display(), which on real Windows yields backslash paths (C:\\Users\\…) — those need to survive verbatim too, reinforcing the raw-arg approach. Worth an execution test (execute_local_jail on Windows CI actually reading back captured output), not just the current string-equality assertion.
| } else if let Some(bash) = bash_path() { | ||
| ( | ||
| bash, | ||
| ["-lc".to_string(), format!("set -o pipefail\n{command}")], |
There was a problem hiding this comment.
💡 Question / minor — behavior change on the sandbox path: sh -c → login shell bash -lc.
Previously execute_unsandboxed and execute_local_jail used sh -c <cmd> (non-login). Routing through platform_shell now runs bash -lc "set -o pipefail\n<cmd>" on Unix. Two side effects worth confirming are intended:
-lsources login profile scripts (/etc/profile, and sinceHOMEis inSANDBOX_ENV_PASSTHROUGH,~/.bash_profile/~/.profile). Anyecho/banner in those files lands in the captured.sandbox_stdoutfor the jail path, corrupting the tool's reported output.- Login-profile sourcing adds per-call latency and can rewrite
PATHout from under the curatedSANDBOX_ENV_PASSTHROUGHset.
If parity with the old non-login sandbox behavior is desired, consider a non-login variant (-c with an explicit set -o pipefail prefix) for the sandbox callers, or document that the sandbox intentionally adopts NativeRuntime's login-shell semantics.
| // 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 |
There was a problem hiding this comment.
Nitpick. The comment points at spawn_in_container, ~line 279 but the Err(Unsupported) return / TODO is at line ~287–295 in the current file. Minor drift; consider referencing the function name only (line numbers rot) — e.g. "see the Err(Unsupported) return at the end of spawn_in_container".
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4723 — fix(shell): route sandbox execution through platform_shell so Windows spawns cmd.exe (#4705)
Walkthrough
Consolidates the "which shell binary do we spawn?" decision into a new src/openhuman/agent/platform_shell.rs shared by NativeRuntime::build_shell_command and both sandbox execution paths (execute_unsandboxed, execute_local_jail). Windows moves from the previously-hardcoded sh (sandbox) / PowerShell (native) to cmd.exe /C; Unix keeps bash -lc "set -o pipefail…" with sh -lc fallback. It also flips AppContainerBackend::is_available() to false (fixing the orphan-spawn P1 that the shell fix would otherwise expose) and restores a cfg-gated hotkey import to unbreak Linux clippy.
Overall this is a well-scoped, well-tested fix that correctly resolves the reported sh-not-found spawn failure on Windows for the default (None-backend) shell path, and the AppContainer + dictation sub-fixes are sound. The main gap is that the Windows cmd /C <command> invocation is built with Command::arg(), whose MSVCRT quoting is not understood by cmd.exe — which breaks the local-jail redirect capture and any command containing quotes. Leaving this as COMMENT for the maintainer to weigh; not blocking-by-fiat.
Changes
| File | Summary |
|---|---|
src/openhuman/agent/platform_shell.rs |
New module: build_tokio_command / build_std_command / wrap_with_output_redirection / bash_path, single-source shell selection + tests. |
src/openhuman/agent/mod.rs |
Register pub mod platform_shell with doc comment. |
src/openhuman/agent/host_runtime.rs |
NativeRuntime::build_shell_command delegates to platform_shell; bash_path moved out; tests use tempdirs + platform-aware asserts. |
src/openhuman/sandbox/ops.rs |
execute_unsandboxed / execute_local_jail swap Command::new("sh") for platform_shell; new cross-OS regression tests; Unix-only gates on the /tmp-based tests. |
src/openhuman/cwd_jail/windows.rs |
AppContainerBackend::is_available() → false until a waitable Child bridge lands; regression test. |
src/openhuman/voice/dictation_listener.rs |
Restore #[cfg(not(target_os = "macos"))] hotkey import removed by #4720 (Linux clippy). |
Actionable comments (4)
⚠️ Major
1. src/openhuman/agent/platform_shell.rs:56-66 & :81-103 — cmd /C <cmd> built with Command::arg() mangles quotes cmd.exe can't parse
Rust's Windows Command::arg escapes interior " as \" (the CommandLineToArgvW convention); cmd.exe does not honor \". So:
- The local-jail redirect wrap is always affected —
wrap_with_output_redirectionalways emits> "out" 2> "err"; afterarg()escaping cmd sees the>inside a quoted span and performs no redirection, leaving.sandbox_stdout/.sandbox_stderrempty. This is theSandboxMode::Sandboxed→Local path (tinyflowscodenode) — the very thing the PR aims to fix on Windows. - Any command containing
"is affected on the non-redirect path too.
Suggested change (Windows arm of the build_* fns):
use std::os::windows::process::CommandExt;
let mut cmd = Command::new("cmd");
cmd.raw_arg("/C").raw_arg(command); // verbatim — no MSVCRT re-quotingPlease verify on real Windows; current tests only assert the arg vector, never spawn cmd with a quoted/redirected command. (Confidence: high on the escaping mechanism; the exact cmd breakage should be confirmed on hardware.)
💡 Question / minor
2. src/openhuman/agent/platform_shell.rs:60-62 — sandbox paths now use a login shell
execute_unsandboxed / execute_local_jail previously ran sh -c (non-login); they now run bash -lc. With HOME in SANDBOX_ENV_PASSTHROUGH, -l sources ~/.bash_profile//etc/profile, whose banner output would pollute captured .sandbox_stdout and whose PATH edits override the curated passthrough set. Confirm intended, or use a non-login -c with an explicit set -o pipefail prefix for the sandbox callers.
Nitpicks (2)
src/openhuman/cwd_jail/windows.rs:119— comment says~line 279but theErr(Unsupported)return is ~287-295; prefer referencing the function name, line numbers rot.src/openhuman/agent/platform_shell.rs:178-193(output_redirection_wraps_per_platform) — the Windows assertion uses forward-slash/tmp/...paths, so it never exercises realistic backslash Windows paths (where the quoting issue in #1 surfaces). Consider aC:\...-style path plus an actual execution test.
Verified / looks good
- AppContainer P1 (Codex) correctly resolved.
is_available() → falseroutes Windows Local sandbox throughNoopBackend, which returns a real waitableChild; the previously-hardcodedshhad masked this because it failed insideCreateProcessW. Regression test pins the invariant. Good backend-level fix. - Dictation cfg gate is correct. The
hotkey/ActivationMode/HotkeyEventimport and its only usage (start_rdev_listener) are both#[cfg(not(target_os = "macos"))], so this compiles on all three OSes and unbreaks Linux clippy. - Unix behavior for the None-backend / native path is preserved (bash+pipefail,
shfallback), and the pipefail regression test is retained viaplatform_shell::bash_path(). - Env scrub, CWD guard (
ensure_usable_cwd),hide_window, timeout semantics all untouched. - CodeRabbit's module-structure comment was reasonably rebutted (22 sibling utility files already in
agent/) and withdrawn — agreed.
Posted as a COMMENT — approve/merge is the maintainer's call.
coderabbitai
Bot
added
the
agent
2 participants
Summary
platform_shellhelper (Windows:cmd.exe /C, Unix:bash -lc "set -o pipefail\n<cmd>"orsh -lc).NativeRuntimeon Windows moves from PowerShell tocmd.exeso%VAR%expansion works and the sandboxed output-capture path stays byte-transparent.tempfile::tempdir()+ shell-builtinecho hello/exit 1.Problem
Both sandbox execution paths in
src/openhuman/sandbox/ops.rs—execute_unsandboxed:165andexecute_local_jail:225— hardcodedCommand::new("sh"). On Windows,shis not inPATH, soCreateProcessWreturnsERROR_FILE_NOT_FOUNDin ~30ms and the shell tool recordsFailed 34msbefore running anything. Any agent withSandboxMode::Sandboxed(and thetinyflowscodenode, which always runs Sandboxed) hit this on every shell call — matching the symptoms in #4705.The direct
NativeRuntime::build_shell_commandpath had Windows detection but used PowerShell — which does not expand%VAR%and writes UTF-16LE when redirecting to a file, so the two implementations disagreed on what "the Windows shell" means. That divergence is the reason the sandbox paths silently regressed.Solution
New module
src/openhuman/agent/platform_shell.rs:build_tokio_command(command)/build_std_command(command)— one helper, twoCommandflavours. Windows →cmd.exe /C <command>. Unix →bash -lc "set -o pipefail\n<command>"when bash is at/usr/bin/bashor/bin/bash,sh -lc <command>otherwise.pipefailpreserved so masked pipe failures still surface (issue Improve agent harness with LangGraph-style state machine or Polaris-like architecture #4249 context).wrap_with_output_redirection(command, stdout, stderr)— platform-aware output-capture wrap used byexecute_local_jail.{ ...; } > 'out' 2> 'err'on Unix (brace grouping so multi-stage commands route all stages),<command> > "out" 2> "err"on Windows (cmd.exe has no{}grouping; trailing>/2>binds to the whole/Cpayload).bash_path()moved fromhost_runtime.rsand madepub(crate)so the existingnative_shell_pipefail_surfaces_failed_pipe_stagetest can still opt-out on bash-less hosts.Rewired call sites:
NativeRuntime::build_shell_command(src/openhuman/agent/host_runtime.rs:75) — delegates toplatform_shell::build_tokio_command. CWD validation,hide_window, and theensure_usable_cwdguard are untouched.execute_unsandboxed(src/openhuman/sandbox/ops.rs:159) —Command::new("sh")→platform_shell::build_tokio_command.execute_local_jail(src/openhuman/sandbox/ops.rs:202) —Command::new("sh")→platform_shell::build_std_command(matchescwd_jail::spawn'sstd::process::Commandsignature); wrap usesplatform_shell::wrap_with_output_redirection.Windows shell choice — PowerShell → cmd.exe. Deliberate. cmd.exe expands
%VAR%per the reporter's acceptance criteria, matches the POSIXsh -ccontract users expect from a shell tool, and its>/2>operators are byte-transparent (PowerShell 5.1 writes UTF-16LE, which would force encoding-aware readback in the local-jail path). Users relying on PowerShell-only syntax ($env:VAR, cmdlets) via the direct shell tool are affected — but the direct shell path was unusable on Windows for Sandboxed agents anyway, and the population is expected to be near-zero.Submission Checklist
platform_shell::testscovers bothCommandvariants and the redirect wrap per platform;sandbox::ops::tests::execute_unsandboxed_echo_runs_on_every_osandexecute_in_sandbox_none_backend_runs_on_every_osare cross-platform Windows-CI regression guards for thesh→ platform-aware fix, including aexit 1failure-path assertion.platform_shell.rs, the two rewired call sites insandbox/ops.rs, and the delegatedNativeRuntime::build_shell_commandare exercised by the tests above and by the existingnative_runtime_reports_capabilities_and_shell_command/native_shell_pipefail_surfaces_failed_pipe_stagetests.## Related— N/A: no matrix feature IDs affected (see above).Commandspawn only.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no new user-facing surface. Existing "shell tool works" smoke covers it.Closes #NNNin the## Relatedsection — see below.Impact
tinyflowscodenode (both previously ~30msCreateProcessWfailure). Default (SandboxMode::None) agents' shell now usescmd.exeinstead of PowerShell —%VAR%expansion works,>/2>no longer produces UTF-16LE files.NativeRuntimestill usesbash -lc "set -o pipefail\n<cmd>"(orsh -lcfallback);execute_unsandboxed/execute_local_jailswap from baresh -cto the same helper, so on Unix the concrete command becomesbash -lc "set -o pipefail\n<cmd>"when bash is present — which is a small robustness improvement (masked pipe failures now surface via the sandbox too) and matches whatNativeRuntimewas already doing.cmd.exeis Microsoft-signed and always inPATHon Windows.Related
maincurrently has widespread pre-existing test-compilation bit-rot in unrelated modules (sandbox/docker.rs::testsmissingDockerOverridesimport,openhuman/webchatmissing symbols,memory_tree,accessibility, etc. — 38 errors across 8 domains). Reproduced by stashing this PR's changes and runningcargo test --libagainst cleanorigin/main. Not fixed here; flagged so reviewers know a separate cleanup PR is needed for the full-suite lane. The domain-filtered CI lite lane should still exercise this PR's tests.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/4705-windows-shell-cmd-exe736666cebValidation Run
pnpm --filter openhuman-app format:check— N/A: Rust-only change, noapp/**files touched.pnpm typecheck— N/A: Rust-only change, no TypeScript touched.platform_shellunit tests added; localcargo test --libblocked by pre-existing test bit-rot onmain(see Related).cargo fmt --checkpasses on all four changed files;GGML_NATIVE=OFF cargo check --libpasses cleanly.app/src-tauri/**files touched.Validation Blocked
command:GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --liberror:38 pre-existing compile errors in unrelated test modules onorigin/main(DockerOverrides,EnvLookup,MEMORY_SYNC_INTERVAL_SECS_ENV_VAR,compose_system_prompt_suffix, etc.) block the whole lib-test binary from linking.impact:This PR's new tests could not be executed locally. They compile in isolation and follow existing style; the domain-filtered CI-Lite lane should exercise them.Behavior Changes
cmd.exe /Cin all three code paths (previously PowerShell forNativeRuntime, hardcodedshfor the two sandbox paths — the latter completely broken).%VAR%expansion (echo %USERPROFILE%) works on the default agent's shell. PowerShell-only syntax ($env:VAR, cmdlets) via the direct shell tool no longer works on Windows.Parity Contract
NativeRuntimeshell selection (bash -lc "set -o pipefail\n…"/sh -lcfallback) is identical. CWD guard (ensure_usable_cwd),hide_windowhandling,SAFE_ENV_VARSscrub, sandbox env-passthrough, and timeout semantics are unchanged.NativeRuntime::build_shell_commandstill routes CWD validation before the returnedCommandis spawned;execute_local_jailstill routes throughcwd_jail::default_backend()with aNoopBackendfallback when the OS backend is unavailable.Duplicate / Superseded PR Handling
Summary by CodeRabbit