diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f2d3883794b..8553fd2f82e 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -505,7 +505,15 @@ const overrides = new Map([ // fix for the Goose Windows installer (PR #2680 interaction with #2750). // +10: pass an explicit PATH through Codex adapter install planning so unit // tests avoid the process-global login-shell PATH cache. - ["src-tauri/src/commands/agent_discovery.rs", 1836], + // +59: run install commands under `pipefail` so a failing `curl` in a + // `curl … | bash` install fails the `cli` step instead of being masked by + // `bash`'s exit 0, plus tests for the arg shape and the real pipeline status. + // +81: install_shell_args re-exports the composed PATH inside the command + // body so login startup files can't clear or reorder it, plus an isolated + // hostile-profile regression the pure composition tests structurally miss. + // +42: gate that re-export off Windows, where join_paths is `;`-separated and + // bash would collapse it into one entry, plus a platform-shape test. + ["src-tauri/src/commands/agent_discovery.rs", 2022], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index e3d9b221893..d97e26001dc 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -547,6 +547,62 @@ fn persist_last_error_on_install( save_managed_agents(app, &records) } +/// Build the `-l -c` argument list for the install shell. +/// +/// The body runs under `pipefail`: every CLI install command is a `curl … | +/// bash` / `| sh` pipe, and without it the pipeline's status is the right-hand +/// side's — `bash`/`sh` fed an empty stdin exits 0 — so a `curl` that fails (or +/// isn't on PATH at all) was recorded as a successful `cli` step, leaving the +/// user an unactionable `verify` error instead of curl's own stderr. Every +/// install shell supports it; the Windows PowerShell path bypasses this shell. +/// `SHELLOPTS` is not exported, so the piped-to vendor script keeps its own +/// defaults. +/// +/// Off Windows, `composed_path` is passed as a positional and re-exported +/// *inside* the body, because `-l` sources the user's login startup files after +/// the process environment is installed: a profile assigning PATH overwrites +/// `cmd.env("PATH", …)` before the vendor command runs. `export PATH=` empties +/// it outright; macOS `/etc/zprofile` runs `path_helper`, which reorders it and +/// costs Buzz's managed Node/npm dirs their precedence. A positional rather than +/// an interpolated body keeps entries containing spaces or quotes intact. +/// +/// The prelude is omitted where it would do harm: +/// - `composed_path` is `None` — `export PATH="$1"` with `$1` unset sets an +/// *empty* PATH, worse than the ambient one. +/// - `is_windows` — `join_paths` uses the platform separator, so the positional +/// would be `;`-joined while bash splits PATH on `:`, collapsing every entry +/// into one nonsense path; and Windows is where the inherited fallback always +/// fires (`login_shell_path()` is unconditionally `None` there), so this would +/// be the steady state. `cmd.env("PATH", …)` already delivers the native form +/// Git Bash translates on entry, and Windows has no login startup files doing +/// the clobbering this prelude defends against. +/// +/// `is_windows` is a parameter rather than a `#[cfg]` so the Windows shape stays +/// asserted on Unix CI — the same reason `should_skip_claude_executable` takes +/// one. Extracted from `install_shell_command` for that testability, not because +/// it has more than one caller. +fn install_shell_args( + command: &str, + composed_path: Option<&std::ffi::OsStr>, + is_windows: bool, +) -> Vec { + let Some(path) = composed_path.filter(|_| !is_windows) else { + return vec![ + "-l".into(), + "-c".into(), + format!("set -o pipefail; {command}").into(), + ]; + }; + vec![ + "-l".into(), + "-c".into(), + format!("export PATH=\"$1\"; set -o pipefail; {command}").into(), + // `$0` is the shell-name slot, so the PATH must be the second positional. + "buzz-install".into(), + path.to_os_string(), + ] +} + /// Build a login-shell `Command` for `command` with hermit env vars stripped, /// Buzz-managed npm locations set, and the user's PATH set. This is the /// single source of truth for @@ -561,7 +617,6 @@ fn install_shell_command(command: &str) -> Result let shell: std::path::PathBuf = resolve_install_shell()?; let mut cmd = std::process::Command::new(&shell); - cmd.args(["-l", "-c", command]); // Strip hermit vars and set managed npm paths (see apply_npm_env). apply_npm_env(&mut cmd); @@ -569,10 +624,18 @@ fn install_shell_command(command: &str) -> Result // Compose the PATH for the install shell using the same kernel as the // runtime/probe path so the two can never drift. managed entries first // (Node/npm bins keep precedence); login-shell entries next; inherited - // process PATH appended last on Windows when no login-shell PATH exists - // (login_shell_path() always returns None on Windows — Git Bash paths are - // POSIX-shaped and poison native children; cmd.env("PATH", …) replaces - // rather than extends, so without inherited the install shell loses npm). + // process PATH appended last when no login-shell PATH exists — the case + // where the composed PATH would otherwise be Buzz's managed Node dirs + // alone, with no `curl`/`sh`/`tar` for the vendor install pipes + // (cmd.env("PATH", …) replaces rather than extends). On Windows that case + // is the steady state: login_shell_path() always returns None there + // because Git Bash paths are POSIX-shaped and poison native children. + // + // The composed PATH is set twice on purpose off Windows: `cmd.env` so the + // login startup files themselves run with a usable PATH, and the `$1` + // export in `install_shell_args` so their own PATH assignments cannot undo + // it. Neither is redundant — see `install_shell_args`, which also explains + // why the export is suppressed on Windows. let login_path = crate::managed_agents::login_shell_path(); let had_login = login_path.is_some(); let managed: Vec = [ @@ -589,14 +652,20 @@ fn install_shell_command(command: &str) -> Result let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - let use_inherited = crate::managed_agents::should_use_inherited(had_login, true, cfg!(windows)); + let use_inherited = crate::managed_agents::should_use_inherited(had_login, true); let path_parts = crate::managed_agents::compose_path_entries(managed, login, inherited, use_inherited); - if !path_parts.is_empty() { - if let Ok(path) = std::env::join_paths(path_parts) { - cmd.env("PATH", path); - } - } + let composed_path = (!path_parts.is_empty()) + .then(|| std::env::join_paths(path_parts).ok()) + .flatten(); + if let Some(path) = composed_path.as_deref() { + cmd.env("PATH", path); + } + cmd.args(install_shell_args( + command, + composed_path.as_deref(), + cfg!(windows), + )); // Detach from the controlling terminal so install scripts that read from // /dev/tty (e.g. Codex's "Start Codex now? [y/N]") fall back to stdin @@ -1408,6 +1477,123 @@ mod tests { assert!(result.is_ok(), "install_shell_command must succeed on Unix"); } + // ── pipefail: install pipes must not mask a failing left-hand side ──────── + + /// The command handed to the install shell must run under `set -o pipefail;` + /// with the vendor command preserved verbatim, so `curl … | bash` fails when + /// `curl` does. Platform-agnostic: only the PATH prelude differs by OS, and + /// `test_install_shell_args_shape_per_platform` pins that. + #[test] + fn test_install_shell_command_enables_pipefail() { + let cmd = super::install_shell_command("curl -fsSL https://example.test/i.sh | bash") + .expect("install shell must resolve on a test host"); + let args: Vec = cmd + .get_args() + .map(|a| a.to_string_lossy().into_owned()) + .collect(); + let body = &args[2]; + assert!( + body.contains("set -o pipefail; "), + "the install body must set pipefail; got: {body}" + ); + assert!( + body.ends_with("curl -fsSL https://example.test/i.sh | bash"), + "the vendor command must be preserved verbatim; got: {body}" + ); + } + + /// The PATH prelude is emitted only where it helps, and the exact argument + /// vector is the contract: a stray trailing positional with no `$1` reader, + /// or an export whose `$1` the shell cannot split, both corrupt PATH. + /// Windows is excluded because `join_paths` is `;`-separated there while bash + /// splits PATH on `:` — and it is the platform where the inherited fallback + /// always fires. See `install_shell_args` for the full reasoning. + #[test] + fn test_install_shell_args_shape_per_platform() { + let composed = std::ffi::OsString::from("/buzz/node/bin:/usr/bin"); + let windows_composed = std::ffi::OsString::from(r"C:\buzz\node;C:\Windows\system32"); + let bare = ["-l", "-c", "set -o pipefail; echo hi"].map(std::ffi::OsString::from); + + assert_eq!( + super::install_shell_args("echo hi", Some(&composed), false), + [ + "-l", + "-c", + "export PATH=\"$1\"; set -o pipefail; echo hi", + "buzz-install", + "/buzz/node/bin:/usr/bin", + ] + .map(std::ffi::OsString::from), + "Unix must re-export the composed PATH after login init" + ); + assert_eq!( + super::install_shell_args("echo hi", Some(&windows_composed), true), + bare, + "Windows must not re-export a `;`-joined PATH inside bash" + ); + assert_eq!( + super::install_shell_args("echo hi", None, false), + bare, + "no composed PATH must yield the bare pipefail body and no positionals" + ); + } + + /// Regression for the login-startup-file overwrite: `cmd.env("PATH", …)` is + /// installed *before* `-l` sources the user's profile, so a profile that + /// assigns PATH silently discards the composed one. Uses `/bin/bash` + /// explicitly — the planted profile is bash-specific, so resolving the host + /// shell (which prefers zsh) would make this vacuous. + #[cfg(unix)] + #[test] + fn test_composed_path_survives_a_profile_that_clears_it() { + let home = tempfile::tempdir().expect("temp HOME"); + std::fs::write(home.path().join(".bash_profile"), "export PATH=\n") + .expect("plant a hostile login profile"); + let composed = std::ffi::OsString::from("/buzz/sentinel/bin:/usr/bin:/bin"); + + // `echo` is a shell builtin, so the child needs no PATH to report one. + let out = std::process::Command::new("/bin/bash") + .args(super::install_shell_args( + "echo \"$PATH\"", + Some(&composed), + false, + )) + .env("HOME", home.path()) + .env("PATH", &composed) + .stdin(std::process::Stdio::null()) + .output() + .expect("bash must spawn"); + + let path = String::from_utf8_lossy(&out.stdout); + assert!( + path.contains("/buzz/sentinel/bin"), + "the composed PATH must survive login init; got: {path:?}" + ); + } + + /// End-to-end on the real resolved install shell (no network): a pipeline + /// whose left-hand side fails must exit non-zero, while a fully successful + /// pipeline must still succeed. Without `pipefail` the status is the + /// right-hand side's and the left-hand failure is invisible. + #[cfg(unix)] + #[test] + fn test_install_shell_pipeline_status_follows_left_side() { + for (command, expect_success) in [("false | true", false), ("echo ok | cat", true)] { + let status = super::install_shell_command(command) + .expect("Unix must always resolve an install shell") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .expect("install shell must spawn"); + assert_eq!( + status.success(), + expect_success, + "`{command}` must report success={expect_success}; got {status:?}" + ); + } + } + // ── Phase A: Windows install shell selection ─────────────────────────────── /// On Windows (CI runner has Git pre-installed), resolve_install_shell succeeds. diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index cf6950f5773..efec0c903ee 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -30,30 +30,25 @@ pub(crate) fn should_skip_claude_executable(path: &std::path::Path, is_windows: /// Decide whether the inherited process PATH should be appended to the /// composed PATH. /// -/// On Windows, `login_shell_path()` always returns `None` because Git Bash -/// returns POSIX colon-delimited paths that poison native children. -/// `Command::env("PATH", …)` replaces rather than extends, so without the -/// inherited PATH every child loses node/npm/git. -/// -/// This pure function takes an explicit `is_windows` flag so it can be -/// unit-tested cross-host (macOS CI can pass `true` to exercise the Windows -/// policy without needing the `cfg!(windows)` target). +/// `Command::env("PATH", …)` replaces rather than extends, so a child whose +/// composed PATH carries no native entries loses every system binary. On +/// Windows that is the steady state — `login_shell_path()` always returns +/// `None` because Git Bash returns POSIX colon-delimited paths that poison +/// native children. On Unix it is the failure mode: a login shell that exits +/// non-zero or prints nothing also yields `None`, and the child is then left +/// with only Buzz's managed Node dirs — no `curl`, `sh`, or `tar`, which +/// silently breaks every `curl … | bash` install. The inherited PATH is the +/// floor under both cases, appended last so managed dirs keep precedence. /// /// Rules: -/// - Only append when `is_windows` — on Unix the login-shell PATH always covers -/// the needed runtimes. /// - Suppress when `had_shell_path` is `true` — if a login-shell PATH was /// supplied it already carries the user's native entries; appending the /// process PATH would double them. /// - Suppress when `has_local_context` is `false` — callers that pass no home /// or exe-parent context must not receive a PATH manufactured from ambient /// process state alone. -pub(crate) fn should_use_inherited( - had_shell_path: bool, - has_local_context: bool, - is_windows: bool, -) -> bool { - is_windows && !had_shell_path && has_local_context +pub(crate) fn should_use_inherited(had_shell_path: bool, has_local_context: bool) -> bool { + !had_shell_path && has_local_context } /// Pure PATH composition kernel shared by the install shell and the runtime/probe paths. @@ -91,10 +86,14 @@ pub(crate) fn compose_path_entries( /// 4. `nvm_bin` — nvm's default Node.js bin dir (if the user uses nvm) /// 5. exe parent dir — DMG sidecars under `Contents/MacOS/` /// 6. user's login-shell `PATH` — runtimes like node/python from other managers -/// 7. Windows only: the current process `PATH` (appended when no login-shell -/// PATH exists, because callers use `Command::env("PATH", …)` which -/// *replaces* the child's PATH — without this, the child loses node/npm/git -/// and every npm `.cmd` shim fails with `'node' is not recognized`) +/// 7. the current process `PATH` — appended on every platform when no +/// login-shell PATH exists, because callers use `Command::env("PATH", …)` +/// which *replaces* the child's PATH. This is the steady state on Windows, +/// where `login_shell_path()` always returns `None` and without it the +/// child loses node/npm/git and every npm `.cmd` shim fails with +/// `'node' is not recognized`; on Unix it is the login-shell-probe failure +/// fallback, which keeps `curl`/`sh`/`tar` reachable. See +/// [`should_use_inherited`] for the suppression rules. /// /// `shell_path` is the raw colon-delimited string from a login shell, so it is /// split into individual entries before joining. Pushing it as a single segment @@ -145,7 +144,7 @@ pub(in crate::managed_agents) fn build_augmented_path( let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - let use_inherited = should_use_inherited(had_shell_path, has_local_context, cfg!(windows)); + let use_inherited = should_use_inherited(had_shell_path, has_local_context); let parts = compose_path_entries(managed, login, inherited, use_inherited); if parts.is_empty() { @@ -223,29 +222,86 @@ mod tests { #[cfg(unix)] #[test] fn nvm_bin_none_does_not_add_segment() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + // With no shell_path the inherited process PATH is appended last, so + // pin it to a sentinel to keep the assertion deterministic. + std::env::set_var("PATH", "/sentinel/inherited"); + let result = build_augmented_path( Some(PathBuf::from("/home/user")), Some(PathBuf::from("/usr/local/bin")), None, None, ); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + let result = result.expect("path"); assert!(result.starts_with("/home/user/.local/bin:"), "{result}"); - assert!(result.ends_with(":/usr/local/bin"), "{result}"); + assert!(!result.contains(".nvm"), "no nvm segment: {result}"); + assert!( + result.contains(":/usr/local/bin:"), + "exe parent must precede the inherited PATH: {result}" + ); + assert!( + result.ends_with(":/sentinel/inherited"), + "inherited PATH must be appended last when no shell_path: {result}" + ); + } + + /// On Unix with no login-shell PATH, `build_augmented_path` must fall back to + /// the inherited process PATH — otherwise the child gets only Buzz-managed + /// dirs and loses every system binary (`curl`, `sh`, `tar`). + #[cfg(unix)] + #[test] + fn unix_appends_process_path_when_no_shell_path() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", "/usr/bin:/bin"); + + let result = build_augmented_path(Some(PathBuf::from("/home/user")), None, None, None); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + let result = result.expect("path must not be None with a home dir"); + assert!( + result.starts_with("/home/user/.local/bin:"), + "home/.local/bin must be first: {result}" + ); + assert!( + result.ends_with(":/usr/bin:/bin"), + "process PATH must be last: {result}" + ); } - /// On Unix, supplying a `shell_path` must NOT trigger the Windows process-PATH - /// fallback — the output must be byte-identical to what it was before this - /// fix. + /// On Unix, supplying a `shell_path` must NOT also append the inherited + /// process PATH — the login-shell PATH already carries the native entries. #[cfg(unix)] #[test] - fn unix_shell_path_output_unchanged_by_windows_fallback_logic() { + fn unix_shell_path_suppresses_inherited_fallback() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", "/should/not/appear"); + let result = build_augmented_path( Some(PathBuf::from("/home/user")), None, Some("/usr/local/bin:/usr/bin:/bin".to_string()), None, ); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + let result = result.expect("path"); assert!( result.ends_with(":/usr/local/bin:/usr/bin:/bin"), @@ -334,8 +390,8 @@ mod tests { // ── Pure policy and composition tests — run on every host ──────────────────── // // These test `should_use_inherited` and `compose_path_entries` with explicit -// inputs, so they run on macOS/Linux CI and validate the Windows policy -// behavior without touching process state or requiring a Windows target. +// inputs, so they run on macOS/Linux CI and validate the cross-platform +// fallback policy without touching process state or requiring a Windows target. #[cfg(test)] mod compose_tests { use super::{compose_path_entries, is_batch_shim, should_use_inherited}; @@ -347,43 +403,37 @@ mod compose_tests { // ── should_use_inherited policy matrix ──────────────────────────────────── - /// Windows + no shell path + has local context → must use inherited. - #[test] - fn policy_windows_no_shell_with_context_uses_inherited() { - assert!( - should_use_inherited(false, true, true), - "Windows, no shell path, has context → must append inherited" - ); - } - - /// Windows + shell path present → must NOT use inherited (login path covers it). + /// No shell path + has local context → must use inherited, on every OS. + /// This is the steady state on Windows (login_shell_path() is always None) + /// and the failure mode on Unix (login shell exited non-zero or printed + /// nothing); in both the child would otherwise get no native PATH entries. #[test] - fn policy_windows_shell_path_present_suppresses_inherited() { + fn policy_no_shell_with_context_uses_inherited() { assert!( - !should_use_inherited(true, true, true), - "Windows, shell path present → must not append inherited" + should_use_inherited(false, true), + "no shell path, has context → must append inherited" ); } - /// Windows + no local context → must NOT use inherited (no ambient state). + /// Shell path present → must NOT use inherited (login path covers it). #[test] - fn policy_windows_no_local_context_suppresses_inherited() { + fn policy_shell_path_present_suppresses_inherited() { assert!( - !should_use_inherited(false, false, true), - "Windows, no local context → must not append inherited" + !should_use_inherited(true, true), + "shell path present → must not append inherited" ); } - /// Non-Windows → never use inherited, regardless of other flags. + /// No local context → must NOT use inherited (no ambient state). #[test] - fn policy_non_windows_never_uses_inherited() { + fn policy_no_local_context_suppresses_inherited() { assert!( - !should_use_inherited(false, true, false), - "non-Windows must never append inherited PATH" + !should_use_inherited(false, false), + "no local context → must not append inherited" ); assert!( - !should_use_inherited(false, false, false), - "non-Windows + no context must never append inherited PATH" + !should_use_inherited(true, false), + "no local context → must not append inherited even with a shell path" ); } @@ -498,23 +548,25 @@ mod compose_tests { // compute the same `should_use_inherited` decision for equivalent inputs. // Tests the policy function directly to confirm the wrappers can't drift. - /// Exhaustive truth-table for `should_use_inherited` — all four input - /// combinations that affect real callers. Confirms the policy is correct - /// before either wrapper binds to it. + /// Exhaustive truth-table for `should_use_inherited` — every input + /// combination. Confirms the policy is correct before either wrapper binds + /// to it. The rule is OS-independent: the inherited PATH is the floor + /// whenever no login-shell PATH was obtained, because the alternative is a + /// child with no native binaries at all. #[test] fn should_use_inherited_policy_truth_table() { - // (had_shell, has_context, is_windows) → expected + // (had_shell, has_context) → expected let cases = [ - (false, true, true, true), // Windows, no shell, context → USE - (true, true, true, false), // Windows, shell present → NO - (false, false, true, false), // Windows, no context → NO - (false, true, false, false), // non-Windows → NO + (false, true, true), // no shell PATH, context → USE (the floor) + (true, true, false), // shell PATH present → NO (already covered) + (false, false, false), // no context → NO (no ambient-only PATH) + (true, false, false), // no context → NO, shell PATH irrelevant ]; - for (had_shell, has_ctx, is_win, expected) in cases { - let result = should_use_inherited(had_shell, has_ctx, is_win); + for (had_shell, has_ctx, expected) in cases { + let result = should_use_inherited(had_shell, has_ctx); assert_eq!( result, expected, - "policy mismatch: had_shell={had_shell} has_ctx={has_ctx} is_win={is_win}" + "policy mismatch: had_shell={had_shell} has_ctx={has_ctx}" ); } }