Give each process its own keyring probe item, and say when the keyring fell back to file by jeremy · Pull Request #70 · basecamp/cli

jeremy · 2026-08-28T20:30:54Z

The failure

Every invocation probes keyring availability by writing and deleting one fixed-name entry, credstore.probe.<service> / __probe__. Twenty concurrent invocations churn one keychain item, and on darwin security add-generic-password -U is find-then-create inside the security tool: a peer's delete or add landing in that window fails the add with errSecDuplicateItem (rc=45) on a perfectly healthy keychain. The loser silently switched to the plaintext file fallback, so a machine whose credentials live in the keychain answered credentials not found for profile:<name> — and where a months-stale credentials.json survived, it answered with expired tokens instead.

Observed today from a connector that fans out basecamp calls: 19 of 20 parallel basecamp auth status -j returned the stale file, basecamp me --profile clawdito said "Not authenticated" 19/20, and one Basecamp event was dropped as "not corroborated". Serial runs: 20/20 fine.

#69 taught the probe to tolerate the lost race. That closed the bounded darwin path but left the unbounded go-keyring path (interactive sessions) losing 149 of 200 probes at 20-way concurrency — the retry loses too and the peer has already deleted its entry — and it was armor around the actual defect: a shared item.

The fix

Per-probe entry. The account is now __probe__.<pid>.<n> — pid plus an in-process sequence number — derived once in probe() so both the bounded (security -i) and unbounded (go-keyring) paths use it. The pid separates processes; the sequence separates probes within one, including a timed-out probe's abandoned worker (non-darwin) from any later probe, so no two probes ever touch the same keychain item. The credstore: don't mistake a lost probe-write race for an unavailable keyring #69 recovery (duplicate-item tolerance, Set retry, Get read-back) is retired as unreachable.

The account is deterministic rather than random so a leaked entry (timed-out probe, cleanup cut short) still self-heals: the next process to reuse the pid overwrites and removes the same-numbered leftover — entry 1 in practice, since a process probes once — instead of the entry becoming unfindable (go-keyring has no list API).

(The first commit used __probe__.<pid> plus an in-process mutex; review caught that a timed-out non-darwin probe keeps running on that account after the mutex is released. The sequence number closes that and obviates the mutex — second commit.)
A fallback is never silent about its cause.
- Store.ProbeError() returns why the probe failed (nil when the keyring is in use or file storage was requested via ForceFile/DisableEnvVar).
- FallbackWarning() names the reason: system keyring unavailable (<reason>), credentials stored in plaintext at <path>.
- A miss on the file fallback reads credentials not found for profile:x: system keyring unavailable (<reason>), fell back to <path> — basecamp-cli wraps that verbatim in Not authenticated for %s: %v, so the reason reaches users with no change there.
- The darwin probe names the keychain failure on both paths. The bounded probe (security -i, headless sessions) captures security's own diagnostic line. The unbounded probe (go-keyring, every interactive session) only ever sees a bare exit status N — go-keyring's darwin Set returns cmd.Wait() and discards the diagnostic — so a darwin-only table maps the exit status (the low byte of the SecBase.h OSStatus: 36, 37, 45, 50, 51, 52, 53, 128) to the text security error <OSStatus> prints, in the same shape: User interaction is not allowed. (exit status 36). Unknown exit statuses and non-exit errors pass through unchanged. A timed-out probe says keyring probe timed out after 10s.

Measurements (real keychain on this machine, 20 parallel × 10 rounds)

	bounded (`security -i`)	unbounded (go-keyring)
raw `security add -U` + delete, one shared account	190/200 failed, all rc=45	—
raw `security`, per-pid accounts	0/200	—
credstore at `04e401b` (what basecamp-cli 0.9.1 pins)	190/200 fell back	190/200
credstore at `main` (#69)	0/200	149/200
credstore at this branch	0/200, 0/500 at 50-parallel	0/200, 0/500

Harness: a 15-line program calling credstore.NewStore{ServiceName: "repro"} and printing UsingKeyring(), run N-way in parallel from a shell loop.

Tests

TestProbeContractIsPerProbeAndReserved, TestProbeUsesIsolatedPerProbeEntry (darwin), TestProbeDirectUsesPerProbeEntry — both paths derive __probe__.<pid>.<n> under the reserved service, and consecutive probes get distinct accounts.
TestProbeFailureIsReportedOnLoad, TestProbeTimeoutFallsBackToFile, TestProbeBoundedFailureCarriesDiagnostic, TestProbeDirectNamesSecurityExitStatus (darwin), TestProbeDirectPassesUnknownExitStatusThrough (darwin), TestProbeTimeoutIsNamed — the probe error is kept and named in the warning and in the fallback Load error, on the bounded and the unbounded path.
TestHealthyProbeUsesKeyring — a successful probe keeps the keyring (UsingKeyring() true, ProbeError() nil, no warning) and zero-value options reach the probe with a zero timeout, the documented contract for tests that mock the keyring. A mutant that always falls back (useKeyring: false) fails this and nothing else.
TestProbeFailureStillReadsFallbackFile, TestRequestedFileStorageReportsNoProbeFailure — file-only hosts keep working; requested file storage is not a "fallback" and gets no warning.

Checked against the pre-change code (with compile-only shims for ProbeError and probeKey() returning the old behavior): the contract, per-probe-entry, timeout-naming, diagnostic, and Load/warning tests all fail there; TestProbeDirectNamesSecurityExitStatus fails with probeDirect returning the error verbatim ("exit status 36"). make check, golangci-lint run, and go test -race ./credstore/ pass.

Declined

Keep credstore: don't mistake a lost probe-write race for an unavailable keyring #69's duplicate-item / retry / read-back recovery as belt-and-braces. With a per-probe account the shared item no longer exists, so those branches cannot fire; keeping them keeps tests for unreachable paths and the "peer entry present proves availability" reasoning that only holds under churn. A fixed-key binary older than this change running alongside a new one uses a different account and does not collide either.
Tolerate rc=45 without the per-process account. It is the wrong layer: it accepts an error the probe can't otherwise distinguish, it doesn't cover the other interleaving (a peer's delete between -U's duplicate detection and its find, errSecItemNotFound), and go-keyring's unbounded path can't see the code at all — hence 149/200.
pid + nanotime / random suffix. Unique per probe, but a leaked entry would never be reclaimed; the pid-and-sequence name keeps the leak self-healing on pid reuse.
Serialize probes in-process with a mutex (first commit). It cannot cover a timed-out non-darwin probe whose worker keeps running after the mutex is released, and holding the mutex until that worker finishes would make the next NewStore wait on the very hang the timeout exists to escape.
Sweep all leftovers under the probe service at cleanup (delete-generic-password -s <svc> without -a, looped). It would delete a concurrent peer's in-flight entry — benign, since its add already succeeded, but it re-couples processes that this change decouples, for a leak that is already harmless (a "probe" string in a reserved namespace) and self-healing.
Refuse the file fallback when a migration marker exists or the file is absent, returning the keyring error directly. ~/.config/basecamp/.migrated marks the bcq→basecamp keyring-service migration, not file→keyring, so it is not evidence the file is stale; MigrateToKeyring isn't called by basecamp-cli at all. And credstore cannot tell "keyring never worked here" (Linux without a secret service, whose credentials legitimately live only in the file) from "the probe failed this once". When the file is absent, the Load error already carries the keyring reason and the path, which is the same information. The right layer for "don't trust a stale file" is naming the failure, which this does.
Capture security's diagnostic on the unbounded path by driving security -i there too. That is the bounded path. The unbounded probe goes through go-keyring on purpose so keyring.MockInit is honored (documented on ProbeTimeout); go-keyring's darwin Set sets no stdout/stderr, so the diagnostic line is gone by the time the error reaches us and only the exit status survives.
Translate the exit status at runtime with security error. Only the low byte of the OSStatus survives the exit, so the full code can't be recovered for lookup; a fixed table of the eight codes an add can produce on an unavailable keychain is exact, and needs no second exec on an already-failing keychain.
Print the warning from credstore on read. A library shouldn't write to stderr; FallbackWarning() now carries the reason and the docstring asks callers to surface it on reads too. basecamp-cli's Store.Load will warn once per process (separate PR there).

Follow-ups

basecamp-cli: bump github.com/basecamp/cli past this merge (go get github.com/basecamp/cli@main && go mod tidy) and warn on fallback reads as well as writes.

…g failure on fallback Every invocation probed keyring availability by writing and deleting one fixed-name entry, credstore.probe.<service> / __probe__. Twenty concurrent invocations therefore churned one keychain item, and on darwin `security add-generic-password -U` is find-then-create inside the security tool: a peer's delete or add landing in that window fails the add with errSecDuplicateItem (rc=45) on a perfectly healthy keychain. The loser silently switched to the plaintext file fallback, so a machine whose credentials live in the keychain answered "credentials not found for profile:<name>" — and where a months-stale credentials.json survived, it answered with expired tokens instead. Observed today from a connector that fans out CLI calls: 19 of 20 parallel `basecamp auth status` reported stale or missing credentials; serial runs were 20/20 fine. #69 taught the probe to tolerate the lost race (accept the duplicate-item answer; retry a failed Set and read back a peer's entry). That closed the bounded darwin path but left the unbounded go-keyring path losing 149 of 200 probes at 20-way concurrency — the retry loses too and the peer has already deleted its entry — and it was armor around the actual defect: a shared item. Now the probe account is __probe__.<pid>, and NewStore serializes probes within a process, so no two in-flight probes ever touch the same entry. The duplicate-item and retry/read-back recovery is retired as unreachable. The account stays deterministic per pid rather than random so a leaked entry still self-heals — the next process reusing that pid overwrites and removes it — instead of becoming unfindable, go-keyring having no list API. Measured against a real keychain, 20 parallel probes x 10 rounds: raw `security` on one shared account 190/200 failed (all rc=45), on per-pid accounts 0/200; credstore at the pre-#69 pin 190/200 both paths, at #69 bounded 0/200 but unbounded 149/200, with this change 0/200 and 0/500 on both paths. A probe failure is also no longer silent on read. The store keeps the probe error (ProbeError), the fallback warning names it, and a miss on the file fallback says "system keyring unavailable (<reason>), fell back to <path>" rather than a bare "credentials not found". The darwin probe folds the security tool's diagnostic into its error and a timed-out probe says so, so the reason is readable rather than "exit status 36".

jeremy · 2026-08-28T20:31:02Z

@codex review

Copilot

Pull request overview

Adds per-process keyring probes and exposes file-fallback causes to callers.

Changes:

Uses PID-scoped probe entries with in-process serialization.
Reports probe failures through warnings and load errors.
Expands regression coverage for concurrency, timeouts, and diagnostics.

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 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`credstore/store.go`	Stores and surfaces probe failures; serializes probes.
`credstore/store_test.go`	Tests fallback reporting and serialization.
`credstore/probe.go`	Introduces PID-scoped probe accounts.
`credstore/probe_test.go`	Tests probe naming and cleanup.
`credstore/probe_other.go`	Documents non-darwin timeout behavior.
`credstore/probe_darwin.go`	Includes keychain diagnostics in errors.
`credstore/probe_darwin_test.go`	Tests isolation, timeouts, and diagnostics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 969ac6243a

ℹ️ 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".

A per-process account plus an in-process mutex left one gap: on non-darwin, a bounded probe that times out abandons its worker goroutine mid-Set, and releasing the mutex then let a later NewStore in the same process probe under the same pid account while that worker was still running against it. Holding the mutex until the worker finished would have made the next NewStore wait on the very hang the timeout exists to escape. An in-process sequence number in the account — __probe__.<pid>.<n> — gives every probe its own entry, which makes the mutex unnecessary and removes it. Leaks still self-heal on pid reuse: the next process with that pid overwrites and removes the same-numbered leftover, and in practice a process probes once, so that is entry 1. Real keychain, 20 parallel x 10 rounds: still 0/200 on both paths.

jeremy · 2026-08-28T20:40:29Z

@codex review

chatgpt-codex-connector · 2026-08-28T20:43:05Z

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 1b3e5b5aa4

ℹ️ 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".

Only the bounded probe folded security's diagnostic into its error. The unbounded probe goes through go-keyring, whose darwin Set returns cmd.Wait()'s bare "exit status N" and discards the diagnostic line — and the unbounded probe is the interactive path, every session with a terminal. So a headless fallback read "User interaction is not allowed. (exit status 36)" while an interactive user with a locked keychain got "system keyring unavailable (exit status 36)": a number, not a reason. security exits with the low byte of the SecBase.h OSStatus, so the codes are stable. A darwin-only table names the ones an add can produce on an unavailable keychain (36, 37, 45, 50, 51, 52, 53, 128) with the text `security error <OSStatus>` prints, in the same "<reason> (exit status N)" shape as the bounded path. Unknown exit statuses and non-exit errors pass through unchanged; other platforms' backends run in-process and already name their failures.

The rewrite of NewStore replaced TestZeroValueOptionsProbeUnbounded, the only test of a successful probe, with the fallback-side tests, so nothing asserted that a healthy probe keeps the keyring: a mutant that always fell back to the plaintext file (useKeyring: false) passed the suite. Restore the happy path — UsingKeyring true, ProbeError nil, no warning — and the zero-timeout contract that tests mocking the keyring rely on.

jeremy · 2026-08-28T20:59:02Z

Since 1b3e5b5 (two commits, 945a6dc is head):

38e7b44 — the unbounded darwin probe now names the keychain failure too. go-keyring's darwin Set returns cmd.Wait()'s bare exit status N and discards security's diagnostic, so the interactive path (every session with a terminal) reported system keyring unavailable (exit status 36) where the bounded path said User interaction is not allowed. A darwin-only table maps the exit status — the low byte of the SecBase.h OSStatus, so stable — to security error's text in the same <reason> (exit status N) shape; unknown statuses and non-exit errors pass through. TestProbeDirectNamesSecurityExitStatus fails against probeDirect returning the error verbatim.
945a6dc — restored the healthy-probe test the earlier rewrite dropped: TestHealthyProbeUsesKeyring pins UsingKeyring() true, ProbeError() nil, empty warning, and zero timeout for zero-value options. A useKeyring: false mutant fails it and nothing else.
PR body: Declined bullet 1 now says per-probe (it still described the first commit's mutex); the diagnostic claim is scoped to both paths with how each gets its reason; two more declined alternatives (drive security -i on the unbounded path; translate the code at runtime with security error).
Resolved the two store.go mutex threads, addressed by 1b3e5b5.

make check, go test -race ./..., golangci-lint run, and GOOS=linux/GOOS=windows go vet ./credstore/ pass.

@codex review

chatgpt-codex-connector · 2026-08-28T21:01:42Z

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 945a6dc838

ℹ️ 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".

…y on login (#664) * auth: warn about the keyring fallback on the first read, not only on login The credential store printed its "system keyring unavailable, credentials stored in plaintext" warning only on Save. Every other command merely reads, so a process whose keyring probe failed served whatever an earlier fallback had left in credentials.json — on one machine today, tokens that expired months ago and profiles that no longer existed — with no word about why. The failing probe was invisible until the next login. Store.Load now runs the same once-per-process warning as Save, so the first read after a fallen-back probe says so on stderr, and once credstore is bumped past basecamp/cli#70 the warning and the Load error also name the probe failure itself. Hosts that mean to use file storage set BASECAMP_NO_KEYRING, which skips the probe and never warns. The wrapper's inner store is now the credStore interface rather than the concrete *credstore.Store, so the test can stand in a fallen-back store without failing a real keyring probe. * auth: reword the credStore seam comment * auth: keep a BASECAMP_TOKEN session off the store in SetUserEmail `basecamp people me` stores the fetched email through SetUserEmail, the one read path with no BASECAMP_TOKEN guard: IsAuthenticated and AuthorizationEndpoint short-circuit on the env token, then SetUserEmail loads the stored credentials anyway. That was wrong twice over. The email names the env token's user, not whoever the stored credentials belong to, so writing it there mislabeled them. And the load ran the keyring probe — now that a fallback read warns, a CI host with a locked keychain and only BASECAMP_TOKEN warned about plaintext credentials it neither stored nor read. BASECAMP_TOKEN wins, matching AccessToken() and AccountID(): SetUserEmail returns without touching the store.

Pulls in basecamp/cli#70: the keyring availability probe now writes a per-probe keychain item (service credstore.probe.<name>, account __probe__.<pid>.<n>), so concurrent CLI processes no longer share one probe entry and race each other's read-back. With the previous pin, 20 parallel invocations against the real macOS keychain returned 19 file-store fallbacks reading stale or missing credentials; with this pin all 20 read the keyring. FallbackWarning() now carries the probe failure reason, so the fallback warning added in #664 names its cause. vendorHash recomputed and the Nix build verified via make update-nix-hash.

Copilot AI balanced review requested due to automatic review settings August 28, 2026 20:30

github-actions Bot added the tests label Aug 28, 2026

Copilot started reviewing on behalf of jeremy August 28, 2026 20:31 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread credstore/store.go Outdated

chatgpt-codex-connector Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread credstore/store.go Outdated

jeremy mentioned this pull request Aug 28, 2026

Warn about the keyring fallback on the first credential read, not only on login basecamp/basecamp-cli#664

Merged

jeremy added 2 commits August 28, 2026 13:56

jeremy merged commit 9adb604 into main Aug 28, 2026
18 checks passed

jeremy deleted the keyring-probe-per-process branch August 28, 2026 23:02

jeremy mentioned this pull request Aug 28, 2026

Bump github.com/basecamp/cli for the per-probe keyring item basecamp/basecamp-cli#666

Merged

Navigation Menu

Uh oh!

Give each process its own keyring probe item, and say when the keyring fell back to file - #70

Give each process its own keyring probe item, and say when the keyring fell back to file#70
jeremy merged 4 commits into
mainfrom
keyring-probe-per-process

jeremy commented Aug 28, 2026 •

edited

Loading

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jeremy commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The failure

The fix

Measurements (real keychain on this machine, 20 parallel × 10 rounds)

Tests

Declined

Follow-ups

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

jeremy commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jeremy commented Aug 28, 2026 •

edited

Loading