From 2ba25d63e348e2c7d3c20faeb168ba9d881b9328 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:08:29 -0400 Subject: [PATCH 1/3] fix(web): reset pending OAuth slots during render, not in an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useOAuthRecovery` cleared its two server-scoped pending-OAuth slots — the deferred re-auth and the step-up prompt — in a `useEffect` keyed on `activeServerId`. An effect runs after the commit, so a server switch painted one frame still carrying the previous server's re-auth banner or step-up prompt: an authorization affordance attached to the wrong server. Reset both during render with `useValueChange` instead. `activeServerId` is a primitive, so it satisfies the helper's `Object.is` stability requirement, and the previous value is read from the `setState` updater argument rather than from `sessionRef.current`, keeping the render-phase callback pure. `react-hooks/set-state-in-effect` did not fire on the original because the effect read the previous value through a ref rather than from a prop or state. The test records one entry per committed render and asserts both slots are empty in the first frame after the switch — the pre-existing `waitFor`-based tests pass against the effect version too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 71 ++++++++++++++++++- clients/web/src/hooks/useOAuthRecovery.ts | 30 +++++--- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 91b3f10d..e2ede1da 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -9,7 +9,7 @@ import type { import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; -import { useLayoutEffect, useRef } from "react"; +import { useEffect, useLayoutEffect, useRef } from "react"; import { renderWithMantine, act, waitFor } from "../test/renderWithMantine"; import { OAUTH_RESUME_KEY, @@ -169,9 +169,22 @@ interface HarnessProps { noFetchLog?: boolean; } +/** One committed render's view of the two server-scoped pending slots. */ +interface PendingCommit { + stepUpServerId: string | undefined; + reauthServerId: string | undefined; +} + interface Harness { api: () => OAuthRecovery; rerender: (next: HarnessProps) => void; + /** + * Every committed render's pending slots, in order. Recorded from an effect + * with no dependency array, so one entry lands per commit — which is what + * lets a test assert that a reset was visible in the *first* frame after a + * server switch rather than merely eventually (#2223). + */ + commits: PendingCommit[]; spies: { setActiveServerId: ReturnType; setFailedServerId: ReturnType; @@ -183,6 +196,7 @@ interface Harness { function harness(initial: HarnessProps = {}): Harness { let latest: OAuthRecovery | undefined; + const commits: PendingCommit[] = []; const spies = { setActiveServerId: vi.fn(), setFailedServerId: vi.fn(), @@ -236,6 +250,15 @@ function harness(initial: HarnessProps = {}): Harness { clearResultPanels: spies.clearResultPanels, setSourceScopedError: spies.setSourceScopedError, }); + // No dependency array: one entry per commit. `pendingReauth` is not part + // of the hook's public surface, so it is read back through the session ref + // the hook mirrors it into. + useEffect(() => { + commits.push({ + stepUpServerId: latest?.pendingStepUp?.serverId, + reauthServerId: sessionRef.current.pendingReauth?.serverId, + }); + }); return null; } @@ -246,6 +269,7 @@ function harness(initial: HarnessProps = {}): Harness { return latest; }, rerender: (next) => rerender(), + commits, spies, }; } @@ -322,6 +346,51 @@ describe("useOAuthRecovery", () => { expect(client.handleAuthChallenge).not.toHaveBeenCalled(); }); + it("clears both slots in the first committed frame after a server switch", async () => { + visibility.visible = false; + const client = fakeClient(); + const servers = [entry("a"), entry("b")]; + const h = harness({ servers, activeServerId: "a", client }); + + await act(async () => { + await h + .api() + .handleCommandScopedAuthRecovery( + new AuthRecoveryRequiredError( + AUTH_URL, + challenge("insufficient_scope"), + ), + { serverId: "a", source: "tool" }, + ); + }); + await act(async () => { + client.emit("authChallengeInteractive", { + challenge: challenge(), + authorizationUrl: AUTH_URL, + }); + }); + await waitFor(() => { + const last = h.commits[h.commits.length - 1]; + expect(last?.stepUpServerId).toBe("a"); + expect(last?.reauthServerId).toBe("a"); + }); + + const before = h.commits.length; + await act(async () => { + h.rerender({ servers, activeServerId: "b", client }); + }); + + // The point of the test: the *first* frame after the switch already + // shows both slots empty. Resetting them in an effect instead would + // commit one frame still carrying server "a"'s step-up prompt and + // deferred re-auth, and would pass a `waitFor` assertion just the same. + expect(h.commits.length).toBeGreaterThan(before); + expect(h.commits[before]).toEqual({ + stepUpServerId: undefined, + reauthServerId: undefined, + }); + }); + it("refuses a second step-up while one is open", 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 428352b7..d5a3d891 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -54,6 +54,7 @@ import { import { OAUTH_CALLBACK_PATH } from "../utils/oauthFlow"; import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs"; import type { PendingReauth } from "../utils/pendingReauth"; +import { useValueChange } from "./useValueChange"; import { authRecoveryRestoredMessage, authRecoveryAbandonedMessage, @@ -318,16 +319,25 @@ export function useOAuthRecovery({ sessionRef.current.pendingReauth = pendingReauth; }); - useEffect(() => { - const pending = sessionRef.current.pendingReauth; - if (pending && pending.serverId !== activeServerId) { - setPendingReauth(null); - } - const stepUp = sessionRef.current.pendingStepUp; - if (stepUp && stepUp.serverId !== activeServerId) { - setPendingStepUp(null); - } - }, [sessionRef, activeServerId]); + // Both slots are server-scoped, so switching servers has to clear whichever + // one belongs to the server we just left. Adjusted **during render** rather + // than in an effect: an effect only runs after the commit, so the frame that + // shows the new server would still paint the previous server's re-auth + // banner or step-up prompt — a modal-grade authorization affordance attached + // to the wrong server (#2223). `activeServerId` is a primitive, so it is + // referentially stable across renders that mean "no change", which is what + // `useValueChange`'s `Object.is` comparison requires. The previous value is + // read from the updater argument rather than from `sessionRef.current`, + // keeping the callback pure — a render can be replayed or abandoned, so it + // must not depend on external mutable state. + useValueChange(activeServerId, () => { + setPendingReauth((current) => + current && current.serverId !== activeServerId ? null : current, + ); + setPendingStepUp((current) => + current && current.serverId !== activeServerId ? null : current, + ); + }); const trySetPendingStepUp = useCallback( (next: PendingStepUp): boolean => { From 3c8fdb081405d7efe8008d62efdff8987d2ed9af Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:27:16 -0400 Subject: [PATCH 2/3] fix(web): drop the step-up retry when its prompt is cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry closure belongs to the prompt, but the two paths that clear the step-up slot as a *consequence* of something else — the server-switch reset and `resetOAuthRecoveryState` on disconnect — left `pendingStepUpRetryRef` installed. The next prompt to open without a retry of its own would inherit it, so authorizing an ambient step-up on server B ran the command server A was left mid-way through. Clear it in the existing mirror effect whenever the slot is null, so no arm has to remember. A ref write in an effect, since the render-phase reset must stay pure. Copilot on #2237. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall --- .../web/src/hooks/useOAuthRecovery.test.tsx | 43 +++++++++++++++++++ clients/web/src/hooks/useOAuthRecovery.ts | 13 ++++++ 2 files changed, 56 insertions(+) diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index e2ede1da..6fee4ec2 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -391,6 +391,49 @@ describe("useOAuthRecovery", () => { }); }); + it("does not carry the left server's retry into a later ambient step-up", async () => { + const client = fakeClient({ + handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }), + }); + const servers = [entry("a", {}, true), entry("b", {}, true)]; + const h = harness({ servers, activeServerId: "a", client }); + const retryA = vi.fn().mockResolvedValue(undefined); + + await act(async () => { + await h + .api() + .handleCommandScopedAuthRecovery( + new AuthRecoveryRequiredError( + AUTH_URL, + challenge("insufficient_scope"), + ), + { serverId: "a", source: "tool", retryOperation: retryA }, + ); + }); + expect(h.api().pendingStepUp?.serverId).toBe("a"); + + await act(async () => { + h.rerender({ servers, activeServerId: "b", client }); + }); + expect(h.api().pendingStepUp).toBeNull(); + + // Server B raises its own step-up, which carries no retry of its own. + await act(async () => { + client.emit("authChallengeInteractive", { + challenge: challenge("insufficient_scope"), + authorizationUrl: AUTH_URL, + }); + }); + await waitFor(() => expect(h.api().pendingStepUp?.serverId).toBe("b")); + + await act(async () => { + await h.api().handleStepUpAuthorize(); + }); + // Authorizing B must not run the command A was left mid-way through. + expect(retryA).not.toHaveBeenCalled(); + expect(toastTitles()).toContain("Permissions updated"); + }); + it("refuses a second step-up while one is open", 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 d5a3d891..1acb59a0 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -317,6 +317,19 @@ export function useOAuthRecovery({ useEffect(() => { sessionRef.current.pendingStepUp = pendingStepUp; sessionRef.current.pendingReauth = pendingReauth; + // The retry belongs to the prompt, so it dies with it. Every arm that + // dismisses a prompt deliberately drops the retry inline — but the two + // that clear the slot as a *consequence* of something else (the + // server-switch reset below, and `resetOAuthRecoveryState` on disconnect) + // would otherwise leave it installed, and the next prompt to open without + // one of its own would inherit it: authorizing an ambient step-up on + // server B would run the command server A was left mid-way through + // (Copilot on #2237). Clearing it here covers both without any of them + // having to remember. It is a ref write in an effect, which is where a ref + // write belongs — the render-phase reset above must stay pure. + if (pendingStepUp === null) { + pendingStepUpRetryRef.current = null; + } }); // Both slots are server-scoped, so switching servers has to clear whichever From 258e7f15267893ee7d5fae564eb0459bc7900ca7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 20:49:40 -0400 Subject: [PATCH 3/3] refactor(web): let trySetPendingStepUp own the step-up retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing the retry from the mirror effect raced: an async command continuation can open a prompt and install its retry before a previous commit's passive effect flushes, and that effect's stale `pendingStepUp === null` snapshot would then delete the new prompt's retry. Install it in `trySetPendingStepUp` instead, in the same synchronous step that opens the prompt — `null` for an ambient prompt, the operation for a command-scoped one. Every prompt comes through there and `handleStepUpAuthorize` is the only reader, so a slot cleared as a side effect can leave a stale closure behind harmlessly: the next prompt overwrites it. A refused prompt still installs nothing (#2165). Copilot round 3 on #2237. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd Signed-off-by: cliffhall --- clients/web/src/hooks/useOAuthRecovery.ts | 76 +++++++++++++---------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 1acb59a0..4dbbd342 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -317,19 +317,6 @@ export function useOAuthRecovery({ useEffect(() => { sessionRef.current.pendingStepUp = pendingStepUp; sessionRef.current.pendingReauth = pendingReauth; - // The retry belongs to the prompt, so it dies with it. Every arm that - // dismisses a prompt deliberately drops the retry inline — but the two - // that clear the slot as a *consequence* of something else (the - // server-switch reset below, and `resetOAuthRecoveryState` on disconnect) - // would otherwise leave it installed, and the next prompt to open without - // one of its own would inherit it: authorizing an ambient step-up on - // server B would run the command server A was left mid-way through - // (Copilot on #2237). Clearing it here covers both without any of them - // having to remember. It is a ref write in an effect, which is where a ref - // write belongs — the render-phase reset above must stay pure. - if (pendingStepUp === null) { - pendingStepUpRetryRef.current = null; - } }); // Both slots are server-scoped, so switching servers has to clear whichever @@ -352,8 +339,32 @@ export function useOAuthRecovery({ ); }); + /** + * Opens the step-up prompt, or refuses when one is already up. + * + * The retry is installed here rather than by the caller because it belongs + * to the prompt and must be written in the same synchronous step that opens + * it (#2237). Two things follow, and both are load-bearing: + * + * - A **refused** prompt installs nothing, so a command that was just told + * it cannot start does not replace the open prompt's operation (#2165). + * - An **ambient** prompt passes `null` and so *overwrites* whatever the + * previous prompt left in the ref. Nothing else has to clear it: the paths + * that drop the slot as a consequence of something else — the + * server-switch reset, `resetOAuthRecoveryState` on disconnect — can leave + * a stale closure behind, because the only thing that reads it is + * `handleStepUpAuthorize`, which needs a prompt, and every prompt comes + * through here. Clearing it from an effect instead would race: an async + * continuation can open a prompt and install its retry before a previous + * commit's passive effect flushes, and that effect's stale + * `pendingStepUp === null` snapshot would then delete the new prompt's + * retry (Copilot on #2237). + */ const trySetPendingStepUp = useCallback( - (next: PendingStepUp): boolean => { + ( + next: PendingStepUp, + retryOperation: (() => Promise) | null, + ): boolean => { if (sessionRef.current.pendingStepUp !== null) { notifications.show({ title: "Step-up authorization in progress", @@ -365,6 +376,7 @@ export function useOAuthRecovery({ return false; } setPendingStepUp(next); + pendingStepUpRetryRef.current = retryOperation; return true; }, [sessionRef], @@ -642,13 +654,18 @@ export function useOAuthRecovery({ }) => { const server = sessionRef.current.servers.find((s) => s.id === serverId); if (isStepUpConfirmation(challenge, server)) { - trySetPendingStepUp({ - challenge, - authorizationUrl, - serverId, - source, - enterpriseManaged: isEmaStepUp(challenge, server), - }); + // Ambient: nothing to retry, and passing `null` is what evicts a + // previous prompt's closure. + trySetPendingStepUp( + { + challenge, + authorizationUrl, + serverId, + source, + enterpriseManaged: isEmaStepUp(challenge, server), + }, + null, + ); return; } prepareOAuthRedirect({ @@ -713,23 +730,16 @@ export function useOAuthRecovery({ ) { return true; } - // The retry belongs to the prompt, so it is installed only once the - // prompt is actually open (#2165). `trySetPendingStepUp` REFUSES a - // second prompt while one is already up — writing the ref first meant - // the refused command's operation replaced the open prompt's, and - // authorizing that prompt then ran the command the user was just told - // could not start. - if ( - trySetPendingStepUp({ + trySetPendingStepUp( + { challenge: error.authChallenge, authorizationUrl: error.authorizationUrl, serverId: options.serverId, source: options.source, enterpriseManaged: isEmaStepUp(error.authChallenge, server), - }) - ) { - pendingStepUpRetryRef.current = options.retryOperation ?? null; - } + }, + options.retryOperation ?? null, + ); return false; }