mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): surface install failures hidden by curl-pipe exit codes (#2892)
Ubuntu Doctor reports `Install failed at verify: The installer finished, but Buzz still could not use claude-code (observed: CLI missing)` while the `cli` step shows success. The `cli` step is lying. ## The masking Every CLI install command is a pipe — `curl -fsSL https://claude.ai/install.sh | bash` (`managed_agents/discovery.rs:109`), `… | sh` for Codex (`:141`), `… | CONFIGURE=false bash` for Goose (`:75`). `install_shell_command` ran them through `bash -l -c` with no `pipefail`, so the pipeline's exit status was the **right-hand** side's. A `curl` that fails — or that isn't on the child's PATH at all — feeds `bash` an empty stdin, and `bash` with nothing to run exits 0: ``` $ /bin/bash -l -c 'curl -fsSL https://nonexistent.invalid/x.sh | bash'; echo $? curl: (6) Could not resolve host: nonexistent.invalid 0 $ PATH=/tmp/empty /bin/bash -l -c 'curl -fsSL https://claude.ai/install.sh | bash'; echo $? bash: line 1: curl: command not found 0 ``` `run_install_command` records exit 0 as `success: true`, the adapter step then installs fine (it uses Buzz's own bundled Node, no system PATH needed), and `post_install_verification` correctly reports the CLI is absent. The user is handed a `verify` riddle instead of curl's error, which is why diagnosing this required three rounds of guessing. Install commands now run under `set -o pipefail`, so the left-hand side's failure is the step's failure and `InstallStepResult.stderr` carries the vendor's own message. `SHELLOPTS` is not exported by either shell, so the piped-to vendor script still runs with its default options. The Windows PowerShell install path (`install_powershell_command`) bypasses this shell and is untouched. ## The PATH collapse it was hiding `install_shell_command` composes the child's PATH and calls `cmd.env("PATH", …)`, which **replaces** rather than extends. `should_use_inherited` was `is_windows && !had_shell_path && has_local_context`, so on Unix the inherited process PATH was never appended. When `login_shell_path()` returns `None` — a login shell that exits non-zero or prints nothing, which a GUI-launched process can easily hit via `~/.profile` — the child's entire PATH becomes Buzz's two managed Node dirs. There is no `curl`, `sh`, `sha256sum`, or `tar` in either, so every curl-pipe install fails, and before this PR it failed invisibly. The `is_windows` requirement is dropped: the inherited PATH is the floor whenever no login-shell PATH was obtained, on every OS. Both existing suppressions are kept — a login-shell PATH present still suppresses it (no doubling), and no home/exe context still suppresses it (never manufacture a PATH from ambient state alone). Inherited entries stay **last**, so managed dirs keep precedence. The other caller, `build_augmented_path` (`runtime/path.rs:148`, feeding agent spawns and CLI probes), reads correctly under the new rule for the same reason: it only gains the inherited PATH in the case where it would otherwise hand a child a PATH with no native entries. When a login-shell PATH exists — the normal case on macOS and Linux — its output is unchanged, which `unix_shell_path_suppresses_inherited_fallback` pins. ## Scope This fixes the reporting defect and the PATH floor. The specific environment failure on the affected Ubuntu box is still being diagnosed and is deliberately not addressed here; the point of this change is that the next attempt produces the real error instead of a `verify` riddle. One interaction worth noting: `install_failure_is_retryable` retries any failure that carries an exit code, so a pipefail-surfaced curl failure now gets 3 attempts with backoff — correct for transient network blips, and harmless for hard failures. `desktop/scripts/check-file-sizes.mjs` ratchets the `agent_discovery.rs` ceiling 1836 → 1895 for the added tests. --------- Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
This commit is contained in:
co-authored by
npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7
parent
c9a73726be
commit
166c6655e8
@@ -492,7 +492,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
|
||||
|
||||
@@ -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<std::ffi::OsString> {
|
||||
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<std::process::Command, String>
|
||||
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<std::process::Command, String>
|
||||
// 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<std::path::PathBuf> = [
|
||||
@@ -589,14 +652,20 @@ fn install_shell_command(command: &str) -> Result<std::process::Command, String>
|
||||
let inherited: Vec<std::path::PathBuf> = 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<String> = 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.
|
||||
|
||||
@@ -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<PathBuf> = 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, 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 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_shell_path_output_unchanged_by_windows_fallback_logic() {
|
||||
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 also append the inherited
|
||||
/// process PATH — the login-shell PATH already carries the native entries.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
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.
|
||||
/// 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_no_shell_with_context_uses_inherited() {
|
||||
fn policy_no_shell_with_context_uses_inherited() {
|
||||
assert!(
|
||||
should_use_inherited(false, true, true),
|
||||
"Windows, no shell path, has context → must append inherited"
|
||||
should_use_inherited(false, true),
|
||||
"no shell path, has context → must append inherited"
|
||||
);
|
||||
}
|
||||
|
||||
/// Windows + shell path present → must NOT use inherited (login path covers it).
|
||||
/// Shell path present → must NOT use inherited (login path covers it).
|
||||
#[test]
|
||||
fn policy_windows_shell_path_present_suppresses_inherited() {
|
||||
fn policy_shell_path_present_suppresses_inherited() {
|
||||
assert!(
|
||||
!should_use_inherited(true, true, true),
|
||||
"Windows, shell path present → must not append inherited"
|
||||
!should_use_inherited(true, true),
|
||||
"shell path present → must not append inherited"
|
||||
);
|
||||
}
|
||||
|
||||
/// Windows + no local context → must NOT use inherited (no ambient state).
|
||||
/// No local context → must NOT use inherited (no ambient state).
|
||||
#[test]
|
||||
fn policy_windows_no_local_context_suppresses_inherited() {
|
||||
fn policy_no_local_context_suppresses_inherited() {
|
||||
assert!(
|
||||
!should_use_inherited(false, false, true),
|
||||
"Windows, no local context → must not append inherited"
|
||||
);
|
||||
}
|
||||
|
||||
/// Non-Windows → never use inherited, regardless of other flags.
|
||||
#[test]
|
||||
fn policy_non_windows_never_uses_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}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user