From cb6651c6fcc57e74ce448f32b4150db644762c6b Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sat, 25 Jul 2026 16:25:40 -0400 Subject: [PATCH 1/4] fix(desktop): surface install failures hidden by curl-pipe exit codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every CLI install command is a `curl … | bash` pipe run through a login shell without `pipefail`, so the pipeline reported the right-hand side's status: a `curl` that failed — or was missing from the child's PATH — fed `bash` an empty stdin, which exits 0. The `cli` step was recorded as success and the user got an unactionable post-install `verify` error instead of curl's own stderr. The install child's PATH could also collapse: `Command::env("PATH", …)` replaces rather than extends, and the inherited process PATH was appended only on Windows, so a login shell that exits non-zero or prints nothing left the child with Buzz's managed Node dirs alone — no `curl`, `sh`, or `tar`. Appending the inherited PATH whenever no login-shell PATH was obtained makes that the floor on every OS; entries stay last so managed dirs keep precedence. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 5 +- .../src-tauri/src/commands/agent_discovery.rs | 71 +++++++- .../src/managed_agents/runtime/path.rs | 160 ++++++++++++------ 3 files changed, 173 insertions(+), 63 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f2d3883794..9b2632ce34 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -505,7 +505,10 @@ 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. + ["src-tauri/src/commands/agent_discovery.rs", 1895], // 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 e3d9b22189..ef62ff0bf0 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -561,7 +561,16 @@ 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]); + // Run 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. A `curl` that fails (or + // isn't on PATH at all) was therefore recorded as a successful `cli` step, + // leaving the user an unactionable post-install `verify` error instead of + // curl's own stderr. Every install shell supports it (`/bin/zsh`, + // `/bin/bash`, Git Bash); the Windows PowerShell path bypasses this shell. + // `SHELLOPTS` is not exported, so the piped-to vendor script still runs + // with its own default options. + cmd.args(["-l", "-c", &format!("set -o pipefail; {command}")]); // Strip hermit vars and set managed npm paths (see apply_npm_env). apply_npm_env(&mut cmd); @@ -569,10 +578,12 @@ 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. let login_path = crate::managed_agents::login_shell_path(); let had_login = login_path.is_some(); let managed: Vec = [ @@ -589,7 +600,7 @@ 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() { @@ -1408,6 +1419,54 @@ 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 be prefixed with + /// `set -o pipefail;`, so `curl … | bash` fails when `curl` does. + #[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 + .last() + .expect("install_shell_command must pass a command body"); + assert!( + body.starts_with("set -o pipefail; "), + "install command must run under pipefail; got: {body}" + ); + assert!( + body.ends_with("curl -fsSL https://example.test/i.sh | bash"), + "the vendor command must be preserved verbatim; got: {body}" + ); + } + + /// 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 cf6950f577..8337b27f54 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. @@ -145,7 +140,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 +218,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"), @@ -347,43 +399,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 +544,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}" ); } } From 20d8aeab16ae9a42403c1c78dc494227bf25e61c Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sat, 25 Jul 2026 16:56:59 -0400 Subject: [PATCH 2/4] docs(desktop): correct PATH fallback policy comments to cross-platform build_augmented_path's priority list and the compose_tests module header still described the inherited-PATH fallback as Windows-only, contradicting should_use_inherited after the gate was dropped. A stale contract here is an invitation to re-add the platform gate, since this function feeds every managed-agent spawn and readiness probe. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/runtime/path.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index 8337b27f54..efec0c903e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -86,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 @@ -386,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}; From b8dea1c2bcdf69dd74bd78366fc3c4855a5ba27d Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sat, 25 Jul 2026 22:35:57 -0400 Subject: [PATCH 3/4] fix(desktop): re-export composed install PATH after login init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmd.env("PATH", …) installs the process environment before `-l` sources the user's login startup files, so a profile that assigns PATH discards the composed one before the vendor command runs. That defeats the inherited-PATH fallback in exactly its own trigger condition: a profile containing `export PATH=` is what makes the login-shell probe return None in the first place. On macOS /etc/zprofile's path_helper reorders PATH instead of clearing it, costing the managed Node/npm dirs the precedence the composition promises — no probe failure required. Passing the path as a positional keeps entries with spaces or quotes intact; interpolating it into the body would not. The prelude is omitted when no path was composed, since `export PATH="$1"` with $1 unset sets an empty PATH. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 5 +- .../src-tauri/src/commands/agent_discovery.rs | 123 +++++++++++++++--- 2 files changed, 108 insertions(+), 20 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 9b2632ce34..f6ab28b096 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -508,7 +508,10 @@ const overrides = new Map([ // +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. - ["src-tauri/src/commands/agent_discovery.rs", 1895], + // +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. + ["src-tauri/src/commands/agent_discovery.rs", 1980], // 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 ef62ff0bf0..a4d694ac61 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -547,6 +547,49 @@ 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. +/// +/// `composed_path`, when present, 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. Passing it as a +/// positional rather than interpolating it into the body is what keeps entries +/// containing spaces or quotes intact. +/// +/// When `composed_path` is `None` the prelude is omitted: `export PATH="$1"` +/// with `$1` unset sets an *empty* PATH, worse than the ambient one. +fn install_shell_args( + command: &str, + composed_path: Option<&std::ffi::OsStr>, +) -> Vec { + let Some(path) = composed_path 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,16 +604,6 @@ fn install_shell_command(command: &str) -> Result let shell: std::path::PathBuf = resolve_install_shell()?; let mut cmd = std::process::Command::new(&shell); - // Run 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. A `curl` that fails (or - // isn't on PATH at all) was therefore recorded as a successful `cli` step, - // leaving the user an unactionable post-install `verify` error instead of - // curl's own stderr. Every install shell supports it (`/bin/zsh`, - // `/bin/bash`, Git Bash); the Windows PowerShell path bypasses this shell. - // `SHELLOPTS` is not exported, so the piped-to vendor script still runs - // with its own default options. - cmd.args(["-l", "-c", &format!("set -o pipefail; {command}")]); // Strip hermit vars and set managed npm paths (see apply_npm_env). apply_npm_env(&mut cmd); @@ -584,6 +617,11 @@ fn install_shell_command(command: &str) -> Result // (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: `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`. let login_path = crate::managed_agents::login_shell_path(); let had_login = login_path.is_some(); let managed: Vec = [ @@ -603,11 +641,13 @@ fn install_shell_command(command: &str) -> Result 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())); // 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 @@ -1431,12 +1471,16 @@ mod tests { .get_args() .map(|a| a.to_string_lossy().into_owned()) .collect(); - let body = args - .last() - .expect("install_shell_command must pass a command body"); + // A composed PATH is expected on any test host: `-l -c BODY $0 PATH`. + assert_eq!( + args.len(), + 5, + "expected `-l -c BODY $0 PATH`; got: {args:?}" + ); + let body = &args[2]; assert!( - body.starts_with("set -o pipefail; "), - "install command must run under pipefail; got: {body}" + body.starts_with("export PATH=\"$1\"; set -o pipefail; "), + "composed PATH must be re-exported, then pipefail set; got: {body}" ); assert!( body.ends_with("curl -fsSL https://example.test/i.sh | bash"), @@ -1444,6 +1488,47 @@ mod tests { ); } + /// Without a composed PATH there is no `$1`, and `export PATH="$1"` would + /// set an *empty* PATH — worse than inheriting the ambient one. The prelude + /// and its positionals must both be omitted in that case. + #[test] + fn test_install_shell_args_omit_export_when_no_composed_path() { + assert_eq!( + super::install_shell_args("echo hi", None), + ["-l", "-c", "set -o pipefail; echo hi"].map(std::ffi::OsString::from), + "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))) + .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 From 49c9d440fe3d38a4d224d1b00c496da6183e74d6 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sat, 25 Jul 2026 23:08:59 -0400 Subject: [PATCH 4/4] fix(desktop): suppress the install PATH re-export on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit std::env::join_paths uses the platform separator, so the composed PATH positional is ";"-joined on Windows while bash splits PATH on ":" — re-exporting it inside Git Bash collapses every entry into one nonsense path. Windows is where this bites hardest: login_shell_path() returns None unconditionally there, so the inherited fallback always fires and the prelude would be the steady state, silently undoing the fallback that keeps node/npm/git reachable for the npm .cmd shims. cmd.env("PATH", …) already delivers the native ";"-form that Git Bash translates on entry, and Windows has no login startup files doing the clobbering the prelude defends against, so it buys nothing there. is_windows is a parameter rather than a #[cfg] so the Windows argument shape stays asserted on Unix CI. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 4 +- .../src-tauri/src/commands/agent_discovery.rs | 100 +++++++++++++----- 2 files changed, 74 insertions(+), 30 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f6ab28b096..8553fd2f82 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -511,7 +511,9 @@ const overrides = new Map([ // +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. - ["src-tauri/src/commands/agent_discovery.rs", 1980], + // +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 a4d694ac61..d97e26001d 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -558,22 +558,35 @@ fn persist_last_error_on_install( /// `SHELLOPTS` is not exported, so the piped-to vendor script keeps its own /// defaults. /// -/// `composed_path`, when present, is passed as a positional and re-exported +/// 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. Passing it as a -/// positional rather than interpolating it into the body is what keeps entries -/// containing spaces or quotes intact. +/// costs Buzz's managed Node/npm dirs their precedence. A positional rather than +/// an interpolated body keeps entries containing spaces or quotes intact. /// -/// When `composed_path` is `None` the prelude is omitted: `export PATH="$1"` -/// with `$1` unset sets an *empty* PATH, worse than the ambient one. +/// 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 else { + let Some(path) = composed_path.filter(|_| !is_windows) else { return vec![ "-l".into(), "-c".into(), @@ -618,10 +631,11 @@ fn install_shell_command(command: &str) -> Result // 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: `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`. + // 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 = [ @@ -647,7 +661,11 @@ fn install_shell_command(command: &str) -> Result if let Some(path) = composed_path.as_deref() { cmd.env("PATH", path); } - cmd.args(install_shell_args(command, composed_path.as_deref())); + 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 @@ -1461,8 +1479,10 @@ mod tests { // ── pipefail: install pipes must not mask a failing left-hand side ──────── - /// The command handed to the install shell must be prefixed with - /// `set -o pipefail;`, so `curl … | bash` fails when `curl` does. + /// 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") @@ -1471,16 +1491,10 @@ mod tests { .get_args() .map(|a| a.to_string_lossy().into_owned()) .collect(); - // A composed PATH is expected on any test host: `-l -c BODY $0 PATH`. - assert_eq!( - args.len(), - 5, - "expected `-l -c BODY $0 PATH`; got: {args:?}" - ); let body = &args[2]; assert!( - body.starts_with("export PATH=\"$1\"; set -o pipefail; "), - "composed PATH must be re-exported, then pipefail set; got: {body}" + 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"), @@ -1488,14 +1502,38 @@ mod tests { ); } - /// Without a composed PATH there is no `$1`, and `export PATH="$1"` would - /// set an *empty* PATH — worse than inheriting the ambient one. The prelude - /// and its positionals must both be omitted in that case. + /// 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_omit_export_when_no_composed_path() { + 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), - ["-l", "-c", "set -o pipefail; echo hi"].map(std::ffi::OsString::from), + super::install_shell_args("echo hi", None, false), + bare, "no composed PATH must yield the bare pipefail body and no positionals" ); } @@ -1515,7 +1553,11 @@ mod tests { // `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))) + .args(super::install_shell_args( + "echo \"$PATH\"", + Some(&composed), + false, + )) .env("HOME", home.path()) .env("PATH", &composed) .stdin(std::process::Stdio::null())