fix(web): open the monitoring sidebar when a connect fails during OAuth

clients/web/src/App.test.tsx

@@ -13,6 +13,7 @@ import {
   act,
 } from "./test/renderWithMantine";
 import userEvent from "@testing-library/user-event";
+import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
 
 // Spy on the toast layer so the progress-notification tests can assert the
 // show/update calls without mounting Mantine's <Notifications/> portal.
@@ -48,18 +49,24 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() }));
 vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
   const actual =
     await importOriginal<typeof import("@inspector/core/mcp/index.js")>();
-  // When set, the next `connect()` rejects with this value (one-shot), so a test
-  // can exercise the handshake-failure path. Cleared after it fires.
-  let nextConnectRejection: unknown = null;
+  // Each armed value makes one `connect()` reject, in FIFO order, so a test can
+  // exercise the handshake-failure path — or a two-connect sequence such as the
+  // auth-recovery retry, where the first call rejects with the recovery error
+  // and the retry behind it fails too. A queue rather than a single slot: with
+  // one slot the second arming would overwrite the first, and the retry would
+  // silently succeed.
+  const connectRejections: unknown[] = [];
   // 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) {
-        const err = nextConnectRejection;
-        nextConnectRejection = null;
-        return Promise.reject(err);
+      if (connectRejections.length > 0) {
+        return Promise.reject(connectRejections.shift());
       }
       return Promise.resolve(undefined);
     });
@@ -108,6 +115,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);
   }
@@ -122,13 +137,18 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
     // Test-only handle so the test can grab the live instance and fire events.
     __clientInstances: instances,
     // Test-only: arm the next connect() to reject (handshake-failure path).
+    // Call it more than once to arm consecutive calls.
     __rejectNextConnect: (err: unknown) => {
-      nextConnectRejection = err;
+      connectRejections.push(err);
     },
     // Test-only: arm the next resumeAfterOAuth() to reject (callback-leg failure).
     __rejectNextResumeAfterOAuth: (err: unknown) => {
       nextResumeRejection = err;
     },
+    // Test-only: arm the next authenticate() to reject (pre-redirect OAuth leg).
+    __rejectNextAuthenticate: (err: unknown) => {
+      nextAuthenticateRejection = err;
+    },
   };
 });
 
@@ -682,15 +702,21 @@ const clientInstances = (
   McpIndex as unknown as { __clientInstances: EventTarget[] }
 ).__clientInstances;
 
-const rejectNextConnect = (
-  McpIndex as unknown as { __rejectNextConnect: (err: unknown) => void }
-).__rejectNextConnect;
+// The mock factory adds three test-only arming hooks to the module namespace.
+// Intersecting with `typeof McpIndex` keeps the real module's shape checked and
+// narrows this to a single cast — `as unknown as` would discard the former and
+// is what AGENTS.md rules out.
+type ArmingHooks = typeof McpIndex & {
+  __rejectNextConnect: (err: unknown) => void;
+  __rejectNextResumeAfterOAuth: (err: unknown) => void;
+  __rejectNextAuthenticate: (err: unknown) => void;
+};
 
-const rejectNextResumeAfterOAuth = (
-  McpIndex as unknown as {
-    __rejectNextResumeAfterOAuth: (err: unknown) => void;
-  }
-).__rejectNextResumeAfterOAuth;
+const { __rejectNextConnect: rejectNextConnect } = McpIndex as ArmingHooks;
+const { __rejectNextResumeAfterOAuth: rejectNextResumeAfterOAuth } =
+  McpIndex as ArmingHooks;
+const { __rejectNextAuthenticate: rejectNextAuthenticate } =
+  McpIndex as ArmingHooks;
 
 const fetchLogInstances = (
   FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] }
@@ -716,6 +742,63 @@ 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(<App />);
+    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"),
+      }),
+    );
+  });
+
+  // The auth-recovery arm re-connects from inside the outer `catch`, so before
+  // #2108 a rejection there escaped `onToggleConnection` entirely as an
+  // unhandled rejection: no toast, no red border, no sidebar.
+  it("flags the server when the satisfied-challenge retry fails (#2108)", async () => {
+    const user = userEvent.setup();
+    renderWithMantine(<App />);
+
+    // First connect asks for recovery; the fake reports the challenge already
+    // satisfied, so App retries the connect — and that retry fails.
+    rejectNextConnect(
+      new AuthRecoveryRequiredError(
+        new URL("https://as.example.com/authorize"),
+        {
+          reason: "unauthorized",
+        },
+      ),
+    );
+    rejectNextConnect(new Error("handshake failed after re-authorization"));
+    await user.click(screen.getByText("connect"));
+
+    await waitFor(() =>
+      expect(screen.getByTestId("errored-server")).toHaveTextContent("A"),
+    );
+    expect(notificationsMock.show).toHaveBeenCalledWith(
+      expect.objectContaining({
+        title: expect.stringContaining("Failed to connect"),
+      }),
+    );
+  });
+
   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 +2537,47 @@ 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).
+  //
+  // The flag is deliberately all this asserts. That arm returns before a client
+  // is rebuilt, so no Network entries are restored and the content-gated column
+  // stays shut — which is right here: the provider's `error` param is the whole
+  // diagnostic and the re-auth banner is already showing it. Asserting a
+  // sidebar would be asserting behavior this arm should not have.
+  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(<App />);
+
+    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(

clients/web/src/App.tsx

@@ -2841,6 +2841,13 @@ function App() {
     if (!params.successful) {
       const pendingId = resumeSnapshot?.serverId;
       if (pendingId) {
+        // Red border only (#1621), not a sidebar. This arm returns before a
+        // client is rebuilt, so the persisted `auth` entries are never
+        // restored and the content-gated column stays shut — correctly: the
+        // provider's own `error` param is the whole diagnostic, and the
+        // re-auth banner below is already showing it. The flag is still right,
+        // because the attempt did fail.
+        setFailedServerId(pendingId);
         queueMicrotask(() => {
           showReAuthBanner(pendingId, generateOAuthErrorDescription(params));
         });
@@ -2876,6 +2883,10 @@ function App() {
         await webOAuthStorage.load();
       } catch (err) {
         connectStartRef.current = undefined;
+        // Red border only, for the same reason as the provider-error arm above
+        // — and here the network log would not help anyway: this is a failure
+        // to read local OAuth storage, not a request that went out.
+        setFailedServerId(server.id);
         queueMicrotask(() => {
           showReAuthBanner(server.id, err instanceof Error ? err : String(err));
         });
@@ -2891,6 +2902,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}"`,
@@ -3048,9 +3066,33 @@ function App() {
         // against the right client. The redirect unloads this page, so there's
         // nothing to do after the await on the success path.
         if (err instanceof AuthRecoveryRequiredError) {
-          if (await client.checkAuthChallengeSatisfied(err.authChallenge)) {
-            connectStartRef.current = Date.now();
-            await client.connect();
+          try {
+            if (await client.checkAuthChallengeSatisfied(err.authChallenge)) {
+              connectStartRef.current = Date.now();
+              await client.connect();
+              return;
+            }
+          } catch (recoveryErr) {
+            // Both awaits above are unguarded connect work sitting inside a
+            // `catch`, so a rejection escapes `onToggleConnection` altogether:
+            // no toast, no red border, no sidebar — the #2108 failure mode in
+            // its most invisible form. Surface it as the failed connect attempt
+            // it is. A throw from `checkAuthChallengeSatisfied` lands here too
+            // rather than falling through to `prepareOAuthRedirect`: it is not
+            // the same as the challenge being *unsatisfied*, and navigating the
+            // whole page away on the strength of an error would bury it.
+            connectStartRef.current = undefined;
+            setFailedServerId(id);
+            const message =
+              recoveryErr instanceof Error
+                ? recoveryErr.message
+                : String(recoveryErr);
+            setConnectErrorMessage(message);
+            notifications.show({
+              title: `Failed to connect to "${target.name}"`,
+              message,
+              color: "red",
+            });
             return;
           }
           prepareOAuthRedirect({
@@ -3091,6 +3133,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);

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(
+        <StatefulInspectorViewHost
+          {...makeProps({
+            servers: [httpServer],
+            activeServer: "beta",
+            connectionStatus: "connecting",
+          })}
+        />,
+      );
+      expect(
+        screen.queryByRole("button", { name: "Close monitoring sidebar" }),
+      ).toBeNull();
+
+      rerender(
+        <StatefulInspectorViewHost
+          {...makeProps({
+            servers: [httpServer],
+            // The failure's teardown clears the active server and settles the
+            // session at "disconnected" — never "error".
+            activeServer: undefined,
+            erroredServerId: "beta",
+            connectionStatus: "disconnected",
+            initializeResult: undefined,
+            network: [
+              {
+                id: "a1",
+                timestamp: new Date(),
+                method: "GET",
+                url: "https://as.example.com/.well-known/oauth-authorization-server",
+                requestHeaders: {},
+                responseStatus: 404,
+                responseStatusText: "Not Found",
+                category: "auth",
+              },
+            ],
+          })}
+        />,
+      );
+
+      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(
+        <StatefulInspectorViewHost
+          {...makeProps({
+            servers: [httpServer],
+            activeServer: undefined,
+            erroredServerId: "beta",
+            connectionStatus: "disconnected",
+            initializeResult: undefined,
+            network: [
+              {
+                id: "a1",
+                timestamp: new Date(),
+                method: "GET",
+                url: "https://as.example.com/.well-known/oauth-authorization-server",
+                requestHeaders: {},
+                responseStatus: 404,
+                responseStatusText: "Not Found",
+                category: "auth",
+              },
+            ],
+          })}
+        />,
+      );
+      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",