fix(web): clear the step-up retry on redirect handoff; justify the voids

Copilot round 4:

- the EMA `interactive` arm kept `pendingStepUpRetryRef`, on a comment
  claiming it was "still on its way to a redirect". That is wrong twice:
  `prepareOAuthRedirect` returns before the navigation is resolved, so a
  pre-navigation failure leaves the operation for a later, unrelated
  step-up to re-run; and the full-page navigation destroys the closure on
  the success path anyway. It now clears the ref, matching the standard
  step-up below.
- the two surviving `void (async () => …)()` sites now state why the
  caller cannot await, as AGENTS.md requires — the comments had described
  only what the catch handles.
- the `randomUUID` fallback test restored an own descriptor that never
  existed (it is inherited from `Crypto.prototype`), leaving an own
  `randomUUID: undefined` that would push every later test in the file
  onto the fallback path. It deletes the temporary property instead, and
  asserts the global is a function again afterwards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/hooks/useOAuthRecovery.test.tsx

@@ -1970,6 +1970,40 @@ describe("useOAuthRecovery", () => {
       expect(retry).not.toHaveBeenCalled();
     });
 
+    it("drops the stored retry when an EMA step-up hands off to a redirect", async () => {
+      // The redirect unloads the page, so the closure cannot survive it
+      // anyway — and `prepareOAuthRedirect` returns before the navigation is
+      // resolved, so on its failure path a retained operation would be left
+      // for a later, unrelated step-up to re-run (#2165).
+      const handleAuthChallenge = vi
+        .fn()
+        .mockResolvedValueOnce({
+          kind: "interactive",
+          challenge: challenge("insufficient_scope"),
+          authorizationUrl: AUTH_URL,
+        })
+        .mockResolvedValue({ kind: "satisfied" });
+      const client = fakeClient({ handleAuthChallenge });
+      const h = harness({
+        servers: [entry("a", {}, true)],
+        activeServerId: "a",
+        client,
+      });
+      const retry = vi.fn().mockResolvedValue(undefined);
+      await openStepUp(h, "tool", retry);
+      await act(async () => {
+        await h.api().handleStepUpAuthorize();
+      });
+      expect(client.beginInteractiveAuthorization).toHaveBeenCalled();
+
+      await openStepUp(h, "tool");
+      await act(async () => {
+        await h.api().handleStepUpAuthorize();
+      });
+      expect(toastTitles()).toContain("Permissions updated");
+      expect(retry).not.toHaveBeenCalled();
+    });
+
     it("cancels back to the panel that asked", async () => {
       const client = fakeClient();
       const h = harness({ servers: [entry("a")], activeServerId: "a", client });

clients/web/src/hooks/useOAuthRecovery.ts

@@ -934,6 +934,8 @@ export function useOAuthRecovery({
       if (!serverId) {
         return;
       }
+      // `void`: an event listener cannot await, and the `.catch` below is
+      // what terminates the chain.
       void (async () => {
         const { challenge, authorizationUrl } = event.detail;
         const server = sessionRef.current.servers.find(
@@ -1338,6 +1340,10 @@ export function useOAuthRecovery({
         });
       }
     })().catch((err: unknown) => {
+      // `void`: an effect body cannot await, and this `.catch` is what
+      // terminates the chain — the justification AGENTS.md asks of every
+      // surviving `void`.
+      //
       // Everything inside has its own arm; this catches what sits *between*
       // them (#2165) — `setupClientForServer` throwing while it builds the
       // client, and the post-resume `checkAuthChallengeSatisfied`. Both land
@@ -1507,6 +1513,14 @@ export function useOAuthRecovery({
             authChallenge: outcome.challenge,
             recoverySource: stepUp.source,
           });
+          // Handing off, so the retry goes too — as the standard (non-EMA)
+          // step-up below already does. A full-page redirect cannot carry a
+          // closure across the navigation, so keeping it buys nothing on the
+          // success path; and `prepareOAuthRedirect` returns before the
+          // navigation is resolved, so on the failure path it would survive
+          // as a stale operation a later, unrelated step-up could re-run
+          // (#2165).
+          pendingStepUpRetryRef.current = null;
           return;
         }
         if (outcome.kind === "failed") {
@@ -1514,8 +1528,7 @@ export function useOAuthRecovery({
           // dismissed, so nothing will ever consume this operation — and a
           // *later* step-up does not overwrite the ref, so leaving it would
           // let that unrelated authorization silently re-run this command
-          // (#2165). The `interactive` arm above deliberately keeps it: that
-          // one is still on its way to a redirect.
+          // (#2165).
           pendingStepUpRetryRef.current = null;
           const failureMessage = emaStepUpFailureMessage(outcome.error.message);
           notifications.show({

clients/web/src/lib/oauthResume.test.ts

@@ -473,10 +473,17 @@ describe("oauthResume", () => {
       expect(token).toEqual(expect.any(String));
       expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
     } finally {
+      // `randomUUID` is inherited from `Crypto.prototype`, so there is
+      // normally no OWN descriptor to put back — restoring only when one
+      // existed would leave the `undefined` own property in place and force
+      // every later test in this file onto the fallback path.
       if (original) {
         Object.defineProperty(globalThis.crypto, "randomUUID", original);
+      } else {
+        delete (globalThis.crypto as { randomUUID?: unknown }).randomUUID;
       }
     }
+    expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function));
   });
 
   it("clearOAuthResumeSnapshot swallows removeItem failures", () => {