Add post-deploy installer smoke canary - #569
Merged
Merged
Conversation
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
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
There was a problem hiding this comment.
Pull request overview
Adds a post-deploy GitHub Actions canary to validate the published installer (the documented curl … | bash path) after changes to scripts/install.sh are merged, plus clarifying comment updates from the #568 incident review to reduce risk of regressions.
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:
- Add a new
installer-smokeworkflow that installs via the public endpoint on Linux and macOS, using an isolatedHOME/BASECAMP_BIN_DIRand verifying the deterministic installed binary path. - Clarify the installer’s sourcing guard comment to document why the
:-$0fallback is required underset -uwhen Bash runs from stdin /-c. - Clarify the e2e installer test comment to accurately describe the PATH setup used to stop before downloading.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/installer-smoke.yml |
New post-deploy canary that runs `curl … |
scripts/install.sh |
Comment-only clarification explaining the stdin/-c + set -u behavior behind the BASH_SOURCE[0]:-$0 guard. |
e2e/installer.bats |
Comment-only fix to accurately describe the test’s PATH isolation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://cdn.github.com/in/1082092?s=60&v=4){kind=small}
jeremy
added a commit
that referenced
this pull request
Jul 27, 2026
The canary's first macOS run hung for its full 10-minute timeout after "Skipping setup wizard": the post-install skill step reaches for the OS keyring, and on a headless runner the `security` tool blocks forever on the locked keychain (the cancelled job's orphan processes were `basecamp` and `security`). Set BASECAMP_NO_KEYRING=1 workflow-wide, the same escape hatch the Test workflow uses. Refs #569
This was referenced Jul 27, 2026
jeremy
added a commit
that referenced
this pull request
Jul 27, 2026
* 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.
jeremy
added a commit
that referenced
this pull request
Jul 27, 2026
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
jeremy
added a commit
that referenced
this pull request
Jul 27, 2026
* 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A post-deploy canary for the published installer, plus two installer-scoped comment fixes from the #568 incident review.
The install endpoint serves
main, so merging a change toscripts/install.shdeploys it immediately. #558 shipped a sourcing guard that broke the documentedcurl … | bashpath underset -u; #568 fixed it and added a bats regression test that gates pre-merge. Nothing verified the published installer post-merge — this closes that gap.The canary (
.github/workflows/installer-smoke.yml)workflow_dispatch, and push tomaintouchingscripts/install.sh.?sha=${GITHUB_SHA}) — raw.githubusercontent.com servesmax-age=300, so a push-triggered run could otherwise fetch the pre-push installer and falsely pass. The canonical endpoint preserves the query on redirect, making each commit's request cache-distinct.ubuntu-latest(modern bash) andmacos-latestvia stock/bin/bash, which asserts exactly Bash 3.2 through the same interpreter that runs the install — the coverage can't silently degrade if a future runner image changes/bin/bash.HOME+ explicitBASECAMP_BIN_DIRcontain the installer's writes (it edits shell config and installs the baseline skill even withBASECAMP_SKIP_SETUP=1/BASECAMP_SETUP_AGENT=none). Asserts the deterministic installed path, not ambient PATH.permissions: {},timeout-minutes: 10, concurrency group; no actions used, so nothing to pin. actionlint and zizmor clean.Comment fixes
e2e/installer.bats: the piped-execution test's PATH comment said "Keep PATH empty"; it actually points at a binary-free tmpdir.scripts/install.sh: document why the guard's:-$0fallback is required (bash from stdin or-cleavesBASH_SOURCEunset, aborting underset -u) so a future cleanup doesn't re-break piping.Verification
bats e2e/installer.bats— 12/12 pass.bin/cigreen.workflow_dispatchthe canary after merge to confirm both legs.Refs #558, #568
Summary by cubic
Adds a post-deploy smoke canary that installs the published CLI via the Quick Start on Linux and macOS. Also updates installer comments and sets explicit workflow concurrency behavior.
New Features
.github/workflows/installer-smoke.yml: runs nightly, on demand, and whenscripts/install.shchanges;permissions: {},timeout-minutes: 10, concurrency group withcancel-in-progress: false.https://basecamp.com/install-cli?sha=${GITHUB_SHA}to bust CDN cache; verifies the binary at the deterministic install path.ubuntu-latestandmacos-latest; macOS uses/bin/bashand asserts Bash 3.2; uses scratchHOMEandBASECAMP_BIN_DIRto isolate writes.Bug Fixes
e2e/installer.batsto note a binary-free tmpdir.${BASH_SOURCE[0]:-$0}is required inscripts/install.shto keepcurl | bashworking withset -u.Written for commit 48bd75c. Summary will update on new commits.