From e4223d5dd8c54b070464c10f1a425b00929e25be Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 11:59:47 -0600 Subject: [PATCH 01/33] docs: design for external credential rotation handling --- ...-29-external-credential-rotation-design.md | 226 ++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 specs/2026-07-29-external-credential-rotation-design.md diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md new file mode 100644 index 0000000..ae1fdaf --- /dev/null +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -0,0 +1,226 @@ +# External credential rotation + +Date: 2026-07-29 +Status: approved, not yet implemented + +## Context + +The plugin assumes it is the only writer to the active Claude Code credential. +That assumption is encoded explicitly at `src/credentials.ts:328-334`: + +> macOS keychain sources stay on the in-memory path; their state is mutated only +> by our own `writeBackCredentials`, so no external-update vector exists for them. + +The assumption is false. Several things rotate or replace that credential: + +- `claude-swap` (`cswap`), switching accounts manually or via `cswap auto` +- the `claude` CLI running in another terminal +- a second OpenCode instance +- Anthropic revoking a token server-side + +This design makes the plugin correct in the presence of external mutation. It is +motivated by cswap but contains no cswap-specific code, no subprocess, no `PATH` +dependency, and no new configuration. + +### Why cswap is safe to cooperate with + +Verified in cswap's source rather than assumed: + +- `switcher.py:4694` — the switch-time backup copies **live** credential bytes + into the departing account's slot. A token this plugin rotates is captured by + cswap on the way out, so it is never orphaned and the account is not + quarantined for a dead refresh token. +- `switcher.py:5074` — cswap fails fast if it cannot read the live credential, + rather than overwriting state that has no safety copy. + +Consequence: refreshing the **active** credential is safe with no coordination. +The plugin is indistinguishable from Claude Code refreshing its own token. This +asymmetry is why the design targets the active credential only. + +## Problems + +1. **External rotation is invisible.** `refreshIfNeeded` re-reads only `file` + sources (`src/credentials.ts:331`). The sole re-read of a Keychain source + during normal operation is in the 401 handler (`src/index.ts:374`). An + external switch produces no 401, because the previous access token is still + valid — so the session stays pinned to the account that was active at startup + for the life of that token (~10h). + +2. **Stale-snapshot write-back.** The proactive refresh tick (`src/index.ts:224`, + every 5 min, acting within 1h of expiry) refreshes from an in-memory snapshot + and calls `writeBackCredentials`. If the active credential was replaced + externally in the meantime, the plugin writes the *stale* account's rotated + tokens over the *current* account's slot. Because refresh tokens rotate on + use, the losing copy is dead. + +3. **401 recovery gives up too early.** `src/index.ts:371-392` re-reads the + source and retries only if the access token changed. When the source still + holds the rejected token, it surfaces the 401. + `forceRefreshActiveAccount()` (`src/credentials.ts:606`) exists for exactly + this case and has **no non-test call sites**. `invalidateCredentialCache()` + (`:647`) and `reloadActiveAccount()` (`:585`) are likewise dead. + +4. **Rate-limit errors leak after an external switch.** When cswap switches + because an account hit its quota, the plugin keeps using the exhausted + account until its cache expires, surfacing 429s that a re-read would avoid. + +## Goals + +- An external account switch takes effect in a live session without a restart. +- A request that fails on an expired or revoked token recovers within the same + request where possible, and always leaves the next prompt working. +- The plugin never writes one account's tokens into another account's slot. + +## Non-goals + +Explicitly out of scope. Each was considered and rejected: + +- **cswap-aware UX** — account roster, email labels, or usage in + `opencode auth login`. Requires cswap on `PATH`. +- **Pinning OpenCode to a non-active cswap slot** (parallel accounts). Cold + slots carry expired access tokens; cswap exposes no "refresh slot N" command, + so the plugin would become an unsynchronized second writer to cswap's backup + store, needing its `fcntl.flock` and `.prev` retention semantics. All of the + risk, only parallelism in return. +- **Plugin-driven `cswap switch`.** cswap already handles rotation; driving it + from here would fight `cswap auto`. +- **Any env var or opt-in gate.** Nothing here is cswap-specific, so there is + nothing to gate. + +## Design + +### 1. Re-read every source in `refreshIfNeeded` + +`src/credentials.ts:331` — drop the `target.source === "file"` guard so Keychain +sources are re-read too. + +Cost is bounded: `refreshIfNeeded` runs only on a `getCachedCredentials()` cache +miss, so at most about twice a minute under load (`CREDENTIAL_CACHE_TTL_MS`, +`src/credentials.ts:26`). A `security find-generic-password` call is ~10-50ms. + +This resolves problems 1 and 2 together: the plugin refreshes and writes back +whatever is *currently* active rather than a stale snapshot. + +**New failure path.** `readKeychainService` throws on a locked, denied, or timed +out Keychain (`src/keychain.ts:96-142`), whereas the file path swallows errors +inside `readCredentialsFile`. The re-read must therefore be wrapped so a +transient Keychain failure degrades to the in-memory credentials instead of +propagating into the request path. + +Worst-case latency for an external switch to take effect: one cache TTL (30s). + +### 2. Compare-and-swap on write-back + +`writeBackCredentials` (`src/keychain.ts:442`) gains an optional expected prior +access token. When supplied and the stored blob no longer carries that token, +the write is skipped and logged rather than performed. + +This closes the residual window in problem 2: the OAuth round-trip between +re-read and write-back is seconds wide, and an external switch can land inside +it. + +Nearly free — the Keychain branch already reads the existing blob at +`src/keychain.ts:475` to preserve unrelated fields, so this adds a comparison +rather than a read. Callers that legitimately have no prior token (initial +sync) omit the argument and keep today's behavior. + +On mismatch the session continues from memory; the next cache miss re-reads and +converges. + +### 3. Bounded 401 recovery loop + +Replace the single if/else at `src/index.ts:371-392` with a loop of at most two +recovery attempts. Each attempt: + +1. Re-read the source (`reloadCredentialsFromSource`). If it yields a token + different from the one just used, retry with it. +2. Otherwise call `forceRefreshActiveAccount()` — the currently dead function — + and retry with its result. +3. If neither produces a token different from the one just used, stop. + +The no-progress check is what bounds the loop; the attempt cap is defence in +depth. + +Two attempts are required, not one. After a switch to a slot whose access token +is cold, step 1 *does* return a different token, so the existing code retries, +401s again, and surfaces — `forceRefreshActiveAccount` never runs. The second +iteration re-reads, finds the token unchanged, and force-refreshes. One +structure covers external rotation, revoked tokens, and cold-slot switches. + +`forceRefreshActiveAccount` already guards borrowed credentials +(`src/credentials.ts:617`), writes back on success, and updates the cache, so it +is drop-in. + +**Intentional behavior change.** `preserveResponseUnchanged` (`src/index.ts:462`) +decides whether the response bypasses `transformResponseStream()`. Today a +non-retried 401 bypasses it but a retried-then-still-401 does not. After this +change the flag is set from the status after the loop exits +(`response.status === 401`), so both surface the raw error body consistently and +a recovered request is still stream-transformed. + +### 4. Re-read on 429 with a changed token + +After the 401 block, if the response is 429, re-read the source once. Retry only +if the access token changed — which is true exactly when an external switch +happened, and false otherwise, making this free in the common case. + +No `src/http.ts` change is needed. `fetchWithRetry` already returns immediately +when `retry-after` exceeds the cap (`src/http.ts:59-66`), which is the +quota-exhaustion signal. + +This must sit **after** the 401 block and **before** the long-context beta loop +(`src/index.ts:396`). A long-context 429 produces no token change, so it falls +through to the beta loop untouched. + +### Rejected: triggering rotation inside `http.ts` + +`fetchWithRetry` is shared with the OAuth refresh path +(`src/credentials.ts:233`). A 429 from the token endpoint must never trigger +account recovery. All recovery logic therefore stays at the `src/index.ts` call +sites. + +## Error handling + +| Failure | Behavior | +| --- | --- | +| Keychain locked/denied/timeout during re-read | Caught; fall back to in-memory credentials. Never propagates to the request path. | +| CAS mismatch on write-back | Skip the write, log, continue from memory. Next cache miss converges. | +| `forceRefreshActiveAccount` returns null | Recovery loop stops; the 401 surfaces with the raw body. | +| Borrowed credentials active | `forceRefreshActiveAccount` already declines (`src/credentials.ts:617`). | +| Both recovery attempts exhausted | Return the 401 unchanged, as today. | + +## Testing + +Follows existing convention: colocated `*.test.ts`, run via +`node --test --experimental-strip-types`. + +`src/credentials.test.ts` +- `refreshIfNeeded` re-reads a Keychain source and adopts an externally rotated token +- a throwing `refreshAccount` degrades to in-memory credentials rather than propagating +- write-back is skipped when the stored token no longer matches the expected prior token + +`src/keychain.test.ts` +- `writeBackCredentials` honors the expected-prior-token argument and is unchanged when omitted + +`src/index.test.ts` +- 401, external rotation present → retry succeeds, response is stream-transformed +- 401, source holds the rejected token → force refresh → retry succeeds +- 401, cold token after a switch → two-stage recovery succeeds +- 401, unrecoverable → single response returned raw, `preserveResponseUnchanged` semantics hold +- 429 with a changed token → retried once +- 429 with an unchanged token → not retried, falls through to the beta loop +- a 429 from the OAuth token endpoint does not trigger account recovery + +## Risks + +- **Keychain read volume.** Up to ~2 subprocesses/minute per session. Acceptable, + and the cache TTL is unchanged. +- **30s convergence window.** An external switch is invisible for up to one cache + TTL. Change 4 keeps that window from surfacing rate-limit errors; the residual + is up to 30s of usage landing on the previous account. Shortening the TTL would + cost a subprocess per request and is not worth it. +- **Behavior change to `preserveResponseUnchanged`.** Described in change 3. + Low blast radius, but it is a change and should be called out in the changelog. +- **Coupling to cswap internals.** None. This design reads and writes only the + active Claude Code credential, which is a Claude Code interface, not a cswap + one. From 354d9d9592c680e4c068488c75a037552cbf0c9a Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:12:27 -0600 Subject: [PATCH 02/33] docs: implementation plan for external credential rotation --- ...-29-external-credential-rotation-design.md | 30 +- ...07-29-external-credential-rotation-plan.md | 1179 +++++++++++++++++ 2 files changed, 1200 insertions(+), 9 deletions(-) create mode 100644 specs/2026-07-29-external-credential-rotation-plan.md diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md index ae1fdaf..92c349c 100644 --- a/specs/2026-07-29-external-credential-rotation-design.md +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -141,11 +141,21 @@ recovery attempts. Each attempt: The no-progress check is what bounds the loop; the attempt cap is defence in depth. -Two attempts are required, not one. After a switch to a slot whose access token -is cold, step 1 *does* return a different token, so the existing code retries, -401s again, and surfaces — `forceRefreshActiveAccount` never runs. The second -iteration re-reads, finds the token unchanged, and force-refreshes. One -structure covers external rotation, revoked tokens, and cold-slot switches. +Most cases resolve on the first attempt. A switch to a slot whose access token +is cold yields *null* from step 1, not a different token — +`reloadCredentialsFromSource` rejects anything expiring within 60s +(`src/credentials.ts:706-710`) — so step 2 runs immediately. The second attempt +exists for the narrower race where step 1 returns a token that is valid-looking +but has itself just been rotated again by a concurrent writer: the retry 401s, +and the next iteration finds the source unchanged and force-refreshes. + +One consequence to note: when step 1 returns null it does **not** update +`account.credentials`, so the force refresh runs against whatever account was +in memory. If a switch has landed, that is the previous account. The refresh +succeeds and the request recovers, but the write-back is correctly suppressed +by the compare-and-swap from change 2, since the store now holds a different +account. The session continues from memory and the next cache miss converges on +the switched-in account. `forceRefreshActiveAccount` already guards borrowed credentials (`src/credentials.ts:617`), writes back on success, and updates the cache, so it @@ -204,12 +214,14 @@ Follows existing convention: colocated `*.test.ts`, run via `src/index.test.ts` - 401, external rotation present → retry succeeds, response is stream-transformed -- 401, source holds the rejected token → force refresh → retry succeeds -- 401, cold token after a switch → two-stage recovery succeeds -- 401, unrecoverable → single response returned raw, `preserveResponseUnchanged` semantics hold +- 401, source yields no better token (unchanged, or null because it is expiring) + → force refresh on the first attempt → retry succeeds +- 401, first retry also rejected → second attempt force-refreshes → retry succeeds +- 401, unrecoverable → response returned raw, no stream transformation - 429 with a changed token → retried once - 429 with an unchanged token → not retried, falls through to the beta loop -- a 429 from the OAuth token endpoint does not trigger account recovery +- a 429 from the OAuth token endpoint surfaces as a failed refresh and does not + loop account recovery ## Risks diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md new file mode 100644 index 0000000..ef6d70a --- /dev/null +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -0,0 +1,1179 @@ +# External Credential Rotation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the plugin correct when something other than itself rotates the active Claude Code credential — cswap switching accounts, the `claude` CLI in another terminal, a second OpenCode instance, or a server-side revocation. + +**Architecture:** Four changes to existing files. Re-read every credential source (not just `file`) on cache miss; compare-and-swap on write-back so a refresh can never land in another account's slot; replace the single-shot 401 handler with a bounded recovery loop that wires up the already-written-but-never-called `forceRefreshActiveAccount()`; re-read once on a 429 so a resolved rate limit isn't surfaced. No cswap dependency, no subprocess, no new configuration. + +**Tech Stack:** TypeScript, Node's built-in test runner (`node --test --experimental-strip-types`), oxlint/oxfmt. Tests are colocated `*.test.ts` and load rewritten copies of `src/` modules from a temp dir with stubbed `keychain.ts` / `logger.ts` / `child-process.ts`. + +**Spec:** `specs/2026-07-29-external-credential-rotation-design.md` + +--- + +## File Structure + +| File | Responsibility | Change | +| --- | --- | --- | +| `src/credentials.ts` | Account state, caching, refresh orchestration | Re-read all sources; pass expected-prior-token to write-back | +| `src/keychain.ts` | OS credential store I/O | `writeBackCredentials` gains compare-and-swap | +| `src/index.ts` | Plugin entry, auth loader, fetch interception | 401 recovery loop; 429 re-read | +| `src/credentials.test.ts` | Harness + credential tests | Source-aware stub; new + repaired tests | +| `src/keychain.test.ts` | Keychain tests | CAS tests | +| `src/index.test.ts` | Plugin/fetch tests | Repaired 401 test; new recovery tests | +| `README.md`, `CHANGELOG.md` | Docs | Behavior note | + +## Known-breaking existing tests + +These pass today and **will fail** partway through. Each is repaired in the task that breaks it — do not "fix" them by reverting the implementation. + +| File:line | Assertion | Why it breaks | Repaired in | +| --- | --- | --- | --- | +| `src/credentials.test.ts:315` | `__getReadCount() === 1` | `getCachedCredentials` now also reads | Task 2 | +| `src/credentials.test.ts:437` | `__getReadCount() === 0` | first call now reads | Task 2 | +| `src/credentials.test.ts:735` | `__getReadCount() === readsBefore + 1` | now two reads | Task 2 | +| `src/index.test.ts:1189` | "does not retry a 401 when the source token is unchanged" | that is the behavior being changed | Task 4 | + +--- + +### Task 1: Make the credentials test harness source-aware + +The stub `refreshAccount(source)` ignores `source` and returns one global blob (`src/credentials.test.ts:148-153`). Harmless while only `file` sources were re-read; once every source is re-read, multi-account tests get one account's credentials for every source. This is a test-only change: the suite must stay green. + +**Files:** +- Modify: `src/credentials.test.ts:126-190` (the `tempKeychain` stub literal) + +- [ ] **Step 1: Add per-source overrides to the stub keychain** + +In the `writeFile(tempKeychain, ...)` template literal, replace the `refreshAccount` and `__setCredentials` definitions and add two exports. Keep `__setCredentials` working as the default so existing tests are untouched. + +```js +let bySource = {} + +export function refreshAccount(source) { + readCount += 1 + if (readError) throw new Error("Keychain read denied") + if (readHook) readHook() + if (Object.prototype.hasOwnProperty.call(bySource, source)) { + return bySource[source] + } + return credentials +} + +export function __setCredentials(c) { + credentials = c +} + +export function __setCredentialsForSource(source, c) { + bySource[source] = c +} + +export function __clearSourceOverrides() { + bySource = {} +} +``` + +- [ ] **Step 2: Record the expected-prior-token argument on write-back** + +Replace the stub `writeBackCredentials` so Task 3 can assert on it. Four parameters, matching the real signature after Task 3. + +```js +export function writeBackCredentials(source, creds, configDir, expectedPriorAccessToken) { + writeCount += 1 + writes.push({ source, creds, configDir, expectedPriorAccessToken }) + return true +} +``` + +- [ ] **Step 3: Widen the harness return types** + +In the `keychainModule` type annotation (both the declared return type at `src/credentials.test.ts:56-64` and the cast at `:226-235`), add the new members and widen `__getWrites`: + +```ts + __setCredentialsForSource: (source: string, c: Creds | null) => void + __clearSourceOverrides: () => void + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> +``` + +- [ ] **Step 4: Run the full suite to confirm nothing regressed** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 249`, `fail 0` + +- [ ] **Step 5: Commit** + +```bash +git add src/credentials.test.ts +git commit -m "test: make credentials keychain stub source-aware" +``` + +--- + +### Task 2: Re-read every credential source on cache miss + +**Files:** +- Modify: `src/credentials.ts:326-334` +- Test: `src/credentials.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Append inside the existing top-level `describe` block in `src/credentials.test.ts`: + +```ts + it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "before-switch", + refreshToken: "rt-before", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // An external process (cswap, the claude CLI, a second OpenCode) + // replaces the stored credential with a different account's. + keychainModule.__setCredentials({ + accessToken: "after-switch", + refreshToken: "rt-after", + expiresAt: now + 10 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "after-switch") + } finally { + Date.now = originalNow + } + }) + + it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory", + refreshToken: "rt", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + keychainModule.__setReadError(true) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "in-memory") + } finally { + Date.now = originalNow + } + }) +``` + +- [ ] **Step 2: Run the new tests to verify they fail** + +Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -12` +Expected: FAIL — the first asserts `'before-switch' !== 'after-switch'`; the second throws `Keychain read denied`. + +- [ ] **Step 3: Implement the re-read** + +In `src/credentials.ts`, replace lines 326-334 (the comment block and the `if (target.source === "file")` guard) with: + +```ts + // Pick up credentials replaced externally — cswap switching accounts, the + // claude CLI in another terminal, or a second OpenCode instance. This used + // to be limited to file sources on the assumption that a keychain entry is + // only ever mutated by our own writeBackCredentials; that assumption is + // false. Bounded by getCachedCredentials's 30s TTL, so it fires at most + // ~2x/min under load. + // + // A keychain read shells out to `security`, which throws when the keychain + // is locked, access is denied, or the call times out. Degrade to the + // in-memory credentials rather than take down the request path. + try { + const stored = refreshAccount(target.source, target.configDir) + if (stored) target.credentials = stored + } catch (err) { + log("source_reread_failed", { + source: target.source, + error: err instanceof Error ? err.message : String(err), + }) + } +``` + +- [ ] **Step 4: Run the new tests to verify they pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -8` +Expected: `pass 2`, `fail 0` + +- [ ] **Step 5: Repair the three read-count assertions** + +Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` +Expected: three failures, at `src/credentials.test.ts` lines ~315, ~437, ~735. + +`src/credentials.test.ts:315` — one extra read now happens inside `getCachedCredentials` before `__setReadError(true)`: + +```ts + assert.equal(keychainModule.__getReadCount(), 2) +``` + +`src/credentials.test.ts:437` — the first `getCachedCredentials()` is a cache miss and now re-reads; the second is a cache hit and does not: + +```ts + assert.equal( + keychainModule.__getReadCount(), + 1, + "cache miss re-reads the source once; the cached call does not", + ) +``` + +`src/credentials.test.ts:729-735` (test: `fallback uses a valid in-memory account without a keychain read`) — `refreshIfNeeded` now re-reads the target's source up front, in addition to the existing post-OAuth-failure re-read. Pin the expired account's own stored value so the up-front read is a no-op rather than swapping in the other account's blob. + +Immediately after that test's `credentialsModule.initAccounts([...])` call, add: + +```ts + keychainModule.__setCredentialsForSource( + "Claude Code-credentials-aabbccdd", + { + accessToken: "stale-suffixed", + refreshToken: "rt-suffixed", + expiresAt: now - 1_000, + }, + ) +``` + +Then update the assertion: + +```ts + assert.equal( + keychainModule.__getReadCount(), + readsBefore + 2, + "one up-front re-read of the target's own source, one after the failed OAuth refresh", + ) +``` + +- [ ] **Step 6: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 251`, `fail 0` + +- [ ] **Step 7: Commit** + +```bash +git add src/credentials.ts src/credentials.test.ts +git commit -m "fix: re-read keychain sources so external credential rotation is picked up" +``` + +--- + +### Task 3: Compare-and-swap on write-back + +Closes the window between reading a credential and writing its refreshed replacement — seconds wide, because an OAuth round-trip sits in the middle. + +**Files:** +- Modify: `src/keychain.ts:442-503` +- Modify: `src/credentials.ts:376`, `:499`, `:622-628` +- Test: `src/keychain.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Add `credentialBlobMatches` to the existing import from `./keychain.ts` at `src/keychain.test.ts:15-23`. + +Add a new top-level `describe` (place it directly above `describe("writeBackCredentials (file source)"` at line 585): + +```ts +describe("credentialBlobMatches", () => { + const blob = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: "rt-a", + expiresAt: 1, + }, + }) + + it("accepts a blob still holding the expected token", () => { + assert.equal(credentialBlobMatches(blob, "account-a"), true) + }) + + it("rejects a blob replaced by another account", () => { + assert.equal(credentialBlobMatches(blob, "account-b"), false) + }) + + it("rejects an unparseable blob rather than assuming a match", () => { + assert.equal(credentialBlobMatches("not json", "account-a"), false) + }) +}) +``` + +Then add this end-to-end test **inside** the existing `describe("writeBackCredentials (file source)")` block, after the test at line 599. It mirrors that test's `HOME` isolation pattern: + +```ts + it("skips the write when the stored token is no longer the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + // Another process switched accounts after we read "expected-at". + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "switched-in-at", + refreshToken: "switched-in-rt", + expiresAt: 1000, + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + }, + undefined, + "expected-at", + ) + + assert.equal(result, false) + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal( + written.claudeAiOauth.accessToken, + "switched-in-at", + "the switched-in credential must survive untouched", + ) + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) +``` + +- [ ] **Step 2: Run to verify they fail** + +Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -12` +Expected: FAIL — `credentialBlobMatches is not a function`, and the skip test writes anyway (`result` is `true`). + +- [ ] **Step 3: Implement the guard** + +In `src/keychain.ts`, add above `writeBackCredentials` (line 442): + +```ts +/** + * Whether a stored credential blob still carries the access token we expect. + * + * Guards write-back against an external switch landing between the read that + * produced the token being refreshed and the write of its replacement. An + * unparseable blob returns false: a write into state we cannot identify is + * exactly what this is meant to prevent. + */ +export function credentialBlobMatches( + raw: string, + expectedAccessToken: string, +): boolean { + const parsed = parseCredentials(raw) + return parsed?.accessToken === expectedAccessToken +} +``` + +Change the signature at line 442 to: + +```ts +export function writeBackCredentials( + source: string, + creds: ClaudeCredentials, + configDir?: string, + expectedPriorAccessToken?: string, +): boolean { +``` + +In the `source === "file"` branch, immediately after `const raw = readFileSync(credPath, "utf-8")`: + +```ts + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + log("writeback_skipped_stale", { source, configDir: dir }) + return false + } +``` + +In the `process.platform === "darwin"` branch, immediately after `if (!raw) return false`: + +```ts + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + log("writeback_skipped_stale", { source }) + return false + } +``` + +- [ ] **Step 4: Run to verify they pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -8` +Expected: `pass 4`, `fail 0` + +- [ ] **Step 5: Pass the expected token from all three refresh call sites** + +`src/credentials.ts:376` — `creds` is the pre-refresh credential captured by `performRefresh`: + +```ts + writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, + ) +``` + +`src/credentials.ts:499` — inside `refreshBorrowedAccount`, `own` is this account's own stored credential: + +```ts + writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + own.accessToken, + ) +``` + +`src/credentials.ts:606-628` — in `forceRefreshActiveAccount`, capture the token **before** the await, because `account.credentials` is reassigned on success: + +```ts + const priorAccessToken = account.credentials.accessToken + const oauthCreds = await refresh(account.credentials.refreshToken) + if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { + account.credentials = oauthCreds + if ( + !writeBackCredentials( + account.source, + oauthCreds, + account.configDir, + priorAccessToken, + ) + ) { +``` + +- [ ] **Step 6: Add the write-back assertion test** + +Append inside the top-level `describe` in `src/credentials.test.ts`: + +```ts + it("performRefresh passes the pre-refresh token as the write-back guard", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + await credentialsModule.refreshIfNeeded() + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].creds.accessToken, "rotated-token") + assert.equal(writes[0].expectedPriorAccessToken, "stale-token") + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } + }) +``` + +- [ ] **Step 7: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 256`, `fail 0` + +- [ ] **Step 8: Commit** + +```bash +git add src/keychain.ts src/keychain.test.ts src/credentials.ts src/credentials.test.ts +git commit -m "fix: guard credential write-back against an external switch" +``` + +--- + +### Task 4: Bounded 401 recovery loop + +**Files:** +- Modify: `src/index.ts:368-392` (401 block), `:462-464` (return), imports at `:20-31` +- Test: `src/index.test.ts:1189-1251` + +- [ ] **Step 1: Import the currently-dead force-refresh helper** + +In the `from "./credentials.ts"` import block in `src/index.ts` (around line 20-31), add: + +```ts + forceRefreshActiveAccount, +``` + +- [ ] **Step 2: Replace the 401 block with the recovery loop** + +Replace `src/index.ts:368-392` in full: + +```ts + // Recover from a rejected token: first by adopting credentials + // rotated externally (cswap switching accounts, the claude CLI, + // another OpenCode instance), then by forcing an OAuth refresh + // when the store still holds the token that was just rejected. + // + // Most cases resolve on the first attempt: a cold or unreadable + // store yields null from the reload (reloadCredentialsFromSource + // rejects anything expiring within 60s), so the force refresh runs + // immediately. The second attempt covers the narrower race where + // the reload returns a valid-looking token that a concurrent + // writer has itself just rotated again. The no-progress break is + // what actually bounds this — the cap is defence in depth. + const MAX_AUTH_RECOVERY_ATTEMPTS = 2 + let tokenInUse = latest.accessToken + + for ( + let attempt = 0; + response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; + attempt++ + ) { + let candidate: ClaudeCredentials | null = null + try { + candidate = reloadCredentialsFromSource() + } catch {} + + if (!candidate || candidate.accessToken === tokenInUse) { + try { + candidate = await forceRefreshActiveAccount() + } catch {} + } + + if (!candidate || candidate.accessToken === tokenInUse) { + log("auth_recovery_exhausted", { + modelId, + attempt: attempt + 1, + }) + break + } + + tokenInUse = candidate.accessToken + log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + } +``` + +- [ ] **Step 3: Derive the stream-transform decision from the final status** + +Replace `src/index.ts:462-464`: + +```ts + // A 401 that survived recovery carries an error body, not an SSE + // stream. Deciding here rather than from a flag set mid-flight + // makes the retried and non-retried paths behave identically. + return response.status === 401 + ? response + : transformResponseStream(response) +``` + +- [ ] **Step 4: Run the suite to see the expected failure** + +Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` +Expected: exactly one failure — `auth fetch does not retry a 401 when the source token is unchanged` (`src/index.test.ts:1189`). Its name describes the behavior being replaced. + +The neighbouring test around `src/index.test.ts:1141` (a 401 recovered by an externally rotated token) and the one at `:1253` (reload throws) must both still pass — the loop preserves those paths. If either fails, the loop is wrong, not the test. + +- [ ] **Step 5: Replace that test with the two behaviors that supersede it** + +Replace the whole `it(...)` block at `src/index.test.ts:1189-1251` with: + +```ts + it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "recovered-token", + refresh_token: "rt-recovered", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + return apiCalls === 1 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") + assert.equal(apiCalls, 2, "the refreshed token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + const errorBody = '{"name":"mcp_UnchangedToken"}' + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + // A 429 from the token endpoint must read as a failed refresh, not + // as something that loops account recovery. retry-after beyond the + // 30s cap makes fetchWithRetry return instead of backing off. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + apiCalls += 1 + return new Response(errorBody, { + status: 401, + headers: { "x-request-id": "unchanged-token-401" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 401) + assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") + assert.equal(await response.text(), errorBody) + assert.equal(apiCalls, 1, "no retry without a different token") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) +``` + +- [ ] **Step 6: Add the test that justifies a second attempt** + +This is the only scenario where one attempt is insufficient: the reload returns a valid-looking token that a concurrent writer has itself just rotated again, so the first retry is also rejected. Append after the two tests from Step 5: + +```ts + it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "second-stage-token", + refresh_token: "rt-second-stage", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + // Calls 1 and 2 are rejected; only the force-refreshed token works. + return apiCalls <= 2 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Attempt 1's reload finds a different, still-valid-looking token. + keychainModule.__setCredentials({ + accessToken: "concurrently-rotated", + refreshToken: "rt-concurrent", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") + assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) +``` + +- [ ] **Step 7: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 258`, `fail 0` + +- [ ] **Step 8: Commit** + +```bash +git add src/index.ts src/index.test.ts +git commit -m "fix: recover from a rejected token by forcing an OAuth refresh" +``` + +--- + +### Task 5: Re-read the source on a 429 + +**Files:** +- Modify: `src/index.ts` — insert after the 401 recovery loop, before the long-context beta loop (`:396`) +- Test: `src/index.test.ts` + +- [ ] **Step 1: Write the failing test** + +Append inside the same `describe` as Task 4's tests: + +```ts + it("auth fetch retries a 429 once when the source token changed", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + if (apiCalls === 1) { + // retry-after beyond the 30s cap: quota exhaustion, not a + // transient limit, so fetchWithRetry returns immediately. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + return new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // An external switch lands while this session is still on the + // exhausted account. + keychainModule.__setCredentials({ + accessToken: "switched-token", + refreshToken: "rt-switched", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 2, "the rotated token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch does not retry a 429 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unchanged token must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) +``` + +`loadHelpersWithCountingKeychain` already returns `keychainModule` with `__setCredentials` (`src/index.test.ts:264-273`), so no harness change is needed. + +- [ ] **Step 2: Run to verify the first fails and the second passes** + +Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -12` +Expected: the "token changed" test FAILS with `apiCalls` 1 ≠ 2; the "unchanged" test passes (no retry exists yet). + +- [ ] **Step 3: Implement the 429 re-read** + +Insert in `src/index.ts` directly after the 401 recovery loop from Task 4 and before the long-context beta loop: + +```ts + // An external switch — cswap rotating off an exhausted account — + // leaves this session on the old token until the 30s credential + // cache expires. Re-read once so a rate limit that has already + // been resolved elsewhere is not surfaced. A changed token is the + // signal that a switch happened; when nothing changed this costs + // one source read and no retry. + if (response.status === 429) { + let rotated: ClaudeCredentials | null = null + try { + rotated = reloadCredentialsFromSource() + } catch {} + + if (rotated && rotated.accessToken !== tokenInUse) { + log("rate_limit_credentials_rotated", { modelId }) + tokenInUse = rotated.accessToken + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + } + } +``` + +- [ ] **Step 4: Run to verify both pass** + +Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -8` +Expected: `fail 0` + +- [ ] **Step 5: Run the full suite** + +Run: `pnpm test 2>&1 | tail -8` +Expected: `pass 260`, `fail 0` + +- [ ] **Step 6: Commit** + +```bash +git add src/index.ts src/index.test.ts +git commit -m "fix: re-read credentials on a 429 so a resolved rate limit is not surfaced" +``` + +--- + +### Task 6: Lint, format, and document + +**Files:** +- Modify: `README.md`, `CHANGELOG.md` + +- [ ] **Step 1: Lint and format** + +Run: `pnpm run lint:fix && pnpm run lint` +Expected: clean exit, no diagnostics. + +- [ ] **Step 2: Add the behavior to the README technical list** + +In `README.md`, under `## How it works (technical)`, insert after the bullet beginning "Syncs credentials to `auth.json`": + +```markdown +- Re-reads the credential source on each cache miss (~every 30s under load), so an account switched externally — by [claude-swap](https://github.com/realiti4/claude-swap), the `claude` CLI in another terminal, or a second OpenCode instance — is picked up in a live session without a restart +- Guards credential write-back with the token it refreshed from, so a switch landing mid-refresh can never write one account's tokens into another's slot +- On a rejected token, adopts an externally rotated credential and otherwise forces an OAuth refresh, retrying the request in place rather than surfacing the 401 +``` + +- [ ] **Step 3: Add a CHANGELOG entry** + +Add under the topmost unreleased heading in `CHANGELOG.md` (create `## Unreleased` above the newest release heading if absent): + +```markdown +### Bug Fixes + +- Pick up credentials rotated by another process (claude-swap, the `claude` CLI, a second OpenCode instance) in a live session instead of staying pinned to the account that was active at startup +- Never write a refreshed token into a slot that was switched underneath us +- Force an OAuth refresh when a request is rejected and the store still holds the rejected token, so the request recovers in place + +### Behavior Changes + +- A 401 that survives recovery is now returned without SSE stream transformation, matching what a non-retried 401 already did +``` + +- [ ] **Step 4: Verify the full suite and a clean tree** + +Run: `pnpm test 2>&1 | tail -8 && git status --short` +Expected: `fail 0`; only `README.md` and `CHANGELOG.md` modified. + +- [ ] **Step 5: Commit** + +```bash +git add README.md CHANGELOG.md +git commit -m "docs: document external credential rotation handling" +``` + +--- + +## Manual verification + +Automated tests stub the Keychain. Verify against real cswap state once: + +- [ ] Confirm at least two cswap accounts and note the active one: `cswap list` +- [ ] Start OpenCode and send a prompt so credentials are cached +- [ ] Switch in another terminal: `cswap switch` +- [ ] Wait 30s, send another prompt in the **same** OpenCode session +- [ ] With `CLAUDE_AUTH_DEBUG=1`, confirm `~/.local/share/opencode/claude-auth-debug.log` shows the new account's token and no `writeback_skipped_stale` storm +- [ ] Confirm `cswap list` still shows both accounts healthy — no slot quarantined for a dead refresh token From 39944d26a1762cce783e0cdbffe05c39e21c0915 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:16:36 -0600 Subject: [PATCH 03/33] test: make credentials keychain stub source-aware --- src/credentials.test.ts | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index d8f318e..d53c45a 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -57,10 +57,17 @@ async function loadCredentialsWithCountingKeychain( __getReadCount: () => number __getWriteCount: () => number __setCredentials: (c: Creds | null) => void + __setCredentialsForSource: (source: string, c: Creds | null) => void + __clearSourceOverrides: () => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void - __getWrites: () => Array<{ source: string; creds: Creds }> + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> } childProcessModule: { __getExecFileSyncCount: () => number @@ -136,6 +143,7 @@ let credentials = { refreshToken: "refresh", expiresAt: ${initialExpiresAt} } +let bySource = {} export const PRIMARY_SERVICE = "Claude Code-credentials" @@ -149,6 +157,9 @@ export function refreshAccount(source) { readCount += 1 if (readError) throw new Error("Keychain read denied") if (readHook) readHook() + if (Object.prototype.hasOwnProperty.call(bySource, source)) { + return bySource[source] + } return credentials } @@ -160,9 +171,9 @@ export function __setReadHook(hook) { readHook = hook } -export function writeBackCredentials(source, creds) { +export function writeBackCredentials(source, creds, configDir, expectedPriorAccessToken) { writeCount += 1 - writes.push({ source, creds }) + writes.push({ source, creds, configDir, expectedPriorAccessToken }) return true } @@ -182,6 +193,14 @@ export function __setCredentials(c) { credentials = c } +export function __setCredentialsForSource(source, c) { + bySource[source] = c +} + +export function __clearSourceOverrides() { + bySource = {} +} + export function __setAccounts(list) { accounts = list } @@ -228,10 +247,17 @@ export function __setAccounts(list) { __getReadCount: () => number __getWriteCount: () => number __setCredentials: (c: Creds | null) => void + __setCredentialsForSource: (source: string, c: Creds | null) => void + __clearSourceOverrides: () => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void - __getWrites: () => Array<{ source: string; creds: Creds }> + __getWrites: () => Array<{ + source: string + creds: Creds + configDir?: string + expectedPriorAccessToken?: string + }> }, childProcessModule: childProcessModule as { __getExecFileSyncCount: () => number From f6decf6cd717044f0947266ec043badb71bd0f1f Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:17:44 -0600 Subject: [PATCH 04/33] docs: drop unused reset helper from harness task --- specs/2026-07-29-external-credential-rotation-plan.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index ef6d70a..d2e0226 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -68,12 +68,10 @@ export function __setCredentials(c) { export function __setCredentialsForSource(source, c) { bySource[source] = c } - -export function __clearSourceOverrides() { - bySource = {} -} ``` +Each test calls `loadCredentialsWithCountingKeychain` fresh, which imports a new module instance with an empty `bySource`, so no reset helper is needed. + - [ ] **Step 2: Record the expected-prior-token argument on write-back** Replace the stub `writeBackCredentials` so Task 3 can assert on it. Four parameters, matching the real signature after Task 3. @@ -92,7 +90,6 @@ In the `keychainModule` type annotation (both the declared return type at `src/c ```ts __setCredentialsForSource: (source: string, c: Creds | null) => void - __clearSourceOverrides: () => void __getWrites: () => Array<{ source: string creds: Creds From d1e6f978c6847dff9c883b6f8fa29448b0363287 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:20:27 -0600 Subject: [PATCH 05/33] test: drop unused __clearSourceOverrides from keychain stub --- src/credentials.test.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index d53c45a..a718ed4 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -58,7 +58,6 @@ async function loadCredentialsWithCountingKeychain( __getWriteCount: () => number __setCredentials: (c: Creds | null) => void __setCredentialsForSource: (source: string, c: Creds | null) => void - __clearSourceOverrides: () => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void @@ -143,7 +142,7 @@ let credentials = { refreshToken: "refresh", expiresAt: ${initialExpiresAt} } -let bySource = {} +const bySource = {} export const PRIMARY_SERVICE = "Claude Code-credentials" @@ -197,10 +196,6 @@ export function __setCredentialsForSource(source, c) { bySource[source] = c } -export function __clearSourceOverrides() { - bySource = {} -} - export function __setAccounts(list) { accounts = list } @@ -248,7 +243,6 @@ export function __setAccounts(list) { __getWriteCount: () => number __setCredentials: (c: Creds | null) => void __setCredentialsForSource: (source: string, c: Creds | null) => void - __clearSourceOverrides: () => void __setAccounts: (list: unknown[]) => void __setReadError: (enabled: boolean) => void __setReadHook: (hook: (() => void) | null) => void From a1a1e14e1f1bc936874b8763b9d40ecaf55f05fe Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:20:59 -0600 Subject: [PATCH 06/33] docs: format spec files with oxfmt --- ...-29-external-credential-rotation-design.md | 23 +- ...07-29-external-credential-rotation-plan.md | 1314 +++++++++-------- 2 files changed, 673 insertions(+), 664 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md index 92c349c..4fca333 100644 --- a/specs/2026-07-29-external-credential-rotation-design.md +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -49,8 +49,8 @@ asymmetry is why the design targets the active credential only. 2. **Stale-snapshot write-back.** The proactive refresh tick (`src/index.ts:224`, every 5 min, acting within 1h of expiry) refreshes from an in-memory snapshot and calls `writeBackCredentials`. If the active credential was replaced - externally in the meantime, the plugin writes the *stale* account's rotated - tokens over the *current* account's slot. Because refresh tokens rotate on + externally in the meantime, the plugin writes the _stale_ account's rotated + tokens over the _current_ account's slot. Because refresh tokens rotate on use, the losing copy is dead. 3. **401 recovery gives up too early.** `src/index.ts:371-392` re-reads the @@ -99,7 +99,7 @@ miss, so at most about twice a minute under load (`CREDENTIAL_CACHE_TTL_MS`, `src/credentials.ts:26`). A `security find-generic-password` call is ~10-50ms. This resolves problems 1 and 2 together: the plugin refreshes and writes back -whatever is *currently* active rather than a stale snapshot. +whatever is _currently_ active rather than a stale snapshot. **New failure path.** `readKeychainService` throws on a locked, denied, or timed out Keychain (`src/keychain.ts:96-142`), whereas the file path swallows errors @@ -142,7 +142,7 @@ The no-progress check is what bounds the loop; the attempt cap is defence in depth. Most cases resolve on the first attempt. A switch to a slot whose access token -is cold yields *null* from step 1, not a different token — +is cold yields _null_ from step 1, not a different token — `reloadCredentialsFromSource` rejects anything expiring within 60s (`src/credentials.ts:706-710`) — so step 2 runs immediately. The second attempt exists for the narrower race where step 1 returns a token that is valid-looking @@ -191,13 +191,13 @@ sites. ## Error handling -| Failure | Behavior | -| --- | --- | +| Failure | Behavior | +| --------------------------------------------- | --------------------------------------------------------------------------------- | | Keychain locked/denied/timeout during re-read | Caught; fall back to in-memory credentials. Never propagates to the request path. | -| CAS mismatch on write-back | Skip the write, log, continue from memory. Next cache miss converges. | -| `forceRefreshActiveAccount` returns null | Recovery loop stops; the 401 surfaces with the raw body. | -| Borrowed credentials active | `forceRefreshActiveAccount` already declines (`src/credentials.ts:617`). | -| Both recovery attempts exhausted | Return the 401 unchanged, as today. | +| CAS mismatch on write-back | Skip the write, log, continue from memory. Next cache miss converges. | +| `forceRefreshActiveAccount` returns null | Recovery loop stops; the 401 surfaces with the raw body. | +| Borrowed credentials active | `forceRefreshActiveAccount` already declines (`src/credentials.ts:617`). | +| Both recovery attempts exhausted | Return the 401 unchanged, as today. | ## Testing @@ -205,14 +205,17 @@ Follows existing convention: colocated `*.test.ts`, run via `node --test --experimental-strip-types`. `src/credentials.test.ts` + - `refreshIfNeeded` re-reads a Keychain source and adopts an externally rotated token - a throwing `refreshAccount` degrades to in-memory credentials rather than propagating - write-back is skipped when the stored token no longer matches the expected prior token `src/keychain.test.ts` + - `writeBackCredentials` honors the expected-prior-token argument and is unchanged when omitted `src/index.test.ts` + - 401, external rotation present → retry succeeds, response is stream-transformed - 401, source yields no better token (unchanged, or null because it is expiring) → force refresh on the first attempt → retry succeeds diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index d2e0226..186fe1a 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -14,26 +14,26 @@ ## File Structure -| File | Responsibility | Change | -| --- | --- | --- | -| `src/credentials.ts` | Account state, caching, refresh orchestration | Re-read all sources; pass expected-prior-token to write-back | -| `src/keychain.ts` | OS credential store I/O | `writeBackCredentials` gains compare-and-swap | -| `src/index.ts` | Plugin entry, auth loader, fetch interception | 401 recovery loop; 429 re-read | -| `src/credentials.test.ts` | Harness + credential tests | Source-aware stub; new + repaired tests | -| `src/keychain.test.ts` | Keychain tests | CAS tests | -| `src/index.test.ts` | Plugin/fetch tests | Repaired 401 test; new recovery tests | -| `README.md`, `CHANGELOG.md` | Docs | Behavior note | +| File | Responsibility | Change | +| --------------------------- | --------------------------------------------- | ------------------------------------------------------------ | +| `src/credentials.ts` | Account state, caching, refresh orchestration | Re-read all sources; pass expected-prior-token to write-back | +| `src/keychain.ts` | OS credential store I/O | `writeBackCredentials` gains compare-and-swap | +| `src/index.ts` | Plugin entry, auth loader, fetch interception | 401 recovery loop; 429 re-read | +| `src/credentials.test.ts` | Harness + credential tests | Source-aware stub; new + repaired tests | +| `src/keychain.test.ts` | Keychain tests | CAS tests | +| `src/index.test.ts` | Plugin/fetch tests | Repaired 401 test; new recovery tests | +| `README.md`, `CHANGELOG.md` | Docs | Behavior note | ## Known-breaking existing tests These pass today and **will fail** partway through. Each is repaired in the task that breaks it — do not "fix" them by reverting the implementation. -| File:line | Assertion | Why it breaks | Repaired in | -| --- | --- | --- | --- | -| `src/credentials.test.ts:315` | `__getReadCount() === 1` | `getCachedCredentials` now also reads | Task 2 | -| `src/credentials.test.ts:437` | `__getReadCount() === 0` | first call now reads | Task 2 | -| `src/credentials.test.ts:735` | `__getReadCount() === readsBefore + 1` | now two reads | Task 2 | -| `src/index.test.ts:1189` | "does not retry a 401 when the source token is unchanged" | that is the behavior being changed | Task 4 | +| File:line | Assertion | Why it breaks | Repaired in | +| ----------------------------- | --------------------------------------------------------- | ------------------------------------- | ----------- | +| `src/credentials.test.ts:315` | `__getReadCount() === 1` | `getCachedCredentials` now also reads | Task 2 | +| `src/credentials.test.ts:437` | `__getReadCount() === 0` | first call now reads | Task 2 | +| `src/credentials.test.ts:735` | `__getReadCount() === readsBefore + 1` | now two reads | Task 2 | +| `src/index.test.ts:1189` | "does not retry a 401 when the source token is unchanged" | that is the behavior being changed | Task 4 | --- @@ -42,6 +42,7 @@ These pass today and **will fail** partway through. Each is repaired in the task The stub `refreshAccount(source)` ignores `source` and returns one global blob (`src/credentials.test.ts:148-153`). Harmless while only `file` sources were re-read; once every source is re-read, multi-account tests get one account's credentials for every source. This is a test-only change: the suite must stay green. **Files:** + - Modify: `src/credentials.test.ts:126-190` (the `tempKeychain` stub literal) - [ ] **Step 1: Add per-source overrides to the stub keychain** @@ -77,7 +78,12 @@ Each test calls `loadCredentialsWithCountingKeychain` fresh, which imports a new Replace the stub `writeBackCredentials` so Task 3 can assert on it. Four parameters, matching the real signature after Task 3. ```js -export function writeBackCredentials(source, creds, configDir, expectedPriorAccessToken) { +export function writeBackCredentials( + source, + creds, + configDir, + expectedPriorAccessToken, +) { writeCount += 1 writes.push({ source, creds, configDir, expectedPriorAccessToken }) return true @@ -115,6 +121,7 @@ git commit -m "test: make credentials keychain stub source-aware" ### Task 2: Re-read every credential source on cache miss **Files:** + - Modify: `src/credentials.ts:326-334` - Test: `src/credentials.test.ts` @@ -123,73 +130,73 @@ git commit -m "test: make credentials keychain stub source-aware" Append inside the existing top-level `describe` block in `src/credentials.test.ts`: ```ts - it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { - const originalNow = Date.now - const now = 1_700_000_000_000 - Date.now = () => now +it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now - try { - const { credentialsModule, keychainModule } = - await loadCredentialsWithCountingKeychain(now + 10 * 60_000) - - credentialsModule.initAccounts([ - { - label: "Account 1", - source: "keychain", - credentials: { - accessToken: "before-switch", - refreshToken: "rt-before", - expiresAt: now + 10 * 60_000, - }, + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "before-switch", + refreshToken: "rt-before", + expiresAt: now + 10 * 60_000, }, - ]) - - // An external process (cswap, the claude CLI, a second OpenCode) - // replaces the stored credential with a different account's. - keychainModule.__setCredentials({ - accessToken: "after-switch", - refreshToken: "rt-after", - expiresAt: now + 10 * 60_000, - }) + }, + ]) + + // An external process (cswap, the claude CLI, a second OpenCode) + // replaces the stored credential with a different account's. + keychainModule.__setCredentials({ + accessToken: "after-switch", + refreshToken: "rt-after", + expiresAt: now + 10 * 60_000, + }) - const result = await credentialsModule.refreshIfNeeded() + const result = await credentialsModule.refreshIfNeeded() - assert.equal(result?.accessToken, "after-switch") - } finally { - Date.now = originalNow - } - }) + assert.equal(result?.accessToken, "after-switch") + } finally { + Date.now = originalNow + } +}) - it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { - const originalNow = Date.now - const now = 1_700_000_000_000 - Date.now = () => now +it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now - try { - const { credentialsModule, keychainModule } = - await loadCredentialsWithCountingKeychain(now + 10 * 60_000) - - credentialsModule.initAccounts([ - { - label: "Account 1", - source: "keychain", - credentials: { - accessToken: "in-memory", - refreshToken: "rt", - expiresAt: now + 10 * 60_000, - }, + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory", + refreshToken: "rt", + expiresAt: now + 10 * 60_000, }, - ]) + }, + ]) - keychainModule.__setReadError(true) + keychainModule.__setReadError(true) - const result = await credentialsModule.refreshIfNeeded() + const result = await credentialsModule.refreshIfNeeded() - assert.equal(result?.accessToken, "in-memory") - } finally { - Date.now = originalNow - } - }) + assert.equal(result?.accessToken, "in-memory") + } finally { + Date.now = originalNow + } +}) ``` - [ ] **Step 2: Run the new tests to verify they fail** @@ -202,25 +209,25 @@ Expected: FAIL — the first asserts `'before-switch' !== 'after-switch'`; the s In `src/credentials.ts`, replace lines 326-334 (the comment block and the `if (target.source === "file")` guard) with: ```ts - // Pick up credentials replaced externally — cswap switching accounts, the - // claude CLI in another terminal, or a second OpenCode instance. This used - // to be limited to file sources on the assumption that a keychain entry is - // only ever mutated by our own writeBackCredentials; that assumption is - // false. Bounded by getCachedCredentials's 30s TTL, so it fires at most - // ~2x/min under load. - // - // A keychain read shells out to `security`, which throws when the keychain - // is locked, access is denied, or the call times out. Degrade to the - // in-memory credentials rather than take down the request path. - try { - const stored = refreshAccount(target.source, target.configDir) - if (stored) target.credentials = stored - } catch (err) { - log("source_reread_failed", { - source: target.source, - error: err instanceof Error ? err.message : String(err), - }) - } +// Pick up credentials replaced externally — cswap switching accounts, the +// claude CLI in another terminal, or a second OpenCode instance. This used +// to be limited to file sources on the assumption that a keychain entry is +// only ever mutated by our own writeBackCredentials; that assumption is +// false. Bounded by getCachedCredentials's 30s TTL, so it fires at most +// ~2x/min under load. +// +// A keychain read shells out to `security`, which throws when the keychain +// is locked, access is denied, or the call times out. Degrade to the +// in-memory credentials rather than take down the request path. +try { + const stored = refreshAccount(target.source, target.configDir) + if (stored) target.credentials = stored +} catch (err) { + log("source_reread_failed", { + source: target.source, + error: err instanceof Error ? err.message : String(err), + }) +} ``` - [ ] **Step 4: Run the new tests to verify they pass** @@ -236,17 +243,17 @@ Expected: three failures, at `src/credentials.test.ts` lines ~315, ~437, ~735. `src/credentials.test.ts:315` — one extra read now happens inside `getCachedCredentials` before `__setReadError(true)`: ```ts - assert.equal(keychainModule.__getReadCount(), 2) +assert.equal(keychainModule.__getReadCount(), 2) ``` `src/credentials.test.ts:437` — the first `getCachedCredentials()` is a cache miss and now re-reads; the second is a cache hit and does not: ```ts - assert.equal( - keychainModule.__getReadCount(), - 1, - "cache miss re-reads the source once; the cached call does not", - ) +assert.equal( + keychainModule.__getReadCount(), + 1, + "cache miss re-reads the source once; the cached call does not", +) ``` `src/credentials.test.ts:729-735` (test: `fallback uses a valid in-memory account without a keychain read`) — `refreshIfNeeded` now re-reads the target's source up front, in addition to the existing post-OAuth-failure re-read. Pin the expired account's own stored value so the up-front read is a no-op rather than swapping in the other account's blob. @@ -254,24 +261,21 @@ Expected: three failures, at `src/credentials.test.ts` lines ~315, ~437, ~735. Immediately after that test's `credentialsModule.initAccounts([...])` call, add: ```ts - keychainModule.__setCredentialsForSource( - "Claude Code-credentials-aabbccdd", - { - accessToken: "stale-suffixed", - refreshToken: "rt-suffixed", - expiresAt: now - 1_000, - }, - ) +keychainModule.__setCredentialsForSource("Claude Code-credentials-aabbccdd", { + accessToken: "stale-suffixed", + refreshToken: "rt-suffixed", + expiresAt: now - 1_000, +}) ``` Then update the assertion: ```ts - assert.equal( - keychainModule.__getReadCount(), - readsBefore + 2, - "one up-front re-read of the target's own source, one after the failed OAuth refresh", - ) +assert.equal( + keychainModule.__getReadCount(), + readsBefore + 2, + "one up-front re-read of the target's own source, one after the failed OAuth refresh", +) ``` - [ ] **Step 6: Run the full suite** @@ -293,6 +297,7 @@ git commit -m "fix: re-read keychain sources so external credential rotation is Closes the window between reading a credential and writing its refreshed replacement — seconds wide, because an OAuth round-trip sits in the middle. **Files:** + - Modify: `src/keychain.ts:442-503` - Modify: `src/credentials.ts:376`, `:499`, `:622-628` - Test: `src/keychain.test.ts` @@ -330,54 +335,54 @@ describe("credentialBlobMatches", () => { Then add this end-to-end test **inside** the existing `describe("writeBackCredentials (file source)")` block, after the test at line 599. It mirrors that test's `HOME` isolation pattern: ```ts - it("skips the write when the stored token is no longer the expected one", async () => { - const originalHome = process.env.HOME - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) - process.env.HOME = tempHome - - try { - const claudeDir = join(tempHome, ".claude") - mkdirSync(claudeDir, { recursive: true }) - const credPath = join(claudeDir, ".credentials.json") - // Another process switched accounts after we read "expected-at". - writeFileSync( - credPath, - JSON.stringify({ - claudeAiOauth: { - accessToken: "switched-in-at", - refreshToken: "switched-in-rt", - expiresAt: 1000, - }, - }), - { encoding: "utf-8", mode: 0o600 }, - ) +it("skips the write when the stored token is no longer the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome - const result = writeBackCredentials( - "file", - { - accessToken: "our-refreshed-at", - refreshToken: "our-refreshed-rt", - expiresAt: 2000, + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + // Another process switched accounts after we read "expected-at". + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "switched-in-at", + refreshToken: "switched-in-rt", + expiresAt: 1000, }, - undefined, - "expected-at", - ) - - assert.equal(result, false) - const written = JSON.parse(readFileSync(credPath, "utf-8")) - assert.equal( - written.claudeAiOauth.accessToken, - "switched-in-at", - "the switched-in credential must survive untouched", - ) - } finally { - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME - } + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + }, + undefined, + "expected-at", + ) + + assert.equal(result, false) + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal( + written.claudeAiOauth.accessToken, + "switched-in-at", + "the switched-in credential must survive untouched", + ) + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME } - }) + } +}) ``` - [ ] **Step 2: Run to verify they fail** @@ -421,25 +426,25 @@ export function writeBackCredentials( In the `source === "file"` branch, immediately after `const raw = readFileSync(credPath, "utf-8")`: ```ts - if ( - expectedPriorAccessToken !== undefined && - !credentialBlobMatches(raw, expectedPriorAccessToken) - ) { - log("writeback_skipped_stale", { source, configDir: dir }) - return false - } +if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) +) { + log("writeback_skipped_stale", { source, configDir: dir }) + return false +} ``` In the `process.platform === "darwin"` branch, immediately after `if (!raw) return false`: ```ts - if ( - expectedPriorAccessToken !== undefined && - !credentialBlobMatches(raw, expectedPriorAccessToken) - ) { - log("writeback_skipped_stale", { source }) - return false - } +if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) +) { + log("writeback_skipped_stale", { source }) + return false +} ``` - [ ] **Step 4: Run to verify they pass** @@ -452,23 +457,23 @@ Expected: `pass 4`, `fail 0` `src/credentials.ts:376` — `creds` is the pre-refresh credential captured by `performRefresh`: ```ts - writeBackCredentials( - target.source, - oauthCreds, - target.configDir, - creds.accessToken, - ) +writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, +) ``` `src/credentials.ts:499` — inside `refreshBorrowedAccount`, `own` is this account's own stored credential: ```ts - writeBackCredentials( - target.source, - oauthCreds, - target.configDir, - own.accessToken, - ) +writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + own.accessToken, +) ``` `src/credentials.ts:606-628` — in `forceRefreshActiveAccount`, capture the token **before** the await, because `account.credentials` is reassigned on success: @@ -493,55 +498,55 @@ Expected: `pass 4`, `fail 0` Append inside the top-level `describe` in `src/credentials.test.ts`: ```ts - it("performRefresh passes the pre-refresh token as the write-back guard", async () => { - const originalNow = Date.now - const now = 1_700_000_000_000 - Date.now = () => now - const originalFetch = globalThis.fetch +it("performRefresh passes the pre-refresh token as the write-back guard", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch - try { - const { credentialsModule, keychainModule } = - await loadCredentialsWithCountingKeychain(now - 60_000) + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) - keychainModule.__setCredentials({ - accessToken: "stale-token", - refreshToken: "rt-stale", - expiresAt: now - 60_000, - }) + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) - credentialsModule.initAccounts([ - { - label: "Account 1", - source: "keychain", - credentials: { - accessToken: "stale-token", - refreshToken: "rt-stale", - expiresAt: now - 60_000, - }, + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, }, - ]) - - globalThis.fetch = (async () => - new Response( - JSON.stringify({ - access_token: "rotated-token", - refresh_token: "rt-rotated", - expires_in: 36_000, - }), - { status: 200 }, - )) as typeof fetch - - await credentialsModule.refreshIfNeeded() + }, + ]) - const writes = keychainModule.__getWrites() - assert.equal(writes.length, 1) - assert.equal(writes[0].creds.accessToken, "rotated-token") - assert.equal(writes[0].expectedPriorAccessToken, "stale-token") - } finally { - Date.now = originalNow - globalThis.fetch = originalFetch - } - }) + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + await credentialsModule.refreshIfNeeded() + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].creds.accessToken, "rotated-token") + assert.equal(writes[0].expectedPriorAccessToken, "stale-token") + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } +}) ``` - [ ] **Step 7: Run the full suite** @@ -561,6 +566,7 @@ git commit -m "fix: guard credential write-back against an external switch" ### Task 4: Bounded 401 recovery loop **Files:** + - Modify: `src/index.ts:368-392` (401 block), `:462-464` (return), imports at `:20-31` - Test: `src/index.test.ts:1189-1251` @@ -577,59 +583,59 @@ In the `from "./credentials.ts"` import block in `src/index.ts` (around line 20- Replace `src/index.ts:368-392` in full: ```ts - // Recover from a rejected token: first by adopting credentials - // rotated externally (cswap switching accounts, the claude CLI, - // another OpenCode instance), then by forcing an OAuth refresh - // when the store still holds the token that was just rejected. - // - // Most cases resolve on the first attempt: a cold or unreadable - // store yields null from the reload (reloadCredentialsFromSource - // rejects anything expiring within 60s), so the force refresh runs - // immediately. The second attempt covers the narrower race where - // the reload returns a valid-looking token that a concurrent - // writer has itself just rotated again. The no-progress break is - // what actually bounds this — the cap is defence in depth. - const MAX_AUTH_RECOVERY_ATTEMPTS = 2 - let tokenInUse = latest.accessToken - - for ( - let attempt = 0; - response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; - attempt++ - ) { - let candidate: ClaudeCredentials | null = null - try { - candidate = reloadCredentialsFromSource() - } catch {} - - if (!candidate || candidate.accessToken === tokenInUse) { - try { - candidate = await forceRefreshActiveAccount() - } catch {} - } - - if (!candidate || candidate.accessToken === tokenInUse) { - log("auth_recovery_exhausted", { - modelId, - attempt: attempt + 1, - }) - break - } - - tokenInUse = candidate.accessToken - log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) - response = await fetchWithRetry(requestUrl, { - ...requestInit, - body, - headers: buildRequestHeaders( - input, - requestInit, - tokenInUse, - modelId, - excluded, - ), - }) - } +// Recover from a rejected token: first by adopting credentials +// rotated externally (cswap switching accounts, the claude CLI, +// another OpenCode instance), then by forcing an OAuth refresh +// when the store still holds the token that was just rejected. +// +// Most cases resolve on the first attempt: a cold or unreadable +// store yields null from the reload (reloadCredentialsFromSource +// rejects anything expiring within 60s), so the force refresh runs +// immediately. The second attempt covers the narrower race where +// the reload returns a valid-looking token that a concurrent +// writer has itself just rotated again. The no-progress break is +// what actually bounds this — the cap is defence in depth. +const MAX_AUTH_RECOVERY_ATTEMPTS = 2 +let tokenInUse = latest.accessToken + +for ( + let attempt = 0; + response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; + attempt++ +) { + let candidate: ClaudeCredentials | null = null + try { + candidate = reloadCredentialsFromSource() + } catch {} + + if (!candidate || candidate.accessToken === tokenInUse) { + try { + candidate = await forceRefreshActiveAccount() + } catch {} + } + + if (!candidate || candidate.accessToken === tokenInUse) { + log("auth_recovery_exhausted", { + modelId, + attempt: attempt + 1, + }) + break + } + + tokenInUse = candidate.accessToken + log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) +} ``` - [ ] **Step 3: Derive the stream-transform decision from the final status** @@ -637,12 +643,10 @@ Replace `src/index.ts:368-392` in full: Replace `src/index.ts:462-464`: ```ts - // A 401 that survived recovery carries an error body, not an SSE - // stream. Deciding here rather than from a flag set mid-flight - // makes the retried and non-retried paths behave identically. - return response.status === 401 - ? response - : transformResponseStream(response) +// A 401 that survived recovery carries an error body, not an SSE +// stream. Deciding here rather than from a flag set mid-flight +// makes the retried and non-retried paths behave identically. +return response.status === 401 ? response : transformResponseStream(response) ``` - [ ] **Step 4: Run the suite to see the expected failure** @@ -657,153 +661,153 @@ The neighbouring test around `src/index.test.ts:1141` (a 401 recovered by an ext Replace the whole `it(...)` block at `src/index.test.ts:1189-1251` with: ```ts - it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { - const originalNow = Date.now - const originalSetInterval = globalThis.setInterval - const originalHome = process.env.HOME - const originalFetch = globalThis.fetch - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) - process.env.HOME = tempHome - Date.now = () => 1_700_000_000_000 - globalThis.setInterval = (() => ({ - unref() {}, - })) as unknown as typeof setInterval - - let apiCalls = 0 - let oauthCalls = 0 +it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 - try { - const { helpersModule } = await loadHelpersWithCountingKeychain( - Date.now() + 10 * 60_000, - ) - - globalThis.fetch = (async (input: RequestInfo | URL) => { - const url = typeof input === "string" ? input : String(input) - if (url.includes("/oauth/token")) { - oauthCalls += 1 - return new Response( - JSON.stringify({ - access_token: "recovered-token", - refresh_token: "rt-recovered", - expires_in: 36_000, - }), - { status: 200 }, - ) - } - apiCalls += 1 - return apiCalls === 1 - ? new Response('{"error":"expired"}', { status: 401 }) - : new Response("data: {}\n\n", { status: 200 }) - }) as typeof fetch - - const plugin = await helpersModule.default({} as never) - const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - const authConfig = await typedPlugin.auth!.loader!( - async () => ({ - type: "oauth", - refresh: "refresh", - access: "access", - expires: Date.now() + 60_000, - }), - { models: {} }, - ) - - const response = await authConfig.fetch( - "https://api.anthropic.com/v1/messages", - { - method: "POST", - body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), - }, - ) - - assert.equal(response.status, 200) - assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") - assert.equal(apiCalls, 2, "the refreshed token must be retried once") - } finally { - Date.now = originalNow - globalThis.setInterval = originalSetInterval - globalThis.fetch = originalFetch - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "recovered-token", + refresh_token: "rt-recovered", + expires_in: 36_000, + }), + { status: 200 }, + ) } + apiCalls += 1 + return apiCalls === 1 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") + assert.equal(apiCalls, 2, "the refreshed token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME } - }) - - it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { - const originalNow = Date.now - const originalSetInterval = globalThis.setInterval - const originalHome = process.env.HOME - const originalFetch = globalThis.fetch - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) - process.env.HOME = tempHome - Date.now = () => 1_700_000_000_000 - globalThis.setInterval = (() => ({ - unref() {}, - })) as unknown as typeof setInterval - - let apiCalls = 0 - const errorBody = '{"name":"mcp_UnchangedToken"}' + } +}) - try { - const { helpersModule } = await loadHelpersWithCountingKeychain( - Date.now() + 10 * 60_000, - ) +it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + const errorBody = '{"name":"mcp_UnchangedToken"}' - globalThis.fetch = (async (input: RequestInfo | URL) => { - const url = typeof input === "string" ? input : String(input) - if (url.includes("/oauth/token")) { - // A 429 from the token endpoint must read as a failed refresh, not - // as something that loops account recovery. retry-after beyond the - // 30s cap makes fetchWithRetry return instead of backing off. - return new Response('{"error":"rate_limited"}', { - status: 429, - headers: { "retry-after": "3600" }, - }) - } - apiCalls += 1 - return new Response(errorBody, { - status: 401, - headers: { "x-request-id": "unchanged-token-401" }, + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + // A 429 from the token endpoint must read as a failed refresh, not + // as something that loops account recovery. retry-after beyond the + // 30s cap makes fetchWithRetry return instead of backing off. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, }) - }) as typeof fetch - - const plugin = await helpersModule.default({} as never) - const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - const authConfig = await typedPlugin.auth!.loader!( - async () => ({ - type: "oauth", - refresh: "refresh", - access: "access", - expires: Date.now() + 60_000, - }), - { models: {} }, - ) - - const response = await authConfig.fetch( - "https://api.anthropic.com/v1/messages", - { - method: "POST", - body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), - }, - ) - - assert.equal(response.status, 401) - assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") - assert.equal(await response.text(), errorBody) - assert.equal(apiCalls, 1, "no retry without a different token") - } finally { - Date.now = originalNow - globalThis.setInterval = originalSetInterval - globalThis.fetch = originalFetch - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME } + apiCalls += 1 + return new Response(errorBody, { + status: 401, + headers: { "x-request-id": "unchanged-token-401" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 401) + assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") + assert.equal(await response.text(), errorBody) + assert.equal(apiCalls, 1, "no retry without a different token") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME } - }) + } +}) ``` - [ ] **Step 6: Add the test that justifies a second attempt** @@ -811,86 +815,86 @@ Replace the whole `it(...)` block at `src/index.test.ts:1189-1251` with: This is the only scenario where one attempt is insufficient: the reload returns a valid-looking token that a concurrent writer has itself just rotated again, so the first retry is also rejected. Append after the two tests from Step 5: ```ts - it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { - const originalNow = Date.now - const originalSetInterval = globalThis.setInterval - const originalHome = process.env.HOME - const originalFetch = globalThis.fetch - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) - process.env.HOME = tempHome - Date.now = () => 1_700_000_000_000 - globalThis.setInterval = (() => ({ - unref() {}, - })) as unknown as typeof setInterval - - let apiCalls = 0 - let oauthCalls = 0 - - try { - const { helpersModule, keychainModule } = - await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) - - globalThis.fetch = (async (input: RequestInfo | URL) => { - const url = typeof input === "string" ? input : String(input) - if (url.includes("/oauth/token")) { - oauthCalls += 1 - return new Response( - JSON.stringify({ - access_token: "second-stage-token", - refresh_token: "rt-second-stage", - expires_in: 36_000, - }), - { status: 200 }, - ) - } - apiCalls += 1 - // Calls 1 and 2 are rejected; only the force-refreshed token works. - return apiCalls <= 2 - ? new Response('{"error":"expired"}', { status: 401 }) - : new Response("data: {}\n\n", { status: 200 }) - }) as typeof fetch - - const plugin = await helpersModule.default({} as never) - const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - const authConfig = await typedPlugin.auth!.loader!( - async () => ({ - type: "oauth", - refresh: "refresh", - access: "access", - expires: Date.now() + 60_000, - }), - { models: {} }, - ) - - // Attempt 1's reload finds a different, still-valid-looking token. - keychainModule.__setCredentials({ - accessToken: "concurrently-rotated", - refreshToken: "rt-concurrent", - expiresAt: Date.now() + 10 * 60_000, - }) - - const response = await authConfig.fetch( - "https://api.anthropic.com/v1/messages", - { - method: "POST", - body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), - }, - ) +it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 - assert.equal(response.status, 200) - assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") - assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") - } finally { - Date.now = originalNow - globalThis.setInterval = originalSetInterval - globalThis.fetch = originalFetch - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "second-stage-token", + refresh_token: "rt-second-stage", + expires_in: 36_000, + }), + { status: 200 }, + ) } + apiCalls += 1 + // Calls 1 and 2 are rejected; only the force-refreshed token works. + return apiCalls <= 2 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Attempt 1's reload finds a different, still-valid-looking token. + keychainModule.__setCredentials({ + accessToken: "concurrently-rotated", + refreshToken: "rt-concurrent", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") + assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME } - }) + } +}) ``` - [ ] **Step 7: Run the full suite** @@ -910,6 +914,7 @@ git commit -m "fix: recover from a rejected token by forcing an OAuth refresh" ### Task 5: Re-read the source on a 429 **Files:** + - Modify: `src/index.ts` — insert after the 401 recovery loop, before the long-context beta loop (`:396`) - Test: `src/index.test.ts` @@ -918,139 +923,139 @@ git commit -m "fix: recover from a rejected token by forcing an OAuth refresh" Append inside the same `describe` as Task 4's tests: ```ts - it("auth fetch retries a 429 once when the source token changed", async () => { - const originalNow = Date.now - const originalSetInterval = globalThis.setInterval - const originalHome = process.env.HOME - const originalFetch = globalThis.fetch - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) - process.env.HOME = tempHome - Date.now = () => 1_700_000_000_000 - globalThis.setInterval = (() => ({ - unref() {}, - })) as unknown as typeof setInterval - - let apiCalls = 0 - - try { - const { helpersModule, keychainModule } = - await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) - - globalThis.fetch = (async () => { - apiCalls += 1 - if (apiCalls === 1) { - // retry-after beyond the 30s cap: quota exhaustion, not a - // transient limit, so fetchWithRetry returns immediately. - return new Response('{"error":"rate_limited"}', { - status: 429, - headers: { "retry-after": "3600" }, - }) - } - return new Response("data: {}\n\n", { status: 200 }) - }) as typeof fetch - - const plugin = await helpersModule.default({} as never) - const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - const authConfig = await typedPlugin.auth!.loader!( - async () => ({ - type: "oauth", - refresh: "refresh", - access: "access", - expires: Date.now() + 60_000, - }), - { models: {} }, - ) +it("auth fetch retries a 429 once when the source token changed", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 - // An external switch lands while this session is still on the - // exhausted account. - keychainModule.__setCredentials({ - accessToken: "switched-token", - refreshToken: "rt-switched", - expiresAt: Date.now() + 10 * 60_000, - }) - - const response = await authConfig.fetch( - "https://api.anthropic.com/v1/messages", - { - method: "POST", - body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), - }, - ) - - assert.equal(response.status, 200) - assert.equal(apiCalls, 2, "the rotated token must be retried once") - } finally { - Date.now = originalNow - globalThis.setInterval = originalSetInterval - globalThis.fetch = originalFetch - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME - } - } - }) - - it("auth fetch does not retry a 429 when the source token is unchanged", async () => { - const originalNow = Date.now - const originalSetInterval = globalThis.setInterval - const originalHome = process.env.HOME - const originalFetch = globalThis.fetch - const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) - process.env.HOME = tempHome - Date.now = () => 1_700_000_000_000 - globalThis.setInterval = (() => ({ - unref() {}, - })) as unknown as typeof setInterval - - let apiCalls = 0 - - try { - const { helpersModule } = await loadHelpersWithCountingKeychain( - Date.now() + 10 * 60_000, - ) - - globalThis.fetch = (async () => { - apiCalls += 1 + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + if (apiCalls === 1) { + // retry-after beyond the 30s cap: quota exhaustion, not a + // transient limit, so fetchWithRetry returns immediately. return new Response('{"error":"rate_limited"}', { status: 429, headers: { "retry-after": "3600" }, }) - }) as typeof fetch - - const plugin = await helpersModule.default({} as never) - const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - const authConfig = await typedPlugin.auth!.loader!( - async () => ({ - type: "oauth", - refresh: "refresh", - access: "access", - expires: Date.now() + 60_000, - }), - { models: {} }, - ) + } + return new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // An external switch lands while this session is still on the + // exhausted account. + keychainModule.__setCredentials({ + accessToken: "switched-token", + refreshToken: "rt-switched", + expiresAt: Date.now() + 10 * 60_000, + }) - const response = await authConfig.fetch( - "https://api.anthropic.com/v1/messages", - { - method: "POST", - body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), - }, - ) + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 2, "the rotated token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } +}) - assert.equal(response.status, 429) - assert.equal(apiCalls, 1, "an unchanged token must not be retried") - } finally { - Date.now = originalNow - globalThis.setInterval = originalSetInterval - globalThis.fetch = originalFetch - if (typeof originalHome === "string") { - process.env.HOME = originalHome - } else { - delete process.env.HOME - } +it("auth fetch does not retry a 429 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unchanged token must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME } - }) + } +}) ``` `loadHelpersWithCountingKeychain` already returns `keychainModule` with `__setCredentials` (`src/index.test.ts:264-273`), so no harness change is needed. @@ -1065,34 +1070,34 @@ Expected: the "token changed" test FAILS with `apiCalls` 1 ≠ 2; the "unchanged Insert in `src/index.ts` directly after the 401 recovery loop from Task 4 and before the long-context beta loop: ```ts - // An external switch — cswap rotating off an exhausted account — - // leaves this session on the old token until the 30s credential - // cache expires. Re-read once so a rate limit that has already - // been resolved elsewhere is not surfaced. A changed token is the - // signal that a switch happened; when nothing changed this costs - // one source read and no retry. - if (response.status === 429) { - let rotated: ClaudeCredentials | null = null - try { - rotated = reloadCredentialsFromSource() - } catch {} - - if (rotated && rotated.accessToken !== tokenInUse) { - log("rate_limit_credentials_rotated", { modelId }) - tokenInUse = rotated.accessToken - response = await fetchWithRetry(requestUrl, { - ...requestInit, - body, - headers: buildRequestHeaders( - input, - requestInit, - tokenInUse, - modelId, - excluded, - ), - }) - } - } +// An external switch — cswap rotating off an exhausted account — +// leaves this session on the old token until the 30s credential +// cache expires. Re-read once so a rate limit that has already +// been resolved elsewhere is not surfaced. A changed token is the +// signal that a switch happened; when nothing changed this costs +// one source read and no retry. +if (response.status === 429) { + let rotated: ClaudeCredentials | null = null + try { + rotated = reloadCredentialsFromSource() + } catch {} + + if (rotated && rotated.accessToken !== tokenInUse) { + log("rate_limit_credentials_rotated", { modelId }) + tokenInUse = rotated.accessToken + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + } +} ``` - [ ] **Step 4: Run to verify both pass** @@ -1117,6 +1122,7 @@ git commit -m "fix: re-read credentials on a 429 so a resolved rate limit is not ### Task 6: Lint, format, and document **Files:** + - Modify: `README.md`, `CHANGELOG.md` - [ ] **Step 1: Lint and format** From 4a81ad0118ae6c91eb1280aa5ec6c1bf42db7cdc Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:40:24 -0600 Subject: [PATCH 07/33] fix: re-read keychain sources so external credential rotation is picked up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/credentials.test.ts | 135 +++++++++++++++++++++++++++++++++++----- src/credentials.ts | 31 ++++++--- 2 files changed, 142 insertions(+), 24 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index a718ed4..98d056a 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -282,12 +282,20 @@ describe("credential caching", () => { }, ]) + // The source agrees with memory to begin with, so priming the cache + // (which re-reads the source) leaves the account on "old-token". + keychainModule.__setCredentialsForSource("keychain", { + accessToken: "old-token", + refreshToken: "old-refresh", + expiresAt: now + 10 * 60_000, + }) + assert.equal( (await credentialsModule.getCachedCredentials())?.accessToken, "old-token", ) - keychainModule.__setCredentials({ + keychainModule.__setCredentialsForSource("keychain", { accessToken: "new-token", refreshToken: "new-refresh", expiresAt: now + 8 * 60 * 60_000, @@ -297,7 +305,11 @@ describe("credential caching", () => { const readCountAfterReload = keychainModule.__getReadCount() assert.equal(reloaded?.accessToken, "new-token") - assert.equal(readCountAfterReload, 1) + assert.equal( + readCountAfterReload, + 2, + "one re-read while priming the cache, one for the explicit reload", + ) assert.equal( (await credentialsModule.getCachedCredentials())?.accessToken, "new-token", @@ -332,7 +344,7 @@ describe("credential caching", () => { keychainModule.__setReadError(true) assert.equal(credentialsModule.reloadCredentialsFromSource(), null) - assert.equal(keychainModule.__getReadCount(), 1) + assert.equal(keychainModule.__getReadCount(), 2) } finally { Date.now = originalNow } @@ -454,7 +466,11 @@ describe("credential caching", () => { assert.ok(first) assert.ok(second) - assert.equal(keychainModule.__getReadCount(), 0) + assert.equal( + keychainModule.__getReadCount(), + 1, + "cache miss re-reads the source once; the cached call does not", + ) } finally { Date.now = originalNow } @@ -744,17 +760,27 @@ describe("credential caching", () => { }, ]) + // Pin the expired account's own stored value so the up-front re-read is + // a no-op rather than swapping in the other account's blob. + keychainModule.__setCredentialsForSource( + "Claude Code-credentials-aabbccdd", + { + accessToken: "stale-suffixed", + refreshToken: "rt-suffixed", + expiresAt: now - 1_000, + }, + ) + const readsBefore = keychainModule.__getReadCount() const result = await credentialsModule.refreshIfNeeded() assert.equal(result?.accessToken, "fresh-in-memory") - // One read only: the target re-reading its OWN source to see whether - // another process rotated it. The fallback account is still borrowed - // straight from memory rather than re-read. + // The fallback account is still borrowed straight from memory rather + // than re-read. assert.equal( keychainModule.__getReadCount(), - readsBefore + 1, - "valid in-memory fallback credentials must not trigger a keychain read", + readsBefore + 2, + "one up-front re-read of the target's own source, one after the failed OAuth refresh", ) } finally { Date.now = originalNow @@ -766,8 +792,8 @@ describe("credential caching", () => { const { credentialsModule, keychainModule } = await loadCredentialsWithCountingKeychain(now + 10 * 60_000) - // Keychain source: refreshIfNeeded never re-reads these while the local - // copy looks valid, so a 401 needs an explicit source reload. + // A 401 must reload the source immediately rather than waiting for the + // next refreshIfNeeded, which the 30s credential cache can defer. const account = { label: "Account 1", source: "keychain", @@ -955,6 +981,74 @@ describe("credential caching", () => { Date.now = originalNow } }) + + it("refreshIfNeeded adopts credentials rotated externally in a keychain source", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "before-switch", + refreshToken: "rt-before", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // An external process (cswap, the claude CLI, a second OpenCode) + // replaces the stored credential with a different account's. + keychainModule.__setCredentials({ + accessToken: "after-switch", + refreshToken: "rt-after", + expiresAt: now + 10 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "after-switch") + } finally { + Date.now = originalNow + } + }) + + it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory", + refreshToken: "rt", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + keychainModule.__setReadError(true) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "in-memory") + } finally { + Date.now = originalNow + } + }) }) describe("syncAuthJson file permissions", () => { @@ -1337,11 +1431,19 @@ describe("refreshIfNeeded CLI fallback scope", () => { }) as typeof fetch try { - const { credentialsModule, childProcessModule } = + const { credentialsModule, keychainModule, childProcessModule } = await loadCredentialsWithCountingKeychain(now + 30 * 60_000) const target = makeAccount(now + 30 * 60_000) credentialsModule.initAccounts([target]) + // Pin the target's own stored value so the up-front re-read is a no-op + // and the assertion below speaks to the CLI, not to source adoption. + keychainModule.__setCredentialsForSource("keychain", { + accessToken: "existing-token", + refreshToken: "existing-refresh", + expiresAt: now + 30 * 60_000, + }) + const result = await credentialsModule.refreshIfNeeded( target, 60 * 60_000, @@ -1517,8 +1619,11 @@ describe("refreshIfNeeded CLI fallback scope", () => { const target = makeAccount(now + 30_000) credentialsModule.initAccounts([target]) - // The store initially matches memory, so the pre-CLI re-read finds - // nothing new. Only once the CLI has run does the entry rotate. + // The store initially matches memory, so neither the up-front re-read + // nor the pre-CLI re-read finds anything new. Only once the CLI has run + // does the entry rotate — hence the third read, not the second: + // 1) refreshIfNeeded's up-front re-read, 2) the re-read after OAuth + // fails, 3) the read after the CLI has rotated the entry. keychainModule.__setCredentials({ accessToken: "existing-token", refreshToken: "existing-refresh", @@ -1527,7 +1632,7 @@ describe("refreshIfNeeded CLI fallback scope", () => { let reads = 0 keychainModule.__setReadHook(() => { reads += 1 - if (reads >= 2) { + if (reads >= 3) { keychainModule.__setCredentials({ accessToken: "cli-rotated-token", refreshToken: "cli-rotated-refresh", diff --git a/src/credentials.ts b/src/credentials.ts index de230f2..6b377a6 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -323,14 +323,24 @@ export async function refreshIfNeeded( const target = account ?? getActiveAccount() if (!target) return null - // Pick up external updates to .credentials.json (e.g. switch_claude_account - // on Windows). Bounded by getCachedCredentials's 30s TTL: fires at most - // ~2x/min under load. macOS keychain sources stay on the in-memory path; - // their state is mutated only by our own writeBackCredentials, so no - // external-update vector exists for them. - if (target.source === "file") { - const onDisk = refreshAccount(target.source) - if (onDisk) target.credentials = onDisk + // Pick up credentials replaced externally — cswap switching accounts, the + // claude CLI in another terminal, or a second OpenCode instance. This used + // to be limited to file sources on the assumption that a keychain entry is + // only ever mutated by our own writeBackCredentials; that assumption is + // false. Bounded by getCachedCredentials's 30s TTL, so it fires at most + // ~2x/min under load. + // + // A keychain read shells out to `security`, which throws when the keychain + // is locked, access is denied, or the call times out. Degrade to the + // in-memory credentials rather than take down the request path. + try { + const stored = refreshAccount(target.source, target.configDir) + if (stored) target.credentials = stored + } catch (err) { + log("source_reread_failed", { + source: target.source, + error: err instanceof Error ? err.message : String(err), + }) } const creds = target.credentials @@ -397,7 +407,10 @@ async function performRefresh( // invalidates the refresh token the others are holding. When ours is // rejected, the instance that won may already have written usable // credentials to the shared store — far cheaper to re-read than to spawn - // the CLI. (File sources are re-read up front by refreshIfNeeded.) + // the CLI. refreshIfNeeded now re-reads every source up front, but the + // OAuth attempt since then cost a network round trip, so this second read + // can still find a newer entry. File sources keep skipping it, as they did + // when the up-front re-read was file-only. if (target.source !== "file") { let stored: ClaudeCredentials | null = null try { From 52ededed7d03cc7a7048c8986418ff5836380d00 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:42:16 -0600 Subject: [PATCH 08/33] docs: record actual Task 2 test breakage and check off completed steps --- ...07-29-external-credential-rotation-plan.md | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 186fe1a..80780b4 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -35,6 +35,16 @@ These pass today and **will fail** partway through. Each is repaired in the task | `src/credentials.test.ts:735` | `__getReadCount() === readsBefore + 1` | now two reads | Task 2 | | `src/index.test.ts:1189` | "does not retry a 401 when the source token is unchanged" | that is the behavior being changed | Task 4 | +Three further Task 2 breakages were found during execution that this plan did not predict. All share the root cause above — the up-front re-read makes the stub's source blob visible to accounts whose in-memory credentials differ from it: + +| Test | Fix | +| ----------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `reloadCredentialsFromSource bypasses cache` | prime via `__setCredentialsForSource` so the rotation under test still lands | +| `does not spawn the claude CLI while credentials are still usable` | pin the source 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; move the threshold to read #3 | + +The last one matters beyond bookkeeping. Left at read #2 the result-token assertion still passes while `__getExecSyncCount()` drops to 0 — the CLI fallback silently stops being exercised, and the test keeps looking meaningful. Verified by reverting the threshold: `actual: 0, expected: 1`. Relaxing that count instead of moving the threshold would have left dead coverage behind. + --- ### Task 1: Make the credentials test harness source-aware @@ -45,7 +55,7 @@ The stub `refreshAccount(source)` ignores `source` and returns one global blob ( - Modify: `src/credentials.test.ts:126-190` (the `tempKeychain` stub literal) -- [ ] **Step 1: Add per-source overrides to the stub keychain** +- [x] **Step 1: Add per-source overrides to the stub keychain** In the `writeFile(tempKeychain, ...)` template literal, replace the `refreshAccount` and `__setCredentials` definitions and add two exports. Keep `__setCredentials` working as the default so existing tests are untouched. @@ -73,7 +83,7 @@ export function __setCredentialsForSource(source, c) { Each test calls `loadCredentialsWithCountingKeychain` fresh, which imports a new module instance with an empty `bySource`, so no reset helper is needed. -- [ ] **Step 2: Record the expected-prior-token argument on write-back** +- [x] **Step 2: Record the expected-prior-token argument on write-back** Replace the stub `writeBackCredentials` so Task 3 can assert on it. Four parameters, matching the real signature after Task 3. @@ -90,7 +100,7 @@ export function writeBackCredentials( } ``` -- [ ] **Step 3: Widen the harness return types** +- [x] **Step 3: Widen the harness return types** In the `keychainModule` type annotation (both the declared return type at `src/credentials.test.ts:56-64` and the cast at `:226-235`), add the new members and widen `__getWrites`: @@ -104,12 +114,12 @@ In the `keychainModule` type annotation (both the declared return type at `src/c }> ``` -- [ ] **Step 4: Run the full suite to confirm nothing regressed** +- [x] **Step 4: Run the full suite to confirm nothing regressed** Run: `pnpm test 2>&1 | tail -8` Expected: `pass 249`, `fail 0` -- [ ] **Step 5: Commit** +- [x] **Step 5: Commit** ```bash git add src/credentials.test.ts @@ -125,7 +135,7 @@ git commit -m "test: make credentials keychain stub source-aware" - Modify: `src/credentials.ts:326-334` - Test: `src/credentials.test.ts` -- [ ] **Step 1: Write the failing tests** +- [x] **Step 1: Write the failing tests** Append inside the existing top-level `describe` block in `src/credentials.test.ts`: @@ -199,12 +209,12 @@ it("refreshIfNeeded keeps in-memory credentials when the source read throws", as }) ``` -- [ ] **Step 2: Run the new tests to verify they fail** +- [x] **Step 2: Run the new tests to verify they fail** Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -12` Expected: FAIL — the first asserts `'before-switch' !== 'after-switch'`; the second throws `Keychain read denied`. -- [ ] **Step 3: Implement the re-read** +- [x] **Step 3: Implement the re-read** In `src/credentials.ts`, replace lines 326-334 (the comment block and the `if (target.source === "file")` guard) with: @@ -230,12 +240,12 @@ try { } ``` -- [ ] **Step 4: Run the new tests to verify they pass** +- [x] **Step 4: Run the new tests to verify they pass** Run: `node --test --experimental-strip-types --test-name-pattern="rotated externally in a keychain source|source read throws" src/credentials.test.ts 2>&1 | tail -8` Expected: `pass 2`, `fail 0` -- [ ] **Step 5: Repair the three read-count assertions** +- [x] **Step 5: Repair the three read-count assertions** Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` Expected: three failures, at `src/credentials.test.ts` lines ~315, ~437, ~735. @@ -278,12 +288,12 @@ assert.equal( ) ``` -- [ ] **Step 6: Run the full suite** +- [x] **Step 6: Run the full suite** Run: `pnpm test 2>&1 | tail -8` Expected: `pass 251`, `fail 0` -- [ ] **Step 7: Commit** +- [x] **Step 7: Commit** ```bash git add src/credentials.ts src/credentials.test.ts From d7b7557e948225d72d0e29401eb9614e2e7aa7d7 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:48:52 -0600 Subject: [PATCH 09/33] docs: state that the post-OAuth file-source exclusion is unjustified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/credentials.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/credentials.ts b/src/credentials.ts index 6b377a6..a3973b9 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -406,11 +406,14 @@ async function performRefresh( // Every OpenCode instance refreshes independently, and a rotation // invalidates the refresh token the others are holding. When ours is // rejected, the instance that won may already have written usable - // credentials to the shared store — far cheaper to re-read than to spawn - // the CLI. refreshIfNeeded now re-reads every source up front, but the - // OAuth attempt since then cost a network round trip, so this second read - // can still find a newer entry. File sources keep skipping it, as they did - // when the up-front re-read was file-only. + // credentials to the shared store during the OAuth round trip — far + // cheaper to re-read than to spawn the CLI. + // + // The file-source exclusion below is a leftover from when refreshIfNeeded + // re-read file sources only. That rationale is gone and the exclusion now + // has none: a sibling process can write a file source mid-round-trip + // exactly as it can a keychain entry. Left in place only to keep this + // change off the file path; removing it is tracked as a follow-up. if (target.source !== "file") { let stored: ClaudeCredentials | null = null try { From 8db725391ac0ca863802a7b35c20b5eeb54692db Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 12:49:32 -0600 Subject: [PATCH 10/33] docs: track the unjustified file-source exclusion as a follow-up --- specs/2026-07-29-external-credential-rotation-plan.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 80780b4..b75bf2c 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1180,6 +1180,14 @@ git commit -m "docs: document external credential rotation handling" --- +## Follow-ups (deliberately out of scope) + +**The post-OAuth file-source exclusion in `performRefresh` is now unjustified.** `src/credentials.ts` re-reads the source after a failed OAuth refresh, to catch credentials a sibling instance wrote during the round trip — but skips that read for `file` sources. That guard was coherent while the up-front re-read was file-only; Task 2 removed that rationale and left nothing in its place. A sibling can write a file source mid-round-trip exactly as it can a keychain entry, a file read is cheaper than shelling out to `security`, and the block is already try/catch-wrapped, so including file sources would add no throw path. + +Consequence today: a file-source account whose OAuth refresh fails goes straight to `refreshViaCli` without checking whether a sibling already wrote usable credentials — the very race the block exists to handle. + +Not fixed here because it changes behavior on the file path, which no current test covers, and it is outside the guard replacement this feature specified. The comment at the guard says so plainly rather than implying the asymmetry is principled. + ## Manual verification Automated tests stub the Keychain. Verify against real cswap state once: From 974171a1d49618d5e155bcd1a0beaf55ce76c479 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:00:59 -0600 Subject: [PATCH 11/33] fix: validate the up-front source re-read before adopting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/credentials.test.ts | 176 ++++++++++++++++++++++++++++++++++++++++ src/credentials.ts | 28 ++++++- 2 files changed, 203 insertions(+), 1 deletion(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index 98d056a..5ad37c7 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -1019,6 +1019,99 @@ describe("credential caching", () => { } }) + // writeBackCredentials can fail while the read that precedes it succeeds: + // a malformed stored blob makes updateCredentialBlob return null, and an + // ACL can permit reads but not add-generic-password. performRefresh + // discards that false, so memory ends up holding freshly refreshed + // credentials while the store still holds the pre-refresh blob — which, + // because we only refresh inside the 60s window, has under 60s left. + // Adopting it would re-enter performRefresh with a refresh token our own + // successful refresh just rotated dead, failing over to two 60s claude + // spawns on every cache miss, forever. + it("refreshIfNeeded does not adopt a stale stored blob over valid in-memory credentials", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule, childProcessModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "fresh-in-memory", + refreshToken: "rt-fresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + // The store still holds the pre-refresh blob the failed write-back + // never replaced. + keychainModule.__setCredentials({ + accessToken: "stale-in-store", + refreshToken: "rt-stale", + expiresAt: now + 30_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "fresh-in-memory") + assert.equal( + childProcessModule.__getExecSyncCount(), + 0, + "adopting the stale blob would fail OAuth and fall through to the CLI", + ) + } finally { + Date.now = originalNow + } + }) + + // Complement of the test above: the guard must decline only the unusable + // blob. If it declined whenever memory was valid, external rotation would + // stop being picked up and this task's whole premise would regress. + it("refreshIfNeeded adopts a usable stored blob even when memory is still valid", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + const account = { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "in-memory-valid", + refreshToken: "rt-memory", + expiresAt: now + 10 * 60_000, + }, + } + credentialsModule.initAccounts([account]) + + keychainModule.__setCredentials({ + accessToken: "rotated-in-store", + refreshToken: "rt-rotated", + expiresAt: now + 8 * 60 * 60_000, + }) + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal(result?.accessToken, "rotated-in-store") + assert.equal( + account.credentials.accessToken, + "rotated-in-store", + "the adoption must land on the account so later calls see it", + ) + } finally { + Date.now = originalNow + } + }) + it("refreshIfNeeded keeps in-memory credentials when the source read throws", async () => { const originalNow = Date.now const now = 1_700_000_000_000 @@ -1902,6 +1995,89 @@ describe("borrowed credentials after exhaustion", () => { } }) + // refreshIfNeeded's up-front re-read pulls from the account's OWN source, + // so anything it adopts is the account's own credentials — it is no longer + // borrowing. Leaving the flag set would make forceRefreshActiveAccount + // refuse to exchange tokens that are legitimately the account's. + it("clears the borrowed flag once the up-front re-read adopts its own credentials", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + globalThis.fetch = (async () => { + throw new Error("network unreachable") + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 60 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // Its own entry is expired, so it ends up on the lender's tokens. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal( + borrower.credentials.accessToken, + "at-lender", + "precondition: the account is running on borrowed tokens", + ) + + // An external process repairs its entry. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-fresh", + refreshToken: "rt-borrower-fresh", + expiresAt: now + 8 * 60 * 60_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal(borrower.credentials.accessToken, "at-borrower-fresh") + + const seen: string[] = [] + const forced = await credentialsModule.forceRefreshActiveAccount( + async (token: string) => { + seen.push(token) + return { + accessToken: "at-forced", + refreshToken: "rt-forced", + expiresAt: now + 8 * 60 * 60_000, + } + }, + ) + + assert.deepEqual( + seen, + ["rt-borrower-fresh"], + "a recovered account must be free to force-refresh its own token", + ) + assert.equal(forced?.accessToken, "at-forced") + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) + it("forceRefreshActiveAccount refuses to exchange a borrowed token", async () => { const originalFetch = globalThis.fetch const originalNow = Date.now diff --git a/src/credentials.ts b/src/credentials.ts index a3973b9..a4d5791 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -333,9 +333,35 @@ export async function refreshIfNeeded( // A keychain read shells out to `security`, which throws when the keychain // is locked, access is denied, or the call times out. Degrade to the // in-memory credentials rather than take down the request path. + // + // Adopt a usable stored blob always; an unusable one only when what we + // already hold is unusable too. Do not simplify this to an unconditional + // adopt: performRefresh ignores writeBackCredentials's return value, and + // that write can fail while the read before it succeeded (a malformed + // blob, or an ACL allowing 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, since that window is the + // only reason we refreshed. Adopting it re-enters performRefresh with a + // refresh token our own refresh just rotated dead, so OAuth fails and we + // fall through to two 60s claude spawns, on every cache miss, forever. + // + // The accepted cost: if an external switch installs an account whose + // stored token is already expired while ours is still usable, we keep ours + // until it expires and adopt the switched-in account then. cswap freshens + // a target before activating it, so a newly-active slot is normally fresh. try { const stored = refreshAccount(target.source, target.configDir) - if (stored) target.credentials = stored + const now = Date.now() + if ( + stored && + (stored.expiresAt > now + 60_000 || + target.credentials.expiresAt <= now + 60_000) + ) { + target.credentials = stored + // Read from this account's own source, so what it returned is this + // account's own credentials — it is no longer running on a lender's. + borrowedCredentialAccounts.delete(target) + } } catch (err) { log("source_reread_failed", { source: target.source, From 8d665ec6c92c6dd8f401db7a981e76a85f581304 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:02:10 -0600 Subject: [PATCH 12/33] docs: record the validated-adopt rule in the design --- ...-29-external-credential-rotation-design.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md index 4fca333..b48abd8 100644 --- a/specs/2026-07-29-external-credential-rotation-design.md +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -109,6 +109,38 @@ propagating into the request path. Worst-case latency for an external switch to take effect: one cache TTL (30s). +**The adopt must be validated.** Taking any non-null stored blob over valid +in-memory credentials is unsafe, because `performRefresh` ignores +`writeBackCredentials`'s return value. When the read succeeds but the write +fails — a malformed stored blob, or an ACL allowing read but not +`add-generic-password` — memory holds freshly refreshed credentials while the +store holds the pre-refresh blob. That blob has under 60s left, since that +window is the only reason a refresh happened. Adopting it re-enters +`performRefresh` with a refresh token the successful refresh just rotated dead, +so OAuth fails and the code falls through to two 60s `claude` spawns — on every +cache miss, indefinitely, on the authentication path. + +The rule: **adopt a usable stored blob always; adopt an unusable one only when +what we already hold is also unusable.** + +| stored | in memory | action | +| -------- | --------- | ------------------------------------------------------------------ | +| usable | usable | adopt — this is the external-switch case | +| usable | unusable | adopt | +| unusable | usable | decline — this is the failed-write-back case | +| unusable | unusable | adopt; nothing is lost and the stored refresh token may still work | + +Accepted cost: if an external switch installs an account whose stored token is +already expired while ours is still usable, the session keeps its own +credentials until they expire and adopts the switched-in account then. cswap +freshens a target before activating it, so a newly-active slot is normally +fresh, and the alternative is the failure loop above. + +The same branch clears the borrowed-credentials flag. A read from the account's +own source returns that account's own credentials, so it is by definition no +longer running on a lender's — matching the invariant `refreshBorrowedAccount` +maintains at every other adoption site. + ### 2. Compare-and-swap on write-back `writeBackCredentials` (`src/keychain.ts:442`) gains an optional expected prior From 87edc204c3468f360681b4330655b59828e3ac97 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:08:54 -0600 Subject: [PATCH 13/33] docs: scope the orphaned-blob window to the reactive path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/credentials.ts | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/credentials.ts b/src/credentials.ts index a4d5791..ef6a10f 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -324,11 +324,10 @@ export async function refreshIfNeeded( if (!target) return null // Pick up credentials replaced externally — cswap switching accounts, the - // claude CLI in another terminal, or a second OpenCode instance. This used - // to be limited to file sources on the assumption that a keychain entry is - // only ever mutated by our own writeBackCredentials; that assumption is - // false. Bounded by getCachedCredentials's 30s TTL, so it fires at most - // ~2x/min under load. + // claude CLI in another terminal, or a second OpenCode instance. This was + // once limited to file sources, on the false assumption that a keychain + // entry is only ever mutated by our own writeBackCredentials. Bounded by + // getCachedCredentials's 30s TTL, so it fires at most ~2x/min under load. // // A keychain read shells out to `security`, which throws when the keychain // is locked, access is denied, or the call times out. Degrade to the @@ -337,18 +336,24 @@ export async function refreshIfNeeded( // Adopt a usable stored blob always; an unusable one only when what we // already hold is unusable too. Do not simplify this to an unconditional // adopt: performRefresh ignores writeBackCredentials's return value, and - // that write can fail while the read before it succeeded (a malformed - // blob, or an ACL allowing 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, since that window is the - // only reason we refreshed. Adopting it re-enters performRefresh with a - // refresh token our own refresh just rotated dead, so OAuth fails and we + // that write can fail while the read before it succeeded (malformed blob, + // or an ACL allowing read but not add-generic-password), leaving memory + // freshly refreshed and the store holding the orphaned pre-refresh blob. + // On the reactive path that blob has under 60s left — that window is the + // only reason we refreshed — so adopting it re-enters performRefresh with + // a refresh token our own refresh just rotated dead: OAuth fails and we // fall through to two 60s claude spawns, on every cache miss, forever. // - // The accepted cost: if an external switch installs an account whose - // stored token is already expired while ours is still usable, we keep ours - // until it expires and adopt the switched-in account then. cswap freshens - // a target before activating it, so a newly-active slot is normally fresh. + // Two accepted residuals. An external switch installing an already-expired + // token while ours is usable is ignored until ours expires; cswap freshens + // a target before activating it, so that is rare. And the proactive timer + // refreshes an hour ahead (index.ts), where a failed write-back orphans a + // blob that is still usable — so it IS adopted, costing wasted background + // refreshes rather than failed requests until it drops under 60s and the + // CLI fallback recovers. No guard here closes that one: the re-read cannot + // tell "stale because our write failed" from "changed because cswap + // switched", as both present as store-disagrees-with-memory-and-usable. + // Only the return value performRefresh discards carries the distinction. try { const stored = refreshAccount(target.source, target.configDir) const now = Date.now() From f36ca88ba65d548f848a55ae8bf3f14dae09a4ab Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:09:34 -0600 Subject: [PATCH 14/33] docs: scope the 60s premise to the reactive path and track the discarded write-back result --- ...-29-external-credential-rotation-design.md | 27 +++++++++++++++---- ...07-29-external-credential-rotation-plan.md | 4 +++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-design.md b/specs/2026-07-29-external-credential-rotation-design.md index b48abd8..36e57b1 100644 --- a/specs/2026-07-29-external-credential-rotation-design.md +++ b/specs/2026-07-29-external-credential-rotation-design.md @@ -114,11 +114,11 @@ in-memory credentials is unsafe, because `performRefresh` ignores `writeBackCredentials`'s return value. When the read succeeds but the write fails — a malformed stored blob, or an ACL allowing read but not `add-generic-password` — memory holds freshly refreshed credentials while the -store holds the pre-refresh blob. That blob has under 60s left, since that -window is the only reason a refresh happened. Adopting it re-enters -`performRefresh` with a refresh token the successful refresh just rotated dead, -so OAuth fails and the code falls through to two 60s `claude` spawns — on every -cache miss, indefinitely, on the authentication path. +store holds the pre-refresh blob. On the reactive path that blob has under 60s +left, since that window is the only reason a refresh happened. Adopting it +re-enters `performRefresh` with a refresh token the successful refresh just +rotated dead, so OAuth fails and the code falls through to two 60s `claude` +spawns — on every cache miss, indefinitely, on the authentication path. The rule: **adopt a usable stored blob always; adopt an unusable one only when what we already hold is also unusable.** @@ -136,6 +136,23 @@ credentials until they expire and adopts the switched-in account then. cswap freshens a target before activating it, so a newly-active slot is normally fresh, and the alternative is the failure loop above. +Second accepted residual, on the proactive path. The background timer refreshes +an hour ahead of expiry, so a write-back failure there orphans a blob with up to +~1h left — which reads as "usable" and is adopted, clobbering the fresh +credentials. The consequence is categorically milder than the reactive case: the +adopted token is genuinely valid, so requests keep succeeding, and the +`CLI_FALLBACK_THRESHOLD_MS` gate in `performRefresh` declines to spawn `claude` +while credentials remain usable. The cost is a handful of wasted background OAuth +round trips over that final hour, resolving in one CLI-fallback recovery once the +blob drops under 60s. + +No guard on the re-read can close this, and that is why it is out of scope here: +the re-read cannot distinguish "the store is stale because our write failed" from +"the store changed because cswap switched" — both present as _store disagrees +with memory, store is usable_. The information exists only at the +`writeBackCredentials` call in `performRefresh`, whose return value is discarded. +See Follow-ups in the plan. + The same branch clears the borrowed-credentials flag. A read from the account's own source returns that account's own credentials, so it is by definition no longer running on a lender's — matching the invariant `refreshBorrowedAccount` diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index b75bf2c..d7b0c88 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1188,6 +1188,10 @@ Consequence today: a file-source account whose OAuth refresh fails goes straight Not fixed here because it changes behavior on the file path, which no current test covers, and it is outside the guard replacement this feature specified. The comment at the guard says so plainly rather than implying the asymmetry is principled. +**`performRefresh` discards `writeBackCredentials`'s return value.** A failed write-back is therefore invisible: memory holds freshly refreshed credentials while the store keeps the pre-refresh blob. On the proactive path that orphaned blob can still have ~1h left, so the validated re-read reads it as usable and adopts it, clobbering the fresh credentials and wasting background OAuth attempts until it drops under 60s. + +This cannot be fixed at the re-read — it cannot distinguish "the store is stale because our write failed" from "the store changed because cswap switched"; both look like _store disagrees with memory, store is usable_. The information exists only at the write-back call site. `forceRefreshActiveAccount` already models the fix, checking the return and logging `force_refresh_writeback_failed`; `performRefresh` does neither. Pre-existing, not introduced by this feature. + ## Manual verification Automated tests stub the Keychain. Verify against real cswap state once: From 2f3ef6930c8469840a55bbc32babfe5b202b14ad Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:14:40 -0600 Subject: [PATCH 15/33] fix: guard credential write-back against an external switch --- src/credentials.test.ts | 50 ++++++++++++++++++++++++++++ src/credentials.ts | 24 ++++++++++++-- src/keychain.test.ts | 73 +++++++++++++++++++++++++++++++++++++++++ src/keychain.ts | 31 +++++++++++++++++ 4 files changed, 175 insertions(+), 3 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index 5ad37c7..ca0d8bc 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -1142,6 +1142,56 @@ describe("credential caching", () => { Date.now = originalNow } }) + + it("performRefresh passes the pre-refresh token as the write-back guard", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + await credentialsModule.refreshIfNeeded() + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].creds.accessToken, "rotated-token") + assert.equal(writes[0].expectedPriorAccessToken, "stale-token") + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } + }) }) describe("syncAuthJson file permissions", () => { diff --git a/src/credentials.ts b/src/credentials.ts index ef6a10f..b3f2db5 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -414,7 +414,12 @@ async function performRefresh( const oauthCreds = await refreshViaOAuth(creds.refreshToken) if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { target.credentials = oauthCreds - writeBackCredentials(target.source, oauthCreds, target.configDir) + writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, + ) return oauthCreds } } @@ -543,7 +548,12 @@ async function refreshBorrowedAccount( if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { borrowedCredentialAccounts.delete(target) target.credentials = oauthCreds - writeBackCredentials(target.source, oauthCreds, target.configDir) + writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + own.accessToken, + ) log("refresh_borrowed_recovered", { source: target.source, via: "oauth" }) return oauthCreds } @@ -666,10 +676,18 @@ export async function forceRefreshActiveAccount( return null } + const priorAccessToken = account.credentials.accessToken const oauthCreds = await refresh(account.credentials.refreshToken) if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { account.credentials = oauthCreds - if (!writeBackCredentials(account.source, oauthCreds)) { + if ( + !writeBackCredentials( + account.source, + oauthCreds, + account.configDir, + priorAccessToken, + ) + ) { // Session continues from memory/cache; a later source re-read may // resurrect the rejected token and trigger another refresh. log("force_refresh_writeback_failed", { source: account.source }) diff --git a/src/keychain.test.ts b/src/keychain.test.ts index 3df07e3..b41e44c 100644 --- a/src/keychain.test.ts +++ b/src/keychain.test.ts @@ -14,6 +14,7 @@ import { join } from "node:path" import { pathToFileURL } from "node:url" import { buildAccountLabels, + credentialBlobMatches, keychainSuffixForDir, parseCredentials, readAllClaudeAccounts as readAllClaudeAccountsReal, @@ -582,6 +583,28 @@ describe("updateCredentialBlob", () => { }) }) +describe("credentialBlobMatches", () => { + const blob = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: "rt-a", + expiresAt: 1, + }, + }) + + it("accepts a blob still holding the expected token", () => { + assert.equal(credentialBlobMatches(blob, "account-a"), true) + }) + + it("rejects a blob replaced by another account", () => { + assert.equal(credentialBlobMatches(blob, "account-b"), false) + }) + + it("rejects an unparseable blob rather than assuming a match", () => { + assert.equal(credentialBlobMatches("not json", "account-a"), false) + }) +}) + describe("writeBackCredentials (file source)", () => { // These tests isolate via HOME; unset CLAUDE_CONFIG_DIR so an ambient value // (e.g. in CI or a dev shell) doesn't redirect the credentials path. @@ -638,6 +661,56 @@ describe("writeBackCredentials (file source)", () => { } }) + it("skips the write when the stored token is no longer the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + // Another process switched accounts after we read "expected-at". + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "switched-in-at", + refreshToken: "switched-in-rt", + expiresAt: 1000, + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + }, + undefined, + "expected-at", + ) + + assert.equal(result, false) + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal( + written.claudeAiOauth.accessToken, + "switched-in-at", + "the switched-in credential must survive untouched", + ) + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + rmSync(tempHome, { recursive: true, force: true }) + } + }) + it("writes file with 0o600 permissions", async () => { if (process.platform === "win32") return diff --git a/src/keychain.ts b/src/keychain.ts index 24c7d99..807ead8 100644 --- a/src/keychain.ts +++ b/src/keychain.ts @@ -439,10 +439,27 @@ function getKeychainAccountName(serviceName: string): string | null { } } +/** + * Whether a stored credential blob still carries the access token we expect. + * + * Guards write-back against an external switch landing between the read that + * produced the token being refreshed and the write of its replacement. An + * unparseable blob returns false: a write into state we cannot identify is + * exactly what this is meant to prevent. + */ +export function credentialBlobMatches( + raw: string, + expectedAccessToken: string, +): boolean { + const parsed = parseCredentials(raw) + return parsed?.accessToken === expectedAccessToken +} + export function writeBackCredentials( source: string, creds: ClaudeCredentials, configDir?: string, + expectedPriorAccessToken?: string, ): boolean { const newCreds = { accessToken: creds.accessToken, @@ -456,6 +473,13 @@ export function writeBackCredentials( configDir ?? process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude") const credPath = join(dir, ".credentials.json") const raw = readFileSync(credPath, "utf-8") + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + log("writeback_skipped_stale", { source, configDir: dir }) + return false + } const updated = updateCredentialBlob(raw, newCreds) if (!updated) return false writeFileSync(credPath, updated, { encoding: "utf-8", mode: 0o600 }) @@ -474,6 +498,13 @@ export function writeBackCredentials( try { const raw = readKeychainService(source) if (!raw) return false + if ( + expectedPriorAccessToken !== undefined && + !credentialBlobMatches(raw, expectedPriorAccessToken) + ) { + log("writeback_skipped_stale", { source }) + return false + } const updated = updateCredentialBlob(raw, newCreds) if (!updated) return false const accountName = getKeychainAccountName(source) ?? source From cd6cba51491611fa6702312d9a58a7964050d5a6 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:29:52 -0600 Subject: [PATCH 16/33] test: pin the write-back guard at every call site 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. --- src/credentials.test.ts | 131 ++++++++++++++++++++++++++++++++++++++++ src/credentials.ts | 8 ++- src/keychain.test.ts | 61 +++++++++++++++++++ src/keychain.ts | 46 +++++++++++++- 4 files changed, 242 insertions(+), 4 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index ca0d8bc..73a0b34 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -864,6 +864,45 @@ describe("credential caching", () => { ) }) + // The token must be captured before the await: account.credentials is + // reassigned to the refreshed pair before write-back runs, so reading it at + // the call site would compare the new token against the store and skip + // every write. + it("forceRefreshActiveAccount guards write-back with the pre-refresh token", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + const account = { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "rejected-token", + refreshToken: "refresh-token", + expiresAt: now + 10 * 60_000, + }, + } + credentialsModule.initAccounts([account]) + + const writesBefore = keychainModule.__getWrites().length + + await credentialsModule.forceRefreshActiveAccount(async () => ({ + accessToken: "oauth-refreshed", + refreshToken: "new-refresh", + expiresAt: now + 10 * 60_000, + })) + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, writesBefore + 1) + const write = writes[writes.length - 1] + assert.equal(write.creds.accessToken, "oauth-refreshed") + assert.equal( + write.expectedPriorAccessToken, + "rejected-token", + "the guard must carry the token the refresh was issued against", + ) + }) + it("forceRefreshActiveAccount returns null and leaves the account untouched on failure", async () => { const now = Date.now() const { credentialsModule } = await loadCredentialsWithCountingKeychain( @@ -1897,6 +1936,98 @@ describe("borrowed fallback credentials", () => { Date.now = originalNow } }) + + // While borrowed, target.credentials holds the LENDER's token, so guarding + // write-back with it would fail the comparison every time and silently stop + // persisting refreshes for exactly the accounts that are recovering. The + // guard must use this account's own stored token. + it("guards a recovering borrower's write-back with its own token, not the lender's", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + + let oauthFails = true + globalThis.fetch = (async () => { + if (oauthFails) throw new Error("network unreachable") + return new Response( + JSON.stringify({ + access_token: "at-recovered", + refresh_token: "rt-recovered", + expires_in: 28_800, + }), + { status: 200 }, + ) + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 30 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // The borrower's own store holds an expired credential distinct from + // anything it has ever had in memory, so the assertion below can only + // pass if the guard came from the store rather than from memory. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + + // OAuth and the CLI both fail (suffixed source, no configDir), so the + // borrower falls back to the lender's credentials. + assert.equal( + (await credentialsModule.refreshIfNeeded(borrower))?.accessToken, + "at-lender", + "borrow should succeed", + ) + + const writesBefore = keychainModule.__getWrites().length + oauthFails = false + + // A threshold wider than the borrowed credential's remaining life + // forces a refresh while leaving it usable, so the up-front re-read + // does not adopt the expired stored blob and clear the borrowed flag. + await credentialsModule.refreshIfNeeded(borrower, 60 * 60_000) + + const writes = keychainModule.__getWrites() + assert.equal(writes.length, writesBefore + 1) + const write = writes[writes.length - 1] + assert.equal(write.creds.accessToken, "at-recovered") + assert.equal( + write.expectedPriorAccessToken, + "at-borrower-own", + "the guard must be the borrower's own stored token", + ) + assert.notEqual( + write.expectedPriorAccessToken, + "at-lender", + "guarding with the lender's token would fail every CAS", + ) + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) }) describe("borrowed credentials after exhaustion", () => { diff --git a/src/credentials.ts b/src/credentials.ts index b3f2db5..cd5dfb1 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -688,8 +688,12 @@ export async function forceRefreshActiveAccount( priorAccessToken, ) ) { - // Session continues from memory/cache; a later source re-read may - // resurrect the rejected token and trigger another refresh. + // Session continues from memory/cache either way, but the two causes + // diverge on a later source re-read. An I/O failure leaves our own + // rejected token in the store, so the re-read resurrects it and + // triggers another refresh. A CAS mismatch means the store now holds + // another account's token, so the re-read adopts that instead and this + // account stops using the credentials it just refreshed. log("force_refresh_writeback_failed", { source: account.source }) } accountCacheMap.set(account.source, { diff --git a/src/keychain.test.ts b/src/keychain.test.ts index b41e44c..7a9236b 100644 --- a/src/keychain.test.ts +++ b/src/keychain.test.ts @@ -603,6 +603,20 @@ describe("credentialBlobMatches", () => { it("rejects an unparseable blob rather than assuming a match", () => { assert.equal(credentialBlobMatches("not json", "account-a"), false) }) + + // The guard validates through parseCredentials; the write it protects uses + // updateCredentialBlob, which would rewrite this blob happily. Pinning the + // stricter side so the divergence is a decision rather than an accident. + it("rejects a blob whose token matches but whose other fields are malformed", () => { + const malformed = JSON.stringify({ + claudeAiOauth: { + accessToken: "account-a", + refreshToken: 12345, + expiresAt: "soon", + }, + }) + assert.equal(credentialBlobMatches(malformed, "account-a"), false) + }) }) describe("writeBackCredentials (file source)", () => { @@ -661,6 +675,53 @@ describe("writeBackCredentials (file source)", () => { } }) + it("writes when the stored token still matches the expected one", async () => { + const originalHome = process.env.HOME + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) + process.env.HOME = tempHome + + try { + const claudeDir = join(tempHome, ".claude") + mkdirSync(claudeDir, { recursive: true }) + const credPath = join(claudeDir, ".credentials.json") + writeFileSync( + credPath, + JSON.stringify({ + claudeAiOauth: { + accessToken: "old-at", + refreshToken: "old-rt", + expiresAt: 1000, + subscriptionType: "pro", + }, + }), + { encoding: "utf-8", mode: 0o600 }, + ) + + const result = writeBackCredentials( + "file", + { + accessToken: "new-at", + refreshToken: "new-rt", + expiresAt: 2000, + }, + undefined, + "old-at", + ) + + assert.equal(result, true, "a matching guard must not block the write") + const written = JSON.parse(readFileSync(credPath, "utf-8")) + assert.equal(written.claudeAiOauth.accessToken, "new-at") + assert.equal(written.claudeAiOauth.subscriptionType, "pro") + } finally { + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + rmSync(tempHome, { recursive: true, force: true }) + } + }) + it("skips the write when the stored token is no longer the expected one", async () => { const originalHome = process.env.HOME const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-cas-")) diff --git a/src/keychain.ts b/src/keychain.ts index 807ead8..4aa9332 100644 --- a/src/keychain.ts +++ b/src/keychain.ts @@ -439,6 +439,15 @@ function getKeychainAccountName(serviceName: string): string | null { } } +/** + * A short, non-reversible tag for an access token, so write-back decisions + * can be correlated across log lines (and against another account's) without + * putting token material in the log. + */ +function tokenFingerprint(token: string): string { + return createHash("sha256").update(token).digest("hex").slice(0, 8) +} + /** * Whether a stored credential blob still carries the access token we expect. * @@ -446,6 +455,16 @@ function getKeychainAccountName(serviceName: string): string | null { * produced the token being refreshed and the write of its replacement. An * unparseable blob returns false: a write into state we cannot identify is * exactly what this is meant to prevent. + * + * Deliberately stricter than the write it guards: this validates through + * parseCredentials (field types, mcpOAuth-only rejection) while + * updateCredentialBlob rewrites anything JSON.parse accepts. So a blob the + * write path would happily update can still be refused here. That gap is + * intentional — reaching this guard means an earlier parseCredentials + * succeeded, so a blob that no longer parses changed shape after we read it + * (a truncated non-atomic external write, or a hand edit), and declining to + * write over it is correct. Anyone tightening parseCredentials should know it + * narrows write-back too, not just reads. */ export function credentialBlobMatches( raw: string, @@ -477,7 +496,20 @@ export function writeBackCredentials( expectedPriorAccessToken !== undefined && !credentialBlobMatches(raw, expectedPriorAccessToken) ) { - log("writeback_skipped_stale", { source, configDir: dir }) + // Two operationally opposite causes: another account legitimately + // holds the slot (benign, no action) versus a blob we cannot read at + // all (corruption, needs a human). Same refusal, different events. + const stored = parseCredentials(raw) + if (stored === null) { + log("writeback_skipped_unparseable", { source, configDir: dir }) + } else { + log("writeback_skipped_stale", { + source, + configDir: dir, + expected: tokenFingerprint(expectedPriorAccessToken), + stored: tokenFingerprint(stored.accessToken), + }) + } return false } const updated = updateCredentialBlob(raw, newCreds) @@ -502,7 +534,17 @@ export function writeBackCredentials( expectedPriorAccessToken !== undefined && !credentialBlobMatches(raw, expectedPriorAccessToken) ) { - log("writeback_skipped_stale", { source }) + // See the file branch: a mismatch is expected, an unreadable blob is not. + const stored = parseCredentials(raw) + if (stored === null) { + log("writeback_skipped_unparseable", { source }) + } else { + log("writeback_skipped_stale", { + source, + expected: tokenFingerprint(expectedPriorAccessToken), + stored: tokenFingerprint(stored.accessToken), + }) + } return false } const updated = updateCredentialBlob(raw, newCreds) From fd6fcd2a4cd0f8ccb243fde343805b11d49d54ca Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:30:56 -0600 Subject: [PATCH 17/33] docs: record the configDir write-back fix and check off Task 3 --- ...07-29-external-credential-rotation-plan.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index d7b0c88..257a406 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -312,7 +312,7 @@ Closes the window between reading a credential and writing its refreshed replace - Modify: `src/credentials.ts:376`, `:499`, `:622-628` - Test: `src/keychain.test.ts` -- [ ] **Step 1: Write the failing tests** +- [x] **Step 1: Write the failing tests** Add `credentialBlobMatches` to the existing import from `./keychain.ts` at `src/keychain.test.ts:15-23`. @@ -395,12 +395,12 @@ it("skips the write when the stored token is no longer the expected one", async }) ``` -- [ ] **Step 2: Run to verify they fail** +- [x] **Step 2: Run to verify they fail** Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -12` Expected: FAIL — `credentialBlobMatches is not a function`, and the skip test writes anyway (`result` is `true`). -- [ ] **Step 3: Implement the guard** +- [x] **Step 3: Implement the guard** In `src/keychain.ts`, add above `writeBackCredentials` (line 442): @@ -457,12 +457,12 @@ if ( } ``` -- [ ] **Step 4: Run to verify they pass** +- [x] **Step 4: Run to verify they pass** Run: `node --test --experimental-strip-types --test-name-pattern="credentialBlobMatches|no longer the expected one" src/keychain.test.ts 2>&1 | tail -8` Expected: `pass 4`, `fail 0` -- [ ] **Step 5: Pass the expected token from all three refresh call sites** +- [x] **Step 5: Pass the expected token from all three refresh call sites** `src/credentials.ts:376` — `creds` is the pre-refresh credential captured by `performRefresh`: @@ -503,7 +503,7 @@ writeBackCredentials( ) { ``` -- [ ] **Step 6: Add the write-back assertion test** +- [x] **Step 6: Add the write-back assertion test** Append inside the top-level `describe` in `src/credentials.test.ts`: @@ -559,12 +559,12 @@ it("performRefresh passes the pre-refresh token as the write-back guard", async }) ``` -- [ ] **Step 7: Run the full suite** +- [x] **Step 7: Run the full suite** Run: `pnpm test 2>&1 | tail -8` -Expected: `pass 256`, `fail 0` +Expected: `pass 259`, `fail 0` (later extended to 263 by review follow-ups) -- [ ] **Step 8: Commit** +- [x] **Step 8: Commit** ```bash git add src/keychain.ts src/keychain.test.ts src/credentials.ts src/credentials.test.ts @@ -1160,6 +1160,7 @@ Add under the topmost unreleased heading in `CHANGELOG.md` (create `## Unrelease - Pick up credentials rotated by another process (claude-swap, the `claude` CLI, a second OpenCode instance) in a live session instead of staying pinned to the account that was active at startup - Never write a refreshed token into a slot that was switched underneath us - Force an OAuth refresh when a request is rejected and the store still holds the rejected token, so the request recovers in place +- Write refreshed credentials back to the account's own config directory on the force-refresh path, which previously fell back to the default directory and could write to the wrong file for a file-source account with a custom `CLAUDE_CONFIG_DIR` ### Behavior Changes From 32041d822d75b6a4aa51831f10de1d074b23eb37 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:45:59 -0600 Subject: [PATCH 18/33] fix: recover from a rejected token by forcing an OAuth refresh --- src/index.test.ts | 195 +++++++++++++++++++++++++++++++++++++++++++--- src/index.ts | 70 ++++++++++++----- 2 files changed, 236 insertions(+), 29 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index bd54d6c..2725f73 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1186,7 +1186,7 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } }) - it("auth fetch does not retry a 401 when the source token is unchanged", async () => { + it("auth fetch force-refreshes on a 401 when the source token is unchanged", async () => { const originalNow = Date.now const originalSetInterval = globalThis.setInterval const originalHome = process.env.HOME @@ -1198,15 +1198,100 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account unref() {}, })) as unknown as typeof setInterval - let requestCount = 0 + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "recovered-token", + refresh_token: "rt-recovered", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + return apiCalls === 1 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(oauthCalls, 1, "the unchanged token must force a refresh") + assert.equal(apiCalls, 2, "the refreshed token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 const errorBody = '{"name":"mcp_UnchangedToken"}' try { const { helpersModule } = await loadHelpersWithCountingKeychain( Date.now() + 10 * 60_000, ) - globalThis.fetch = (async () => { - requestCount += 1 + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + // A 429 from the token endpoint must read as a failed refresh, not + // as something that loops account recovery. retry-after beyond the + // 30s cap makes fetchWithRetry return instead of backing off. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + apiCalls += 1 return new Response(errorBody, { status: 401, headers: { "x-request-id": "unchanged-token-401" }, @@ -1215,7 +1300,6 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account const plugin = await helpersModule.default({} as never) const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } - assert.equal(typeof typedPlugin.auth?.loader, "function") const authConfig = await typedPlugin.auth!.loader!( async () => ({ type: "oauth", @@ -1237,7 +1321,88 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account assert.equal(response.status, 401) assert.equal(response.headers.get("x-request-id"), "unchanged-token-401") assert.equal(await response.text(), errorBody) - assert.equal(requestCount, 1) + assert.equal(apiCalls, 1, "no retry without a different token") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch makes a second recovery attempt when the first retry is also rejected", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + let oauthCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response( + JSON.stringify({ + access_token: "second-stage-token", + refresh_token: "rt-second-stage", + expires_in: 36_000, + }), + { status: 200 }, + ) + } + apiCalls += 1 + // Calls 1 and 2 are rejected; only the force-refreshed token works. + return apiCalls <= 2 + ? new Response('{"error":"expired"}', { status: 401 }) + : new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Attempt 1's reload finds a different, still-valid-looking token. + keychainModule.__setCredentials({ + accessToken: "concurrently-rotated", + refreshToken: "rt-concurrent", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 3, "original, reload retry, force-refresh retry") + assert.equal(oauthCalls, 1, "only the second attempt force-refreshes") } finally { Date.now = originalNow globalThis.setInterval = originalSetInterval @@ -1262,7 +1427,8 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account unref() {}, })) as unknown as typeof setInterval - let requestCount = 0 + let apiCalls = 0 + let oauthCalls = 0 const errorBody = '{"name":"mcp_ReloadFailure"}' try { @@ -1270,8 +1436,16 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account Date.now() + 10 * 60_000, { throwOnReload: true }, ) - globalThis.fetch = (async () => { - requestCount += 1 + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : String(input) + // A thrown reload leaves no candidate, so the loop falls through to + // the force refresh. Rejecting it here is what makes recovery + // terminal and surfaces the original 401 untouched. + if (url.includes("/oauth/token")) { + oauthCalls += 1 + return new Response('{"error":"invalid_grant"}', { status: 401 }) + } + apiCalls += 1 return new Response(errorBody, { status: 401, statusText: "Unauthorized", @@ -1308,7 +1482,8 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account assert.equal(response.headers.get("content-type"), "text/plain") assert.equal(response.headers.get("x-request-id"), "request-401") assert.equal(await response.text(), errorBody) - assert.equal(requestCount, 1) + assert.equal(apiCalls, 1, "no retry once recovery cannot progress") + assert.equal(oauthCalls, 1, "a thrown reload still forces a refresh") } finally { Date.now = originalNow globalThis.setInterval = originalSetInterval diff --git a/src/index.ts b/src/index.ts index 2538e77..943df70 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,6 +20,7 @@ import { import { getCachedCredentials, reloadCredentialsFromSource, + forceRefreshActiveAccount, getActiveAccount, syncAuthJson, initAccounts, @@ -365,30 +366,58 @@ const plugin: Plugin = async () => { retryAttempt: 0, }) - // On 401, bypass the in-memory cache to pick up credentials rotated by - // another client, then retry once only when the access token changed. - let preserveResponseUnchanged = false - if (response.status === 401) { - let refreshed: ClaudeCredentials | null = null + // Recover from a rejected token: first by adopting credentials + // rotated externally (cswap switching accounts, the claude CLI, + // another OpenCode instance), then by forcing an OAuth refresh + // when the store still holds the token that was just rejected. + // + // Most cases resolve on the first attempt: a cold or unreadable + // store yields null from the reload (reloadCredentialsFromSource + // rejects anything expiring within 60s), so the force refresh runs + // immediately. The second attempt covers the narrower race where + // the reload returns a valid-looking token that a concurrent + // writer has itself just rotated again. The no-progress break is + // what actually bounds this — the cap is defence in depth. + const MAX_AUTH_RECOVERY_ATTEMPTS = 2 + let tokenInUse = latest.accessToken + + for ( + let attempt = 0; + response.status === 401 && attempt < MAX_AUTH_RECOVERY_ATTEMPTS; + attempt++ + ) { + let candidate: ClaudeCredentials | null = null try { - refreshed = reloadCredentialsFromSource() + candidate = reloadCredentialsFromSource() } catch {} - if (refreshed && refreshed.accessToken !== latest.accessToken) { - const retryHeaders = buildRequestHeaders( + + if (!candidate || candidate.accessToken === tokenInUse) { + try { + candidate = await forceRefreshActiveAccount() + } catch {} + } + + if (!candidate || candidate.accessToken === tokenInUse) { + log("auth_recovery_exhausted", { + modelId, + attempt: attempt + 1, + }) + break + } + + tokenInUse = candidate.accessToken + log("auth_recovery_retry", { modelId, attempt: attempt + 1 }) + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( input, requestInit, - refreshed.accessToken, + tokenInUse, modelId, excluded, - ) - response = await fetchWithRetry(requestUrl, { - ...requestInit, - body, - headers: retryHeaders, - }) - } else { - preserveResponseUnchanged = true - } + ), + }) } // Check for long-context beta errors and retry with betas excluded @@ -459,7 +488,10 @@ const plugin: Plugin = async () => { .catch(() => {}) } - return preserveResponseUnchanged + // A 401 that survived recovery carries an error body, not an SSE + // stream. Deciding here rather than from a flag set mid-flight + // makes the retried and non-retried paths behave identically. + return response.status === 401 ? response : transformResponseStream(response) }, From 1b0a1757f04d8bd70df40f378d90fc751e1effbe Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 13:51:29 -0600 Subject: [PATCH 19/33] docs: correct Task 4 failure prediction and record the transform follow-up --- ...07-29-external-credential-rotation-plan.md | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 257a406..6fdf163 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -580,7 +580,7 @@ git commit -m "fix: guard credential write-back against an external switch" - Modify: `src/index.ts:368-392` (401 block), `:462-464` (return), imports at `:20-31` - Test: `src/index.test.ts:1189-1251` -- [ ] **Step 1: Import the currently-dead force-refresh helper** +- [x] **Step 1: Import the currently-dead force-refresh helper** In the `from "./credentials.ts"` import block in `src/index.ts` (around line 20-31), add: @@ -588,7 +588,7 @@ In the `from "./credentials.ts"` import block in `src/index.ts` (around line 20- forceRefreshActiveAccount, ``` -- [ ] **Step 2: Replace the 401 block with the recovery loop** +- [x] **Step 2: Replace the 401 block with the recovery loop** Replace `src/index.ts:368-392` in full: @@ -648,7 +648,7 @@ for ( } ``` -- [ ] **Step 3: Derive the stream-transform decision from the final status** +- [x] **Step 3: Derive the stream-transform decision from the final status** Replace `src/index.ts:462-464`: @@ -659,14 +659,17 @@ Replace `src/index.ts:462-464`: return response.status === 401 ? response : transformResponseStream(response) ``` -- [ ] **Step 4: Run the suite to see the expected failure** +- [x] **Step 4: Run the suite to see the expected failure** Run: `pnpm test 2>&1 | grep -E "^not ok|✖" | head` -Expected: exactly one failure — `auth fetch does not retry a 401 when the source token is unchanged` (`src/index.test.ts:1189`). Its name describes the behavior being replaced. +Expected: **two** failures. -The neighbouring test around `src/index.test.ts:1141` (a 401 recovered by an externally rotated token) and the one at `:1253` (reload throws) must both still pass — the loop preserves those paths. If either fails, the loop is wrong, not the test. +1. `auth fetch does not retry a 401 when the source token is unchanged` — its name describes the behavior being replaced. +2. `auth fetch preserves the original 401 when credential reload throws` — fails **only** on its request-count assertion. Its mock counts every `fetch` without discriminating the OAuth token endpoint, so the new force-refresh call inflates the count from 1 to 2. Every behavioral assertion (status, statusText, `content-type`, `x-request-id`, body) still passes, and the API is still hit exactly once. Fix by splitting the mock into `apiCalls` / `oauthCalls`, keeping all existing assertions and adding `oauthCalls === 1` — a thrown reload must still force a refresh. -- [ ] **Step 5: Replace that test with the two behaviors that supersede it** +The neighbouring test that recovers a 401 via an externally rotated token must still pass — the loop preserves that path. If it fails, the loop is wrong, not the test. + +- [x] **Step 5: Replace that test with the two behaviors that supersede it** Replace the whole `it(...)` block at `src/index.test.ts:1189-1251` with: @@ -820,7 +823,7 @@ it("auth fetch surfaces the 401 unchanged when recovery cannot progress", async }) ``` -- [ ] **Step 6: Add the test that justifies a second attempt** +- [x] **Step 6: Add the test that justifies a second attempt** This is the only scenario where one attempt is insufficient: the reload returns a valid-looking token that a concurrent writer has itself just rotated again, so the first retry is also rejected. Append after the two tests from Step 5: @@ -907,12 +910,12 @@ it("auth fetch makes a second recovery attempt when the first retry is also reje }) ``` -- [ ] **Step 7: Run the full suite** +- [x] **Step 7: Run the full suite** Run: `pnpm test 2>&1 | tail -8` -Expected: `pass 258`, `fail 0` +Expected: `pass 265`, `fail 0` (actual, measured — earlier tasks landed extra tests) -- [ ] **Step 8: Commit** +- [x] **Step 8: Commit** ```bash git add src/index.ts src/index.test.ts @@ -1193,6 +1196,8 @@ Not fixed here because it changes behavior on the file path, which no current te This cannot be fixed at the re-read — it cannot distinguish "the store is stale because our write failed" from "the store changed because cswap switched"; both look like _store disagrees with memory, store is usable_. The information exists only at the write-back call site. `forceRefreshActiveAccount` already models the fix, checking the return and logging `force_refresh_writeback_failed`; `performRefresh` does neither. Pre-existing, not introduced by this feature. +**`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which does not preserve headers and body faithfully — surfaced while proving a Task 4 test non-vacuous. Pre-existing and unchanged by this feature, but it means a non-401 API error may reach the user altered. Worth its own investigation. + ## Manual verification Automated tests stub the Keychain. Verify against real cswap state once: From 62f5cc60cc59d868401d7ce2325ab62a09d26fd2 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:06:42 -0600 Subject: [PATCH 20/33] refactor: log auth recovery failures and correct the loop's bound comment --- src/index.ts | 48 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/index.ts b/src/index.ts index 943df70..4fa2a9e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -376,8 +376,19 @@ const plugin: Plugin = async () => { // rejects anything expiring within 60s), so the force refresh runs // immediately. The second attempt covers the narrower race where // the reload returns a valid-looking token that a concurrent - // writer has itself just rotated again. The no-progress break is - // what actually bounds this — the cap is defence in depth. + // writer has itself just rotated again. + // + // The cap is the real bound. Against a store being rotated on + // every read, every candidate differs from tokenInUse, so the + // no-progress break never fires and the cap alone stops the loop. + // The break is the fast path out of the common cases, not the + // guarantee of termination. + // + // tokenInUse deliberately tracks only the last token tried rather + // than the set of all of them: a store cycling A->B->A wastes one + // request on the second attempt, which is a better trade than + // threading extra state through a loop whose whole virtue is a + // hard ceiling of three API calls. const MAX_AUTH_RECOVERY_ATTEMPTS = 2 let tokenInUse = latest.accessToken @@ -387,16 +398,41 @@ const plugin: Plugin = async () => { attempt++ ) { let candidate: ClaudeCredentials | null = null + // reloadCredentialsFromSource already catches its own source + // read and returns null, so this is unreachable today. It stays + // because the guarantee worth keeping is that no reload failure + // turns a well-formed 401 into an exception thrown out of + // fetch() — degrading to the original response beats crashing + // the request. It logs so a future reload that does throw is + // diagnosable rather than silently null-coalesced. try { candidate = reloadCredentialsFromSource() - } catch {} + } catch (err) { + log("auth_recovery_reload_threw", { + modelId, + attempt: attempt + 1, + error: err instanceof Error ? err.message : String(err), + }) + } if (!candidate || candidate.accessToken === tokenInUse) { try { candidate = await forceRefreshActiveAccount() - } catch {} + } catch (err) { + // A rejected refresh and a refresh that returned null are + // different operator-facing diagnoses; auth_recovery_ + // exhausted below collapses them, so record this one here. + log("auth_recovery_force_refresh_threw", { + modelId, + attempt: attempt + 1, + error: err instanceof Error ? err.message : String(err), + }) + } } + // Re-checked, not copy-pasted: the guard above decides whether + // to force a refresh, this one decides whether that refresh + // actually produced a token worth retrying with. if (!candidate || candidate.accessToken === tokenInUse) { log("auth_recovery_exhausted", { modelId, @@ -450,8 +486,10 @@ const plugin: Plugin = async () => { }) // Rebuild headers without the excluded beta and retry + // Falls back to tokenInUse, not latest: after a 401 recovery the + // latter is the token the API already rejected. const currentCreds = await getCachedCredentials() - const retryToken = currentCreds?.accessToken ?? latest.accessToken + const retryToken = currentCreds?.accessToken ?? tokenInUse const newExcluded = getExcludedBetas(modelId) const newHeaders = buildRequestHeaders( input, From a413e539b9a72023a27c8dce4c4ecea23bdee4ce Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:07:24 -0600 Subject: [PATCH 21/33] docs: correct the transformResponseStream follow-up premise --- specs/2026-07-29-external-credential-rotation-plan.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 6fdf163..3138f70 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1196,7 +1196,7 @@ Not fixed here because it changes behavior on the file path, which no current te This cannot be fixed at the re-read — it cannot distinguish "the store is stale because our write failed" from "the store changed because cswap switched"; both look like _store disagrees with memory, store is usable_. The information exists only at the write-back call site. `forceRefreshActiveAccount` already models the fix, checking the return and logging `force_refresh_writeback_failed`; `performRefresh` does neither. Pre-existing, not introduced by this feature. -**`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which does not preserve headers and body faithfully — surfaced while proving a Task 4 test non-vacuous. Pre-existing and unchanged by this feature, but it means a non-401 API error may reach the user altered. Worth its own investigation. +**`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which strips tool-name prefixes from the body. The transform's `!ok` branch already passes status and headers through intact, so the practical difference for a 401 is only that prefix stripping — the carve-out is narrower than "the transform mangles errors". Still asymmetric: the comment's rationale ("carries an error body, not an SSE stream") applies equally to 400/429/500, which do go through it. Pre-existing and unchanged by this feature. Worth deciding whether the bypass should cover all `!ok` statuses rather than 401 alone. ## Manual verification From dcd70a8eb11908fa01e05526c24bc887bea6bb6d Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:11:15 -0600 Subject: [PATCH 22/33] fix: re-read credentials on a 429 so a resolved rate limit is not surfaced --- src/index.test.ts | 134 ++++++++++++++++++++++++++++++++++++++++++++++ src/index.ts | 49 +++++++++++++++++ 2 files changed, 183 insertions(+) diff --git a/src/index.test.ts b/src/index.test.ts index 2725f73..fe7a077 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1580,6 +1580,140 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } } }) + + it("auth fetch retries a 429 once when the source token changed", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + if (apiCalls === 1) { + // retry-after beyond the 30s cap: quota exhaustion, not a + // transient limit, so fetchWithRetry returns immediately. + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + } + return new Response("data: {}\n\n", { status: 200 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // An external switch lands while this session is still on the + // exhausted account. + keychainModule.__setCredentials({ + accessToken: "switched-token", + refreshToken: "rt-switched", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 200) + assert.equal(apiCalls, 2, "the rotated token must be retried once") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch does not retry a 429 when the source token is unchanged", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule } = await loadHelpersWithCountingKeychain( + Date.now() + 10 * 60_000, + ) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unchanged token must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) }) describe("auth hook — account resolution", () => { diff --git a/src/index.ts b/src/index.ts index 4fa2a9e..37b4349 100644 --- a/src/index.ts +++ b/src/index.ts @@ -456,6 +456,55 @@ const plugin: Plugin = async () => { }) } + // An external switch — cswap rotating off an exhausted account — + // leaves this session on the old token until the 30s credential + // cache expires. Re-read once so a rate limit that has already + // been resolved elsewhere is not surfaced. A changed token is the + // signal that a switch happened; when nothing changed this costs + // one source read and no retry. + // + // Ordered before the long-context beta loop deliberately. A + // long-context 429 is a header problem, not an account one, so it + // rotates no token and falls through here untouched. In the rare + // case a switch lands on the same 429, this spends one retry that + // comes back with the same long-context error and the beta loop + // then handles it off the fresh response — one wasted request, + // same outcome. + if (response.status === 429) { + let rotated: ClaudeCredentials | null = null + // Unreachable today for the same reason as the 401 loop's + // reload catch: reloadCredentialsFromSource swallows its own + // source read and returns null. Kept, and logged, on the same + // grounds — no reload failure should turn a readable 429 into + // an exception thrown out of fetch(), and a future reload that + // does throw should be diagnosable rather than silently + // coalesced to "nothing rotated". + try { + rotated = reloadCredentialsFromSource() + } catch (err) { + log("rate_limit_reload_threw", { + modelId, + error: err instanceof Error ? err.message : String(err), + }) + } + + if (rotated && rotated.accessToken !== tokenInUse) { + log("rate_limit_credentials_rotated", { modelId }) + tokenInUse = rotated.accessToken + response = await fetchWithRetry(requestUrl, { + ...requestInit, + body, + headers: buildRequestHeaders( + input, + requestInit, + tokenInUse, + modelId, + excluded, + ), + }) + } + } + // Check for long-context beta errors and retry with betas excluded // Try up to LONG_CONTEXT_BETAS.length times, excluding one more beta each time for ( From 14955e1ae1d1b8faedd7568073b88ad833545f9d Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:21:24 -0600 Subject: [PATCH 23/33] test: pin the 429 retry to the rotated token and cover the unreadable-source arm --- src/index.test.ts | 89 ++++++++++++++++++++++++++++++++++++++++++++--- src/index.ts | 27 +++++++++++++- 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index fe7a077..fa0b150 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1593,15 +1593,20 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account unref() {}, })) as unknown as typeof setInterval - let apiCalls = 0 + // Captured per call, not just counted: a count alone cannot tell a + // retry that carried the rotated token from one that replayed the + // exhausted token, which is the whole behavior under test. + const authorizationHeaders: string[] = [] try { const { helpersModule, keychainModule } = await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) - globalThis.fetch = (async () => { - apiCalls += 1 - if (apiCalls === 1) { + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + if (authorizationHeaders.length === 1) { // retry-after beyond the 30s cap: quota exhaustion, not a // transient limit, so fetchWithRetry returns immediately. return new Response('{"error":"rate_limited"}', { @@ -1641,7 +1646,11 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account ) assert.equal(response.status, 200) - assert.equal(apiCalls, 2, "the rotated token must be retried once") + assert.deepEqual( + authorizationHeaders, + ["Bearer token", "Bearer switched-token"], + "the retry must carry the rotated token, not replay the exhausted one", + ) } finally { Date.now = originalNow globalThis.setInterval = originalSetInterval @@ -1714,6 +1723,76 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } } }) + + it("auth fetch does not retry a 429 when the source is unreadable", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + let apiCalls = 0 + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async () => { + apiCalls += 1 + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // Distinct from the unchanged-token arm: the token DID change, but + // reloadCredentialsFromSource rejects anything expiring within 60s, + // so it yields null. Retrying with a token the reload refused to + // vouch for would burn a request on a credential already known bad. + keychainModule.__setCredentials({ + accessToken: "expiring-token", + refreshToken: "rt-expiring", + expiresAt: Date.now() + 30_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.equal(apiCalls, 1, "an unusable reload must not be retried") + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) }) describe("auth hook — account resolution", () => { diff --git a/src/index.ts b/src/index.ts index 37b4349..fff9c6c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -463,6 +463,16 @@ const plugin: Plugin = async () => { // signal that a switch happened; when nothing changed this costs // one source read and no retry. // + // Ordered AFTER the 401 recovery loop, and that is a real + // dependency, not incidental sequencing: it must compare against + // the token the loop last tried, so a 401 recovered into a 429 is + // measured against the recovered token rather than the rejected + // one. Only half of this is compiler-enforced — hoisting the block + // above `let tokenInUse` is a TDZ error, but moving it between + // that declaration and the loop still compiles and still passes, + // while silently comparing against a stale token on the + // 401 -> retry -> 429 path. + // // Ordered before the long-context beta loop deliberately. A // long-context 429 is a header problem, not an account one, so it // rotates no token and falls through here untouched. In the rare @@ -489,7 +499,14 @@ const plugin: Plugin = async () => { } if (rotated && rotated.accessToken !== tokenInUse) { - log("rate_limit_credentials_rotated", { modelId }) + // Named for what was observed, not for what it implies. A + // changed token is not proof of an account switch: a routine + // refresh of this same exhausted account by another instance + // or the claude CLI changes the token too, and that retry hits + // the same quota. Accepted cost — one request — but the log + // must not tell a quota investigation "we switched accounts" + // when all it saw was a different token. + log("rate_limit_token_changed", { modelId }) tokenInUse = rotated.accessToken response = await fetchWithRetry(requestUrl, { ...requestInit, @@ -502,6 +519,14 @@ const plugin: Plugin = async () => { excluded, ), }) + // Whether rotating resolved the limit is the question this + // whole block exists to answer, so record it outright rather + // than leaving success to be inferred from the absence of a + // fetch_error_response line. + log("rate_limit_retry_response", { + modelId, + status: response.status, + }) } } From af5f93894d82c309dbb6bd27aa7a15a757359170 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:30:04 -0600 Subject: [PATCH 24/33] docs: document external credential rotation handling --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index b53d0fb..4979a26 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,11 @@ export ANTHROPIC_CLI_VERSION=2.2.0 - Provides an account switcher via `opencode auth login` when multiple accounts are found; persists selection to `~/.local/share/opencode/claude-account-source.txt` - Syncs credentials to `auth.json` on startup and every 5 minutes as a fallback; that same tick proactively refreshes once the token is within an hour of expiry - On Windows, writes to both `%USERPROFILE%\.local\share\opencode\auth.json` and `%LOCALAPPDATA%\opencode\auth.json` +- Re-reads the credential source on every cache miss, so an account rotated by something other than this plugin — the `claude` CLI in another terminal, a second OpenCode instance, or a switcher like [claude-swap](https://github.com/realiti4/claude-swap) — gets picked up mid-session without a restart. Bounded by the same 30s cache, so it adds at most about two source reads a minute under load. A stored token is adopted whenever it is usable, and when it isn't only if the one already held is also unusable — otherwise a failed write-back would resurrect the pre-refresh token it left behind +- Guards credential write-back with the access token the refresh started from, so a switch landing mid-refresh can't write one account's rotated tokens into another account's slot - Retries API requests on 429 (rate limit) and 529 (overloaded) with exponential backoff, respecting `retry-after` headers +- On a 429 that outlives those backoff retries, re-reads the source once and retries only if the access token changed, so a rate limit another process has already resolved by switching accounts isn't surfaced. A changed token isn't proof of a switch — a routine refresh of the same account changes it too — so this costs at most one extra request +- On a 401, recovers in place rather than surfacing it: adopts an externally rotated token if the source now holds one, otherwise forces an OAuth refresh, then retries the request. Bounded at two attempts, so a rejected token costs at most three API calls. A 401 that survives recovery is returned unmodified, without SSE stream transformation, since it carries an error body rather than a stream - Refreshes directly via `POST https://claude.ai/v1/oauth/token` using the runtime's own `fetch` (no LLM tokens consumed, no subprocess). Requests are triggered within 60 seconds of expiry on the API request path and within an hour on the background tick; concurrent refreshes of one account share a single request, since each rotation invalidates the previous refresh token - Falls back to the `claude` CLI only within the 60-second window, the point at which Claude Code will actually rotate the token — running it earlier costs a real API request and returns the same token. New tokens are written back to Keychain (macOS) or credentials file (Linux/Windows) to keep stored credentials in sync with rotated refresh tokens - If credentials aren't OAuth-based, the auth loader returns `{}` and falls through to API key auth From 38244628ef453dc944ece069ebbd60e7e6c7cb92 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:54:07 -0600 Subject: [PATCH 25/33] fix: clear the borrowed flag when a source reload adopts own credentials --- src/credentials.test.ts | 90 +++++++++++++++++++++++++++++++++++++++++ src/credentials.ts | 7 ++++ 2 files changed, 97 insertions(+) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index 73a0b34..fa1d4a7 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -2259,6 +2259,96 @@ describe("borrowed credentials after exhaustion", () => { } }) + // The 401 recovery loop reaches its second attempt only through + // reloadCredentialsFromSource, so that adoption site has to clear the flag + // for the same reason refreshIfNeeded's does — matching the invariant + // refreshBorrowedAccount maintains at every other adoption site. Without + // it, a borrower whose own entry has since been repaired reloads onto its + // own credentials, gets rejected again, and then force-refresh declines on + // a flag that is false in fact: a recoverable 401 reaches the user. + it("clears the borrowed flag once a source reload adopts its own credentials", async () => { + const originalFetch = globalThis.fetch + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + globalThis.fetch = (async () => { + throw new Error("network unreachable") + }) as typeof fetch + + try { + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now - 1_000) + + const borrower = { + label: "Account 1", + source: "Claude Code-credentials-aabbccdd", + credentials: { + accessToken: "at-borrower", + refreshToken: "rt-borrower", + expiresAt: now - 1_000, + }, + } + const lender = { + label: "Account 2", + source: "Claude Code-credentials", + credentials: { + accessToken: "at-lender", + refreshToken: "rt-lender", + expiresAt: now + 60 * 60_000, + }, + } + credentialsModule.initAccounts([borrower, lender]) + + // Its own entry is expired, so it ends up on the lender's tokens. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-own", + refreshToken: "rt-borrower-own", + expiresAt: now - 1_000, + }) + await credentialsModule.refreshIfNeeded(borrower) + assert.equal( + borrower.credentials.accessToken, + "at-lender", + "precondition: the account is running on borrowed tokens", + ) + + // An external process repairs its entry, and the 401 handler picks that + // up through the reload rather than through refreshIfNeeded. + keychainModule.__setCredentialsForSource(borrower.source, { + accessToken: "at-borrower-fresh", + refreshToken: "rt-borrower-fresh", + expiresAt: now + 8 * 60 * 60_000, + }) + assert.equal( + credentialsModule.reloadCredentialsFromSource()?.accessToken, + "at-borrower-fresh", + "precondition: the reload adopts the account's own credentials", + ) + + const seen: string[] = [] + const forced = await credentialsModule.forceRefreshActiveAccount( + async (token: string) => { + seen.push(token) + return { + accessToken: "at-forced", + refreshToken: "rt-forced", + expiresAt: now + 8 * 60 * 60_000, + } + }, + ) + + assert.deepEqual( + seen, + ["rt-borrower-fresh"], + "an account reloaded onto its own token must be free to force-refresh it", + ) + assert.equal(forced?.accessToken, "at-forced") + } finally { + globalThis.fetch = originalFetch + Date.now = originalNow + } + }) + it("forceRefreshActiveAccount refuses to exchange a borrowed token", async () => { const originalFetch = globalThis.fetch const originalNow = Date.now diff --git a/src/credentials.ts b/src/credentials.ts index cd5dfb1..47814de 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -791,6 +791,13 @@ export function reloadCredentialsFromSource(): ClaudeCredentials | null { } account.credentials = reloaded + // Read from this account's own source, so what it returned is this + // account's own credentials — it is no longer running on a lender's. + // Same invariant as refreshIfNeeded's up-front re-read: leaving the flag + // set here makes forceRefreshActiveAccount decline to exchange a token + // that is legitimately this account's, which strands the 401 recovery + // loop's second attempt on a credential it could have refreshed. + borrowedCredentialAccounts.delete(account) accountCacheMap.set(account.source, { creds: reloaded, cachedAt: now }) log("credentials_source_reload", { source: account.source, From 616930aae0513deeed747f88b9e49bc443ff7af9 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:56:28 -0600 Subject: [PATCH 26/33] test: pin the keychain-branch write-back guard --- src/keychain.test.ts | 142 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 123 insertions(+), 19 deletions(-) diff --git a/src/keychain.test.ts b/src/keychain.test.ts index 7a9236b..be32e64 100644 --- a/src/keychain.test.ts +++ b/src/keychain.test.ts @@ -24,10 +24,7 @@ import { } from "./keychain.ts" // Mirrors listClaudeKeychainServices regex logic for unit testing -async function loadKeychainWithMockedSecurity( - securityDump: string, - keychainEntries: Record, -): Promise<{ +type MockedKeychain = { readAllClaudeAccounts: () => Array<{ label: string source: string @@ -39,7 +36,19 @@ async function loadKeychainWithMockedSecurity( subscriptionType?: string } }> -}> { + writeBackCredentials: ( + source: string, + creds: { accessToken: string; refreshToken: string; expiresAt: number }, + configDir?: string, + expectedPriorAccessToken?: string, + ) => boolean + __getSecurityWrites: () => Array<{ service: string; value: string }> +} + +async function loadKeychainWithMockedSecurity( + securityDump: string, + keychainEntries: Record, +): Promise { const tempDir = await mkdtemp( join(tmpdir(), "opencode-claude-auth-keychain-"), ) @@ -82,11 +91,23 @@ export function execSync(command) { throw new Error("unexpected execSync call: " + command) } +const securityWrites = [] + export function execFileSync(file, args) { if (file !== "/usr/bin/security") { throw new Error("unexpected execFileSync file: " + file) } const service = args[args.indexOf("-s") + 1] + + // add-generic-password is the write path. Record it and mutate the backing + // store, so a test can tell "the write was skipped" from "the write ran". + if (args[0] === "add-generic-password") { + const value = args[args.indexOf("-w") + 1] + securityWrites.push({ service, value }) + keychainEntries[service] = value + return "" + } + const raw = keychainEntries[service] if (raw === undefined) { const error = new Error("The specified item could not be found in the keychain.") @@ -96,25 +117,23 @@ export function execFileSync(file, args) { } return raw } + +export function __getSecurityWrites() { + return securityWrites +} `, "utf8", ) await writeFile(tempKeychain, rewritten, "utf8") - const keychainModule = await import(pathToFileURL(tempKeychain).href) - return keychainModule as { - readAllClaudeAccounts: () => Array<{ - label: string - source: string - configDir?: string - credentials: { - accessToken: string - refreshToken: string - expiresAt: number - subscriptionType?: string - } - }> - } + const [keychainModule, childProcessModule] = await Promise.all([ + import(pathToFileURL(tempKeychain).href), + import(pathToFileURL(tempChildProcess).href), + ]) + return { + ...keychainModule, + __getSecurityWrites: childProcessModule.__getSecurityWrites, + } as MockedKeychain } function extractServicesFromDump(output: string): string[] { @@ -910,6 +929,91 @@ describe("writeBackCredentials (file source)", () => { }) }) +describe("writeBackCredentials (keychain source)", () => { + // The keychain is the primary platform and the reason the compare-and-swap + // guard exists, so it needs its own coverage: the file-source tests cannot + // reach this branch, and `process.platform` is rewritten to "darwin" by the + // harness so these run on any host. + const SERVICE = "Claude Code-credentials" + const DUMP = `"${SERVICE}"` + + const blob = (accessToken: string) => + JSON.stringify({ + claudeAiOauth: { + accessToken, + refreshToken: `rt-for-${accessToken}`, + expiresAt: 1000, + subscriptionType: "max", + }, + }) + + const refreshed = { + accessToken: "our-refreshed-at", + refreshToken: "our-refreshed-rt", + expiresAt: 2000, + } + + it("skips the write when the stored token is no longer the expected one", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + // Another account was switched in after we read "expected-at". + [SERVICE]: blob("switched-in-at"), + }) + + const result = writeBackCredentials( + SERVICE, + refreshed, + undefined, + "expected-at", + ) + + assert.equal(result, false) + assert.deepEqual( + __getSecurityWrites(), + [], + "the switched-in credential must survive untouched", + ) + }) + + it("writes when the stored token still matches the expected one", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + [SERVICE]: blob("expected-at"), + }) + + const result = writeBackCredentials( + SERVICE, + refreshed, + undefined, + "expected-at", + ) + + assert.equal(result, true) + const writes = __getSecurityWrites() + assert.equal(writes.length, 1) + assert.equal(writes[0].service, SERVICE) + const written = JSON.parse(writes[0].value) as { + claudeAiOauth: { accessToken: string; subscriptionType: string } + } + assert.equal(written.claudeAiOauth.accessToken, "our-refreshed-at") + assert.equal( + written.claudeAiOauth.subscriptionType, + "max", + "unrelated fields must be preserved", + ) + }) + + it("writes without a guard when no expected token is supplied", async () => { + const { writeBackCredentials, __getSecurityWrites } = + await loadKeychainWithMockedSecurity(DUMP, { + [SERVICE]: blob("whatever-is-there"), + }) + + assert.equal(writeBackCredentials(SERVICE, refreshed), true) + assert.equal(__getSecurityWrites().length, 1) + }) +}) + describe("readAllClaudeAccounts (file source)", () => { it("reads credentials from CLAUDE_CONFIG_DIR when set", async () => { // Non-darwin reads go straight to the credentials file; on darwin the From 544f9ac8b283f9d03f55d71d5a53f9a21ea802a4 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:58:58 -0600 Subject: [PATCH 27/33] test: pin the recovery attempt cap and the 429 token comparison --- src/index.test.ts | 156 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 156 insertions(+) diff --git a/src/index.test.ts b/src/index.test.ts index fa0b150..9c356a3 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -1581,6 +1581,162 @@ export function buildAccountLabels(creds) { return creds.map((_, i) => \`Account } }) + it("auth fetch stops at the recovery attempt cap when the store rotates on every read", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + // Against a store rotated on every read, every candidate differs from the + // token in use, so the no-progress break never fires and the attempt cap + // is the only thing stopping the loop. Nothing else pins that ceiling — + // the two-attempt test exits via success, so it constrains the floor. + const authorizationHeaders: string[] = [] + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + // Rotate the store before answering, so the next reload always finds + // a token it has not seen. + keychainModule.__setCredentials({ + accessToken: `rotated-${authorizationHeaders.length}`, + refreshToken: `rt-${authorizationHeaders.length}`, + expiresAt: Date.now() + 10 * 60_000, + }) + return new Response('{"error":"expired"}', { status: 401 }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 401) + assert.equal( + authorizationHeaders.length, + 3, + "original request plus two capped recovery attempts, and no more", + ) + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + + it("auth fetch compares the 429 re-read against the recovered token, not the original", async () => { + const originalNow = Date.now + const originalSetInterval = globalThis.setInterval + const originalHome = process.env.HOME + const originalFetch = globalThis.fetch + const tempHome = await mkdtemp(join(tmpdir(), "opencode-claude-auth-home-")) + process.env.HOME = tempHome + Date.now = () => 1_700_000_000_000 + globalThis.setInterval = (() => ({ + unref() {}, + })) as unknown as typeof setInterval + + // 401 -> recover -> the retry comes back 429. The 429 block must compare + // the source against the token it just recovered with. Comparing against + // the original `latest` instead still compiles and passes every other + // test, while retrying a quota error on a token the source already agrees + // is current — the exact hazard the block's own comment predicts. + const authorizationHeaders: string[] = [] + + try { + const { helpersModule, keychainModule } = + await loadHelpersWithCountingKeychain(Date.now() + 10 * 60_000) + + globalThis.fetch = (async (_input, init) => { + authorizationHeaders.push( + new Headers(init?.headers).get("authorization") ?? "", + ) + if (authorizationHeaders.length === 1) { + return new Response('{"error":"expired"}', { status: 401 }) + } + return new Response('{"error":"rate_limited"}', { + status: 429, + headers: { "retry-after": "3600" }, + }) + }) as typeof fetch + + const plugin = await helpersModule.default({} as never) + const typedPlugin = plugin as { auth?: { loader?: TestAuthLoader } } + const authConfig = await typedPlugin.auth!.loader!( + async () => ({ + type: "oauth", + refresh: "refresh", + access: "access", + expires: Date.now() + 60_000, + }), + { models: {} }, + ) + + // The store already holds the token the 401 recovery will adopt, so + // once recovered there is nothing further to rotate to. + keychainModule.__setCredentials({ + accessToken: "recovered-token", + refreshToken: "rt-recovered", + expiresAt: Date.now() + 10 * 60_000, + }) + + const response = await authConfig.fetch( + "https://api.anthropic.com/v1/messages", + { + method: "POST", + body: JSON.stringify({ model: "claude-haiku-4-5", messages: [] }), + }, + ) + + assert.equal(response.status, 429) + assert.deepEqual( + authorizationHeaders, + ["Bearer token", "Bearer recovered-token"], + "the 429 must not be retried once the source agrees with the token in use", + ) + } finally { + Date.now = originalNow + globalThis.setInterval = originalSetInterval + globalThis.fetch = originalFetch + if (typeof originalHome === "string") { + process.env.HOME = originalHome + } else { + delete process.env.HOME + } + } + }) + it("auth fetch retries a 429 once when the source token changed", async () => { const originalNow = Date.now const originalSetInterval = globalThis.setInterval From 9501d222b81678314cb1f76715af6637e1bad6b6 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:59:18 -0600 Subject: [PATCH 28/33] docs: record deferred findings from the final review --- specs/2026-07-29-external-credential-rotation-plan.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 3138f70..3ecca5b 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1138,12 +1138,12 @@ git commit -m "fix: re-read credentials on a 429 so a resolved rate limit is not - Modify: `README.md`, `CHANGELOG.md` -- [ ] **Step 1: Lint and format** +- [x] **Step 1: Lint and format** Run: `pnpm run lint:fix && pnpm run lint` Expected: clean exit, no diagnostics. -- [ ] **Step 2: Add the behavior to the README technical list** +- [x] **Step 2: Add the behavior to the README technical list** In `README.md`, under `## How it works (technical)`, insert after the bullet beginning "Syncs credentials to `auth.json`": @@ -1153,7 +1153,7 @@ In `README.md`, under `## How it works (technical)`, insert after the bullet beg - On a rejected token, adopts an externally rotated credential and otherwise forces an OAuth refresh, retrying the request in place rather than surfacing the 401 ``` -- [ ] **Step 3: Add a CHANGELOG entry** +- [x] **Step 3: Add a CHANGELOG entry** Add under the topmost unreleased heading in `CHANGELOG.md` (create `## Unreleased` above the newest release heading if absent): @@ -1170,12 +1170,12 @@ Add under the topmost unreleased heading in `CHANGELOG.md` (create `## Unrelease - A 401 that survives recovery is now returned without SSE stream transformation, matching what a non-retried 401 already did ``` -- [ ] **Step 4: Verify the full suite and a clean tree** +- [x] **Step 4: Verify the full suite and a clean tree** Run: `pnpm test 2>&1 | tail -8 && git status --short` Expected: `fail 0`; only `README.md` and `CHANGELOG.md` modified. -- [ ] **Step 5: Commit** +- [x] **Step 5: Commit** ```bash git add README.md CHANGELOG.md From d50866be8e521ca9e3fadf3c2da14f0e4dba826a Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 16:59:44 -0600 Subject: [PATCH 29/33] docs: record deferred findings from the final review --- specs/2026-07-29-external-credential-rotation-plan.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 3ecca5b..e5f2032 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1198,6 +1198,16 @@ This cannot be fixed at the re-read — it cannot distinguish "the store is stal **`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which strips tool-name prefixes from the body. The transform's `!ok` branch already passes status and headers through intact, so the practical difference for a 401 is only that prefix stripping — the carve-out is narrower than "the transform mangles errors". Still asymmetric: the comment's rationale ("carries an error body, not an SSE stream") applies equally to 400/429/500, which do go through it. Pre-existing and unchanged by this feature. Worth deciding whether the bypass should cover all `!ok` statuses rather than 401 alone. +**`configDir` is honored on one re-read path but not the others.** `refreshIfNeeded` passes `target.configDir` to `refreshAccount`; `reloadCredentialsFromSource` and `reloadActiveAccount` do not. Not reachable today — every file account is assigned `CLAUDE_CONFIG_DIR ?? ~/.claude`, exactly what `readCredentialsFile` recomputes by default — but latent, and the compare-and-swap guard now depends on the read and the write resolving to the same file. + +**`reloadActiveAccount` and `invalidateCredentialCache` are still dead, with now-false docstrings.** Both claim to be "used on 401". The 401 path uses `reloadCredentialsFromSource` instead. This feature revived the third of the three dead functions the design doc identified and left these two behind. Either wire them up or delete them. + +**No negative caching of a terminal auth failure.** On a server-side-revoked credential the store keeps returning a locally-unexpired token, so every request pays the full recovery loop — three API calls plus an OAuth exchange plus `security` spawns — indefinitely, where before it paid one request. Worst-case cost per `fetch()` is now roughly 18 Anthropic requests and several minutes of capped backoff, bounded by the caller's abort signal but with no deadline of the plugin's own. + +**The log redactor protects structured fields only.** `logger.ts` redacts by key name or an anchored `^eyJ` whole-value match, so a token embedded mid-string in an `err.message` would pass through. Unreachable today — every callee either catches internally or throws fixed strings — but two new sites log `err.message`. + +**Narrow in-memory orphaned-rotation race.** If the proactive timer's refresh is in flight while a request-path re-read adopts an externally written blob, the timer's OAuth result overwrites `target.credentials` while its write-back guard correctly refuses. Both tokens are usable so nothing breaks, but a rotation is consumed and stored nowhere. Requires an external write inside a 15s OAuth round trip plus two concurrent callers. The store-side variant is covered above; this in-memory one is distinct. + ## Manual verification Automated tests stub the Keychain. Verify against real cswap state once: From 5805947e048543d9aee6ca6eae55572ead2e0059 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 17:00:00 -0600 Subject: [PATCH 30/33] docs: check off completed Task 5 and 6 steps --- .../2026-07-29-external-credential-rotation-plan.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index e5f2032..979ff10 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -931,7 +931,7 @@ git commit -m "fix: recover from a rejected token by forcing an OAuth refresh" - Modify: `src/index.ts` — insert after the 401 recovery loop, before the long-context beta loop (`:396`) - Test: `src/index.test.ts` -- [ ] **Step 1: Write the failing test** +- [x] **Step 1: Write the failing test** Append inside the same `describe` as Task 4's tests: @@ -1073,12 +1073,12 @@ it("auth fetch does not retry a 429 when the source token is unchanged", async ( `loadHelpersWithCountingKeychain` already returns `keychainModule` with `__setCredentials` (`src/index.test.ts:264-273`), so no harness change is needed. -- [ ] **Step 2: Run to verify the first fails and the second passes** +- [x] **Step 2: Run to verify the first fails and the second passes** Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -12` Expected: the "token changed" test FAILS with `apiCalls` 1 ≠ 2; the "unchanged" test passes (no retry exists yet). -- [ ] **Step 3: Implement the 429 re-read** +- [x] **Step 3: Implement the 429 re-read** Insert in `src/index.ts` directly after the 401 recovery loop from Task 4 and before the long-context beta loop: @@ -1113,17 +1113,17 @@ if (response.status === 429) { } ``` -- [ ] **Step 4: Run to verify both pass** +- [x] **Step 4: Run to verify both pass** Run: `node --test --experimental-strip-types --test-name-pattern="429" src/index.test.ts 2>&1 | tail -8` Expected: `fail 0` -- [ ] **Step 5: Run the full suite** +- [x] **Step 5: Run the full suite** Run: `pnpm test 2>&1 | tail -8` Expected: `pass 260`, `fail 0` -- [ ] **Step 6: Commit** +- [x] **Step 6: Commit** ```bash git add src/index.ts src/index.test.ts From b080afc44d8cbc112bc05db698b995526b516981 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 19:46:26 -0600 Subject: [PATCH 31/33] refactor: forward configDir on the re-read paths and log a rejected write-back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/credentials.ts | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/credentials.ts b/src/credentials.ts index 47814de..35ead1d 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -414,12 +414,22 @@ async function performRefresh( const oauthCreds = await refreshViaOAuth(creds.refreshToken) if (oauthCreds && oauthCreds.expiresAt > Date.now() + 60_000) { target.credentials = oauthCreds - writeBackCredentials( - target.source, - oauthCreds, - target.configDir, - creds.accessToken, - ) + if ( + !writeBackCredentials( + target.source, + oauthCreds, + target.configDir, + creds.accessToken, + ) + ) { + // Mirrors force_refresh_writeback_failed on the forced path. The + // session continues from memory either way, so this stays a log + // rather than a control-flow change: acting on the two causes + // (I/O failure vs. CAS mismatch) differs, and the proactive-path + // consequence — a still-usable orphaned blob being re-adopted by + // the validated re-read — is tracked as a follow-up. + log("refresh_writeback_failed", { source: target.source }) + } return oauthCreds } } @@ -635,15 +645,21 @@ export function getCredentialsForSync(): ClaudeCredentials | null { /** * Re-read only the active account's credentials from its source (single - * keychain service read or credentials file) and update them in place. - * Used on 401 so an externally refreshed token is picked up without a - * full multi-account keychain rescan. + * keychain service read or credentials file) and update them in place, + * so an externally refreshed token is picked up without a full + * multi-account keychain rescan. + * + * Currently has no call sites: the 401 path uses + * reloadCredentialsFromSource, which additionally validates the result + * and refreshes the cache. Wiring this up or deleting it is tracked as a + * follow-up; until then it must stay consistent with the read paths that + * are live, hence the configDir below. */ export function reloadActiveAccount(): void { const account = getActiveAccount() if (!account) return try { - const fresh = refreshAccount(account.source) + const fresh = refreshAccount(account.source, account.configDir) if (fresh) account.credentials = fresh } catch (err) { log("account_reload_failed", { @@ -761,7 +777,9 @@ export function reloadCredentialsFromSource(): ClaudeCredentials | null { let reloaded: ClaudeCredentials | null try { - reloaded = refreshAccount(account.source) + // Same configDir the write path resolves, so the compare-and-swap in + // writeBackCredentials compares against the file this read came from. + reloaded = refreshAccount(account.source, account.configDir) } catch { accountCacheMap.delete(account.source) log("credentials_source_reload", { From c5332c3a078a6671ced12e15c5251c256a3df151 Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 19:46:33 -0600 Subject: [PATCH 32/33] test: pin configDir forwarding and the rejected write-back log 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. --- src/credentials.test.ts | 170 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 167 insertions(+), 3 deletions(-) diff --git a/src/credentials.test.ts b/src/credentials.test.ts index fa1d4a7..5b7186e 100644 --- a/src/credentials.test.ts +++ b/src/credentials.test.ts @@ -67,11 +67,16 @@ async function loadCredentialsWithCountingKeychain( configDir?: string expectedPriorAccessToken?: string }> + __getReads: () => Array<{ source: string; configDir?: string }> + __setWriteResult: (v: boolean) => void } childProcessModule: { __getExecFileSyncCount: () => number __getExecSyncCount: () => number } + loggerModule: { + __getLogs: () => Array<{ event: string; data?: unknown }> + } }> { const tempDir = await mkdtemp(join(tmpdir(), "opencode-claude-auth-creds-")) const tempKeychain = join(tempDir, "keychain.ts") @@ -99,7 +104,16 @@ async function loadCredentialsWithCountingKeychain( await writeFile( tempLogger, - `export function log() {}\nexport function initLogger() {}\nexport function closeLogger() {}\n`, + `const logs = [] +export function log(event, data) { + logs.push({ event, data }) +} +export function __getLogs() { + return logs +} +export function initLogger() {} +export function closeLogger() {} +`, "utf8", ) @@ -134,6 +148,8 @@ export function __getExecSyncCount() { `let readCount = 0 let writeCount = 0 const writes = [] +const reads = [] +let writeResult = true let accounts = null // null = derive a single account from the credentials var let readError = false let readHook = null @@ -152,8 +168,9 @@ export function readAllClaudeAccounts() { return [{ label: "Account 1", source: "keychain", credentials }] } -export function refreshAccount(source) { +export function refreshAccount(source, configDir) { readCount += 1 + reads.push({ source, configDir }) if (readError) throw new Error("Keychain read denied") if (readHook) readHook() if (Object.prototype.hasOwnProperty.call(bySource, source)) { @@ -173,13 +190,21 @@ export function __setReadHook(hook) { export function writeBackCredentials(source, creds, configDir, expectedPriorAccessToken) { writeCount += 1 writes.push({ source, creds, configDir, expectedPriorAccessToken }) - return true + return writeResult +} + +export function __setWriteResult(v) { + writeResult = v } export function __getWrites() { return writes } +export function __getReads() { + return reads +} + export function __getReadCount() { return readCount } @@ -216,6 +241,7 @@ export function __setAccounts(list) { import(pathToFileURL(tempKeychain).href), import(pathToFileURL(tempChildProcess).href), ]) + const loggerModule = await import(pathToFileURL(tempLogger).href) return { credentialsModule: credentialsModule as { @@ -252,11 +278,16 @@ export function __setAccounts(list) { configDir?: string expectedPriorAccessToken?: string }> + __getReads: () => Array<{ source: string; configDir?: string }> + __setWriteResult: (v: boolean) => void }, childProcessModule: childProcessModule as { __getExecFileSyncCount: () => number __getExecSyncCount: () => number }, + loggerModule: loggerModule as { + __getLogs: () => Array<{ event: string; data?: unknown }> + }, } } @@ -816,6 +847,73 @@ describe("credential caching", () => { assert.equal(account.credentials.accessToken, "rotated") }) + // Every other refreshAccount call site in credentials.ts forwards the + // account's configDir; these two re-read paths did not. Unreachable while + // readAllClaudeAccounts assigns every file account + // `CLAUDE_CONFIG_DIR ?? ~/.claude` — exactly what readCredentialsFile + // recomputes by default — but the CAS guard added here compares a read + // against a write, so the two must resolve the same file by construction + // rather than by coincidence. + it("reloadCredentialsFromSource reads the account's own configDir", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "file", + configDir: "/custom/config/dir", + credentials: { + accessToken: "token", + refreshToken: "refresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + const readsBefore = keychainModule.__getReads().length + credentialsModule.reloadCredentialsFromSource() + + const reads = keychainModule.__getReads().slice(readsBefore) + assert.ok(reads.length > 0, "expected a source read") + assert.equal( + reads[reads.length - 1].configDir, + "/custom/config/dir", + "the re-read must target the account's own config directory", + ) + }) + + it("reloadActiveAccount reads the account's own configDir", async () => { + const now = Date.now() + const { credentialsModule, keychainModule } = + await loadCredentialsWithCountingKeychain(now + 10 * 60_000) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "file", + configDir: "/custom/config/dir", + credentials: { + accessToken: "token", + refreshToken: "refresh", + expiresAt: now + 10 * 60_000, + }, + }, + ]) + + const readsBefore = keychainModule.__getReads().length + credentialsModule.reloadActiveAccount() + + const reads = keychainModule.__getReads().slice(readsBefore) + assert.ok(reads.length > 0, "expected a source read") + assert.equal( + reads[reads.length - 1].configDir, + "/custom/config/dir", + "the re-read must target the account's own config directory", + ) + }) + it("forceRefreshActiveAccount swaps in OAuth-refreshed credentials and writes back", async () => { const now = Date.now() const { credentialsModule, keychainModule } = @@ -1231,6 +1329,72 @@ describe("credential caching", () => { globalThis.fetch = originalFetch } }) + + // forceRefreshActiveAccount logs force_refresh_writeback_failed on the same + // condition; performRefresh discarded the return value entirely, so a + // rejected write-back left no trace anywhere in the debug log. The session + // continues from memory either way — this pins the observability, not a + // control-flow change. + it("performRefresh logs a rejected write-back", async () => { + const originalNow = Date.now + const now = 1_700_000_000_000 + Date.now = () => now + const originalFetch = globalThis.fetch + + try { + const { credentialsModule, keychainModule, loggerModule } = + await loadCredentialsWithCountingKeychain(now - 60_000) + + keychainModule.__setCredentials({ + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }) + + credentialsModule.initAccounts([ + { + label: "Account 1", + source: "keychain", + credentials: { + accessToken: "stale-token", + refreshToken: "rt-stale", + expiresAt: now - 60_000, + }, + }, + ]) + + // An external switch landed inside the OAuth round trip, so the CAS in + // writeBackCredentials rejects the write. + keychainModule.__setWriteResult(false) + + globalThis.fetch = (async () => + new Response( + JSON.stringify({ + access_token: "rotated-token", + refresh_token: "rt-rotated", + expires_in: 36_000, + }), + { status: 200 }, + )) as typeof fetch + + const result = await credentialsModule.refreshIfNeeded() + + assert.equal( + result?.accessToken, + "rotated-token", + "the caller still receives the refreshed credentials", + ) + assert.ok( + loggerModule + .__getLogs() + .some((entry) => entry.event === "refresh_writeback_failed"), + "a rejected write-back must be visible in the debug log", + ) + } finally { + Date.now = originalNow + globalThis.fetch = originalFetch + } + }) }) describe("syncAuthJson file permissions", () => { From 4311539a33508236c82d964223e0acfb58d3588e Mon Sep 17 00:00:00 2001 From: Griffin Martin Date: Wed, 29 Jul 2026 19:46:33 -0600 Subject: [PATCH 33/33] docs: close the configDir follow-up and scope the write-back one 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. --- specs/2026-07-29-external-credential-rotation-plan.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/specs/2026-07-29-external-credential-rotation-plan.md b/specs/2026-07-29-external-credential-rotation-plan.md index 979ff10..366db42 100644 --- a/specs/2026-07-29-external-credential-rotation-plan.md +++ b/specs/2026-07-29-external-credential-rotation-plan.md @@ -1196,11 +1196,13 @@ Not fixed here because it changes behavior on the file path, which no current te This cannot be fixed at the re-read — it cannot distinguish "the store is stale because our write failed" from "the store changed because cswap switched"; both look like _store disagrees with memory, store is usable_. The information exists only at the write-back call site. `forceRefreshActiveAccount` already models the fix, checking the return and logging `force_refresh_writeback_failed`; `performRefresh` does neither. Pre-existing, not introduced by this feature. +Partially closed in review: `performRefresh` now checks the return and logs `refresh_writeback_failed`, so the failure is no longer invisible. The remaining half — not re-adopting an orphaned-but-still-usable blob on the proactive path — is a control-flow change that interacts with the adoption logic this feature introduced, and stays deferred. + **`transformResponseStream` is applied to non-401 error responses.** Task 4 scopes the bypass to 401 only, matching the prior behavior for that status. But 4xx and 5xx responses other than 401 still flow through the SSE transform, which strips tool-name prefixes from the body. The transform's `!ok` branch already passes status and headers through intact, so the practical difference for a 401 is only that prefix stripping — the carve-out is narrower than "the transform mangles errors". Still asymmetric: the comment's rationale ("carries an error body, not an SSE stream") applies equally to 400/429/500, which do go through it. Pre-existing and unchanged by this feature. Worth deciding whether the bypass should cover all `!ok` statuses rather than 401 alone. -**`configDir` is honored on one re-read path but not the others.** `refreshIfNeeded` passes `target.configDir` to `refreshAccount`; `reloadCredentialsFromSource` and `reloadActiveAccount` do not. Not reachable today — every file account is assigned `CLAUDE_CONFIG_DIR ?? ~/.claude`, exactly what `readCredentialsFile` recomputes by default — but latent, and the compare-and-swap guard now depends on the read and the write resolving to the same file. +**~~`configDir` is honored on one re-read path but not the others.~~ Closed in review.** `refreshIfNeeded` passed `target.configDir` to `refreshAccount`; `reloadCredentialsFromSource` and `reloadActiveAccount` did not. Never reachable — 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 agreeing on one file load-bearing, so both paths now forward it by construction rather than by coincidence. Pinned by a test per path. -**`reloadActiveAccount` and `invalidateCredentialCache` are still dead, with now-false docstrings.** Both claim to be "used on 401". The 401 path uses `reloadCredentialsFromSource` instead. This feature revived the third of the three dead functions the design doc identified and left these two behind. Either wire them up or delete them. +**`reloadActiveAccount` and `invalidateCredentialCache` are still dead.** Both docstrings claimed to be "used on 401"; `reloadActiveAccount`'s now records that it has no call sites, `invalidateCredentialCache`'s is still wrong. The 401 path uses `reloadCredentialsFromSource` instead. This feature revived the third of the three dead functions the design doc identified and left these two behind. Either wire them up or delete them. **No negative caching of a terminal auth failure.** On a server-side-revoked credential the store keeps returning a locally-unexpired token, so every request pays the full recovery loop — three API calls plus an OAuth exchange plus `security` spawns — indefinitely, where before it paid one request. Worst-case cost per `fetch()` is now roughly 18 Anthropic requests and several minutes of capped backoff, bounded by the caller's abort signal but with no deadline of the plugin's own.