fix(web): terminate the five void-discarded OAuth recovery promises

[cliffhall]

Closes #2165

Five promises in useOAuthRecovery were discarded with void. That satisfies @typescript-eslint/no-floating-promises (#1959), but not the narrower rule AGENTS.md states: void is acceptable only when the callee already owns its failures or the caller genuinely cannot await, and with a one-line comment saying which. None of the five had that argument holding — each rejection became an unhandled rejection whose user-visible consequence was a wrong diagnosis or no response at all.

Each site now picks a user-visible outcome rather than taking a blanket .catch().

The five sites

Site	What it does now
prepareOAuthRedirect → beginInteractiveAuthorization	Clears the resume snapshot, flags the server (#1621), raises the re-auth banner. The snapshot is written immediately before the navigation; if the navigation never happens, the next page load reads it as an abandoned redirect and offers that diagnosis — which is wrong for a flow that failed before it started.
the authChallengeInteractive handler	Flags the server and raises the banner. A listener cannot be awaited, so this is the only place the failure can be reported; without it the challenge drew no UI response at all.
resumePendingReauth	Restores the pending slot and toasts. The slot is cleared on entry only to keep a tab-visible event and a reconnect from starting the same authorization twice — not because the recovery was delivered. It is still owed, so the next trigger retries it instead of losing it silently.
the /oauth/callback effect	Flags the server and raises the banner. This catches what sits between the arms that already have their own handling — setupClientForServer throwing, and the post-resume checkAuthChallengeSatisfied. Both land after the callback URL and the one-shot snapshot are spent, so it is reported here or nowhere.
handleStepUpAuthorize, EMA branch	Reported exactly as the existing outcome.kind === "failed" arm is — toast plus setSourceScopedError(stepUp.source, …) — so the two cannot disagree. StepUpAuthModal calls this as void onAuthorize(), so a rejection escaping reached nobody.

A sixth, smaller decision rides the last one: the stored retry (await retry()) gets its own catch rather than the outer one. The permissions leg succeeded by then, so a failure from there belongs to the retried command and is reported as that command's — routed to the panel that issued it — rather than dressed up as a step-up failure.

The two voids that survive

Both callers of resumePendingReauth still void it, and each now carries the justification AGENTS.md asks for: a visibility listener and an effect body cannot be awaited, and the callee terminates its own failures.

Copy

One new shared string, authRecoveryRetryFailedMessage in core/auth/oauthUx.ts (the shared copy home), re-exported through the web utils/oauthUx barrel. It is phrased as "will try again" rather than as a final failure, because the pending recovery is restored when it fires.

Testing

Seven new tests — one per failure arm, plus the retry split and the copy helper:

• the redirect that never starts clears the snapshot and banners
• an ambient challenge whose recovery check rejects banners instead of going silent
• a rejected resume restores the slot, and a second tab-visible event retries it
• a client rebuild that throws, and a post-resume scope check that rejects, both flag and banner
• an EMA step-up that rejects routes to the panel that asked
• a failed retry is reported as the command's failure, not the step-up's
• authRecoveryRetryFailedMessage appends only a non-blank detail

npm run local:gate green.

No screenshots: every arm here is a failure path that renders UI the client already has — the existing re-auth banner (#1621 red border included) and Mantine toasts. Nothing new is drawn; what changed is whether they are drawn at all, which is what the tests assert.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY

[Copilot]

🟡 Changes recommended

Navigation-state handling and asynchronous server/recovery races can misroute or lose OAuth recovery state.

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

Pull request overview

Improves web OAuth recovery by surfacing previously discarded promise failures and preserving retry behavior.

Changes:

• Adds targeted failure handling for five OAuth recovery paths.
• Restores deferred recovery after transient failures.
• Adds shared UX copy and regression tests.

File summaries

File	Description
core/auth/oauthUx.ts	Adds deferred-recovery failure copy.
clients/web/src/utils/oauthUx.ts	Re-exports the copy helper.
clients/web/src/test/core/auth/oauthUx.test.ts	Tests failure-message formatting.
clients/web/src/hooks/useOAuthRecovery.ts	Handles OAuth recovery rejections.
clients/web/src/hooks/useOAuthRecovery.test.tsx	Tests the new failure paths.

Review details

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

---

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

[cliffhall]

Round 1 — all three Copilot findings addressed

Mirrored here because inline replies go hidden once the fix is pushed and the threads go outdated.

1. beginInteractiveAuthorization could reject after navigation — correct, and the more serious of the three. Verified in core/mcp/oauthManager.ts: navigateToAuthorization was called ahead of createOAuthProvider() and clientInformation(), so a rejection from either arrived after the browser had already been handed over. Clearing the resume snapshot there would destroy the one thing the callback uses to identify the initiating server.

Fixed at the root rather than by adding a "did navigation start" flag for the call site to interpret: the navigation now resolves last, after every fallible step. Every rejection the method can produce is therefore a pre-navigation rejection, and the web catch's reading is unconditionally correct. It is also the better behaviour on its own terms — a provider that cannot be built, or client information that cannot be read, means the callback could not have completed anyway, so navigating first only bought the user a round trip to a dead end. New core test asserts neither navigateToAuthorization nor dispatchOAuthAuthorizationRequired fires when the setup rejects.

2. resumePendingReauth could overwrite a newer deferral — correct. The restore is now guarded on sessionRef.current.pendingReauth === null. The tab can go hidden during the awaits and deferAmbientReauth store a newer challenge describing the session as it is now; latest wins. New test holds handleAuthChallenge unresolved, defers a newer step-up challenge into the hidden tab, then rejects the first — the next trigger opens the step-up prompt for the newer one, and handleAuthChallenge is not called again.

3. the ambient catch re-read the active server — correct. serverId is read once, synchronously in the listener, and both the async body and the catch use that captured value. New test switches the active server from a to b mid-await: the banner is raised for a, and setFailedServerId is called with a and never b.

npm run local:gate green on the result — 7349 web tests under the ≥90% coverage gate, all smokes under Chromium and Firefox.

[Copilot]

🟡 Changes recommended

Concurrent recovery attempts can delete newer snapshots or restore stale challenges.

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

Review details

Suppressed comments (1)

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

• This clears whichever resume snapshot is current, not necessarily the one written by this invocation. prepareOAuthRedirect has no single-flight guard and can run from concurrent connect/challenge paths; if attempt A rejects after attempt B writes its snapshot (or requests navigation), A deletes B's callback routing state. Associate the snapshot with an attempt token and clear it only while that token is still current.

          clearOAuthResumeSnapshot();

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

[cliffhall]

Round 2 — both findings addressed (including the suppressed one)

1. resumePendingReauth could resurrect a challenge whose session ended (inline). Right on both halves. The awaits can outlive the session — a server switch or disconnect clears the slot deliberately — so the restore is now gated on the attempt still targeting the live session, with client identity part of the check: reconnecting to the same server builds a replacement InspectorClient that an id-only check would admit. Same shape as stillTargetsActiveSession() in clearServerOAuthAndDisconnect. And the emptiness check is now a functional update, so it sees queued state rather than the render-time value:

if (
  sessionRef.current.activeServerId === pending.serverId &&
  sessionRef.current.inspectorClient === client
) {
  setPendingReauth((prev) => prev ?? pending);
}

2. clearOAuthResumeSnapshot() cleared whichever snapshot was current (suppressed — reading those too). Also right, and the failure it describes is worse than the one the clear was added for: nothing single-flights prepareOAuthRedirect, so an attempt failing before navigation could delete a later attempt's callback-routing state and strand the redirect actually in flight.

Took the attempt-token remedy, with the token being the stored serialization rather than a new schema field: writeOAuthResumeSnapshot now returns exactly what it wrote, and clearOwnOAuthResumeSnapshot(token) removes it only while it is still current. That makes two byte-identical snapshots indistinguishable, which is deliberate and stated on the helper — identical bytes describe the same redirect for the same server with the same shell state, so clearing "the other one" reaches the same end state.

Five new tests: the ended-session restore, and four on the lib helper (only-its-own, no token, storage throw, no window).

npm run local:gate green — 514 tests in the last tier, all smokes under Chromium and Firefox.

[Copilot]

🟡 Changes recommended

Snapshot concurrency, stale retry state, misleading recovery copy, and incomplete connect teardown remain unresolved.

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

Review details

Suppressed comments (1)

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

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

• When this EMA attempt rejects, the prompt has already been dismissed but its pendingStepUpRetryRef is retained. A later ambient EMA step-up does not replace that ref, so if it succeeds it can unexpectedly rerun the old command. Clear the stored retry in this terminal failure path.

      } catch (err) {

• Files reviewed: 9/9 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Round 3 — four items, all addressed

1. The snapshot token was not unique (oauthResume.ts). Right, and my justification for the byte comparison was wrong: the snapshot records neither the authorization URL nor its state, so two concurrent redirects for the same server with unchanged UI serialize identically while being different attempts. OAuthResumeSnapshot now carries an optional attemptId, stamped per write and returned as the token; the clear matches on that. Optional on purpose — an older build's snapshot mid-redirect across an upgrade has none, and treating that as a match would strand a live callback.

2. The retry copy was shown when nothing was restored. Right, and my own ended-session test was what proved the toast lied. The message is now chosen by whether anything is left pending: authRecoveryRetryFailedMessage when the session survives (this attempt's challenge or the newer one it yielded to), and a new authRecoveryAbandonedMessage — "the session it belonged to has ended, reconnect to authorize again" — when it does not. The test asserts both directions.

3. A failed connect-scoped redirect left the connection spinning. Confirmed at both ends: inspectorClient skips the status = "error" transition for a connect auth-recovery error, and useConnectionLifecycle disconnects in its analogous arm for exactly this reason. The catch now disconnects first when preRedirectContext === "connect" — scoped to that context, since an ambient or step-up redirect failing does not mean the session is dead.

4. The EMA failure path retained pendingStepUpRetryRef (suppressed — checked those too). Right: the prompt is dismissed and a later step-up does not overwrite the ref, so an unrelated authorization could silently re-run the old command. Cleared in both terminal EMA failure arms — the failed outcome as well as the catch, since they are the same defect and the two should not disagree. The interactive arm deliberately keeps it: that one is still on its way to a redirect. Two tests drive fail-then-succeed and reject-then-succeed, asserting the stale retry never runs.

Gate rerunning; I will confirm green before requesting the next review.

[cliffhall]

Gate green on round 3: npm run local:gate passes — 7363 web tests under the ≥90% per-file coverage gate, plus cli/tui/launcher, all smokes under Chromium and Firefox, and the Storybook play functions.

[Copilot]

🟡 Changes recommended

A failed EMA redirect can retain and later execute a stale command retry.

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

Review details

Suppressed comments (1)

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

• The interactive arm is not guaranteed to navigate: prepareOAuthRedirect returns before beginInteractiveAuthorization settles, and its new catch handles a pre-navigation rejection. Keeping the command retry here leaves it in pendingStepUpRetryRef on that failure path; a later ambient EMA step-up does not replace the ref and can execute this stale command. A full-page redirect cannot preserve the closure anyway, so clear it before handing off.

          // (#2165). The `interactive` arm above deliberately keeps it: that
          // one is still on its way to a redirect.

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

[cliffhall]

Round 4 — three items, all addressed

1. The EMA interactive arm kept the stored retry (suppressed). Right, and my comment justifying it was wrong on both counts: prepareOAuthRedirect returns before the navigation is resolved, so a pre-navigation failure leaves the operation for a later, unrelated step-up to execute; and on the success path the full-page navigation destroys the closure anyway, so keeping it buys nothing. The arm now clears the ref, which also makes it consistent with the standard (non-EMA) step-up below, which has always cleared it right after handing off. New test: an EMA step-up that redirects, then a second that succeeds — the first attempt's retry never runs.

2. The surviving voids described the catch, not the justification. Fixed — both now say why the caller cannot await: an effect body for the /oauth/callback chain, an event listener for the authChallengeInteractive one, each naming the .catch that terminates it. Those are the only two void (async …)() sites in the file; the two void resumePendingReauth(pending) calls already carried theirs.

3. The randomUUID test cleanup restored a descriptor that never existed. Correct — it is inherited from Crypto.prototype, so getOwnPropertyDescriptor returned undefined and the guarded restore was a no-op, leaving an own randomUUID: undefined that would have pushed every later test in the file onto the fallback path (including the one asserting the UUID path, which would then have passed for the wrong reason). It deletes the temporary property when there was no own descriptor, and asserts the global is a function again afterwards so a future regression fails there rather than in a neighbour.

npm run local:gate green — 7364 web tests under the ≥90% gate, all smokes under Chromium and Firefox.

[Copilot]

🟡 Changes recommended

The shared EMA retry reference can be replaced during authorization awaits, causing newer retries to be executed, misrouted, or discarded by an older attempt.

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

Review details

Suppressed comments (3)

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

clients/web/src/lib/oauthResume.ts:190

• This docstring says the function returns the exact stored serialization, but the implementation returns only attemptId; the stored serialization is the JSON string containing the entire snapshot. Describe the identifier return value directly so callers do not infer byte-comparison semantics.

 * Persist the snapshot, returning the exact serialization stored — the
 * "attempt token" {@link clearOwnOAuthResumeSnapshot} matches against (#2165).

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

• This clears whichever retry is currently in the shared ref, not necessarily the retry owned by this authorization attempt. While handleAuthChallenge is pending, the dismissed prompt allows a newer command to create another step-up and replace the ref; if this older attempt then returns failed, the newer prompt remains but its command retry is silently lost. Clear only the retry captured for this attempt.

          pendingStepUpRetryRef.current = null;

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

• The rejection arm has the same retry-ownership race: a newer step-up can replace pendingStepUpRetryRef.current while this attempt awaits handleAuthChallenge, and this catch then deletes the newer command's retry. Associate the retry with the attempt before awaiting and avoid clearing a value installed by a later prompt.

        pendingStepUpRetryRef.current = null;

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

[cliffhall]

Round 5 — retry ownership, plus the stale docstring

1. The shared pendingStepUpRetryRef could be replaced mid-flight (one defect, three sites). Right. The EMA branch dismisses the prompt before its awaits, which frees the step-up slot, so a newer command can open its own prompt and install its own retry while the first authorization is still running. The satisfied arm would then execute that newer command under this authorization and report its failure to this attempt's source; the failed arm and the catch would delete it.

Took the capture-before-awaiting option, which removes all three ref accesses rather than guarding each:

setPendingStepUp(null);
const retry = pendingStepUpRetryRef.current;
pendingStepUpRetryRef.current = null;
// every arm below uses the local `retry` and touches the ref never

The attempt now owns its retry for its whole lifetime — the satisfied arm runs its operation, the terminal arms let it die with the attempt, and a later prompt's retry is never read or cleared. That also removed the interactive-arm clear I added last round: the value is already out of the ref by then. The standard (non-EMA) path redirects and clears synchronously with no awaits in between, so it has no such window.

New test drives the interleaving: hold the first EMA handleAuthChallenge unresolved, open a second step-up from a different source with its own retry, then fail the first. The old attempt reports to tool and runs nothing; the newer prompt's retry survives and runs on its own authorization.

2. writeOAuthResumeSnapshot's docstring (suppressed) still described the byte-comparison token from two rounds ago — it would have led a caller straight back to the semantics we removed. Rewritten to say it returns the attemptId, and that a fresh one is minted per write so two attempts stay distinguishable even when their snapshots are byte-identical.

npm run local:gate green — 7365 web tests under the ≥90% gate, all smokes under Chromium and Firefox.

[Copilot]

🔵 Needs a closer look

A refused concurrent step-up can overwrite the open prompt’s retry before it is captured and executed.

Review details

Suppressed comments (1)

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

• This capture still does not guarantee that the retry belongs to stepUp. handleCommandScopedAuthRecovery assigns pendingStepUpRetryRef.current at line 693 before trySetPendingStepUp; when another command requests step-up while this prompt is already open, trySetPendingStepUp refuses the new prompt but its retry has already replaced this prompt's retry. Authorizing the original prompt therefore captures and executes the rejected command's operation. Reserve the prompt and retry atomically (or assign the retry only after trySetPendingStepUp confirms success) so the two cannot diverge.

      const retry = pendingStepUpRetryRef.current;
      pendingStepUpRetryRef.current = null;

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

[cliffhall]

Round 6 — the other half of the retry pairing

Copilot moved this to 🔵 Needs a closer look with no new inline comments and one suppressed finding, which is real and pre-existing rather than something last round introduced.

handleCommandScopedAuthRecovery wrote pendingStepUpRetryRef before calling trySetPendingStepUp, and that helper refuses a second prompt while one is already open. So a refused command's operation replaced the open prompt's, and authorizing that prompt ran the command the user had just been told could not start.

The retry now goes in only on a prompt that actually opened:

if (trySetPendingStepUp({ /* … */ })) {
  pendingStepUpRetryRef.current = options.retryOperation ?? null;
}

Which makes the pairing hold in both directions — the prompt and its retry are installed together, and last round's capture takes them out together. A refused command leaves the open prompt's retry untouched; an accepted prompt with no retry still clears a stale one.

New test: open a step-up with a retry, have a second refused, authorize — the first command runs and the refused one never does. Without the fix that test fails in both directions.

npm run local:gate green — 7366 web tests under the ≥90% gate, all smokes under Chromium and Firefox.

[Copilot]

🔵 Needs a closer look

OAuth navigation, session recovery, and concurrent retry ownership span multiple stateful layers and warrant final human validation.

Review details

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

[cliffhall]

Copilot review (#pullrequestreview-5057258482): 🔵 Needs a closer look — "OAuth navigation, session recovery, and concurrent retry ownership span multiple stateful layers and warrant final human validation."

Fair ask — three stateful layers, zero inline comments, so the question is behavioral rather than about any particular line. I smoke tested all three against a live OAuth server in a clean worktree at the PR head (26fdc48b), driving a real headless browser through the prod --web bundle. Results below, including one finding that goes against the PR description.

npm run local:gate is green on that worktree (exit 0).

Setup

test-servers/configs/oauth-step-up-demo.json (combined AS + resource, DCR, refresh tokens) with the catalog entry pinned to a narrow oauth.scopes: "mcp", so get_temp — which requires weather:read — raises a genuine insufficient_scope step-up rather than a synthetic one. OAuth storage isolated per run via MCP_STORAGE_DIR. Snapshot writes are observed by hooking sessionStorage.setItem and reporting out to the driver, because reading storage from a route handler deadlocks against the pending navigation.

1. OAuth navigation ✅

Connect → redirect to the AS → consent → /oauth/callback → connected.

This is the one that could have regressed: beginInteractiveAuthorization was reordered so navigateToAuthorization resolves last. It still hands the browser over and still comes back.

2. Session recovery ✅

The resume snapshot is written pre-navigation carrying a real attemptId, and is consumed by the callback (sessionStorage empty afterwards) — the one-shot property the new clearOwnOAuthResumeSnapshot scoping depends on.

The step-up redirect writes its own snapshot with a distinct attemptId and recoverySource: "tool". That is the observable half of the attemptId design: two attempts for the same server with identical shell state are distinguishable, which a byte comparison could not have done.

✓ snapshot written pre-navigation, attemptId=47d465fb… serverId=step-up-demo activeTab=Servers
✓ snapshot consumed by the callback (one-shot session recovery)
✓ step-up wrote its OWN snapshot, attemptId=e8dd9fa7… recoverySource="tool"

After the step-up callback, the Tools tab, the selected tool and the argument values are restored:

3. Retry ownership ✅ (with a caveat the PR already states)

Real insufficient_scope step-up, correctly showing mcp as already granted and weather:read as new (SEP-2350 union):

Authorize → consent → return → get_temp succeeds with no second challenge, i.e. the widened grant took:

Worth being precise about what this does and does not cover. The standard step-up ends in a full-page redirect, so the retry closure cannot survive it — as the PR says in as many words — and the call is correctly not auto-run on return; the user re-runs it against the widened scope. The retry-ownership changes (const retry = … lifted before the awaits, the trySetPendingStepUp(...) return value gating the ref write) live on the EMA branch, which authorizes in place. That branch needs an enterprise IdP and is not reachable from any test fixture, so it stays covered by the unit tests — and the two that matter for Copilot's "concurrent" wording are there and named for it:

• never touches a retry installed by a newer step-up
• keeps the open prompt's retry when a second step-up is refused

Finding: three of the five arms are not reachable from the browser

This qualifies the PR description, which frames each site as having a "user-visible consequence". That is true of the consequence, but I could not produce the failure under any user-facing condition:

• prepareOAuthRedirect → beginInteractiveAuthorization. After the snapshot is written, every remaining await (createOAuthProvider, clientInformation) resolves from in-process caches, and the session-state push is Promise.resolve()-wrapped and fire-and-forget. I blocked the whole backend API surface the instant the snapshot landed — zero requests followed and the redirect proceeded normally.
• The post-resume checkAuthChallengeSatisfied. Its storage.getTokens() reads the already-loaded in-memory store; blocking GET /api/storage/oauth lands on the pre-existing webOAuthStorage.load() catch one statement earlier, which flags and banners on v2/main too. I built the pre-fix tree and confirmed it behaves identically, so that experiment does not discriminate.

Not an objection — these are terminations of promises ESLint proved were floating, and unreachable-by-fault-injection is exactly the case where unit tests are the only honest instrument. The seven new tests are the right call. I'm flagging it so nobody later reads "user-visible" as "reproducible by hand" and burns the afternoon I did.

What is reachable is the recovery UI all five arms feed into — red border (#1621), banner, and the connect toggle correctly torn down rather than left spinning:

Verdict

Navigation, session recovery and the reachable half of retry ownership all behave correctly at the PR head, with no console errors or unhandled rejections across any run. Nothing here changes my read of the diff.