Merge branch 'v2/main' into v2/chore/2148-web-tab-smokes

AGENTS.md

@@ -1066,7 +1066,7 @@ The top-level **`src/types/`** is a sibling of both and is not the place for new
 
 Cross-directory imports point **one way, `lib → utils`** (infra depends on pure helpers, never the reverse). Keep it that way: if a `utils/` module needs a type currently exported from a `lib/` module, declare the type in `utils/` and re-export it from `lib/` (as `pendingReauth` owns `OAuthResumeAuthKind` and `oauthResume` re-exports it), rather than importing "up" from `utils` into `lib`.
 
-Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The **deliberate, documented** top-level-file exceptions are `src/App.tsx` — a ~4.3k-line composition root at ~42% branch coverage (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) — and the `src/main.tsx` / `src/index.ts` bootstraps (browser `createRoot` render and the bin `runWeb` re-export, the analog of `clients/cli`'s excluded `src/index.ts`). All three are called out in a comment on the `include` array itself rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs.
+Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The **deliberate, documented** top-level-file exceptions are `src/App.tsx` — a ~3.2k-line composition root at ~42% branch coverage (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) — and the `src/main.tsx` / `src/index.ts` bootstraps (browser `createRoot` render and the bin `runWeb` re-export, the analog of `clients/cli`'s excluded `src/index.ts`). All three are called out in a comment on the `include` array itself rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs.
 
 ## React instructions
 

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

@@ -2,8 +2,6 @@ import { describe, it, expect } from "vitest";
 import { InspectorClient } from "@inspector/core/mcp/index.js";
 import type { ServerEntry } from "@inspector/core/mcp/types.js";
 import { renderWithMantine } from "../test/renderWithMantine";
-import type { PendingReauth } from "../utils/pendingReauth";
-import type { PendingStepUp } from "../utils/stepUp";
 import {
   useSessionRef,
   type SessionRef,
@@ -27,27 +25,10 @@ const entry = (id: string, name = id): ServerEntry => ({
 const client = (): InspectorClient =>
   Object.create(InspectorClient.prototype) as InspectorClient;
 
-const stepUp = (serverId: string): PendingStepUp => ({
-  serverId,
-  challenge: { reason: "unauthorized" },
-  authorizationUrl: new URL("https://as.example/authorize"),
-  source: "tool",
-});
-
-const reauth = (serverId: string): PendingReauth => ({
-  serverId,
-  challenge: { reason: "unauthorized" },
-  authorizationUrl: new URL("https://as.example/authorize"),
-  authKind: "reauth",
-  source: "tool",
-});
-
 const values = (over: Partial<SessionValues> = {}): SessionValues => ({
   activeServerId: undefined,
   servers: [],
   inspectorClient: null,
-  pendingStepUp: null,
-  pendingReauth: null,
   ...over,
 });
 
@@ -84,6 +65,8 @@ describe("useSessionRef", () => {
     expect(snapshot.activeServerId).toBe("a");
     expect(snapshot.servers).toBe(servers);
     expect(snapshot.inspectorClient).toBe(c);
+    // Seeded null and left alone: the two pending-OAuth slots are written by
+    // their owner, `useOAuthRecovery`, not supplied to this hook (#2153).
     expect(snapshot.pendingStepUp).toBeNull();
     expect(snapshot.pendingReauth).toBeNull();
   });
@@ -99,34 +82,39 @@ describe("useSessionRef", () => {
     const h = harness(values());
     const servers = [entry("a"), entry("b")];
     const c = client();
-    const up = stepUp("a");
-    const re = reauth("b");
     h.update(
       values({
         activeServerId: "b",
         servers,
         inspectorClient: c,
-        pendingStepUp: up,
-        pendingReauth: re,
       }),
     );
     expect(h.ref().current).toMatchObject({
       activeServerId: "b",
       servers,
       inspectorClient: c,
-      pendingStepUp: up,
-      pendingReauth: re,
     });
   });
 
+  it("leaves the owner-written pending slots alone across renders", () => {
+    const h = harness(values({ activeServerId: "a", servers: [entry("a")] }));
+    // Stands in for `useOAuthRecovery`'s own mirror effect.
+    h.ref().current.pendingStepUp = {
+      serverId: "a",
+      challenge: { reason: "unauthorized" },
+      authorizationUrl: new URL("https://as.example/authorize"),
+      source: "tool",
+    };
+    h.update(values({ activeServerId: "a", servers: [entry("a")] }));
+    expect(h.ref().current.pendingStepUp?.serverId).toBe("a");
+  });
+
   it("clears a value that becomes null or undefined", () => {
     const h = harness(
       values({
         activeServerId: "a",
         servers: [entry("a")],
         inspectorClient: client(),
-        pendingStepUp: stepUp("a"),
-        pendingReauth: reauth("a"),
       }),
     );
     h.update(values());
@@ -134,8 +122,6 @@ describe("useSessionRef", () => {
       activeServerId: undefined,
       servers: [],
       inspectorClient: null,
-      pendingStepUp: null,
-      pendingReauth: null,
     });
   });
 

clients/web/src/hooks/useSessionRef.ts

@@ -24,12 +24,26 @@ export interface SessionSnapshot {
    * keeps the value the write replaced (#2089).
    */
   inspectorClient: InspectorClient | null;
+  /**
+   * The two OAuth-recovery slots. Unlike the fields above they are **not**
+   * supplied to this hook: they are owned by `useOAuthRecovery`, which mirrors
+   * them in an effect of its own (#2153). Keeping them off `SessionValues`
+   * is what lets that hook both own the state and read the ref — App.tsx
+   * would otherwise have to hold the state purely to hand it back.
+   */
   pendingStepUp: PendingStepUp | null;
   pendingReauth: PendingReauth | null;
 }
 
-/** What the caller supplies each render; `activeServerName` is derived. */
-export type SessionValues = Omit<SessionSnapshot, "activeServerName">;
+/**
+ * What the caller supplies each render. `activeServerName` is derived, and the
+ * two pending-OAuth slots are written by their owner — see
+ * {@link SessionSnapshot}.
+ */
+export type SessionValues = Omit<
+  SessionSnapshot,
+  "activeServerName" | "pendingStepUp" | "pendingReauth"
+>;
 
 /** A stable handle onto the latest {@link SessionSnapshot}. */
 export type SessionRef = RefObject<SessionSnapshot>;
@@ -60,6 +74,8 @@ export type SessionRef = RefObject<SessionSnapshot>;
 export function useSessionRef(values: SessionValues): SessionRef {
   const ref = useRef<SessionSnapshot>({
     activeServerName: undefined,
+    pendingStepUp: null,
+    pendingReauth: null,
     ...values,
   });
   // Deliberately un-gated: this is a passive mirror, so re-assigning the same
@@ -73,8 +89,6 @@ export function useSessionRef(values: SessionValues): SessionRef {
     ref.current.activeServerId = values.activeServerId;
     ref.current.servers = values.servers;
     ref.current.inspectorClient = values.inspectorClient;
-    ref.current.pendingStepUp = values.pendingStepUp;
-    ref.current.pendingReauth = values.pendingReauth;
   });
   return ref;
 }

clients/web/vite.config.ts

@@ -189,7 +189,7 @@ export default defineConfig(({ command }) => {
         reporter: ["text", "html", "json-summary"],
         // Whitelist of gated directories. Deliberate top-level-file omissions
         // (every src *directory* below is gated):
-        //   • `src/App.tsx` — a ~4.3k-line composition root at ~42% branch
+        //   • `src/App.tsx` — a ~3.2k-line composition root at ~42% branch
         //     coverage; gating it is a dedicated testing/decomposition effort,
         //     not a whitelist tweak.
         //   • `src/main.tsx` / `src/index.ts` — the browser and bin bootstraps