feat(auth): revoke OAuth tokens at the authorization server on clear (RFC 7009)

[cliffhall]

Closes #2144

Clearing the Inspector's OAuth state deleted the local copy of the tokens and stopped there. The access token — and the refresh token, which is long-lived by design — stayed valid at the authorization server until they expired on their own. From the AS's side nothing happened; the client just went quiet, and a day of connect/disconnect iteration left it holding a pile of grants for sessions that ended hours ago. RFC 7009 §1 describes exactly that case, and nothing in the repo constructed a revocation request, so a server author implementing revocation_endpoint also had no way to exercise it with the tool that exists to exercise this.

The clear path now sends an RFC 7009 revocation before wiping local state.

What it does

core/auth/revocation.ts builds and sends the request; the three clear paths call it, so the behavior is written once:

• web — "Clear OAuth state and disconnect" (both the active-connection and stored-only paths)
• TUI — Auth tab → "Clear OAuth State"
• CLI — --relogin

The ordering is snapshot → clear → revoke, and both halves of it matter.

The snapshot has to come first, because the token, the client credentials and the discovered revocation_endpoint all live in the store the clear is about to empty. But the clear must not wait on the network: a five-second revocation would otherwise leave a window in which a fresh authorization completes and is then deleted by a clear still reasoning about the grant it replaced. That is reachable in all three clients — a same-server reconnect in the TUI, a stored-only web clear for a server that becomes active mid-flight, and another CLI/TUI process writing to the shared file-backed store.

So core/auth/revocation.ts exposes two halves, clearAndPlanRevocation and executeOAuthRevocation. The first takes the stored state and clears it in one atomic storage step (OAuthStorage.takeRevocationSnapshot); the second sends from what was taken. There is deliberately no single-call wrapper and no separate storage.clear() in any caller, so the clear cannot be forgotten or reordered.

Two consequences worth knowing. A failure of the take-and-clear rejects rather than returning an outcome — the state was not cleared, and every caller's contract is that it was; both clients have a rejection path for that. And cross-process atomicity is not claimed: nothing in the OAuth store takes a lock, every mutation is a read-modify-write over a loaded snapshot, and clear() alone always had that property.

The request names the refresh token when there is one. RFC 7009 §2.1 asks the AS to also invalidate the access tokens issued under the same grant, so one request covers both halves; naming the access token instead would leave the long-lived one alive.

Only grants the endpoint can be proved to belong to are sent: metadata is cached once per server rather than per issuer, so an issuer-bound grant must match the cached issuer, and a document naming none is a mismatch rather than a free pass. A grant that cannot be matched is reported as dropped-unrevoked instead of being sent to an authorization server that may never have minted it.

It is best-effort, and the local clear always finishes

Every path returns a TokenRevocationOutcome rather than throwing:

• No advertised revocation_endpoint → nothing is sent, and that authorization server behaves exactly as it did before this existed. This is the path that makes the change safe against every AS without RFC 7009 support.
• Network error, non-2xx, or the 5s timeout → reported (a toast in web, the status line in the TUI, a stderr warning from the CLI) and nothing more.
• A snapshot that cannot be interpreted → same. (A failure of the take-and-clear itself is the exception above: it rejects, because the clear did not happen.)

Forgetting the tokens is what the user asked for, so no failure on this leg stops it.

One path deliberately never revokes: lost_authorization_state recovery clears a half-finished flow in order to retry it, and an authorization that never completed has no grant to revoke.

The setting

oauth.revokeOnClear (default true), surfaced as Revoke tokens on clear in Server Settings → Authorization, plus a per-run --no-revoke on the CLI. Only false is written to disk, matching oauth.requestRefreshToken next to it, so an entry that never touched the switch keeps a byte-stable round-trip.

Unlike its neighbours it is read at clear time rather than connect time, so toggling it takes effect without reconnecting.

Turning it off is a testing affordance rather than only an escape hatch — a client that walks away still holding live tokens is a case a server author may want to reproduce on purpose.

Testing it in-repo

test-servers/src/test-server-oauth.ts now advertises revocation_endpoint and serves POST /oauth/revoke, faithful on the two points the Inspector depends on: §2.2 (an unknown token is a success, so an already-expired token is not reported as a failure) and §2.1 (revoking a refresh token invalidates the access tokens under the same grant — a fixture without that linkage would let a regression through).

Two showcase configs for driving it by hand: oauth-revocation-http.json and oauth-no-revocation-http.json, identical but for the advertised endpoint.

Proof

Web — Server Settings → OAuth Settings. The new control, on by default and opted out. Captured by driving the prod --web build in headless Chromium.

Default (on)	Opted out

TUI — Auth tab, revocation failed. The frame AuthTab renders when the local clear succeeded but the RFC 7009 request did not. A terminal capture rather than a PNG: script(1) needs its stdin to be a terminal, so a driven TUI cannot also be fed keystrokes from a pipe here. This is the real component's output, not a mock's.

 OAuth

 Cleared locally, but revoking the grant at the authorization server failed: fetch failed. It may still
  be valid there.

 No OAuth information yet.
 Connect (C) to authorize when this server requires it.


 ▶ Clear OAuth State

It renders in yellow, not the cyan the informational channel uses: the local state really was cleared, so this is not an OAuth error, but the grant may still be live and cyan understated that.

Tests

• core/auth/revocation.test.ts — token selection, RFC 8414 auth-method fallback, all three client-authentication shapes, and every outcome branch.
• revocation-e2e.test.ts — a real authorization-code exchange against the test server, then a revocation, then asserting the MCP endpoint rejects the bearer token. This is the half only a real AS can show: that the request is accepted, and that the §2.1 grant linkage holds.
• oauthManager.test.ts — the clear-then-revoke ordering, and that the clear still happens when the request fails.
• CLI, clearServerOAuthState, webProxiedFetch, the settings round-trip, /api/servers validation and write-through, and the two checkboxes.

Note on the browser

The revocation POST goes through the backend-proxied fetch the OAuth flow already uses. Going direct is not an option: an authorization server serves no CORS headers for a page origin it has never heard of, so globalThis.fetch would fail on nearly every real deployment while working on a permissive one. On the stored-only path there is no live client to borrow one from, so getWebProxiedFetch builds it; with no proxied fetch available the leg reports skipped rather than sending a request that cannot work.

🤖 Generated with Claude Code

https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh

[Copilot]

Pull request overview

Adds best-effort RFC 7009 OAuth token revocation before clearing local state across the Web, TUI, and CLI clients.

Changes:

• Implements shared token revocation with endpoint discovery, client authentication, timeout handling, and outcomes.
• Adds per-server and CLI opt-outs plus user-facing status reporting.
• Extends OAuth fixtures, integration coverage, configuration validation, and documentation.

Reviewed changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
test-servers/src/test-server-oauth.ts	Adds revocation endpoint and token linkage.
test-servers/src/test-server-fixtures.ts	Adds revocation fixture option.
test-servers/src/load-config.ts	Extends OAuth config schema.
test-servers/src/composable-test-server.ts	Documents server revocation support.
test-servers/configs/oauth-revocation-http.json	Adds revocation-enabled fixture.
test-servers/configs/oauth-no-revocation-http.json	Adds no-revocation fixture.
README.md	Documents manual revocation testing.
docs/mcp-server-configuration.md	Documents revokeOnClear.
core/mcp/types.ts	Adds revocation settings types.
core/mcp/serverList.ts	Maps persisted revocation settings.
core/mcp/remote/node/server.ts	Validates and persists the setting.
core/mcp/oauthManager.ts	Revokes before clearing storage.
core/mcp/inspectorClient.ts	Exposes revocation outcomes.
core/auth/revocation.ts	Implements RFC 7009 requests.
core/auth/index.ts	Exports revocation APIs.
clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts	Tests remote settings handling.
clients/web/src/test/integration/mcp/inspectorClient.test.ts	Updates clear outcome expectations.
clients/web/src/test/integration/auth/revocation-e2e.test.ts	Adds end-to-end revocation tests.
clients/web/src/test/core/mcp/serverList.test.ts	Tests settings persistence.
clients/web/src/test/core/mcp/oauthManager.test.ts	Tests revoke-before-clear behavior.
clients/web/src/test/core/auth/revocation.test.ts	Tests revocation request logic.
clients/web/src/lib/webProxiedFetch.ts	Creates cached backend-proxied fetches.
clients/web/src/lib/webProxiedFetch.test.ts	Tests proxied-fetch construction.
clients/web/src/lib/clearServerOAuthState.ts	Adds revocation to web clear paths.
clients/web/src/lib/clearServerOAuthState.test.ts	Tests web clear sequencing.
clients/web/src/hooks/useOAuthRecovery.ts	Wires settings and result notifications.
clients/web/src/hooks/useOAuthRecovery.test.tsx	Tests revocation notification text.
clients/web/src/hooks/useConnectionLifecycle.ts	Skips revocation during recovery.
clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx	Maps the new setting.
clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx	Tests modal setting conversion.
clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx	Adds the revocation checkbox.
clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx	Tests checkbox behavior.
clients/tui/src/App.tsx	Adds revocation to TUI clearing.
clients/tui/README.md	Documents TUI behavior.
clients/tui/__tests__/App.test.tsx	Updates client mock outcome.
clients/cli/src/cli.ts	Adds --no-revoke and warnings.
clients/cli/src/clear-stored-auth-for-relogin.ts	Revokes before CLI relogin clearing.
clients/cli/README.md	Documents CLI revocation.
clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts	Tests CLI revocation helper.
AGENTS.md	Records the architecture behavior.

Suppressed comments (1)

clients/web/src/test/core/auth/revocation.test.ts:386

• This is another unjustified as unknown as, which the repository's type-safety rules prohibit. Type the logger object against the parameter type so changes to InspectorLogger remain compiler-checked.

      logger: logger as unknown as Parameters<
        typeof revokeStoredOAuthTokens
      >[0]["logger"],

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Fair — added clients/cli/__tests__/relogin-revocation.test.ts, driving runCli with a stubbed fetch that answers only the revocation endpoint. Five cases: revocation by default, --no-revoke skipping it (which is what pins Commander's --no-* mapping), the per-server oauth.revokeOnClear: false opt-out via --catalog, a catalog server that did not opt out, and the stderr warning on a 500.

Each case fails to connect on purpose — the stub refuses everything else — which is fine, because revocation runs before the connect.

[cliffhall]

Review round 1 — all nine addressed

Mirroring the inline replies here, since they go outdated once the fixes are pushed.

#	Finding	Outcome
1	RFC 8414 revocation auth-method default	Fixed. §2 defaults an omitted list to client_secret_basic, not the token endpoint's. Now returns only that field (or [], which resolves to exactly the RFC default via selectClientAuthMethod).
2	5s timeout inert on the web paths	Fixed. Confirmed both hops drop the signal. A wall-clock race now enforces the deadline; the signal is kept because it does cancel the direct-fetch paths.
3	Settings-modal Clear read the persisted value	Fixed. Merges settingsDraft at the call site. No test — see the caveat below.
4	--relogin key precedence	Fixed, via your second option: both spellings are deleted, so both are revoked from. Normalised first (matching findStoredServerState); a duplicate token is skipped.
5	Fixture did not enforce client auth	Fixed. /oauth/revoke now enforces §2.1 with both §2.3.1 credential forms, plus two negative e2e assertions.
6	as unknown as on the logger mock	Fixed. Typed fakeLogger(): InspectorLogger.
7	Spread-and-cast storage stand-in	Fixed. vi.spyOn on the real instance.
8	No CLI-level test for --no-revoke	Fixed. New relogin-revocation.test.ts, five cases through runCli.
9	No TUI test for the new branches	Fixed. Three cases: default on, opt-out forwarded, failure rendered.

One gap flagged rather than papered over (#3). The behavior is fixed, but there is no test. App.test.tsx's settings harness connects the stdio SERVER_A, so the OAuth section — and the Clear button — never render there; switching that fixture to HTTP changes the server type for every neighbouring test in the file. App.tsx is also one of the documented coverage-gate exemptions. Worth a follow-up if the settings harness ever grows an HTTP variant.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

clients/cli/src/cli.ts:784

• --no-revoke is silently accepted when --relogin is absent, even though it then has no effect. This conflicts with the parser's established rejection of accepted-but-inert flags (see the --strict rationale at clients/cli/src/cli.ts:870-876) and can give users false confidence that another clear path was changed. Add an early parse-time requirement that options.revoke === false implies options.relogin, including before short-circuit returns.

    .option(
      "--no-revoke",
      "With --relogin, skip the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.",
    )

clients/web/src/lib/clearServerOAuthState.ts:81

• When revocation is enabled but fetchFn is unavailable, this reports reason: "disabled", whose public type explicitly means the caller opted out. That makes ClearServerOAuthStateResult inaccurate for callers and conflates an unavailable proxy with user intent. Add a distinct skip reason such as no_fetch/fetch_unavailable and return it for this branch.

  const revocation: TokenRevocationOutcome =
    revoke && fetchFn
      ? await revokeStoredOAuthTokens({
          serverUrl,
          storage: params.oauthStorage,
          fetchFn,
        })
      : { status: "skipped", reason: "disabled" };

clients/web/src/hooks/useOAuthRecovery.ts:1263

• The web hook's per-server opt-out wiring is not covered: the added tests exercise revocationSuffix, but none asserts that server.settings.oauthRevokeOnClear === false reaches clearServerOAuthState as revoke: false. Add a hook test for both the default and opt-out values so this client cannot silently diverge from the tested CLI/TUI behavior.

        revoke: server.settings?.oauthRevokeOnClear !== false,
        fetchFn: getWebProxiedFetch(getAuthToken()),

[cliffhall]

Took the "extract a testable helper" option — you are right that the hook tests cannot see this, since they receive whatever they are handed and cannot tell a draft from a persisted entry.

utils/serverWithDraftSettings(entry, draft) now owns the rule and has four cases: the draft wins in both directions (unchecking and re-checking, since the second is just as wrong), a nullish draft returns the entry unchanged by identity, and the entry's id/name/config survive the merge. It takes | null | undefined deliberately — useSettingsDraft types its draft | null — so a caller never normalizes one into the other, which is exactly where the distinction would get lost.

An App-level test still is not practical: App.test.tsx's settings harness connects the stdio SERVER_A, so the OAuth section and its Clear button never render, and switching that fixture to HTTP changes the server type for every neighbouring test in the file.

[cliffhall]

Review round 2 — all four addressed

Including the suppressed "previously missed" one, which was a fair catch.

Finding	Outcome
getTokens pre-read outside the best-effort catch	Fixed. It parses through OAuthTokensSchema, so an unparseable persisted token rejected and abandoned the local delete --relogin promises. Now caught and retained as a failed outcome; both clear() calls still run.
Multi-key bookkeeping	Fixed, both halves. A token counts as spent only on a revoked outcome, so a failed attempt no longer skips a duplicate entry that may hold what would have worked. And a failed outcome outranks an earlier success for reporting, so a live stale grant can't hide behind the first key's 200.
--no-revoke accepted without --relogin (suppressed)	Fixed. Rejected at parse time, ahead of the short-circuit returns, matching the --strict precedent. On its own it reads as "this run will not revoke anything", true only because nothing was being cleared.
No test for the App draft wiring	Fixed via your extract-a-helper option: utils/serverWithDraftSettings, four cases.

Still no App-level test, and the reason is unchanged. App.test.tsx's settings harness connects the stdio SERVER_A, so the OAuth section and its Clear button never render; switching that fixture to HTTP changes the server type for every neighbouring test in the file. The helper is the mitigation, not a claim that the wiring itself is covered.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

test-servers/src/test-server-oauth.ts:748

• The authenticated client is not checked against the token owner before deletion. RefreshTokenData already records clientId, yet any other valid registered client can revoke that refresh token; access tokens have no owner recorded at all. RFC 7009 §2.1 requires verifying that the token was issued to the requesting client. Keep the 200 response for unknown/foreign tokens, but only delete tokens owned by the authenticated client (and track ownership for access tokens too).
  clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx:936
• This adds a visible web control and changes the TUI status output, but the PR body contains no before/after screenshots or interaction proof. AGENTS.md requires screenshots (or a short GIF) for every web UI or TUI change; please attach proof for both affected surfaces from the gitignored pr-screenshots/ workflow.

              <Checkbox
                label="Revoke tokens on clear"
                description="Calls the authorization server's RFC 7009 revocation endpoint before clearing the stored OAuth state, so the grant ends when the session does instead of staying valid until it expires. Servers that advertise no revocation_endpoint are unaffected. Uncheck it to reproduce a client that disconnects still holding live tokens."
                checked={settings.oauthRevokeOnClear ?? true}
                onChange={handleRevokeOnClearChange}
              />

[cliffhall]

Review round 3 — all three addressed

Finding	Outcome
Only the active issuer's grant was revoked	Fixed. OAuthStorage.listIssuers + collectGrants cover every grant clear deletes. See the inline reply for the one thing that constrains it: metadata is cached per server, not per issuer, so a grant bound to an issuer the cached document doesn't describe is now reported as dropped-unrevoked rather than misdirected to the wrong AS.
Fixture didn't verify token ownership (suppressed)	Fixed. Access tokens record their owning client_id; /oauth/revoke only deletes tokens belonging to the authenticated client, and still answers 200 per §2.2 — the response must not tell one client whether another's token exists. New e2e case proves a second registered client can't revoke the first's grant.
No screenshots for the web/TUI change (suppressed)	Fixed. Added to the PR body.

On the TUI proof specifically — it is a terminal capture, not a PNG, and that is a deliberate limitation rather than a shortcut: script(1) requires its own stdin to be a terminal, so a driven TUI cannot simultaneously be fed keystrokes from a pipe on this platform. What is in the body is the real AuthTab output for the failure state, not a mock's.

Capturing it also turned up something worth fixing: the message was rendering in cyan, the informational tone. It is a partial success — the local clear really happened, so it is not an error status — but the grant may still be live at the authorization server, and cyan understated that. AuthTab now takes an oauthMessageTone, the failure renders yellow, and the message says "It may still be valid there" outright.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/hooks/useOAuthRecovery.ts:1263

• The hook tests mock clearServerOAuthState, but none asserts these new arguments. As a result, removing the per-server opt-out or replacing the proxied fetch with the wrong fetch would leave the tests green, even though this is the web client's production wiring for revocation. Add a hook-level case with oauthRevokeOnClear: false that verifies revoke: false and the backend-proxied fetch are passed.

        revoke: server.settings?.oauthRevokeOnClear !== false,
        fetchFn: getWebProxiedFetch(getAuthToken()),

[cliffhall]

Review round 4 — all three addressed

Finding	Outcome
getTokens(url, issuer) falls back, so an enumerated issuer could be attributed the legacy unkeyed token	Fixed. New OAuthStorage.getIssuerTokens — byIssuer slot only. collectGrants uses it for every enumerated issuer; only the ctx-less read still falls back, which is what covers a pre-SEP-2352 entry.
TUI warning tone was sticky	Fixed by deleting the state rather than resetting it: the tone is now derived from the message (oauthMessageToneFor), so it cannot outlive it.
Hook tests didn't assert the web wiring (suppressed)	Fixed. Two cases assert revoke and the backend-proxied fetch reach clearServerOAuthState.

Worth calling out on the tone: the obvious shape — one { text, tone } state with useCallback setters — is what I wrote first, and it makes setOauthMessage a value react-hooks/exhaustive-deps demands in seven dependency arrays for an identity that never changes. Deriving avoids that and is strictly stronger: there is no tone to forget to clear.

Also: the tone is not observable from a rendered frame — ink-testing-library strips colour (verified by dumping the raw frame bytes). An App-level assertion would have passed whichever tone was used, which is why the derivation is extracted and unit-tested instead.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/cli/src/clear-stored-auth-for-relogin.ts:111

• This skips the entire second storage key based only on the single active token returned by getTokens(), but revokeStoredOAuthTokens() now enumerates all issuer slots under that key. If the raw and normalized entries share an active token while the second entry also contains another issuer-bound grant, that additional grant is never examined and is then deleted locally. Revoke each distinct key (duplicate RFC 7009 requests are harmless) or deduplicate the full enumerated grant set instead of pre-reading one token.

    if (token !== undefined && revokedTokens.has(token)) continue;

[cliffhall]

Review round 5 — all three addressed

All three were the same family: my grant enumeration was too coarse in one direction and too fragile in another.

Finding	Outcome
Dedup by token string conflated grants from different issuers	Fixed. Key is now issuer+token. The ctx-less read keeps a narrower suppression — skipped only when its token already came from some slot, which is exactly the active-issuer duplicate and never the legacy one.
One unreadable issuer slot aborted the whole enumeration	Fixed. Slots are read independently; a failure is carried as an outcome beside the grants that are still revocable, including through the no_metadata / no_endpoint returns so a skip can't swallow it.
CLI dedup across the two key spellings (suppressed)	Removed entirely. It could only be done by pre-reading one token, which skipped the second key's other issuer-bound grants. A duplicate RFC 7009 request is harmless (§2.2); a missed one is the leak. Deleting it also removes the pre-read that round 2 had to add error handling for.

Three new tests: two issuers minting the same token value are not collapsed, a corrupt slot doesn't stop the readable grants being revoked, and both CLI keys are revoked from even when they hold the same token.

npm run validate and npm run coverage both green.

[cliffhall]

Review round 16 — "no new comments", but three in the suppressed block

The review body says generated no new comments; its Suppressed comments (3) section carried three real findings, all in the same family — a completion acting on shared state it no longer owns. Addressed:

Finding	Outcome
AuthTab released clearInFlightRef before checking current()	Fixed. A stale clear owns nothing: current() is checked before either shared ref is touched. Otherwise server A settling after the user moved to B dropped B's lock and let a second B clear run concurrently against the same store entry.
Switching servers left ownClearRef set	Fixed. The retired clear returns before its oauthRevision bump, so the marker had no bump to skip and swallowed the first unrelated revision on the newly selected server.
Web session check compared only the server id	Fixed. A disconnect/reconnect to the same server builds a replacement InspectorClient and passes an id-only check, so the old clear would have run its session-wide cleanup against the new session. Client identity is part of the check now.

Both new tests were verified to fail with their fix reverted — the first version of the web one passed either way (it asserted on the replacement client, which the code never touches regardless), so it now asserts the original client is not torn down, which is the actual behavior change.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

clients/cli/tests/clear-stored-auth-for-relogin.test.ts:73

• This comment reverses the implemented race-avoidance contract: only the request data is captured before deletion; the HTTP request is deliberately sent afterward. Update it so the test documentation does not reintroduce the old ordering.

  // #2144 — RFC 7009. The request has to go out *before* the delete, since it
  // is built from the token, the client id and the cached metadata the delete
  // removes.

[cliffhall]

Review round 17 — both addressed

Finding	Outcome
Revocation could authenticate with a different client than minted the token	Fixed as far as the storage shape allows. The registration bound to the grant's issuer now wins, with the preregistered entry as the fallback — the reverse of BaseOAuthClientProvider, deliberately: the provider answers "who should I authenticate as now", revocation asks "who minted this token".
Stale ordering comment in a CLI test (suppressed)	Fixed.

The failure mode on the first is the sharp part and worth restating: RFC 7009 §2.2 answers 200 for a token the server does not recognise as the caller's, so wrong credentials report revoked while the grant stays live — and the local record is already gone. A silent false success is worse than a reported failure.

What is not fixed, stated plainly in the code rather than implied: the store records client information per issuer, not per token, so a server that re-registered dynamically under one issuer still cannot tell which registration minted which grant. Binding the client identity to the token at save time is the real fix; that is a storage-shape change plus a migration, and forcing reauthorization when the configured identity changes is a behavior change with its own blast radius. Both are beyond this PR — happy to file a follow-up issue.

Two tests, one per direction (DCR grant after a configured id is added; configured-client grant with an empty issuer slot).

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 4 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/tui/src/components/AuthTab.tsx:440

• A rejection can come from disconnectInspector() after clearOAuthTokens() has already succeeded, so this line incorrectly reports that OAuth state was not cleared. Distinguish a clear failure from a post-clear disconnect failure (and retain the successful-clear confirmation for the latter).

                Could not clear OAuth state

clients/web/src/App.tsx:1353

• This title is false when clearServerOAuthState() has already cleared storage but the later client.disconnect() rejects: the promise propagates that rejection, suppresses the success toast, and this handler tells the user the stored state was not cleared. Preserve/report the partial result so disconnect failures are distinguished from storage-clear failures.

            title: "Could not clear the stored OAuth state",

[cliffhall]

Same fix — this path now calls clearAndPlanRevocation, which takes and clears atomically, then sends. You are right that the useOAuthRecovery session checks are downstream of this helper and cannot protect the store.

[cliffhall]

Review round 18 — all six addressed (4 inline + 2 suppressed)

Three of the four inline findings were the same check-then-act, and the criticism lands: I closed the network-sized window in round 13 and left the same shape behind at a smaller scale. planOAuthRevocation is async, so an OAuth completion landing at any of its reads still saved a grant the following clear() destroyed.

OAuthStorage.takeRevocationSnapshot(serverUrl) now reads what revocation needs and clears the server in one synchronous pass over the in-memory state, persisting after the mutation. clearAndPlanRevocation replaces planOAuthRevocation, and no caller has a separate clear call left — it cannot be forgotten or reordered. The snapshot returns values unparsed on purpose: validation is pure and belongs after the mutation, since doing it inside would reintroduce the await this exists to remove.

Two consequences worth stating rather than leaving to be discovered:

• A failure of the take-and-clear now rejects instead of returning an outcome — the state was not cleared, and every caller's contract is that it was. Both clients already have a rejection path.
• Cross-process atomicity is explicitly not claimed. Nothing in OAuthStorageBase takes a lock; every mutation is a read-modify-write over a loaded snapshot, and clear() alone has always had this property. This PR no longer widens that window, and I have documented the residual on the method rather than implying it is gone. Locking the OAuth store is a worthwhile follow-up, not something to graft on here.

Other findings	Outcome
CLI budget check ran before plan.outcome	Fixed. A key needing no network was reported as budget-exhausted, which outranked the real outcome and warned about a grant that key never held.
Disconnect failure reported as "could not clear" (both clients, suppressed)	Fixed. It goes to the disconnect channel, and the successful clear is still reported.

Test-suite note: the snapshot change moved what the tests can stub, so several were rewritten — including the shared-deadline one, which now drives executeOAuthRevocation with a hand-built plan, since three revocable grants means three tokens under one issuer and byIssuer holds one slot per issuer.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 5 comments.

[cliffhall]

Fixed — and this one mattered more than a name: it also still told callers to make a separate storage.clear() call, which is exactly the second clear the atomic step exists to remove. Rewritten to describe clearAndPlanRevocation + takeRevocationSnapshot, including the explicit note that cross-process atomicity is not claimed.

[cliffhall]

Review round 19 — all five addressed

Finding	Outcome
Malformed preconfigured registration aborted the whole plan	Fixed. It is a fallback; a bad one is recorded and skipped, matching the per-slot salvage. New test: a valid issuer-bound grant is still revoked with its own registration while the malformed fallback is reported.
sendPlans preserved an earlier success over an unattempted key	Fixed — and investigating it was more informative than the fix. See below.
AGENTS.md named planOAuthRevocation and a separate storage.clear()	Fixed. The second half mattered more than the name: following that text would have reintroduced the very clear the atomic step exists to remove.
Two stale JSDoc links	Fixed.

On the budget branch: the policy criticism was right, but the branch turns out to be a bound rather than a path — withDeadline fails any plan that reaches the deadline, so a preceding plan cannot both succeed and leave zero budget. It exists so a plan reached with nothing left is reported rather than firing a request it would immediately abandon. It now carries a justified v8 ignore saying that, instead of an assignment that reads as reachable policy.

That also needed a seam: clearStoredAuthForRelogin takes an optional budgetMs, which is what lets the shared-budget behaviour be tested without a five-second test. The new case asserts one 20ms budget bounds both keys rather than each getting a fresh one.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/App.tsx:1346

• This single global lock also suppresses clears for different servers. If server A's revocation is pending and the user switches to B or opens B's settings, clicking B's clear silently returns even though the underlying callback explicitly supports session switches. Key the lock by server.id (for example, a Set) so duplicate clears for A are blocked without dropping B's action.

      if (clearOAuthInFlightRef.current) return;
      clearOAuthInFlightRef.current = true;

clients/web/src/hooks/useOAuthRecovery.ts:1308

• This now skips resume-snapshot cleanup when the target is active but inspectorClient is currently null, which is reachable while a session is being created or torn down. The explicit clear then deletes storage but leaves stale OAuth recovery state behind. Include that case only while it still targets the same active session.

      } else if (!isActive) {
        clearOAuthResumeOnExplicitDisconnect();

clients/cli/src/clear-stored-auth-for-relogin.ts:130

• This branch is reachable: one plan can resolve just before the deadline and the next Date.now() can run after it; an injected budgetMs of zero reaches it directly as well. Suppressing it as dead bypasses the repository's branch-coverage policy for behavior that decides whether a later grant is attempted. Remove the ignore and add a test for exhaustion before the second plan.

    /* v8 ignore next 10 -- A bound, not a path: `withDeadline` fails any plan
       that reaches the deadline, so a preceding plan cannot both succeed and
       leave zero budget. This exists so a plan reached with nothing left is
       reported rather than firing a request it would immediately abandon. */
    if (needsNetwork && remainingMs <= 0) {

core/auth/revocation.ts:115

• RFC 8414 defines an omitted revocation_endpoint_auth_methods_supported as client_secret_basic, but returning [] does not enforce that with this SDK: selectClientAuthMethod treats an empty list as “metadata omitted” and may honor the registration's token_endpoint_auth_method. A DCR client registered with client_secret_post will therefore send body credentials to a revocation endpoint that omitted this field and only promised Basic, causing revocation to fail. Return ['client_secret_basic'] when the field is absent and cover the registered-post/omitted-list case.

[cliffhall]

Review round 20 — "no new comments", four in the suppressed block

Same pattern as round 16: the body reports none, the Suppressed comments (4) section carries four real ones. All addressed.

Finding	Outcome
Web in-flight lock was global	Fixed. A pending clear for server A silently swallowed a click on B's, even though the callback explicitly supports clearing a non-active server. Keyed by server id.
else if (!isActive) skipped resume cleanup when active-but-clientless	Fixed. Reachable while a session is being built or torn down; the clear then emptied storage and left OAuth recovery state pointing at credentials that no longer existed.
revocationAuthMethods returned [] for an omitted list	Fixed. [] does not mean "apply the RFC 8414 default" to selectClientAuthMethod — it means "the metadata said nothing", and the SDK then honors the registration's own token_endpoint_auth_method. The default is named literally now.
The v8 ignore I added last round was wrong	Removed, and the challenge was right. budgetMs: 0 reaches that branch directly — through a seam I had added myself the round before, which I then failed to account for when calling it unreachable. Covered by a test asserting no request is made and the unattempted key is reported.

That last one is the one I got wrong twice in two rounds, and it is worth naming: I justified an ignore on reachability grounds while holding the very injection point that makes it reachable. The repo's branch-coverage policy caught what my reasoning did not.

One nuance on the auth-methods fix: OAuthClientInformation does not carry token_endpoint_auth_method, so this path cannot construct the exact DCR-registered-post case today. Naming the default is still right — it is what keeps the behaviour correct if that type widens — and the comment says so rather than implying the scenario is live.

npm run validate and npm run coverage both green.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/auth/storage.ts:251

• listIssuers and getIssuerTokens are now unused production API: the atomic takeRevocationSnapshot refactor supplies all issuer slots directly, and repository search finds these methods only in their implementations and tests. Keeping them expands the exported OAuthStorage contract, forces every storage mock/implementation to carry dead methods, and preserves obsolete tests. Remove both methods and their associated implementation/test scaffolding.

core/auth/revocation.ts:376

• The PR body still contradicts this implementation in three places: it says revocation is sent “before wiping local state,” names the removed planOAuthRevocation API, and promises a storage-read failure returns an outcome. This function intentionally performs takeRevocationSnapshot (including the clear) before sending, and rethrows when that take/clear fails. Update the body to describe the current contract so future callers do not reintroduce the race or rely on a nonexistent API.