diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0dcb32c9..2cc3a90f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -106,8 +106,9 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency. - **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged. - **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke. -- **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run local:gate` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check. +- **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run local:gate` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check, as is **`smoke:web:tabs`** (#2148), the connected-flow smoke for the Tools / Resources / Prompts tabs: those are ordinary React and Mantine rather than the sandbox, so it is deliberately **outside `ENGINE_SMOKES`** and pins `chromium` on **both** halves of its npm script — install and launch. Flag a supposedly single-engine script that reads ambient `SMOKE_BROWSER` (an ambient value must not be able to redirect a gated tier), or one whose install and launch engines can disagree. +- **A browser-driven smoke asserts `data-*` attributes, never visible copy** (#2148). Each screen exposes a readiness contract — `data-app-status` for Apps, and `data-tool-count` / `data-call-status` and their Resources / Prompts equivalents for the core tabs, tabulated in `clients/web/README.md`. Copy-based waits fail on the next rewording, and as an opaque timeout rather than a mismatch. Two things to flag: a new smoke matching on text or a heading, and an assertion that stops at an **RPC status** where the issue asked for something to *render* — `data-read-status="ok"` flips the moment `resources/read` resolves and stays true with the preview panel deleted, so the rendered-panel testid must be asserted alongside it. Each attribute name is pinned by a screen unit test so a rename fails there, not in a five-minute gate. Shared drive steps belong in `scripts/lib/` (the deep link lives in `deep-link-connect.mjs`): `parseDeepLink` *ignores* a link it cannot validate rather than reporting one, so a copied-and-drifted link fails as a silent connect timeout. - **Build output is never a gate target.** Lint, format, and typecheck read first-party source only; everything a build writes (`clients/*/build`, `clients/web/dist`, `storybook-static`, `coverage`, `test-servers/build`, `core/**/{build,dist}`, `*.tsbuildinfo`) stays out via each scope's `globalIgnores`, `format` globs, and tsconfig `include`. Gating generated code reports defects in vendored third-party source that nobody can fix, and a rule promotion turns that warning into a `validate` failure (#2043). Flag a PR that adds a build location without ignoring it in the same change, that widens an ignore to silence a finding in first-party code, or that adds a build directory to a tsconfig `include` to make a generated `.d.ts` resolve. Note the coverage guards don't catch this — they assert source is _covered_, not that output is _excluded_. - **Lint has no warning tier.** Every `lint` script runs `--max-warnings 0`, so a warning fails `validate` exactly as an error does (#2085) — a `warn`-level `react-hooks/exhaustive-deps` finding otherwise let a stale-closure bug pass the pre-push gate and reach review. Flag a PR that silences a finding to make the gate pass (widening a `globalIgnores`, dropping a rule, or an inline disable with no justification comment); the fix is the defect, not the message. A rule meant to be enforced should be set to `error` rather than left at `warn` and carried by the flag. - **Every PR references an issue**, first body line `Closes #`. diff --git a/AGENTS.md b/AGENTS.md index 115e6546..6de36926 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -972,6 +972,8 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension, and since #2003 it owns it *properly* — it drives this smoke's first phase against the installed tarball rather than only asserting the proxy page exists. The flow therefore lives in `scripts/lib/mcp-app-flow.mjs` (deep-link construction and the staged assertions; the browser launch and the page-diagnostics split live one level down, in `scripts/lib/headless-browser.mjs`) and is **shared, not copied**: two copies of the deep-link shape would drift, and a drifted link fails as a silent timeout rather than a mismatch. Keep both consumers — `pack:verify` is network-bound and local/release-only, so it does not run in `npm run local:gate`, where this smoke is the only thing exercising the App path at all, and the **`_meta.ui.domain` phase (#2056) is this smoke's alone**: the dedicated app origin is served by the same runner the packaging checks already cover, so driving it a second time from the tarball would cost a browser launch for no new packaging signal. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. +- `smoke:web:tabs` (`scripts/smoke-web-tabs.mjs`, #2148) is the connected-flow smoke for the **core tabs** — the surfaces the App smokes never touch. Before it, the web client's only connected smoke was the Apps tab, so Tools, Resources, and Prompts — what nearly every session actually uses — were unexercised through the prod bundle: unit tests render a screen with fixture props, Storybook does the same in a browser, and the integration project drives `InspectorClient` with no browser at all, so a regression living only in the built bundle or in the prod Hono backend's wiring of a tab passed all of them. It boots the same prod `--web` server, connects once through the shared deep link against `web-tabs-http.json`, and drives three tabs on **one** browser launch (launch and boot dominate the cost; `npm run local:gate` is already several minutes): Tools runs `list_items` and asserts the `structuredContent` section rendered (#1908 — a panel showing only the `content[]` summary looks right while dropping the payload), Resources reads `events` with both list counts asserted separately (`resources/list` and `resources/templates/list` are different calls and either can fail alone), and Prompts selects the argument-less `simple_prompt`, which auto-fetches. Three things worth knowing: every assertion is on a **`data-*` attribute**, never on copy — the contract is tabulated in [clients/web/README.md](clients/web/README.md#core-tab-automation-contract) and each attribute name is pinned by a screen unit test, so a rename fails loudly there rather than as an opaque 45s timeout here; it is **Chromium-only and deliberately not in `ENGINE_SMOKES`**, because the engine tiers exist for the MCP Apps sandbox's genuinely divergent primitives and these tabs are ordinary React and Mantine, so a second Firefox pass would double the cost for coverage that is not engine-sensitive; and Network / Protocol / Logs / Tasks are **not** covered — each needs its own config and more waiting, and that remains a stated gap rather than an oversight. +- **The deep-link connect step is shared, not copied** (`scripts/lib/deep-link-connect.mjs`, #2148). Every browser smoke that touches a server begins with the same `?serverUrl=…&autoConnect=` link, the `data-deeplink` token gate, and the `data-status="connected"` wait; `driveAppFlow` had it inline and `smoke:web:elicit` carried a second hand-rolled copy. Sharing matters more than the line count here: `parseDeepLink` **ignores** a link it cannot validate rather than reporting one, so a drifted copy fails as an opaque connect timeout in whichever smoke was not updated, not as a mismatch. Its failure branches are unit-tested against a stand-in `page` (`deep-link-connect.test.mjs`), since every consumer only ever drives the happy path. - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run local:gate` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. - `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 ` **under a pseudoterminal**, asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then waits `SMOKE_TUI_SURVIVE_MS` (default 2s) and asserts it is **still running** before SIGTERMing it — a shallow boot/render/survival check, not full interaction. Both rebuild `test-servers/build` on **every run** — once per process, whether or not it already exists (#2111): presence is not freshness, and a smoke driving a stale fixture reports a product failure rather than a staleness one. diff --git a/README.md b/README.md index db2642a3..6b4d5b44 100644 --- a/README.md +++ b/README.md @@ -508,7 +508,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | -| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. All three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. Those three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). A fourth, **`smoke:web:tabs`**, drives the core tabs against one server on one browser launch — Tools runs a tool and asserts its `structuredContent` section, Resources reads a resource with templates listed, Prompts fetches one ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)) — asserting the `data-*` contract in [the web README](./clients/web/README.md#core-tab-automation-contract) rather than visible copy. It is **Chromium-only**: those tabs are ordinary React and Mantine, so unlike the sandbox they are not engine-sensitive. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | diff --git a/clients/web/README.md b/clients/web/README.md index f233141f..372c960a 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -91,6 +91,36 @@ The top-level `src/types/` is a separate sibling — ambient `.d.ts` module stub Nothing _enforces_ the boundary — no path alias keys off it, and the coverage `include` in `vite.config.ts` lists both directories, so a move between them is coverage-neutral. It's a human-legible import-time signal. See [`AGENTS.md`](../../AGENTS.md) for the full rule (including the whitelist caveat — a module placed outside `components`/`lib`/`utils`/`server` falls out of the ≥90 gate). +## Core tab automation contract + +The Tools, Resources, and Prompts screens each expose a `data-testid` plus a +small set of `data-*` attributes, so a headless driver can `waitForSelector` on +a deterministic signal rather than on visible copy. `scripts/smoke-web-tabs.mjs` +drives all three against `test-servers/configs/web-tabs-http.json` ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)). +Treat them as a public contract, for the same reason as the Apps ones below: + +| Attribute | Where | Meaning | +| ------------------------------------ | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `data-testid="tools-screen"` | Tools screen root | The element carrying the two attributes below. | +| `data-tool-count` | on `tools-screen` | How many tools the list holds. `0` is a *populated screen with an empty list* — distinct from the screen being absent, which is what a driver waits out. | +| `data-call-status` | on `tools-screen` | `idle` → `pending` → `ok` / `error` for the current `tools/call`. Always a value; an absent call state reads `idle`, never empty. | +| `data-testid="structured-output"` | result panel | The `structuredContent` section of a tool result ([#1908](https://github.com/modelcontextprotocol/inspector/issues/1908)). Absent when a result carries none. | +| `data-testid="resources-screen"` | Resources root | The element carrying the three attributes below. | +| `data-resource-count` | on `resources-screen` | Entries from `resources/list`. | +| `data-template-count` | on `resources-screen` | Entries from `resources/templates/list` — a **separate** call that can fail on its own, so it is reported separately. | +| `data-read-status` | on `resources-screen` | `idle` → `pending` → `ok` / `error` for the current `resources/read`. | +| `data-testid="resource-preview"` | preview panel | The read resource's **rendered** contents. Assert this *and* `data-read-status`: the status flips as soon as the RPC resolves, so it alone stays true with the panel removed. | +| `data-testid="prompts-screen"` | Prompts root | The element carrying the two attributes below. | +| `data-prompt-count` | on `prompts-screen`| Entries from `prompts/list`. | +| `data-get-status` | on `prompts-screen`| `idle` → `pending` → `ok` / `error` for the current `prompts/get`. | +| `data-testid="prompt-messages"` | messages panel | The fetched prompt's **rendered** messages — the `prompts/get` counterpart of `resource-preview`, and asserted alongside `data-get-status` for the same reason. | + +Why attributes rather than text: a smoke that waited on a label fails the next +time the label is reworded, which is noise rather than signal — and it fails as +an opaque timeout, because there is nothing to compare against. The screen tests +pin each attribute name for the same reason: a rename should fail loudly in a +unit test, not silently in a five-minute gate. + ## MCP Apps screen automation contract The Apps screen exposes a small, stable set of `data-testid` / `data-*` attributes so an automated driver (deep-link auto-open, CI review harness) can `waitForSelector` on a deterministic signal instead of sleeping. Treat these as a public contract — drivers depend on them staying stable: diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index b7108b27..8b5d849c 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -5,7 +5,6 @@ import { useMemo, useRef, useState, - useSyncExternalStore, } from "react"; import { Box, Text } from "@mantine/core"; import { notifications } from "@mantine/notifications"; @@ -14,7 +13,6 @@ import type { ElicitResult, InitializeResult, LoggingLevel, - Resource, Tool, } from "@modelcontextprotocol/client"; import { InspectorClient } from "@inspector/core/mcp/index.js"; @@ -77,6 +75,7 @@ import { type SetupClientForServer, } from "./hooks/useOAuthRecovery"; import { useInspectorStores } from "./hooks/useInspectorStores"; +import { useMcpApps } from "./hooks/useMcpApps"; import { useExportActions } from "./hooks/useExportActions"; import { useProgressToasts } from "./hooks/useProgressToasts"; import { useTaskToasts } from "./hooks/useTaskToasts"; @@ -92,14 +91,7 @@ import type { import type { GetPromptState } from "./components/screens/PromptsScreen/PromptsScreen"; import type { ReadResourceState } from "./components/screens/ResourcesScreen/ResourcesScreen"; import { clearScrollMemory } from "./hooks/useScrollMemory"; -import type { AppRendererHandle } from "./components/elements/AppRenderer/AppRenderer"; -import { createAppBridgeFactory } from "./components/elements/AppRenderer/createAppBridgeFactory"; -import { publishAppDocument } from "./lib/publishAppDocument"; import { AppElicitationHost } from "./components/elements/AppElicitation/AppElicitationHost"; -import { - AppElicitationController, - type AppElicitationSession, -} from "./lib/appElicitationController"; import type { LogEntryData } from "./components/elements/LogEntry/LogEntry"; import { ServerConfigModal, @@ -251,12 +243,6 @@ function App() { const [inspectorClient, setInspectorClient] = useState(null); - // MCP Apps runtime wiring. `sandboxUrl` is the inspector's sandbox-proxy page - // (the trusted outer iframe); `appRendererRef` lets the app handlers push tool - // input/result into the running app and tear it down. The bridge factory wraps - // the active client's underlying SDK client so the running view can call the - // server, and reads the tool's UI resource into the sandbox on handshake. - const appRendererRef = useRef(null); const configBaseUrl = typeof window !== "undefined" ? window.location.origin : "http://localhost"; // One `GET /api/config` fetch recovers every static payload field the app @@ -296,81 +282,6 @@ function App() { }); }, [configBaseUrl]); - // The `resources/list` entries, read lazily by the App bridge factories below. - // ext-apps treats a listing entry's `_meta.ui` as the static default for its - // UI resource (a read content item's own `_meta.ui` wins), so the sandbox CSP - // has to be able to see it. A ref rather than a dependency: `resources` is - // derived further down this component, and the factories only read it inside - // an async sandboxready handler, long after any render that produced it. - const listedResourcesRef = useRef([]); - // Best-effort by construction: this reads the list as it stands when the app - // opens, and does not distinguish "no entry for this URI" from "the list - // hasn't arrived yet". Both yield no hints, which is the same outcome as - // having no listing carrier at all. Blocking the render on list readiness - // instead would mean waiting on a request that, for a server advertising no - // `resources` capability or whose list errored, never resolves — trading a - // missing default for an app that never renders. - const getListedResourceMeta = useCallback( - (uri: string) => - listedResourcesRef.current.find((r) => r.uri === uri)?._meta, - [], - ); - - // `_meta.ui.domain` support (#2056): hand a wrapped app document to the - // backend so it can serve it from a dedicated origin, giving the app's - // requests a real `Origin`. Resolves `null` on any backend that can't, and - // the factory then renders the app the default (opaque-origin) way. - const publishDocument = useCallback( - (doc: { html: string; csp?: string }) => - publishAppDocument(doc, { - baseUrl: configBaseUrl, - authToken: getAuthToken(), - }), - [configBaseUrl], - ); - - const sandboxBridgeFactory = useMemo( - () => - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - createAppBridgeFactory({ - publishAppDocument: publishDocument, - getClient: () => inspectorClient?.getAppRendererClient() ?? null, - getListedResourceMeta, - readResource: async (uri) => { - if (!inspectorClient) throw new Error("No MCP client connected."); - const invocation = await inspectorClient.readResource(uri); - return invocation.result; - }, - // The bridge's sandboxready handler reads + posts the UI resource - // inside a detached async block; without this hook a 404 / malformed - // resource is console.error-only and the user stares at a blank - // frame. Surface it as a toast. The renderer separately drives - // `data-app-status` so an automated driver can time out on - // never-reaching-"ready" and read the toast. - onResourceError: (err) => { - notifications.show({ - title: "App resource failed to load", - message: err.message, - color: "red", - }); - }, - }), - [inspectorClient, getListedResourceMeta, publishDocument], - ); - - // App-rendered form elicitations (#1854). The controller is created once and - // handed to every InspectorClient at construction — its `render` is what opts - // this client into advertising the nested MCP Apps `elicitation` capability, - // which is why only the web client (the one with a sandbox) claims it. - const appElicitationControllerRef = useRef(null); - appElicitationControllerRef.current ??= new AppElicitationController(); - const appElicitationController = appElicitationControllerRef.current; - // The window onto the controller for the CURRENT client. Closing it when the - // client is replaced both rejects that connection's queued requests and - // refuses any it enqueues during its own (asynchronous) teardown — a late - // entry would otherwise be rendered by a factory bound to the replacement - // client, i.e. read and answered through a different server. - const appElicitationSessionRef = useRef(null); // `setupClientForServer` is synchronous and memoized, so a caller that // awaited the config would still resume with the `sandboxUrl` captured by the // render it STARTED in — undefined, on the very load this matters for. The @@ -398,66 +309,6 @@ function App() { useEffect(() => { if (!initialConfigLoading) initialConfigSettledRef.current?.resolve(); }, [initialConfigLoading]); - const appElicitations = useSyncExternalStore( - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - appElicitationController.subscribe, - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - appElicitationController.getEntries, - ); - // A SECOND factory, differing from `sandboxBridgeFactory` only in that it - // advertises `hostCapabilities.elicitation`. An App-tool frame is never handed - // an elicitation, so telling those apps otherwise would be a false claim. - const elicitationBridgeFactory = useMemo( - () => - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - createAppBridgeFactory({ - advertiseElicitation: true, - publishAppDocument: publishDocument, - getClient: () => inspectorClient?.getAppRendererClient() ?? null, - getListedResourceMeta, - readResource: async (uri) => { - if (!inspectorClient) throw new Error("No MCP client connected."); - const invocation = await inspectorClient.readResource(uri); - return invocation.result; - }, - // Unlike the Apps tab there is no persistent surface to show the - // failure on — the modal is about to be replaced by the native form — - // so the toast is the only place the user learns why. - onResourceError: (err) => { - notifications.show({ - title: "Elicitation app failed to load", - message: err.message, - color: "red", - }); - }, - }), - [inspectorClient, getListedResourceMeta, publishDocument], - ); - /** - * Close the previous client's session and open one for the client being - * constructed. Synchronous, and called at construction, so the swap itself is - * the moment ownership changes hands. - */ - const newAppElicitationSession = useCallback(() => { - appElicitationSessionRef.current?.close( - new Error("Connection replaced before the app answered"), - ); - const session = appElicitationController.openSession(); - appElicitationSessionRef.current = session; - return session; - }, [appElicitationController]); - const handleAppElicitationSettle = useCallback( - (requestId: string, result: ElicitResult) => { - appElicitationController.settle(requestId, result); - }, - [appElicitationController], - ); - const handleAppElicitationFail = useCallback( - (requestId: string, error: Error) => { - appElicitationController.fail(requestId, error); - }, - [appElicitationController], - ); // Optimistic log level — `logging/setLevel` has no echo notification, so // the parent keeps the current value locally. @@ -695,15 +546,20 @@ function App() { const tools = toolsPagination.items; const prompts = promptsPagination.items; const resources = resourcesPagination.items; - // Whatever the resource list currently holds — every page in the default - // aggregate mode, only the pages fetched so far under `paginatedLists`. The - // listing is a documented *default* that a read content item overrides, and - // an app whose entry sits on an unfetched page simply falls back to no hints - // (`connect-src 'none'`), exactly as before this wiring existed. Walking the - // whole list to close that would issue the very requests the user opted out - // of by turning pagination on, so the setting wins. - // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) - listedResourcesRef.current = resources; + + // MCP Apps runtime wiring (#2156): the sandbox bridge factories, the renderer + // handle, and the app-rendered elicitation controller. Called here rather + // than beside the other client-scoped hooks above because it reads the + // resource listing, which is only derived at this point. + const { + appRendererRef, + sandboxBridgeFactory, + elicitationBridgeFactory, + appElicitations, + newAppElicitationSession, + handleAppElicitationSettle, + handleAppElicitationFail, + } = useMcpApps({ inspectorClient, configBaseUrl, resources }); // Fold the transport errors the SDK throws rather than delivers (e.g. -32601 // on HTTP 404) onto their still-pending Protocol requests, by correlating with @@ -1882,12 +1738,13 @@ function App() { onAppError, activeServerId, handleCommandScopedAuthRecovery, + appRendererRef, ], ); const onCloseApp = useCallback(() => { void appRendererRef.current?.teardown(); - }, []); + }, [appRendererRef]); const onGetPrompt = useCallback( async (name: string, args: Record) => { diff --git a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx index 23364e8b..d359b9ba 100644 --- a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx +++ b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.test.tsx @@ -64,3 +64,14 @@ describe("PromptMessagesDisplay", () => { ).not.toBeInTheDocument(); }); }); + +// The `data-testid` the headless tab smoke waits on (#2148). It asserts the +// messages RENDERED, which `data-get-status="ok"` alone does not — that flips +// as soon as `prompts/get` resolves. Pinned here so a rename fails loudly in a +// unit test rather than as an opaque timeout in a five-minute gate. +describe("automation contract (#2148)", () => { + it("exposes a stable data-testid on the panel", () => { + renderWithMantine(); + expect(screen.getByTestId("prompt-messages")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx index 3947d6d8..9bff4a72 100644 --- a/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx +++ b/clients/web/src/components/groups/PromptMessagesDisplay/PromptMessagesDisplay.tsx @@ -67,7 +67,10 @@ export function PromptMessagesDisplay({ onClose, }: PromptMessagesDisplayProps) { return ( - + // `data-testid` so a driver can wait on the messages having actually + // RENDERED, not merely on `prompts/get` having returned (#2148) — see the + // ResourcePreviewPanel counterpart for why the RPC status is not enough. + {onClose && ( diff --git a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx index 35cafff8..b8283ed9 100644 --- a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx +++ b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.test.tsx @@ -566,3 +566,22 @@ describe("ResourcePreviewPanel", () => { }); }); }); + +// The `data-testid` the headless tab smoke waits on (#2148) — see the +// PromptMessagesDisplay counterpart: `data-read-status="ok"` flips as soon as +// `resources/read` resolves, so only this proves the preview rendered. +describe("automation contract (#2148)", () => { + it("exposes a stable data-testid on the panel", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("resource-preview")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx index 39630e89..5988ed0d 100644 --- a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx +++ b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx @@ -222,7 +222,12 @@ export function ResourcePreviewPanel({ const sourceToggleable = isSourceToggleable(mimeType); return ( - + // `data-testid` so a driver can wait on the preview having actually + // RENDERED, not merely on `resources/read` having returned (#2148). The + // read status flips as soon as the RPC resolves, so a smoke stopping there + // would stay green with this panel removed. A `data-*` attribute is not + // part of a component's typed props, so it is passed at the call site. + {onClose && ( diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx index abdaf91e..dd23d489 100644 --- a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx @@ -85,3 +85,15 @@ describe("StructuredOutputPanel", () => { expect(screen.getByTestId("code-highlight")).toHaveTextContent("{}"); }); }); + +// The `data-testid` the headless tab smoke waits on (#2148). Asserting the +// section by its heading copy would make the smoke fail on a rewording, so the +// smoke keys off this attribute — pinned here so a rename fails loudly. +describe("automation contract (#2148)", () => { + it("exposes a stable data-testid on the section", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("structured-output")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx index 0f088e97..9f90bff0 100644 --- a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx @@ -72,7 +72,10 @@ export function StructuredOutputPanel({ const [expanded, setExpanded] = useState(defaultExpanded); return ( - + // `data-testid` so the headless tab smoke (#2148) can assert this section + // rendered without matching on its heading copy. A `data-*` attribute is + // not part of a component's typed props, so it is passed at the call site. + Structured Output diff --git a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.test.tsx b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.test.tsx index bbb96910..efe681c0 100644 --- a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.test.tsx +++ b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.test.tsx @@ -391,3 +391,34 @@ describe("PromptsScreen", () => { }); }); }); + +// See the ToolsScreen counterpart: the `data-*` readiness contract the headless +// tab smoke drives (#2148), pinned here so a rename fails loudly. +describe("automation contract (#2148)", () => { + it("reports the prompt count and an idle get status", () => { + renderWithMantine(); + const root = screen.getByTestId("prompts-screen"); + expect(root).toHaveAttribute( + "data-prompt-count", + String(promptsWithArgs.length), + ); + expect(root).toHaveAttribute("data-get-status", "idle"); + }); + + it("tracks the get status through to a result", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("prompts-screen")).toHaveAttribute( + "data-get-status", + "ok", + ); + }); +}); diff --git a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx index 8b491cc8..1a83382f 100644 --- a/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx +++ b/clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx @@ -274,7 +274,13 @@ export function PromptsScreen({ } return ( - + // `data-*` readiness contract for the headless tab smoke (#2148); see + // clients/web/README.md#core-tab-automation-contract. + { ).not.toBeInTheDocument(); }); }); + +// See the ToolsScreen counterpart: the `data-*` readiness contract the headless +// tab smoke drives (#2148), pinned here so a rename fails loudly. +describe("automation contract (#2148)", () => { + it("reports the resource and template counts separately", () => { + renderWithMantine(); + const root = screen.getByTestId("resources-screen"); + // Two attributes rather than one: `resources/list` and + // `resources/templates/list` are separate calls and either can fail alone. + expect(root).toHaveAttribute( + "data-resource-count", + String(resources.length), + ); + expect(root).toHaveAttribute( + "data-template-count", + String(templates.length), + ); + expect(root).toHaveAttribute("data-read-status", "idle"); + }); + + it("tracks the read status through to a result", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("resources-screen")).toHaveAttribute( + "data-read-status", + "ok", + ); + }); +}); diff --git a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx index 15c4a838..1fdb1862 100644 --- a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx +++ b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx @@ -319,7 +319,14 @@ export function ResourcesScreen({ } return ( - + // `data-*` readiness contract for the headless tab smoke (#2148); see + // clients/web/README.md#core-tab-automation-contract. + { expect(screen.getByText("invalid_header_tool")).toBeInTheDocument(); }); }); + +// The `data-*` readiness contract the headless tab smoke drives (#2148). It is +// a documented public contract (clients/web/README.md), and a smoke asserting +// it fails as an opaque 45s timeout rather than a mismatch — so the attribute +// names are pinned here, where a rename fails loudly instead. +describe("automation contract (#2148)", () => { + it("reports the tool count and an idle call status", () => { + renderWithMantine(); + const root = screen.getByTestId("tools-screen"); + expect(root).toHaveAttribute("data-tool-count", String(tools.length)); + // Absent call state reads `idle`, not empty: the smoke waits on a value. + expect(root).toHaveAttribute("data-call-status", "idle"); + }); + + it("reports an empty list distinctly from a missing one", () => { + renderWithMantine(); + expect(screen.getByTestId("tools-screen")).toHaveAttribute( + "data-tool-count", + "0", + ); + }); + + it("tracks the call status through to a result", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("tools-screen")).toHaveAttribute( + "data-call-status", + "ok", + ); + }); +}); diff --git a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx index e7bb6084..38173396 100644 --- a/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx +++ b/clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx @@ -198,7 +198,15 @@ export function ToolsScreen({ }; return ( - + // `data-*` readiness contract for the headless tab smoke (#2148); see + // clients/web/README.md#core-tab-automation-contract. Attributes are not + // part of a component's typed props, so they are passed here rather than + // baked into the `ScreenLayout` constant. + ({ + notificationsMock: { show: vi.fn() }, + createFactoryMock: vi.fn(), + publishMock: vi.fn(), + }), +); + +vi.mock("@mantine/notifications", () => ({ + notifications: notificationsMock, +})); + +// The two bridge factories are what this hook is mostly *for*, and everything +// interesting about them is in the deps object it hands `createAppBridgeFactory` +// — which resource meta a URI resolves to, which client `readResource` reaches, +// which toast an error raises. Standing in for the real factory is what makes +// those readable: the real one needs a live sandbox iframe and an SDK client +// before it will even return, and would answer none of these questions. +vi.mock("../components/elements/AppRenderer/createAppBridgeFactory", () => ({ + createAppBridgeFactory: createFactoryMock, +})); + +vi.mock("../lib/publishAppDocument", () => ({ + publishAppDocument: publishMock, +})); + +vi.mock("../lib/authToken", () => ({ + getAuthToken: () => "test-token", +})); + +/** + * A distinct client identity per call. `Object.create` does not run the + * constructor, so this is a real `InspectorClient` prototype chain with no + * transport behind it — enough for a hook that only stores the reference and + * calls the two methods stubbed here. + */ +const client = (over: Record = {}): InspectorClient => + Object.assign( + Object.create(InspectorClient.prototype), + over, + ) as InspectorClient; + +const resource = (uri: string, meta?: Record): Resource => ({ + uri, + name: uri, + ...(meta && { _meta: meta }), +}); + +interface Harness { + hook: () => McpApps; + rerender: (next: { + inspectorClient?: InspectorClient | null; + configBaseUrl?: string; + resources?: Resource[]; + }) => void; +} + +function harness( + initial: { + inspectorClient?: InspectorClient | null; + configBaseUrl?: string; + resources?: Resource[]; + } = {}, +): Harness { + let latest: McpApps | undefined; + const props = (over: typeof initial) => ({ + inspectorClient: null, + configBaseUrl: "http://localhost:6274", + resources: [] as Resource[], + ...over, + }); + function Probe(p: Required) { + latest = useMcpApps(p); + return null; + } + const { rerender } = renderWithMantine(); + return { + hook: () => { + if (!latest) throw new Error("hook did not render"); + return latest; + }, + rerender: (next) => { + act(() => { + rerender(); + }); + }, + }; +} + +/** The deps the Nth `createAppBridgeFactory` call was made with. */ +const depsOf = (call: number): AppBridgeFactoryDeps => + createFactoryMock.mock.calls[call]?.[0] as AppBridgeFactoryDeps; + +/** The most recent sandbox (non-elicitation) and elicitation dep objects. */ +function latestDeps(): { + sandbox: AppBridgeFactoryDeps; + elicitation: AppBridgeFactoryDeps; +} { + const all = createFactoryMock.mock.calls.map( + (c) => c[0] as AppBridgeFactoryDeps, + ); + const sandbox = all.filter((d) => !d.advertiseElicitation).at(-1); + const elicitation = all.filter((d) => d.advertiseElicitation).at(-1); + if (!sandbox || !elicitation) throw new Error("factories were not built"); + return { sandbox, elicitation }; +} + +const elicitationRequest = ( + requestId: string, + signal: AbortSignal, +): AppElicitationRequest => ({ + requestId, + resourceUri: "ui://demo/form.html", + params: { + message: "pick one", + requestedSchema: { + type: "object", + properties: { choice: { type: "string" } }, + }, + }, + signal, +}); + +describe("useMcpApps", () => { + beforeEach(() => { + notificationsMock.show.mockReset(); + publishMock.mockReset(); + createFactoryMock.mockReset(); + // A fresh identity per call so a rebuilt factory is observable. + createFactoryMock.mockImplementation(() => vi.fn()); + }); + + it("builds one sandbox factory and one elicitation factory", () => { + const h = harness(); + expect(createFactoryMock).toHaveBeenCalledTimes(2); + expect(depsOf(0).advertiseElicitation).toBeUndefined(); + expect(depsOf(1).advertiseElicitation).toBe(true); + expect(h.hook().sandboxBridgeFactory).not.toBe( + h.hook().elicitationBridgeFactory, + ); + }); + + it("keeps both factory identities stable across an unrelated re-render", () => { + const c = client(); + const h = harness({ inspectorClient: c }); + const first = h.hook(); + h.rerender({ inspectorClient: c, resources: [resource("ui://a")] }); + expect(h.hook().sandboxBridgeFactory).toBe(first.sandboxBridgeFactory); + expect(h.hook().elicitationBridgeFactory).toBe( + first.elicitationBridgeFactory, + ); + expect(createFactoryMock).toHaveBeenCalledTimes(2); + }); + + it("rebuilds both factories when the client is replaced", () => { + const h = harness({ inspectorClient: client() }); + const first = h.hook(); + h.rerender({ inspectorClient: client() }); + expect(h.hook().sandboxBridgeFactory).not.toBe(first.sandboxBridgeFactory); + expect(h.hook().elicitationBridgeFactory).not.toBe( + first.elicitationBridgeFactory, + ); + expect(createFactoryMock).toHaveBeenCalledTimes(4); + }); + + it("resolves listed resource meta against the latest listing", () => { + const h = harness({ resources: [resource("ui://a", { ui: { csp: {} } })] }); + expect(latestDeps().sandbox.getListedResourceMeta?.("ui://a")).toEqual({ + ui: { csp: {} }, + }); + expect( + latestDeps().sandbox.getListedResourceMeta?.("ui://b"), + ).toBeUndefined(); + + // The listing is read through a ref, so a new page arriving is visible to + // the factory that was built before it without rebuilding the factory. + h.rerender({ resources: [resource("ui://b", { ui: {} })] }); + expect(latestDeps().sandbox.getListedResourceMeta?.("ui://b")).toEqual({ + ui: {}, + }); + expect( + latestDeps().sandbox.getListedResourceMeta?.("ui://a"), + ).toBeUndefined(); + }); + + it("returns undefined meta for a listing entry that carries none", () => { + harness({ resources: [resource("ui://a")] }); + expect( + latestDeps().sandbox.getListedResourceMeta?.("ui://a"), + ).toBeUndefined(); + }); + + describe("getClient", () => { + it("hands the live client's app-renderer client to both factories", () => { + const inner = { id: "sdk" }; + const c = client({ getAppRendererClient: () => inner }); + harness({ inspectorClient: c }); + expect(latestDeps().sandbox.getClient()).toBe(inner); + expect(latestDeps().elicitation.getClient()).toBe(inner); + }); + + it("is null while disconnected", () => { + harness(); + expect(latestDeps().sandbox.getClient()).toBeNull(); + expect(latestDeps().elicitation.getClient()).toBeNull(); + }); + + it("is null when the client has no app-renderer client", () => { + const c = client({ getAppRendererClient: () => undefined }); + harness({ inspectorClient: c }); + expect(latestDeps().sandbox.getClient()).toBeNull(); + expect(latestDeps().elicitation.getClient()).toBeNull(); + }); + }); + + describe("readResource", () => { + it("returns the invocation result for both factories", async () => { + const result = { contents: [] }; + const c = client({ + readResource: vi.fn().mockResolvedValue({ result }), + }); + harness({ inspectorClient: c }); + await expect(latestDeps().sandbox.readResource("ui://a")).resolves.toBe( + result, + ); + await expect( + latestDeps().elicitation.readResource("ui://a"), + ).resolves.toBe(result); + }); + + it("rejects while disconnected", async () => { + harness(); + await expect(latestDeps().sandbox.readResource("ui://a")).rejects.toThrow( + "No MCP client connected.", + ); + await expect( + latestDeps().elicitation.readResource("ui://a"), + ).rejects.toThrow("No MCP client connected."); + }); + }); + + describe("onResourceError", () => { + it("names the Apps tab for the sandbox factory", () => { + harness(); + latestDeps().sandbox.onResourceError?.(new Error("404")); + expect(notificationsMock.show).toHaveBeenCalledWith({ + title: "App resource failed to load", + message: "404", + color: "red", + }); + }); + + it("names the elicitation modal for the elicitation factory", () => { + harness(); + latestDeps().elicitation.onResourceError?.(new Error("boom")); + expect(notificationsMock.show).toHaveBeenCalledWith({ + title: "Elicitation app failed to load", + message: "boom", + color: "red", + }); + }); + }); + + describe("publishAppDocument", () => { + it("posts the document against the configured base URL", async () => { + publishMock.mockResolvedValue("http://127.0.0.1:6278/app-document/x"); + harness({ configBaseUrl: "http://127.0.0.1:6274" }); + const doc = { html: "

hi

", csp: "default-src 'none'" }; + await expect( + latestDeps().sandbox.publishAppDocument?.(doc), + ).resolves.toBe("http://127.0.0.1:6278/app-document/x"); + expect(publishMock).toHaveBeenCalledWith(doc, { + baseUrl: "http://127.0.0.1:6274", + authToken: "test-token", + }); + }); + + it("rebuilds the factories when the base URL changes", () => { + const h = harness({ configBaseUrl: "http://a" }); + const first = h.hook().sandboxBridgeFactory; + h.rerender({ configBaseUrl: "http://b" }); + expect(h.hook().sandboxBridgeFactory).not.toBe(first); + }); + }); + + describe("app elicitations", () => { + it("starts with no entries and a stable handler identity", () => { + const h = harness(); + const first = h.hook(); + expect(first.appElicitations).toEqual([]); + h.rerender({ resources: [resource("ui://a")] }); + expect(h.hook().newAppElicitationSession).toBe( + first.newAppElicitationSession, + ); + expect(h.hook().handleAppElicitationSettle).toBe( + first.handleAppElicitationSettle, + ); + expect(h.hook().handleAppElicitationFail).toBe( + first.handleAppElicitationFail, + ); + }); + + it("surfaces a queued request and settles it with the app's result", async () => { + const h = harness(); + const session = h.hook().newAppElicitationSession(); + const controller = new AbortController(); + let pending!: Promise; + act(() => { + pending = session.render(elicitationRequest("r1", controller.signal)); + }); + expect(h.hook().appElicitations).toHaveLength(1); + expect(h.hook().appElicitations[0]?.requestId).toBe("r1"); + + act(() => { + h.hook().handleAppElicitationSettle("r1", { action: "accept" }); + }); + await expect(pending).resolves.toEqual({ action: "accept" }); + expect(h.hook().appElicitations).toEqual([]); + }); + + it("fails a request back to the native form", async () => { + const h = harness(); + const session = h.hook().newAppElicitationSession(); + const controller = new AbortController(); + let pending!: Promise; + act(() => { + pending = session.render(elicitationRequest("r1", controller.signal)); + }); + act(() => { + h.hook().handleAppElicitationFail("r1", new Error("app died")); + }); + await expect(pending).rejects.toThrow("app died"); + expect(h.hook().appElicitations).toEqual([]); + }); + + it("closes the previous session when a new client is constructed", async () => { + const h = harness(); + const first = h.hook().newAppElicitationSession(); + const controller = new AbortController(); + let pending!: Promise; + act(() => { + pending = first.render(elicitationRequest("r1", controller.signal)); + }); + act(() => { + h.hook().newAppElicitationSession(); + }); + await expect(pending).rejects.toThrow( + "Connection replaced before the app answered", + ); + expect(h.hook().appElicitations).toEqual([]); + + // And the closed session refuses anything it enqueues afterwards — the + // late entry a disconnecting client can still produce. + await expect( + first.render(elicitationRequest("r2", controller.signal)), + ).rejects.toThrow("App elicitation session is closed"); + }); + + it("opens the first session with nothing to close", () => { + const h = harness(); + expect(() => h.hook().newAppElicitationSession()).not.toThrow(); + expect(h.hook().appElicitations).toEqual([]); + }); + + it("keeps one controller across re-renders", async () => { + const h = harness(); + const session = h.hook().newAppElicitationSession(); + const controller = new AbortController(); + let pending!: Promise; + act(() => { + pending = session.render(elicitationRequest("r1", controller.signal)); + }); + h.rerender({ inspectorClient: client() }); + expect(h.hook().appElicitations).toHaveLength(1); + act(() => { + h.hook().handleAppElicitationSettle("r1", { action: "cancel" }); + }); + await expect(pending).resolves.toEqual({ action: "cancel" }); + }); + }); + + it("exposes a renderer ref that starts empty and stays identical", () => { + const h = harness(); + const ref = h.hook().appRendererRef; + expect(ref.current).toBeNull(); + h.rerender({ inspectorClient: client() }); + expect(h.hook().appRendererRef).toBe(ref); + }); +}); diff --git a/clients/web/src/hooks/useMcpApps.ts b/clients/web/src/hooks/useMcpApps.ts new file mode 100644 index 00000000..e77ea305 --- /dev/null +++ b/clients/web/src/hooks/useMcpApps.ts @@ -0,0 +1,231 @@ +import { useCallback, useMemo, useRef, useSyncExternalStore } from "react"; +import type { RefObject } from "react"; +import { notifications } from "@mantine/notifications"; +import type { ElicitResult, Resource } from "@modelcontextprotocol/client"; +import type { InspectorClient } from "@inspector/core/mcp/index.js"; +import type { + AppRendererHandle, + BridgeFactory, +} from "../components/elements/AppRenderer/AppRenderer"; +import { createAppBridgeFactory } from "../components/elements/AppRenderer/createAppBridgeFactory"; +import { publishAppDocument } from "../lib/publishAppDocument"; +import { + AppElicitationController, + type AppElicitationEntry, + type AppElicitationSession, +} from "../lib/appElicitationController"; +import { getAuthToken } from "../lib/authToken"; + +export interface UseMcpAppsOptions { + /** The live client. Null while disconnected; the bridges read it lazily. */ + inspectorClient: InspectorClient | null; + /** Origin the backend's `/api/*` routes are reached on. */ + configBaseUrl: string; + /** + * Whatever the resource list currently holds — every page in the default + * aggregate mode, only the pages fetched so far under `paginatedLists`. Read + * through a ref rather than a dependency (see below), so a new array + * identity on every render costs nothing here. + */ + resources: Resource[]; +} + +export interface McpApps { + /** Pushes tool input/result into the running app, and tears it down. */ + appRendererRef: RefObject; + /** Bridge for an App-tool frame in the Apps tab. */ + sandboxBridgeFactory: BridgeFactory; + /** Bridge for an app-rendered elicitation modal (advertises `elicitation`). */ + elicitationBridgeFactory: BridgeFactory; + /** The elicitations currently awaiting an answer, for `AppElicitationHost`. */ + appElicitations: AppElicitationEntry[]; + /** + * Close the previous client's session and open one for the client being + * constructed. Synchronous, and called at construction, so the swap itself is + * the moment ownership changes hands. + */ + newAppElicitationSession: () => AppElicitationSession; + handleAppElicitationSettle: (requestId: string, result: ElicitResult) => void; + handleAppElicitationFail: (requestId: string, error: Error) => void; +} + +/** + * The MCP Apps runtime wiring — the two sandbox bridge factories, the renderer + * handle, and the app-rendered elicitation controller (#1854) — lifted out of + * `App.tsx` by the phase-2 decomposition (#2156, under #2129/#2126). + * + * Nearly a leaf: it reads the active client and the resource listing and calls + * nothing back. What keeps it one hook rather than several is that both + * factories are built from the same two lazily-read inputs, and the elicitation + * session is owned by the controller the second factory renders through. + * + * The caller still owns `sandboxUrl` and the client construction that consumes + * `newAppElicitationSession` — whether the sandbox exists decides whether the + * nested MCP Apps `elicitation` capability is advertised, and that is a + * connection-lifecycle question rather than an Apps one. + */ +export function useMcpApps({ + inspectorClient, + configBaseUrl, + resources, +}: UseMcpAppsOptions): McpApps { + // `sandboxUrl` is the inspector's sandbox-proxy page (the trusted outer + // iframe); `appRendererRef` lets the app handlers push tool input/result into + // the running app and tear it down. The bridge factory wraps the active + // client's underlying SDK client so the running view can call the server, and + // reads the tool's UI resource into the sandbox on handshake. + const appRendererRef = useRef(null); + + // The `resources/list` entries, read lazily by the App bridge factories below. + // ext-apps treats a listing entry's `_meta.ui` as the static default for its + // UI resource (a read content item's own `_meta.ui` wins), so the sandbox CSP + // has to be able to see it. A ref rather than a dependency: the factories only + // read it inside an async sandboxready handler, long after any render that + // produced the listing. + // + // The listing is a documented *default* that a read content item overrides, + // and an app whose entry sits on an unfetched page simply falls back to no + // hints (`connect-src 'none'`), exactly as before this wiring existed. + // Walking the whole list to close that would issue the very requests the user + // opted out of by turning pagination on, so the setting wins. + const listedResourcesRef = useRef([]); + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + listedResourcesRef.current = resources; + // Best-effort by construction: this reads the list as it stands when the app + // opens, and does not distinguish "no entry for this URI" from "the list + // hasn't arrived yet". Both yield no hints, which is the same outcome as + // having no listing carrier at all. Blocking the render on list readiness + // instead would mean waiting on a request that, for a server advertising no + // `resources` capability or whose list errored, never resolves — trading a + // missing default for an app that never renders. + const getListedResourceMeta = useCallback( + (uri: string) => + listedResourcesRef.current.find((r) => r.uri === uri)?._meta, + [], + ); + + // `_meta.ui.domain` support (#2056): hand a wrapped app document to the + // backend so it can serve it from a dedicated origin, giving the app's + // requests a real `Origin`. Resolves `null` on any backend that can't, and + // the factory then renders the app the default (opaque-origin) way. + const publishDocument = useCallback( + (doc: { html: string; csp?: string }) => + publishAppDocument(doc, { + baseUrl: configBaseUrl, + authToken: getAuthToken(), + }), + [configBaseUrl], + ); + + const sandboxBridgeFactory = useMemo( + () => + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + createAppBridgeFactory({ + publishAppDocument: publishDocument, + getClient: () => inspectorClient?.getAppRendererClient() ?? null, + getListedResourceMeta, + readResource: async (uri) => { + if (!inspectorClient) throw new Error("No MCP client connected."); + const invocation = await inspectorClient.readResource(uri); + return invocation.result; + }, + // The bridge's sandboxready handler reads + posts the UI resource + // inside a detached async block; without this hook a 404 / malformed + // resource is console.error-only and the user stares at a blank + // frame. Surface it as a toast. The renderer separately drives + // `data-app-status` so an automated driver can time out on + // never-reaching-"ready" and read the toast. + onResourceError: (err) => { + notifications.show({ + title: "App resource failed to load", + message: err.message, + color: "red", + }); + }, + }), + [inspectorClient, getListedResourceMeta, publishDocument], + ); + + // App-rendered form elicitations (#1854). The controller is created once and + // handed to every InspectorClient at construction — its `render` is what opts + // this client into advertising the nested MCP Apps `elicitation` capability, + // which is why only the web client (the one with a sandbox) claims it. + const appElicitationControllerRef = useRef(null); + appElicitationControllerRef.current ??= new AppElicitationController(); + const appElicitationController = appElicitationControllerRef.current; + // The window onto the controller for the CURRENT client. Closing it when the + // client is replaced both rejects that connection's queued requests and + // refuses any it enqueues during its own (asynchronous) teardown — a late + // entry would otherwise be rendered by a factory bound to the replacement + // client, i.e. read and answered through a different server. + const appElicitationSessionRef = useRef(null); + const appElicitations = useSyncExternalStore( + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + appElicitationController.subscribe, + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + appElicitationController.getEntries, + ); + + // A SECOND factory, differing from `sandboxBridgeFactory` only in that it + // advertises `hostCapabilities.elicitation`. An App-tool frame is never handed + // an elicitation, so telling those apps otherwise would be a false claim. + const elicitationBridgeFactory = useMemo( + () => + // eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161) + createAppBridgeFactory({ + advertiseElicitation: true, + publishAppDocument: publishDocument, + getClient: () => inspectorClient?.getAppRendererClient() ?? null, + getListedResourceMeta, + readResource: async (uri) => { + if (!inspectorClient) throw new Error("No MCP client connected."); + const invocation = await inspectorClient.readResource(uri); + return invocation.result; + }, + // Unlike the Apps tab there is no persistent surface to show the + // failure on — the modal is about to be replaced by the native form — + // so the toast is the only place the user learns why. + onResourceError: (err) => { + notifications.show({ + title: "Elicitation app failed to load", + message: err.message, + color: "red", + }); + }, + }), + [inspectorClient, getListedResourceMeta, publishDocument], + ); + + const newAppElicitationSession = useCallback(() => { + appElicitationSessionRef.current?.close( + new Error("Connection replaced before the app answered"), + ); + const session = appElicitationController.openSession(); + appElicitationSessionRef.current = session; + return session; + }, [appElicitationController]); + + const handleAppElicitationSettle = useCallback( + (requestId: string, result: ElicitResult) => { + appElicitationController.settle(requestId, result); + }, + [appElicitationController], + ); + + const handleAppElicitationFail = useCallback( + (requestId: string, error: Error) => { + appElicitationController.fail(requestId, error); + }, + [appElicitationController], + ); + + return { + appRendererRef, + sandboxBridgeFactory, + elicitationBridgeFactory, + appElicitations, + newAppElicitationSession, + handleAppElicitationSettle, + handleAppElicitationFail, + }; +} diff --git a/package.json b/package.json index 75d7caaf..e1c21d42 100644 --- a/package.json +++ b/package.json @@ -66,13 +66,14 @@ "coverage:tui": "cd clients/tui && npm run test:coverage", "coverage:web": "cd clients/web && npm run test:coverage", "coverage:launcher": "cd clients/launcher && npm run test:coverage", - "smoke": "npm run smoke:launcher && npm run smoke:cli && npm run smoke:tui && npm run smoke:web && npm run smoke:web:chromium", + "smoke": "npm run smoke:launcher && npm run smoke:cli && npm run smoke:tui && npm run smoke:web && npm run smoke:web:chromium && npm run smoke:web:tabs", "smoke:cli": "node scripts/smoke-cli.mjs", "smoke:tui": "node scripts/smoke-tui.mjs", "smoke:web": "node scripts/smoke-web.mjs", "smoke:web:browser": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-browser.mjs", "smoke:web:app": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-app.mjs", "smoke:web:elicit": "node scripts/install-smoke-browser.mjs && node scripts/smoke-web-elicitation.mjs", + "smoke:web:tabs": "node scripts/install-smoke-browser.mjs chromium && node scripts/smoke-web-tabs.mjs chromium", "smoke:web:engine": "node scripts/run-engine-smokes.mjs", "smoke:web:chromium": "node scripts/run-engine-smokes.mjs chromium", "smoke:web:firefox": "node scripts/run-engine-smokes.mjs firefox", diff --git a/scripts/lib/deep-link-connect.mjs b/scripts/lib/deep-link-connect.mjs new file mode 100644 index 00000000..aaa7dfe4 --- /dev/null +++ b/scripts/lib/deep-link-connect.mjs @@ -0,0 +1,87 @@ +/** + * The **connect** half of a headless web smoke: build the deep link, navigate + * to it, and wait until the Inspector reports a live connection (#2148). + * + * Every browser-driven smoke that touches a server starts the same way — the + * `?serverUrl=…&transport=http&autoConnect=` link, the `data-deeplink` + * token gate, and the `data-status="connected"` wait. `smoke:web:app` had it + * inside `driveAppFlow`, and `smoke:web:elicit` carried a second, hand-rolled + * copy; a third copy for the tab smoke is what this exists to prevent. + * + * Sharing it matters more than the line count. The deep link's shape is owned + * by clients/web/README.md#deep-link-auto-connect and enforced by + * `parseDeepLink`, which **ignores** a link it cannot validate rather than + * reporting one — so a drifted copy does not fail as a mismatch, it fails as an + * opaque 45s connect timeout in whichever smoke was not updated. One copy is + * one thing to keep right. + * + * The two gates are separate on purpose and both are worth asserting: + * + * - `data-deeplink` distinguishes "the link was rejected by the CSRF token + * gate" from "no deep link was present at all". Without it a bad token + * reads as a connect that simply never happened. + * - `data-status` is the connection itself. + * + * Takes only the slice of Playwright's `page` it uses (`goto`, `locator` → + * `waitFor`/`getAttribute`), which is what lets its failure branches be + * unit-tested against a stand-in — a smoke only ever drives its happy path. + */ + +/** + * The deep link that connects to `mcpUrl`. + * + * `autoConnect` must equal the session's `MCP_INSPECTOR_API_TOKEN`: it is the + * CSRF gate, and a link that fails it is silently ignored. Shape owned by + * clients/web/README.md#deep-link-auto-connect. + */ +export function buildConnectDeepLink({ + baseUrl, + mcpUrl, + token, + transport = "http", +}) { + return ( + `${baseUrl}/?serverUrl=${encodeURIComponent(mcpUrl)}` + + `&transport=${transport}&autoConnect=${token}` + ); +} + +/** + * Navigate to `url` and wait for a connected Inspector. + * + * `expectDeepLink: false` skips the token-gate assertion — useful when + * re-navigating an already-proven page to a second server, where a rejected + * link would fail at the connect wait anyway and the gate is not the subject. + */ +export async function connectViaDeepLink({ + page, + url, + expectDeepLink = true, + gotoTimeoutMs = 30_000, + connectTimeoutMs = 45_000, +}) { + const response = await page.goto(url, { + waitUntil: "domcontentloaded", + timeout: gotoTimeoutMs, + }); + if (!response || !response.ok()) { + throw new Error( + `GET / returned HTTP ${response ? response.status() : "no response"}`, + ); + } + + const status = page.locator('[data-testid="connection-status"]'); + if (expectDeepLink) { + await status.waitFor({ state: "attached", timeout: gotoTimeoutMs }); + const deeplink = await status.getAttribute("data-deeplink"); + if (deeplink !== "parsed") { + throw new Error( + `deep link was not accepted (data-deeplink="${deeplink}") — expected "parsed"`, + ); + } + } + + await page + .locator('[data-testid="connection-status"][data-status="connected"]') + .waitFor({ state: "attached", timeout: connectTimeoutMs }); +} diff --git a/scripts/lib/deep-link-connect.test.mjs b/scripts/lib/deep-link-connect.test.mjs new file mode 100644 index 00000000..effd1457 --- /dev/null +++ b/scripts/lib/deep-link-connect.test.mjs @@ -0,0 +1,148 @@ +/** + * Unit tests for the shared deep-link connect step (#2148). + * + * These cover what no smoke can check itself. Every consumer only ever drives + * the happy path — the page connects — so both failure branches below are dead + * code from their point of view, and the link's *shape* is asserted by nothing + * once it is built rather than inlined. Those are exactly the parts that rot + * silently: `parseDeepLink` **ignores** a link it cannot validate rather than + * reporting one, so a dropped `autoConnect` token or a mangled `serverUrl` + * produces a page that simply never connects, reported as an opaque 45s + * timeout in whichever smoke was not updated. + * + * `connectViaDeepLink` is driven against a stand-in page rather than a real + * browser — it takes only `goto` and `locator(...)` → `waitFor`/`getAttribute`, + * which is what makes that possible. + */ + +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + buildConnectDeepLink, + connectViaDeepLink, +} from "./deep-link-connect.mjs"; + +/** Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. */ +function fakePage(plan, { gotoStatus = 200 } = {}) { + const gotos = []; + return { + gotos, + goto: async (url) => { + gotos.push(url); + return { ok: () => gotoStatus === 200, status: () => gotoStatus }; + }, + locator: (selector) => { + const entry = plan[selector] ?? {}; + return { + waitFor: async () => { + if (entry.waitFails) throw new Error(`timeout: ${selector}`); + }, + getAttribute: async (name) => entry.attrs?.[name] ?? null, + }; + }, + }; +} + +const STATUS = '[data-testid="connection-status"]'; +const CONNECTED = '[data-testid="connection-status"][data-status="connected"]'; + +const happyPlan = () => ({ + [STATUS]: { attrs: { "data-deeplink": "parsed" } }, + [CONNECTED]: {}, +}); + +describe("buildConnectDeepLink", () => { + const link = buildConnectDeepLink({ + baseUrl: "http://127.0.0.1:6399", + mcpUrl: "http://127.0.0.1:3130/mcp?a=1", + token: "tok", + }); + const params = new URL(link).searchParams; + + it("percent-encodes the server URL so its own query can't leak", () => { + // Unencoded, the fixture's `?a=1` would terminate `serverUrl` and become a + // sibling param — the link would still parse, just against a different + // server than the caller asked for. + assert.ok( + link.includes(encodeURIComponent("http://127.0.0.1:3130/mcp?a=1")), + ); + assert.equal(params.get("serverUrl"), "http://127.0.0.1:3130/mcp?a=1"); + }); + + it("carries the session token on the CSRF gate", () => { + assert.equal(params.get("autoConnect"), "tok"); + }); + + it("defaults to streamable HTTP and honors an explicit transport", () => { + assert.equal(params.get("transport"), "http"); + assert.equal( + new URL( + buildConnectDeepLink({ + baseUrl: "http://x", + mcpUrl: "http://y", + token: "t", + transport: "sse", + }), + ).searchParams.get("transport"), + "sse", + ); + }); +}); + +describe("connectViaDeepLink", () => { + it("resolves once the gate passes and the status reads connected", async () => { + const page = fakePage(happyPlan()); + await connectViaDeepLink({ page, url: "http://x/" }); + assert.deepEqual(page.gotos, ["http://x/"]); + }); + + it("reports a non-200 document instead of waiting on selectors", async () => { + await assert.rejects( + connectViaDeepLink({ + page: fakePage(happyPlan(), { gotoStatus: 500 }), + url: "http://x/", + }), + /GET \/ returned HTTP 500/, + ); + }); + + it("distinguishes a rejected deep link from an absent one", async () => { + // The whole reason the gate is asserted separately: both `rejected` and + // `none` leave the connection idle, and only this attribute says which. + for (const value of ["rejected", "none"]) { + await assert.rejects( + connectViaDeepLink({ + page: fakePage({ + ...happyPlan(), + [STATUS]: { attrs: { "data-deeplink": value } }, + }), + url: "http://x/", + }), + new RegExp(`data-deeplink="${value}"`), + ); + } + }); + + it("skips the gate assertion when the caller opts out", async () => { + // A re-navigate of an already-proven page: the gate was established on the + // first navigate, and a rejected one still fails at the connect wait. + await connectViaDeepLink({ + page: fakePage({ + ...happyPlan(), + [STATUS]: { attrs: { "data-deeplink": "none" } }, + }), + url: "http://x/", + expectDeepLink: false, + }); + }); + + it("propagates a connect timeout", async () => { + await assert.rejects( + connectViaDeepLink({ + page: fakePage({ ...happyPlan(), [CONNECTED]: { waitFails: true } }), + url: "http://x/", + }), + /timeout/, + ); + }); +}); diff --git a/scripts/lib/mcp-app-flow.mjs b/scripts/lib/mcp-app-flow.mjs index 99a21e3c..4a8082ff 100644 --- a/scripts/lib/mcp-app-flow.mjs +++ b/scripts/lib/mcp-app-flow.mjs @@ -30,6 +30,10 @@ import { join } from "node:path"; import { startAnnouncedChild } from "./announced-child.mjs"; +import { + buildConnectDeepLink, + connectViaDeepLink, +} from "./deep-link-connect.mjs"; import { ensureTestServers, testServerEntryPath, @@ -116,6 +120,9 @@ export function encodeAppArgs(args) { * The deep link that connects, switches to the Apps tab, pre-selects the app * tool, and fires "Open App". `autoConnect`/`autoOpen` must equal the session * token (CSRF gate). Shape owned by clients/web/README.md#deep-link-auto-connect. + * + * The connect half comes from `deep-link-connect.mjs`, shared with every other + * browser smoke; only the three App params (#1859) are added here. */ export function buildAppDeepLink({ baseUrl, @@ -125,8 +132,8 @@ export function buildAppDeepLink({ appArgs = {}, }) { return ( - `${baseUrl}/?serverUrl=${encodeURIComponent(mcpUrl)}` + - `&transport=http&autoConnect=${token}&openApp=${appTool}` + + `${buildConnectDeepLink({ baseUrl, mcpUrl, token })}` + + `&openApp=${appTool}` + `&appArgs=${encodeAppArgs(appArgs)}&autoOpen=${token}` ); } @@ -171,32 +178,14 @@ export async function driveAppFlow({ connectTimeoutMs = 45_000, readyTimeoutMs = 45_000, }) { - const response = await page.goto(url, { - waitUntil: "domcontentloaded", - timeout: gotoTimeoutMs, + // 1-2. Navigated, the deep link accepted by the token gate, and connected. + await connectViaDeepLink({ + page, + url, + expectDeepLink, + gotoTimeoutMs, + connectTimeoutMs, }); - if (!response || !response.ok()) { - throw new Error( - `GET / returned HTTP ${response ? response.status() : "no response"}`, - ); - } - - // 1. The deep link must be accepted (not rejected by the token gate). - if (expectDeepLink) { - const status = page.locator('[data-testid="connection-status"]'); - await status.waitFor({ state: "attached", timeout: gotoTimeoutMs }); - const deeplink = await status.getAttribute("data-deeplink"); - if (deeplink !== "parsed") { - throw new Error( - `deep link was not accepted (data-deeplink="${deeplink}") — expected "parsed"`, - ); - } - } - - // 2. Connected to the test server. - await page - .locator('[data-testid="connection-status"][data-status="connected"]') - .waitFor({ state: "attached", timeout: connectTimeoutMs }); // 3. The widget rendered inside the sandbox and completed its handshake. try { diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index f022a1b8..482b8236 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -100,6 +100,14 @@ describe("findWorkflowViolations", () => { text: " - run: npm run smoke:web:chromium\n", rules: [], }, + { + // #2148's tab smoke pins Chromium on both halves of its npm script and is + // deliberately outside ENGINE_SMOKES — it is not an engine pass, and the + // guard must not read `smoke:web:` as forbidden by itself. + name: "allows the Chromium-only tab smoke", + text: " - run: npm run smoke:web:tabs\n", + rules: [], + }, { name: "allows smoke:tui, which self-skips under CI on its own", text: " - run: npm run smoke:tui\n", diff --git a/scripts/run-engine-smokes.test.mjs b/scripts/run-engine-smokes.test.mjs index d7975a15..78aff741 100644 --- a/scripts/run-engine-smokes.test.mjs +++ b/scripts/run-engine-smokes.test.mjs @@ -90,6 +90,23 @@ describe("every engine tier consumes ENGINE_SMOKES", () => { } }); + it("the Chromium-only tab smoke pins its engine on BOTH halves", () => { + // `smoke:web:tabs` is not engine-sensitive and deliberately stays out of + // ENGINE_SMOKES — but "Chromium-only" is a claim the script has to keep. + // Without the literal engine it followed ambient `SMOKE_BROWSER`, so + // `SMOKE_BROWSER=firefox npm run local:gate` would have run it under Firefox + // (Copilot, #2148). Both halves are pinned because they must AGREE: an + // install of one engine and a launch of another surfaces as a confusing + // "Executable doesn't exist" rather than as the wiring error it is. + assert.match( + scripts["smoke:web:tabs"], + /install-smoke-browser\.mjs chromium .*smoke-web-tabs\.mjs chromium$/, + ); + // And it must not be in ENGINE_SMOKES, or the Firefox tier would pick it + // up regardless of what its own script pins. + assert.ok(!ENGINE_SMOKES.includes("smoke-web-tabs.mjs")); + }); + it("each gated tier names its engine explicitly rather than reading the env", () => { // An ambient SMOKE_BROWSER must not be able to redirect a gate: without the // literal engine, `SMOKE_BROWSER=firefox npm run local:gate` would run Firefox twice diff --git a/scripts/smoke-web-elicitation.mjs b/scripts/smoke-web-elicitation.mjs index 994398fc..6cbeaf9a 100644 --- a/scripts/smoke-web-elicitation.mjs +++ b/scripts/smoke-web-elicitation.mjs @@ -66,6 +66,10 @@ import { ensureTestServers, testServerEntryPath, } from "./lib/ensure-test-servers.mjs"; +import { + buildConnectDeepLink, + connectViaDeepLink, +} from "./lib/deep-link-connect.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); @@ -176,31 +180,22 @@ async function shot(page, name) { console.log(`${LABEL} — captured ${name}.png`); } -/** Connect to `mcpUrl` through the deep link and wait for the Tools list. */ +/** + * Connect to `mcpUrl` through the deep link. + * + * Shared with every other browser smoke (#2148) rather than hand-rolled here: + * a link `parseDeepLink` cannot validate is *ignored* rather than reported, so + * a drifted copy surfaces as an opaque connect timeout instead of a mismatch. + */ async function connect(page, mcpUrl) { - const url = - `${web.baseUrl}/?serverUrl=${encodeURIComponent(mcpUrl)}` + - `&transport=http&autoConnect=${TOKEN}`; - const response = await page.goto(url, { - waitUntil: "domcontentloaded", - timeout: 30_000, + await connectViaDeepLink({ + page, + url: buildConnectDeepLink({ + baseUrl: web.baseUrl, + mcpUrl, + token: TOKEN, + }), }); - if (!response || !response.ok()) { - throw new Error( - `GET / returned HTTP ${response ? response.status() : "no response"}`, - ); - } - const status = page.locator('[data-testid="connection-status"]'); - await status.waitFor({ state: "attached", timeout: 30_000 }); - const deeplink = await status.getAttribute("data-deeplink"); - if (deeplink !== "parsed") { - throw new Error( - `deep link was not accepted (data-deeplink="${deeplink}") — expected "parsed"`, - ); - } - await page - .locator('[data-testid="connection-status"][data-status="connected"]') - .waitFor({ state: "attached", timeout: 45_000 }); } /** Select the elicitation tool in the Tools tab and run it. */ diff --git a/scripts/smoke-web-tabs.mjs b/scripts/smoke-web-tabs.mjs new file mode 100644 index 00000000..4bc30777 --- /dev/null +++ b/scripts/smoke-web-tabs.mjs @@ -0,0 +1,366 @@ +#!/usr/bin/env node +/** + * Headless-browser connected-flow smoke for the core tabs — Tools, Resources, + * Prompts (#2148). + * + * ── What was missing ──────────────────────────────────────────────────────── + * + * The web client had exactly one connected-flow smoke, and it was the Apps tab. + * `smoke:web:browser` proves the bundle boots and paints; it never connects. + * `smoke:web:app` / `smoke:web:elicit` connect, but only ever drive the Apps + * surface. So the three highest-traffic tabs — the ones nearly every session + * actually uses — were unexercised through the prod bundle end to end. + * + * Everything that *did* cover them mocks or bypasses the prod server: unit + * tests render a screen with fixture props, Storybook play functions do the + * same in a browser, and the integration project drives `InspectorClient` + * without a browser at all. A regression living only in the built bundle, or in + * the prod Hono backend's wiring of a tab, passes all of them. That is the same + * class `smoke:web:browser` was added for (#1615) and `smoke:web:app` extended + * (#1859). + * + * ── What it drives ────────────────────────────────────────────────────────── + * + * One server, one browser, one page, three tabs: + * + * 1. **Tools** — the list renders, `list_items` is selected and executed, and + * the result panel populates *including* its `structuredContent` section. + * That last part is the half #1908 fixed: a tool declaring an + * `outputSchema` returns its real payload there and the `content[]` blocks + * only summarize it, so a result panel that renders text alone looks + * correct while dropping the data. + * 2. **Resources** — the list renders, a resource is read, and the preview + * panel renders its contents. Templates are asserted present (the RFC 6570 + * fixture, #1919), which is what proves `resources/templates/list` reached + * the screen — that list is fetched by a *different* call than + * `resources/list` and can fail on its own. + * 3. **Prompts** — the list renders, `simple_prompt` is fetched (selecting an + * argument-less prompt auto-fetches), and the messages panel renders. + * + * The read/get stages assert the RPC status **and** the rendered panel, not the + * status alone. Each `data-*-status` flips the moment its call resolves, so a + * smoke stopping there would stay green with the panel deleted — which is the + * opposite of what a connected-flow smoke is for. + * + * Deliberately **one script over one browser launch** rather than one per tab: + * launching the browser and booting the server dominate the cost, and + * `npm run local:gate` is already several minutes. Measured added wall-clock is ~5s + * on a warm checkout (macOS, Chromium), the test-server build included. + * + * ── What it asserts on ────────────────────────────────────────────────────── + * + * `data-*` attributes, never copy. Each screen carries a readiness contract + * mirroring the `data-app-status` precedent — `data-testid="tools-screen"` with + * `data-tool-count` / `data-call-status`, and the Resources / Prompts + * equivalents. See clients/web/README.md#core-tab-automation-contract. A smoke + * that waited on visible text would fail the next time a label is reworded, + * which is noise rather than signal. + * + * The connect half is `lib/deep-link-connect.mjs`, shared with every other + * browser smoke — see that header for why a copied deep link fails as an opaque + * timeout rather than a mismatch. + * + * ── Scope ─────────────────────────────────────────────────────────────────── + * + * **Chromium only, and not in `ENGINE_SMOKES`.** The engine tiers exist for the + * MCP Apps sandbox, which is built out of the primitives that genuinely diverge + * between engines (`srcdoc` CSP inheritance, nested sandboxed iframes, + * cross-frame `postMessage`). These three tabs are ordinary React and Mantine; + * running them a second time under Firefox would double their cost for + * coverage that is not engine-sensitive. Cross-engine parameterization is + * #2031's third bullet and is tracked separately. + * + * The deeper tabs — Network, Protocol, Logs, Tasks — are **not** covered here. + * Each needs its own server config and more waiting, and #2148 explicitly + * allows splitting. They remain unsmoked; that is a stated gap, not an + * oversight. + * + * Expects `clients/web/dist` and `clients/launcher/build` to be built first — + * the validate / CI ordering guarantees this. `test-servers/build` is rebuilt on + * every run; see `scripts/lib/ensure-test-servers.mjs` for why presence is not + * freshness (#2111). + */ + +import { resolve } from "node:path"; +import { startProdWebServer } from "./lib/prod-web-server.mjs"; +import { stopChild } from "./lib/child-cleanup.mjs"; +import { attachPageDiagnostics, loadBrowser } from "./lib/headless-browser.mjs"; +import { resolveRequestedBrowser } from "./install-smoke-browser.mjs"; +import { + buildConnectDeepLink, + connectViaDeepLink, +} from "./lib/deep-link-connect.mjs"; +import { startMcpAppServer } from "./lib/mcp-app-flow.mjs"; + +const repoRoot = resolve(import.meta.dirname, ".."); + +// The engine comes from an explicit ARGUMENT first, and only then from +// `SMOKE_BROWSER`. That precedence is the load-bearing part: `smoke:web:tabs` +// passes `chromium` to both this and the installer, so an ambient +// `SMOKE_BROWSER=firefox npm run local:gate` cannot silently redirect a gated tier that +// claims to be Chromium-only — the same invariant `run-engine-smokes.test.mjs` +// pins for the engine tiers (Copilot, #2148). Running another engine by hand is +// still one argument away: `node scripts/smoke-web-tabs.mjs firefox`. +// +// Resolved before anything starts, so an unsupported value fails immediately +// rather than after a web server and an MCP server are up. Every message +// carries the engine, so a failure names which run it came from. +let BROWSER; +try { + BROWSER = resolveRequestedBrowser(process.argv.slice(2), process.env); +} catch (err) { + console.error( + `smoke:web:tabs FAILED — ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); +} +const LABEL = `smoke:web:tabs [${BROWSER}]`; + +const HOST = "127.0.0.1"; +// Distinct from the other web smokes (6299 / 6298 / 6297 / 6296) so a prior +// run whose port is still in TIME_WAIT can't EADDRINUSE this one. +const PORT = process.env.SMOKE_WEB_TABS_PORT ?? "6295"; +const TOKEN = "smoke-web-tabs-token"; + +/** The fixture's tool with an `outputSchema`, so its result carries + * `structuredContent` (#1908) as well as a `content[]` summary. */ +const TOOL = "list_items"; +/** The fixture's plain resource — `foobar://events`, listed as "events". */ +const RESOURCE = "events"; +/** The fixture's argument-less prompt; selecting it auto-fetches. */ +const PROMPT = "simple_prompt"; + +let mcpServer = null; +let browser = null; +const web = startProdWebServer({ + host: HOST, + port: PORT, + token: TOKEN, + label: LABEL, +}); + +async function shutdown() { + if (browser) { + try { + await browser.close(); + } catch { + // best-effort + } + browser = null; + } + await web.stop(); + if (mcpServer) { + const child = mcpServer; + mcpServer = null; + await stopChild(child, { label: LABEL, what: "MCP test server" }); + } +} + +async function fail(message) { + console.error(`${LABEL} FAILED — ${message}`); + await shutdown(); + process.exit(1); +} + +/** + * Switch to a main-view tab. + * + * The tabs are a Mantine `SegmentedControl`: a visually-hidden radio plus a + * sibling `