Defer keyring probe to first credential use; belt the installers

.github/workflows/installer-smoke.yml

@@ -17,13 +17,13 @@ on:
 
 permissions: {}
 
-# The post-install skill step reaches for the OS keyring; on a headless macOS
-# runner the `security` tool blocks forever on the locked keychain (caught by
-# this canary's first run). Also keeps the Windows legs out of Credential
-# Manager. Same escape hatch the Test workflow uses.
-env:
-  BASECAMP_NO_KEYRING: "1"
-
+# Deliberately NO workflow-level BASECAMP_NO_KEYRING: the deployed installer
+# sets it per-command for its own best-effort children (the belt for release
+# binaries <= v0.7.2, whose eager startup keyring probe hangs on a locked
+# headless keychain), and the canary must exercise that deployed belt — on the
+# macOS leg a missing belt means a hung `security` child, a 10-minute timeout,
+# and a notify. The direct `basecamp version` asserts don't need the var:
+# help/version skip the CLI's PersistentPreRunE and never reach the probe.
 concurrency:
   group: installer-smoke-${{ github.ref }}
   cancel-in-progress: false

e2e/installer.bats

@@ -48,6 +48,25 @@ write_stub() {
   chmod +x "$STUB_DIR/basecamp"
 }
 
+# write_nk_stub emits a stub that logs BASECAMP_NO_KEYRING alongside argv, for
+# asserting the escape-hatch belt. Separate from write_stub so the existing
+# argv-shape assertions stay byte-exact. mode mirrors write_stub's new/old.
+write_nk_stub() {
+  local mode="$1"
+  {
+    echo '#!/usr/bin/env bash'
+    echo "echo \"nk=\${BASECAMP_NO_KEYRING:-unset} \$@\" >> \"$LOG\""
+    echo 'if [[ "$1 $2" == "setup --help" ]]; then'
+    if [[ "$mode" == "new" ]]; then
+      echo '  echo "  agents  Install the Basecamp skill and connect detected coding agents"'
+    fi
+    echo '  exit 0'
+    echo 'fi'
+    echo 'exit 0'
+  } > "$STUB_DIR/basecamp"
+  chmod +x "$STUB_DIR/basecamp"
+}
+
 run_post_install_setup() {
   run bash -c "
     set -euo pipefail
@@ -157,4 +176,102 @@ run_post_install_setup() {
   # Explicit claude|codex selectors must be capability-checked before dispatch,
   # so an old binary never gets an unadvertised subcommand as a stray arg.
   grep -qF 'match "(?m)^\s+$selector\s"' "$INSTALL_PS1"
+  # The keyring escape hatch belt (see the BASECAMP_NO_KEYRING tests below).
+  grep -q 'BASECAMP_NO_KEYRING' "$INSTALL_PS1"
+}
+
+# Release binaries up to v0.7.2 probe the OS keyring on startup for every
+# command, and a locked headless keychain blocks that probe forever (#568
+# canary discovery). The installer's best-effort children never touch
+# credentials, so they must carry BASECAMP_NO_KEYRING=1 — per-command, not
+# blanket: the `setup --help` capability probe stays bare (help
+# short-circuits before the probe). e2e/run.sh exports BASECAMP_NO_KEYRING
+# suite-wide, so the test shell must unset it first or the stub would see
+# nk=1 even without the installer's belt.
+@test "new binary: real setup calls carry BASECAMP_NO_KEYRING=1, capability probe does not" {
+  write_nk_stub new
+  run bash -c "
+    set -euo pipefail
+    unset BASECAMP_NO_KEYRING
+    source '$INSTALL_SH'
+    BIN_DIR='$STUB_DIR'
+    post_install_setup basecamp
+    cat '$LOG'
+  "
+  [[ "$status" -eq 0 ]]
+  [[ "$output" == *"nk=unset setup --help"* ]]
+  [[ "$output" == *"nk=1 setup agents"* ]]
+}
+
+@test "old binary: skill install fallback carries BASECAMP_NO_KEYRING=1" {
+  write_nk_stub old
+  run bash -c "
+    set -euo pipefail
+    unset BASECAMP_NO_KEYRING
+    source '$INSTALL_SH'
+    BIN_DIR='$STUB_DIR'
+    post_install_setup basecamp
+    cat '$LOG'
+  "
+  [[ "$status" -eq 0 ]]
+  [[ "$output" == *"nk=unset setup --help"* ]]
+  [[ "$output" == *"nk=1 skill install"* ]]
+}
+
+# The Windows canary can never prove the ps1 belt — Credential Manager works
+# headlessly with or without it — so pin the behavior here. The function under
+# test is extracted from install.ps1's AST and evaluated alone: Main never
+# runs, and the installer needs no test hooks.
+@test "install.ps1 Invoke-PostInstallSetup sets and restores BASECAMP_NO_KEYRING" {
+  if ! command -v pwsh >/dev/null 2>&1; then
+    if [[ -n "${CI:-}" ]]; then
+      # Fail closed: a runner image dropping pwsh must surface as a failure,
+      # not silent coverage loss.
+      echo "pwsh is required in CI for install.ps1 belt coverage" >&2
+      return 1
+    fi
+    skip "pwsh not installed"
+  fi
+
+  PS_LOG="$STUB_DIR/ps-calls.log"
+
+  cat > "$STUB_DIR/basecamp-stub.ps1" <<'EOF'
+$rest = $args -join ' '
+$nk = if ($null -eq $env:BASECAMP_NO_KEYRING) { 'unset' } else { $env:BASECAMP_NO_KEYRING }
+Add-Content -LiteralPath $env:PS_LOG "nk=$nk $rest"
+if ($rest -eq 'setup --help') {
+  '  agents  Install the Basecamp skill and connect detected coding agents'
+}
+EOF
+
+  cat > "$STUB_DIR/driver.ps1" <<'EOF'
+$ErrorActionPreference = 'Stop'
+$tokens = $null; $parseErrors = $null
+$ast = [System.Management.Automation.Language.Parser]::ParseFile($env:INSTALL_PS1_PATH, [ref]$tokens, [ref]$parseErrors)
+if ($parseErrors.Count -gt 0) { throw "install.ps1 parse errors: $($parseErrors -join '; ')" }
+$fn = $ast.Find({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq 'Invoke-PostInstallSetup' }, $true)
+if (-not $fn) { throw 'Invoke-PostInstallSetup not found in install.ps1' }
+# Evaluate only the function definition — the installer's Main never runs.
+. ([scriptblock]::Create($fn.Extent.Text))
+Invoke-PostInstallSetup $env:PS_STUB
+if ($null -ne $env:BASECAMP_NO_KEYRING) { throw "BASECAMP_NO_KEYRING not restored: '$env:BASECAMP_NO_KEYRING'" }
+'restored-ok'
+# Second pass: a caller-set value must survive, not just absence.
+$env:BASECAMP_NO_KEYRING = '0'
+Invoke-PostInstallSetup $env:PS_STUB
+if ($env:BASECAMP_NO_KEYRING -ne '0') { throw "existing BASECAMP_NO_KEYRING value not restored: '$env:BASECAMP_NO_KEYRING'" }
+'restored-value-ok'
+EOF
+
+  run bash -c "
+    set -euo pipefail
+    unset BASECAMP_NO_KEYRING
+    export PS_LOG='$PS_LOG' PS_STUB='$STUB_DIR/basecamp-stub.ps1' INSTALL_PS1_PATH='$INSTALL_PS1'
+    pwsh -NoProfile -File '$STUB_DIR/driver.ps1'
+    cat '$PS_LOG'
+  "
+  [[ "$status" -eq 0 ]]
+  [[ "$output" == *"restored-ok"* ]]
+  [[ "$output" == *"restored-value-ok"* ]]
+  [[ "$output" == *"nk=1 setup agents"* ]]
 }

internal/auth/keyring.go

@@ -22,33 +22,55 @@ type Credentials struct {
 }
 
 // Store wraps credstore.Store with typed Credentials marshaling.
+//
+// Construction of the underlying credstore.Store is deferred to first use:
+// credstore.NewStore probes the OS keyring availability with a write, and on
+// a locked keychain with no TTY or GUI (headless macOS) that probe blocks
+// forever in an uncancellable `security` child process. Credential-free
+// commands must never pay it.
 type Store struct {
-	inner    *credstore.Store
-	warnOnce sync.Once
+	fallbackDir string
+	initOnce    sync.Once
+	inner       *credstore.Store
+	warnOnce    sync.Once
 }
 
-// NewStore creates a credential store.
+// newCredStore is replaceable in tests to avoid real keyring access.
+var newCredStore = credstore.NewStore
+
+// NewStore creates a credential store. The OS keyring is not touched until
+// the first credential operation.
 func NewStore(fallbackDir string) *Store {
-	s := credstore.NewStore(credstore.StoreOptions{
-		ServiceName:   "basecamp",
-		DisableEnvVar: "BASECAMP_NO_KEYRING",
-		FallbackDir:   fallbackDir,
+	return &Store{fallbackDir: fallbackDir}
+}
+
+// ensure constructs the underlying store on first use. Callers reach here
+// from paths that don't hold Manager.mu (e.g. IsAuthenticated), so the
+// sync.Once provides the synchronization.
+func (s *Store) ensure() *credstore.Store {
+	s.initOnce.Do(func() {
+		s.inner = newCredStore(credstore.StoreOptions{
+			ServiceName:   "basecamp",
+			DisableEnvVar: "BASECAMP_NO_KEYRING",
+			FallbackDir:   s.fallbackDir,
+		})
 	})
-	return &Store{inner: s}
+	return s.inner
 }
 
 // warnFallback prints the keyring fallback warning once, on first credential write.
 func (s *Store) warnFallback() {
+	inner := s.ensure()
 	s.warnOnce.Do(func() {
-		if w := s.inner.FallbackWarning(); w != "" {
+		if w := inner.FallbackWarning(); w != "" {
 			fmt.Fprintf(os.Stderr, "warning: %s\n", w)
 		}
 	})
 }
 
 // Load retrieves credentials for the given origin.
 func (s *Store) Load(origin string) (*Credentials, error) {
-	data, err := s.inner.Load(origin)
+	data, err := s.ensure().Load(origin)
 	if err != nil {
 		return nil, err
 	}
@@ -66,14 +88,14 @@ func (s *Store) Save(origin string, creds *Credentials) error {
 	if err != nil {
 		return err
 	}
-	return s.inner.Save(origin, data)
+	return s.ensure().Save(origin, data)
 }
 
 // Delete removes credentials for the given origin.
-func (s *Store) Delete(origin string) error { return s.inner.Delete(origin) }
+func (s *Store) Delete(origin string) error { return s.ensure().Delete(origin) }
 
 // MigrateToKeyring migrates credentials from file to keyring.
-func (s *Store) MigrateToKeyring() error { return s.inner.MigrateToKeyring() }
+func (s *Store) MigrateToKeyring() error { return s.ensure().MigrateToKeyring() }
 
 // UsingKeyring returns true if the store is using the system keyring.
-func (s *Store) UsingKeyring() bool { return s.inner.UsingKeyring() }
+func (s *Store) UsingKeyring() bool { return s.ensure().UsingKeyring() }

internal/auth/keyring_test.go

@@ -0,0 +1,46 @@
+package auth
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/basecamp/cli/credstore"
+)
+
+// swapNewCredStore replaces the credstore constructor seam for the test.
+func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) *credstore.Store) {
+	t.Helper()
+	orig := newCredStore
+	newCredStore = fn
+	t.Cleanup(func() { newCredStore = orig })
+}
+
+// TestNewStoreIsLazy proves the constructor never touches credstore:
+// credstore.NewStore probes the OS keyring with a write, and that probe can
+// block forever on a locked headless keychain. This test pins constructor
+// laziness only — the "credential-free commands never probe" conclusion
+// additionally rests on those command paths (setup agents, skill install,
+// version, help) not performing credential operations, which is audited at
+// the command layer, not asserted here.
+func TestNewStoreIsLazy(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring
+
+	calls := 0
+	swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store {
+		calls++
+		return credstore.NewStore(opts)
+	})
+
+	store := NewStore(t.TempDir())
+	require.NotNil(t, store)
+	assert.Equal(t, 0, calls, "NewStore must not construct the credstore")
+
+	_, err := store.Load("https://test.example.com")
+	require.Error(t, err, "no credentials saved")
+	assert.Equal(t, 1, calls, "first operation constructs the credstore")
+
+	store.UsingKeyring()
+	assert.Equal(t, 1, calls, "construction happens once")
+}

scripts/install.ps1

@@ -222,35 +222,45 @@ function Test-InteractiveSession {
 # binary supports; unset/auto/ambiguous installs the shared skill only. Each
 # native call is individually guarded so a nonzero exit never aborts the install.
 function Invoke-PostInstallSetup([string]$Binary) {
-  try { $help = & $Binary setup --help 2>$null } catch { $help = '' }
+  # None of these calls touch credentials, but release binaries up to v0.7.2
+  # probe the OS keyring on startup for every command, and a locked headless
+  # keychain blocks that probe forever. Set the escape hatch for the duration
+  # of setup only, restoring the caller's value (or absence) on the way out.
+  $savedNoKeyring = $env:BASECAMP_NO_KEYRING
+  $env:BASECAMP_NO_KEYRING = '1'
+  try {
+    try { $help = & $Binary setup --help 2>$null } catch { $help = '' }
 
-  if ($help -match '(?m)^\s+agents\s') {
-    try { & $Binary setup agents } catch { }
-    return
-  }
+    if ($help -match '(?m)^\s+agents\s') {
+      try { & $Binary setup agents } catch { }
+      return
+    }
 
-  $selector = $env:BASECAMP_SETUP_AGENT
-  if ($selector -in @('claude', 'codex')) {
-    # Capability-check first: an old `setup` parent accepts an unadvertised agent
-    # id as a stray arg and launches the INTERACTIVE wizard. Degrade to the skill.
-    if ($help -match "(?m)^\s+$selector\s") {
-      try { & $Binary setup $selector } catch { }
+    $selector = $env:BASECAMP_SETUP_AGENT
+    if ($selector -in @('claude', 'codex')) {
+      # Capability-check first: an old `setup` parent accepts an unadvertised agent
+      # id as a stray arg and launches the INTERACTIVE wizard. Degrade to the skill.
+      if ($help -match "(?m)^\s+$selector\s") {
+        try { & $Binary setup $selector } catch { }
+      } else {
+        try { & $Binary skill install } catch { }
+      }
+    } elseif ($selector -eq 'all') {
+      $ranAgent = $false
+      foreach ($agent in @('claude', 'codex')) {
+        if ($help -match "(?m)^\s+$agent\s") {
+          # Mark attempted (not succeeded) — matches install.sh's `ran_agent=1`,
+          # which is set regardless of the setup call's exit status.
+          $ranAgent = $true
+          try { & $Binary setup $agent } catch { }
+        }
+      }
+      if (-not $ranAgent) { try { & $Binary skill install } catch { } }
     } else {
       try { & $Binary skill install } catch { }
     }
-  } elseif ($selector -eq 'all') {
-    $ranAgent = $false
-    foreach ($agent in @('claude', 'codex')) {
-      if ($help -match "(?m)^\s+$agent\s") {
-        # Mark attempted (not succeeded) — matches install.sh's `ran_agent=1`,
-        # which is set regardless of the setup call's exit status.
-        $ranAgent = $true
-        try { & $Binary setup $agent } catch { }
-      }
-    }
-    if (-not $ranAgent) { try { & $Binary skill install } catch { } }
-  } else {
-    try { & $Binary skill install } catch { }
+  } finally {
+    $env:BASECAMP_NO_KEYRING = $savedNoKeyring
   }
 }