fix: refresh OAuth tokens with native fetch instead of a subprocess - #258
Conversation
Greptile SummaryThis PR replaces the broken OAuth token refresh subprocess path (
Confidence Score: 5/5Safe to merge. All changed code paths are well-exercised by 240 passing tests, including targeted regressions for the three failure modes (subprocess vs. fetch, early CLI spawn, concurrent refreshes). The core rewrite (subprocess → fetch) is straightforward and the async propagation is consistent across every call site. The borrowed-credential guard handles every recovery branch: own-source recovery, own-token OAuth recovery, re-borrow, and exhaustion with correct state restoration. The in-flight deduplication is correctly keyed and scoped to one synchronous turn, so there is no window for a second caller to install a duplicate entry. No pre-existing behaviour is silently changed — the CLI fallback remains, just bounded to the window where it is actually effective. Files Needing Attention: No files require special attention. The most complex logic is in src/credentials.ts (refreshBorrowedAccount and the inFlightRefreshes guard), and it is covered by dedicated tests for each branch.
|
| Filename | Overview |
|---|---|
| src/credentials.ts | Core credential management rewritten as async; refreshViaOAuth replaced subprocess with fetch, in-flight deduplication added, CLI fallback correctly gated on 60s expiry window, and borrowed-credential guard implemented throughout. |
| src/http.ts | New module extracting fetchWithRetry (previously in index.ts) and adding sleepUnlessAborted so the AbortController timeout in refreshViaOAuth can interrupt retry backoff. |
| src/credentials.test.ts | Extensive new tests for fetch-based OAuth, concurrency deduplication, CLI fallback scoping, and borrowed-credential isolation; test harness made hermetic by stubbing child_process. |
| src/index.ts | All call sites of getCachedCredentials/refreshIfNeeded awaited correctly; sync timer callback made async; fetchWithRetry import moved from local definition to http.ts. |
| src/index.test.ts | Made hermetic by stubbing child_process in copySourceFiles; timer tick callbacks awaited; new test for fetchWithRetry abort-during-backoff added. |
| scripts/test-models.ts | getCachedCredentials call awaited to match the now-async signature. |
Sequence Diagram
sequenceDiagram
participant Timer as Sync Timer (5min)
participant Request as API Request Path
participant RIN as refreshIfNeeded
participant IFM as inFlightRefreshes
participant PR as performRefresh
participant OAuth as refreshViaOAuth (fetch)
participant CLI as claude CLI (execSync)
participant KC as Keychain/Store
Note over Timer,KC: Proactive refresh (1h threshold)
Timer->>RIN: refreshIfNeeded(undefined, 3600s)
RIN->>IFM: get(source) → null (no in-flight)
RIN->>PR: performRefresh(target, creds)
PR->>OAuth: refreshViaOAuth(refreshToken)
OAuth->>KC: POST /v1/oauth/token (fetch + AbortController 15s)
Note over Request,KC: Concurrent request during proactive refresh
Request->>RIN: refreshIfNeeded(account, 60s)
RIN->>RIN: "creds.expiresAt > now+60s → return creds early"
OAuth-->>PR: new ClaudeCredentials
PR->>KC: writeBackCredentials
PR-->>RIN: oauthCreds
RIN->>IFM: delete(source)
RIN-->>Timer: fresh credentials
Note over Timer,KC: OAuth fails within 60s reactive window
PR->>KC: refreshAccount (check external rotation)
KC-->>PR: null
PR->>CLI: execSync(claude -p . --model haiku)
CLI-->>PR: success
PR->>KC: refreshAccount
KC-->>PR: fresh credentials
Note over Timer,KC: Borrowed account refresh
PR->>PR: borrowedCredentialAccounts.has(target)
PR->>KC: refreshAccount(target.source)
KC-->>PR: own expired creds with refreshToken
PR->>OAuth: refreshViaOAuth(own.refreshToken)
OAuth-->>PR: new credentials for borrower
PR->>KC: writeBackCredentials(target.source, ownCreds)
Reviews (6): Last reviewed commit: "test: pin borrowed state to the account ..." | Re-trigger Greptile
Both paths could still present a lender's refresh token as the borrower's and persist the result to the borrower's store. refreshBorrowedAccount cleared the guard on the exhaustion path while target.credentials still held the lender's tokens, so the next cycle took the normal path and exchanged them. It now restores the account's own credentials when they are readable and otherwise keeps the guard. forceRefreshActiveAccount exchanged account.credentials.refreshToken with no borrowed check, so a forced refresh on a borrowed account rotated the lender's token. It now declines and leaves recovery to refreshIfNeeded. Reported by Greptile on #258.
fetchWithRetry slept between attempts with a bare setTimeout, so a caller that bounded its request with an AbortController got the retry delay (capped at 30s) as its effective timeout instead. refreshViaOAuth sets a 15s deadline, and a 429 carrying retry-after made it wait far past that, then spend two more attempts that could only fail. The backoff now resolves early on abort and returns the rate-limit response instead of retrying. Also documents that the post-loop request is reachable only when retries < 1, rather than leaving it looking like dead code. Reported by Greptile on #258.
refreshViaOAuth ran its request inside a child process spawned as `process.execPath -e <script>`, which assumes process.execPath is a JavaScript runtime. That does not hold inside OpenCode: the plugin runs in a compiled single-file executable, so process.execPath is the OpenCode binary and `-e` is not a script to evaluate. The call exited status 1 with empty stdout, stderr was discarded via stdio "ignore", and the error was only log()ged, so the direct OAuth refresh failed silently on every attempt and always fell through to spawning the claude CLI. Node 18+ and Bun both expose a global fetch, so the subprocess is unnecessary. The refresh chain becomes async as a result: getCachedCredentials, refreshIfNeeded, refreshViaOAuth and forceRefreshActiveAccount now return promises. These are reachable from the package entry point but are internal to the plugin in practice, so this ships as a patch rather than a major. This also scopes the CLI fallback to the reactive window. The claude CLI only rotates a token that is itself near expiry, so invoking it an hour ahead (as the proactive sync tick does) spent a real API request per tick and returned the same token, then reported success. It now runs only within 60 seconds of expiry and still-usable credentials are returned untouched. Concurrent refreshes of one account are collapsed into a single request. The sync timer calls refreshIfNeeded directly while the request path arrives through getCachedCredentials; because each rotation invalidates the refresh token it was issued against, the two racing left one caller holding an already-dead token. The existing tests could not catch this: `node --test` sets process.execPath to the node binary, where `-e` works.
refreshViaOAuth takes an optional timeoutMs so the abort path can be tested without waiting the full 15 seconds. Default behaviour is unchanged. Without it, a stalled response was accepted after 2s instead of surfacing as null. Also pins the deduplication behaviour when a refresh exhausts: callers that joined a failed attempt all observe null, and the retry round they trigger collapses into a single request rather than one per caller.
…very Three defects surfaced while validating the native-fetch refresh against a live account. Borrowed credentials were persisted onto the borrowing account. When a refresh exhausted, tryFallbackAccount handed back another account's tokens and refreshIfNeeded assigned them to target.credentials. Once those neared expiry the borrower exchanged the LENDER's refresh token and wrote the rotated result into its OWN store, destroying its credentials and rotating the lender's token out from under it. Borrowed accounts are now tracked and routed through a path that re-reads their own source, refreshes only with their own token, and writes back only their own result. This was previously unreachable because refreshViaOAuth never succeeded inside OpenCode; making it work exposed the path. The refresh had no retry. The token endpoint rate-limits valid refresh requests, observed as repeated HTTP 429s against a real account, and several OpenCode instances refreshing near expiry cluster their calls. The request path already retried 429/529, so fetchWithRetry moves to a shared http module and both paths use it rather than duplicating the logic. A rotation invalidates the refresh token every other instance holds, and the loser had no way to notice. Its own source is now re-read before the CLI fallback, so it adopts what the winner already wrote instead of spawning `claude -p`. Also fails the credentials test fetch closed. refreshViaOAuth uses the runtime's fetch, so unstubbed tests were reaching the real token endpoint.
Both paths could still present a lender's refresh token as the borrower's and persist the result to the borrower's store. refreshBorrowedAccount cleared the guard on the exhaustion path while target.credentials still held the lender's tokens, so the next cycle took the normal path and exchanged them. It now restores the account's own credentials when they are readable and otherwise keeps the guard. forceRefreshActiveAccount exchanged account.credentials.refreshToken with no borrowed check, so a forced refresh on a borrowed account rotated the lender's token. It now declines and leaves recovery to refreshIfNeeded. Reported by Greptile on #258.
fetchWithRetry slept between attempts with a bare setTimeout, so a caller that bounded its request with an AbortController got the retry delay (capped at 30s) as its effective timeout instead. refreshViaOAuth sets a 15s deadline, and a 429 carrying retry-after made it wait far past that, then spend two more attempts that could only fail. The backoff now resolves early on abort and returns the rate-limit response instead of retrying. Also documents that the post-loop request is reachable only when retries < 1, rather than leaving it looking like dead code. Reported by Greptile on #258.
refreshAccountsList swaps in accounts read fresh from their own stores, so the WeakSet entry goes with the object it was keyed on. That is correct only because the rebuilt account carries its own tokens rather than the lender's; this pins that invariant.
2611ad9 to
c42baab
Compare
🤖 I have created a release *beep* *boop* --- ## [2.1.5](v2.1.4...v2.1.5) (2026-07-30) ### Bug Fixes * handle external rotation of the Claude Code credential ([#260](#260)) ([5a44883](5a44883)) * refresh OAuth tokens with native fetch instead of a subprocess ([#258](#258)) ([231165b](231165b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
refreshViaOAuthran its HTTP request inside a child process spawned asprocess.execPath -e <script>. That assumesprocess.execPathis a JavaScript runtime. It isn't inside OpenCode — the plugin runs in a compiled single-file executable, soprocess.execPathis the OpenCode binary and-eis not a script to evaluate.Captured from a real OpenCode runtime (v1.18.8, macOS):
{ "execPath": "/opt/homebrew/Cellar/opencode/1.18.8/bin/opencode", "execPath_dash_e": { "threw": true, "status": 1, "stdout": "" }, "globalFetch": "function" }opencode -e '<script>'prints the CLI help banner and exits 1 with empty stdout. stderr was discarded (stdio: ["pipe", "pipe", "ignore"]) and the failure was onlylog()ged — logging is off unlessCLAUDE_AUTH_DEBUGis set — so every direct OAuth refresh failed silently and fell through to spawning theclaudeCLI. Trace of the production path before this change:Node 18+ and Bun both expose a global
fetch, so the subprocess is unnecessary. The refresh chain becomes async as a result.The existing tests could not have caught this:
node --testsetsprocess.execPathto the node binary, where-eworks fine.What else is in here
Validating the fix against a live account surfaced four more defects, each with a failing-first test.
CLI fallback was burning real API requests. The
claudeCLI only rotates a token that is itself near expiry. Since #238 the sync tick callsrefreshIfNeeded(undefined, 1 hour), so for the final hour of every token the plugin spent a realclaude -prequest every 5 minutes that returned the same token — then reported success, because the re-read credentials still passedexpiresAt > now + 60s. That is exactly the idle token consumption #104 set out to eliminate; it was masked because the OAuth step it relied on never ran. The fallback is now scoped to the 60-second reactive window.Concurrent refreshes raced. The sync timer calls
refreshIfNeededdirectly while the request path arrives viagetCachedCredentials. Each rotation invalidates the refresh token it was issued against, so the two racing left one caller holding a dead token. A test for this failed with 2 concurrent OAuth calls before the guard was added.Borrowed credentials were clobbering their lender. When a refresh exhausted,
tryFallbackAccounthanded back another account's tokens andrefreshIfNeededassigned them totarget.credentials. Once those neared expiry the borrower exchanged the lender's refresh token and wrote the rotated result into its own store — destroying its credentials and rotating the lender's token out from under it. This was unreachable whilerefreshViaOAuthnever succeeded; making it work exposed the path. Borrowed accounts are now tracked and routed through a path that re-reads their own source, refreshes only with their own token, and writes back only their own result.The refresh had no retry. The token endpoint rate-limits valid refresh requests — observed as repeated HTTP 429s against a real account — and several OpenCode instances refreshing near expiry cluster their calls. The request path already retried 429/529, so
fetchWithRetrymoves to a sharedhttpmodule and both paths use it instead of duplicating the logic. Its backoff also now honours the caller'sAbortSignal; previously aretry-aftercould stretchrefreshViaOAuth's 15s deadline to the 30s cap.Plus a multi-process recovery: a rotation invalidates the refresh token every other instance holds, and the loser had no way to notice. Its own source is now re-read before the CLI fallback, so it adopts what the winner already wrote rather than spawning
claude -p.Related issues
Refs #104, #238.
Testing
make all— 249 tests pass, 0 lint warnings, clean buildCommand failed: opencode -e, and thatrefresh_cli_skippedfires where the old code would have spawnedclaude -pTwo test-infrastructure fixes came out of this.
copySourceFilesdid not stubnode:child_process, soindex.test.tswas invoking the realclaudebinary; and once the refresh moved tofetch, unstubbed credential tests were reaching the real token endpoint. Both now fail closed. Suite time dropped from 16.7s to ~11s.Note on versioning
getCachedCredentials,refreshIfNeeded,refreshViaOAuthandforceRefreshActiveAccountnow return promises. They are reachable from the package entry point, but internal to the plugin in practice, so this is deliberately not marked breaking and will cut a patch release.Checklist
make allpasses locally (runs lint, build, and test)