Fix piped installer under nounset by pradhankukiran · Pull Request #568 · basecamp/basecamp-cli

pradhankukiran · 2026-07-27T16:21:53Z

What

Make the installer source guard tolerate an unset BASH_SOURCE[0] when Bash reads the script from standard input.
Add a regression test that pipes the real installer into Bash without downloading or installing anything.

Why

The documented curl ... | bash installation path runs with BASH_SOURCE[0] unset. The source guard added in #558 dereferences that value while set -u is active, so the installer exits before main runs. Falling back to $0 preserves direct and piped execution while still preventing main from running when the script is sourced.

Testing

bats e2e/installer.bats
bin/ci

Summary by cubic

Fix the installer so piping to bash works under set -u. The source guard now tolerates an unset BASH_SOURCE[0], so curl ... | bash runs main as expected.

Bug Fixes
- Update guard to [[ "${BASH_SOURCE[0]:-$0}" == "$0" ]] to handle stdin execution while still preventing execution when sourced.
- Add e2e test that pipes the real installer to bash, runs with the current $BASH, and asserts it fails on missing curl (not on an unbound variable).

^{Written for commit 0f3b638. Summary will update on new commits.}

Copilot

Pull request overview

Fixes the Bash installer’s “source guard” so curl … | bash works under set -u by tolerating an unset BASH_SOURCE[0], while still preventing main from running when the script is sourced (e.g., by tests).

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Changes:

Update scripts/install.sh to safely compare ${BASH_SOURCE[0]} against $0 even when BASH_SOURCE[0] is unset during stdin execution.
Add a Bats regression test that pipes the real installer into Bash and asserts it fails at the curl prerequisite (without downloading/installing) rather than exiting due to nounset.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
scripts/install.sh	Makes the source-vs-exec guard safe under `set -u` for piped stdin execution.
e2e/installer.bats	Adds a regression test that simulates `curl … \| bash` and asserts the installer reaches `main` without a `BASH_SOURCE` unbound error.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cubic-dev-ai

All reported issues were addressed across 2 files

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

e2e/installer.bats:72

The comment says "Keep PATH empty", but the test actually sets PATH to "$BATS_TEST_TMPDIR". This is effectively a minimal/isolated PATH, but the wording is misleading and makes it harder to understand why this is safe (and why "$BASH" is invoked via an absolute path).

  # Keep PATH empty so main stops at its curl prerequisite without downloading
  # anything or modifying the test environment.

pradhankukiran · 2026-07-27T16:42:18Z

@jeremy This fixes an stdin edge case in the source guard added in #558.

jeremy · 2026-07-27T17:38:56Z

Thanks @pradhankukiran! Taking a look.

jeremy

Verified locally: old guard fails piped under nounset, new guard reaches main; bats installer suite passes; confirmed under stock bash 3.2.57. Thanks for the fix.

* Add post-deploy installer smoke canary The install endpoint serves main, so merging a change to scripts/install.sh deploys it immediately. #558 shipped a sourcing guard that broke the documented `curl | bash` path under `set -u`; the bats suite now gates that regression pre-merge (#568), but nothing checked the published installer post-merge. Add .github/workflows/installer-smoke.yml: nightly + workflow_dispatch + push-to-main (scripts/install.sh) runs of the canonical Quick Start command against the published endpoint, cache-busted with the commit SHA since raw.githubusercontent.com serves max-age=300. Two legs: Linux (modern bash) and macOS via stock /bin/bash, which asserts exactly Bash 3.2 through the same interpreter so the coverage can't silently degrade. Installs into a scratch HOME with explicit BASECAMP_BIN_DIR and asserts the deterministic installed path. Also two installer-scoped comment fixes from the incident review: correct the "Keep PATH empty" description in e2e/installer.bats (PATH is a binary-free tmpdir, not empty), and document why the guard's `:-$0` fallback is required so a future cleanup doesn't re-break piping. Refs #558, #568 * Set cancel-in-progress explicitly to match workflow convention

* ci: harden installer canary and add pre-merge Bash 3.2 gate Canary (installer-smoke.yml): - Trigger on scripts/install.ps1 pushes too — the raw URL serves main, so merge == deploy for the PowerShell installer as well. - New Windows job running the documented irm | iex shape under both Windows PowerShell 5.1 and pwsh 7 (matrix), against scratch USERPROFILE/BASECAMP_BIN_DIR under RUNNER_TEMP. - New notify job: on any leg's failure, file or update an "Installer canary failure" issue (exact-title dedupe, best-effort gh calls, ::error:: annotation). Skipped entirely on green runs. Pre-merge (test.yml): new installer-bash32 job on macos-latest — plain steps, not bats (bats 1.11 execs `env bash` at every layer, so the interpreter is not pinnable through it). Asserts /bin/bash is exactly 3.2, syntax-checks install.sh, and runs it piped with curl off PATH, asserting the guard's clean failure message and the absence of unbound-variable errors (the #558 regression class). Path-filtered to installer files plus test.yml itself so the job proves itself on the PR that introduces or edits it; skipped filters still report success, so the job is safe to mark required. Refs #558 #568 #569 * TEMP: notify drill (revert me) * Revert "TEMP: notify drill (revert me)" This reverts commit 37cf5f7. * ci: run a full offline install under Bash 3.2, not just the curl guard The piped-execution step stops at the curl prerequisite, and `bash -n` only checks grammar — neither catches Bash-4-only constructs in the download, checksum, extract, or post-install code. Add a step that runs one complete install path under real /bin/bash 3.2 using a fixture curl serving a local release (stub binary, real sha256 checksums) with PATH restricted to fixture + system dirs, then asserts the installed stub runs. Review feedback on #575. * ci: grant the 3.2 gate pull-requests read for paths-filter dorny/paths-filter enumerates changed PR files via the REST API and documents a pull-requests: read requirement; the job-level permissions block zeroes everything unlisted. Review feedback on #575.

Every CLI command runs root PersistentPreRunE -> appctx.NewApp -> auth.NewManager -> auth.NewStore, and credstore.NewStore eagerly probes OS keyring availability with a write. On macOS that probe shells out to an uncancellable `security` child; a locked keychain with no TTY or GUI blocks it forever — on *every* command, including credential-free ones like `setup agents` and `skill install`. That is how the installer hung headless (#568, canary discovery #569/#571 -> #574 drill lineage). Go fix (reaches users at the next release): Store now records the fallback dir at construction and builds the credstore lazily behind sync.Once on first credential operation. Credential-free commands never touch the keyring; credential-touching commands probe on first use exactly as before, with the documented BASECAMP_NO_KEYRING=1 escape hatch. No timeout was added deliberately: an abandoned goroutine cannot kill the hung `security` child, so a timeout would hide the hang while keeping the leak. Bounded fallback needs cancellation support upstream in credstore/go-keyring first. Installer belt (deploys on merge, covers released binaries <= v0.7.2 which still probe eagerly): post_install_setup in install.sh prefixes each best-effort child with per-command BASECAMP_NO_KEYRING=1; Invoke-PostInstallSetup in install.ps1 sets it for the duration of setup and restores the caller's value on exit. These children never touch credentials — the var only skips the old binaries' startup probe. Tests: keyring_test.go pins constructor laziness through an injectable credstore seam. installer.bats gains belt tests that explicitly unset the suite-wide BASECAMP_NO_KEYRING and assert both directions (real calls carry it, the sh capability probe does not), plus a pwsh test that extracts Invoke-PostInstallSetup from install.ps1's AST (no test hooks in the production script), runs it against a logging stub, and asserts set + restore; it fails closed when pwsh is missing in CI. The canary's workflow-level BASECAMP_NO_KEYRING is removed with no step-scoped replacement: the macOS leg now genuinely exercises the deployed belt (a missing belt = hung security child = timeout = notify), and the direct `basecamp version` asserts skip PersistentPreRunE entirely. Refs #558 #568 #569

* Defer keyring probe to first credential use; belt the installers Every CLI command runs root PersistentPreRunE -> appctx.NewApp -> auth.NewManager -> auth.NewStore, and credstore.NewStore eagerly probes OS keyring availability with a write. On macOS that probe shells out to an uncancellable `security` child; a locked keychain with no TTY or GUI blocks it forever — on *every* command, including credential-free ones like `setup agents` and `skill install`. That is how the installer hung headless (#568, canary discovery #569/#571 -> #574 drill lineage). Go fix (reaches users at the next release): Store now records the fallback dir at construction and builds the credstore lazily behind sync.Once on first credential operation. Credential-free commands never touch the keyring; credential-touching commands probe on first use exactly as before, with the documented BASECAMP_NO_KEYRING=1 escape hatch. No timeout was added deliberately: an abandoned goroutine cannot kill the hung `security` child, so a timeout would hide the hang while keeping the leak. Bounded fallback needs cancellation support upstream in credstore/go-keyring first. Installer belt (deploys on merge, covers released binaries <= v0.7.2 which still probe eagerly): post_install_setup in install.sh prefixes each best-effort child with per-command BASECAMP_NO_KEYRING=1; Invoke-PostInstallSetup in install.ps1 sets it for the duration of setup and restores the caller's value on exit. These children never touch credentials — the var only skips the old binaries' startup probe. Tests: keyring_test.go pins constructor laziness through an injectable credstore seam. installer.bats gains belt tests that explicitly unset the suite-wide BASECAMP_NO_KEYRING and assert both directions (real calls carry it, the sh capability probe does not), plus a pwsh test that extracts Invoke-PostInstallSetup from install.ps1's AST (no test hooks in the production script), runs it against a logging stub, and asserts set + restore; it fails closed when pwsh is missing in CI. The canary's workflow-level BASECAMP_NO_KEYRING is removed with no step-scoped replacement: the macOS leg now genuinely exercises the deployed belt (a missing belt = hung security child = timeout = notify), and the direct `basecamp version` asserts skip PersistentPreRunE entirely. Refs #558 #568 #569 * test(installer): pin restore of a caller-set BASECAMP_NO_KEYRING The pwsh belt test proved restore-to-unset only; a regression that overwrites an existing caller value would have passed. Second driver pass sets the var to 0 before Invoke-PostInstallSetup and asserts it survives. Review feedback on #578.

…56) * credstore: forced file-only construction and a bounded keyring probe NewStore probes keyring availability with keyring.Set, which on darwin execs /usr/bin/security -i and Wait()s with no cancellation path. On a locked keychain with no TTY or GUI the child blocks forever and cannot be reclaimed from outside go-keyring, so every credential-touching caller hangs unbounded (basecamp/basecamp-cli#568). Two additions to StoreOptions: * ForceFile forces file-backed storage with no keyring probe — the programmatic equivalent of DisableEnvVar, and the fallback target a caller reaches for after a probe timeout. Like DisableEnvVar, an explicit opt-out carries no fallback warning. * ProbeTimeout bounds the availability probe. Zero keeps today's unbounded probe. When set, the darwin probe mirrors go-keyring's Set (security -i fed add-generic-password over stdin, same escaping and base64 password encoding) via exec.CommandContext, so expiry kills the child instead of leaking it. Non-darwin backends run in-process with no child to reclaim, so the bound there is a plain goroutine timeout. A timed-out probe falls back to file storage with the same warning as a failed probe. Fixes #55 * credstore: document non-positive ProbeTimeout as unbounded The probe treats any non-positive timeout as unbounded (timeout <= 0), but the ProbeTimeout and probe doc comments claimed only zero did. Align both comments with the behavior. * credstore: run probe cleanup under its own budget A successful add may consume the probe deadline; the delete then ran under the exhausted probe context, was skipped or killed, and the probe still returned nil — leaving a stray __probe_* entry in the keychain. Cleanup now gets a fresh bounded context (probeCleanupTimeout) and remains best-effort: the add already proved availability, so a failed delete must not fail the probe. afterProbeAdd is a test seam pinning cleanup's independence from the probe context. * credstore: quote stub paths and bound the PID watcher The generated security stubs embedded pid/args file paths into shell source unquoted, so a space-containing TMPDIR broke the redirections and could hang the kill test's PID watcher indefinitely. Stub paths are now shell-quoted and every stub lives in a dir-with-space so the quoting is pinned on all machines; the watcher observes a deadline and fails fast on fixture breakage. Adds the cleanup-survives-expiry regression for the probe fix. * credstore: fall back to PID+time probe key on rand failure probeKeyName ignored rand.Read errors: a failed or short read yields a predictable (zeroed) key, raising the odds of colliding with an existing entry — which cleanup would then delete. The fallback keys on PID + wall clock, unique enough for a transient probe entry, behind a randRead seam so the failure path is testable. * credstore: run probe cleanup asynchronously A slow cleanup delete under its fresh five-second budget could stretch NewStore past the caller's configured ProbeTimeout, contradicting the documented guarantee that ProbeTimeout bounds the probe. The best-effort delete now runs in a fire-and-forget goroutine: probe latency stays within ProbeTimeout, cleanup keeps its own budget, and the securityPath value is captured synchronously so the goroutine never races the test seam. * credstore: treat short rand reads like rand failures probeKeyName's comment promised protection against failed or short reads but the code only checked the error. A short read now takes the same PID + time fallback, pinned by TestProbeKeyNameShortRead. * credstore: keep probe cleanup joined; document the bounded tail Cleanup returns to synchronous execution under its own budget: Go does not wait for goroutines at process exit, so the detached cleanup could leak one __probe_* entry per short-lived CLI invocation — the primary consumer shape. The honest contract lives in the ProbeTimeout docs now: worst-case construction is ProbeTimeout plus the five-second cleanup bound, typically milliseconds since cleanup only runs when the keyring just proved responsive. Leaking keychain state was judged worse than a theoretical latency tail. Also from the same review round: DisableEnvVar/ForceFile docs now say explicitly that the *named environment variable* being non-empty is what forces file mode; the fallback probe key gains a per-process atomic sequence (UnixNano can repeat on coarse clocks), and the fallback-shape regexes cover the new __probe_<pid>_<nanos>_<seq> format. * credstore: align expiry-regression comment with the joined cleanup Leftover from the asynchronous interlude: the comment claimed detached cleanup and a hard ProbeTimeout-only bound, contradicting the documented additive bound the code now implements. * credstore: use a deterministic probe key so leaks self-heal A timed-out non-darwin probe abandons its goroutine; if the blocked Set later succeeds and the process exits before Delete, the probe entry leaks — and go-keyring has no list API, so under a random name the leftover is permanently unfindable. A fixed, reserved __probe__ name makes the leak self-healing: the next probe's Set overwrites it and its Delete removes it. Concurrent probes sharing the name are harmless — Set results are unaffected and the losing Delete's failure is already ignored. Supersedes the random-key machinery (rand fallback, PID+time key, per-process sequence) and its tests. * credstore: isolate the probe in its own service namespace A private constant does not enforce a name reservation: a service that already held an account named __probe__ would have its password overwritten by the probe's Set and removed by its Delete. The probe now operates in serviceName + ".probe", so it cannot touch any credential in the caller's real service, whatever its name — while keeping the deterministic key's self-healing property within the probe namespace. Also from this round: ProbeTimeout docs scope the additive cleanup bound to darwin (elsewhere cleanup is part of the probe itself), and stale __probe_* wording no longer implies the removed randomized scheme. * credstore: reserve a declared credstore.probe namespace A bare .probe suffix on the caller's service is an ordinary global keyring name, not a reservation. The probe now writes under credstore.probe.<ServiceName>, and the ProbeTimeout docs publicly declare that namespace as reserved by this package — a colliding consumer would have to deliberately adopt the declared namespace, at which point the deterministic key's self-healing semantics are exactly what they'd share. * credstore: pin the probe leak-containment contract cross-platform The deterministic-name containment for abandoned probes (a timed-out non-darwin probe whose blocked Set completes after process exit leaves at most one known entry, overwritten and removed by the next probe) was only exercised by darwin-local tests — invisible to ubuntu CI. A pure derivation helper and a platform-independent test now pin the reserved service and account names; changing either would orphan entries written by earlier releases. * credstore: document that bounded darwin probes sit below MockInit The child-process probe exists precisely to operate beneath go-keyring's uncancellable exec layer, so it cannot observe the mocked provider — go-keyring exports no way to detect MockInit. State the limitation on ProbeTimeout with the test guidance: mocked-keyring tests use a zero timeout (probeDirect honors the mock) or ForceFile.

Stderr alone misclassified routine interactive use: `2>auth.log` from a terminal would have bounded the probe and, on a >10s unlock answer, silently degraded to plaintext file storage with the warning hidden in the redirected log. Headless now requires stdin, stdout, and stderr to all be non-terminals — any attached stream means a human can answer the unlock prompt on the controlling terminal or the GUI. Review feedback on #581.

All three stdio streams being non-terminals is not proof of headlessness: a macOS app or IDE task runner launches the CLI fully detached while the user can still answer the keychain unlock dialog on the WindowServer. Headless now additionally requires no GUI session — on darwin via `launchctl managername` != Aqua (bounded, cannot touch the keychain, failures count as no GUI so true headless keeps the bounded probe); elsewhere via DISPLAY/WAYLAND_DISPLAY, with Windows always false since Credential Manager never prompts. Review feedback on #581.

The !darwin implementation's claim that Windows is always non-GUI was false — that build returned true on any platform with DISPLAY set. A dedicated session_windows.go hard-codes false with the rationale, the unix implementation drops the stale claim, and the env test gains the Wayland-only branch. Review feedback on #581.

* Bound the keyring probe on headless sessions (Refs #568) Bumps github.com/basecamp/cli to the post-#56 credstore (v0.2.2-0.20260728023309-04e401b12c6c), which adds StoreOptions.ProbeTimeout: a bounded availability probe that kills the hung `security` child on darwin and falls back to file storage as if the probe had failed. The CLI wires it interactivity-aware: when stderr is not a terminal (CI, piped installers, ssh without a TTY) the probe is bounded at ten seconds — a headless session can never answer a keychain unlock prompt, so hanging forever in an uncancellable child was the only alternative (the #568 incident class, previously mitigated only by lazy construction and the BASECAMP_NO_KEYRING escape hatch). Interactive sessions keep the unbounded probe: a locked keychain there raises an unlock prompt, and cutting it off mid-answer would silently degrade the user to plaintext file storage. ForceFile is deliberately not wired: credstore's internal timeout-to-file fallback covers the CLI's need, and the env-var path already provides forced file mode. * Treat only fully detached sessions as headless (Refs #568) Stderr alone misclassified routine interactive use: `2>auth.log` from a terminal would have bounded the probe and, on a >10s unlock answer, silently degraded to plaintext file storage with the warning hidden in the redirected log. Headless now requires stdin, stdout, and stderr to all be non-terminals — any attached stream means a human can answer the unlock prompt on the controlling terminal or the GUI. Review feedback on #581. * Recognize GUI sessions before bounding the probe (Refs #568) All three stdio streams being non-terminals is not proof of headlessness: a macOS app or IDE task runner launches the CLI fully detached while the user can still answer the keychain unlock dialog on the WindowServer. Headless now additionally requires no GUI session — on darwin via `launchctl managername` != Aqua (bounded, cannot touch the keychain, failures count as no GUI so true headless keeps the bounded probe); elsewhere via DISPLAY/WAYLAND_DISPLAY, with Windows always false since Credential Manager never prompts. Review feedback on #581. * Split the Windows GUI-session stub into its own file (Refs #568) The !darwin implementation's claim that Windows is always non-GUI was false — that build returned true on any platform with DISPLAY set. A dedicated session_windows.go hard-codes false with the rationale, the unix implementation drops the stale claim, and the env test gains the Wayland-only branch. Review feedback on #581.

Fix piped installer under nounset

dc145ee

Copilot AI review requested due to automatic review settings July 27, 2026 16:21

github-actions Bot added tests Tests (unit and e2e) bug Something isn't working labels Jul 27, 2026

Copilot started reviewing on behalf of pradhankukiran July 27, 2026 16:22 View session

pradhankukiran marked this pull request as ready for review July 27, 2026 16:25

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread e2e/installer.bats Outdated

cubic-dev-ai Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread e2e/installer.bats Outdated

Use current Bash in installer regression test

0f3b638

Copilot AI review requested due to automatic review settings July 27, 2026 16:36

Copilot started reviewing on behalf of pradhankukiran July 27, 2026 16:37 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

jeremy approved these changes Jul 27, 2026

View reviewed changes

jeremy merged commit 93d31a4 into basecamp:main Jul 27, 2026
26 checks passed

jeremy mentioned this pull request Jul 27, 2026

Add post-deploy installer smoke canary #569

Merged

This was referenced Jul 27, 2026

ci: harden installer canary and add pre-merge Bash 3.2 gate #575

Merged

Defer keyring probe to first credential use; belt the installers #578

Merged

credstore: forced file-only construction and a cancellable keyring probe basecamp/cli#55

Closed

jeremy mentioned this pull request Jul 28, 2026

Bound the keyring probe on headless sessions #581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix piped installer under nounset - #568

Fix piped installer under nounset#568
jeremy merged 2 commits into
basecamp:mainfrom
pradhankukiran:fix/piped-installer-nounset

pradhankukiran commented Jul 27, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

pradhankukiran commented Jul 27, 2026

Uh oh!

jeremy commented Jul 27, 2026

Uh oh!

jeremy left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pradhankukiran commented Jul 27, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Testing

Summary by cubic

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

pradhankukiran commented Jul 27, 2026

Uh oh!

jeremy commented Jul 27, 2026

Uh oh!

jeremy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pradhankukiran commented Jul 27, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading