Defer keyring probe to first credential use; belt the installers

Merged
JE
3mo ago
+252-51

Stacked on #575 (retargets to main when it merges). Draft until #575 lands.

The hang is CLI-wide, not a CI quirk

Every command runs root PersistentPreRunE → appctx.NewApp → auth.NewManager → auth.NewStore → credstore.NewStore, which eagerly probes OS keyring availability with a write. On macOS that probe shells out to an uncancellable security child (go-keyring keyring_darwin.go); a locked keychain with no TTY or GUI blocks forever — on every command, including credential-free ones like setup agents and skill install. That's how the headless installer hung (#568; canary discovery #569).

The fix (two layers)

Go — lazy construction (internal/auth/keyring.go): NewStore records the fallback dir; the credstore is built behind sync.Once on first credential operation. Credential-free commands (setup agents, skill install, completion, etc.) never touch the keyring. Credential-touching commands probe on first use exactly as before, with the documented BASECAMP_NO_KEYRING=1 escape hatch. Reaches users at the next release.

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 old binaries' startup probe. The setup --help capability probes in install.sh stay bare (help short-circuits PersistentPreRunE).

Deliberately no probe timeout: an abandoned goroutine cannot kill the hung security child, so a timeout would hide the hang while preserving the resource leak. Bounded fallback for credential-touching commands is deferred pending cancellation support upstream in credstore/go-keyring (issue to follow on basecamp/cli).

Canary re-scope

The 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 = 10-minute timeout = notify), and the direct basecamp version asserts skip PersistentPreRunE entirely (root.go handles help/version before app construction). Once a release with the lazy fix ships, the canary stops distinguishing belt presence; the bats + pwsh tests remain the durable belt verification.

Tests

  • keyring_test.go: counting-fake seam proves the constructor makes zero credstore calls, first operation makes one, subsequent operations reuse it. (The "credential-free commands never probe" conclusion additionally rests on the audited command paths — stated in the test comment, not overclaimed.)
  • installer.bats: belt tests explicitly unset the suite-wide BASECAMP_NO_KEYRING and assert both directions — real setup agents/skill install calls log nk=1, the sh setup --help probe logs nk=unset (belt is scoped, not blanket).
  • pwsh behavioral test (the Windows canary can never prove the ps1 belt — Credential Manager works headlessly either way): extracts Invoke-PostInstallSetup from install.ps1's AST via Parser.ParseFile and evaluates only that function — no test hooks or skip-main env vars in the production script — then asserts real calls see 1 and the env var is restored (removed) after return. Skips locally without pwsh; fails closed in CI.

Verification

  • go test ./internal/auth/... and full bin/ci green; bats e2e/installer.bats 14 pass + pwsh skip locally.
  • pwsh test validated end-to-end in a PowerShell container (arm64): restored-ok, nk=1 setup agents.
  • Manual lazy proof on macOS: credential-free completion bash averages 105ms on the eager binary vs 43ms lazy — the ~60ms delta is the keyring probe, gone. auth status output is byte-identical eager vs lazy with the keyring active, and unchanged under BASECAMP_NO_KEYRING=1.
  • After merge: the push canary fires on both scripts (post-#575 paths) — the macOS leg is genuine belt coverage; Linux and Windows are deployment smoke. Plus one manual cache-busted curl … install-cli?sha=<merge-sha> | bash in a scratch HOME without exporting BASECAMP_NO_KEYRING.

Refs #558 #568 #569

Summary by cubic

Defers the OS keyring probe to first credential use and adds a scoped installer “belt” that sets BASECAMP_NO_KEYRING=1 to prevent hangs on headless macOS and in older releases (≤ v0.7.2). Also updates CI to exercise the belt directly.

  • Bug Fixes

    • Lazy keyring: internal/auth/keyring.go now builds credstore.Store on first use via sync.Once with a test seam newCredStore. Credential-free commands never touch the keyring; credentialed paths are unchanged and still honor BASECAMP_NO_KEYRING.
    • Installer belt: scripts/install.sh prefixes real setup/skill calls with BASECAMP_NO_KEYRING=1 while keeping setup --help bare; scripts/install.ps1 sets BASECAMP_NO_KEYRING=1 for the duration of Invoke-PostInstallSetup and restores any caller value on exit. .github/workflows/installer-smoke.yml removes the workflow-level var so macOS canaries validate the deployed belt.

  • Tests

    • Added internal/auth/keyring_test.go to pin lazy construction. Expanded e2e/installer.bats to assert the sh belt and a PowerShell pass that verifies both restoring to unset and preserving a caller-set BASECAMP_NO_KEYRING value.