Bound the keyring probe on headless sessions - #581
Conversation
Bumps github.com/basecamp/cli to the post-#56 credstore (v0.2.2-0.20260728023309-04e401b12c6c), which adds StoreOptions.ProbeTimeout: a bounded availability probe that kills the hung `security` child on darwin and falls back to file storage as if the probe had failed. The CLI wires it interactivity-aware: when stderr is not a terminal (CI, piped installers, ssh without a TTY) the probe is bounded at ten seconds — a headless session can never answer a keychain unlock prompt, so hanging forever in an uncancellable child was the only alternative (the #568 incident class, previously mitigated only by lazy construction and the BASECAMP_NO_KEYRING escape hatch). Interactive sessions keep the unbounded probe: a locked keychain there raises an unlock prompt, and cutting it off mid-answer would silently degrade the user to plaintext file storage. ForceFile is deliberately not wired: credstore's internal timeout-to-file fallback covers the CLI's need, and the env-var path already provides forced file mode.
github-actions
Bot
added
tests
There was a problem hiding this comment.
Pull request overview
This PR prevents credential-touching commands from hanging indefinitely on locked macOS keychains in headless sessions by bounding the keyring availability probe and relying on credstore’s timeout-to-file fallback, while preserving the existing unbounded behavior for interactive sessions.
Changes:
- Detect “headless” via whether stderr is a TTY and set
credstore.StoreOptions.ProbeTimeoutto10sonly in that case. - Keep the interactive path unbounded to avoid cutting off legitimate unlock prompts.
- Bump
github.com/basecamp/clito a pseudo-version that includesStoreOptions.ProbeTimeout, and add a unit test pinning both branches.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/auth/keyring.go | Adds stderr-based headless detection and applies a 10s keyring probe timeout only when headless. |
| internal/auth/keyring_test.go | Adds coverage to assert probe timeout is set only for headless sessions via a test seam. |
| go.mod | Bumps github.com/basecamp/cli to a pseudo-version that provides ProbeTimeout support. |
| go.sum | Updates checksums for the bumped github.com/basecamp/cli version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: edb710faf5
ℹ️ 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".
Stderr alone misclassified routine interactive use: `2>auth.log` from a terminal would have bounded the probe and, on a >10s unlock answer, silently degraded to plaintext file storage with the warning hidden in the redirected log. Headless now requires stdin, stdout, and stderr to all be non-terminals — any attached stream means a human can answer the unlock prompt on the controlling terminal or the GUI. Review feedback on #581.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b72bfc9e
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
All three stdio streams being non-terminals is not proof of headlessness: a macOS app or IDE task runner launches the CLI fully detached while the user can still answer the keychain unlock dialog on the WindowServer. Headless now additionally requires no GUI session — on darwin via `launchctl managername` != Aqua (bounded, cannot touch the keychain, failures count as no GUI so true headless keeps the bounded probe); elsewhere via DISPLAY/WAYLAND_DISPLAY, with Windows always false since Credential Manager never prompts. Review feedback on #581.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The !darwin implementation's claim that Windows is always non-GUI was false — that build returned true on any platform with DISPLAY set. A dedicated session_windows.go hard-codes false with the rationale, the unix implementation drops the stale claim, and the env test gains the Wayland-only branch. Review feedback on #581.
resourceOrigin now lowercases the host (and the localhost carve-out checks the lowered host) and strips explicit default ports, normalizing leading zeros — the SDK binds the RFC 9728 protected-resource identifier to this string code-point exact, so an equivalent spelling (HTTPS://3.BasecampAPI.com, https://3.basecampapi.com:443) previously mismatched the advertised identifier and silently soft-fell back to Launchpad. IPv6 brackets are restored around the lowered hostname. Also restacked onto main 31c4936, preserving #581's bounded headless keyring probe alongside the credential resource field, and tidied the go.sum lines the auto-merge duplicated.
resourceOrigin now lowercases the host (and the localhost carve-out checks the lowered host) and strips explicit default ports, normalizing leading zeros — the SDK binds the RFC 9728 protected-resource identifier to this string code-point exact, so an equivalent spelling (HTTPS://3.BasecampAPI.com, https://3.basecampapi.com:443) previously mismatched the advertised identifier and silently soft-fell back to Launchpad. IPv6 brackets are restored around the lowered hostname. Also restacked onto main 31c4936, preserving #581's bounded headless keyring probe alongside the credential resource field, and tidied the go.sum lines the auto-merge duplicated.
Completes the #568 keychain-hang lineage: after lazy store construction (#578) fixed credential-free commands, credential-touching commands on a locked headless keychain still blocked unbounded. basecamp/cli#56 added the upstream prerequisite (
StoreOptions.ProbeTimeout— a bounded probe whose darwin implementation actually kills the hungsecuritychild and falls back to file storage). This PR bumps to that credstore (pseudo-version of the merge commit; no release tag exists yet) and wires it.Interactivity-aware bound
BASECAMP_NO_KEYRING=1escape hatch. Now: bounded probe → file fallback with the existing plaintext warning.| jq) while prompts still reach the terminal.ForceFileis deliberately not wired — credstore's internal timeout-to-file fallback covers the CLI's need, andBASECAMP_NO_KEYRINGalready provides forced file mode.Tests
TestEnsureBoundsProbeOnlyWhenHeadlesspins both branches through the existingnewCredStoreseam plus a newstderrIsTerminalseam (precedent:stdoutIsTerminalin update_notice.go). Existing laziness test unchanged.Verification
bin/cigreen;go test -race ./internal/auth/...green.auth statusprobes within the bound against a healthy keychain and loads keyring credentials (expired: false; the file fallback holds a stale token, distinguishing the paths).-race -count=2proof at merge head.Refs #568
Summary by cubic
Bound the keyring probe to 10s only in fully headless sessions (no TTYs and no GUI) and fall back to file storage to prevent hangs on locked keychains. Interactive or GUI-launched sessions keep the unbounded probe. Addresses #568.
Bug Fixes
launchctl managername!= Aqua; Linux/*BSD: noDISPLAY/WAYLAND_DISPLAY; Windows: GUI check is always false since Credential Manager never prompts); setProbeTimeoutto 10s socredstorefalls back to file storage.Dependencies
github.com/basecamp/clitov0.2.2-0.20260728023309-04e401b12c6cto usecredstore.StoreOptions.ProbeTimeout.Written for commit 4eb0779. Summary will update on new commits.