diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0d0f7007..69438655 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -54,6 +54,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { // Same one-shot arming for the `/oauth/callback` token exchange, so a test can // exercise the callback-leg failure paths (#1808). let nextResumeRejection: unknown = null; + // And for `authenticate()`, the pre-redirect OAuth leg (discovery + DCR), so a + // test can exercise a failure there — the case that never reaches the "error" + // connection status (#2108). + let nextAuthenticateRejection: unknown = null; class FakeInspectorClient extends EventTarget { connect = vi.fn(() => { if (nextConnectRejection !== null) { @@ -108,6 +112,14 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { } return Promise.resolve(undefined); }); + authenticate = vi.fn(() => { + if (nextAuthenticateRejection !== null) { + const err = nextAuthenticateRejection; + nextAuthenticateRejection = null; + return Promise.reject(err); + } + return Promise.resolve(undefined); + }); checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(true); clearOAuthTokens = vi.fn().mockResolvedValue(undefined); } @@ -129,6 +141,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { __rejectNextResumeAfterOAuth: (err: unknown) => { nextResumeRejection = err; }, + // Test-only: arm the next authenticate() to reject (pre-redirect OAuth leg). + __rejectNextAuthenticate: (err: unknown) => { + nextAuthenticateRejection = err; + }, }; }); @@ -692,6 +708,10 @@ const rejectNextResumeAfterOAuth = ( } ).__rejectNextResumeAfterOAuth; +const rejectNextAuthenticate = ( + McpIndex as unknown as { __rejectNextAuthenticate: (err: unknown) => void } +).__rejectNextAuthenticate; + const fetchLogInstances = ( FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] } ).__fetchLogInstances; @@ -716,6 +736,33 @@ describe("App failed-connection card border (#1621)", () => { ); }); + // An OAuth leg that fails never reaches the `"error"` connection status — the + // handler tears the client down, leaving the session `"disconnected"` — so the + // flag is the only signal downstream that a connect attempt died. Without it + // the monitoring sidebar stays shut on exactly the failure the Network tab + // exists to explain (#2108). + it("flags the server when the OAuth authorization leg fails (#2108)", async () => { + const user = userEvent.setup(); + renderWithMantine(); + expect(screen.getByTestId("errored-server")).toHaveTextContent("none"); + + // 401 from the server -> App starts the authorization-code flow, whose + // discovery/DCR round-trip then fails (e.g. an invalid + // `/.well-known/oauth-authorization-server`). + rejectNextConnect(Object.assign(new Error("HTTP 401"), { status: 401 })); + rejectNextAuthenticate(new Error("discovery failed")); + await user.click(screen.getByText("connect")); + + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), + ); + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: expect.stringContaining("OAuth authorization failed"), + }), + ); + }); + it("clears the flag when a new connection attempt starts", async () => { // Report status "error" so the *second* connect click is treated as a fresh // attempt (not a disconnect of a live session), exercising the clear. @@ -2454,6 +2501,41 @@ describe("App OAuth callback issuer-binding failures (#1808)", () => { expect(screen.queryByText("Re-authentication required")).toBeNull(); }); + // The callback leg is a connect attempt too, so a failure there must flag the + // server — that is what opens the monitoring sidebar onto the OAuth requests + // (discovery, DCR and the token exchange, restored from the pre-redirect + // session) rather than leaving the user with only a banner (#2108). + it("flags the server when the callback leg fails (#2108)", async () => { + renderCallbackWithFailure(new Error("token exchange rejected")); + + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), + ); + }); + + // The other callback arm: the provider redirected back with an error instead + // of a code, so no token exchange is even attempted. Still a connect attempt + // that failed, so the server carries the same flag (#2108). + it("flags the server when the provider returns an error to the callback (#2108)", async () => { + writeOAuthResumeSnapshot({ + version: 1, + serverId: "A", + activeTab: "Tools", + authKind: "reauth", + tabUi: {}, + }); + window.history.replaceState( + {}, + "", + `${OAUTH_CALLBACK_PATH}?error=access_denied`, + ); + renderWithMantine(); + + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), + ); + }); + it("clears the stale OAuth state and reconnects when the affordance is used", async () => { const user = userEvent.setup(); renderCallbackWithFailure( diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index d566e65a..f2025854 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -2841,6 +2841,7 @@ function App() { if (!params.successful) { const pendingId = resumeSnapshot?.serverId; if (pendingId) { + setFailedServerId(pendingId); queueMicrotask(() => { showReAuthBanner(pendingId, generateOAuthErrorDescription(params)); }); @@ -2876,6 +2877,7 @@ function App() { await webOAuthStorage.load(); } catch (err) { connectStartRef.current = undefined; + setFailedServerId(server.id); queueMicrotask(() => { showReAuthBanner(server.id, err instanceof Error ? err : String(err)); }); @@ -2891,6 +2893,13 @@ function App() { }); } catch (err) { connectStartRef.current = undefined; + // The token exchange (or the re-handshake behind it) failed. Flag the + // server (#1621) so the monitoring sidebar opens onto the OAuth + // requests that explain it (#2108) — the rebuilt client restored the + // pre-redirect `auth` fetch entries from the session, so discovery, + // DCR and the token exchange are all there. Set before the + // classification fan-out below so every arm carries it. + setFailedServerId(server.id); if (isEmaClientNotConfiguredError(err)) { notifications.show({ title: `Cannot connect to "${server.name}"`, @@ -3091,6 +3100,13 @@ function App() { }); return; } + // The connect attempt failed, same as any other handshake error — + // flag the card (#1621) and, with it, open the monitoring sidebar + // onto the OAuth requests that explain the failure (#2108). This + // leg never reaches the `"error"` connection status (the + // `disconnect()` above settles it at `"disconnected"`), so this + // flag is the only signal the view has that a connect attempt died. + setFailedServerId(id); const message = authErr instanceof Error ? authErr.message : String(authErr); setConnectErrorMessage(message); diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 27477168..d7e2e5c1 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -1689,6 +1689,87 @@ describe("InspectorView", () => { expect(screen.queryByRole("radio", { name: "Console" })).toBeNull(); }); + // An OAuth failure is torn down by the parent, so the session rests at + // "disconnected" rather than "error" — the status gate the failure column + // used to carry never saw it, and the requests explaining the failure were + // unreachable (#2108). The parent's `erroredServerId` is the signal instead. + it("opens the monitoring sidebar to Network when OAuth fails without an error status (#2108)", async () => { + const { rerender } = renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { name: "Close monitoring sidebar" }), + ).toBeNull(); + + rerender( + , + ); + + expect( + await screen.findByRole("button", { name: "Close monitoring sidebar" }), + ).toBeInTheDocument(); + expect(screen.getByRole("radio", { name: "Network" })).toBeChecked(); + }); + + it("does not re-open the sidebar on a mount that starts already flagged (#2108)", () => { + // Same props as the failure above, but as the *initial* render: there is + // no transition, so a user who closed the column isn't fought on remount. + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { name: "Close monitoring sidebar" }), + ).toBeNull(); + }); + it("surfaces Console (stderr), not Network, in the failure column for a stdio server (#1621)", async () => { const stdioErr: ServerEntry = { id: "beta", diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 281ccb30..fef3d470 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -388,6 +388,13 @@ export interface InspectorViewProps { * the Servers screen draws a red border until another server is connected or * a new connection is attempted. Independent of `activeServer`, which the * parent clears on the failure's `disconnect` event. + * + * It is also the sole signal that opens the monitoring sidebar onto the + * failure's diagnostics (#2108). That used to be gated on the `"error"` + * connection status, which an OAuth failure never reaches — the parent drives + * that leg and tears the client down itself, settling the session at + * `"disconnected"`. So the parent must set this for *every* connect-attempt + * failure, auth legs included, and clear it as each new attempt starts. */ erroredServerId?: string; /** @@ -817,18 +824,33 @@ export function InspectorView({ // connecting/disconnected → error, never connected → error. This keeps the // auto-open aligned with the red-border (`erroredServerId`), which the parent // also sets only for connect-attempt failures. + // + // A connect attempt that fails *during OAuth* (#2108) never reaches the + // `"error"` status at all: the auth leg is driven from the parent, which + // tears the client down on failure, so the session settles at + // `"disconnected"`. So the failure arm also fires on the parent raising + // `erroredServerId` — the canonical "this server's last connect attempt + // failed" flag, which the parent clears at the start of every new attempt. + // Tracked through a ref for the same reason as the status: a mount that + // starts already flagged must not re-open a column the user closed. const prevStatusRef = useRef(connectionStatus); + const prevErroredServerIdRef = useRef(erroredServerId); useEffect(() => { const prev = prevStatusRef.current; prevStatusRef.current = connectionStatus; + const prevErrored = prevErroredServerIdRef.current; + prevErroredServerIdRef.current = erroredServerId; const becameConnected = connectionStatus === "connected" && prev !== "connected"; const becameError = - connectionStatus === "error" && prev !== "error" && prev !== "connected"; + (connectionStatus === "error" && + prev !== "error" && + prev !== "connected") || + (erroredServerId !== undefined && prevErrored !== erroredServerId); if (becameConnected || becameError) { setMonitorPinned(true); } - }, [connectionStatus, setMonitorPinned]); + }, [connectionStatus, erroredServerId, setMonitorPinned]); const appTools = useMemo(() => { return tools.filter((tool) => { @@ -924,7 +946,14 @@ export function InspectorView({ // (set by the parent only for connect-attempt failures, not mid-session // crashes) so a crash of a previously-connected server doesn't reorganize the // column into the failure tab set — matching the auto-open effect above. - const failed = connectionStatus === "error" && erroredServerId !== undefined; + // + // Keyed on `erroredServerId` alone rather than on the `"error"` status + // (#2108): an OAuth leg that fails is torn down by the parent, so the session + // rests at `"disconnected"` and a status gate would never see the failure — + // which is exactly the case a server developer needs the Network tab for. + // `!connected` is the only status condition left, and it holds vacuously + // whenever the flag is set (the parent clears it as each new attempt starts). + const failed = !connected && erroredServerId !== undefined; const monitorAvailable = useMemo(() => { if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t)); if (failed) {