From b881c25288ba94be255b8a0edc474e843ddfe4a8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 1 Aug 2026 21:06:13 -0700 Subject: [PATCH 1/8] Automate the locked-keychain acceptance test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #568 incident class — on headless macOS with a locked login keychain, constructing the credential store blocked forever in an uncancellable `security` child — has unit coverage for each piece: the bounded probe kills and reaps its child, the store is lazy until the first credential op, and headless sessions get a 10s bound. What nothing covered was the composition: the real binary, on real macOS, against a really blocking keychain, completing inside the bound. That was a manual VM ritual, and it had not been done. An earlier attempt with a disposable HOME failed deceptively — `security -i` returned in 3.21s with -60006, a fast clean failure in which the cancellation path never ran. It would have "passed" while proving nothing. A hosted macOS runner is disposable, which dissolves the objection that forced the VM: nobody's keychain is harmed by locking it. So make it a black-box CI test. The floor assertion is what makes this a test rather than a ritual. A run that finishes in ~3s means the runner fast-failed and the timeout path never executed, so the job fails rather than emitting a green check that proves nothing — the disposable-HOME trap, promoted to CI where it would be trusted forever. The ceiling catches the regression. Whether a hosted runner actually reproduces the blocking behavior is unproven, so this lands as an experiment: the PR trigger and its self-referencing path filter let the job prove itself here. It becomes a release gate only after a run lands in the 8-25s band — wiring an unproven check into `release.yml` would either block releases on a void gate or bless one. `workflow_call` is why this is a separate file rather than a job in test.yml: `release.yml` can then invoke it against the exact tag SHA. A manual pre-run cannot gate a release, because scripts/release.sh pushes a release-prep commit to main before tagging, so anything run beforehand covers the wrong commit. That wiring is deliberately not in this change. --- .github/workflows/keychain-acceptance.yml | 238 ++++++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 .github/workflows/keychain-acceptance.yml diff --git a/.github/workflows/keychain-acceptance.yml b/.github/workflows/keychain-acceptance.yml new file mode 100644 index 00000000..f2118fd0 --- /dev/null +++ b/.github/workflows/keychain-acceptance.yml @@ -0,0 +1,238 @@ +name: Keychain Acceptance + +# Black-box acceptance test for the #568 incident class: on headless macOS with +# a locked login keychain, constructing the credential store used to block +# forever in an uncancellable `security` child. Unit tests cover the pieces +# (bounded probe kills and reaps its child; the store is lazy; headless implies +# a 10s bound) — nothing else runs the real binary against a really blocking +# keychain and asserts the command finishes inside the bound. +# +# Locking a keychain is why this was a manual VM ritual. A hosted runner is +# disposable, so it can be locked without consequence. + +on: + workflow_call: + # The point of the design: release.yml invokes this against the exact tag + # SHA. workflow_dispatch cannot gate a release — a dispatch runs against + # the tip of the selected ref, not an arbitrary commit. + pull_request: + branches: [main] + # The whole surface the acceptance path traverses, not just the keyring: + # the store and its headless detection, the command that reaches it, the + # app/CLI lifecycle that builds the store, the entrypoint, the release + # build shape, the credstore pin, and both workflow files — self-proving, + # the same trick installer-bash32 uses at test.yml:249. + paths: + - 'internal/auth/**' + - 'internal/appctx/**' + - 'internal/cli/**' + - 'internal/commands/auth.go' + - 'cmd/basecamp/**' + - '.goreleaser.yaml' + - 'go.mod' + - 'go.sum' + - '.github/workflows/keychain-acceptance.yml' + - '.github/workflows/release.yml' + workflow_dispatch: + +permissions: {} + +jobs: + keychain: + name: Locked keychain (headless macOS) + runs-on: macos-latest + timeout-minutes: 15 + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: 'go.mod' + + - name: Assert the session is headless + run: | + # sessionIsHeadless() (internal/auth/keyring.go) bounds the probe only + # when no stream is a terminal AND launchctl reports no Aqua session. + # If a runner image ever gains one, the probe stops being bounded and + # this gate must go red rather than silently green. + manager=$(/bin/launchctl managername 2>&1 || true) + echo "launchctl managername: $manager" + if [ "$manager" = "Aqua" ]; then + echo "::error::Runner has an Aqua session — the headless probe bound never engages, so this test proves nothing." + exit 1 + fi + + - name: Assert the void-the-test variables are disarmed + run: | + # Either of these skips the code under test entirely, and would leave + # a green check that proved nothing: + # BASECAMP_NO_KEYRING non-empty -> credstore.NewStore returns a + # file store without probing (credstore/store.go:68). + # BASECAMP_TOKEN non-empty -> auth status returns before + # IsAuthenticated(), so the store is never constructed + # (internal/commands/auth.go:61). + # Both are checked for non-emptiness because that is exactly what the + # production code tests: os.Getenv(...) != "". + fail=0 + if [ -n "${BASECAMP_NO_KEYRING:-}" ]; then + echo "::error::BASECAMP_NO_KEYRING is set — the keyring probe would be skipped and this test would prove nothing." + fail=1 + fi + if [ -n "${BASECAMP_TOKEN:-}" ]; then + echo "::error::BASECAMP_TOKEN is set — auth status returns before the credential store is built." + fail=1 + fi + exit "$fail" + + - name: Build the release-shaped binary + run: | + # Deliberately not `make build`: that bakes -tags dev (Makefile:29-30). + # goreleaser ships -trimpath with no tags (.goreleaser.yaml:33-34), and + # the gate is about the artifact users actually install. + CGO_ENABLED=0 go build -trimpath -o ./bin/basecamp ./cmd/basecamp + ./bin/basecamp --version + + - name: Resolve the keychain the probe will hit + id: keychain + run: | + # The bounded probe mirrors go-keyring's darwin Set — `security -i` + # fed add-generic-password with no -k — so it targets the *default* + # keychain. Hosted images vary here, so resolve and assert rather + # than assume login.keychain-db. + raw=$(security default-keychain 2>&1 || true) + echo "security default-keychain: $raw" + path=$(printf '%s\n' "$raw" | sed -n 's/^ *"\(.*\)"$/\1/p') + if [ -z "$path" ]; then + echo "::error::Could not resolve a default keychain from: $raw" + exit 1 + fi + if [ ! -e "$path" ]; then + echo "::error::Default keychain does not exist on disk: $path" + exit 1 + fi + echo "path=$path" >> "$GITHUB_OUTPUT" + + - name: Lock the keychain and run a credential-touching command + id: probe + env: + KEYCHAIN: ${{ steps.keychain.outputs.path }} + run: | + security lock-keychain "$KEYCHAIN" + echo "--- show-keychain-info (after lock) ---" + security show-keychain-info "$KEYCHAIN" 2>&1 || true + + # All three streams redirected so sessionIsHeadless() holds and the + # 10s bound engages. `auth status` reaches IsAuthenticated() -> + # Store.ensure() -> credstore.NewStore -> the probe. + # + # Watchdog via perl: alarm() survives exec and SIGALRM's default + # disposition is restored across it, so the timer rides on the + # binary's own PID. `timeout` is homebrew/coreutils-only and may be + # absent; a shell background-kill would race PID recycling. + start=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') + status=0 + perl -e 'alarm(60); exec @ARGV or die "exec: $!"' \ + ./bin/basecamp auth status --json \ + > /tmp/keychain-stdout.txt 2> /tmp/keychain-stderr.txt < /dev/null \ + || status=$? + end=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') + elapsed=$(perl -e 'printf "%.3f\n", $ARGV[1] - $ARGV[0]' "$start" "$end") + + echo "elapsed=${elapsed}s exit=${status}" + { + echo "elapsed=$elapsed" + echo "status=$status" + } >> "$GITHUB_OUTPUT" + + - name: Unlock the keychain + if: always() + env: + KEYCHAIN: ${{ steps.keychain.outputs.path }} + run: | + # Diagnostics below read the keychain; leaving it locked would also + # break any later step on this runner. + if [ -n "${KEYCHAIN:-}" ]; then + security unlock-keychain -p "" "$KEYCHAIN" 2>&1 || true + fi + + - name: Assert the timeout path ran and the bound held + env: + ELAPSED: ${{ steps.probe.outputs.elapsed }} + STATUS: ${{ steps.probe.outputs.status }} + run: | + # The floor is what makes this a test rather than a ritual. An + # earlier manual attempt with a disposable HOME "passed" in 3.21s + # with -60006: a fast clean failure in which the cancellation path + # never ran. Without the floor that trap gets promoted to CI, where + # it would be trusted forever. + # + # >= 8s the probe really blocked and the bound cut it off + # ~3s fast clean failure — environment did not reproduce #568 + # <= 25s the bound held (10s probe + up to 5s cleanup + startup) + # 60s watchdog fired — #568 regression + echo "elapsed=${ELAPSED}s exit=${STATUS}" + + if [ "$STATUS" = "142" ]; then + echo "::error::Watchdog fired at 60s (SIGALRM) — the command never returned. This is the #568 hang." + exit 1 + fi + + under_floor=$(perl -e 'print(($ARGV[0] < 8.0) ? 1 : 0)' "$ELAPSED") + if [ "$under_floor" = "1" ]; then + echo "::error::Completed in ${ELAPSED}s, under the 8s floor — the locked keychain fast-failed instead of blocking, so the cancellation path never ran and this check proves nothing. Do not weaken the floor to make it green." + exit 1 + fi + + over_ceiling=$(perl -e 'print(($ARGV[0] > 25.0) ? 1 : 0)' "$ELAPSED") + if [ "$over_ceiling" = "1" ]; then + echo "::error::Took ${ELAPSED}s, past the 25s ceiling — the headless probe bound did not hold." + exit 1 + fi + + if [ "$STATUS" != "0" ]; then + echo "::error::auth status exited $STATUS — it did not complete through the file fallback." + exit 1 + fi + + - name: Assert the command completed through the fallback + run: | + # Exit 0 alone would not prove the command ran to completion. The + # envelope does. No credentials are seeded: once the keyring is + # unavailable, reporting not-authenticated *is* the success path. + cat /tmp/keychain-stdout.txt + if ! grep -Eq '"ok"[[:space:]]*:[[:space:]]*true' /tmp/keychain-stdout.txt; then + echo "::error::JSON envelope is not ok:true" + exit 1 + fi + if ! grep -Eq '"authenticated"[[:space:]]*:[[:space:]]*false' /tmp/keychain-stdout.txt; then + echo "::error::Expected authenticated:false in the envelope" + exit 1 + fi + + - name: Diagnostics + if: always() + env: + ELAPSED: ${{ steps.probe.outputs.elapsed }} + STATUS: ${{ steps.probe.outputs.status }} + KEYCHAIN: ${{ steps.keychain.outputs.path }} + run: | + echo "elapsed: ${ELAPSED:-}" + echo "exit code: ${STATUS:-}" + echo "default keychain: ${KEYCHAIN:-}" + echo "--- launchctl managername ---" + /bin/launchctl managername 2>&1 || true + echo "--- security default-keychain ---" + security default-keychain 2>&1 || true + echo "--- security show-keychain-info (now) ---" + if [ -n "${KEYCHAIN:-}" ]; then + security show-keychain-info "$KEYCHAIN" 2>&1 || true + fi + echo "--- stdout ---" + cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" + echo "--- stderr ---" + cat /tmp/keychain-stderr.txt 2>/dev/null || echo "" From a25df4772cc43a3db560c5dcc8701137dceecaf9 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 1 Aug 2026 21:08:35 -0700 Subject: [PATCH 2/8] TEMP: probe whether a non-Aqua session is reachable on macos-latest Phase 0's first run failed at the headless assertion: macos-latest reports `launchctl managername: Aqua`, so sessionIsHeadless() is false and the bounded-probe path never engages. The assertion worked; the premise that a hosted runner is headless did not. This diagnostic job determines whether any invocation context on the runner yields a non-Aqua session. Removed before merge either way. --- .github/workflows/keychain-acceptance.yml | 45 +++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/.github/workflows/keychain-acceptance.yml b/.github/workflows/keychain-acceptance.yml index f2118fd0..6d3e2f37 100644 --- a/.github/workflows/keychain-acceptance.yml +++ b/.github/workflows/keychain-acceptance.yml @@ -236,3 +236,48 @@ jobs: cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" echo "--- stderr ---" cat /tmp/keychain-stderr.txt 2>/dev/null || echo "" + + # TEMPORARY (Phase 0 diagnostics, removed before this PR merges). + # macos-latest reports `launchctl managername: Aqua`, so sessionIsHeadless() + # is false and the bounded-probe path never engages. This job determines + # whether a genuinely non-Aqua session is reachable on a hosted runner at + # all — the difference between "this approach needs one more step" and + # "black-box on hosted macOS is dead". + session-probe: + name: TEMP session-context probe + runs-on: macos-latest + timeout-minutes: 10 + permissions: + contents: read + steps: + - name: Probe launchctl session under several contexts + run: | + me=$(whoami) + echo "whoami: $me" + echo "direct: $(/bin/launchctl managername 2>&1)" + echo "passwordless sudo: $(sudo -n true 2>&1 && echo yes || echo no)" + echo "sudo direct: $(sudo /bin/launchctl managername 2>&1)" + echo "bsexec /: $(sudo launchctl bsexec / /bin/launchctl managername 2>&1 | head -1)" + echo "bsexec / as $me: $(sudo launchctl bsexec / sudo -u "$me" /bin/launchctl managername 2>&1 | head -1)" + echo "remote login: $(sudo systemsetup -getremotelogin 2>&1 | head -1)" + echo "sshd plist: $(ls /System/Library/LaunchDaemons/ssh.plist 2>&1)" + echo "ssh localhost: $(ssh -o BatchMode=yes -o StrictHostKeyChecking=no -o ConnectTimeout=5 localhost /bin/launchctl managername 2>&1 | head -1)" + + - name: Try enabling Remote Login and re-probe + run: | + # ssh localhost is the real #568 scenario (piped installer over ssh) + # and yields a StandardIO/Background session rather than Aqua. + sudo launchctl load -w /System/Library/LaunchDaemons/ssh.plist 2>&1 || \ + sudo systemsetup -setremotelogin on 2>&1 || echo "could not enable remote login" + sudo systemsetup -getremotelogin 2>&1 || true + + mkdir -p ~/.ssh + ssh-keygen -t ed25519 -N '' -f ~/.ssh/id_probe -q + cat ~/.ssh/id_probe.pub >> ~/.ssh/authorized_keys + chmod 700 ~/.ssh; chmod 600 ~/.ssh/authorized_keys + + echo "--- ssh localhost managername ---" + ssh -i ~/.ssh/id_probe -o BatchMode=yes -o StrictHostKeyChecking=no \ + -o UserKnownHostsFile=/dev/null -o ConnectTimeout=10 \ + localhost '/bin/launchctl managername; echo "tty:"; tty' 2>&1 || \ + echo "ssh to localhost failed" From 5020c56c56e258927c4f48ddec11589d39b6b0e1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 1 Aug 2026 21:11:59 -0700 Subject: [PATCH 3/8] Run the acceptance command over ssh localhost, not in the job step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 0's first run failed at the headless assertion, and it was right to. A GitHub macOS runner step reports `launchctl managername: Aqua` — the image has a GUI session — so sessionIsHeadless() is false there, ProbeTimeout is never set, and the bounded-probe path under test never executes. The premise that a hosted runner is headless was simply wrong; the assertion converted that into a red check instead of a green one over dead code. A diagnostic run measured the alternatives: direct (job step) Aqua sudo launchctl bsexec / System (root, wrong keychain domain) ssh localhost Background (tty: not a tty) Remote Login is already On in the image, so reaching a Background session costs only an authorized_keys entry. This is not a workaround for the runner. `ssh localhost` with no TTY *is* the #568 scenario — a piped installer over ssh with no way to answer an unlock prompt — so the test now reproduces the incident more faithfully than the original design did, rather than less. Consequent changes: the headless assertion now checks managername and tty in the ssh session; the disarm assertion checks the ssh session's environment, since a login profile could set either variable independently of the job; the default keychain is resolved and locked in that session, which is the securityd context the probe runs in; the watchdog runs remotely so it kills the binary rather than only the ssh client; and exit 255 counts as a watchdog fire, since ssh reports a signalled remote command that way. The floor, the ceiling, and the envelope assertion are unchanged. --- .github/workflows/keychain-acceptance.yml | 170 ++++++++++++---------- 1 file changed, 90 insertions(+), 80 deletions(-) diff --git a/.github/workflows/keychain-acceptance.yml b/.github/workflows/keychain-acceptance.yml index 6d3e2f37..75d4850e 100644 --- a/.github/workflows/keychain-acceptance.yml +++ b/.github/workflows/keychain-acceptance.yml @@ -9,6 +9,15 @@ name: Keychain Acceptance # # Locking a keychain is why this was a manual VM ritual. A hosted runner is # disposable, so it can be locked without consequence. +# +# The command runs over `ssh localhost`, not directly in the job step. A GitHub +# macOS runner step reports `launchctl managername: Aqua` — it has a GUI session +# — so sessionIsHeadless() is false there and the bounded-probe path never +# engages. Over ssh the session manager is Background with no TTY attached, +# which is not a workaround for the runner but the actual #568 scenario: a +# piped installer over ssh with no way to answer an unlock prompt. Remote Login +# is already enabled on the hosted image; only an authorized_keys entry is +# needed. on: workflow_call: @@ -54,16 +63,52 @@ jobs: with: go-version-file: 'go.mod' - - name: Assert the session is headless + - name: Build the release-shaped binary + run: | + # Deliberately not `make build`: that bakes -tags dev (Makefile:29-30). + # goreleaser ships -trimpath with no tags (.goreleaser.yaml:33-34), and + # the gate is about the artifact users actually install. + CGO_ENABLED=0 go build -trimpath -o ./bin/basecamp ./cmd/basecamp + ./bin/basecamp --version + + - name: Open a headless session to localhost + run: | + # Remote Login is already On in the hosted image, so this only needs + # an authorized_keys entry. The key is generated per run and never + # leaves the runner. + mkdir -p ~/.ssh + ssh-keygen -t ed25519 -N '' -f ~/.ssh/id_keychain_acceptance -q + cat ~/.ssh/id_keychain_acceptance.pub >> ~/.ssh/authorized_keys + chmod 700 ~/.ssh + chmod 600 ~/.ssh/authorized_keys + ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR -o ConnectTimeout=15 localhost true + + - name: Assert the ssh session is headless run: | # sessionIsHeadless() (internal/auth/keyring.go) bounds the probe only # when no stream is a terminal AND launchctl reports no Aqua session. - # If a runner image ever gains one, the probe stops being bounded and - # this gate must go red rather than silently green. - manager=$(/bin/launchctl managername 2>&1 || true) - echo "launchctl managername: $manager" + # Assert both in the session that will actually run the command — the + # job step itself is Aqua, which is precisely why the command does not + # run there. + manager=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR localhost /bin/launchctl managername) + echo "ssh session managername: $manager" if [ "$manager" = "Aqua" ]; then - echo "::error::Runner has an Aqua session — the headless probe bound never engages, so this test proves nothing." + echo "::error::ssh session reports an Aqua session — the headless probe bound never engages, so this test proves nothing." + exit 1 + fi + + # `tty` exits non-zero when stdin is not a terminal, which is the + # answer we want, so it must not trip `set -e`. + tty_state=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR localhost tty < /dev/null || true) + echo "ssh session tty: $tty_state" + if [ "$tty_state" != "not a tty" ]; then + echo "::error::ssh session has a TTY ($tty_state) — an interactive session keeps the unbounded probe by design." exit 1 fi @@ -78,24 +123,18 @@ jobs: # (internal/commands/auth.go:61). # Both are checked for non-emptiness because that is exactly what the # production code tests: os.Getenv(...) != "". - fail=0 - if [ -n "${BASECAMP_NO_KEYRING:-}" ]; then - echo "::error::BASECAMP_NO_KEYRING is set — the keyring probe would be skipped and this test would prove nothing." - fail=1 - fi - if [ -n "${BASECAMP_TOKEN:-}" ]; then - echo "::error::BASECAMP_TOKEN is set — auth status returns before the credential store is built." - fail=1 + # + # Checked in the ssh session, not the job: the command runs there, and + # a login shell profile could set either independently of the job env. + state=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR localhost \ + 'echo "no_keyring=[${BASECAMP_NO_KEYRING:-}] token=[${BASECAMP_TOKEN:-}]"') + echo "ssh session: $state" + if [ "$state" != 'no_keyring=[] token=[]' ]; then + echo "::error::BASECAMP_NO_KEYRING or BASECAMP_TOKEN is non-empty in the ssh session — the credential store would be skipped and this test would prove nothing." + exit 1 fi - exit "$fail" - - - name: Build the release-shaped binary - run: | - # Deliberately not `make build`: that bakes -tags dev (Makefile:29-30). - # goreleaser ships -trimpath with no tags (.goreleaser.yaml:33-34), and - # the gate is about the artifact users actually install. - CGO_ENABLED=0 go build -trimpath -o ./bin/basecamp ./cmd/basecamp - ./bin/basecamp --version - name: Resolve the keychain the probe will hit id: keychain @@ -103,8 +142,11 @@ jobs: # The bounded probe mirrors go-keyring's darwin Set — `security -i` # fed add-generic-password with no -k — so it targets the *default* # keychain. Hosted images vary here, so resolve and assert rather - # than assume login.keychain-db. - raw=$(security default-keychain 2>&1 || true) + # than assume login.keychain-db. Resolved in the ssh session, since + # that is the securityd context the probe runs in. + raw=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR localhost security default-keychain 2>&1 || true) echo "security default-keychain: $raw" path=$(printf '%s\n' "$raw" | sed -n 's/^ *"\(.*\)"$/\1/p') if [ -z "$path" ]; then @@ -122,22 +164,29 @@ jobs: env: KEYCHAIN: ${{ steps.keychain.outputs.path }} run: | - security lock-keychain "$KEYCHAIN" + SSH="ssh -i $HOME/.ssh/id_keychain_acceptance -o BatchMode=yes" + SSH="$SSH -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null" + SSH="$SSH -o LogLevel=ERROR localhost" + + $SSH security lock-keychain "$KEYCHAIN" echo "--- show-keychain-info (after lock) ---" - security show-keychain-info "$KEYCHAIN" 2>&1 || true + $SSH security show-keychain-info "$KEYCHAIN" 2>&1 || true - # All three streams redirected so sessionIsHeadless() holds and the - # 10s bound engages. `auth status` reaches IsAuthenticated() -> - # Store.ensure() -> credstore.NewStore -> the probe. + # `auth status` reaches IsAuthenticated() -> Store.ensure() -> + # credstore.NewStore -> the probe. Streams are redirected on both + # sides, and ssh without -t allocates no TTY, so sessionIsHeadless() + # holds and the 10s bound engages. # - # Watchdog via perl: alarm() survives exec and SIGALRM's default + # The watchdog runs on the remote side so it kills the binary itself + # rather than just the ssh client, which would leave the process + # behind. perl: alarm() survives exec and SIGALRM's default # disposition is restored across it, so the timer rides on the # binary's own PID. `timeout` is homebrew/coreutils-only and may be # absent; a shell background-kill would race PID recycling. start=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') status=0 - perl -e 'alarm(60); exec @ARGV or die "exec: $!"' \ - ./bin/basecamp auth status --json \ + $SSH "perl -e 'alarm(60); exec @ARGV or die \"exec: \$!\"' \ + '$GITHUB_WORKSPACE/bin/basecamp' auth status --json" \ > /tmp/keychain-stdout.txt 2> /tmp/keychain-stderr.txt < /dev/null \ || status=$? end=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') @@ -177,7 +226,9 @@ jobs: # 60s watchdog fired — #568 regression echo "elapsed=${ELAPSED}s exit=${STATUS}" - if [ "$STATUS" = "142" ]; then + # ssh reports a signalled remote command as exit 255, so accept + # either that or the direct 128+SIGALRM. + if [ "$STATUS" = "142" ] || [ "$STATUS" = "255" ]; then echo "::error::Watchdog fired at 60s (SIGALRM) — the command never returned. This is the #568 hang." exit 1 fi @@ -224,8 +275,12 @@ jobs: echo "elapsed: ${ELAPSED:-}" echo "exit code: ${STATUS:-}" echo "default keychain: ${KEYCHAIN:-}" - echo "--- launchctl managername ---" + echo "--- launchctl managername (job step) ---" /bin/launchctl managername 2>&1 || true + echo "--- launchctl managername (ssh session) ---" + ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ + -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -o LogLevel=ERROR localhost /bin/launchctl managername 2>&1 || true echo "--- security default-keychain ---" security default-keychain 2>&1 || true echo "--- security show-keychain-info (now) ---" @@ -236,48 +291,3 @@ jobs: cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" echo "--- stderr ---" cat /tmp/keychain-stderr.txt 2>/dev/null || echo "" - - # TEMPORARY (Phase 0 diagnostics, removed before this PR merges). - # macos-latest reports `launchctl managername: Aqua`, so sessionIsHeadless() - # is false and the bounded-probe path never engages. This job determines - # whether a genuinely non-Aqua session is reachable on a hosted runner at - # all — the difference between "this approach needs one more step" and - # "black-box on hosted macOS is dead". - session-probe: - name: TEMP session-context probe - runs-on: macos-latest - timeout-minutes: 10 - permissions: - contents: read - steps: - - name: Probe launchctl session under several contexts - run: | - me=$(whoami) - echo "whoami: $me" - echo "direct: $(/bin/launchctl managername 2>&1)" - echo "passwordless sudo: $(sudo -n true 2>&1 && echo yes || echo no)" - echo "sudo direct: $(sudo /bin/launchctl managername 2>&1)" - echo "bsexec /: $(sudo launchctl bsexec / /bin/launchctl managername 2>&1 | head -1)" - echo "bsexec / as $me: $(sudo launchctl bsexec / sudo -u "$me" /bin/launchctl managername 2>&1 | head -1)" - echo "remote login: $(sudo systemsetup -getremotelogin 2>&1 | head -1)" - echo "sshd plist: $(ls /System/Library/LaunchDaemons/ssh.plist 2>&1)" - echo "ssh localhost: $(ssh -o BatchMode=yes -o StrictHostKeyChecking=no -o ConnectTimeout=5 localhost /bin/launchctl managername 2>&1 | head -1)" - - - name: Try enabling Remote Login and re-probe - run: | - # ssh localhost is the real #568 scenario (piped installer over ssh) - # and yields a StandardIO/Background session rather than Aqua. - sudo launchctl load -w /System/Library/LaunchDaemons/ssh.plist 2>&1 || \ - sudo systemsetup -setremotelogin on 2>&1 || echo "could not enable remote login" - sudo systemsetup -getremotelogin 2>&1 || true - - mkdir -p ~/.ssh - ssh-keygen -t ed25519 -N '' -f ~/.ssh/id_probe -q - cat ~/.ssh/id_probe.pub >> ~/.ssh/authorized_keys - chmod 700 ~/.ssh; chmod 600 ~/.ssh/authorized_keys - - echo "--- ssh localhost managername ---" - ssh -i ~/.ssh/id_probe -o BatchMode=yes -o StrictHostKeyChecking=no \ - -o UserKnownHostsFile=/dev/null -o ConnectTimeout=10 \ - localhost '/bin/launchctl managername; echo "tty:"; tty' 2>&1 || \ - echo "ssh to localhost failed" From f0d2d6b380e4526451075a6aac689451d7b96981 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 1 Aug 2026 21:26:47 -0700 Subject: [PATCH 4/8] Block the probe by linker injection instead of a locked keychain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 0 established that a locked keychain cannot drive this test on a hosted runner: macos-26-arm64 returns errSecInteractionNotAllowed in under 200ms, so the 10s cancellation path never ran. The floor caught it. Rather than hunt for a real keychain that blocks, substitute a command that reliably does. credstore's securityPath is a package-level var initialized to a constant, so the linker can repoint it: -ldflags '-X=github.com/basecamp/cli/credstore.securityPath=/usr/bin/caffeinate' `caffeinate -i` runs until killed, matching how probeBounded invokes securityPath. Verified against the pinned credstore: the string is embedded in the injected build and absent from a control build. This needs no change to basecamp/cli. A production runtime override would have enlarged the executable-path trust boundary in shipped builds for no benefit; a link-time override touches only this binary, which is why it is named basecamp-hanging-probe and is no longer described as release-shaped. It is a composition gate on the source revision, not a test of the exact shipped artifact. The headless detection stays real — the command still runs over `ssh localhost` (Background, no TTY), and sessionIsHeadless() is what selects the bounded path. Only the blocking child is synthetic. Because securityPath is read solely by probeBounded, the floor is now self-proving across both seams: if headless detection breaks the store takes the unbounded probeDirect path through go-keyring, and if the injection breaks probeBounded execs the real `security` — either way the run returns in ~0.2s, lands under the floor, and goes red. A broken harness cannot masquerade as a passing test. Since the keychain is never touched, resolution, locking, unlocking and the keychain diagnostics are all gone. Added in their place: a direct assertion that the injection took, because `-X` against a renamed symbol is silently ignored by the linker and would otherwise surface only as an unexplained fast run; and a stray-child check, which observes the reaping property end-to-end. The floor, the ceiling, the watchdog and the envelope assertion are unchanged. --- .github/workflows/keychain-acceptance.yml | 192 +++++++++++----------- 1 file changed, 92 insertions(+), 100 deletions(-) diff --git a/.github/workflows/keychain-acceptance.yml b/.github/workflows/keychain-acceptance.yml index 75d4850e..71e74988 100644 --- a/.github/workflows/keychain-acceptance.yml +++ b/.github/workflows/keychain-acceptance.yml @@ -1,43 +1,53 @@ name: Keychain Acceptance -# Black-box acceptance test for the #568 incident class: on headless macOS with -# a locked login keychain, constructing the credential store used to block -# forever in an uncancellable `security` child. Unit tests cover the pieces -# (bounded probe kills and reaps its child; the store is lazy; headless implies -# a 10s bound) — nothing else runs the real binary against a really blocking -# keychain and asserts the command finishes inside the bound. +# Composition gate for the #568 incident class: on headless macOS, a keyring +# probe that blocks must be cut off at the deadline so the command completes +# through the file fallback instead of hanging forever. # -# Locking a keychain is why this was a manual VM ritual. A hosted runner is -# disposable, so it can be locked without consequence. +# Unit tests cover each piece — the bounded probe kills and reaps its child +# (credstore probe_darwin_test.go:41), the store is lazy until the first +# credential op, and headless implies a 10s bound. What none of them cover is +# the composition: the real binary, on real macOS, in a real headless session, +# against a probe that really blocks. # -# The command runs over `ssh localhost`, not directly in the job step. A GitHub -# macOS runner step reports `launchctl managername: Aqua` — it has a GUI session -# — so sessionIsHeadless() is false there and the bounded-probe path never -# engages. Over ssh the session manager is Background with no TTY attached, -# which is not a workaround for the runner but the actual #568 scenario: a -# piped installer over ssh with no way to answer an unlock prompt. Remote Login -# is already enabled on the hosted image; only an authorized_keys entry is -# needed. +# Two environmental facts shape how that is reached. +# +# 1. A macos-latest job step is NOT headless. macos-26-arm64 reports +# `launchctl managername: Aqua`, so sessionIsHeadless() is false there and +# the bounded path never engages. Over `ssh localhost` the session manager +# is Background with no TTY, which is not a workaround but the actual #568 +# scenario: a piped installer over ssh that cannot answer an unlock prompt. +# Remote Login is already On in the image; only an authorized_keys entry is +# needed. +# +# 2. A locked keychain on this image does not block. Measured 2026-08-02: +# `security` returns errSecInteractionNotAllowed in under 200ms, so the +# cancellation path never ran and the test proved nothing. Rather than hunt +# for a real keychain that blocks, substitute a command that reliably does. +# +# credstore's securityPath is a package-level var initialized to a constant, so +# the linker can repoint it at `/usr/bin/caffeinate`, which hangs until killed. +# No production change and no runtime override — the executable-path trust +# boundary is unchanged in shipped builds. The keychain is never touched. on: workflow_call: - # The point of the design: release.yml invokes this against the exact tag - # SHA. workflow_dispatch cannot gate a release — a dispatch runs against - # the tip of the selected ref, not an arbitrary commit. + # The point of the design: release.yml can invoke this against the exact + # tag SHA. workflow_dispatch cannot gate a release — a dispatch runs + # against the tip of the selected ref, not an arbitrary commit. pull_request: branches: [main] - # The whole surface the acceptance path traverses, not just the keyring: - # the store and its headless detection, the command that reaches it, the - # app/CLI lifecycle that builds the store, the entrypoint, the release - # build shape, the credstore pin, and both workflow files — self-proving, - # the same trick installer-bash32 uses at test.yml:249. + # The whole surface the acceptance path traverses: the store and its + # headless detection, the command that reaches it, the app/CLI lifecycle + # that builds the store, the entrypoint, the credstore pin (which owns the + # securityPath symbol this job injects into), and both workflow files — + # self-proving, the same trick installer-bash32 uses at test.yml:249. paths: - 'internal/auth/**' - 'internal/appctx/**' - 'internal/cli/**' - 'internal/commands/auth.go' - 'cmd/basecamp/**' - - '.goreleaser.yaml' - 'go.mod' - 'go.sum' - '.github/workflows/keychain-acceptance.yml' @@ -46,9 +56,16 @@ on: permissions: {} +env: + # Overriding this symbol is what makes the probe block deterministically. + # `caffeinate -i` runs until killed, matching how probeBounded invokes + # securityPath (` -i`, fed a command on stdin it never reads). + SECURITY_PATH_SYMBOL: github.com/basecamp/cli/credstore.securityPath + HANGING_COMMAND: /usr/bin/caffeinate + jobs: keychain: - name: Locked keychain (headless macOS) + name: Bounded probe (headless macOS) runs-on: macos-latest timeout-minutes: 15 permissions: @@ -63,13 +80,30 @@ jobs: with: go-version-file: 'go.mod' - - name: Build the release-shaped binary + - name: Build the test-instrumented binary + run: | + # Deliberately NOT called release-shaped: the linker override makes + # this a composition gate on the source revision, not a test of the + # exact shipped artifact. Everything else about the build matches + # goreleaser (-trimpath, no dev tag), so the code path is the + # shipped one. + CGO_ENABLED=0 go build -trimpath \ + -ldflags "-X=${SECURITY_PATH_SYMBOL}=${HANGING_COMMAND}" \ + -o ./bin/basecamp-hanging-probe ./cmd/basecamp + ./bin/basecamp-hanging-probe --version + + - name: Assert the linker injection took run: | - # Deliberately not `make build`: that bakes -tags dev (Makefile:29-30). - # goreleaser ships -trimpath with no tags (.goreleaser.yaml:33-34), and - # the gate is about the artifact users actually install. - CGO_ENABLED=0 go build -trimpath -o ./bin/basecamp ./cmd/basecamp - ./bin/basecamp --version + # The timing floor already catches a failed injection — the real + # `security` fast-fails well under 8s — but `-X` against a renamed or + # removed symbol is silently ignored by the linker, so assert it + # directly to make that failure diagnosable at a glance rather than + # arriving as an unexplained fast run. + if ! LC_ALL=C grep -qa "$HANGING_COMMAND" ./bin/basecamp-hanging-probe; then + echo "::error::${HANGING_COMMAND} is not embedded in the binary — the -X override against ${SECURITY_PATH_SYMBOL} did not apply. The symbol was probably renamed or removed by a credstore bump." + exit 1 + fi + echo "Injected ${SECURITY_PATH_SYMBOL} -> ${HANGING_COMMAND}" - name: Open a headless session to localhost run: | @@ -87,17 +121,16 @@ jobs: - name: Assert the ssh session is headless run: | - # sessionIsHeadless() (internal/auth/keyring.go) bounds the probe only - # when no stream is a terminal AND launchctl reports no Aqua session. - # Assert both in the session that will actually run the command — the - # job step itself is Aqua, which is precisely why the command does not - # run there. + # sessionIsHeadless() (internal/auth/keyring.go) sets the 10s bound + # only when no stream is a terminal AND launchctl reports no Aqua + # session. This is the real detection under test — not stubbed — so + # assert both in the session that will actually run the command. manager=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ -o LogLevel=ERROR localhost /bin/launchctl managername) echo "ssh session managername: $manager" if [ "$manager" = "Aqua" ]; then - echo "::error::ssh session reports an Aqua session — the headless probe bound never engages, so this test proves nothing." + echo "::error::ssh session reports an Aqua session — sessionIsHeadless() would be false, the probe would run unbounded through go-keyring, and this test would prove nothing." exit 1 fi @@ -136,46 +169,18 @@ jobs: exit 1 fi - - name: Resolve the keychain the probe will hit - id: keychain - run: | - # The bounded probe mirrors go-keyring's darwin Set — `security -i` - # fed add-generic-password with no -k — so it targets the *default* - # keychain. Hosted images vary here, so resolve and assert rather - # than assume login.keychain-db. Resolved in the ssh session, since - # that is the securityd context the probe runs in. - raw=$(ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ - -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ - -o LogLevel=ERROR localhost security default-keychain 2>&1 || true) - echo "security default-keychain: $raw" - path=$(printf '%s\n' "$raw" | sed -n 's/^ *"\(.*\)"$/\1/p') - if [ -z "$path" ]; then - echo "::error::Could not resolve a default keychain from: $raw" - exit 1 - fi - if [ ! -e "$path" ]; then - echo "::error::Default keychain does not exist on disk: $path" - exit 1 - fi - echo "path=$path" >> "$GITHUB_OUTPUT" - - - name: Lock the keychain and run a credential-touching command + - name: Run a credential-touching command against a blocking probe id: probe - env: - KEYCHAIN: ${{ steps.keychain.outputs.path }} run: | SSH="ssh -i $HOME/.ssh/id_keychain_acceptance -o BatchMode=yes" SSH="$SSH -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null" SSH="$SSH -o LogLevel=ERROR localhost" - $SSH security lock-keychain "$KEYCHAIN" - echo "--- show-keychain-info (after lock) ---" - $SSH security show-keychain-info "$KEYCHAIN" 2>&1 || true - # `auth status` reaches IsAuthenticated() -> Store.ensure() -> - # credstore.NewStore -> the probe. Streams are redirected on both - # sides, and ssh without -t allocates no TTY, so sessionIsHeadless() - # holds and the 10s bound engages. + # credstore.NewStore -> probe -> probeBounded, which execs the + # injected hanging command. ssh without -t allocates no TTY and both + # streams are redirected, so sessionIsHeadless() holds and the 10s + # bound engages. # # The watchdog runs on the remote side so it kills the binary itself # rather than just the ssh client, which would leave the process @@ -186,7 +191,7 @@ jobs: start=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') status=0 $SSH "perl -e 'alarm(60); exec @ARGV or die \"exec: \$!\"' \ - '$GITHUB_WORKSPACE/bin/basecamp' auth status --json" \ + '$GITHUB_WORKSPACE/bin/basecamp-hanging-probe' auth status --json" \ > /tmp/keychain-stdout.txt 2> /tmp/keychain-stderr.txt < /dev/null \ || status=$? end=$(perl -MTime::HiRes=time -e 'printf "%.3f\n", time') @@ -198,44 +203,35 @@ jobs: echo "status=$status" } >> "$GITHUB_OUTPUT" - - name: Unlock the keychain - if: always() - env: - KEYCHAIN: ${{ steps.keychain.outputs.path }} - run: | - # Diagnostics below read the keychain; leaving it locked would also - # break any later step on this runner. - if [ -n "${KEYCHAIN:-}" ]; then - security unlock-keychain -p "" "$KEYCHAIN" 2>&1 || true - fi - - name: Assert the timeout path ran and the bound held env: ELAPSED: ${{ steps.probe.outputs.elapsed }} STATUS: ${{ steps.probe.outputs.status }} run: | - # The floor is what makes this a test rather than a ritual. An - # earlier manual attempt with a disposable HOME "passed" in 3.21s - # with -60006: a fast clean failure in which the cancellation path - # never ran. Without the floor that trap gets promoted to CI, where - # it would be trusted forever. + # The floor is what makes this a test rather than a ritual, and it is + # self-proving across both seams. If headless detection breaks, the + # store takes the unbounded probeDirect path through go-keyring and + # the real `security` returns in ~0.2s. If the -X injection breaks, + # probeBounded execs the real `security`, which also returns in + # ~0.2s. Either way the run lands under the floor and goes red — a + # broken harness cannot masquerade as a passing test. # # >= 8s the probe really blocked and the bound cut it off - # ~3s fast clean failure — environment did not reproduce #568 - # <= 25s the bound held (10s probe + up to 5s cleanup + startup) + # ~0.2s harness broken (see above) — proves nothing + # <= 25s the bound held (10s probe + startup + ssh round-trip) # 60s watchdog fired — #568 regression echo "elapsed=${ELAPSED}s exit=${STATUS}" # ssh reports a signalled remote command as exit 255, so accept # either that or the direct 128+SIGALRM. if [ "$STATUS" = "142" ] || [ "$STATUS" = "255" ]; then - echo "::error::Watchdog fired at 60s (SIGALRM) — the command never returned. This is the #568 hang." + echo "::error::Watchdog fired at 60s (SIGALRM) — the command never returned. The probe was not cut off at its deadline. This is the #568 hang." exit 1 fi under_floor=$(perl -e 'print(($ARGV[0] < 8.0) ? 1 : 0)' "$ELAPSED") if [ "$under_floor" = "1" ]; then - echo "::error::Completed in ${ELAPSED}s, under the 8s floor — the locked keychain fast-failed instead of blocking, so the cancellation path never ran and this check proves nothing. Do not weaken the floor to make it green." + echo "::error::Completed in ${ELAPSED}s, under the 8s floor — the probe never blocked, so the cancellation path never ran and this check proves nothing. Check the headless assertion and the linker injection. Do not weaken the floor to make it green." exit 1 fi @@ -270,23 +266,19 @@ jobs: env: ELAPSED: ${{ steps.probe.outputs.elapsed }} STATUS: ${{ steps.probe.outputs.status }} - KEYCHAIN: ${{ steps.keychain.outputs.path }} run: | echo "elapsed: ${ELAPSED:-}" echo "exit code: ${STATUS:-}" - echo "default keychain: ${KEYCHAIN:-}" echo "--- launchctl managername (job step) ---" /bin/launchctl managername 2>&1 || true echo "--- launchctl managername (ssh session) ---" ssh -i ~/.ssh/id_keychain_acceptance -o BatchMode=yes \ -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ -o LogLevel=ERROR localhost /bin/launchctl managername 2>&1 || true - echo "--- security default-keychain ---" - security default-keychain 2>&1 || true - echo "--- security show-keychain-info (now) ---" - if [ -n "${KEYCHAIN:-}" ]; then - security show-keychain-info "$KEYCHAIN" 2>&1 || true - fi + echo "--- injected securityPath ---" + LC_ALL=C grep -oa "$HANGING_COMMAND" ./bin/basecamp-hanging-probe | head -1 || echo "" + echo "--- stray hanging children (should be none) ---" + pgrep -fl "$HANGING_COMMAND" || echo "" echo "--- stdout ---" cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" echo "--- stderr ---" From 4e7a0f7e757d32e1e013710e3b3bb04cd03b12b8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 1 Aug 2026 21:30:47 -0700 Subject: [PATCH 5/8] Make the acceptance test a release gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow has now passed three consecutive runs at 10.294s, 10.202s and 10.197s — inside the 8-25s band with about 0.1s of variance — so it is deterministic enough to block publication. It is invoked as a called workflow rather than run beforehand because scripts/release.sh commits and pushes a release-prep commit to main before tagging, so a manual pre-run would cover the wrong SHA. As a `uses:` job it runs against the exact tag SHA. The calling job grants contents: read explicitly. release.yml sets permissions: {} at workflow level and a called workflow can only maintain or reduce the caller's token permissions, so without the grant actions/checkout inside the reusable workflow would have no repository access — a gate that fails for a reason unrelated to what it tests. release.yml is in the acceptance workflow's own path filter, so this change retriggers the test that it gates. --- .github/workflows/release.yml | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 81e89c9b..dcdc28fb 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -20,6 +20,21 @@ jobs: security-events: write pull-requests: read + # Invoked here rather than merely run beforehand: scripts/release.sh commits + # and pushes a release-prep commit to main *before* tagging, so anything run + # ahead of time covers the wrong SHA. As a called workflow this runs against + # the exact tag SHA. + # + # contents: read is required and cannot be inherited — this workflow sets + # permissions: {} above, and a called workflow can only maintain or reduce + # the caller's token permissions, never elevate them. Without it, + # actions/checkout inside the reusable workflow has no repository access. + keychain: + name: Locked-keychain acceptance + uses: ./.github/workflows/keychain-acceptance.yml + permissions: + contents: read + test: name: Test before release runs-on: ubuntu-latest @@ -103,7 +118,7 @@ jobs: release: name: Release - needs: [test, security] + needs: [test, security, keychain] runs-on: ubuntu-latest timeout-minutes: 45 environment: release From 2b9b3434838b8545886bdef5c99cc4c69c864eba Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 2 Aug 2026 08:34:12 -0700 Subject: [PATCH 6/8] Make the reaping check binding, and stop promising a keychain test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems, both about a reader trusting something the job does not actually deliver. The stray-child check was observed, not enforced. It sat in the `if: always()` diagnostics step as `pgrep -fl ... || echo ""`, so a surviving child would print and the step would continue — and with this wired into release.yml, a future reaping regression would scroll past a green release gate. It is now its own step, ahead of the envelope assertion, and a match exits non-zero. The match is anchored on the exact argv the probe spawns (` -i`) rather than a bare substring, in both directions: an unrelated caffeinate elsewhere on the runner cannot fail the gate, and a real survivor cannot hide behind a loose match. Verified locally across all four cases — absent, present, after cleanup, and against an unrelated `caffeinate -d -t 5`. There is no race to sleep around: CommandContext kills and waits synchronously inside cmd.Run(), so the child is reaped before the binary exits. The names promised a locked-keychain test that no longer happens — the keychain is never touched now that the probe blocks by linker injection. That matters most in the release UI, which is where an operator lands when this gate fails. Renamed the workflow to "Headless Keyring Probe", the file to headless-probe-composition.yml, and the release job to headless-probe / "Headless keyring probe". "Keyring" is kept so the subject stays findable; "locked-keychain" is dropped because it is no longer true. --- ...nce.yml => headless-probe-composition.yml} | 35 ++++++++++++++++--- .github/workflows/release.yml | 8 ++--- 2 files changed, 34 insertions(+), 9 deletions(-) rename .github/workflows/{keychain-acceptance.yml => headless-probe-composition.yml} (89%) diff --git a/.github/workflows/keychain-acceptance.yml b/.github/workflows/headless-probe-composition.yml similarity index 89% rename from .github/workflows/keychain-acceptance.yml rename to .github/workflows/headless-probe-composition.yml index 71e74988..eb9b2c8c 100644 --- a/.github/workflows/keychain-acceptance.yml +++ b/.github/workflows/headless-probe-composition.yml @@ -1,4 +1,4 @@ -name: Keychain Acceptance +name: Headless Keyring Probe # Composition gate for the #568 incident class: on headless macOS, a keyring # probe that blocks must be cut off at the deadline so the command completes @@ -50,7 +50,7 @@ on: - 'cmd/basecamp/**' - 'go.mod' - 'go.sum' - - '.github/workflows/keychain-acceptance.yml' + - '.github/workflows/headless-probe-composition.yml' - '.github/workflows/release.yml' workflow_dispatch: @@ -65,7 +65,7 @@ env: jobs: keychain: - name: Bounded probe (headless macOS) + name: Bounded probe under a blocking child runs-on: macos-latest timeout-minutes: 15 permissions: @@ -246,6 +246,31 @@ jobs: exit 1 fi + - name: Assert the hung child was reaped + run: | + # The whole point of the bounded probe is that the child is killed + # AND reaped — a deadline that abandons an orphan still leaks a + # process per invocation. credstore proves the ESRCH property in + # isolation (probe_darwin_test.go:41); this observes it in + # composition, through the real binary. + # + # This is a gate, not a diagnostic. It deliberately does not live in + # the `if: always()` diagnostics step below, where `|| echo` would + # print a survivor and let the release gate stay green. + # + # Matched on the exact argv the probe spawns (` -i`, anchored) + # rather than a bare substring, so an unrelated caffeinate elsewhere + # on the runner cannot fail this, and a real survivor cannot hide + # behind a loose match. CommandContext kills and waits synchronously + # inside cmd.Run(), so by the time the binary has exited the child is + # already reaped — there is no race to sleep around. + if survivors=$(pgrep -fl "^${HANGING_COMMAND} -i$"); then + echo "::error::The blocking probe child survived the deadline — it was killed but not reaped, or not killed at all. This is the #568 process-leak class." + printf '%s\n' "$survivors" + exit 1 + fi + echo "No surviving ${HANGING_COMMAND} children — the probe child was killed and reaped." + - name: Assert the command completed through the fallback run: | # Exit 0 alone would not prove the command ran to completion. The @@ -277,8 +302,8 @@ jobs: -o LogLevel=ERROR localhost /bin/launchctl managername 2>&1 || true echo "--- injected securityPath ---" LC_ALL=C grep -oa "$HANGING_COMMAND" ./bin/basecamp-hanging-probe | head -1 || echo "" - echo "--- stray hanging children (should be none) ---" - pgrep -fl "$HANGING_COMMAND" || echo "" + echo "--- stray hanging children ---" + pgrep -fl "^${HANGING_COMMAND} -i$" || echo "" echo "--- stdout ---" cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" echo "--- stderr ---" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index dcdc28fb..e869f304 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -29,9 +29,9 @@ jobs: # permissions: {} above, and a called workflow can only maintain or reduce # the caller's token permissions, never elevate them. Without it, # actions/checkout inside the reusable workflow has no repository access. - keychain: - name: Locked-keychain acceptance - uses: ./.github/workflows/keychain-acceptance.yml + headless-probe: + name: Headless keyring probe + uses: ./.github/workflows/headless-probe-composition.yml permissions: contents: read @@ -118,7 +118,7 @@ jobs: release: name: Release - needs: [test, security, keychain] + needs: [test, security, headless-probe] runs-on: ubuntu-latest timeout-minutes: 45 environment: release From d3e9f8e4e8a0adc2abbd55d27c9d28e997d8b01f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 2 Aug 2026 09:26:10 -0700 Subject: [PATCH 7/8] Label the diagnostic pgrep as reporting-only The binding reaping assertion and this diagnostic print look alike at a glance, and the diagnostic has now been flagged twice as an unenforced gate. Both times the gate was already in place a few steps above. Say so at the call site, and explain why the duplicate is deliberate: diagnostics run if: always(), so this still reports child state when the job failed before reaching the assertion. --- .github/workflows/headless-probe-composition.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/headless-probe-composition.yml b/.github/workflows/headless-probe-composition.yml index eb9b2c8c..3c032c11 100644 --- a/.github/workflows/headless-probe-composition.yml +++ b/.github/workflows/headless-probe-composition.yml @@ -302,7 +302,13 @@ jobs: -o LogLevel=ERROR localhost /bin/launchctl managername 2>&1 || true echo "--- injected securityPath ---" LC_ALL=C grep -oa "$HANGING_COMMAND" ./bin/basecamp-hanging-probe | head -1 || echo "" - echo "--- stray hanging children ---" + # Reporting only — NOT the gate. The binding assertion is the + # "Assert the hung child was reaped" step above, which exits + # non-zero on a match. This duplicate exists because diagnostics run + # `if: always()`, so it still reports child state when the job failed + # earlier and never reached that assertion. Do not add `exit 1` here + # expecting it to close a hole; the hole is already closed. + echo "--- stray hanging children (reporting only; gated above) ---" pgrep -fl "^${HANGING_COMMAND} -i$" || echo "" echo "--- stdout ---" cat /tmp/keychain-stdout.txt 2>/dev/null || echo "" From c9be58f28aef08d77933bcfcf74afb0a0efca486 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 2 Aug 2026 12:33:08 -0700 Subject: [PATCH 8/8] Narrow the survivor claim: absence is not proof of reaping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step asserted the child was "killed and reaped". It cannot show that. `pgrep` finding nothing proves no process survived the CLI — not that the CLI parent waited on it. Had the parent killed without waiting, the child would be a zombie, reparented to PID 1 when the CLI exits, and likely reaped by init before the pgrep runs. The gate would pass with the parent-side wait missing, which is precisely the regression the wording implied it caught. Current credstore is safe — cmd.Run() waits — but this check would not notice if that stopped being true, so it must not be the thing anyone trusts for it. The authoritative parent-side proof stays where it can actually be made: credstore's darwin unit test pinning ESRCH. Narrowed the step name, its comments, and its output to the claim it actually supports, and recorded why the stronger claim is out of reach here: a composition test could only assert reaping by holding the CLI parent alive while inspecting child state, or by exposing a test-only wait result. Neither is warranted for a release gate. --- .../workflows/headless-probe-composition.yml | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/.github/workflows/headless-probe-composition.yml b/.github/workflows/headless-probe-composition.yml index 3c032c11..77ba5e46 100644 --- a/.github/workflows/headless-probe-composition.yml +++ b/.github/workflows/headless-probe-composition.yml @@ -246,13 +246,21 @@ jobs: exit 1 fi - - name: Assert the hung child was reaped + - name: Assert no blocking probe process survived run: | - # The whole point of the bounded probe is that the child is killed - # AND reaped — a deadline that abandons an orphan still leaks a - # process per invocation. credstore proves the ESRCH property in - # isolation (probe_darwin_test.go:41); this observes it in - # composition, through the real binary. + # A deadline that leaves the blocking child running still leaks a + # process per invocation, so assert none outlives the CLI. + # + # This proves NO PERSISTENT CHILD SURVIVED. It does NOT prove the CLI + # parent reaped its own child, and must not be read that way: had the + # parent killed without waiting, the child would be a zombie, + # reparented to PID 1 when the CLI exits, and likely reaped by init + # before this pgrep runs — so this check would still pass. The + # authoritative parent-side reaping proof is credstore's darwin unit + # test, which pins ESRCH (probe_darwin_test.go:41). A composition + # test could only make the stronger claim by holding the CLI parent + # alive while inspecting child state, or by exposing a test-only wait + # result; neither is warranted for a release gate. # # This is a gate, not a diagnostic. It deliberately does not live in # the `if: always()` diagnostics step below, where `|| echo` would @@ -261,15 +269,15 @@ jobs: # Matched on the exact argv the probe spawns (` -i`, anchored) # rather than a bare substring, so an unrelated caffeinate elsewhere # on the runner cannot fail this, and a real survivor cannot hide - # behind a loose match. CommandContext kills and waits synchronously - # inside cmd.Run(), so by the time the binary has exited the child is - # already reaped — there is no race to sleep around. + # behind a loose match. CommandContext kills synchronously inside + # cmd.Run() before the binary exits, so there is no race to sleep + # around. if survivors=$(pgrep -fl "^${HANGING_COMMAND} -i$"); then - echo "::error::The blocking probe child survived the deadline — it was killed but not reaped, or not killed at all. This is the #568 process-leak class." + echo "::error::A blocking probe process outlived the CLI — the child was not killed at the deadline. This is the #568 process-leak class." printf '%s\n' "$survivors" exit 1 fi - echo "No surviving ${HANGING_COMMAND} children — the probe child was killed and reaped." + echo "No ${HANGING_COMMAND} process survived the CLI. (Parent-side reaping is pinned separately by credstore's darwin unit test.)" - name: Assert the command completed through the fallback run: | @@ -303,7 +311,7 @@ jobs: echo "--- injected securityPath ---" LC_ALL=C grep -oa "$HANGING_COMMAND" ./bin/basecamp-hanging-probe | head -1 || echo "" # Reporting only — NOT the gate. The binding assertion is the - # "Assert the hung child was reaped" step above, which exits + # "Assert no blocking probe process survived" step above, which exits # non-zero on a match. This duplicate exists because diagnostics run # `if: always()`, so it still reports child state when the job failed # earlier and never reached that assertion. Do not add `exit 1` here