fix: handle external rotation of the Claude Code credential - #260
Conversation
…ed up refreshIfNeeded re-read only file sources, on the stated assumption that a keychain entry is mutated solely by our own writeBackCredentials. That is false: cswap, the claude CLI in another terminal, and a second OpenCode instance all rotate it. An external switch produces no 401, so nothing triggered a re-read and the session stayed pinned to the account that was active at startup for the ~10h life of its token. Re-reading every source also stops the proactive refresh timer writing a stale account's rotated tokens over whichever account is now active. refreshAccount throws for keychain sources when the keychain is locked, access is denied, or `security` times out — a new throw path on the request path, hence the catch that degrades to the in-memory credentials. Test repairs, all one root cause: the up-front re-read makes the stub's source blob visible to accounts whose in-memory credentials differ, so several tests needed their source pinned or their read counts corrected. Three were anticipated. Three were not, and are worth noting: - "reloadCredentialsFromSource bypasses cache": primed via __setCredentialsForSource so the rotation it tests still lands. - "does not spawn the claude CLI while credentials are still usable": source pinned to the account's own value. - "falls back to the claude CLI once credentials reach the expiry window": its __setReadHook rotated the store on read #2, which is now the pre-CLI re-read rather than the post-CLI one. The result assertion still passed while the CLI path silently stopped being exercised; the threshold moves to read #3 to restore the original coverage.
The previous wording gave a rationale for the second source read — a sibling process can write during the OAuth round trip — that applies just as well to file sources, then excluded them anyway "as they did when the up-front re-read was file-only". That describes the old behavior rather than justifying the new one, and reads as though the asymmetry were principled. It isn't. Removing the guard is a behavior change on the file path and is tracked as a follow-up; say so plainly so the next reader does not mistake it for an invariant worth preserving.
The re-read adopted any non-null blob over valid in-memory credentials. performRefresh ignores writeBackCredentials's return value, and that write can fail while the read before it succeeded — updateCredentialBlob returns null on a malformed blob, and an ACL can permit read but not add-generic-password. Memory then holds freshly refreshed credentials while the store holds the pre-refresh blob, which has under 60s left because that window is the only reason we refreshed at all. Adopting it re-entered performRefresh with a refresh token our own successful refresh had just rotated dead: OAuth fails, the post-OAuth re-read finds the blob it already adopted, and it falls through to two 60s claude spawns — on every cache miss, every 30s, indefinitely, on the authentication path. Keychain sources were immune to this before the up-front re-read was extended to them. The rule is: adopt a usable stored blob always, an unusable one only when what we hold is unusable too. A plain freshness guard would also decline an expired blob written by a legitimate external switch, reintroducing the bug this re-read exists to fix. The residual cost is recorded at the call site. Also clear the borrowed flag on adopt. The re-read pulls from the account's own source, so anything it adopts is the account's own credentials; refreshBorrowedAccount already pairs every adoption with the same delete. Left set, forceRefreshActiveAccount would refuse to exchange tokens that are legitimately the account's.
The guard's rationale claimed the orphaned pre-refresh blob "has under 60s left, since that window is the only reason we refreshed". That holds on the reactive path but not on the proactive one: the sync timer passes PROACTIVE_REFRESH_THRESHOLD_MS (1h), so a failed write-back there orphans a blob with up to an hour remaining — which the guard's first clause reads as usable and adopts. The guard is unchanged and still correct; the comment was under-describing the remaining exposure, and a maintainer would have read the failure as fully closed. The proactive case is categorically milder: the adopted token is genuinely valid so requests keep succeeding, and performRefresh's CLI_FALLBACK_THRESHOLD_MS gate declines to spawn claude while credentials remain usable, so the cost is wasted background refreshes until the blob drops under 60s and the CLI fallback recovers. Also record that no guard on the re-read can close it: the re-read cannot distinguish "store is stale because our write failed" from "store changed because cswap switched", since both present as store-disagrees-with-memory and store-is-usable. That distinction exists only in the return value performRefresh discards, which is tracked separately.
…ded write-back result
Split writeback_skipped_stale into stale and unparseable: a mismatch is an expected account switch, an unreadable blob is corruption, and only the second needs a human. Both now carry hashed token fingerprints so the decision can be correlated without logging token material. Cover the two call sites that had no test: the borrowed-recovery path, where guarding with the lender's token would have failed every CAS silently, and the force-refresh path, where reading the token after the await would compare the refreshed token against the store.
Greptile SummaryThis PR fixes a long-standing assumption that this plugin was the sole writer to the active Claude Code credential, enabling external tools (the
Confidence Score: 5/5Safe to merge. The four changes are well-scoped, all new code paths are protected by try/catch with graceful degradation, and the test suite grew by 25 cases with mutation verification on previously green-but-broken tests. The core logic — source re-read on cache miss, CAS write-back guard, bounded 401 recovery loop, single 429 re-read — is correct and carefully documented. No existing callers were broken. The two observations (429 → retry → 401 escapes recovery; borrowed-flag assumption on file accounts) are edge cases in already-exotic paths that the author has explicitly flagged as follow-up territory in the specs. None introduce wrong data or broken state for the common cases. Files Needing Attention: The
|
| Filename | Overview |
|---|---|
| src/credentials.ts | Core changes: refreshIfNeeded now re-reads every source (keychain and file) on cache miss; performRefresh has a documented-but-unresolved file-source exclusion; forceRefreshActiveAccount gains CAS write-back and cache update; reloadCredentialsFromSource clears the borrowed flag correctly. |
| src/keychain.ts | New credentialBlobMatches and CAS logic in writeBackCredentials are clean and correctly implement the compare-and-swap guard for both file and keychain paths. |
| src/index.ts | 401 recovery loop wired correctly; 429 re-read ordered after the loop and correctly compares against tokenInUse; a 401 from the 429 retry is returned untransformed without running through recovery, which is intentional but limits handling of a narrow edge case. |
| src/credentials.test.ts | 25 new tests covering the borrowed-flag invariant, CAS write-back, configDir forwarding, source re-read adoption logic, and mutation verification for previously green-but-broken cases. |
| src/index.test.ts | New tests pin the 401 recovery loop cap, the reload-throws path, 429 re-read ordering, and the rotated-store-on-every-read termination guarantee. |
| src/keychain.test.ts | credentialBlobMatches unit tests and two writeBackCredentials end-to-end branches (match/mismatch) added; mock security layer extended to record and replay add-generic-password writes. |
Reviews (2): Last reviewed commit: "docs: close the configDir follow-up and ..." | Re-trigger Greptile
…rite-back Every refreshAccount call site in this file forwards the account's configDir except the two re-read paths. Unreachable today — every file account is assigned CLAUDE_CONFIG_DIR ?? ~/.claude, exactly what readCredentialsFile recomputes by default — but the compare-and-swap guard makes the read and the write resolving to the same file load-bearing, so make that hold by construction rather than by coincidence. performRefresh discarded writeBackCredentials's return value, so a rejected write-back left no trace anywhere in the debug log while forceRefreshActiveAccount logged the same condition. Log it. Acting on it is a control-flow change that interacts with the adoption logic this branch introduces, and stays deferred. Also drops reloadActiveAccount's claim to be used on 401; it has no call sites.
Records reads and lets the write-back result be forced in the keychain stub, and captures log events in the logger stub, so all three arms are observable. Mutation-verified: reverting either configDir forward or inverting the write-back check fails exactly the matching test.
The configDir asymmetry is fixed and pinned. The discarded write-back return value is now half-closed: the failure is logged, while not re-adopting an orphaned-but-usable blob on the proactive path stays deferred.
|
Reviewed all three. One diagnosis is wrong, one is understated, and two are staying deferred. Pushed 1.
|
🤖 I have created a release *beep* *boop* --- ## [2.1.5](v2.1.4...v2.1.5) (2026-07-30) ### Bug Fixes * handle external rotation of the Claude Code credential ([#260](#260)) ([5a44883](5a44883)) * refresh OAuth tokens with native fetch instead of a subprocess ([#258](#258)) ([231165b](231165b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
The plugin assumed it was the only writer to the active Claude Code credential.
src/credentials.tsstated it outright:That is false. claude-swap, the
claudeCLI in another terminal, and second OpenCode instances all rotate it, as does a server-side revocation.The user-visible bug: an external account switch produces no 401, because the previous access token is still valid. Nothing triggered a re-read, so an OpenCode session stayed pinned to whatever account was active at startup for the ~10h life of that token. Running
cswap autohad no effect on OpenCode at all.Four changes, none claude-swap-specific — no new configuration, no environment variable, no subprocess, no dependency on cswap being installed:
refreshIfNeededre-reads every source, not justfilewriteBackCredentialsforceRefreshActiveAccount, which was fully written, documented and tested with zero call sitesThe re-read validates before adopting: a usable stored blob is always taken, an unusable one only when what is already held is also unusable. Without that, a failed write-back resurrects the pre-refresh token and loops
claudeCLI spawns every 30s.Why cooperating with cswap is safe, verified in its source rather than assumed:
switcher.py:4694copies live credential bytes into the departing account's slot on switch, so a token this plugin rotates is captured rather than orphaned.Also fixed
CLAUDE_CONFIG_DIRcould be written to the wrong file. Latent, since that path had no callers until now.Behavior change
Not covered by the generated changelog
CHANGELOG.mdis release-please generated from commit subjects. Both items above live inside commits whose subjects do not mention them, so neither will appear in the release notes automatically.Test plan
Suite: 249 → 274 passing, lint clean,
tscclean.Review found three places where a test passed while the code was broken. Each is now pinned and mutation-verified:
MAX_AUTH_RECOVERY_ATTEMPTSraised 2 → 5 — was green, now failscredentialBlobMatchesunit + both write-back branches end-to-endEvery automated test stubs the Keychain, so the following still needs doing against real state before release:
cswap list— confirm two accounts, note the active onecswap switchin another terminalCLAUDE_AUTH_DEBUG=1, check for the new account's token and nowriteback_skipped_stalestormcswap list— confirm neither slot is quarantined for a dead refresh tokenNotes for review
Design and implementation plan are committed under
specs/, including a Follow-ups section recording nine deliberately deferred findings with rationale — among them thatperformRefreshdiscardswriteBackCredentials's return value, that the post-OAuth file-source exclusion is now an unjustified historical artifact, and that two functions remain dead with docstrings that no longer describe the code.