fix(oauth): store keyring tokens as one entry per provider

[euxaristia]

Summary

PR #574 moved the macOS keyring write path to security -i, which is
necessary to keep the secret out of the process list and out of a
getpass(3) /dev/tty prompt. But security -i's command parser caps a
single write at 4095 bytes, and the oauth store combines every
provider and MCP token into one JSON blob under a single keyring
entry. That blob has no size bound of its own: three or more
logged-in providers routinely exceeds the cap, so Set() starts failing
for every provider, not just the one that pushed it over the line.

This splits keyring storage into one entry per token key (account =
key), plus a small index entry listing which keys currently exist,
since KeyringClient has no list operation. Each write is now bounded
to a single token's size, comfortably under the 4095-byte cap
regardless of how many providers are logged in.

Installs still on the old combined-entry format keep reading
correctly through a legacy fallback, and get migrated to per-key
entries (with the legacy entry removed) the next time anything is
saved.

Test plan

• go build ./...
• make lint (fmt-check + vet)
• go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... -race
• New test simulates 5 logged-in providers with realistic JWT-sized
  tokens and asserts no single keyring entry exceeds a 3000-byte
  margin under the line cap
• New test covers migration from the legacy combined-entry format
• Verified against the real macOS security CLI in a throwaway
  test keychain that the underlying security -i write/read
  mechanism this builds on works correctly (quoting, round-trip,
  length guard)

Summary by CodeRabbit

• Improvements
  • OAuth tokens are stored in the OS keyring as per-token entries with an indexed layout to improve multi-provider reliability and mixed-version migration.
  • Added safer, cross-process read/write coordination for keyring operations (including lock lease refreshing) and more resilient load/status behavior.
  • Token refresh now preserves existing scopes when the server omits scope in the response.
• Bug Fixes
  • Missing indexed entries no longer block other tokens from loading; corrupted/oversized index data is rejected safely.
  • Logout surfaces legacy cleanup failures to prevent token resurrection.
• Tests
  • Expanded migration, interruption (“no invisible credentials”), size-limit, corruption/DoS, and locking/timing coverage.

[coderabbitai]

Walkthrough

OAuth keyring storage now uses individually encoded token entries coordinated by a bounded chunked index. Reads support legacy recovery and serialized multi-entry access, while writes reconcile migrations, interruptions, deletions, and mixed-version updates. Refresh also preserves existing scopes when responses omit them.

Changes

OAuth keyring storage

Layer / File(s)	Summary
Keyring format and read recovery internal/oauth/store.go, internal/oauth/store_keyring_test.go	The backend reconstructs tokens from per-key entries, validates chunked or legacy indexes, tolerates missing entries, and recovers legacy data.
Indexed writes and migration reconciliation internal/oauth/store.go, internal/oauth/store_keyring_test.go	Writes publish union indexes before mutations, reconcile legacy and indexed tokens, clean up removed entries and legacy data, and enforce index and entry limits.
Read and write locking internal/oauth/store.go, internal/oauth/lock.go, internal/oauth/store_keyring_test.go	Keyring reads and writes use leased, identity-scoped locks with wall-clock deadlines; file reads remain lock-free.
Keyring behavior validation internal/oauth/store_keyring_test.go	Tests cover encoded storage, migration, interruption safety, mixed-version updates, corruption handling, index bounds, deletion, and lock behavior.

OAuth refresh scopes

Layer / File(s)	Summary
Refresh scope preservation internal/oauth/flow.go, internal/oauth/flow_test.go	Refresh retains current token scopes when the token response omits a scope field, using configured scopes only when current scopes are empty.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Store
  participant keyringBlob
  participant keyIndex
  participant tokenEntries
  participant legacyEntry
  Store->>keyringBlob: Load or Status under read lock
  keyringBlob->>keyIndex: Read and validate index
  keyIndex-->>keyringBlob: Return token keys
  keyringBlob->>tokenEntries: Read encoded token entries
  tokenEntries-->>keyringBlob: Return available tokens
  Store->>keyringBlob: Save updated store under write lock
  keyringBlob->>legacyEntry: Reconcile legacy tokens
  keyringBlob->>keyIndex: Publish union key set
  keyringBlob->>tokenEntries: Write or delete token entries
  keyringBlob->>legacyEntry: Delete legacy whole-blob entry
  keyringBlob->>keyIndex: Publish exact key set

Loading

Possibly related PRs

• Gitlawb/zero#708: Both changes modify OAuth refresh flow plumbing in internal/oauth/flow.go.

Suggested reviewers: gnanam1990, vasanthdev2004, jatmn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main OAuth keyring change: switching token storage to one entry per provider.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/oauth/store_keyring_test.go (1)

131-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider testing the index/entry desync recovery path.

Migration and deletion-reconciliation coverage look solid. One gap: read()'s continue when a key is listed in the index but its entry is missing (store.go Lines 483-488) — the mechanism the design relies on for surviving interrupted writes — isn't exercised anywhere. A small test seeding an index that references a key with no corresponding entry (mimicking a killed process mid-write) would lock in that behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/oauth/store_keyring_test.go` around lines 131 - 172, Add a focused
test for the keyring store’s index/entry desynchronization recovery in the read
path, such as the relevant Store read method. Seed the fake keyring with an
index referencing one missing entry and at least one valid entry, then verify
reading skips the missing key without returning an error and still returns the
valid token. Model the setup after TestStoreKeyringMigratesLegacyCombinedEntry
and use the existing fake keyring helpers and keyring symbols.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/oauth/store.go`:
- Around line 447-510: Update Store.Load’s keyring path to execute
keyringBlob.read() through blob.withLock, using the same cross-process locking
established for Save/Delete. Ensure the lock covers the complete index-and-entry
read so concurrent writes cannot produce a stale or incomplete result; preserve
existing read errors and returned data.

---

Nitpick comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 131-172: Add a focused test for the keyring store’s index/entry
desynchronization recovery in the read path, such as the relevant Store read
method. Seed the fake keyring with an index referencing one missing entry and at
least one valid entry, then verify reading skips the missing key without
returning an error and still returns the valid token. Model the setup after
TestStoreKeyringMigratesLegacyCombinedEntry and use the existing fake keyring
helpers and keyring symbols.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 23533b19-6702-40c4-8c89-873eaeb55792

📥 Commits

Reviewing files that changed from the base of the PR and between 80c39aa and 8eac9aa.

📒 Files selected for processing (2)

• internal/oauth/store.go
• internal/oauth/store_keyring_test.go

[euxaristia]

Addressed CodeRabbit's review.

• Store.Load now runs its read through blob.withLock, matching Save/Delete. The keyring backend's read is several separate Get calls (index, then each entry), not one atomic snapshot, so an unlocked Load could run concurrently with another process's Save/Delete mid write and observe a torn state. I extended the same fix to Status, which had the identical unprotected read.
• Added a test for read()'s index/entry desync recovery: a key listed in the index whose own entry is missing is skipped rather than failing the whole read.

go build, go vet, and go test -race -count=1 ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... are all clean.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Bound the key index as well as each token entry
  internal/oauth/store.go:560
  The new oauth-tokens-index is still one base64-encoded value written through Keyring.Set, and therefore through macOS security -i's 4095-byte command-line cap. ValidateKey permits 137-byte provider keys; an index containing 22 such keys serializes to 3,081 bytes and base64-expands to 4,108 bytes before the security command framing, so the index write fails even when every token is tiny. The token entry has already been written, but it is not indexed and cannot be loaded. Use a bounded/chunked enumeration format (or another bounded scheme) and add a cap-aware regression test.

• [P1] Make the multi-entry update recoverable instead of leaving invisible credentials behind
  internal/oauth/store.go:549
  A write now has several externally visible steps with no durable recovery state. If the process dies after a per-token Set but before the index write, the new credential is unindexed forever; if it dies or Delete fails after line 564 publishes a reduced index, a logged-out credential remains in the OS keychain forever because future cleanup only consults that reduced index. An index-write failure can also return an error after replacing an already-indexed token. This is especially harmful for refresh tokens: the caller can be told login/logout failed while the secret has already changed or remains resident but is unreachable through Zero. Add transactional/recovery metadata (or order the operations with a recoverable invariant) and failure-injection coverage for every interruption boundary.

• [P1] Keep the lock valid for the longer multi-key keyring operation
  internal/oauth/store.go:609
  The split format holds oauth-keyring.lockfile while it runs one external keyring command per entry, but acquireFileLock reclaims any lock older than 30 seconds and never refreshes its mtime. Each command may legitimately take up to 10 seconds, so a normal read or write with only a few slow entries can exceed 30 seconds; another process then reclaims the live lock and resumes a concurrent read-modify-write, allowing the token-loss race this lock is intended to prevent. Refresh/lease the lock during the operation or use a stale timeout that safely covers the bounded keyring work.

• [P1] Preserve tokens when old and new binaries run during an upgrade or downgrade
  internal/oauth/store.go:488
  Once a new binary creates the index it ignores the legacy combined entry, while an older running binary continues to read and write only that legacy entry. After migration, an old process can save token C to the legacy blob; the next new-process save reads only the index, rewrites it without C, and deletes the legacy blob, silently losing C. The shared lock serializes the two processes but cannot reconcile the two schemas. Provide a compatibility/dual-write transition or otherwise detect and merge legacy writes before removing the legacy entry.

• [P2] Do not make file-backed reads fail behind a crashed writer's fresh lock
  internal/oauth/store.go:279
Load and Status now unconditionally acquire the blob lock, including for the file backend. File writes are atomic renames and reads were intentionally lock-free; after a writer crash leaves its lock file, the new read waits only five seconds and errors while the stale-lock threshold remains 30 seconds. That turns a recoverable process crash into roughly 30 seconds of OAuth read failures even though the last complete token file is readable. Restrict the read-side lock to the multi-get keyring backend, or adjust the file-lock recovery behavior so reads retain the former crash tolerance.

[gnanam1990]

Local review: built and ran go test ./internal/oauth on darwin/arm64; all pass. One real correctness concern worth confirming.

[euxaristia]

Pushed 0b616f7 for jatmn's five findings and gnanam1990's lock-path comment.

• Bounded index: the key index is now chunked. Continuation entries (oauth-tokens-index-1, -2, ...) hold overflow keys and are written before the header that references them, so no single index entry can exceed the macOS security -i line cap no matter how many providers are logged in. The old single-array format is still read transparently. TestStoreKeyringIndexStaysUnderEntryLimit saves 40 near-maximum-length keys, asserts every keyring value stays under the cap with margin, that the index actually chunked, and that shrinking removes stale chunks.
• Recoverable updates: write() now publishes the union of the prior and new key sets first, writes token entries second, deletes removed entries while the index still lists them, and shrinks the index last. The invariant is that any token entry existing in the keyring at any instant is listed in the published index, so an interrupted login or logout can never strand an invisible credential; read() already skips over-listed keys and the next write reconciles. TestStoreKeyringWriteInterruptionsLeaveNoInvisibleTokens and its Delete counterpart inject a failure at every mutating operation in turn and check that invariant plus full reconciliation at each boundary.
• Lock lease: withLock refreshes the lock file's mtime every 10 seconds while the multi-command operation runs, so the 30-second stale threshold can only expire for a genuinely crashed holder. TestStoreKeyringWithLockRefreshesLease covers it with a shortened interval.
• Mixed versions: a legacy combined entry that exists even though the index has been published was recreated by an old binary running alongside; its keys unseen by the indexed schema are merged into the state before the legacy entry is deleted, so the old binary's freshly saved token survives the next new-binary write. Keys the index already listed are not merged, so a deliberate delete is not resurrected. An old binary's in-place update to an already-indexed token is the one case that still loses to the next new-binary write; full dual-writing of the combined blob would reintroduce the very line-cap overflow this PR removes, so that narrow window is documented instead. TestStoreKeyringMergesFreshLegacyWriteFromOldBinary covers the save case.
• Read-side lock scoping: the blob interface gained withReadLock. The keyring backend routes it through the same cross-process lock as writes (its read is a multi-Get pass), while the file backend's is deliberately lock-free since its writes are atomic renames, restoring the old crash tolerance. TestStoreFileLoadToleratesCrashedWriterLock plants a fresh never-released lock file and asserts Load/Status succeed immediately.
• gnanam1990's P2: when ResolveStorePath fails, the lock path now falls back to the OS temp directory instead of disabling cross-process exclusion, so withLock is never a silent no-op for the keyring backend.

go build, go vet, and go test ./internal/oauth pass locally.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/oauth/store.go (1)

212-222: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Prefer a per-user fallback before /tmp.
acquireFileLock already uses O_EXCL and 0600, so symlink-following isn’t the issue, but os.TempDir() still gives a fixed lock path in a shared directory. On multi-user hosts another local user can pre-create or hold that file and make keyring saves time out. os.UserCacheDir() would avoid the shared-path DoS; add a test for the fallback branch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/oauth/store.go` around lines 212 - 222, Update the fallback
lock-path logic in the Store construction to prefer a per-user directory from
os.UserCacheDir(), creating or selecting an appropriate cache location before
falling back to os.TempDir() only if the user cache directory cannot be
resolved. Preserve the resolved store-path behavior, and add coverage for the
fallback branch verifying the per-user lock location.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 570-573: Extend the crashed-writer lock test around Status to
verify that Status remains lock-free, rather than merely accepting a successful
call after stale-lock expiry. Add a bounded timing or immediate-return assertion
for s.Status("") while preserving the existing error and single-entry checks;
keep Load’s existing timing assertion unchanged.

---

Outside diff comments:
In `@internal/oauth/store.go`:
- Around line 212-222: Update the fallback lock-path logic in the Store
construction to prefer a per-user directory from os.UserCacheDir(), creating or
selecting an appropriate cache location before falling back to os.TempDir() only
if the user cache directory cannot be resolved. Preserve the resolved store-path
behavior, and add coverage for the fallback branch verifying the per-user lock
location.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6bf70feb-0af6-48ed-a808-dbcac2b2f97f

📥 Commits

Reviewing files that changed from the base of the PR and between 8eac9aa and 0b616f7.

📒 Files selected for processing (2)

• internal/oauth/store.go
• internal/oauth/store_keyring_test.go

[Vasanthdev2004]

Clean approve from me this latest commit addresses the earlier collaborator P1s (chunked index, recoverable multi-step write, lock-lease refresh, mixed-version legacy merge, lock-free file reads), and the interruption-boundary tests cover every write/delete step; build, vet, gofmt, and the oauth suite pass. Two minor things worth a glance, neither blocking: when ResolveStorePath fails the keyring lock falls back to a shared os.TempDir() file, which on a shared /tmp multi-user host could cross users (a per-user cache dir would be safer); and in the mixed-version window a token an old binary saves to the legacy entry stays invisible to new-binary Load until the next new-binary Save (the write path merges it, so it's not lost, just temporarily unreadable).

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Keep the legacy blob readable until a migration has copied every entry
  internal/oauth/store.go:620
  On a legacy-only installation, Save first publishes the indexed header and only then writes the per-token entries. If the process dies or kr.Set fails after that header write, read sees an index and stops consulting the still-intact legacy blob, so the old credentials disappear. A later save treats the incomplete indexed view as authoritative, removes the missing keys, and deletes the legacy blob, making the logout permanent. The interruption tests seed an already-indexed store and do not cover this legacy-to-indexed boundary. Keep a committed migration marker/legacy fallback until all entries are present, and add a failure-at-each-step migration test.

• [P1] Do not discard a same-key refresh made by a concurrently running old binary
  internal/oauth/store.go:591
  Once the new index exists, an old binary can still refresh provider:alpha in the legacy combined entry. The next new-binary save reads the stale indexed alpha, skips the newer legacy value solely because prior[alpha] is true, rewrites the stale value, and deletes the legacy entry. This silently loses a successful refresh and leads to later authentication failures; the added mixed-version test covers only a newly introduced key, not an update to an already-indexed one. Reconcile same-key legacy updates (or retain/dual-write the legacy data for the compatibility window) rather than treating their presence as a deliberate delete.

• [P2] Refresh lock leases with wall-clock time, not the injectable token-expiry clock
  internal/oauth/store.go:811
StoreOptions.Now is a public option and may be a fixed/stale clock. After the first refresh, this sets the live lock mtime to that old value, while acquireFileLock judges staleness with real time.Since; another process can immediately reclaim the live lock and re-enter the keyring read-modify-write path concurrently. That revives the credential-loss race the lease is meant to prevent. Use time.Now() for the filesystem lease (or make both sides use the same clock) and cover a fixed custom clock.

• [P2] Bound the advertised index chunk count before issuing keyring reads
  internal/oauth/store.go:703
  The chunked-index parser accepts any integer in header.Chunks and performs one OS-keyring lookup for every value before returning. A corrupt index such as {"v":1,"chunks":1000000000,"keys":[]} makes every Load, Status, Save, and Delete loop through up to a billion lookups while holding the store lock; a real lookup can itself wait ten seconds. Reject unsupported headers and impose a sane maximum before the loop so damaged keyring state fails promptly instead of wedging OAuth operations.

• [P2] Do not put a per-user keyring lock at a predictable shared-temp path
  internal/oauth/store.go:218
  When ResolveStorePath fails, every user falls back to the same ${TMPDIR}/zero-oauth-keyring.lockfile. On a multi-user host another account can pre-create or keep refreshing that path; the victim then times out acquiring the lock for Load, Status, Save, and Delete despite using a separate OS keychain. The 0600 mode only protects the file after its creator wins. Prefer a user-private cache/state directory for the fallback and add coverage for this path.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 774-781: Strengthen the unsupported-version test around
blob.readKeyIndex by resetting the keyring get-call tracker before invoking it
and asserting exactly one lookup occurred afterward. This must verify that only
the index header is read and no advertised chunk is fetched before rejecting
version 2.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ae205459-3f0d-4de9-ab6d-908745acb243

📥 Commits

Reviewing files that changed from the base of the PR and between 0b616f7 and 7ddca09.

📒 Files selected for processing (2)

• internal/oauth/store.go
• internal/oauth/store_keyring_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/oauth/store.go

[euxaristia]

Pushed fixes for the review findings:

• Lock lease renewal now uses wall-clock time unconditionally, instead of the injectable store clock, so a fixed/backdated clock can no longer let another process reclaim a live lock mid-operation.
• The keyring index now rejects unsupported versions and out-of-range chunk counts before issuing any keyring reads, so a corrupted index can no longer force unbounded lookups.
• The fallback lock (used when the store path can't be resolved) now lives in a per-user location instead of a shared, predictable temp path.
• Also hardened legacy-to-indexed migration: reads now recover tokens from the legacy blob if a migration was interrupted, and writes reconcile a same-key refresh made concurrently by an older binary instead of discarding it.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve mixed-version writes on the read path
  internal/oauth/store.go:527
  Once the index exists, read consults the legacy entry only for keys already named by the index. An old binary that is still running can therefore write a new provider:carol login to the legacy blob, but a new binary's Load, Status, and FirstStored report it absent until an unrelated new-binary Save happens. The same one-way merge cannot observe an old-binary logout of an indexed key, so that credential remains usable. This is an explicitly supported upgrade window; retain/reconcile the legacy state on reads (with a protocol that can distinguish stale state from writes) until old writers cannot exist.

• [P1] Do not discard a refresh whose response has no expiry
  internal/oauth/store.go:612
legacyIsFresher accepts a legacy update only when both expirations are non-zero and the legacy value is later. PostToken intentionally leaves ExpiresAt zero when an OAuth response omits the optional expires_in, so an old binary can write a newly rotated access/refresh token that the next new-binary save treats as stale. It then overwrites the fresh value and deletes the legacy entry, producing authentication failures. Use a migration/write protocol that preserves such updates rather than using expiry as the sole version signal.

• [P1] Do not ignore failure to remove the legacy credential blob
  internal/oauth/store.go:727
  A Delete(alpha) can remove alpha from the indexed store and return success even when deletion of a leftover legacy blob fails. On a later save, alpha is no longer in prior, so the stale legacy value is classified as a fresh old-binary login and is written back. The user has successfully logged out according to the API but silently becomes logged in again; propagate/handle that failure or record reconciliation state that prevents resurrection.

• [P1] Bound a single token entry before sending it to the macOS keyring
  internal/oauth/store.go:703
  Splitting the blob does not ensure that one complete JSON Token, after base64 expansion and security -i framing, fits macOS's 4095-byte command-line limit. Large but valid JWT access/ID tokens or refresh tokens still make KeyringClient.Set fail, so the login path retains the very failure this PR claims to eliminate for that provider. Enforce a clear per-token limit with an actionable fallback, or chunk the token representation; the five-provider test only covers smaller synthetic values.

• [P2] Reject an index that the reader cannot reopen
  internal/oauth/store.go:826
writeKeyIndex will publish any number of chunks, while readKeyIndex rejects headers above maxKeyringIndexChunks (128). Enough valid provider/MCP keys therefore lets Save succeed and persist a 129-chunk header, after which every Load, Status, Save, and Delete fails before it can recover the store. Enforce the same maximum before publishing (or make the on-disk format readable beyond it).

[Vasanthdev2004]

Holding at changes requested, but this is close. The latest push handles both of my earlier notes: the per-user lock fallback (UserCacheDir, uid-scoped temp as last resort) is exactly what I had in mind, and the read-side legacy recovery plus the wall-clock lease refresh are good hardening.

Out of jatmn's latest round, one finding is a real bug I want fixed before merge: write() throws away the error from the final legacy-blob delete (store.go:727). After a logout, if that delete fails, Delete still reports success while the secret stays resident in the keychain — and worse, the next Save sees the leftover legacy key with prior[key] now false and writes it back as a "fresh old-binary login". A logged-out user silently becomes logged in again, and even a retried logout hits the same merge. Propagate that error and add a failure-injection case at that boundary like the ones you already built. While you're in there, enforce maxKeyringIndexChunks on the write side too — writeKeyIndex will happily publish a header that readKeyIndex then refuses, and the guard is a couple of lines.

The rest I'm deliberately not gating on: the read-path visibility gap and the zero-expiry freshness heuristic are confined to the transient old-binary window and documented as best-effort, which I'm fine with — I don't want more protocol thrown at that window. And a single token blowing the 4095-byte cap predates this PR (the combined blob hit that wall sooner), so a clearer error there can be a follow-up. Fix the two items above and I'll approve.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 471-475: Strengthen the legacy merge regression assertions: at
internal/oauth/store_keyring_test.go lines 471-475, load carol and verify its
access and refresh tokens match the expected values, not just presence; at lines
693-699, also assert the loaded credential’s ExpiresAt.Equal(fresh). Preserve
the existing checks for the other legacy values.
- Around line 408-412: Update the fault-injection tests in
internal/oauth/store_keyring_test.go at lines 408-412 and 627-635: capture the
Delete error in the first test and assert that a fired injection is returned;
capture the Save error in the second, assert it is returned, and verify seeded
tokens remain readable before reconciliation. Use the existing test helpers and
symbols, keeping reconciliation after these assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d6ae3098-ce1c-449e-9a50-629517bc5d25

📥 Commits

Reviewing files that changed from the base of the PR and between 7ddca09 and 496070e.

📒 Files selected for processing (2)

• internal/oauth/store.go
• internal/oauth/store_keyring_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/oauth/store.go

[euxaristia]

Pushed 496070e for the two gating items: the final legacy-blob delete's failure now propagates (a logout can no longer report success with the secret resident), and it runs while the union index still lists removed keys — so a retried logout reconciles instead of the stale legacy key being resurrected as a "fresh old-binary login" on the next save. writeKeyIndex now enforces maxKeyringIndexChunks before publishing anything, so Save can't persist a header readKeyIndex refuses. Added a failure-injection test at the legacy-delete boundary (including the no-resurrection-after-retry assertion) and a write-side cap test; the existing write-interruption test now asserts every mutating boundary surfaces its injected failure.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Bound the decoded index keys before iterating them
  internal/oauth/store.go:785
  The new guard only limits header.Chunks; it accepts an arbitrarily large header.Keys array (and the supported legacy bare-array format), then read invokes kr.Get once per element while holding the cross-process lock. A corrupt index containing thousands of duplicate or invalid keys therefore serializes thousands of OS-keyring calls—each can wait up to ten seconds—and wedges Load, Status, Save, and Delete. Cap the raw/decoded index and total unique valid keys before the fan-out, including every chunk and the legacy-array branch.

• [P1] Use a wall-clock deadline when taking the keyring read lock
  internal/oauth/store.go:933
withReadLock now calls withLock, which passes StoreOptions.Now to acquireFileLock. That helper derives both its deadline and timeout check from the supplied clock; with a legitimate fixed test/embedded clock, now().After(now().Add(5s)) can never become true. A fresh peer or orphan lock then makes keyring Load and Status retry forever instead of returning a lock-timeout error. Use wall/monotonic time for the acquisition deadline, as the lease refresher already does.

[euxaristia]

Pushed 7b22465.

Fixed:

• acquireFileLock computed its timeout deadline using the injectable clock, so a fixed clock could make lock contention retry forever instead of timing out. Now computes the deadline against wall-clock time, keeping the injectable clock for token generation only.
• readKeyIndex had no cap on keys per chunk, so a corrupted index could drive an unbounded keyring lookup fan-out while holding the store lock. Added a cap on both the legacy bare-array and chunked-header decode paths.
• Strengthened five existing tests that were marked addressed but weren't fully (missing error captures, missing value assertions, missing reset+assertion pairs).

Not fixing, per the sign-off already on this thread:

• The transient read-path visibility gap for an old binary's concurrent write, and the zero-expiry freshness heuristic: confined to the old-binary compatibility window and fine as documented best-effort.
• A single token exceeding the 4095-byte macOS security -i cap: pre-existing, predates this PR, follow-up item not a blocker here.

-race wasn't runnable in my environment (no gcc for cgo), worth running before merge.

[euxaristia]

Pushed ea5b965.

Hardening on top of 7b22465 / 496070e for the remaining write/read symmetry gap around the key-count cap:

• writeKeyIndex now refuses a key list over maxKeyringIndexKeys before publishing anything (same class of bug as the write-side maxKeyringIndexChunks guard). Short keys can still fit under the chunk-count cap while exceeding the reader key cap; without this check Save could persist an index every later Load/Status/Save/Delete would reject.
• Extended the oversized key-list reader test: header-only and legacy bare-array paths now assert header lookup only, and a multi-chunk accumulation case (small header + oversized chunk-1) is covered.
• Added TestStoreKeyringWriteIndexRejectsOverCapKeys for the write-side path.

Human findings already on this PR, with the commits that closed them:

• jatmn (on 496070e): wall-clock lock acquisition deadline + bound decoded index keys before fan-out -> 7b22465; write-side key-cap symmetry above -> ea5b965
• Vasanthdev2004 (on 11d038d): propagate legacy-blob delete failure on logout + write-side maxKeyringIndexChunks -> 496070e

Still not fixing, per the existing sign-off on this PR:

• Transient read-path visibility gap for an old binary's concurrent write, and the zero-expiry freshness heuristic: confined to the old-binary compatibility window and documented best-effort.
• A single token exceeding the 4095-byte macOS security -i cap: pre-existing, predates this PR, follow-up item.

go fmt, go vet, golangci-lint (unused/ineffassign/staticcheck), govulncheck, and go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... are clean. -race needs gcc/cgo and was not runnable in this environment.

[euxaristia]

@Vasanthdev2004 Ready for re-review on head ea5b965.

Both gating items are in:

• write() now propagates the legacy-blob delete error (store.go), with a failure-injection case at that boundary: TestStoreKeyringLogoutSurfacesLegacyBlobDeleteFailure.
• The index caps are enforced on the write side too: writeKeyIndex refuses over-cap chunk counts and key counts before publishing anything (TestStoreKeyringWriteIndexRejectsOverCapChunks, TestStoreKeyringWriteIndexRejectsOverCapKeys).

jatmn's two P1s from the same round (bounding decoded index keys in readKeyIndex and the wall-clock lock deadline) are also fixed, with tests. CI is green on all jobs.

[euxaristia]

Addressed both P1s from the latest CHANGES_REQUESTED review (and rebased onto current main).

[P1] Bound decoded index keys before iterating them

readKeyIndex now enforces maxKeyringIndexKeys (128 chunks * 200) on:

• the legacy bare-array format
• header.Keys before any chunk fan-out
• accumulated keys across continuation chunks

writeKeyIndex refuses to publish a key set over that same cap, so Save cannot strand the store with an unreadable index. Regression: TestStoreKeyringReadIndexRejectsOversizedKeyList, TestStoreKeyringWriteIndexRejectsOverCapKeys.

[P1] Use wall-clock deadline when taking the keyring read lock

acquireFileLock measures the acquisition deadline and timeout check with time.Now(), not the injectable StoreOptions.Now. A fixed test/embedded clock can no longer turn a contested lock into an infinite retry. withReadLock shares that path via withLock. Regression: TestAcquireFileLockDeadlineUsesWallClockNotInjectedClock.

Verification

• go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... -count=1 (pass; -race unavailable here without cgo)
• Branch rebased onto gitlawb/main and pushed to origin

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Keep the keyring lock independent of the file-store location
  internal/oauth/store.go:218
  The keyring data is always shared under the fixed zero service and oauth-tokens-index account, but this derives its cross-process lock from ZERO_OAUTH_TOKENS_PATH/XDG_CONFIG_HOME. Two concurrent keyring-backed Zero processes using different supported config roots therefore acquire different lock files, both read the same index, and publish competing read-modify-write updates; the last header hides the other process's token entry. Use one per-user lock identity derived from the keyring store itself, rather than from an unrelated file-backend path.

• [P1] Do not delete an unreadable legacy blob during mixed-version reconciliation
  internal/oauth/store.go:600
  Once an index exists, a live old binary can still write a new login into the legacy blob. If readLegacy has a transient keyring/decode error here, readLegacyTokens converts it to an empty map, so write proceeds and deletes that legacy entry at line 729. The next save has then irreversibly discarded the old binary's credentials even though its recovery path was unavailable. Propagate the read/validation failure (or retain the blob and defer cleanup) whenever a legacy entry cannot be safely reconciled.

• [P2] Deduplicate and validate the decoded index before reading token entries
  internal/oauth/store.go:809
  The new cap permits 25,600 raw entries but does not reject duplicate or invalid keys. A corrupt index containing 25,600 copies of provider:alpha passes these checks, and read then performs one native keyring lookup per copy while holding the cross-process lock. Since each lookup may consume the keyring client's 10-second timeout, Load, Status, Save, and Delete can be blocked for hours or days—the corruption fan-out this change is intended to bound. Validate and deduplicate entries before the fan-out, and cap the resulting unique key set to a practical limit.

[euxaristia]

Rebased onto current upstream main (was 17 commits behind) and pushed fixes for the three findings from jatmn's review.

Cross-process lock keyed off the wrong identity (P1): the lock guarding the keyring token index was derived from ResolveStorePath (ZERO_OAUTH_TOKENS_PATH / XDG_CONFIG_HOME), not from the keyring's own identity. Two processes with different config roots but pointed at the same OS keyring entry got different lock files and could race a read-modify-write on the shared index. The lock path is now derived from the keyring service and account (keyringLockPath), so it always matches the entry actually being touched regardless of caller config. Added TestKeyringLockPathDerivedFromKeyringIdentityNotFileConfig, which builds two stores with different XDG_CONFIG_HOME roots and asserts they resolve to the identical lock path.

Transient read error treated as empty, destroying data (P1): readLegacyTokens collapsed every failure reading the legacy combined entry (a transient keyring error, bad base64, invalid JSON) into the same result as "doesn't exist." write() used that during mixed-version reconciliation and then deleted the legacy blob unconditionally, so a transient read failure looked identical to "nothing to reconcile" and the blob got deleted anyway. readLegacyTokens now returns an explicit error distinct from "not found." read()'s recovery fallback stays best-effort since it doesn't delete anything, but write() now aborts the whole write and surfaces the error instead of proceeding as if the blob were empty. Added TestStoreKeyringWriteRefusesToDeleteLegacyBlobOnTransientReadError, which injects a Get failure on the legacy entry and checks the blob survives, the write fails, and everything reconciles correctly once the failure clears.

Unbounded index fan-out DoS (P2): the decoded index (documented cap 25,600 entries) wasn't deduplicated before Load/Status/Save/Delete fan out into one keyring lookup per key. A corrupted index repeating the same key thousands of times still cost one blocking lookup per repeat (up to the 10s command timeout each) while the store lock was held. readKeyIndex now runs its result through dedupeValidKeys, dropping duplicates and malformed (non-ValidateKey-shaped) entries before any caller fans out. Added TestStoreKeyringReadIndexDedupesDuplicateKeys and TestStoreKeyringDuplicateIndexDoesNotFanOutPerEntry (the latter asserts Status makes 2 keyring gets, not 3000, against a 3000-duplicate index).

Verification: for each fix I temporarily reverted just that change, confirmed the new regression test failed against the reverted code, then restored the fix and confirmed it passed. Also ran go build ./..., go vet ./..., the full internal/oauth suite, and oauth-adjacent tests in internal/cli and internal/tui, all green. Checked git diff --check against upstream/main for the changed files, clean.

Pushed as three separate commits (one per finding) on top of the rebase, no squashing of the prior history.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Restore atomic cron job-ID reservation
  internal/cron/store.go:102
allocID now returns an ID after an unlocked os.Stat, and Add creates the directory only later through writeJob. Two zero cron add processes in the same second can therefore both choose the same ID and race on metadata.json, with the last rename silently replacing one caller's job even though both calls succeed. This also reopens the Add/Remove resurrection window. Please retain an atomic mkdir reservation (and the per-ID lock) through the initial metadata write; the deleted concurrent-add regression test covered this exact case.

• [P1] Make the keyring lock path independent of XDG_CACHE_HOME
internal/oauth/store.go:261
  The fixed keyring service/index is shared by all same-user processes, but os.UserCacheDir() is selected from each process's XDG_CACHE_HOME (and the fallback is similarly selected by TMPDIR). Processes using different cache roots consequently acquire different locks, both read the same index, and can publish competing read-modify-write updates that hide one token. The new test varies XDG_CONFIG_HOME, which this helper does not read. Use a stable per-keyring-user lock location or otherwise make the lock independent of those per-process roots, with a regression test for distinct cache roots.

• [P1] Coordinate migration with the legacy lock used by old binaries
  internal/oauth/store.go:220
  Pre-PR binaries lock beside ResolveStorePath, whereas this version locks under the cache-derived keyring path. They therefore do not exclude each other during the explicitly supported mixed-version window. A new save can reconcile the legacy blob, an old process can then save a fresh legacy credential, and the new save unconditionally deletes that blob at store.go:781 without having observed the old write. The old process's successful login or refresh is lost. Keep a compatible lock/protocol for the legacy account until old writers are no longer supported, and cover a write occurring between reconciliation and legacy deletion.

[euxaristia]

I rebased this branch onto upstream/main. That picked up #686 (fix(cron): reserve job IDs atomically), which fixes the atomic job-ID reservation finding at internal/cron/store.go:102 for free; that file now has no diff against upstream/main.

For the other two findings, both store.go:

The keyring lock was anchored on os.UserCacheDir(), which reads XDG_CACHE_HOME per process (falling back through os.TempDir()/TMPDIR). Two same-user processes with different cache or temp roots picked different lock files, both read the shared keyring index, and could race a read-modify-write that hid one process's token. I moved the anchor to the user's home directory instead, which does not vary this way between processes of the same real user. Added TestKeyringLockPathIndependentOfCacheAndTempRoots, which varies XDG_CACHE_HOME/TMPDIR between two simulated processes and confirms they still resolve to the same lock.

Anchoring on the home directory also lines the new lock up with where a pre-PR binary resolves its own lock (beside ResolveStorePath, which falls back to home too). I made write() hold that legacy lock as well for the whole reconcile-then-delete pass over the legacy combined entry, so a live old binary can't write a fresh legacy login or refresh in the window between this binary's reconciliation read and its legacy-blob delete and have that write silently discarded. Added TestStoreKeyringWriteWaitsForLegacyLockDuringReconciliation, which holds the legacy lock the way a live old binary would and confirms Save blocks until it's released, with the seeded legacy token intact afterward.

go build ./..., go vet, go test ./internal/oauth/... ./internal/cron/..., and gofmt are all clean.

[coderabbitai]

🧹 Nitpick comments (1)

internal/oauth/store_keyring_test.go (1)

1014-1029: 🩺 Stability & Availability | 🔵 Trivial

Make sure this concurrent path actually runs under -race in CI.

This is the one test spawning a goroutine that races Save against a held legacy lock — precisely the cross-process hazard the PR exists to fix. The PR notes race testing was skipped where gcc/cgo was missing, which means the highest-value concurrency test can silently run without the detector. Ensure at least one CI lane builds with cgo and runs go test -race ./internal/oauth/....

As per coding guidelines: "run affected concurrent code under the race detector."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/oauth/store_keyring_test.go` around lines 1014 - 1029, Update CI
configuration to add at least one lane with cgo/gcc available that runs `go test
-race ./internal/oauth/...`. Ensure the lane does not silently skip race testing
when cgo is unavailable, so the concurrent Save test around the legacy lock
executes under the race detector.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 1014-1029: Update CI configuration to add at least one lane with
cgo/gcc available that runs `go test -race ./internal/oauth/...`. Ensure the
lane does not silently skip race testing when cgo is unavailable, so the
concurrent Save test around the legacy lock executes under the race detector.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6cf993ee-5593-43dc-b059-d9516a070a64

📥 Commits

Reviewing files that changed from the base of the PR and between 98e1664 and 9e3f52c.

📒 Files selected for processing (3)

• internal/oauth/lock.go
• internal/oauth/store.go
• internal/oauth/store_keyring_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/oauth/lock.go
• internal/oauth/store.go

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Coordinate the legacy migration lock across different config roots
  internal/oauth/store.go:298
legacyKeyringLockPath is computed from this process's ResolveStorePath, while pre-PR binaries lock beside their configured file-store path even though both versions modify the same fixed zero/oauth-tokens keyring item. With different supported XDG_CONFIG_HOME or ZERO_OAUTH_TOKENS_PATH values, the locks differ: the new writer can reconcile the legacy blob, an old writer can save a credential, and the new writer then deletes that unseen credential at step 4. The new global lock cannot protect this because old binaries do not take it.

• [P1] Do not discard legacy token rotations with an unknown or unchanged expiry
  internal/oauth/store.go:703
legacyIsFresher accepts an old-binary update only when both expiries are nonzero and the legacy expiry is strictly later. Zero expiry is a supported token state, and a standards-valid refresh response that omits expires_in produces exactly that state; equal expiries can also accompany a rotated access/refresh token. The next new-version save keeps the stale indexed credential and deletes the only fresh legacy copy, leaving the user with revoked credentials.

• [P1] Read a live legacy credential after an index already exists
  internal/oauth/store.go:596
  Once any index exists, read consults the legacy blob only for keys already listed in that index whose individual entries are missing. A concurrently running old binary can therefore successfully log in to a new provider after migration (or after the new binary writes an empty index on logout), but new-version Load and Status return no token indefinitely unless some unrelated new-version save happens to reconcile it. This breaks the advertised mixed-version window for normal read-only startup and request paths.

• [P2] Reduce the accepted index size to a practical bounded operation
  internal/oauth/store.go:850
  The new cap still accepts 25,600 distinct valid keys. Every Load, Status, Save, and Delete subsequently performs one keyring lookup per key while holding both the store mutex and cross-process lock; each lookup may consume the keyring command's ten-second timeout. A damaged but in-range index can therefore block all OAuth operations for many hours, which defeats the corruption/fan-out protection this change is adding. Cap the number of credentials to a realistic bound (or otherwise bound the total lookup time/work) before fanning out.

[coderabbitai]

🧹 Nitpick comments (1)

internal/oauth/flow_test.go (1)

303-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the new empty-scope fallback branch.

This test covers non-empty current.Scopes, but the change also adds the current.Scopes == empty → cfg.Scopes path. Add a case with an empty current scope list, configured fallback scopes, and an omitted response scope, then assert the configured scopes are preserved.

As per coding guidelines, **/*_test.go files must add regression tests for behavior changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/oauth/flow_test.go` around lines 303 - 316, The existing
TestRefreshPreservesScopesWhenOmitted only covers non-empty current.Scopes. Add
a regression case covering an empty current scope list with configured
cfg.Scopes and no scope in the refresh response, then assert Refresh preserves
the configured fallback scopes.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/oauth/flow_test.go`:
- Around line 303-316: The existing TestRefreshPreservesScopesWhenOmitted only
covers non-empty current.Scopes. Add a regression case covering an empty current
scope list with configured cfg.Scopes and no scope in the refresh response, then
assert Refresh preserves the configured fallback scopes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0444b2d3-7dca-4f68-99c2-6c2b66e81a9b

📥 Commits

Reviewing files that changed from the base of the PR and between 9e3f52c and 226852b.

📒 Files selected for processing (3)

• internal/oauth/flow.go
• internal/oauth/flow_test.go
• internal/oauth/store.go