diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f2b75c507..d3fe46aec 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -13,7 +13,7 @@ jobs: - name: Setup Node.js uses: actions/setup-node@v6 with: - node-version: '20.x' + node-version: '22.x' cache: 'npm' - name: Install dependencies (root + all clients) @@ -25,18 +25,27 @@ jobs: - name: Validate (format, lint, build, fast tests) # Each client self-validates: format:check + lint + build + test (no # coverage instrumentation — fast). This also builds every client bundle - # (web dist, cli/tui/launcher) that the smokes below need. The per-file - # coverage gate (`npm run coverage`) is intentionally NOT run in CI — run - # it locally before pushing when you want the gate (#1484). + # (web dist, cli/tui/launcher) that the smokes below need. The heavier + # per-file coverage gate runs in its own step below. Unit tests run in + # both steps (fast here, instrumented under coverage); the double unit + # run is an accepted trade-off for keeping this fast build step intact. + # A future optimization could split coverage into per-client parallel + # jobs, but that's a larger restructure and deliberately out of scope. run: npm run validate - - name: Run web integration tests - # `validate` runs the web UNIT project only (fast); the integration - # project (real stdio/HTTP servers, e2e OAuth, filesystem storage) is - # otherwise only reached via test:coverage. Run it here without coverage - # so CI still exercises those paths even though the gate is local-only. - working-directory: ./clients/web - run: npm run test:integration + - name: Enforce per-file coverage gate (≥90% on all four dimensions) + # CI-ENFORCED coverage gate (#1550): runs `npm run coverage`, which + # chains every client's `test:coverage` (v8-instrumented) and fails the + # job if ANY file drops below 90% on lines, statements, functions, or + # branches. This is the whole point of the step — a PR that regresses + # coverage below the threshold now blocks merge instead of relying on a + # contributor remembering to run the gate locally. + # + # This also covers the web integration project: web's `test:coverage` + # runs `--project=unit --project=integration --coverage`, so the former + # standalone `Run web integration tests` step was removed to avoid + # running the integration suite twice. + run: npm run coverage - name: Run cross-client smokes # End-to-end smokes through the built launcher (--help dispatch + prod diff --git a/AGENTS.md b/AGENTS.md index 12500c434..a69546a9b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -176,12 +176,12 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - Run CLI tests with `npm run test` from `clients/cli/` (builds test-servers + CLI bin first via `pretest`) - Run TUI tests with `npm run test` from `clients/tui/` - The repo root has no aggregate `test` script — each client self-validates, so run `npm run validate` from the root (all clients, fast) or `cd clients/ && npm run validate` (one client). Each client still exposes its own `test` / `test:coverage` for quick iteration. -- **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want the gate. **CI does NOT run `coverage`** — the gate is local-only; CI runs `validate` (fast) plus the web integration suite (`clients/web` `test:integration`, no coverage) so the integration paths are still exercised. +- **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want to reproduce the gate locally before pushing. **CI runs `coverage`** on every push (#1550): the per-file ≥90 gate is CI-enforced, so a PR that drops any file below 90 on lines/statements/functions/branches fails the job. CI runs `validate` (fast) for format/lint/build/unit tests, then `coverage` for the instrumented gate. Because web's `test:coverage` already runs the integration project, CI has no separate `test:integration` step — the integration paths are exercised inside the coverage gate. - Each client's `test:coverage` enforces a **uniform per-file gate of ≥ 90 on all four dimensions** — lines, statements, functions, and branches — across `clients/web`, `clients/cli`, `clients/tui`, and `clients/launcher` (CI enforces this gate). This is the result of a codebase-wide audit: the branch floor was first lifted 50 → 70 for web (#1271), then the whole gate raised to 90 with real tests added for every outlier. Genuinely-unreachable branches are **not** waved through by lowering the gate — they are annotated at the source with a justified `/* v8 ignore … -- */` comment. Acceptable reasons are happy-dom-inherent paths (Mantine portal mount points, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards (e.g. a `?? fallback` for a value the types guarantee non-null, or a `Select.onChange` receiving a value outside the allowed list). New code must clear 90 on every dimension; reach for a justified `v8 ignore` only when a branch is genuinely impossible to exercise. - The **same per-file gate** is enforced for the CLI and TUI (#1484), not just web: - **CLI** (`clients/cli`): tests run **in-process** by importing `runCli()` (see `__tests__/helpers/cli-runner.ts`) so `clients/cli/src` is measured under v8 instrumentation. A thin out-of-process layer (`__tests__/e2e.test.ts` + `scripts/smoke-cli.mjs`) still spawns the built binary for the shebang/`process.exit` paths; `src/index.ts` (binary bootstrap) is the only coverage exclusion. `commander` uses `.exitOverride()` so a parse error throws instead of tearing down the test worker. - **TUI** (`clients/tui`): the gate covers the **non-React logic** only — `logger.ts`, `components/tabsConfig.ts`, and `utils/*` (server resolution lives in `core/` and is measured by the web suite). The Ink components, `App.tsx`, and `hooks/` are an **interim exclusion** in `clients/tui/vitest.config.ts` pending the renderer-based follow-up (#1501). When adding new **non-React** logic under `clients/tui/src`, it falls under the gate automatically — add tests for it. -- Run `npm run test:integration` (also from `clients/web/`) for the InspectorClient + transport + auth integration suite. It runs under a separate `integration` vitest project in node env (no happy-dom) with 30s timeouts. The script builds `test-servers/` first via `tsc -p ../../test-servers --noCheck` so the stdio MCP test server can be spawned as a real subprocess. CI runs it as its own step after unit tests. +- Run `npm run test:integration` (also from `clients/web/`) for the InspectorClient + transport + auth integration suite. It runs under a separate `integration` vitest project in node env (no happy-dom) with 30s timeouts. The script builds `test-servers/` first via `tsc -p ../../test-servers --noCheck` so the stdio MCP test server can be spawned as a real subprocess. CI does not run `test:integration` as its own step — the integration project is covered by the CI `coverage` gate, whose web `test:coverage` runs `--project=unit --project=integration --coverage`. - Test files live alongside the source as `.test.tsx` (or `.test.ts` for non-React modules). Integration tests live under `clients/web/src/test/integration/`, mirroring the `core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`, `auth/node/`, `storage/`). Any test file under that folder is automatically picked up by the `integration` vitest project (node env, 30s timeouts) via the folder glob in `vite.config.ts` — placement is the manifest, there is no enumeration to keep in sync. Tests outside the folder run in the `unit` project (happy-dom). When adding a new test for, e.g., `core/mcp/remote/foo.ts`, put it at `src/test/integration/mcp/remote/foo.test.ts`. - Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme @@ -196,8 +196,8 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **`npm run ci` before pushing** — runs the same steps as `.github/workflows/main.yml` (minus `npm install`): `validate` → web `test:integration` → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). Use this when you want CI parity; expect several minutes. **`npm run validate`** remains the fast loop during development (unit tests only — no web integration, smoke, or Storybook). - ALWAYS do `npm run format` before committing, then **`npm run ci`** (or at minimum `npm run validate`) before pushing. From the repo root, `validate` chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each delegates to that client's own `npm run validate` = `format:check` + `lint` + `build` + `test` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && build && test`. - - Optionally also run **`npm run coverage`** when you want the local-only per-file ≥90 gate — CI does **not** run `coverage`; it relies on `validate` + web `test:integration` instead. -- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). + - Before pushing, also run **`npm run coverage`** — `validate` is fast and does NOT enforce the per-file gate (or, for web, run the integration project); `coverage` does both. **CI runs `coverage`** too (#1550), so the gate blocks PRs that regress below 90 — running it locally first just catches failures before CI does. CI has no separate `test:integration` step: web's `test:coverage` already runs the integration project. +- **`smoke` is a separate top-level target, NOT part of `validate`.** Run it (or the individual `smoke:*`) after a build/validate: `npm run validate && npm run smoke`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke`. Storybook is the only CI step left out (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. - `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. diff --git a/clients/cli/README.md b/clients/cli/README.md index 9f9b4db67..98eed8f60 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -80,6 +80,12 @@ npx @modelcontextprotocol/inspector --cli https://my-mcp-server.example.com --tr When a server is loaded from a `--catalog`/`--config` file, its per-server settings (headers, connection/request timeouts, and OAuth) are applied to the connection — the same resolution the TUI uses. A `--header` flag overrides the file's headers for that run while leaving the file's timeouts and OAuth in place. +### HTTP proxy support + +Connections to remote HTTP/SSE servers honor the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. This applies to the Node transport shared by the CLI and the web backend — no inspector-specific flag is needed. When a proxy variable is set, outbound requests are routed through undici's `EnvHttpProxyAgent`. + +Proxy routing is powered by the [`undici`](https://www.npmjs.com/package/undici) package (`^8.5.0`, which requires Node `>= 22.19.0` — the inspector's supported floor). It is imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. + ## Options ### MCP server (which server to connect to) @@ -131,13 +137,13 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha #### Flags -| Option | Env | Description | -| ------ | --- | ----------- | -| `--client-config ` | `MCP_CLIENT_CONFIG_PATH` | Install-level client config (default: `~/.mcp-inspector/storage/client.json`). | -| `--client-id ` | — | OAuth client ID (static client); overrides `client.json`. | -| `--client-secret ` | — | OAuth client secret; overrides `client.json`. | -| `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | -| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | +| Option | Env | Description | +| ----------------------------- | ------------------------ | ------------------------------------------------------------------------------------------------ | +| `--client-config ` | `MCP_CLIENT_CONFIG_PATH` | Install-level client config (default: `~/.mcp-inspector/storage/client.json`). | +| `--client-id ` | — | OAuth client ID (static client); overrides `client.json`. | +| `--client-secret ` | — | OAuth client secret; overrides `client.json`. | +| `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | +| `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: @@ -147,7 +153,33 @@ npx @modelcontextprotocol/inspector --cli --catalog mcp.json --server my-http-se --method tools/list ``` -See [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAuth smoke testing](../../specification/v2_auth_smoke_testing.md) (§3 Stytch/CIMD; [§5 mid-session manual validation](v2_auth_smoke_testing.md#5-mid-session-auth--step-up--manual-validation) — CLI **C1–C2**). +See [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAuth smoke testing](../../specification/v2_auth_smoke_testing.md) (§3 Stytch/CIMD; [§5 mid-session manual validation](../../specification/v2_auth_smoke_testing.md#5-mid-session-auth--step-up--manual-validation) — CLI **C1–C2**). + +## Exit codes & error envelopes + +Every non-zero exit maps to a stable failure class, so a programmatic caller +(CI, a script, an agent) can branch on _why_ the CLI failed without scraping +prose from stderr: + +| Code | Meaning | +| ---- | ------- | +| `0` | Success. | +| `1` | Usage / unexpected error (the catch-all). | +| `2` | No MCP App found on the tool (`--app-info` probe). | +| `3` | Server requires authentication (401/403, `WWW-Authenticate`, OAuth). | +| `4` | Server unreachable (DNS, connection refused, timeout, `fetch failed`). | +| `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). | + +On any non-zero exit the CLI also writes a single JSON line to **stderr** — the +`ErrorEnvelope`: + +```json +{ "error": { "code": "auth_required", "message": "Unauthorized", "status": 401, "url": "https://api.example/mcp" } } +``` + +The `code` is a stable identifier for the failure class; `message` is the +human-readable error; `cause`, `status`, and `url` are included when known. +Because it is one line, a caller can parse it with `2>&1 | tail -1 | jq .error`. ## Why use the CLI? diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 79d0eab8d..fa7bac1dd 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, afterEach } from "vitest"; -import { handleError } from "../src/error-handler.js"; +import { + CliExitCodeError, + EXIT_CODES, + classifyError, + formatErrorOutput, + handleError, +} from "../src/error-handler.js"; /** * `handleError` is the binary's last-resort error sink (wired up in @@ -13,33 +19,217 @@ describe("handleError", () => { vi.restoreAllMocks(); }); - it("logs an Error's message and exits with code 1", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + it("emits a JSON error envelope on stderr and exits with the classified code", () => { + const writeSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation((() => true) as never); const exitSpy = vi .spyOn(process, "exit") .mockImplementation((() => undefined) as never); handleError(new Error("boom")); - expect(errorSpy).toHaveBeenCalledWith("boom"); - expect(exitSpy).toHaveBeenCalledWith(1); + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE); + const written = writeSpy.mock.calls[0]![0] as string; + const parsed = JSON.parse(written) as { + error: { code: string; message: string }; + }; + expect(parsed.error.code).toBe("error"); + expect(parsed.error.message).toBe("boom"); }); - it("logs a string error verbatim", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); + it("uses a CliExitCodeError's exitCode and envelope code", () => { + vi.spyOn(process.stderr, "write").mockImplementation((() => true) as never); + const exitSpy = vi + .spyOn(process, "exit") + .mockImplementation((() => undefined) as never); - handleError("plain failure"); + handleError( + new CliExitCodeError(EXIT_CODES.NO_APP, "no app", { code: "no_app" }), + ); + + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.NO_APP); + }); +}); - expect(errorSpy).toHaveBeenCalledWith("plain failure"); +describe("classifyError", () => { + it("classifies a 401 status as AUTH_REQUIRED", () => { + const err = Object.assign(new Error("Unauthorized"), { status: 401 }); + const { exitCode, envelope } = classifyError(err, { + url: "https://x.example/mcp", + }); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.code).toBe("auth_required"); + expect(envelope.status).toBe(401); + expect(envelope.url).toBe("https://x.example/mcp"); }); - it("falls back to 'Unknown error' for non-Error, non-string values", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); + it("classifies a WWW-Authenticate message as AUTH_REQUIRED without a status", () => { + const { exitCode } = classifyError( + new Error("Dynamic client registration failed: WWW-Authenticate Bearer"), + ); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + }); + + it("classifies ENOTFOUND / fetch failed as UNREACHABLE", () => { + const err = new Error("fetch failed"); + (err as { cause?: unknown }).cause = new Error( + "getaddrinfo ENOTFOUND no.such.host", + ); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); + expect(envelope.cause).toContain("ENOTFOUND"); + }); + + it("classifies a connection-refused cause as UNREACHABLE", () => { + const err = new Error("connect ECONNREFUSED 127.0.0.1:1"); + expect(classifyError(err).exitCode).toBe(EXIT_CODES.UNREACHABLE); + }); + + it("falls back to USAGE for an unrecognized Error", () => { + expect(classifyError(new Error("something else")).exitCode).toBe( + EXIT_CODES.USAGE, + ); + }); + + it("handles a string error", () => { + const { exitCode, envelope } = classifyError("plain failure"); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.message).toBe("plain failure"); + }); + + it("handles a non-Error, non-string value", () => { + const { envelope } = classifyError({ unexpected: true }); + expect(envelope.message).toBe("Unknown error"); + }); + + it("reads an HTTP status from a nested response object", () => { + const err = { response: { status: 401 } }; + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.status).toBe(401); + }); + + it("preserves a CliExitCodeError's explicit envelope code over the default", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.TOOL_ERROR, "x", { + code: "tool_not_found", + }), + ); + expect(envelope.code).toBe("tool_not_found"); + }); - handleError({ unexpected: true }); + it("carries a CliExitCodeError's explicit status and url onto the envelope", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, "denied", { + status: 401, + url: "https://api.example/mcp", + }), + ); + expect(envelope.status).toBe(401); + expect(envelope.url).toBe("https://api.example/mcp"); + }); + + it("falls back to the context url for a CliExitCodeError without its own url", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.UNREACHABLE, "down"), + { url: "https://ctx.example/mcp" }, + ); + expect(envelope.url).toBe("https://ctx.example/mcp"); + }); + + it("derives a default envelope code for a bare CliExitCodeError", () => { + expect( + classifyError(new CliExitCodeError(EXIT_CODES.UNREACHABLE, "x")).envelope + .code, + ).toBe("unreachable"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.NO_APP, "x")).envelope.code, + ).toBe("no_app"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, "x")) + .envelope.code, + ).toBe("auth_required"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.TOOL_ERROR, "x")).envelope + .code, + ).toBe("tool_error"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.USAGE, "x")).envelope.code, + ).toBe("error"); + }); + + it("terminates on a self-referential cause chain without recursing infinitely", () => { + // A cyclic `error.cause` must not hang the classifier; the depth cap bounds + // the walk and returns a finite string. + const err = new Error("looping"); + (err as { cause?: unknown }).cause = err; + const { envelope } = classifyError(err); + expect(envelope.message).toBe("looping"); + // Bounded by the depth cap: a finite string, not an infinite loop. + expect(typeof envelope.cause).toBe("string"); + expect(envelope.cause).toContain("looping"); + }); + + it("captures a non-Error cause as a string", () => { + const err = new Error("outer"); + (err as { cause?: unknown }).cause = { reason: "blocked" }; + const { envelope } = classifyError(err); + expect(envelope.cause).toBe("[object Object]"); + }); + + it("flattens a nested Error cause chain into one string", () => { + const inner = new Error("getaddrinfo ENOTFOUND host"); + const middle = Object.assign(new Error("connect failed"), { cause: inner }); + const outer = Object.assign(new Error("fetch failed"), { cause: middle }); + const { envelope } = classifyError(outer); + expect(envelope.cause).toBe("connect failed: getaddrinfo ENOTFOUND host"); + }); + + it("reads a numeric SDK-style .code as an HTTP status", () => { + const err = Object.assign(new Error("forbidden"), { code: 403 }); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.status).toBe(403); + }); + + it("ignores a JSON-RPC McpError numeric .code (outside the HTTP range) as a status", () => { + // MCP SDK's McpError carries a numeric `.code` that is a JSON-RPC error + // code (e.g. -32601 MethodNotFound), NOT an HTTP status. It must not leak + // into `status`, nor get misclassified as AUTH_REQUIRED. + const err = Object.assign(new Error("Method not found"), { code: -32601 }); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.status).toBeUndefined(); + }); + + it("ignores a non-numeric node error code when reading the status", () => { + const err = Object.assign(new Error("getaddrinfo ENOTFOUND host"), { + code: "ENOTFOUND", + }); + const { exitCode, envelope } = classifyError(err); + // The message matches the UNREACHABLE pattern, and the string node code + // must NOT be treated as an HTTP status. + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); + expect(envelope.status).toBeUndefined(); + }); +}); + +describe("formatErrorOutput", () => { + it("emits one JSON line on stderr ending in a newline", () => { + const { stderr, exitCode } = formatErrorOutput(new Error("nope")); + expect(stderr.endsWith("\n")).toBe(true); + expect(stderr.split("\n").length).toBe(2); + const parsed = JSON.parse(stderr) as { error: { message: string } }; + expect(parsed.error.message).toBe("nope"); + expect(exitCode).toBe(EXIT_CODES.USAGE); + }); - expect(errorSpy).toHaveBeenCalledWith("Unknown error"); + it("threads the context url into the envelope", () => { + const { stderr } = formatErrorOutput(new Error("nope"), { + url: "https://ctx.example/mcp", + }); + const parsed = JSON.parse(stderr) as { error: { url?: string } }; + expect(parsed.error.url).toBe("https://ctx.example/mcp"); }); }); diff --git a/clients/cli/__tests__/helpers/cli-runner.ts b/clients/cli/__tests__/helpers/cli-runner.ts index 38b401d14..d5c8abe66 100644 --- a/clients/cli/__tests__/helpers/cli-runner.ts +++ b/clients/cli/__tests__/helpers/cli-runner.ts @@ -1,4 +1,5 @@ import { runCli as invokeCli } from "../../src/cli.js"; +import { formatErrorOutput } from "../../src/error-handler.js"; export interface CliResult { exitCode: number | null; @@ -52,7 +53,8 @@ function captureWrite(append: (text: string) => void) { * (`exitCode` / `stdout` / `stderr` / `output`) so every existing test and the * shared assertion helpers keep working unchanged: * - stdout/stderr are captured by temporarily patching `process.std*.write` - * (and `console.error`/`console.warn`, which commander and error paths use). + * (which also captures `console.error`/`console.warn`, since those route + * through `process.stderr.write`). * - A thrown error (bad args, failed connect, unsupported method) maps to * `exitCode: 1` with the message appended to stderr — mirroring how the real * binary (`index.ts`) routes errors through `handleError` → `process.exit(1)`. @@ -77,8 +79,6 @@ export async function runCli( const originalStdoutWrite = process.stdout.write; const originalStderrWrite = process.stderr.write; - const originalConsoleError = console.error; - const originalConsoleWarn = console.warn; // Snapshot and apply env overrides; `undefined` records keys that did not // exist so they can be deleted (not set to the string "undefined") on restore. @@ -93,15 +93,12 @@ export async function runCli( process.stdout.write = captureWrite((text) => { stdout += text; }) as typeof process.stdout.write; + // `console.error` / `console.warn` route through `process.stderr.write`, so + // patching the underlying stream captures them too — no separate console + // override is needed (the previous explicit overrides were redundant). process.stderr.write = captureWrite((text) => { stderr += text; }) as typeof process.stderr.write; - console.error = (...parts: unknown[]) => { - stderr += parts.map(String).join(" ") + "\n"; - }; - console.warn = (...parts: unknown[]) => { - stderr += parts.map(String).join(" ") + "\n"; - }; // `runCli` reads `process.argv.slice(2)`, so prepend two placeholder entries // ([node, script]) to mirror how the binary is launched. @@ -127,14 +124,15 @@ export async function runCli( try { await Promise.race([invokeCli(argv), timeout]); } catch (error) { - exitCode = 1; - stderr += (error instanceof Error ? error.message : String(error)) + "\n"; + // Mirror the binary's `handleError` exactly so tests observe the same exit + // code and stderr (one JSON envelope line) the real CLI would emit. + const out = formatErrorOutput(error); + exitCode = out.exitCode; + stderr += out.stderr; } finally { if (timer) clearTimeout(timer); process.stdout.write = originalStdoutWrite; process.stderr.write = originalStderrWrite; - console.error = originalConsoleError; - console.warn = originalConsoleWarn; for (const [key, value] of Object.entries(envBackup)) { if (value === undefined) delete process.env[key]; else process.env[key] = value; diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index 940cfb559..6dc3d8b43 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -15,6 +15,7 @@ "atomically": "^2.1.1", "commander": "^13.1.0", "pino": "^9.14.0", + "undici": "^8.5.0", "zod": "^4.3.6", "zustand": "^5.0.13" }, @@ -5378,6 +5379,15 @@ "dev": true, "license": "MIT" }, + "node_modules/undici": { + "version": "8.5.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-8.5.0.tgz", + "integrity": "sha512-xamtWoB1EshgjpmlXd7GGm2VfdDtw1+rD8uhry8pSNW3If6S8E0m2T2+orSKeZXEn/aPJMviCpDBA65WJt8zhg==", + "license": "MIT", + "engines": { + "node": ">=22.19.0" + } + }, "node_modules/undici-types": { "version": "7.18.2", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.18.2.tgz", diff --git a/clients/cli/package.json b/clients/cli/package.json index d76b3b455..5f1855508 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -38,6 +38,7 @@ "atomically": "^2.1.1", "commander": "^13.1.0", "pino": "^9.14.0", + "undici": "^8.5.0", "zod": "^4.3.6", "zustand": "^5.0.13" }, diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 972577453..5578076c3 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -1,20 +1,216 @@ -function formatError(error: unknown): string { - let message: string; - - if (error instanceof Error) { - message = error.message; - } else if (typeof error === "string") { - message = error; - } else { - message = "Unknown error"; +/** + * Exit-code map. Non-zero codes let an automated caller (CI, an agent) branch + * on the failure class without regex-scraping stderr: + * + * - 0: success + * - 1: usage / unexpected error (the catch-all; same as before this map) + * - 2: `--app-info` probe found no MCP App on the tool + * - 3: server requires authentication (401 / WWW-Authenticate / OAuth) + * - 4: server unreachable (DNS, connect refused, timeout, fetch failure) + * - 5: tool error (`tools/call` returned `isError:true`, or tool not found) + */ +export const EXIT_CODES = { + OK: 0, + USAGE: 1, + NO_APP: 2, + AUTH_REQUIRED: 3, + UNREACHABLE: 4, + TOOL_ERROR: 5, +} as const; + +/** Machine-readable error envelope written as one JSON line on stderr. */ +export interface ErrorEnvelope { + /** Stable identifier for the failure class (e.g. "auth_required"). */ + code: string; + /** Human-readable message. */ + message: string; + /** The underlying error's `cause` (e.g. undici's ENOTFOUND), if any. */ + cause?: string; + /** HTTP status when the failure was an HTTP-level response. */ + status?: number; + /** The server URL the failure was against, when known. */ + url?: string; +} + +/** + * Thrown by the CLI to request a specific non-zero exit code without routing + * through the generic error path. {@link formatErrorOutput} reads `exitCode` + * and `envelope`; the in-process test runner does the same so tests observe + * the real code and stderr. + */ +export class CliExitCodeError extends Error { + constructor( + public readonly exitCode: number, + message: string, + public readonly envelope?: Partial, + ) { + super(message); + this.name = "CliExitCodeError"; } +} - return message; +/** Depth cap for {@link causeOf} — bounds a pathological/self-referential + * `error.cause` chain so a cycle can never recurse infinitely. */ +const MAX_CAUSE_DEPTH = 16; + +/** Read an `Error.cause` chain into a single readable string, if present. */ +function causeOf(error: unknown, depth = 0): string | undefined { + if (depth >= MAX_CAUSE_DEPTH) return undefined; + if (!(error instanceof Error)) return undefined; + const cause = (error as { cause?: unknown }).cause; + if (cause === undefined || cause === null) return undefined; + if (cause instanceof Error) { + const nested = causeOf(cause, depth + 1); + return nested ? `${cause.message}: ${nested}` : cause.message; + } + return String(cause); } -export function handleError(error: unknown): never { - const errorMessage = formatError(error); - console.error(errorMessage); +/** Best-effort HTTP status from common error shapes (undici, MCP SDK, fetch). */ +function statusOf(error: unknown): number | undefined { + if (error == null || typeof error !== "object") return undefined; + const e = error as { + status?: unknown; + code?: unknown; + response?: { status?: unknown }; + }; + if (typeof e.status === "number") return e.status; + if (typeof e.response?.status === "number") return e.response.status; + // SDK SseError / StreamableHTTPError expose `.code` as the HTTP status. Guard + // to the HTTP range (100-599) so a JSON-RPC McpError code (e.g. -32601 + // MethodNotFound) is not mistaken for an HTTP status and leaked into the + // envelope or misclassified as AUTH_REQUIRED. String node codes like + // "ENOTFOUND" are already excluded by the numeric check. + if (typeof e.code === "number" && e.code >= 100 && e.code <= 599) { + return e.code; + } + return undefined; +} + +const UNREACHABLE_PATTERN = + /ENOTFOUND|ECONNREFUSED|ECONNRESET|EAI_AGAIN|ETIMEDOUT|fetch failed|getaddrinfo|connect timed out|aborted/i; + +/** + * Classify an arbitrary error into an exit code and envelope. Used both by the + * binary's {@link handleError} and by callers that want to throw a + * {@link CliExitCodeError} with the right code up front. + */ +export function classifyError( + error: unknown, + context?: { url?: string }, +): { exitCode: number; envelope: ErrorEnvelope } { + const message = + error instanceof Error + ? error.message + : typeof error === "string" + ? error + : "Unknown error"; + const cause = causeOf(error); + const status = statusOf(error); + const url = context?.url; + + // A pre-classified error carries its own exit code; fill in any envelope + // fields the thrower didn't supply. + if (error instanceof CliExitCodeError) { + return { + exitCode: error.exitCode, + envelope: { + code: error.envelope?.code ?? codeForExit(error.exitCode), + message, + ...(cause !== undefined && { cause }), + ...(error.envelope?.status !== undefined && { + status: error.envelope.status, + }), + ...((error.envelope?.url ?? url) !== undefined && { + url: error.envelope?.url ?? url, + }), + }, + }; + } - process.exit(1); + // 401 / OAuth-required → AUTH_REQUIRED so the caller can kick the auth flow. + if ( + status === 401 || + status === 403 || + /WWW-Authenticate|Unauthorized|invalid_token|OAuth/i.test( + message + " " + (cause ?? ""), + ) + ) { + return { + exitCode: EXIT_CODES.AUTH_REQUIRED, + envelope: { + code: "auth_required", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; + } + + // Network-layer failure → UNREACHABLE so the caller can retry via a proxy. + if (UNREACHABLE_PATTERN.test(message + " " + (cause ?? ""))) { + return { + exitCode: EXIT_CODES.UNREACHABLE, + envelope: { + code: "unreachable", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; + } + + return { + exitCode: EXIT_CODES.USAGE, + envelope: { + code: "error", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; +} + +function codeForExit(exitCode: number): string { + switch (exitCode) { + case EXIT_CODES.NO_APP: + return "no_app"; + case EXIT_CODES.AUTH_REQUIRED: + return "auth_required"; + case EXIT_CODES.UNREACHABLE: + return "unreachable"; + case EXIT_CODES.TOOL_ERROR: + return "tool_error"; + default: + return "error"; + } +} + +/** + * Single source of truth for mapping a thrown error to the CLI's exit code and + * stderr text. The binary entry ({@link handleError}) and the in-process test + * runner both call this, so tests observe exactly what the binary would emit. + * + * stderr is one JSON line `{"error":{...}}` so a caller can `2>&1 | tail -1 | + * jq .error`; the message is inside the envelope so nothing is lost vs. the + * previous bare-message behaviour. + */ +export function formatErrorOutput( + error: unknown, + context?: { url?: string }, +): { exitCode: number; stderr: string } { + const { exitCode, envelope } = classifyError(error, context); + return { + exitCode, + stderr: JSON.stringify({ error: envelope }) + "\n", + }; +} + +export function handleError(error: unknown): never { + const { exitCode, stderr } = formatErrorOutput(error); + process.stderr.write(stderr); + process.exit(exitCode); } diff --git a/clients/web/README.md b/clients/web/README.md index 7dbf7ebf3..fcb72b06f 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -71,3 +71,7 @@ export default defineConfig([ }, ]) ``` + +## HTTP proxy support + +The web backend connects to remote MCP servers through the shared Node transport (`core/mcp/node/transport.ts`), which honors the conventional proxy environment variables: `HTTPS_PROXY` / `HTTP_PROXY` (and their lowercase forms) select the proxy, and `NO_PROXY` exempts hosts. Routing is powered by [`undici`](https://www.npmjs.com/package/undici)'s `EnvHttpProxyAgent`, imported lazily only when a proxy variable is set, so runs without a proxy configured pay no cost. See the CLI README for more detail. diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index eef9a6405..7d7e2abae 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1605,6 +1605,75 @@ describe("App history pin/replay", () => { }); }); +// The `/oauth/callback` handler must reject a returned `state` that does not +// parse to the expected 64-char-hex authId shape (a forgery indicator) instead +// of silently proceeding. See #1562. +describe("App OAuth callback state validation", () => { + const originalUrl = window.location.href; + + beforeEach(() => { + notificationsMock.show.mockClear(); + window.sessionStorage.clear(); + }); + + afterEach(() => { + window.history.replaceState({}, "", originalUrl); + }); + + it("rejects an unparseable state param with a clear error toast", async () => { + window.history.replaceState( + {}, + "", + "/oauth/callback?code=abc123&state=not-a-valid-state", + ); + + renderWithMantine(); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "OAuth callback rejected", + color: "red", + }), + ), + ); + }); + + it("does not reject when the state param parses to a valid authId", async () => { + // A well-formed 64-char-hex state passes the shape guard, so the handler + // proceeds to the server-matching step. With the resume snapshot pointing at + // a server id that is not registered, that step surfaces the "could not be + // matched" toast — asserting on that specific downstream toast proves the + // state was accepted (never the "OAuth callback rejected" toast) rather than + // relying on an indirect "some toast fired" check. + writeOAuthResumeSnapshot({ + version: 1, + serverId: "server-that-does-not-exist", + activeTab: "Tools", + authKind: "reauth", + tabUi: {}, + }); + window.history.replaceState( + {}, + "", + `/oauth/callback?code=abc123&state=${"a".repeat(64)}`, + ); + + renderWithMantine(); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "OAuth callback could not be matched", + }), + ), + ); + expect(notificationsMock.show).not.toHaveBeenCalledWith( + expect.objectContaining({ title: "OAuth callback rejected" }), + ); + }); +}); + describe("App OAuth resume lifecycle", () => { const storage = new Map(); @@ -1744,7 +1813,6 @@ describe("App OAuth resume lifecycle", () => { INSPECTOR_SERVERS_TAB, ), ); - expect(readOAuthResumeSnapshot()).toBeUndefined(); await user.click(screen.getByText("connect")); await waitFor(() => diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 1999cd1cb..c197c8ac1 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -86,7 +86,10 @@ import { useServers } from "@inspector/core/react/useServers.js"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft.js"; import { useEmaIdpLoginState } from "@inspector/core/react/useEmaIdpLoginState.js"; -import { getBrowserOAuthStorage } from "@inspector/core/auth/browser/index.js"; +import { + getRemoteOAuthStorage, + getWebOAuthBaseUrl, +} from "./lib/remoteOAuthStorage"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; import { useManagedPrompts } from "@inspector/core/react/useManagedPrompts.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; @@ -2098,9 +2101,10 @@ function App() { // `onToggleConnection` unloaded the previous one), so all React state is // reset and we recover the initiating server from sessionStorage. We wait for // `servers` to hydrate before acting; the ref guard keeps the exchange to a - // single run. The persisted PKCE verifier + DCR client info live in - // `BrowserOAuthStorage` and survive the redirect, so `completeOAuthFlow` - // exchanges the code without needing the original in-memory state machine. + // single run. The persisted PKCE verifier + DCR client info live in the + // backend-backed `RemoteOAuthStorage` (`~/.mcp-inspector/storage/oauth.json`) + // and survive the redirect, so `completeOAuthFlow` exchanges the code without + // needing the original in-memory state machine. useEffect(() => { if (typeof window === "undefined") return; if (window.location.pathname !== OAUTH_CALLBACK_PATH) return; @@ -2115,16 +2119,41 @@ function App() { // session key the pre-redirect page saved the fetch log under, so the // rebuilt client can restore those `auth` entries. Read it before the // URL is cleared below. + // + // Defense-in-depth: a `state` that is present but does not parse to our + // expected 64-char-hex authId shape did not originate from + // `generateOAuthState`, so reject the callback instead of silently + // proceeding with an undefined sessionId. This is a shape check, not full + // state-matching — the primary CSRF protection remains PKCE + // (`code_verifier`); this layer catches malformed/forged `state` early. + // + // Intentional asymmetry: a present-but-malformed `state` (including the + // empty string `?state=`) is rejected, but a wholly absent `state` + // (`stateParam === null`) is *not* — it falls through with + // `sessionId = undefined` and is matched via the OAuth resume snapshot + // (`resumeSnapshot.serverId`) instead. Rejecting the null case would turn any provider error redirect + // that omits `state` into a misleading "rejected" toast, hiding the real + // OAuth error surfaced by the `!params.successful` branch below. const stateParam = new URLSearchParams(window.location.search).get("state"); - const sessionId = stateParam - ? (parseOAuthState(stateParam)?.authId ?? undefined) - : undefined; + const parsedState = stateParam ? parseOAuthState(stateParam) : null; + const stateRejected = stateParam !== null && parsedState === null; + const sessionId = parsedState?.authId ?? undefined; const resumeSnapshot = consumeOAuthResumeSnapshot(); // Strip the code/state off the URL immediately so a reload can't replay // the (now single-use) authorization code through the exchange again. window.history.replaceState({}, "", "/"); + if (stateRejected) { + notifications.show({ + title: "OAuth callback rejected", + message: + "OAuth callback carried an unrecognized state parameter that did not originate from this session. Please try connecting again.", + color: "red", + }); + return; + } + const applyResumeUiOnce = ( snapshot: NonNullable, ) => { @@ -3239,7 +3268,10 @@ function App() { const clientSettingsModalValue = clientSettingsDraft ?? EMPTY_CLIENT_SETTINGS; - const emaOAuthStorage = useMemo(() => getBrowserOAuthStorage(), []); + const emaOAuthStorage = useMemo( + () => getRemoteOAuthStorage(getWebOAuthBaseUrl(), getAuthToken()), + [], + ); const { loginState: emaIdpLoginState, logout: logoutEmaIdp } = useEmaIdpLoginState( emaOAuthStorage, @@ -3270,6 +3302,10 @@ function App() { config: server.config, inspectorClient: isActive ? inspectorClient : null, isActiveConnection: isActive, + oauthStorage: getRemoteOAuthStorage( + getWebOAuthBaseUrl(), + getAuthToken(), + ), }); if (!cleared) return; diff --git a/clients/web/src/components/elements/AppRenderer/hostContext.test.ts b/clients/web/src/components/elements/AppRenderer/hostContext.test.ts new file mode 100644 index 000000000..230413327 --- /dev/null +++ b/clients/web/src/components/elements/AppRenderer/hostContext.test.ts @@ -0,0 +1,208 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { McpUiDisplayMode } from "@modelcontextprotocol/ext-apps/app-bridge"; +import { + currentStyles, + currentTheme, + measureContainerDimensions, + snapshotHostContext, +} from "./hostContext"; + +const COLOR_SCHEME_ATTR = "data-mantine-color-scheme"; + +/** + * Build a fake CSSStyleDeclaration whose `getPropertyValue` reads from a + * lookup table, so tests control exactly which CSS variables resolve (and to + * what) without depending on happy-dom's custom-property resolution. + */ +function fakeComputedStyle( + values: Record, +): CSSStyleDeclaration { + return { + getPropertyValue: (name: string) => values[name] ?? "", + } as unknown as CSSStyleDeclaration; +} + +function stubComputedStyle(values: Record) { + vi.spyOn(window, "getComputedStyle").mockReturnValue( + fakeComputedStyle(values), + ); +} + +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + document.documentElement.removeAttribute(COLOR_SCHEME_ATTR); +}); + +describe("currentTheme", () => { + it("returns the resolved dark scheme written by Mantine", () => { + document.documentElement.setAttribute(COLOR_SCHEME_ATTR, "dark"); + expect(currentTheme()).toBe("dark"); + }); + + it("returns the resolved light scheme written by Mantine", () => { + document.documentElement.setAttribute(COLOR_SCHEME_ATTR, "light"); + expect(currentTheme()).toBe("light"); + }); + + it("ignores an unrecognized attribute value and falls back", () => { + document.documentElement.setAttribute(COLOR_SCHEME_ATTR, "sepia"); + vi.stubGlobal("matchMedia", () => ({ matches: false })); + expect(currentTheme()).toBe("light"); + }); + + it("falls back to matchMedia dark when the attribute is absent", () => { + document.documentElement.removeAttribute(COLOR_SCHEME_ATTR); + vi.stubGlobal("matchMedia", (query: string) => ({ + matches: query.includes("dark"), + })); + expect(currentTheme()).toBe("dark"); + }); + + it("falls back to light when matchMedia does not match dark", () => { + document.documentElement.removeAttribute(COLOR_SCHEME_ATTR); + vi.stubGlobal("matchMedia", () => ({ matches: false })); + expect(currentTheme()).toBe("light"); + }); + + it("falls back to light when matchMedia is unavailable", () => { + document.documentElement.removeAttribute(COLOR_SCHEME_ATTR); + vi.stubGlobal("matchMedia", undefined); + expect(currentTheme()).toBe("light"); + }); + + it("returns light in a non-DOM environment (no document, no window)", () => { + vi.stubGlobal("document", undefined); + vi.stubGlobal("window", undefined); + expect(currentTheme()).toBe("light"); + }); + + it("consults matchMedia when document is absent but window is present", () => { + vi.stubGlobal("document", undefined); + vi.stubGlobal("matchMedia", (query: string) => ({ + matches: query.includes("dark"), + })); + expect(currentTheme()).toBe("dark"); + }); +}); + +describe("currentStyles", () => { + it("maps resolved Mantine tokens to spec style variables", () => { + stubComputedStyle({ + "--mantine-color-body": "#101113", + "--mantine-color-text": "#c9c9c9", + "--mantine-radius-md": "0.5rem", + }); + const styles = currentStyles(); + expect(styles).toEqual({ + variables: { + "--color-background-primary": "#101113", + "--color-text-primary": "#c9c9c9", + "--border-radius-md": "0.5rem", + }, + }); + }); + + it("trims whitespace and omits variables that resolve to empty", () => { + stubComputedStyle({ + "--mantine-color-body": " #ffffff ", + "--mantine-color-text": " ", + }); + const styles = currentStyles(); + expect(styles?.variables?.["--color-background-primary"]).toBe("#ffffff"); + expect(styles?.variables?.["--color-text-primary"]).toBeUndefined(); + }); + + it("returns undefined when nothing resolves", () => { + stubComputedStyle({}); + expect(currentStyles()).toBeUndefined(); + }); + + it("returns undefined in a non-DOM environment (no document)", () => { + vi.stubGlobal("document", undefined); + expect(currentStyles()).toBeUndefined(); + }); + + it("returns undefined in a non-DOM environment (no window)", () => { + vi.stubGlobal("window", undefined); + expect(currentStyles()).toBeUndefined(); + }); +}); + +describe("measureContainerDimensions", () => { + it("returns whole-pixel width and height for a laid-out element", () => { + const el = document.createElement("div"); + vi.spyOn(el, "getBoundingClientRect").mockReturnValue({ + width: 320.6, + height: 199.4, + } as DOMRect); + expect(measureContainerDimensions(el)).toEqual({ width: 321, height: 199 }); + }); + + it("returns undefined for a 0x0 (unattached) element", () => { + const el = document.createElement("div"); + vi.spyOn(el, "getBoundingClientRect").mockReturnValue({ + width: 0, + height: 0, + } as DOMRect); + expect(measureContainerDimensions(el)).toBeUndefined(); + }); + + it("returns undefined when only one dimension is zero", () => { + const el = document.createElement("div"); + vi.spyOn(el, "getBoundingClientRect").mockReturnValue({ + width: 100, + height: 0, + } as DOMRect); + expect(measureContainerDimensions(el)).toBeUndefined(); + }); + + it("returns undefined when getBoundingClientRect is unavailable", () => { + const el = {} as unknown as HTMLElement; + expect(measureContainerDimensions(el)).toBeUndefined(); + }); +}); + +describe("snapshotHostContext", () => { + const MODES: readonly McpUiDisplayMode[] = ["inline", "fullscreen"]; + + it("seeds theme, inline display mode, and a copied available-modes list", () => { + document.documentElement.setAttribute(COLOR_SCHEME_ATTR, "dark"); + stubComputedStyle({}); + const context = snapshotHostContext(null, MODES); + expect(context.theme).toBe("dark"); + expect(context.displayMode).toBe("inline"); + expect(context.availableDisplayModes).toEqual(["inline", "fullscreen"]); + // A copy, not the caller's array. + expect(context.availableDisplayModes).not.toBe(MODES); + expect(context.styles).toBeUndefined(); + expect(context.containerDimensions).toBeUndefined(); + }); + + it("includes styles and container dimensions when both resolve", () => { + document.documentElement.setAttribute(COLOR_SCHEME_ATTR, "light"); + stubComputedStyle({ "--mantine-color-body": "#ffffff" }); + const container = document.createElement("div"); + vi.spyOn(container, "getBoundingClientRect").mockReturnValue({ + width: 640, + height: 480, + } as DOMRect); + const context = snapshotHostContext(container, MODES); + expect(context.theme).toBe("light"); + expect(context.styles).toEqual({ + variables: { "--color-background-primary": "#ffffff" }, + }); + expect(context.containerDimensions).toEqual({ width: 640, height: 480 }); + }); + + it("omits container dimensions when the container has no layout box", () => { + stubComputedStyle({}); + const container = document.createElement("div"); + vi.spyOn(container, "getBoundingClientRect").mockReturnValue({ + width: 0, + height: 0, + } as DOMRect); + const context = snapshotHostContext(container, MODES); + expect(context.containerDimensions).toBeUndefined(); + }); +}); diff --git a/clients/web/src/components/elements/AppRenderer/hostContext.ts b/clients/web/src/components/elements/AppRenderer/hostContext.ts new file mode 100644 index 000000000..9ea6f8f08 --- /dev/null +++ b/clients/web/src/components/elements/AppRenderer/hostContext.ts @@ -0,0 +1,156 @@ +import type { + McpUiDisplayMode, + McpUiHostContext, + McpUiHostStyles, + McpUiStyles, + McpUiStyleVariableKey, +} from "@modelcontextprotocol/ext-apps/app-bridge"; + +/** + * Resolve the host theme from the DOM. Mantine writes the resolved color + * scheme to ``. Reading it here (rather than + * capturing React state) keeps the bridge factory's identity stable across + * theme toggles — the renderer treats a new factory identity as "rebuild the + * bridge", which would reload a running app's iframe on every theme flip. + * + * The attribute is only ever `"light"` or `"dark"` — Mantine resolves + * `defaultColorScheme="auto"` to the system value before paint and never + * writes `"auto"` here, so no `auto` branch is needed. The matchMedia + * fallback only covers the attribute being absent (e.g. a hydration race). + */ +export function currentTheme(): "light" | "dark" { + if (typeof document !== "undefined") { + const attr = document.documentElement.getAttribute( + "data-mantine-color-scheme", + ); + if (attr === "dark" || attr === "light") return attr; + } + if ( + typeof window !== "undefined" && + window.matchMedia?.("(prefers-color-scheme: dark)").matches + ) { + return "dark"; + } + return "light"; +} + +/** + * Maps the spec's host-style variable keys ({@link McpUiStyleVariableKey}) to + * the inspector's underlying CSS custom properties. The inspector themes itself + * with Mantine, so each spec token resolves to the matching Mantine design-token + * variable (or an `--inspector-*` token layered on top of one). Only a curated + * subset of the ~90 spec keys is mapped — the ones the inspector has a + * meaningful equivalent for; the rest are omitted, which the spec allows (hosts + * may provide any subset). + */ +const STYLE_VARIABLE_SOURCES: Partial> = { + "--color-background-primary": "--mantine-color-body", + "--color-background-secondary": "--inspector-surface-card", + "--color-background-tertiary": "--inspector-surface-subtle", + "--color-text-primary": "--mantine-color-text", + "--color-text-secondary": "--inspector-text-secondary", + "--color-text-inverse": "--inspector-text-inverse", + "--color-text-info": "--inspector-log-info", + "--color-text-danger": "--inspector-log-error", + "--color-text-success": "--inspector-status-connected", + "--color-text-warning": "--inspector-log-warning", + "--color-border-primary": "--inspector-border-default", + "--color-border-secondary": "--inspector-border-subtle", + "--font-sans": "--mantine-font-family", + "--font-mono": "--mantine-font-family-monospace", + "--font-text-xs-size": "--mantine-font-size-xs", + "--font-text-sm-size": "--mantine-font-size-sm", + "--font-text-md-size": "--mantine-font-size-md", + "--font-text-lg-size": "--mantine-font-size-lg", + "--border-radius-xs": "--mantine-radius-xs", + "--border-radius-sm": "--mantine-radius-sm", + "--border-radius-md": "--mantine-radius-md", + "--border-radius-lg": "--mantine-radius-lg", + "--border-radius-xl": "--mantine-radius-xl", + "--shadow-sm": "--mantine-shadow-sm", + "--shadow-md": "--mantine-shadow-md", + "--shadow-lg": "--mantine-shadow-lg", +}; + +const STYLE_VARIABLE_ENTRIES = Object.entries(STYLE_VARIABLE_SOURCES) as [ + McpUiStyleVariableKey, + string, +][]; + +/** + * Resolve the inspector's design tokens into a {@link McpUiHostStyles} for + * hostContext, so style-aware apps can theme themselves from the host instead + * of falling back to their own defaults. Reads the computed value of each + * mapped CSS variable from the document root — which reflects the active + * Mantine color scheme — and keeps only the ones that resolve to a non-empty + * value. Returns undefined when nothing resolves (e.g. a non-DOM/test + * environment) so we never advertise an empty styles object. + */ +export function currentStyles(): McpUiHostStyles | undefined { + if (typeof document === "undefined" || typeof window === "undefined") { + return undefined; + } + const computed = window.getComputedStyle(document.documentElement); + const variables: McpUiStyles = {} as McpUiStyles; + let resolved = false; + for (const [specKey, sourceVar] of STYLE_VARIABLE_ENTRIES) { + const value = computed.getPropertyValue(sourceVar).trim(); + if (value) { + variables[specKey] = value; + resolved = true; + } + } + return resolved ? { variables } : undefined; +} + +/** + * Spec shape for `hostContext.containerDimensions`. Derived from + * {@link McpUiHostContext} so the seed and live-push paths share one source of + * truth and stay in lockstep with the spec types. + */ +export type ContainerDimensions = NonNullable< + McpUiHostContext["containerDimensions"] +>; + +/** + * Measure the host container an app renders into and return its concrete + * `{ width, height }` (whole pixels). Returns undefined when the element has + * no layout box yet (0×0 — e.g. before the iframe is attached, or in a + * non-DOM/test environment) so a meaningless size is never seeded into + * hostContext. The return type is the concrete pair rather than the spec's + * {@link ContainerDimensions} union so callers can compare both fields. + */ +export function measureContainerDimensions( + element: HTMLElement, +): { width: number; height: number } | undefined { + if (typeof element.getBoundingClientRect !== "function") return undefined; + const rect = element.getBoundingClientRect(); + const width = Math.round(rect.width); + const height = Math.round(rect.height); + if (width <= 0 || height <= 0) return undefined; + return { width, height }; +} + +/** + * Read the live host UI state into a {@link McpUiHostContext} for the bridge + * handshake — the single place that decides which fields the inspector seeds. + * Optional fields are omitted (not set undefined) so the SDK's diff stays + * accurate; subsequent live changes are pushed by the renderer's observers as + * partial `host-context-changed` notifications. + */ +export function snapshotHostContext( + container: HTMLElement | null, + availableDisplayModes: readonly McpUiDisplayMode[], +): McpUiHostContext { + const styles = currentStyles(); + const containerDimensions = container + ? measureContainerDimensions(container) + : undefined; + return { + theme: currentTheme(), + displayMode: "inline", + availableDisplayModes: [...availableDisplayModes], + ...(styles ? { styles } : {}), + ...(containerDimensions ? { containerDimensions } : {}), + }; +} diff --git a/clients/web/src/components/groups/PromptArgumentsForm/PromptArgumentsForm.test.tsx b/clients/web/src/components/groups/PromptArgumentsForm/PromptArgumentsForm.test.tsx index 679bd02eb..f00afc6c2 100644 --- a/clients/web/src/components/groups/PromptArgumentsForm/PromptArgumentsForm.test.tsx +++ b/clients/web/src/components/groups/PromptArgumentsForm/PromptArgumentsForm.test.tsx @@ -2,9 +2,25 @@ import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { Prompt } from "@modelcontextprotocol/sdk/types.js"; -import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { + renderWithMantine, + screen, + waitFor, + fireEvent, + act, +} from "../../../test/renderWithMantine"; import { PromptArgumentsForm } from "./PromptArgumentsForm"; +// Completion requests are debounced (COMPLETION_DEBOUNCE_MS = 300ms). Rather +// than sleeping past that window on the wall clock — which races the debounce +// timer under instrumented/concurrent load (#1596) — every completion test +// below uses `userEvent.setup({ delay: null })` (no per-keystroke real-timer +// scheduling) and waits on the *real* rendered outcome via `findBy`/`waitFor`. +// Those poll until the debounce fires and the response settles, so they +// resolve as soon as the work is done and never depend on machine speed. A +// scoped 3s timeout covers the debounce + async settle under heavy load. +const SETTLE = { timeout: 3000 } as const; + /** * Wrapper that owns `argumentValues` state so completion tests can * type multi-character input naturally — the production parent @@ -130,7 +146,7 @@ describe("PromptArgumentsForm", () => { }); it("invokes onArgumentChange when typing in an argument input", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onArgumentChange = vi.fn(); renderWithMantine( { }); it("clears an argument via its Clear button (onArgumentChange with empty value)", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onArgumentChange = vi.fn(); renderWithMantine( { }); it("invokes onGetPrompt when Get Prompt is clicked", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onGetPrompt = vi.fn(); renderWithMantine( { }); it("disables Get Prompt until every required argument has a value", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( , ); @@ -219,7 +235,7 @@ describe("PromptArgumentsForm", () => { describe("completions", () => { it("fires a completion immediately on focus before any keystroke", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi .fn< ( @@ -240,17 +256,16 @@ describe("PromptArgumentsForm", () => { ); await user.click(screen.getByRole("textbox", { name: /^text/ })); - // No debounce on focus — the call fires synchronously off the - // focus handler. A microtask is enough for the response to settle. - await new Promise((r) => setTimeout(r, 0)); + // No debounce on focus — the call fires synchronously off the focus + // handler; wait on the rendered option rather than a fixed sleep. + expect(await screen.findByText("alpha")).toBeInTheDocument(); expect(onCompleteArgument).toHaveBeenCalledWith("text", "", { targetLanguage: "", }); - expect(await screen.findByText("alpha")).toBeInTheDocument(); }); it("calls onCompleteArgument (debounced) and surfaces values when supported", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi .fn< ( @@ -271,18 +286,19 @@ describe("PromptArgumentsForm", () => { ); await user.type(screen.getByRole("textbox", { name: /^text/ }), "al"); - await new Promise((r) => setTimeout(r, 400)); - // The last call carries the typed value and the context for the - // other (still empty) sibling. + // Wait for the debounced response to render, then assert the last call + // carries the typed value and the (still empty) sibling context. + expect( + await screen.findByText("alpha", undefined, SETTLE), + ).toBeInTheDocument(); + expect(screen.getByText("alphabet")).toBeInTheDocument(); expect(onCompleteArgument).toHaveBeenLastCalledWith("text", "al", { targetLanguage: "", }); - expect(await screen.findByText("alpha")).toBeInTheDocument(); - expect(screen.getByText("alphabet")).toBeInTheDocument(); }); it("sends every sibling argument in context, including the unset ones", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi .fn< ( @@ -304,16 +320,20 @@ describe("PromptArgumentsForm", () => { ); await user.type(screen.getByRole("textbox", { name: /^text/ }), "h"); - await new Promise((r) => setTimeout(r, 400)); - // The completing arg ("text") is excluded from context; every - // other declared argument rides along — even ones still empty. - expect(onCompleteArgument).toHaveBeenLastCalledWith("text", "h", { - targetLanguage: "es", - }); + // The completing arg ("text") is excluded from context; every other + // declared argument rides along — even ones still empty. Poll until the + // debounced "text" call lands rather than sleeping past the window. + await waitFor( + () => + expect(onCompleteArgument).toHaveBeenLastCalledWith("text", "h", { + targetLanguage: "es", + }), + SETTLE, + ); }); it("captures sibling values at fire time, not at schedule time", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi .fn< ( @@ -335,34 +355,31 @@ describe("PromptArgumentsForm", () => { // Type into "text" — this schedules a debounced completion call. await user.type(screen.getByRole("textbox", { name: /^text/ }), "h"); - // Before the 300ms debounce fires, update the sibling. The - // text-arg fire that lands at t=300 must see the latest sibling - // value, not the empty one captured at schedule time. + // Before the 300ms debounce fires, update the sibling. Because typing is + // synchronous (delay: null), the sibling holds its value well before the + // debounce window elapses. The text-arg fire must read the sibling at + // *fire* time, not the empty value captured at schedule time. await user.type( screen.getByRole("textbox", { name: /targetLanguage/ }), "es", ); - await new Promise((r) => setTimeout(r, 400)); - - // The most recent call for "text" carries the up-to-date sibling - // value, even though it was scheduled before the sibling was typed. - // (There's also a focus-fire call when the second input gained focus — - // separate stream, not asserted here.) The exact sibling string depends - // on where the 300ms debounce lands relative to the two real-timer - // keystrokes ("e" then "es"), so assert the load-bearing fact: the fire - // captured a NON-EMPTY sibling, i.e. read at fire time rather than the - // empty value present when the call was scheduled. - const textCalls = onCompleteArgument.mock.calls.filter( - ([n]) => n === "text", - ); - const lastTextCall = textCalls.at(-1); - expect(lastTextCall?.[0]).toBe("text"); - expect(lastTextCall?.[1]).toBe("h"); - expect(lastTextCall?.[2].targetLanguage).toMatch(/^es?$/); + + // Poll until the debounced "text" call fires, then assert it captured a + // non-empty sibling (read at fire time, not the empty value present when + // the call was scheduled). With delay:null the sibling is fully typed + // ("es") before the debounce fires; the /^es?$/ regex stays tolerant. + await waitFor(() => { + const textCalls = onCompleteArgument.mock.calls.filter( + ([n]) => n === "text", + ); + const lastTextCall = textCalls.at(-1); + expect(lastTextCall?.[1]).toBe("h"); + expect(lastTextCall?.[2].targetLanguage).toMatch(/^es?$/); + }, SETTLE); }); it("clears stale dropdown options the instant a new keystroke arrives", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const deferred: Array<{ value: string; resolve: (values: string[]) => void; @@ -386,22 +403,21 @@ describe("PromptArgumentsForm", () => { // Focus → first call (value=""). Resolve so the dropdown has // something to show. await user.click(screen.getByRole("textbox", { name: /^text/ })); - await new Promise((r) => setTimeout(r, 0)); - expect(deferred.length).toBe(1); + await waitFor(() => expect(deferred.length).toBe(1)); deferred[0].resolve(["alpha", "alphabet"]); expect(await screen.findByText("alpha")).toBeInTheDocument(); - // Type a new character — the keystroke handler must drop the - // stale options immediately so the dropdown doesn't show - // "alpha" / "alphabet" while the next request is in flight - // (300ms debounce + network latency). + // Type a new character — the keystroke handler must drop the stale + // options synchronously so the dropdown doesn't show "alpha" / + // "alphabet" while the next request is in flight (300ms debounce + + // network latency). await user.type(screen.getByRole("textbox", { name: /^text/ }), "z"); expect(screen.queryByText("alpha")).not.toBeInTheDocument(); expect(screen.queryByText("alphabet")).not.toBeInTheDocument(); }); it("aborts an in-flight request when a faster keystroke arrives", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const calls: Array<{ value: string; resolve: (values: string[]) => void; @@ -422,22 +438,25 @@ describe("PromptArgumentsForm", () => { />, ); - // Focus fires the first call (value=""). Type "h" → second call - // after debounce. Type "i" → third call after debounce. + // Focus fires the first call (value=""). Type "h" → second call after + // the debounce; wait for it to actually be in flight before typing more. await user.type(screen.getByRole("textbox", { name: /^text/ }), "h"); - await new Promise((r) => setTimeout(r, 350)); + await waitFor( + () => expect(calls.some((c) => c.value === "h")).toBe(true), + SETTLE, + ); await user.type(screen.getByRole("textbox", { name: /^text/ }), "i"); - await new Promise((r) => setTimeout(r, 350)); + await waitFor( + () => expect(calls.some((c) => c.value === "hi")).toBe(true), + SETTLE, + ); // Resolve the late "h" response — it should be dropped because // the form aborted that controller when the "hi" request started. const hi = calls.find((c) => c.value === "hi"); const h = calls.find((c) => c.value === "h"); - expect(hi).toBeDefined(); - expect(h).toBeDefined(); h?.resolve(["from-stale-h"]); hi?.resolve(["from-fresh-hi"]); - await new Promise((r) => setTimeout(r, 0)); // The dropdown shows the fresh response, not the stale one. expect(await screen.findByText("from-fresh-hi")).toBeInTheDocument(); @@ -445,7 +464,7 @@ describe("PromptArgumentsForm", () => { }); it("surfaces an empty dropdown when the completion request rejects", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); // The first (focus) call resolves with options; the debounced // keystroke call rejects. The rejection (not aborted) must clear the // dropdown to [] rather than leave the stale options showing. @@ -471,60 +490,88 @@ describe("PromptArgumentsForm", () => { const input = screen.getByRole("textbox", { name: /^text/ }); await user.click(input); - await new Promise((r) => setTimeout(r, 0)); expect(await screen.findByText("alpha")).toBeInTheDocument(); - // Type a char → debounced call rejects → options reset to []. + // Type a char → the stale options drop synchronously, and the debounced + // call then rejects, leaving the dropdown empty. Wait for the second + // (rejecting) call to fire so the catch → reset-to-[] path runs. await user.type(input, "z"); - await new Promise((r) => setTimeout(r, 400)); - expect(screen.queryByText("alpha")).not.toBeInTheDocument(); - expect(screen.queryByText("alphabet")).not.toBeInTheDocument(); - }); - - it("cancels a pending debounce timer when the input is re-focused", async () => { - const user = userEvent.setup(); - const onCompleteArgument = vi - .fn< - ( - argName: string, - value: string, - context: Record, - ) => Promise - >() - .mockResolvedValue([]); - - renderWithMantine( - , + await waitFor( + () => expect(onCompleteArgument).toHaveBeenCalledTimes(2), + SETTLE, ); - - const textInput = screen.getByRole("textbox", { name: /^text/ }); - const siblingInput = screen.getByRole("textbox", { - name: /targetLanguage/, + await waitFor(() => { + expect(screen.queryByText("alpha")).not.toBeInTheDocument(); + expect(screen.queryByText("alphabet")).not.toBeInTheDocument(); }); + }); - // Type into text → schedules a debounce timer. Immediately move focus - // away and back before the 300ms fires, so handleFocus sees a pending - // timer for "text" and clears it. - await user.type(textInput, "h"); - await user.click(siblingInput); - await user.click(textInput); - onCompleteArgument.mockClear(); - // The cancelled debounce must not fire a stale keystroke completion. - await new Promise((r) => setTimeout(r, 400)); - const textCalls = onCompleteArgument.mock.calls.filter( - ([n]) => n === "text", - ); - // Only the focus-fire call (value "h"), never the cancelled debounce. - expect(textCalls.every(([, v]) => v === "h")).toBe(true); + it("cancels a pending debounce timer when the input is re-focused", async () => { + // This is the one completion test that must let the 300ms debounce + // window fully elapse to be meaningful — asserting at t≈0 (as the + // sibling negative tests do) would pass whether or not handleFocus's + // clearTimeout actually cancels the pending timer. Fake timers make that + // window deterministic: we advance the clock directly. The interaction + // is driven with fireEvent rather than userEvent, because userEvent's + // async internals deadlock under vitest fake timers with this + // Mantine/happy-dom stack. This test is load-bearing: if the + // clearTimeout in handleFocus were removed, the debounce would fire + // during the advance and the final assertion would fail (verified). + vi.useFakeTimers(); + try { + const onCompleteArgument = vi + .fn< + ( + argName: string, + value: string, + context: Record, + ) => Promise + >() + .mockResolvedValue([]); + + renderWithMantine( + , + ); + + const textInput = screen.getByRole("textbox", { name: /^text/ }); + const siblingInput = screen.getByRole("textbox", { + name: /targetLanguage/, + }); + + // Type into text → schedules the 300ms debounce timer for "text". + fireEvent.focus(textInput); + fireEvent.change(textInput, { target: { value: "h" } }); + // Move focus to the sibling and back — the re-focus makes + // handleFocus("text") see the pending debounce timer and clear it. + fireEvent.focus(siblingInput); + fireEvent.focus(textInput); + + // Drop the focus-fire calls made above; only a *debounce* fire after + // this point would indicate the cancellation failed. + onCompleteArgument.mockClear(); + + // Elapse well past the debounce window. The cancelled timer must not + // fire a stale keystroke completion. + await act(async () => { + await vi.advanceTimersByTimeAsync(400); + }); + + const textCalls = onCompleteArgument.mock.calls.filter( + ([n]) => n === "text", + ); + expect(textCalls.length).toBe(0); + } finally { + vi.useRealTimers(); + } }); it("does not call onCompleteArgument when completions are unsupported", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi.fn(); renderWithMantine( { onCompleteArgument={onCompleteArgument} />, ); - // Focus the input first, then type — neither should trigger a call. + // Focus the input first, then type — neither path is wired to + // completions, so no request is ever scheduled or fired. await user.click(screen.getByPlaceholderText("Enter text...")); await user.type(screen.getByPlaceholderText("Enter text..."), "ab"); - await new Promise((r) => setTimeout(r, 400)); expect(onCompleteArgument).not.toHaveBeenCalled(); }); }); diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx index 14fe826e8..884a180b5 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx @@ -38,7 +38,7 @@ describe("ServerConfigModal", () => { }); it("rejects an id containing illegal characters", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine(); await user.type(screen.getByLabelText(/Server ID/i), "bad id!"); expect( @@ -47,7 +47,7 @@ describe("ServerConfigModal", () => { }); it("flags duplicate ids against existingIds", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( , ); @@ -56,7 +56,7 @@ describe("ServerConfigModal", () => { }); it("calls onSubmit with a valid stdio config", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -74,7 +74,7 @@ describe("ServerConfigModal", () => { }); it("requires a command for stdio submission", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -86,7 +86,7 @@ describe("ServerConfigModal", () => { }); it("rejects malformed env lines", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -118,7 +118,7 @@ describe("ServerConfigModal", () => { }); it("submits an sse config with just the url (headers move to the settings form)", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine( { }); it("submits a streamable-http config", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine( { }); it("rejects an empty URL on sse submission", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine( { }); it("shows the error message and stays open when onSubmit rejects", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = { ...base(), onSubmit: vi.fn().mockRejectedValue(new Error("server full")), @@ -252,7 +252,7 @@ describe("ServerConfigModal", () => { }); it("blocks submit and shows the id error message on bad-id submit click", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base({ existingIds: ["alpha"] }); renderWithMantine(); @@ -265,7 +265,7 @@ describe("ServerConfigModal", () => { }); it("supports editing the working directory field", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -283,7 +283,7 @@ describe("ServerConfigModal", () => { }); it("clears the Server ID field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("clears the Command field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("clears the URL field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("clears the Arguments field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("clears the Environment field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("clears the Working directory field via its Clear button", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("switches transport via the Transport select, revealing the URL field", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine(); // Starts on stdio — Command is shown, URL is not. @@ -411,7 +411,7 @@ describe("ServerConfigModal", () => { }); it("requires a server id before submitting", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -427,7 +427,7 @@ describe("ServerConfigModal", () => { }); it("rejects an env line whose '=' is at the start (no key)", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); @@ -442,7 +442,7 @@ describe("ServerConfigModal", () => { }); it("calls onClose when Cancel is clicked", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine(); await user.click(screen.getByRole("button", { name: /Cancel/ })); diff --git a/clients/web/src/components/groups/ServerImportConfigModal/ServerImportConfigModal.test.tsx b/clients/web/src/components/groups/ServerImportConfigModal/ServerImportConfigModal.test.tsx index 072f0edb1..cd038c576 100644 --- a/clients/web/src/components/groups/ServerImportConfigModal/ServerImportConfigModal.test.tsx +++ b/clients/web/src/components/groups/ServerImportConfigModal/ServerImportConfigModal.test.tsx @@ -94,14 +94,14 @@ describe("ServerImportConfigModal", () => { }); it("enables Import once a client is selected", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); await user.selectOptions(screen.getByLabelText("Client"), "Cursor"); expect(screen.getByRole("button", { name: "Import" })).toBeEnabled(); }); it("fetches a source and shows additions + conflicts in review", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => @@ -114,7 +114,7 @@ describe("ServerImportConfigModal", () => { }); it("imports additions and skips conflicts by default", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("New servers (1)")); @@ -136,7 +136,7 @@ describe("ServerImportConfigModal", () => { }); it("lets the user skip an individual new server", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("New servers (1)")); @@ -150,7 +150,7 @@ describe("ServerImportConfigModal", () => { }); it("skips a deselected new server while still importing others", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("New servers (1)")); @@ -171,7 +171,7 @@ describe("ServerImportConfigModal", () => { }); it("overwrites a conflict when chosen", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("Already exists (1)")); @@ -185,7 +185,7 @@ describe("ServerImportConfigModal", () => { }); it("renames a conflict to the supplied id", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("Already exists (1)")); @@ -204,7 +204,7 @@ describe("ServerImportConfigModal", () => { }); it("flags an invalid/colliding rename target and blocks import", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("Already exists (1)")); @@ -240,7 +240,7 @@ describe("ServerImportConfigModal", () => { }); it("reports a per-server failure in the summary", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup(); h.onAddServer.mockRejectedValueOnce(new Error("boom")); await pickClientAndImport(user, "Cursor"); @@ -253,7 +253,7 @@ describe("ServerImportConfigModal", () => { }); it("shows the searched-paths notice when no config is found", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup({ type: "cursor", found: false, @@ -266,7 +266,7 @@ describe("ServerImportConfigModal", () => { }); it("shows an error when the source has a parse error", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup({ type: "cursor", found: true, @@ -280,7 +280,7 @@ describe("ServerImportConfigModal", () => { }); it("shows an error when the fetch rejects", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(new Error("network down")); await pickClientAndImport(user, "Cursor"); await waitFor(() => @@ -289,7 +289,7 @@ describe("ServerImportConfigModal", () => { }); it("parses an uploaded file (mcpServers) into review", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); const file = new File( [JSON.stringify({ mcpServers: { fromfile: { command: "x" } } })], @@ -306,7 +306,7 @@ describe("ServerImportConfigModal", () => { }); it("parses an uploaded VS Code file (servers) into review", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); const file = new File( [JSON.stringify({ servers: { vscodesrv: { command: "x" } } })], @@ -323,7 +323,7 @@ describe("ServerImportConfigModal", () => { }); it("shows an error for an unparseable uploaded file", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); const file = new File(["{not json"], "mcp.json", { type: "application/json", @@ -338,7 +338,7 @@ describe("ServerImportConfigModal", () => { }); it("reports when the selected source has no servers", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup({ type: "cursor", found: true, @@ -352,7 +352,7 @@ describe("ServerImportConfigModal", () => { }); it("reflects the chosen client as the dropdown value", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); // Selecting a client makes the dropdown's value non-empty (the truthy // branch of `vm.selectedType ?? ""`). @@ -361,7 +361,7 @@ describe("ServerImportConfigModal", () => { }); it("resets the dropdown to no selection when the placeholder is re-chosen", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); const dropdown = screen.getByLabelText("Client"); await user.selectOptions(dropdown, "Cursor"); @@ -374,7 +374,7 @@ describe("ServerImportConfigModal", () => { }); it("renders a conflicts-only review without a new-servers section", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); // Every incoming server already exists → no additions, only conflicts (the // false branch of `plan.additions.length > 0`). setup( @@ -395,7 +395,7 @@ describe("ServerImportConfigModal", () => { }); it("renders an additions-only review without a conflicts section", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); // No existing ids → every incoming server is a brand-new addition, so the // "Already exists" conflicts section never renders (the false branch of // `plan.conflicts.length > 0`). @@ -415,7 +415,7 @@ describe("ServerImportConfigModal", () => { }); it("goes back from review to the source picker", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); setup(); await pickClientAndImport(user, "Cursor"); await waitFor(() => screen.getByText("New servers (1)")); @@ -426,7 +426,7 @@ describe("ServerImportConfigModal", () => { }); it("closes from the summary via Done", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const h = setup({ type: "cursor", found: true, diff --git a/clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx b/clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx index baedb6686..1627e3713 100644 --- a/clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx +++ b/clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx @@ -114,7 +114,7 @@ describe("ServerImportJsonModal", () => { }); it("builds the config with env overrides and calls onAddServer", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onAddServer = vi.fn().mockResolvedValue(undefined); const onClose = vi.fn(); renderWithMantine( @@ -142,7 +142,7 @@ describe("ServerImportJsonModal", () => { }); it("honors a server name override", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onAddServer = vi.fn().mockResolvedValue(undefined); renderWithMantine( { }); it("lets the user pick among multiple packages", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onAddServer = vi.fn().mockResolvedValue(undefined); renderWithMantine( { }); it("surfaces an onAddServer rejection instead of closing", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onAddServer = vi.fn().mockRejectedValue(new Error("disk full")); const onClose = vi.fn(); renderWithMantine( @@ -241,7 +241,7 @@ describe("ServerImportJsonModal", () => { }); it("loads server.json from a chosen file", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("re-opens File Contents when the textarea is cleared", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("rejects an invalid id override and blocks Add Server", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onAddServer = vi.fn(); renderWithMantine( { }); it("closes via the Escape key (no Cancel button)", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onClose = vi.fn(); renderWithMantine( { }); it("shows the read error alert when error and a resource is selected", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("falls back to default error when error message is missing", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( , ); @@ -98,7 +98,7 @@ describe("ResourcesScreen", () => { }); it("shows the loading state when reading", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( , ); @@ -107,7 +107,7 @@ describe("ResourcesScreen", () => { }); it("renders the preview panel when readState has a result", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("toggles a section's open state through the lifted openSections handler", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine(); const urisHeader = screen.getByRole("button", { name: /URIs \(2\)/ }); // Open by default (compact=false); clicking routes through ResourcesScreen's @@ -150,7 +150,7 @@ describe("ResourcesScreen", () => { }); it("renders the template panel when a template is selected", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine(); // Templates section is open by default; select the template to open its form. await user.click(screen.getByText("files")); @@ -160,7 +160,7 @@ describe("ResourcesScreen", () => { }); it("hides the template panel once the user reads the resource", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onReadResource = vi.fn(); renderWithMantine( , @@ -176,7 +176,7 @@ describe("ResourcesScreen", () => { }); it("auto-reads when a resource is clicked in the sidebar", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onReadResource = vi.fn(); renderWithMantine( , @@ -186,7 +186,7 @@ describe("ResourcesScreen", () => { }); it("forwards refresh and subscribe events from the preview panel", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onReadResource = vi.fn(); const onSubscribeResource = vi.fn(); renderWithMantine( @@ -209,7 +209,7 @@ describe("ResourcesScreen", () => { }); it("closing the preview returns to the originating template form", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onReadResource = vi.fn(); const templates: ResourceTemplate[] = [ { uriTemplate: "file:///{path}", name: "files" }, @@ -251,7 +251,7 @@ describe("ResourcesScreen", () => { }); it("closing the preview from the error state returns to the template form", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const templates: ResourceTemplate[] = [ { uriTemplate: "demo://resource/dynamic/text/{id}", name: "Dynamic" }, ]; @@ -282,7 +282,7 @@ describe("ResourcesScreen", () => { }); it("closing the preview for a plain resource returns to the empty state", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("invokes onUnsubscribeResource when already subscribed", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onUnsubscribeResource = vi.fn(); renderWithMantine( { }); it("routes sidebar search text through onUiChange", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onUiChange = vi.fn(); renderWithMantine(); await user.type(screen.getByPlaceholderText("Search..."), "y.txt"); @@ -329,7 +329,7 @@ describe("ResourcesScreen", () => { }); it("renders nothing in the preview when an ok state carries no result", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("threads onCompleteArgument with a ref/resource envelope from the template form", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const onCompleteArgument = vi .fn< ( @@ -379,7 +379,7 @@ describe("ResourcesScreen", () => { }); it("hides the Subscriptions section and Subscribe button when subscriptionsSupported is false", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { it("dispatches onToggleConnection with the server id when the card toggle is clicked", async () => { const onToggleConnection = vi.fn(); - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { })} />, ); - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const tabSelect = await screen.findByDisplayValue("Servers"); await user.click(tabSelect); await user.click(await screen.findByText("Tools")); @@ -515,7 +515,7 @@ describe("InspectorView", () => { }); it("filters tools to apps and auto-launches a no-fields app on the Apps tab", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); // Plain (non-app) tool plus a tool with a malformed UI resource URI // exercise both branches of the appTools filter: the non-app drop and // the try/catch around `isAppTool` for malformed metadata. @@ -631,7 +631,7 @@ describe("InspectorView", () => { }); it("snaps activeTab back to Servers when the Apps tab disappears after a refresh", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const { rerender } = renderWithMantine( { it("dispatches onSetLogLevel through to the Logs screen", async () => { const onSetLogLevel = vi.fn(); - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( { }); it("persists Logs sort direction to localStorage and restores it on remount", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const { unmount } = renderWithMantine( { }); it("falls back to newest-first when a corrupted sort value is stored", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); window.localStorage.setItem("inspector.sortDirection.history", "garbage"); renderWithMantine( { }); it("persists History list compact state to localStorage and restores it on remount", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const historyEntry = { id: "req-1", timestamp: new Date("2026-03-17T10:00:00Z"), @@ -982,7 +982,7 @@ describe("InspectorView", () => { }); it("falls back to collapsed when a corrupted compact value is stored", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); window.localStorage.setItem("inspector.listCompact.history", "garbage"); const historyEntry = { id: "req-1", @@ -1085,7 +1085,7 @@ describe("InspectorView", () => { }); it("toggles the Servers list compact state from the list toggle", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); renderWithMantine( , ); @@ -1099,7 +1099,7 @@ describe("InspectorView", () => { }); it("toggles the Network list compact state from the list toggle", async () => { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const httpServer: ServerEntry = { id: "beta", name: "Beta", @@ -1168,7 +1168,7 @@ describe("InspectorView", () => { // The indicator only mounts on the active screen, so each case connects, // navigates to the target tab, and asserts the "List updated" affordance. async function gotoTab(tab: string) { - const user = userEvent.setup(); + const user = userEvent.setup({ delay: null }); const tabSelect = await screen.findByDisplayValue("Servers"); await user.click(tabSelect); await user.click(await screen.findByText(tab)); diff --git a/clients/web/src/lib/downloadFile.ts b/clients/web/src/lib/downloadFile.ts index 77590717d..50b8c62d5 100644 --- a/clients/web/src/lib/downloadFile.ts +++ b/clients/web/src/lib/downloadFile.ts @@ -2,22 +2,23 @@ * Browser-side helpers for triggering file downloads from in-memory content. * * Centralized so the temp-anchor incantation (`appendChild` for Firefox, - * try/finally to keep cleanup bulletproof, `revokeObjectURL` to release the - * object URL) lives in one place — and so the wiring is unit-testable - * under happy-dom without dragging React along. + * deferred `revokeObjectURL` so the scheduled download can read the blob) + * lives in one place — and so the wiring is unit-testable under happy-dom + * without dragging React along. */ /** - * Download an in-memory string as a JSON file. Uses a temporary anchor + * Download an in-memory {@link Blob} as `filename`. Uses a temporary anchor * element to trigger the browser's save dialog. The append-to-body step is * for older Firefox versions that wouldn't fire `click()` on a detached * anchor; modern browsers don't require it but it stays as the safe path. * - * The cleanup (removeChild + revokeObjectURL) runs in a `finally` so even - * a thrown `click()` doesn't leak the DOM node or the object URL. + * The object-URL revoke is deferred to a task: `link.click()` only schedules + * the download, and revoking the URL synchronously can abort it before the + * browser reads the blob (Firefox/Safari, intermittently Chrome for larger + * blobs). */ -export function downloadJsonFile(filename: string, json: string): void { - const blob = new Blob([json], { type: "application/json" }); +export function downloadBlob(filename: string, blob: Blob): void { const url = URL.createObjectURL(blob); const anchor = document.createElement("a"); anchor.href = url; @@ -27,7 +28,44 @@ export function downloadJsonFile(filename: string, json: string): void { anchor.click(); } finally { document.body.removeChild(anchor); - URL.revokeObjectURL(url); + setTimeout(() => URL.revokeObjectURL(url), 0); + } +} + +/** Download an in-memory JSON string as `filename`. */ +export function downloadJsonFile(filename: string, json: string): void { + downloadBlob(filename, new Blob([json], { type: "application/json" })); +} + +/** + * Derive a safe suggested filename from a resource URI: the last path segment, + * stripped of control/format characters, path separators, and characters + * disallowed in filenames on common platforms. Falls back to `"download"` when + * nothing usable remains. + */ +export function fileNameFromUri(uri: string): string { + /* v8 ignore next -- String.prototype.split always returns a non-empty array, so .pop() is never undefined; the `?? ""` fallback is unreachable. */ + const tail = uri.split(/[\\/]/).pop() ?? ""; + const safe = tail + .replace(/[\p{Cc}\p{Cf}]+/gu, "") + .replace(/[\\/:*?"<>|]+/g, "_") + .trim(); + return safe.length > 0 ? safe.slice(0, 255) : "download"; +} + +/** + * Parse `url` and return it only if its scheme is `http:` or `https:`; + * otherwise null. Shared http(s)-only allowlist for opening or downloading + * server-supplied URLs. + */ +export function isHttpUrl(url: string): URL | null { + try { + const parsed = new URL(url); + return parsed.protocol === "https:" || parsed.protocol === "http:" + ? parsed + : null; + } catch { + return null; } } diff --git a/clients/web/src/lib/environmentFactory.test.ts b/clients/web/src/lib/environmentFactory.test.ts new file mode 100644 index 000000000..742b78d46 --- /dev/null +++ b/clients/web/src/lib/environmentFactory.test.ts @@ -0,0 +1,111 @@ +/** + * Tests for the browser `InspectorClientEnvironment` assembly. + * + * The three remote factories are mocked so we can (a) assert the `baseUrl` / + * `authToken` derived from `window.location` are threaded into each, and + * (b) capture the internal `fetchFn` wrapper and invoke it to prove it + * delegates to `globalThis.fetch` (exercising the arrow body that exists to + * preserve the global receiver). `BrowserNavigation` and the OAuth storage + * accessor are the real implementations. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { BrowserNavigation } from "@inspector/core/auth/browser/index.js"; +import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; + +interface CapturedOptions { + baseUrl: string; + authToken: string | undefined; + fetchFn: typeof fetch; +} + +const captured: { + transport?: CapturedOptions; + fetch?: CapturedOptions; + logger?: CapturedOptions; +} = {}; + +vi.mock("@inspector/core/mcp/remote/index.js", () => ({ + createRemoteTransport: (opts: CapturedOptions) => { + captured.transport = opts; + return { transport: true }; + }, + createRemoteFetch: (opts: CapturedOptions) => { + captured.fetch = opts; + return (async () => new Response("ok")) as unknown as typeof fetch; + }, + createRemoteLogger: (opts: CapturedOptions) => { + captured.logger = opts; + return { info: vi.fn() }; + }, +})); + +import { createWebEnvironment } from "./environmentFactory"; + +const REDIRECT: RedirectUrlProvider = { + getRedirectUrl: () => "http://localhost/callback", +}; + +describe("createWebEnvironment", () => { + beforeEach(() => { + captured.transport = undefined; + captured.fetch = undefined; + captured.logger = undefined; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("threads the window.location origin and auth token into every remote factory", () => { + const expectedBaseUrl = `${window.location.protocol}//${window.location.host}`; + const { environment, logger } = createWebEnvironment("tok-123", REDIRECT); + + for (const opts of [captured.transport, captured.fetch, captured.logger]) { + expect(opts?.baseUrl).toBe(expectedBaseUrl); + expect(opts?.authToken).toBe("tok-123"); + } + + // The returned logger is the same instance the factory produced. + expect(logger).toBe(environment.logger); + expect(environment.transport).toEqual({ transport: true }); + expect(typeof environment.fetch).toBe("function"); + }); + + it("passes an undefined auth token straight through", () => { + createWebEnvironment(undefined, REDIRECT); + expect(captured.logger?.authToken).toBeUndefined(); + }); + + it("wraps fetch so the call preserves the global receiver", async () => { + const spy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("delegated", { status: 200 })); + createWebEnvironment("tok", REDIRECT); + + // Every factory receives the identical wrapper; invoking it must delegate. + const wrapper = captured.transport!.fetchFn; + const res = await wrapper("http://example.test/x"); + expect(spy).toHaveBeenCalledWith("http://example.test/x"); + expect(res.status).toBe(200); + }); + + it("builds a BrowserNavigation and wires the OAuth storage + redirect provider", () => { + const onBeforeRedirect = vi.fn(); + const { environment } = createWebEnvironment( + "tok", + REDIRECT, + onBeforeRedirect, + ); + + const oauth = environment.oauth; + expect(oauth?.navigation).toBeInstanceOf(BrowserNavigation); + expect(oauth?.redirectUrlProvider).toBe(REDIRECT); + expect(oauth?.storage).toBeDefined(); + }); + + it("works without an onBeforeOAuthRedirect callback", () => { + const { environment } = createWebEnvironment(undefined, REDIRECT); + expect(environment.oauth?.navigation).toBeInstanceOf(BrowserNavigation); + }); +}); diff --git a/clients/web/src/lib/environmentFactory.ts b/clients/web/src/lib/environmentFactory.ts index e64cadef5..7c9497a07 100644 --- a/clients/web/src/lib/environmentFactory.ts +++ b/clients/web/src/lib/environmentFactory.ts @@ -4,11 +4,9 @@ import { createRemoteFetch, createRemoteLogger, } from "@inspector/core/mcp/remote/index.js"; -import { - getBrowserOAuthStorage, - BrowserNavigation, -} from "@inspector/core/auth/browser/index.js"; +import { BrowserNavigation } from "@inspector/core/auth/browser/index.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; +import { getRemoteOAuthStorage } from "./remoteOAuthStorage"; export interface WebEnvironmentResult { environment: InspectorClientEnvironment; @@ -20,8 +18,11 @@ export interface WebEnvironmentResult { * - transport / fetch / logger all routed through the in-process Hono * backend at `window.location.origin` (the `clients/web/server` * dev-backend wires this in `/api/*`). - * - OAuth storage + navigation use the `BrowserOAuthStorage` (sessionStorage) - * and `BrowserNavigation` (full-page redirect) adapters. + * - OAuth storage uses `RemoteOAuthStorage` (POSTs to the same backend's + * `/api/storage/oauth`, which writes `~/.mcp-inspector/storage/oauth.json` + * mode 0600), so a token obtained in this browser session is also + * available to the CLI/TUI on the same host. Navigation still uses + * `BrowserNavigation` (full-page redirect). * * Returns both the assembled environment and the logger so callers can share * the same pino instance for any direct logging they need to do, instead of @@ -67,7 +68,7 @@ export function createWebEnvironment( }), logger, oauth: { - storage: getBrowserOAuthStorage(), + storage: getRemoteOAuthStorage(baseUrl, authToken, fetchFn), navigation: new BrowserNavigation(undefined, onBeforeOAuthRedirect), redirectUrlProvider, }, diff --git a/clients/web/src/lib/remoteOAuthStorage.test.ts b/clients/web/src/lib/remoteOAuthStorage.test.ts new file mode 100644 index 000000000..c1d57dbe5 --- /dev/null +++ b/clients/web/src/lib/remoteOAuthStorage.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, vi } from "vitest"; +import { RemoteOAuthStorage } from "@inspector/core/auth/remote/index.js"; +import { + getRemoteOAuthStorage, + getWebOAuthBaseUrl, + webOAuthFetch, +} from "./remoteOAuthStorage"; + +const NOOP_FETCH = vi.fn( + async () => + new Response(JSON.stringify({ state: { servers: {} }, version: 0 }), { + status: 200, + }), +) as unknown as typeof fetch; + +describe("remoteOAuthStorage", () => { + it("getWebOAuthBaseUrl derives the backend origin from window.location", () => { + expect(getWebOAuthBaseUrl()).toBe( + `${window.location.protocol}//${window.location.host}`, + ); + }); + + it("returns a RemoteOAuthStorage instance", () => { + const storage = getRemoteOAuthStorage( + "http://a.example", + undefined, + NOOP_FETCH, + ); + expect(storage).toBeInstanceOf(RemoteOAuthStorage); + }); + + it("memoizes one instance per {baseUrl, authToken}", () => { + const a1 = getRemoteOAuthStorage("http://memo.example", "tok", NOOP_FETCH); + const a2 = getRemoteOAuthStorage("http://memo.example", "tok", NOOP_FETCH); + expect(a1).toBe(a2); + + // A different auth token is a distinct key → distinct instance. + const b = getRemoteOAuthStorage("http://memo.example", "other", NOOP_FETCH); + expect(b).not.toBe(a1); + + // A different base URL is a distinct key → distinct instance. + const c = getRemoteOAuthStorage("http://memo2.example", "tok", NOOP_FETCH); + expect(c).not.toBe(a1); + }); + + it("treats an undefined authToken as its own stable key", () => { + const a = getRemoteOAuthStorage("http://undef.example", undefined); + const b = getRemoteOAuthStorage("http://undef.example", undefined); + expect(a).toBe(b); + }); + + it("webOAuthFetch delegates to globalThis.fetch preserving the receiver", async () => { + const spy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("ok", { status: 200 })); + try { + const res = await webOAuthFetch("http://x.example"); + expect(spy).toHaveBeenCalledWith("http://x.example"); + expect(res.status).toBe(200); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/clients/web/src/lib/remoteOAuthStorage.ts b/clients/web/src/lib/remoteOAuthStorage.ts new file mode 100644 index 000000000..d0497c376 --- /dev/null +++ b/clients/web/src/lib/remoteOAuthStorage.ts @@ -0,0 +1,56 @@ +import { RemoteOAuthStorage } from "@inspector/core/auth/remote/index.js"; + +/** + * Shared `RemoteOAuthStorage` accessor for the web client. + * + * The browser's OAuth state is persisted through the in-process Hono backend + * (`POST /api/storage/oauth` → `~/.mcp-inspector/storage/oauth.json`, mode + * 0600) rather than `sessionStorage`, so a credential obtained in the browser + * is the same blob the TUI/CLI read on the same host (web ⇄ TUI ⇄ CLI parity). + * + * One instance per `{baseUrl, authToken}`. The environment factory is called + * per-connect (a fresh `InspectorClient` is built for each server switch), but + * the OAuth blob on disk is a single shared resource: a stale instance's + * whole-blob POST could clobber a write made by a newer one. Memoizing also + * avoids re-hydrating the same file on every connect, and lets the connection + * path, the EMA IdP hook, and the per-server "clear OAuth" action all share + * one in-memory view of the store. + */ +const oauthStorageCache = new Map(); + +/** Backend origin the web client talks to (same origin that serves the SPA). */ +export function getWebOAuthBaseUrl(): string { + return `${window.location.protocol}//${window.location.host}`; +} + +/** + * `window.fetch` loses its `this` binding when extracted, raising "Illegal + * invocation"; wrap so the call site preserves the global receiver. + */ +export const webOAuthFetch: typeof fetch = (...args) => + globalThis.fetch(...args); + +/** + * Memoized `RemoteOAuthStorage` for the given backend + auth token. + * + * The cache key is intentionally `{baseUrl, authToken}` only — `fetchFn` is NOT + * part of it. Whichever call constructs an instance first wins, so a later + * caller's `fetchFn` is ignored. This is safe today because every call site + * either omits `fetchFn` (defaulting to {@link webOAuthFetch}) or passes an + * `environmentFactory` wrapper that is functionally identical (`globalThis.fetch`), + * so the shared-instance goal (one whole-blob writer per backend) matters more + * than which wrapper is used. A future caller needing a genuinely different + * transport must key on it (or bypass the cache). + */ +export function getRemoteOAuthStorage( + baseUrl: string, + authToken: string | undefined, + fetchFn: typeof fetch = webOAuthFetch, +): RemoteOAuthStorage { + const key = `${baseUrl} ${authToken ?? ""}`; + const cached = oauthStorageCache.get(key); + if (cached) return cached; + const storage = new RemoteOAuthStorage({ baseUrl, authToken, fetchFn }); + oauthStorageCache.set(key, storage); + return storage; +} diff --git a/clients/web/src/lib/sandbox-csp.test.ts b/clients/web/src/lib/sandbox-csp.test.ts new file mode 100644 index 000000000..3cd471c2c --- /dev/null +++ b/clients/web/src/lib/sandbox-csp.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, it, vi } from "vitest"; +import { + SAFE_CSP_SOURCE, + approveCspSources, + buildSandboxCspPolicy, + escapeHtmlAttr, + wrapSandboxedHtml, +} from "./sandbox-csp"; + +describe("SAFE_CSP_SOURCE", () => { + it.each([ + "*", + "https://api.example.com", + "https://api.example.com:8443", + "https://api.example.com/path/seg", + "wss://realtime.service.com", + "*.example.com", + "https://*.cloudflare.com", + "data:", + "blob:", + "example.com", + // Schemes are case-insensitive per the URL spec, so a capitalized or + // mixed-case scheme is still a valid source (over-rejecting it would + // silently drop a restriction a server asked for). + "HTTPS://api.example.com", + "WSS://realtime.service.com", + "Https://*.cloudflare.com", + ])("accepts %s", (s) => { + expect(SAFE_CSP_SOURCE.test(s)).toBe(true); + }); + + it.each([ + "https://a.com; script-src *", + 'https://a.com" onload=', + "https://a.com>", + "app`; + const wrapped = wrapSandboxedHtml(evil, "default-src 'none'"); + // The first in the output is OURS; the untrusted token only + // appears after . + const ourHead = wrapped.indexOf(""); + const body = wrapped.indexOf(""); + const evilHead = wrapped.indexOf("", ourHead + 1); + expect(ourHead).toBeLessThan(body); + expect(evilHead).toBeGreaterThan(body); + }); + + it("does not introduce a host-authored