From 01cc79c61b2c669021f33cb0dc6f73dc66921b8d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 4 Jul 2026 16:47:48 -0400 Subject: [PATCH 1/4] =?UTF-8?q?test(cli):=20cover=20cliOAuth.ts=20step-up/?= =?UTF-8?q?recovery=20paths=20to=20the=20=E2=89=A590%=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring clients/cli/src/cliOAuth.ts from 75% to ≥90% on all four coverage dimensions (now 100/96.42/100/100) by exercising the previously-untested interactive step-up and auth-recovery paths: - confirmStepUpFromStdin (the default stdin confirmer): drive it via the default `confirmStepUp` parameter of handleCliAuthRecoveryRequired with node:readline/promises mocked — covers the y / "yes" (trimmed, case-insensitive) accept paths and the n decline path (which throws). - connectInspectorWithOAuth's AuthRecoveryRequiredError branch: both the storage-already-satisfies resume and the interactive-recovery sub-branches, plus the isUnauthorizedError branch (with a rejecting disconnect to hit the `.catch(() => {})` guard), the non-OAuth-capable-config rethrow, and the non-OAuth-error rethrow. Part of #1610 — v2/main mid-session-auth code below the per-file coverage gate that v2/main's CI does not run but the Wave-1 rollup (#1550) does. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/cli/__tests__/cliOAuth.test.ts | 227 +++++++++++++++++++++++++ 1 file changed, 227 insertions(+) diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 42178e295..f9d191539 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -3,15 +3,39 @@ import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { MutableRedirectUrlProvider } from "@inspector/core/auth/index.js"; import * as runnerInteractive from "@inspector/core/auth/node/runner-interactive-oauth.js"; import { + connectInspectorWithOAuth, handleCliAuthRecoveryRequired, isStandardOAuthStepUp, runCliInteractiveOAuth, withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; + +// `confirmStepUpFromStdin` (the default step-up confirmer) reads a line from +// stdin via node:readline/promises. Mock the module so the default path can be +// exercised deterministically without real TTY input. +const { mockQuestion, mockClose } = vi.hoisted(() => ({ + mockQuestion: vi.fn(), + mockClose: vi.fn(), +})); +vi.mock("node:readline/promises", () => ({ + createInterface: vi.fn(() => ({ + question: mockQuestion, + close: mockClose, + })), +})); + +const CALLBACK_URL_CONFIG = { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", +}; describe("cliOAuth", () => { afterEach(() => { vi.restoreAllMocks(); + mockQuestion.mockReset(); + mockClose.mockReset(); }); describe("isStandardOAuthStepUp", () => { @@ -227,4 +251,207 @@ describe("cliOAuth", () => { expect(result).toBe("ok"); expect(fn).toHaveBeenCalledTimes(2); }); + + describe("confirmStepUpFromStdin (default stdin confirmer)", () => { + const standardStepUpError = () => + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "insufficient_scope", + requiredScopes: ["weather:read"], + }); + + const clientNeedingStepUp = () => ({ + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }); + + it("proceeds with OAuth when the user answers y (no confirmStepUp arg)", async () => { + mockQuestion.mockResolvedValue("y"); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + + // Omitting the confirmStepUp argument exercises the default + // confirmStepUpFromStdin, which reads from the mocked readline interface. + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ); + + expect(mockQuestion).toHaveBeenCalled(); + expect(mockClose).toHaveBeenCalled(); + expect(runSpy).toHaveBeenCalled(); + }); + + it("accepts a whitespace-padded, upper-case 'YES'", async () => { + mockQuestion.mockResolvedValue(" YES "); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ); + + expect(runSpy).toHaveBeenCalled(); + }); + + it("declines (throws) when the user answers n", async () => { + mockQuestion.mockResolvedValue("n"); + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + + await expect( + handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + ), + ).rejects.toThrow("Step-up authorization declined."); + + expect(mockClose).toHaveBeenCalled(); + expect(runSpy).not.toHaveBeenCalled(); + }); + }); + + describe("connectInspectorWithOAuth recovery branch", () => { + const oauthServerConfig = { + type: "streamable-http", + url: "https://as.example/mcp", + } as MCPServerConfig; + + it("resumes without re-auth when storage already satisfies the challenge", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "insufficient_scope", requiredScopes: ["weather:read"] }, + ), + ) + .mockResolvedValueOnce(undefined); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(true), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(connect).toHaveBeenCalledTimes(2); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("runs interactive recovery when storage does not satisfy the challenge", async () => { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const connect = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "token_expired" }, + ), + ) + .mockResolvedValueOnce(undefined); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(connect).toHaveBeenCalledTimes(2); + expect(runSpy).toHaveBeenCalled(); + }); + + it("runs interactive OAuth on a plain unauthorized error (disconnect failure is swallowed)", async () => { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const connect = vi + .fn() + .mockRejectedValueOnce(new Error("Connection failed for server (401)")) + .mockResolvedValueOnce(undefined); + // A rejecting disconnect exercises the `.catch(() => {})` guard. + const client = { + connect, + disconnect: vi.fn().mockRejectedValue(new Error("disconnect failed")), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + + expect(client.disconnect).toHaveBeenCalled(); + expect(runSpy).toHaveBeenCalled(); + expect(connect).toHaveBeenCalledTimes(2); + }); + + it("rethrows a non-OAuth error unchanged", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValue(new Error("some unrelated failure")); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await expect( + connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ), + ).rejects.toThrow("some unrelated failure"); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("rethrows when the server config is not OAuth-capable", async () => { + const connect = vi.fn().mockRejectedValue(new Error("nope (401)")); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await expect( + connectInspectorWithOAuth( + client, + { type: "stdio", command: "x" } as MCPServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ), + ).rejects.toThrow("nope (401)"); + }); + }); }); From 9a3604d01d453f719f76addba307cc30688a74d8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 4 Jul 2026 16:59:51 -0400 Subject: [PATCH 2/4] test(tui): cover AuthTab step-up up-arrow and a/c shortcuts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring clients/tui/src/components/AuthTab.tsx to ≥90 on all four dimensions (now 97.43/90.81/100/100) by exercising the previously-untested step-up keypress paths in the `useInput` handler: up-arrow navigation back to the "authorize" choice, the `a` (authorize) and `c` (cancel) shortcuts, and the fall-through for an unrelated key while the prompt is pending. Part of #1610. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/tui/__tests__/AuthTab.test.tsx | 60 ++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index fbefcf2e0..d12f16f39 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -309,6 +309,66 @@ describe("AuthTab", () => { expect(lastFrame() ?? "").toContain("OAuth Details"); }); + it("moves the step-up selection back up with the up arrow", async () => { + const onAuthorizeStepUp = vi.fn(); + const onCancelStepUp = vi.fn(); + const { client } = makeClient(sampleOAuthState); + const { stdin } = render( + , + ); + await tick(); + // Move down to "cancel" (index 1), then back up to "authorize" (index 0). + stdin.write(DOWN); + await tick(); + stdin.write(UP); + await tick(); + stdin.write("\r"); + await tick(); + expect(onAuthorizeStepUp).toHaveBeenCalledTimes(1); + expect(onCancelStepUp).not.toHaveBeenCalled(); + }); + + it("authorizes step-up with 'a', cancels with 'c', and ignores other keys", async () => { + const onAuthorizeStepUp = vi.fn(); + const onCancelStepUp = vi.fn(); + const { client } = makeClient(sampleOAuthState); + const { stdin } = render( + , + ); + await tick(); + stdin.write("a"); + await tick(); + expect(onAuthorizeStepUp).toHaveBeenCalledTimes(1); + + stdin.write("c"); + await tick(); + expect(onCancelStepUp).toHaveBeenCalledTimes(1); + + // An unrelated key is swallowed while the step-up prompt is pending. + stdin.write("x"); + await tick(); + expect(onAuthorizeStepUp).toHaveBeenCalledTimes(1); + expect(onCancelStepUp).toHaveBeenCalledTimes(1); + }); + it("refreshes OAuth state when oauthComplete fires", async () => { const { client, getOAuthState, fire, listeners } = makeClient(sampleOAuthState); From f7ab287f6a3edd2568ef92e9df822c1de2d8eeda Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 4 Jul 2026 17:24:10 -0400 Subject: [PATCH 3/4] =?UTF-8?q?test(tui):=20cover=20App.tsx=20mid-session?= =?UTF-8?q?=20auth=20flows=20to=20the=20=E2=89=A590%=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring clients/tui/src/App.tsx from 79% to ≥90 on all four dimensions (92.23 / 90.20 / 92.85 / 92.68) by exercising the mid-session / step-up auth machinery that the existing suite didn't reach: - runOAuthAuthentication result branches (success / already_authorized / insufficient_scope / unsupported), driven deterministically via a new opt-in runRunnerInteractiveOAuth override (defaults to the real runner so the existing callback-driven OAuth tests are unchanged). - The per-client auth-lifecycle listeners (authChallengeAmbient / Recovered / Interactive / oauthError), fired through a new client-event registry on the FakeClient, including the non-selected-server early-return guards. - handleAuthRecoveryRequired step-up-confirm and reauth paths, handleConnect's OAuth-on-401 branches (AuthRecoveryRequired + EmaClientNotConfigured thrown mid-flow), handleClearOAuth's connected/disconnect path, and the AuthTab onAuthorizeStepUp handler (EMA satisfied/interactive/failed/throw + standard non-EMA authorize, cancel). Part of #1610. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/tui/__tests__/App.test.tsx | 383 ++++++++++++++++++++++++++++- 1 file changed, 381 insertions(+), 2 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 7ea98657b..dda5f894a 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -75,6 +75,20 @@ const h = vi.hoisted(() => { start: callbackStart, stop: callbackStop, })); + // Registry of the auth-lifecycle listeners App registers per client, so a test + // can fire authChallengeAmbient / authChallengeRecovered / authChallengeInteractive + // / oauthError against whichever FakeClient instance App built. + const clientEvents = new Map void>>(); + const fireClientEvent = (event: string, detail?: unknown) => { + clientEvents.get(event)?.forEach((fn) => fn({ detail })); + }; + // Optional per-test override for runRunnerInteractiveOAuth. Left null, the real + // runner runs (existing tests drive it via the captured callback opts); set it + // to return a specific { kind } to deterministically exercise the result + // branches without steering the real callback flow. + const runner: { + override: null | ((opts: unknown) => Promise); + } = { override: null }; class FakeManager { destroy = vi.fn(); } @@ -106,8 +120,15 @@ const h = vi.hoisted(() => { readResource = vi.fn(async () => ({ result: { contents: [{ uri: "file://x", text: "hello" }] }, })); - addEventListener = vi.fn(); - removeEventListener = vi.fn(); + addEventListener = vi.fn((event: string, fn: (event: unknown) => void) => { + if (!clientEvents.has(event)) clientEvents.set(event, new Set()); + clientEvents.get(event)!.add(fn); + }); + removeEventListener = vi.fn( + (event: string, fn: (event: unknown) => void) => { + clientEvents.get(event)?.delete(fn); + }, + ); // Reject so the unmount cleanup's `.catch(() => {})` arrow is exercised. disconnect = vi.fn().mockRejectedValue(new Error("cleanup disconnect")); } @@ -121,6 +142,9 @@ const h = vi.hoisted(() => { createOAuthCallbackServer, callbackStart, callbackStop, + clientEvents, + fireClientEvent, + runner, FakeManager, FakeClient, useInspectorClient: vi.fn(() => ({ @@ -200,6 +224,12 @@ vi.mock("@inspector/core/auth/node/index.js", async (importOriginal) => { ...actual, createOAuthCallbackServer: h.createOAuthCallbackServer, NodeOAuthStorage: class {}, + runRunnerInteractiveOAuth: (opts: unknown) => + h.runner.override + ? h.runner.override(opts) + : actual.runRunnerInteractiveOAuth( + opts as Parameters[0], + ), }; }); vi.mock("../src/utils/openUrl.js", () => ({ @@ -212,6 +242,7 @@ import { AuthRecoveryRequiredError, EMA_STEP_UP_PENDING_URL, } from "@inspector/core/auth/challenge.js"; +import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js"; const tick = () => new Promise((r) => setTimeout(r, 25)); const callbackUrlConfig = { hostname: "127.0.0.1", port: 0, pathname: "/cb" }; @@ -272,6 +303,16 @@ function oneEmaHttp(): Record { }; } +// Two OAuth-capable http servers (first auto-selected). Drives the per-server +// auth-event guards (events from the non-selected server return early) and the +// "a step-up is already pending for another server" branch. +function twoHttp(): Record { + return { + web: { config: { type: "streamable-http", url: "http://a" } } as never, + api: { config: { type: "streamable-http", url: "http://b" } } as never, + }; +} + // Mixed catalog: an OAuth-capable http server first (auto-selected) followed by // a stdio server � drives per-server tab gating + the tab-switch-away effects. function httpThenStdio(): Record { @@ -497,6 +538,8 @@ beforeEach(() => { h.cb.opts = null; h.callbackStart.mockClear(); h.callbackStop.mockClear(); + h.clientEvents.clear(); + h.runner.override = null; h.clientSpies.authenticate.mockReset(); h.clientSpies.authenticate.mockResolvedValue("https://auth.example/start"); h.clientSpies.clearOAuthTokens.mockReset(); @@ -1041,3 +1084,339 @@ describe("App (OAuth flows)", () => { await expectFrame(r, "Step-up authorization succeeded"); }); }); + +describe("App (mid-session auth lifecycle events)", () => { + it("shows the ambient-refresh message then clears it on recovery", async () => { + const r = await mount(oneHttp()); + await press(r, ["a"]); // Auth tab so oauthMessage is visible + h.fireClientEvent("authChallengeAmbient"); + await expectFrame(r, "Refreshing authorization"); + h.fireClientEvent("authChallengeRecovered"); + await waitUntil( + () => !(r.lastFrame() ?? "").includes("Refreshing authorization"), + ); + expect(r.lastFrame() ?? "").not.toContain("Refreshing authorization"); + }); + + it("surfaces an oauthError event for both Error and non-Error payloads", async () => { + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("oauthError", { error: new Error("oauth boom") }); + await expectFrame(r, "oauth boom"); + h.fireClientEvent("oauthError", { error: "plain oauth string" }); + await expectFrame(r, "plain oauth string"); + }); + + it("clears OAuth tokens and disconnects when connected", async () => { + h.ctrl.status = "connected"; + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalled(); + expect(h.disconnect).toHaveBeenCalled(); + }); + + const stepUpChallenge = { + reason: "insufficient_scope" as const, + requiredScopes: ["env:read"], + authorizationScopes: ["tools:read", "env:read"], + context: { toolName: "get-env" }, + }; + + it("presents a standard step-up on an interactive auth-challenge event", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: new URL("https://as.example/authorize"), + challenge: stepUpChallenge, + }); + await expectFrame(r, "needs additional OAuth scopes"); + }); + + it("skips step-up when the challenge is already satisfied (interactive event)", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(true); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: new URL("https://as.example/authorize"), + challenge: stepUpChallenge, + }); + await expectFrame(r, "Authorization updated"); + }); + + it("auto-runs OAuth on an interactive reauth event (no step-up confirm)", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: new URL("https://as.example/authorize"), + challenge: { reason: "unauthorized" as const }, + }); + await waitUntil(() => h.callbackStart.mock.calls.length > 0); + expect(h.callbackStart).toHaveBeenCalled(); + }); + + it("routes a connect AuthRecoveryRequiredError into recovery", async () => { + h.connect + .mockRejectedValueOnce( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "unauthorized", + }), + ) + .mockResolvedValue(undefined); + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + const r = await mount(oneHttp()); + await press(r, ["c"]); + await waitUntil(() => h.callbackStart.mock.calls.length > 0); + expect(h.callbackStart).toHaveBeenCalled(); + }); + + it("surfaces an EMA-client-not-configured error on connect", async () => { + h.connect.mockRejectedValue( + new EmaClientNotConfiguredError("not_configured"), + ); + const r = await mount(oneEmaHttp()); + await press(r, ["a", "c"]); + await waitUntil(() => (r.lastFrame() ?? "").length > 0); + await expectFrame(r, "enterprise"); + }); +}); + +describe("App (step-up authorize outcomes)", () => { + const challenge = { + reason: "insufficient_scope" as const, + requiredScopes: ["env:read"], + authorizationScopes: ["tools:read", "env:read"], + context: { toolName: "get-env" }, + }; + + async function presentEmaStepUp() { + h.clientSpies.callTool.mockRejectedValue( + new AuthRecoveryRequiredError(EMA_STEP_UP_PENDING_URL, challenge, { + emaStepUpConfirm: true, + }), + ); + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.ctrl.status = "connected"; + h.ctrl.tools = [sampleTool]; + const r = await mount(oneEmaHttp()); + await press(r, ["t", TAB, ENTER]); + await expectFrame(r, "MOCK_FORM"); + await press(r, [ENTER]); + await waitUntil(() => h.clientSpies.callTool.mock.calls.length > 0); + await expectFrame(r, "organization before it can continue"); + return r; + } + + it("reports a failed EMA step-up outcome", async () => { + const r = await presentEmaStepUp(); + h.clientSpies.handleAuthChallenge.mockResolvedValue({ + kind: "failed", + error: new Error("mint failed"), + }); + await press(r, ["a"]); + await expectFrame(r, "mint failed"); + }); + + it("runs interactive OAuth when EMA step-up returns interactive", async () => { + const r = await presentEmaStepUp(); + h.clientSpies.handleAuthChallenge.mockResolvedValue({ + kind: "interactive", + challenge, + authorizationUrl: new URL("https://as.example/authorize"), + }); + await press(r, ["a"]); + await waitUntil(() => h.callbackStart.mock.calls.length > 0); + expect(h.callbackStart).toHaveBeenCalled(); + }); + + it("surfaces an error when EMA handleAuthChallenge throws", async () => { + const r = await presentEmaStepUp(); + h.clientSpies.handleAuthChallenge.mockRejectedValue( + new Error("challenge boom"), + ); + await press(r, ["a"]); + await expectFrame(r, "challenge boom"); + }); + + it("cancels a pending step-up with 'c'", async () => { + const r = await presentEmaStepUp(); + await press(r, ["c"]); + await expectFrame(r, "Authorization cancelled"); + }); + + it("completes an EMA interactive step-up when OAuth succeeds", async () => { + const r = await presentEmaStepUp(); + h.clientSpies.handleAuthChallenge.mockResolvedValue({ + kind: "interactive", + challenge, + authorizationUrl: new URL("https://as.example/authorize"), + }); + h.runner.override = async () => ({ kind: "success" }); + await press(r, ["a"]); + await expectFrame(r, "Step-up authorization succeeded"); + }); + + it("completes an EMA interactive step-up when OAuth returns already_authorized", async () => { + const r = await presentEmaStepUp(); + h.clientSpies.handleAuthChallenge.mockResolvedValue({ + kind: "interactive", + challenge, + authorizationUrl: new URL("https://as.example/authorize"), + }); + h.runner.override = async () => ({ kind: "already_authorized" }); + await press(r, ["a"]); + await expectFrame(r, "Step-up authorization succeeded"); + }); +}); + +describe("App (OAuth result branches)", () => { + const unauthorized = Object.assign(new Error("request failed (401)"), { + status: 401, + }); + const stepUpChallenge = { + reason: "insufficient_scope" as const, + requiredScopes: ["env:read"], + authorizationScopes: ["tools:read", "env:read"], + context: { toolName: "get-env" }, + }; + const authUrl = () => new URL("https://as.example/authorize"); + + it("re-connects after OAuth returns already_authorized on a 401", async () => { + h.connect.mockRejectedValueOnce(unauthorized).mockResolvedValue(undefined); + h.runner.override = async () => ({ kind: "already_authorized" }); + const r = await mount(oneHttp()); + await press(r, ["c"]); + await waitUntil(() => h.connect.mock.calls.length >= 2); + expect(h.connect).toHaveBeenCalledTimes(2); + }); + + it("handles an unsupported OAuth result on a 401 without reconnecting", async () => { + h.connect.mockRejectedValueOnce(unauthorized).mockResolvedValue(undefined); + h.runner.override = async () => ({ kind: "failed" }); + const r = await mount(oneHttp()); + await press(r, ["c"]); + await tick(); + await tick(); + expect(h.connect).toHaveBeenCalledTimes(1); + }); + + it("routes an AuthRecoveryRequiredError thrown during 401 OAuth to recovery", async () => { + h.connect.mockRejectedValueOnce(unauthorized).mockResolvedValue(undefined); + h.runner.override = async () => { + throw new AuthRecoveryRequiredError(authUrl(), { + reason: "unauthorized", + }); + }; + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(true); + const r = await mount(oneHttp()); + await press(r, ["c"]); + await waitUntil(() => h.disconnect.mock.calls.length > 0); + await press(r, ["a"]); // view the Auth tab where oauthMessage renders + await expectFrame(r, "Authorization updated"); + }); + + it("surfaces an EMA-not-configured error thrown during 401 OAuth", async () => { + h.connect.mockRejectedValueOnce(unauthorized).mockResolvedValue(undefined); + h.runner.override = async () => { + throw new EmaClientNotConfiguredError("disabled"); + }; + const r = await mount(oneHttp()); + await press(r, ["a", "c"]); + await expectFrame(r, "enterprise"); + }); + + it("completes a standard step-up authorize when OAuth succeeds", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.runner.override = async () => ({ kind: "success" }); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: stepUpChallenge, + }); + await expectFrame(r, "needs additional OAuth scopes"); + await press(r, ["a"]); + await expectFrame(r, "Step-up authorization succeeded"); + }); + + it("completes a standard step-up authorize when OAuth returns already_authorized", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.runner.override = async () => ({ kind: "already_authorized" }); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: stepUpChallenge, + }); + await expectFrame(r, "needs additional OAuth scopes"); + await press(r, ["a"]); + await expectFrame(r, "Step-up authorization succeeded"); + }); + + it("shows insufficient-scope message when step-up OAuth stays insufficient", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.runner.override = async () => ({ + kind: "insufficient_scope", + challenge: stepUpChallenge, + }); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: stepUpChallenge, + }); + await expectFrame(r, "needs additional OAuth scopes"); + await press(r, ["a"]); + await expectFrame(r, "were not granted"); + }); + + it("skips reauth when already satisfied (interactive event)", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(true); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: { reason: "unauthorized" }, + }); + await expectFrame(r, "Authorization updated"); + }); + + it("completes reauth via OAuth on an interactive event", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.runner.override = async () => ({ kind: "success" }); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: { reason: "unauthorized" }, + }); + await expectFrame(r, "Authorization updated. Retry your action"); + }); + + it("completes reauth when OAuth returns already_authorized", async () => { + h.clientSpies.checkAuthChallengeSatisfied.mockResolvedValue(false); + h.runner.override = async () => ({ kind: "already_authorized" }); + const r = await mount(oneHttp()); + await press(r, ["a"]); + h.fireClientEvent("authChallengeInteractive", { + authorizationUrl: authUrl(), + challenge: { reason: "unauthorized" }, + }); + await expectFrame(r, "Authorization updated. Retry your action"); + }); + + it("ignores auth lifecycle events from a non-selected server", async () => { + const r = await mount(twoHttp()); // web is selected; api is not + await press(r, ["a"]); + // Fired for every registered client; the non-selected `api` handlers hit + // their early-return guards while the selected `web` handlers proceed. + h.fireClientEvent("authChallengeAmbient"); + h.fireClientEvent("authChallengeRecovered"); + h.fireClientEvent("oauthError", { + error: new Error("selected-only error"), + }); + await expectFrame(r, "selected-only error"); + }); +}); From 1f7ef9184c3f50f8628b9ba7afa92d89d8b2d94a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 4 Jul 2026 19:08:44 -0400 Subject: [PATCH 4/4] test(tui): truly assert the non-selected-server auth guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the @claude review of #1611: the event registry is now keyed by (client, fn) rather than event name alone, and a new `fireClientEventFor(client, …)` helper fires a single client's listeners. The "ignores auth lifecycle events from a non-selected server" test now fires ambient/recovered/oauthError for ONLY the non-selected client and asserts no message appears — so removing the `selectedServerRef.current !== serverName` guard would fail the test — then fires for the selected client to confirm it does act. Coverage of App.tsx is unchanged (92.23 / 90.20 / 92.85 / 92.68); existing fire-all `fireClientEvent` behavior is preserved. Part of #1610. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/tui/__tests__/App.test.tsx | 64 ++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 13 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index dda5f894a..b8efdb7c5 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -78,9 +78,25 @@ const h = vi.hoisted(() => { // Registry of the auth-lifecycle listeners App registers per client, so a test // can fire authChallengeAmbient / authChallengeRecovered / authChallengeInteractive // / oauthError against whichever FakeClient instance App built. - const clientEvents = new Map void>>(); + // Each entry records which FakeClient registered the handler so a test can + // fire an event for a single client (`fireClientEventFor`) — needed to truly + // assert the per-server `selectedServerRef.current !== serverName` guards, + // not merely execute them. `fireClientEvent` still fires every client's + // handler for the event (the common single-server case). + type EventEntry = { client: unknown; fn: (event: unknown) => void }; + const clientEvents = new Map>(); + const clientInstances: Array<{ cfg?: { type?: string; url?: string } }> = []; const fireClientEvent = (event: string, detail?: unknown) => { - clientEvents.get(event)?.forEach((fn) => fn({ detail })); + clientEvents.get(event)?.forEach((e) => e.fn({ detail })); + }; + const fireClientEventFor = ( + client: unknown, + event: string, + detail?: unknown, + ) => { + clientEvents.get(event)?.forEach((e) => { + if (e.client === client) e.fn({ detail }); + }); }; // Optional per-test override for runRunnerInteractiveOAuth. Left null, the real // runner runs (existing tests drive it via the captured callback opts); set it @@ -93,9 +109,10 @@ const h = vi.hoisted(() => { destroy = vi.fn(); } class FakeClient { - cfg: { type?: string } | undefined; - constructor(config?: { type?: string }) { + cfg: { type?: string; url?: string } | undefined; + constructor(config?: { type?: string; url?: string }) { this.cfg = config; + clientInstances.push(this); } // Derive the transport type from the server config the client was built // with (config.type aligns with the serverType union) so per-server gating @@ -122,11 +139,18 @@ const h = vi.hoisted(() => { })); addEventListener = vi.fn((event: string, fn: (event: unknown) => void) => { if (!clientEvents.has(event)) clientEvents.set(event, new Set()); - clientEvents.get(event)!.add(fn); + clientEvents.get(event)!.add({ client: this, fn }); }); removeEventListener = vi.fn( (event: string, fn: (event: unknown) => void) => { - clientEvents.get(event)?.delete(fn); + const set = clientEvents.get(event); + if (!set) return; + for (const entry of set) { + if (entry.fn === fn) { + set.delete(entry); + break; + } + } }, ); // Reject so the unmount cleanup's `.catch(() => {})` arrow is exercised. @@ -143,7 +167,9 @@ const h = vi.hoisted(() => { callbackStart, callbackStop, clientEvents, + clientInstances, fireClientEvent, + fireClientEventFor, runner, FakeManager, FakeClient, @@ -539,6 +565,7 @@ beforeEach(() => { h.callbackStart.mockClear(); h.callbackStop.mockClear(); h.clientEvents.clear(); + h.clientInstances.length = 0; h.runner.override = null; h.clientSpies.authenticate.mockReset(); h.clientSpies.authenticate.mockResolvedValue("https://auth.example/start"); @@ -1410,13 +1437,24 @@ describe("App (OAuth result branches)", () => { it("ignores auth lifecycle events from a non-selected server", async () => { const r = await mount(twoHttp()); // web is selected; api is not await press(r, ["a"]); - // Fired for every registered client; the non-selected `api` handlers hit - // their early-return guards while the selected `web` handlers proceed. - h.fireClientEvent("authChallengeAmbient"); - h.fireClientEvent("authChallengeRecovered"); - h.fireClientEvent("oauthError", { - error: new Error("selected-only error"), + const api = h.clientInstances.find((c) => c.cfg?.url === "http://b"); + const web = h.clientInstances.find((c) => c.cfg?.url === "http://a"); + // Firing ONLY the non-selected server's handlers must hit the + // `selectedServerRef.current !== serverName` guard on each listener and set + // no message — if any guard were removed, a negative assertion would fail. + h.fireClientEventFor(api, "authChallengeAmbient"); + h.fireClientEventFor(api, "authChallengeRecovered"); + h.fireClientEventFor(api, "oauthError", { + error: new Error("api-only error"), }); - await expectFrame(r, "selected-only error"); + await tick(); + expect(r.lastFrame() ?? "").not.toContain("Refreshing authorization"); + expect(r.lastFrame() ?? "").not.toContain("api-only error"); + // The selected server's handlers do act (guard false): ambient refreshes, + // then an error surfaces. + h.fireClientEventFor(web, "authChallengeAmbient"); + await expectFrame(r, "Refreshing authorization"); + h.fireClientEventFor(web, "oauthError", { error: new Error("web error") }); + await expectFrame(r, "web error"); }); });