From d5ddaf8818ed40f5c4d9422d2bde4143ff5d22bf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 02:48:28 -0400 Subject: [PATCH] fix(web): clear the step-up retry on redirect handoff; justify the voids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 34 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 17 ++++++++-- clients/web/src/lib/oauthResume.test.ts | 7 ++++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 23a72829..ca25de3a 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/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 }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 2030b54f..05d4e2fd 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/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({ diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index ee3a31c0..fb8d1bd9 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/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", () => {