fix(oauth): recover refreshable_bearer refresh failures like plain OAuth

[feniix]

Summary

Ports the plain-OAuth refresh-failure recovery to the refreshable_bearer path in src/oauth-persistence.ts, closing the gap where a single lost refresh race against a rotating-refresh-token provider (e.g. the hosted Notion MCP) escalated into full grant revocation.

• refreshBearerToken now reads the token endpoint error body on !response.ok and attaches the OAuth error code, so invalid_grant / invalid_client / unauthorized_client are distinguishable from transient failures (previously only Token endpoint returned HTTP 400.).
• On an unrecoverable code, readExplicitRefreshableBearerToken re-reads persistence and adopts the tokens a concurrent refresh persisted first, instead of spuriously failing the losing invocation.
• With no concurrent winner, it clears the cached credentials (tokens scope for invalid_grant, all otherwise) before throwing, so a rotated-out refresh token is never replayed — replays past the provider's grace window are treated as stolen-token signals and revoke the entire grant.
• The recovery logic is extracted into a shared recoverFromUnrecoverableRefresh used by both the plain-OAuth and bearer paths, so the two can't drift apart again.

Also fixes a test-isolation bug found while working on this: the vault honors XDG_DATA_HOME (src/paths.ts), which the tests' os.homedir() spy does not cover, so running tests/oauth-persistence.test.ts on a machine with XDG_DATA_HOME set read from and wrote test entries into the developer's real credentials.json. The suite now unsets XDG_* vars in beforeEach. (tests/oauth-callback.test.ts has the same exposure and probably deserves a follow-up.)

Fixes #226

Type of change

• Feature
• Bug fix
• Hotfix
• Spike / exploration
• Documentation
• Refactor

Test plan

Developed test-first (red/green); new regression tests in tests/oauth-persistence.test.ts:

• clears cached bearer tokens instead of replaying a refresh token rejected with invalid_grant (and verifies the next invocation does not hit the token endpoint again)
• adopts the concurrent winner token when a bearer refresh loses the race with invalid_grant
• clears all cached bearer credentials when refresh reports an invalid_client
• transient HTTP 500 refresh failures still throw and preserve the stored refresh token for retry

Verified per the issue's acceptance criteria:

• pnpm exec vitest run tests/oauth-persistence.test.ts tests/runtime-transport.test.ts — 61 passed
• pnpm check — clean
• pnpm test — 817 passed, 3 skipped

Runtime proof

Captured against the real CLI (this branch, pnpm build, node dist/cli.js) talking to a local mock provider that implements the Notion token semantics from #226: refresh tokens rotate on every use, the grant keeps a one-step grace window, a grace-window replay returns invalid_grant without revoking, and any older replay is a stolen-token signal that revokes the whole grant. The provider also serves a real MCP endpoint (@modelcontextprotocol/sdk StreamableHTTPServerTransport) that 401s unless the presented bearer token is currently valid. Everything runs in an isolated $HOME with a tokenCacheDir-backed cache; all hosts are 127.0.0.1 and all tokens are synthetic, so nothing is redacted.

Config entry used:

{
  "mcpServers": {
    "notion-sim": {
      "url": "http://127.0.0.1:8976/mcp",
      "auth": "refreshable_bearer",
      "tokenCacheDir": "<isolated>/token-cache",
      "refresh": {
        "tokenEndpoint": "http://127.0.0.1:8976/token",
        "clientIdEnv": "SIM_CLIENT_ID",
        "clientAuthMethod": "none"
      }
    }
  }
}

Before (v0.12.3 via npx -y mcporter@0.12.3): poison token replayed forever

Cache seeded with an expired access token and a rotated-out refresh token (rt-dead, past the grace window). Three consecutive invocations:

  Tools: <unavailable>
  Reason: Failed to refresh cached bearer token for 'notion-sim': Token endpoint returned HTTP 400.

Token cache after the runs — the dead refresh token is still there:

{
  "access_token": "at-stale",
  "token_type": "Bearer",
  "refresh_token": "rt-dead",
  "expires_at": 1783123357
}

Provider log — every invocation replays the dead token; the first replay revokes the grant:

[provider 00:04:06.804] POST /token #1 refresh_token=rt-dead
[provider 00:04:06.804]   -> #1 400 invalid_grant *** STOLEN-TOKEN SIGNAL: ENTIRE GRANT REVOKED ***
[provider 00:04:16.658] POST /token #2 refresh_token=rt-dead
[provider 00:04:16.658]   -> #2 400 invalid_grant (grant already revoked)
[provider 00:04:28.763] POST /token #3 refresh_token=rt-dead
[provider 00:04:28.763]   -> #3 400 invalid_grant (grant already revoked)

After (this branch): dead grant cleared, never replayed

Same poison seed, run 1 — the error is now classified and the cache is cleared:

$ node dist/cli.js list notion-sim --debug
  Tools: <unavailable>
  Reason: Failed to refresh cached bearer token for 'notion-sim': Token endpoint returned HTTP 400 (invalid_grant).

$ ls <isolated>/token-cache/
(tokens.json gone)

Run 2 — fails cleanly with no replay:

$ node dist/cli.js list notion-sim
  Tools: <unavailable>
  Reason: Server 'notion-sim' uses refreshable_bearer auth but has no cached access token.

Provider log across both runs — exactly one /token request total:

[provider 00:03:03.909] POST /token #1 refresh_token=rt-dead
[provider 00:03:03.910]   -> #1 400 invalid_grant *** STOLEN-TOKEN SIGNAL: ENTIRE GRANT REVOKED ***

After (this branch): concurrent refresh race — loser adopts the winner

Cache seeded with an expired access token and the live refresh token rt-1. Two CLI invocations launched 400 ms apart; provider response latency arranged so both submit rt-1 before either response returns, and the loser's rejection arrives after the winner has persisted (the realistic ordering — the winner's round-trip completes first):

=== process A (winner):
  1 tool · 1048ms · HTTP http://127.0.0.1:8976/mcp
=== process B (loser — refresh rejected with invalid_grant, adopts A's token, still succeeds):
  1 tool · 3030ms · HTTP http://127.0.0.1:8976/mcp

Provider log — both in flight with the same token, one rotation, one grace-window rejection:

[provider 00:05:42.227] POST /token #1 refresh_token=rt-1
[provider 00:05:42.626] POST /token #2 refresh_token=rt-1
[provider 00:05:43.227]   -> #1 200 rotated: access_token=at-2 refresh_token=rt-2 (previous rt-1 enters grace window)
[provider 00:05:45.627]   -> #2 400 invalid_grant (grace-window replay: concurrent refresh lost the race, grant survives)

Final token cache — the winner's tokens survive, neither clobbered nor cleared, and process B's MCP call was served with at-2:

{
  "access_token": "at-2",
  "token_type": "Bearer",
  "refresh_token": "rt-2",
  "expires_in": 3600,
  "expires_at": 1783127143
}

On v0.12.3 the same race fails process B's tool call outright (this is gap 2 in #226); this run shows both invocations succeeding.

Note on scope: if the loser's invalid_grant arrives before the winner has persisted, the loser still fails that one call (there is nothing newer to adopt yet) — but the grant survives because the winner's tokens land afterwards, and per the review-driven compare-and-clear hardening (b992406) the loser's recovery can never delete tokens it didn't fail with, no matter when the winner's per-store writes land. Fully eliminating the one failed call is the optional per-entry refresh serialization the issue defers as follow-up hardening.

Note on storage path: the captures above exercise the tokenCacheDir directory cache. A later review note flagged that loadVaultEntry can also source tokens from a same-URL legacy rename entry (<name>-oauth) when a renamed definition's exact vault entry has none — a path the original compare-and-clear only protected on the exact key, so an inherited poison refresh token was left in the source entry to be replayed. The follow-up commit extends the same clear-and-adopt guarantee to the inherited source entry (poison cleared wherever readTokens() sources it, inherited client info kept for re-auth, a concurrent winner's newer tokens adopted rather than cleared). That vault-inheritance path is covered by regression tests in tests/oauth-persistence.test.ts verified to fail without the fix, rather than a separate runtime capture, since it is the identical guarantee on a different storage backend.

🤖 Generated with Claude Code

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 2:15 PM ET / 18:15 UTC.

Summary
The PR ports unrecoverable OAuth refresh recovery to refreshable_bearer, adds compare-and-clear token cleanup for directory/vault caches, and expands OAuth persistence regression tests.

Reproducibility: yes. Source inspection of current main shows refreshable_bearer still throws without error classification, concurrent-winner adoption, or cache clearing, and the PR body includes before/after CLI and provider logs for the same behavior.

Review metrics: 2 noteworthy metrics.

• Changed surface: 3 files, +576/-22. The diff is focused, but it changes OAuth credential recovery behavior and its regression coverage.
• Checks observed: 5 successful check runs. GitHub reports Linux, macOS, Windows, and Socket checks green on the reviewed head.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #226
Summary: This PR is the active candidate implementation for the linked refreshable_bearer invalid_grant and concurrent-refresh issue; older OAuth/cache reports are adjacent but distinct.

Members:

• canonical: refreshable_bearer: refresh failure path lacks invalid_grant/concurrent-refresh recovery — stale rotated refresh token replayed until provider revokes the grant (Notion MCP) #226 - The PR uses closing syntax for this issue and implements the requested error classification, concurrent-winner adoption, and terminal cache clearing behavior.
• adjacent_distinct: list reports auth required on expired access_token even when refresh_token is valid #166 - That closed issue concerned refreshing expired plain OAuth access tokens from list, not terminal refreshable_bearer invalid_grant recovery.
• adjacent_distinct: Serialize concurrent file writes — credentials.json, mcporter.json, OAuth persistence files race under parallel invocations #167 - That closed issue concerned atomic and locked persistence writes, while this PR handles OAuth protocol recovery after refresh-token races.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• [P2] Get maintainer acceptance of the terminal refreshable_bearer cache-clearing semantics before merge.

Risk before merge

• [P1] The PR intentionally changes terminal refreshable_bearer provider failures to clear matching token or credential state, so affected users may need to re-authenticate or reseed instead of retrying a poisoned cache forever.
• [P1] Full per-entry refresh serialization remains deferred, so a losing concurrent invocation can still fail one call if invalid_grant arrives before a winner persists, although compare-and-clear prevents deleting later winner tokens.

Maintainer options:

1. Land after auth-provider review (recommended)
   Accept the intentional terminal-cache clearing behavior because it prevents rotated refresh-token replay and is covered by focused tests plus real CLI/provider logs.
2. Require per-entry serialization first
   Ask for refresh serialization in this PR if maintainers want to eliminate the remaining one-call loser failure before changing cache semantics.
3. Pause for alternate recovery semantics
   Pause or close this branch if maintainers do not want refreshable_bearer invalid_grant to clear cached token state by default.

Next step before merge

• No automated repair lane is needed; the next action is maintainer acceptance of the auth-provider cache-clearing behavior before merge.

Maintainer decision needed

• Question: Should this PR land with terminal refreshable_bearer refresh failures clearing matching cached token or credential state?
• Rationale: The patch appears correct, but it intentionally changes OAuth bearer cache recovery semantics and can require re-authentication or token reseeding for terminal provider errors.
• Likely owner: steipete — He is the strongest current-main history owner for refreshable_bearer, OAuth persistence, vault recovery, and related concurrency hardening.
• Options:
  • Accept the recovery behavior (recommended): Merge after ordinary maintainer review because the PR prevents poisoned refresh-token replay and has focused tests plus runtime logs.
  • Require serialization first: Hold the PR until per-entry refresh serialization eliminates the remaining one-call concurrent-loser failure mode.
  • Choose different cache semantics: Decline or narrow the cache-clearing behavior and leave the linked issue open for a different recovery design.

Security
Cleared: No dependency, workflow, or supply-chain changes were found; credential-clearing behavior is tracked as auth-provider merge risk.

Review details

Best possible solution:

Land the focused auth-provider recovery after maintainer acceptance of the credential-clearing behavior, while keeping #226 as the canonical tracker until merge.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection of current main shows refreshable_bearer still throws without error classification, concurrent-winner adoption, or cache clearing, and the PR body includes before/after CLI and provider logs for the same behavior.

Is this the best way to solve the issue?

Yes. Sharing unrecoverable refresh recovery with store-level compare-and-clear is a narrow maintainable fix, and the latest head covers the inherited same-URL vault fallback called out in review.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f95142b66511.

Label changes

Label justifications:

• P2: This is a normal-priority auth reliability fix for refreshable_bearer users and rotating-refresh-token providers.
• merge-risk: 🚨 auth-provider: The PR intentionally changes OAuth bearer refresh error classification and cached credential clearing behavior.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body includes copied real CLI output and provider logs showing before failure, after dead-token clearing, and after concurrent-loser adoption with synthetic local tokens.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied real CLI output and provider logs showing before failure, after dead-token clearing, and after concurrent-loser adoption with synthetic local tokens.

Evidence reviewed

What I checked:

• AGENTS policy read: AGENTS.md was read fully; its conservative TypeScript review and verification guidance informed the merge-risk/manual-review route. (AGENTS.md:1, f95142b66511)
• Current-main bearer gap: Current main's refreshable_bearer catch path logs and rethrows refresh failures without classifying OAuth error bodies, adopting a concurrent winner, or clearing terminal cache state. (src/oauth-persistence.ts:521, f95142b66511)
• Current-main token endpoint errors are opaque: Current main throws only the HTTP status for non-OK bearer token refresh responses, so invalid_grant cannot reach unrecoverableOAuthRefreshCode. (src/oauth-persistence.ts:578, f95142b66511)
• PR recovery implementation: The PR compare-clears failed tokens first, rereads persistence, and adopts a newer concurrent-winner token before falling back to cache clearing. (src/oauth-persistence.ts:571, 51fd3e0d039f)
• Prior inherited-vault concern addressed: The latest PR head extends vault compare-and-clear to the exact key plus same-URL fallback source keys, matching the review note about loadVaultEntry inheritance. (src/oauth-vault.ts:221, 51fd3e0d039f)
• Regression coverage: The PR adds focused tests for invalid_grant token clearing, inherited same-URL token clearing, inherited concurrent-winner adoption, transient failure preservation, and normal concurrent-winner adoption. (tests/oauth-persistence.test.ts:1104, 51fd3e0d039f)

Likely related people:

• steipete: Current-main blame and history show Peter Steinberger authored the refreshable_bearer support, plain OAuth refresh recovery, vault recovery hardening, and file-lock/concurrency work that define this behavior. (role: feature owner and recent area contributor; confidence: high; commits: 7f1e9a8ce0b9, 49dc62b9eec9, f9f60d7cc45a; files: src/oauth-persistence.ts, src/oauth-vault.ts, src/fs-json.ts)
• Krasimir Kralev: Recent merged work touched corrupt OAuth credential-cache recovery in the same persistence module and tests, making this person useful adjacent context but not the primary auth-provider decision owner. (role: recent adjacent OAuth persistence contributor; confidence: medium; commits: 53747cac6362; files: src/oauth-persistence.ts, tests/oauth-persistence.test.ts)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (3 earlier review cycles)

• reviewed 2026-07-04T04:05:35.220Z sha b992406 :: needs changes before merge. :: [P2] Clear inherited vault fallback tokens during compare-and-clear
• reviewed 2026-07-07T21:53:08.685Z sha 51fd3e0 :: needs maintainer review before merge. :: none
• reviewed 2026-07-07T21:58:16.463Z sha 51fd3e0 :: needs maintainer review before merge. :: none

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 412fecf9c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[feniix]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

[feniix]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

[rpelevin]

Small review note on the inherited-vault fallback case: the current compare-and-clear helper appears to protect the exact vault key, but loadVaultEntry can source tokens from a same-URL legacy rename entry. I would keep the fix narrow and add one regression around that path:

1. seed a legacy same-URL vault entry whose tokens and client info are inherited by the renamed refreshable_bearer definition;
2. make the token endpoint return invalid_grant;
3. assert the stale fallback tokens are conditionally removed from the source entry, inherited client info remains usable for re-auth, and a later invocation cannot reread or replay the same refresh token;
4. include the concurrent-winner guard: if either the exact entry or fallback source has newer tokens by the time recovery runs, recovery adopts and preserves those tokens instead of clearing them.

That keeps the terminal provider-refusal path separate from transient refresh failures: invalid_grant clears only the matching poison token, while a changed token remains authority for the next call.

[steipete]

The complete fix is now on main, so this zero-diff contributor PR is superseded:

• rejected-refresh recovery, concurrent-winner preservation, inherited-vault cleanup, and test isolation: 85f4c9e
• explicit-cache priority regression found during closeout: c6940ef
• exact-head CI and landing proof: fix(oauth): preserve primary cache fallback #228 (comment)

Sebastian's credit is preserved: the canonical recovery commit is co-authored by Sebastian Otaegui, and the unreleased changelog thanks @feniix via this PR.

Thank you, @feniix, for the excellent report, implementation, provider-race analysis, and follow-through on the inherited-vault review case.