Automate the headless keyring-probe composition test - #596
Merged
Conversation
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.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
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.
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.
jeremy
marked this pull request as draft
August 2, 2026 04:14
jeremy
changed the title
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.
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.
jeremy
changed the title
jeremy
marked this pull request as ready for review
August 2, 2026 04:32
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 "<none>"`, 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 (`<path> -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.
jeremy
changed the title
![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: 2b9b343483
ℹ️ 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".
| # 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 |
There was a problem hiding this comment.
If the bounded probe regresses to kill the child without waiting for it, auth status can still return and the CLI then exits before this check runs; macOS reparents the zombie to launchd, which can reap it before pgrep executes. The check therefore reports no survivor even though the CLI itself failed to reap the child, so it does not provide the claimed end-to-end reaping assertion. Observe the child while the CLI parent remains alive or expose an explicit wait/reap result from the test harness.
Useful? React with 👍 / 👎.
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.
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.
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.
Closes the last gap in the #568 lineage: nothing ran the real binary, on real macOS, in a real headless session, against a probe that really blocks, and asserted it completed inside the bound.
Unit tests already 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 (#578), and headless implies a 10s bound. The gap was composition, and it was a manual VM ritual that had never been performed.The gate is deterministic: every run so far has landed at 10.2–10.3s against a 10s bound, well inside the 8–25s band.
Two findings shaped the design
1. A
macos-latestjob step is not headless.macos-26-arm64reportslaunchctl managername: Aqua, sosessionIsHeadless()is false there and the bounded path never engages. Measured alternatives:managernameAquasudo launchctl bsexec /Systemssh localhostBackgroundtty: not a ttyRemote Login is already On in the image. Running there is not a workaround — an ssh session with no TTY is the #568 scenario (a piped installer over ssh, unable to answer an unlock prompt).
2. On the tested
macos-26-arm64image a locked keychain fast-fails rather than blocking. With every precondition asserted correct,securityreturnederrSecInteractionNotAllowedin 0.196s and the cancellation path never ran. (Scoped to that image — a different macOS version, FileVault state, or securityd under load could still stall.)So rather than hunt for a real keychain that blocks, the probe is blocked deterministically.
credstore'ssecurityPathis a package-level var initialized to a constant, so the linker can repoint it:caffeinate -iruns until killed, matching howprobeBoundedinvokes it. No change tobasecamp/cliis needed — and deliberately no production runtime override, which would enlarge the executable-path trust boundary in shipped builds for no benefit. A link-time override touches only this binary, which is why it is namedbasecamp-hanging-probeand is not described as release-shaped: this is a composition gate on the source revision, not a test of the exact shipped artifact.The keychain is never touched, which is why the workflow is named for the headless probe rather than for a locked keychain. The headless detection stays real —
sessionIsHeadless()is what selects the bounded path; only the blocking child is synthetic.The floor is self-proving
securityPathis read only byprobeBounded, so a broken harness cannot masquerade as a passing test:caffeinate -i, killed at 10sprobeDirect→ go-keyring → realsecurity-Xinjection (symbol renamed)probeBounded→ realsecurityA direct assertion that the injection took is included as well, since
-Xagainst a renamed symbol is silently ignored by the linker and would otherwise surface only as an unexplained fast run.No probe process survives
A deadline that leaves the blocking child running still leaks a process per invocation, so a gating step asserts none outlives the CLI — not a diagnostic, and deliberately not inside the
if: always()block where|| echowould print a survivor and let the gate stay green. The match is anchored on the exact argv the probe spawns (<path> -i), verified in all four directions: absent, present, after cleanup, and against an unrelatedcaffeinate -d -t 5(no false positive).This proves no persistent child survived. It does not prove the CLI parent reaped its child — a zombie would be reparented to PID 1 on CLI exit and likely reaped by init before the check runs, so a regression dropping
cmd.Run()'s wait would still pass here. The authoritative parent-side proof stays where it can actually be made: credstore's darwin unit test pinning ESRCH. Making the stronger claim in composition would require holding the CLI parent alive while inspecting child state, or exposing a test-only wait result — not warranted for a release gate.Release gate
release.ymlgains aheadless-probe:job thatuses:this workflow, andreleasenowneeds: [test, security, headless-probe]. Invoked that way it runs against the exact tag SHA — a manual pre-run cannot gate a release, becausescripts/release.shpushes a release-prep commit to main before tagging.The calling job grants
contents: readexplicitly:release.ymlsetspermissions: {}and a called workflow can only maintain or reduce the caller's permissions, never elevate them.release.ymlis in this workflow's own path filter, so that edit retriggers the test it gates.Scope
No production Go changes. The #568 fix itself merged 2026-07-27 and is already in
v0.8.0-rc.1; this proves it rather than changing it.The gate is not active on
mainuntil this merges, and promoting stablev0.8.0remains a separate decision.Refs #568