fix: key the OAuth clear's active-session check on the storage key

[cliffhall]

Closes #2217

The mismatch

OAuth state is keyed by server URL. clearServerOAuthAndDisconnect decided "is this the active connection" from the catalog entry id. Those are different identities and nothing keeps them in sync — core/mcp/serverList.ts enforces no URL uniqueness, so two entries with distinct ids against one URL are a supported state, and a natural one: separate entries are how a user keeps different names, custom headers or per-server settings against the same server.

With entry a connected and entry b inactive, clearing b deleted the shared URL-keyed blob (a's tokens, DCR client id, PKCE state), revoked a's grant at the authorization server over RFC 7009 (#2144) — and then took the inactive branch, because b.id !== activeServerId. No live-client clear, no disconnect, no session cleanup, no notice. Entry a was left connected on an in-memory access token whose persisted state was gone and whose grant may be dead, with the break surfacing later and somewhere else: the next refresh, the next 401, the next reload.

What changed

The branch now compares the OAuth storage key of the cleared entry against the active entry's, and treats a match as affecting the active session — live-client clear, disconnect, session cleanup — exactly as clearing the active entry does. The toast says why the session went away.

The existing id and client-identity checks stay on top of it: they guard the separate stale-session race from #2144 (a suspended callback applying to a session the user switched to), which is a different axis and still real. One adjustment there — the id stillTargetsActiveSession revalidates is now the active id snapshotted alongside isActive, not the cleared entry's, since for a shared-key clear those differ and the session being protected is the active one.

The alternative the issue raises — prohibiting duplicate URL keys in serverList.ts — was not taken: it removes a legitimate workflow to fix a bug in the consumer, and it cannot repair catalogs that already hold duplicates.

Tests

• disconnects the active session when clearing an entry that shares its URL — two entries, same URL, one connected; asserts the active client is disconnected and the clear routed through it (isActiveConnection: true). Verified to fail against the old id-only check.
• does not disconnect a session switched to during a shared-URL clear — the new branch still snapshots the session it acted on, so a switch mid-flight must not drag the cleanup onto the new one.
• The existing leaves a live session alone when clearing another server case was fixed at the fixture: it used entry("a")/entry("b"), which share a URL, so it was asserting the buggy behavior. It now clears an entry with its own URL, which is what "another server" was meant to mean.

npm run local:gate passes. (One integration test, matches responses to requests when a sibling listener fires a request inside the same connect event, flaked on the first run and passes in isolation and on re-run; it is unrelated to this diff.)

Screenshots

None. The only visual surface is the "OAuth state cleared" toast string, which the tests assert directly; reproducing the shared-URL case in a live browser needs two catalog entries with a completed OAuth grant against one authorization server, which is a disproportionate setup for a one-sentence copy change. Everything else in the diff is behavior with no rendered surface.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VGbw6TezNWnXZSCSp7sqb4

[Copilot]

🟡 Changes recommended

The shared-URL toast can incorrectly claim that a session was disconnected when disconnect was skipped or failed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes OAuth clearing for catalog entries sharing the active server’s URL.

Changes:

• Compares OAuth storage keys instead of only entry IDs.
• Disconnects affected active sessions while preserving stale-session guards.
• Adds shared-URL and session-switch regression tests.

File summaries

File	Description
clients/web/src/hooks/useOAuthRecovery.ts	Detects shared OAuth state and disconnects affected sessions.
clients/web/src/hooks/useOAuthRecovery.test.tsx	Covers shared URLs and in-flight session switching.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

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

[cliffhall]

Fixed in fcec17f..HEAD — good catch. The message now states the credential impact rather than the disconnect: The active session authorizes against the same URL, so its stored tokens went too — reconnect to run a fresh authorization flow. That holds whether the disconnect ran, was skipped by the stale-session guard, or rejected (which surfaces its own toast). Comment added at the source saying why.

[cliffhall]

Copilot review round 1 — 1 comment, addressed.

useOAuthRecovery.ts:1477 — the shared-URL toast asserted a disconnect that may not have happened. Correct. The stale-session guard deliberately skips disconnect() after a mid-flight server switch, and disconnect() can reject (that failure already gets its own toast). The message now states what is invariably true — the shared stored tokens are gone, so the session must reconnect — instead of the disconnect:

Stored OAuth state was removed for "Server b". The active session authorizes against the same URL, so its stored tokens went too — reconnect to run a fresh authorization flow.

A comment at the source records why it is worded that way. Test assertion updated to match; npm run validate is green and the full local:gate passed on the previous revision (this round is a string plus a comment).

[Copilot]

🔵 Needs a closer look

The existing ID-keyed in-flight guard still permits concurrent clears of the same shared OAuth state.

Review details

Suppressed comments (1)

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

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

• affectsActiveSession makes distinct catalog IDs sharing a URL operate on the same OAuth blob and live client, but App.tsx:1352-1383 still deduplicates in-flight clears only by server.id. Clearing both duplicate entries while revocation is pending can therefore run concurrent store/client clears and emit duplicate or conflicting teardown toasts—the exact race that runClear is intended to prevent. Key that guard by the OAuth storage key (while retaining an ID fallback for non-OAuth configs), or otherwise serialize clears that affect the same active session.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 2 — 1 suppressed comment, addressed.

useOAuthRecovery.ts:1408 (suppressed) — App.tsx's in-flight clear guard still dedupes by server.id. Correct, and it is the same identity mismatch this PR fixes, one layer up: two entries against one URL share one blob, one grant and one revocation, so an id-keyed guard let exactly the race runClear exists to prevent through between them — concurrent store writes, concurrent RFC 7009 requests, two contradictory teardown toasts.

Fixed as suggested, keyed by the OAuth storage key with an id fallback. The key derivation is a new pure helper, clients/web/src/utils/oauthClearKey.ts, rather than an expression inline in App.tsx — App.tsx is outside the coverage whitelist, so a helper is what makes the rule testable:

• two entries with the same URL key identically;
• different URLs stay distinct;
• a config with no OAuth server URL (stdio) falls back to its entry id, so those stay distinct from each other rather than collapsing onto one shared key;
• both forms are prefixed (url: / id:) so a user-supplied id cannot impersonate a URL.

Four unit tests cover those. npm run local:gate is green.

[Copilot]

🟡 Changes recommended

The new App-level concurrent-clear suppression lacks required behavioral test coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Added, as suggested — App.test.tsx: suppresses a second clear for another entry with the same URL, and allows one after it settles. It connects entry A, holds its clear open on a deferred clearOAuthTokens, drives a clear for entry B (different catalog id, same URL) before the first settles, and asserts the underlying clear ran exactly once — then once more after settlement, so the key is proven released rather than leaking a permanently dead control. Verified to fail against the id-keyed guard.

[cliffhall]

Copilot review round 3 — 1 comment, addressed.

App.tsx:1387 — the same-URL suppression had no App-level behavioral test. Fair: oauthClearKey's unit tests prove the derivation, but a regression that dropped the has check, added or deleted the wrong key, or invoked the clear twice would still have passed. Added the case as described:

App.test.tsx → suppresses a second clear for another entry with the same URL, and allows one after it settles — connect entry A, hold its clear open on a deferred clearOAuthTokens, drive a clear for entry B (different catalog id, same URL) before the first settles, assert the underlying clear ran exactly once; then settle and clear again, asserting it runs, so the key is proven released rather than leaking a permanently dead control. Verified to fail against the id-keyed guard.

Two supporting changes it needed, both worth flagging:

• an open-settings-b control on the mocked InspectorView — the only way to reach a settings modal for a server other than the active one;
• @inspector/core/mcp/remote/index.js is now a partial mock in App.test.tsx. getWebProxiedFetch reaches for createRemoteFetch there, and the bare stub threw a missing-export error before the clear under test ever ran, surfacing as the generic "Could not clear the stored OAuth state" toast. Worth knowing: any future App-level test of this path would have hit the same wall.

The case carries an explicit 20s timeout and delay: null — it drives three full modal interaction sequences against the whole App tree and runs past the 5s default when the suite is under load.

npm run local:gate is green.

Unrelated, found while running the gate: ServerImportJsonModal > guards against a live edit made before the debounce re-validates is flaky on a clean origin/v2/main tree (its comment states it must click while the debounce is still pending, and nothing enforces that). Filed as #2241 rather than touched here.

[Copilot]

🟡 Changes recommended

The new App test contains two unjustified double casts that bypass required type contracts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Fixed. Removing the cast immediately proved the point: tsc reported the mock was missing reorderServers and importSource, so App code calling either would have read undefined with this test still passing type-check. The full UseServersResult shape is supplied now, matching the mockServersWith helper above.

[cliffhall]

Fixed — clientInstances[0] as EventTarget & { clearOAuthTokens: ReturnType<typeof vi.fn> }. Typechecks clean.

[cliffhall]

Copilot review round 4 — 2 comments, both addressed. Both were correct and both are the repo's own rule (AGENTS.md: an unjustified as unknown as is not acceptable in review).

App.test.tsx:4261 — the useServers double cast hid missing members. The more useful of the two: removing the cast made tsc report the mock was missing reorderServers and importSource, so App code calling either would have read undefined with this test still passing type-check — exactly the failure mode named. The full UseServersResult shape is supplied now, matching the mockServersWith helper further up the file.

App.test.tsx:4304 — the client double cast. Now an intersection: clientInstances[0] as EventTarget & { clearOAuthTokens: ReturnType<typeof vi.fn> }. The instances are already typed EventTarget, so this names the test-only spy without erasing the rest.

npm run local:gate is green.

One note on the gate, since it is relevant to reading CI here: three earlier runs of this branch went red on 5s test timeouts in files this PR does not touch (ServerSettingsModal, TaskListPanel, AppsScreen, ServerImportJsonModal). Load average on the machine was 46 with a second full gate running concurrently; the run that produced the green above was on a quiet machine. ServerImportJsonModal > guards against a live edit made before the debounce re-validates is genuinely flaky independent of load — it fails on a clean origin/v2/main tree — and is filed as #2241.

[Copilot]

🟢 Approval recommended

The implementation addresses the identity mismatch with focused regression coverage and preserves stale-session safeguards.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Mutable catalog URLs can diverge from the live client’s OAuth key, reintroducing unsafe clearing and concurrency behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Same root cause, same fix. Both consumers now call one exported resolveOAuthClearIdentity, which returns the inFlightKey alongside the affects-active-session answer, so an active-session clear locks on the client's key (url:X) rather than the edited entry's (url:Y) and collides with a concurrent clear of entry B at X, as it must. Two resolver tests cover it.

[cliffhall]

Correct, and confirmed: onConfigSubmit (App.tsx) writes the catalog on an edit and does not rebuild the client, so an edited-while-connected entry reads a URL the session never authorized against. The comparison now resolves from InspectorClient.getTransportConfig() — the config the client was constructed with, which is what its credentials are keyed under — and falls back to the catalog entry only when there is no client. Covered by a hook test: A edited to Y while its client is still on X, clearing entry B at X disconnects.

[cliffhall]

Copilot review round 6 — 2 comments, one root cause, both addressed. (Round 5 was a zero-comment approval; this round was requested as a confirming pass and turned up something real, which is why we run two.)

The premise checks out. onConfigSubmit in App.tsx writes the catalog on an edit and does not rebuild the client, and ServerCard's Edit is not gated on connection state. So after A connects to X and is edited to Y, the entry reads Y while the live session is still authorized against X. Both consumers read the entry, so both were wrong the same way:

• the hook compared a cleared entry against A's entry URL, so clearing another entry still at X missed the match — deleting and revoking the live client's X-keyed credentials without disconnecting it, which is exactly the failure this PR exists to prevent;
• runClear locked on the entry's key, so a clear of edited-A (url:Y) and a clear of entry B at X (url:X) took different locks while both performed the same X-keyed operation through the same live client.

Fix. InspectorClient.getTransportConfig() returns the config the client was constructed with — immutable, and what its credentials are keyed under. Both consumers resolve from that, falling back to the catalog entry only when there is no client.

And the rule moved into one place. resolveOAuthClearIdentity (in utils/oauthClearKey.ts) now returns { isActive, sharesActiveOAuthKey, affectsActiveSession, inFlightKey }, and the hook and runClear both call it. Two copies of this rule that disagree is precisely the class of bug #2217 is — and the two consumers each re-deriving it is how this round's finding came to exist at all, so consolidating is the durable half of the fix.

Tests — three, each verified to fail against the entry-only lookup:

• resolver: the client's URL wins over an edited entry (and an entry at the entry's new URL is correctly not shared);
• resolver: an active-session clear locks on the client's key, and a clear of the entry sharing that key takes the same lock;
• hook: A edited to Y with its client still on X — clearing entry B at X disconnects.

Both fakes gained a getTransportConfig to match the real client.

npm run local:gate is green.

[Copilot]

🟢 Approval recommended

The URL-key mismatch and related races are addressed consistently with focused regression coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

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