Fix #528: non-TTY installer hardcodes the Claude agent - #558
Merged
Conversation
The piped install (`curl … | bash`) takes the non-TTY branch of scripts/install.sh, which unconditionally ran `basecamp setup claude`. The post-login nudge likewise pointed at the first detected-unhealthy agent in registry order (Claude-first when both were present). Add `basecamp setup agents`: a non-interactive command that installs the baseline skill and connects coding agents by intent, not registry order. Selection is driven by BASECAMP_SETUP_AGENT (claude|codex|all|none); unset auto-detects — 0 detected installs the skill only, 1 connects it, ≥2 stays neutral and surfaces the per-agent commands. Every safety-critical outcome lives in a top-level flat field of the result envelope so it survives the styled renderer (which skips nested map/[]map); the `agents` array is JSON-only detail. `all` forces every registered handler and synthesizes symmetric missing-binary remediation (the Claude handler treats an absent binary as no-op success while Codex errors). - install.sh / install.ps1: both non-TTY and skip branches now run `setup agents`; install.sh gains an if-form source guard and documents BASECAMP_SETUP_AGENT. - auth.go: rewrite printAgentNudge — one detected-unhealthy agent prints its `setup <id>`; several print every choice directly, never guessing. - Regenerate .surface; add smoke declaration and e2e/installer.bats.
github-actions
Bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76dc027cdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes the non-TTY installer and post-login “agent nudge” bias by introducing an intent-based, non-interactive basecamp setup agents command and routing non-interactive install paths through it, avoiding Claude-first guessing when multiple agents are present.
Changes:
- Added
basecamp setup agents(non-interactive) to install the baseline skill and connect agents based onBASECAMP_SETUP_AGENTor auto-detection. - Updated Bash/PowerShell installers to run
setup agentsin non-interactive / skip-wizard paths (instead of hardcodingsetup claude) and added a sourcing guard toinstall.sh. - Updated the login nudge logic to avoid Claude-first ordering and to surface explicit per-agent
basecamp setup <id>choices.
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.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Routes non-interactive install flows through setup agents; adds sourcing guard and documents BASECAMP_SETUP_AGENT. |
| scripts/install.ps1 | Runs setup agents for skip/non-interactive install flows and documents env selectors. |
| internal/commands/wizard.go | Registers the new setup agents subcommand under setup. |
| internal/commands/wizard_agents.go | Implements setup agents selection logic and the flat, styled-safe result envelope. |
| internal/commands/setup_agents_test.go | Adds JSON + styled-output regression coverage for setup agents behaviors and ordering contracts. |
| internal/commands/auth.go | Updates printAgentNudge to avoid “Claude-first” nudging and list explicit choices when needed. |
| internal/commands/auth_test.go | Adds test coverage for nudge behavior across none/single/multiple unhealthy detections. |
| install.md | Documents piped-installer behavior and how to force agent selection via BASECAMP_SETUP_AGENT. |
| e2e/smoke/smoke_lifecycle.bats | Marks setup agents as out-of-scope for smoke lifecycle. |
| e2e/installer.bats | Adds e2e assertions that installers dispatch to setup agents and that install.sh is safe to source. |
| .surface | Regenerates CLI surface snapshot to include basecamp setup agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…kill docs Three follow-up fixes from review of #558: 1. Cross-version installer strategy (install.sh + install.ps1). The hosted install script from main can outrun the latest release, whose binary lacks `setup agents` (v0.7.2 has only `setup`/`setup claude`/`skill install`). Both scripts now probe the installed binary for `setup agents` support and, on an older binary, fall back WITHOUT reintroducing the Claude-first bug: only an explicitly selected agent is connected; an unset/auto/ambiguous selector installs the shared skill via `skill install`. Added old-binary regression tests in e2e/installer.bats. 2. Guard PowerShell post-install setup. Both calls now route through a single best-effort Invoke-PostInstallSetup helper wrapped in try/catch, so a nonzero native exit (under $ErrorActionPreference='Stop' + $PSNativeCommandUseErrorActionPreference) can no longer terminate an otherwise-successful install. 3. Skill docs. Document `setup agents` + BASECAMP_SETUP_AGENT in skills/basecamp/SKILL.md and add it to the remediation list in skills/basecamp-doctor/SKILL.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 267c7efebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… binaries - setup agents: add cobra.NoArgs — selection is env-driven, so positional args (typos, or confusion with `setup <id>`) are now rejected, not silently ignored. - install.sh / install.ps1: on old release binaries lacking `setup agents`, BASECAMP_SETUP_AGENT=all now dispatches every per-agent `setup <id>` the binary supports instead of collapsing to skill-only. Explicit "all" is honored intent, distinct from the intent-neutral unset/auto/none fallback. PowerShell calls are now individually guarded so one agent's failure can't skip the rest. - Tests: NoArgs rejection (Go) + `all` old-binary dispatch (installer.bats).
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
On a pre-`setup agents` release (v0.7.2 advertises only `setup claude`), an explicit BASECAMP_SETUP_AGENT=codex ran `basecamp setup codex`. The old `setup` parent accepts `codex` as a stray positional arg and launches the INTERACTIVE setup/OAuth wizard — violating the non-interactive contract. Both installers now capability-check explicit claude|codex selectors the same way `all` already did: only dispatch `setup <id>` if the binary advertises it, otherwise degrade to `skill install` and never invoke the unknown subcommand. Tests: the old-binary stub now rejects every unadvertised `setup <sub>` (not just `setup agents`), and new coverage asserts explicit `codex` degrades to `skill install` (install.sh) plus a static guard on install.ps1's capability check.
2 tasks
jeremy
added a commit
that referenced
this pull request
Jul 27, 2026
* 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
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.
Fixes #528.
Both symptoms
setup claude— the piped install (curl -fsSL https://basecamp.com/install-cli | bash, a 302 redirect to this repo'sscripts/install.sh) took the non-TTY branch, which unconditionally ranbasecamp setup claude. TheBASECAMP_SKIP_SETUP=1branch did the same.printAgentNudgepointed at the first detected-unhealthy agent in registry order, so it always recommended Claude when both agents were present.Approach — detection proves presence, not intent
New
basecamp setup agentscommand (always non-interactive): installs the baseline skill and connects coding agents by intent.BASECAMP_SETUP_AGENTwins:claude|codex|all|none.setup <id>choices (never guess).allforces every registered handler and synthesizes symmetric missing-binary remediation — the Claude handler treats an absent binary as no-op success while Codex errors, so the aggregate runner fills the gap for either.BASECAMP_SKIP_SETUP=1skips only the interactive wizard;setup agentsstill runs.Styled-safe result envelope
curl | bashrenders styled output (piped stdin, TTY stdout), and the styled renderer skips nestedmap/[]map. So every safety-critical outcome —skill_installed,selector,ambiguous,detected_before,attempted_agents,errors(union of baseline + id-prefixed per-agent),warnings,manual_commands— lives in a top-level flat field; theagentsarray is JSON-only detail. Ordering is deterministic: set-like fields sorted by id;manual_commands/errorspreserve each handler's own sequence with stable first-seen dedup (Codex's remediation is an ordered marketplace-add → upgrade → plugin-add sequence, never lexicographically sorted).Changes
internal/commands/wizard_agents.go—newSetupAgentsCmd+runNonInteractiveAgentSetupand the typed envelope. Existingsetup claude/setup codexunchanged.internal/commands/auth.go— rewriteprintAgentNudge: one detected-unhealthy agent → itssetup <id>; several → every choice printed directly.scripts/install.sh/scripts/install.ps1— both non-TTY and skip branches runsetup agents; install.sh gains an if-form source guard and documentsBASECAMP_SETUP_AGENT..surfaceregenerated (onlysetup agentsadditions); smoke declaration + newe2e/installer.bats;install.mdupdated.Tests
internal/commands/setup_agents_test.go— full-CLI (cobra Execute) coverage: styled-output regression (failure survives via top-levelerrors), 0/1/≥2 detection, baseline-skill failure,=allzero/one/two,=codexmissing-binary contract, Codex manual-order preservation (guards against string-sorting),=claudeno-binary (symlink + detection flip + synthesized remediation),=none, invalid selector.internal/commands/auth_test.go— nudge: none / single / multiple detected-unhealthy.e2e/installer.bats— sourcing guard,post_install_setupdispatch (non-TTY + skip →setup agents, neversetup claude), static branch assertions for both scripts.Verified:
make build, targetedgo test,make check-surface/check-skill-drift/check-bare-groups/check-naming/check-smoke-coverage/fmt-check/vet/lint, and the smoke + installer bats suites all green. (The full unit suite has a pre-existing network-dependent todos-reposition test that hangs offline; unrelated to this change.)Summary by cubic
Replaces the non-TTY installer’s hardcoded Claude path with an intent-based, non-interactive agent setup. Adds
basecamp setup agents, makes installers cross-version safe, and prevents old releases from launching the interactive wizard.New Features
basecamp setup agents(non-interactive): installs the baseline skill and connects coding agents by intent; honorsBASECAMP_SETUP_AGENT=claude|codex|all|none(unset auto-detects: 0→skill only, 1→connect it, ≥2→skill only + surfacesetup <id>choices).scripts/install.sh,scripts/install.ps1) runsetup agentsfor non-TTY andBASECAMP_SKIP_SETUP=1. Docs updated with piped-install auto-connect and how to setBASECAMP_SETUP_AGENTin Bash/PowerShell.agentsarray is JSON-only detail.Bug Fixes
setup agents. On older binaries, fall back without reintroducing Claude-first behavior—allruns each supported per-agent setup; explicitclaude/codexis capability-checked and degrades toskill installif unsupported; all calls are guarded so failures never abort install.Invoke-PostInstallSetup: marks attempted agents on dispatch (not success) to match Bash, so=allonly falls back toskill installwhen no supported per-agent subcommands exist.setup agentsrejects positional args to prevent accidentalsetup <id>typos.basecamp setup <id>option (no bias).Written for commit 5cdc4ec. Summary will update on new commits.