fix(desktop): surface install failures hidden by curl-pipe exit codes#2892
fix(desktop): surface install failures hidden by curl-pipe exit codes#2892wpfleger96 wants to merge 4 commits into
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Reviewed at 9a0a424. Applying pipefail at the shared Unix/Git-Bash command boundary correctly surfaces a failing left-hand fetch while leaving native PowerShell untouched. The inherited PATH fallback is lowest precedence and only activates when no login-shell PATH was obtained; the deterministic shell and policy tests cover activation, suppression, ordering, and healthy pipelines. Current CI is fully green, including Windows Rust. No blocking findings.
wesbillman
left a comment
There was a problem hiding this comment.
Correction after a late independent review finding, validated at 9a0a424d2b21fcc798bca0bf7bf3d3bc02b62649:
The Unix inherited-PATH fallback does not necessarily survive until the install command. install_shell_command() sets the composed fallback with cmd.env("PATH", ...) at desktop/src-tauri/src/commands/agent_discovery.rs:587-608, then starts the shell with -l -c at lines 563-573. Login startup files execute after the process environment is installed and can overwrite or empty PATH before set -o pipefail; <vendor command> runs.
This is reachable through the exact fallback branch: fetch_login_shell_path_inner() (managed_agents/discovery.rs:726-741) returns None when the login shell produces no non-empty PATH line. If the profile contains export PATH=, the probe returns None; the install command receives the inherited fallback through cmd.env, starts the same login shell, and that profile empties PATH again. I reproduced that shell behavior directly with an isolated .bash_profile: HOME=<temp> PATH=/usr/bin:/bin /bin/bash -l -c ... observed an empty PATH and could not resolve curl.
Please make the fallback effective after login initialization (safely set PATH inside the command body), or avoid login initialization for the install execution when the probe failed. Add an isolated-profile regression covering a login profile that clears PATH. The existing composition tests do not exercise startup-file overwrite.
The pipefail behavior itself remains correct and current CI is green, but the PATH half of the stated fix is incomplete. This review supersedes my earlier approval.
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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
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 <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
9a0a424 to
b8dea1c
Compare
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 <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Ubuntu Doctor reports
Install failed at verify: The installer finished, but Buzz still could not use claude-code (observed: CLI missing)while theclistep shows success. Theclistep is lying.The masking
Every CLI install command is a pipe —
curl -fsSL https://claude.ai/install.sh | bash(managed_agents/discovery.rs:109),… | shfor Codex (:141),… | CONFIGURE=false bashfor Goose (:75).install_shell_commandran them throughbash -l -cwith nopipefail, so the pipeline's exit status was the right-hand side's. Acurlthat fails — or that isn't on the child's PATH at all — feedsbashan empty stdin, andbashwith nothing to run exits 0:run_install_commandrecords exit 0 assuccess: true, the adapter step then installs fine (it uses Buzz's own bundled Node, no system PATH needed), andpost_install_verificationcorrectly reports the CLI is absent. The user is handed averifyriddle 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 andInstallStepResult.stderrcarries the vendor's own message.SHELLOPTSis 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_commandcomposes the child's PATH and callscmd.env("PATH", …), which replaces rather than extends.should_use_inheritedwasis_windows && !had_shell_path && has_local_context, so on Unix the inherited process PATH was never appended. Whenlogin_shell_path()returnsNone— 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 nocurl,sh,sha256sum, ortarin either, so every curl-pipe install fails, and before this PR it failed invisibly.The
is_windowsrequirement 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, whichunix_shell_path_suppresses_inherited_fallbackpins.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
verifyriddle.One interaction worth noting:
install_failure_is_retryableretries 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.mjsratchets theagent_discovery.rsceiling 1836 → 1895 for the added tests.