diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 35297743..a3a09e87 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -105,6 +105,8 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. - **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. +- **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) and CI covers Chromium plus Firefox (#2086; WebKit runs locally but is out of the matrix pending #2132, a real Safari SSE hang it found), 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. + - **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/.github/workflows/main.yml b/.github/workflows/main.yml index a7191880..b9676258 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -28,8 +28,8 @@ jobs: - name: Setup Node.js uses: actions/setup-node@v7 with: - node-version: '22.x' - cache: 'npm' + node-version: "22.x" + cache: "npm" - name: Install dependencies (root + all clients) # The root postinstall (scripts/install-clients.mjs) cascades @@ -115,6 +115,84 @@ jobs: working-directory: ./clients/web run: npm run test:storybook + # The same headless web smokes the `build` job already runs in Chromium, run + # again in the other engines this repo supports (#2086). + # + # Why this is a job of its own rather than a matrix over `build`: `build` also + # runs validate, the coverage gate, two verify gates and Storybook, none of + # which are engine-dependent — matrixing it would triple all of that to gain + # three browser smokes. Chromium is deliberately NOT in the matrix here for + # the same reason: `npm run smoke` inside `build` is exactly the local + # `npm run ci` path, and it already covers it. Supported set = the Chromium run + # there plus whatever the matrix below names. + # + # This is the only place the MCP Apps sandbox is exercised on a non-Chromium + # engine, and it earned its keep immediately: pointing the smokes at WebKit is + # what found #2132, a Safari-only hang in the web client's SSE transport that + # every Chromium-only tier had been green through. + # + # The unit tests cannot substitute — `sandbox-csp.test.ts` asserts + # which policy STRING is built, which passes identically on an engine that + # ignores `` CSP entirely — and no Storybook story reaches the sandbox at + # all (all three App stories use a `data:` placeholder iframe and a mock + # bridge). Note Playwright's WebKit is a WebKit build, not Safari: close enough + # to catch engine-level CSP and iframe divergence, not close enough to certify + # Safari. + browser-engine-smokes: + runs-on: ubuntu-latest + strategy: + # Report every engine's verdict. Failing fast would hide a WebKit-only + # regression behind a Firefox-only one, which is the exact distinction + # this job exists to draw. + fail-fast: false + matrix: + # `webkit` belongs here and is deliberately absent: the smokes RUN in it + # (`SMOKE_BROWSER=webkit` works, and smoke:web:browser passes), but the + # two App smokes fail on a real, pre-existing Safari incompatibility in + # the web client's SSE transport — #2132, which carries the diagnosis and + # the verified fix. Adding it here is a one-word diff once that lands. + # An engine is either green or absent; a `continue-on-error` job would + # report coverage nobody is holding to a standard. + browser: [firefox] + name: Sandbox smokes (${{ matrix.browser }}) + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: "22.x" + cache: "npm" + + - name: Install dependencies (root + all clients) + run: npm install + + - name: Build all clients + # The smokes need clients/web/dist and the cli/tui/launcher bundles. + # `build` rather than `validate` — the format/lint/typecheck half is + # engine-independent and already ran in the `build` job. + run: npm run build + + - name: Cache Playwright browsers + uses: actions/cache@v6 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ matrix.browser }}-${{ runner.os }}-${{ hashFiles('clients/web/package-lock.json') }} + + - name: Install Playwright ${{ matrix.browser }} + working-directory: ./clients/web + # `--with-deps` for the system libraries a bare runner lacks; WebKit is + # the large download here and dominates this step. + run: npx playwright install --with-deps ${{ matrix.browser }} + + - name: Run the headless web smokes in ${{ matrix.browser }} + # The set of smokes lives in the `smoke:web:engine` npm script, not + # enumerated here, so adding one covers every engine automatically. + env: + SMOKE_BROWSER: ${{ matrix.browser }} + run: npm run smoke:web:engine + # Publish the single `@modelcontextprotocol/inspector` package to npm on a # published GitHub release. v2 is not an npm workspace, so there is no # `publish-all` / `--workspaces` (v1) — just one `npm publish`, whose `prepack` @@ -127,7 +205,7 @@ jobs: runs-on: ubuntu-latest if: github.event_name == 'release' environment: release - needs: build + needs: [build, browser-engine-smokes] # Serialize publishes so two releases cut in quick succession can't run # overlapping `npm publish`es. Never cancel an in-flight publish. concurrency: @@ -145,9 +223,9 @@ jobs: - name: Setup Node.js uses: actions/setup-node@v7 with: - node-version: '22.x' - cache: 'npm' - registry-url: 'https://registry.npmjs.org' + node-version: "22.x" + cache: "npm" + registry-url: "https://registry.npmjs.org" - name: Assert release tag matches package version # `npm publish` ships whatever `version` is in the root package.json, @@ -220,7 +298,7 @@ jobs: runs-on: ubuntu-latest if: github.event_name == 'release' environment: release - needs: build + needs: [build, browser-engine-smokes] permissions: contents: read packages: write diff --git a/AGENTS.md b/AGENTS.md index 936af5d8..fcc7d935 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -270,7 +270,12 @@ v2/main/ │ # pack-and-verify.mjs, and lib/ shared helpers │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program -│ # through — #1965; announced-child.mjs owns the +│ # through — #1965; headless-browser.mjs owns the +│ # browser the web smokes drive — SMOKE_BROWSER +│ # engine selection (deny-by-default), the launch, +│ # and the fatal-vs-benign page-diagnostics split, +│ # shared by all three smokes and pack:verify — +│ # #2086; announced-child.mjs owns the │ # spawn-and-wait-for-a-readiness-line step, publishing │ # the child via `onSpawn` BEFORE the wait so the │ # caller's teardown reaches it on every throw path — @@ -895,14 +900,29 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with **`smoke:web:browser`, `smoke:web:app` and `smoke:web:elicit`**, so the four can't drift. **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy. -- `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). -- `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, the staged assertions, the Chromium/diagnostics plumbing) 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 ci`, 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:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in a headless browser (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). That resolution and the launch itself now live in `scripts/lib/headless-browser.mjs`, shared by all three web smokes and `pack:verify`, which is also where `SMOKE_BROWSER` picks the engine (#2086, below). The npm script no longer prefixes `cd clients/web && npx playwright install chromium`: `scripts/install-smoke-browser.mjs` fetches the engine's binary instead, because the engine is now a variable (a `${VAR:-default}` expansion does not expand under Windows' `cmd.exe`, so it would try to install a browser literally named that) and because `npx` is a `.cmd` shim there that a shell-free spawn cannot start — the #1939 problem, solved the #1939 way, via `resolveNodeBin`. +- `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 ci`, 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. - **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 ci` 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 ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. +### The web smokes run in three browser engines (#2086) + +**`smoke:web:browser`, `smoke:web:app` and `smoke:web:elicit` support Chromium, Firefox and WebKit; `SMOKE_BROWSER` picks one, unset means `chromium`.** CI is green on Chromium and Firefox; **WebKit runs but is not in the CI matrix yet** — see the last bullet. `npm run smoke:web:engine` runs all three smokes in whichever engine is selected — that script, not the workflow YAML, is the list of engine-covered smokes, so adding a fourth covers every engine without touching CI. + +Everything about the selection lives in **`scripts/lib/headless-browser.mjs`** — `resolveBrowserName`, `loadBrowser`, and the `attachPageDiagnostics` / `FATAL_CONSOLE` split the smokes had each hand-rolled. Reach for it rather than launching Playwright in a new script. + +- **The engine matters for one surface, and it is the MCP Apps sandbox.** Most of the web client is React and Mantine, where a second engine buys little. The sandbox is built out of the primitives that genuinely diverge: a CSP `` injected as the first `` child of a **`srcdoc`** document, a **nested** sandboxed iframe, a `Permissions-Policy` `allow` attribute, and `postMessage` origin discipline across those two frames. +- **No other tier can substitute, so don't propose one.** `sandbox-csp.test.ts` asserts which policy _string_ is built — environment-independent by construction, and it would pass identically on an engine that ignores `` CSP entirely. And **no Storybook story reaches the sandbox at all**: all three App stories (`AppRenderer`, `AppsScreen`, `AppElicitationHost`) point the iframe at a `data:` placeholder and hand the renderer a mock bridge, so `sandbox-csp.ts` is imported by exactly two things in the tree — its own test and `createAppBridgeFactory.ts`. Storybook stays Chromium-only; broadening it covers a much larger, differently-shaped surface and is a separate decision to be judged on its own cost. +- **An unrecognized `SMOKE_BROWSER` is an error, never a fallback.** Falling back to Chromium would report a green Chromium run under a job labelled `webkit` — coverage claimed but not run, which is worse than none. +- **A launch failure names the engine that failed** and its `npx playwright install --with-deps `. Naming `chromium` while WebKit was the missing one sends the reader to install a browser they already have. +- **CI runs Chromium in the `build` job's `npm run smoke` and the rest in a `Sandbox smokes ()` matrix job** with `fail-fast: false`, so a WebKit-only regression is not hidden behind a Firefox-only one. Chromium is deliberately _not_ in that matrix — `build`'s `npm run smoke` is exactly the local `npm run ci` path and already covers it, and matrixing `build` itself would triple validate, the coverage gate, two verify gates and Storybook to gain three browser smokes. Both `publish` jobs `needs` the matrix, so a release cannot ship past a non-Chromium failure. +- **`pack:verify` stays pinned to Chromium**, passing it explicitly rather than reading `SMOKE_BROWSER`. It is a _packaging_ check; the engine question belongs where the sandbox is under test, and pinning it also means it can't be pointed at an engine its npm script never installed. +- **WebKit is deliberately out of the CI matrix for now, and `continue-on-error` is not the answer.** The two App smokes fail there on a real, pre-existing Safari incompatibility in the web client's SSE transport — [#2132](https://github.com/modelcontextprotocol/inspector/issues/2132) carries the diagnosis and a verified fix. Adding `webkit` to the matrix is a one-word diff once that lands; until then an engine is either green in CI or absent from it, because a job allowed to fail reports coverage nobody is held to. Worth noting the matrix found this on its first outing, against a bug every Chromium-only tier had been green through — and it is a _product_ bug, not a test one. +- ⚠️ **Playwright's WebKit is a WebKit build, not Safari.** Close enough to catch engine-level CSP and iframe divergence; not close enough to certify Safari. Don't write, in a doc or a PR description, that a green run means Safari works. + ### Build output is never a gate target **No gate — lint, format, or typecheck — may read generated output.** The gated surface is first-party source only: `clients/*/src`, `clients/*/__tests__`, `clients/web/{server,.storybook}`, each client's top-level configs, `core/`, `test-servers/src`, `scripts/`, and the root shared files. Everything a build writes is out of scope: `clients/*/build` (the tsup/tsc bundles), `clients/web/dist` (the Vite SPA), `clients/web/storybook-static`, `clients/*/coverage`, `test-servers/build`, `core/**/{build,dist}`, and any `*.tsbuildinfo`. Each scope states this itself — `globalIgnores([...])` in every `eslint.config.js`, the `format`/`format:check` globs in each `package.json`, and a tsconfig `include` that names source directories rather than the package root. diff --git a/README.md b/README.md index 1272b2b0..6e5280ce 100644 --- a/README.md +++ b/README.md @@ -452,7 +452,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `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), plus three headless-Chromium 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. | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), 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 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 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 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`. | @@ -470,6 +470,25 @@ Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), For the full testing rules — the ≥90% per-file gate, where test files live, the unit vs. integration vs. storybook projects, and the `v8 ignore` policy — see [`AGENTS.md`](./AGENTS.md). +### Supported browsers + +The three headless web smokes — `smoke:web:browser`, `smoke:web:app`, `smoke:web:elicit` — take their browser engine from `SMOKE_BROWSER`, which accepts **`chromium`, `firefox` and `webkit`** ([#2086](https://github.com/modelcontextprotocol/inspector/issues/2086)): + +```bash +SMOKE_BROWSER=webkit npm run smoke:web:app # one smoke, one engine +SMOKE_BROWSER=firefox npm run smoke:web:engine # all three smokes, one engine +``` + +Unset, the engine is `chromium` — so `npm run ci` and `npm run smoke` are unchanged. CI runs Chromium inside the `build` job's `npm run smoke` and the remaining engines in a separate `Sandbox smokes ()` matrix job, rather than running the whole `build` job once per engine (validate, the coverage gate, the two verify gates and Storybook are all engine-independent). An unrecognized value is an error, not a fallback: a silent fallback would report a green Chromium run under a job labelled `webkit`. A missing browser binary fails naming the engine and its `npx playwright install --with-deps `. + +**CI green today: Chromium and Firefox.** WebKit is supported by the tooling and worth running locally, but it is **not yet in the CI matrix** — the two App smokes fail there on a real, pre-existing Safari incompatibility in the web client's SSE transport, tracked with its diagnosis and a verified fix in [#2132](https://github.com/modelcontextprotocol/inspector/issues/2132). That is the matrix doing its job on its first outing: the bug affects the shipped web UI in Safari, and every Chromium-only tier had been green through it. An engine is either green in CI or absent from it — a `continue-on-error` job would report coverage nobody is held to. + +**Why these smokes specifically.** Most of the web client's behavior is React and Mantine, where a second engine buys little. The MCP Apps sandbox is the exception — it is built out of the primitives that genuinely diverge between engines: a CSP `` injected as the first `` child of a `srcdoc` document, a nested sandboxed iframe, a `Permissions-Policy` `allow` attribute, and `postMessage` origin discipline across those two frames. Nothing else covers that: `sandbox-csp.test.ts` asserts which policy _string_ is built, which passes identically on an engine that ignores `` CSP entirely, and no Storybook story reaches the sandbox at all (all three App stories point the iframe at a `data:` placeholder and hand the renderer a mock bridge). Storybook itself remains Chromium-only — broadening it covers a much larger and differently-shaped surface, and is a separate decision. + +> ⚠️ **Playwright's WebKit is a WebKit build, not Safari.** It is close enough to catch engine-level CSP and iframe divergence, and not close enough to certify Safari specifically. A green run here is not a Safari guarantee. + +`pack:verify` stays Chromium-only on purpose — it is a _packaging_ check, and the engine question belongs where the sandbox is under test. + ## Publishing The root `@modelcontextprotocol/inspector` package ships as **one tarball with a single version number** — no separate `-web` / `-cli` / `-tui` / `-core` packages. `npm run build` builds every client, then `prepack` runs before `npm publish`. Runtime dependencies are declared on the root `package.json`; client builds bundle `@inspector/core` and externalize npm packages resolved from the root install. diff --git a/package.json b/package.json index 1eddb213..9337b5a1 100644 --- a/package.json +++ b/package.json @@ -70,11 +70,12 @@ "smoke:cli": "node scripts/smoke-cli.mjs", "smoke:tui": "node scripts/smoke-tui.mjs", "smoke:web": "node scripts/smoke-web.mjs", - "smoke:web:browser": "cd clients/web && npx playwright install chromium && node ../../scripts/smoke-web-browser.mjs", - "smoke:web:app": "cd clients/web && npx playwright install chromium && node ../../scripts/smoke-web-app.mjs", - "smoke:web:elicit": "cd clients/web && npx playwright install chromium && node ../../scripts/smoke-web-elicitation.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:engine": "npm run smoke:web:browser && npm run smoke:web:app && npm run smoke:web:elicit", "smoke:launcher": "node scripts/smoke-launcher.mjs", - "pack:verify": "cd clients/web && npx playwright install chromium && cd ../.. && node scripts/pack-and-verify.mjs", + "pack:verify": "node scripts/install-smoke-browser.mjs chromium && node scripts/pack-and-verify.mjs", "prepack": "npm run build", "postinstall": "node scripts/install-clients.mjs" }, diff --git a/scripts/install-smoke-browser.mjs b/scripts/install-smoke-browser.mjs new file mode 100644 index 00000000..d052528c --- /dev/null +++ b/scripts/install-smoke-browser.mjs @@ -0,0 +1,84 @@ +#!/usr/bin/env node +/** + * Fetch the Playwright browser binary a headless web smoke is about to launch + * (#2086). + * + * This replaces the `cd clients/web && npx playwright install chromium` prefix + * each smoke's npm script used to carry. Two reasons it is a script rather than + * a longer npm-script line: + * + * - The engine is now a variable. `npx playwright install ${SMOKE_BROWSER:-…}` + * is a POSIX-shell expansion that does not expand under Windows' `cmd.exe`, + * which npm uses there — it would silently try to install a browser named + * `${SMOKE_BROWSER:-chromium}`. + * - `npx` is a `.cmd` shim on Windows, which a shell-free spawn cannot start. + * `resolveNodeBin` walks to the JS entry behind the package's `bin` and runs + * it with `process.execPath` instead — the same fix #1939 made for the + * verify scripts, and it keeps the resolution pinned to the repo's own + * Playwright rather than whatever `npx` might fetch. + * + * Usage: `node scripts/install-smoke-browser.mjs [engine]`. With no argument the + * engine comes from `SMOKE_BROWSER` (default `chromium`); pass one explicitly + * where the consumer's engine is fixed regardless of that variable, as + * `pack:verify` does. + */ + +import { spawnSync } from "node:child_process"; +import { join, resolve } from "node:path"; +import { + SUPPORTED_BROWSERS, + resolveBrowserName, +} from "./lib/headless-browser.mjs"; +import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; + +const repoRoot = resolve(import.meta.dirname, ".."); +const webDir = join(repoRoot, "clients", "web"); + +const requested = process.argv[2]; +let browserName; +try { + if (requested === undefined) { + browserName = resolveBrowserName(); + } else if (SUPPORTED_BROWSERS.includes(requested)) { + browserName = requested; + } else { + throw new Error( + `unsupported browser "${requested}" — expected one of ${SUPPORTED_BROWSERS.join(", ")}`, + ); + } +} catch (err) { + console.error( + `install-smoke-browser: ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); +} + +let cli; +try { + cli = resolveNodeBin("playwright", "playwright", webDir); +} catch (err) { + console.error( + "install-smoke-browser: could not resolve the Playwright CLI from clients/web — " + + `run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, + ); + process.exit(1); +} + +// `install` is a no-op when the binary is already present, so this is cheap on +// a warm machine and on a CI runner whose Playwright cache was restored. +const result = spawnSync(process.execPath, [cli, "install", browserName], { + cwd: webDir, + stdio: "inherit", +}); +if (result.error) { + console.error( + `install-smoke-browser: could not run the Playwright CLI: ${result.error.message}`, + ); + process.exit(1); +} +if (result.status !== 0) { + console.error( + `install-smoke-browser: \`playwright install ${browserName}\` exited ${result.status}`, + ); + process.exit(result.status ?? 1); +} diff --git a/scripts/lib/headless-browser.mjs b/scripts/lib/headless-browser.mjs new file mode 100644 index 00000000..f20ec266 --- /dev/null +++ b/scripts/lib/headless-browser.mjs @@ -0,0 +1,146 @@ +/** + * The headless browser the web smokes drive: which engine, how it is launched, + * and the two error channels a page reports on (#2086). + * + * All three smokes (`smoke:web:browser`, `smoke:web:app`, `smoke:web:elicit`) + * and `pack:verify` go through here, so the engine is a parameter in exactly + * one place and the diagnostics split is stated once rather than hand-rolled + * four times. + * + * Every browser-driven check in this repo used to be Chromium-only. That is + * fine for most of the web client, whose behavior is React and Mantine — and it + * is NOT fine for the MCP Apps sandbox, which is built out of exactly the + * primitives that diverge between engines: a CSP `` injected into a + * `srcdoc` document, a nested sandboxed iframe, a `Permissions-Policy` `allow` + * attribute, and `postMessage` origin discipline across those two frames. A + * regression in any of those is invisible unless it also reproduces in Chromium. + * + * The unit tests cannot cover it either: `sandbox-csp.test.ts` asserts which + * policy STRING is built, which would pass identically on an engine that + * ignores `` CSP entirely. And no Storybook story reaches the sandbox at + * all — all three App stories point the iframe at a `data:` placeholder and hand + * the renderer a mock bridge. So the smokes are the only place the sandbox is + * genuinely exercised, and this module is what lets them be pointed at another + * engine. + * + * ⚠️ Playwright's WebKit is a WebKit build, not Safari. It is close enough to + * catch engine-level CSP and iframe divergence, and not close enough to certify + * Safari specifically — a green run here is not a Safari guarantee. + * + * Playwright is resolved with a `createRequire` based at + * clients/web/package.json rather than a bare `import("playwright")`: a bare ESM + * specifier resolves relative to `scripts/`, not the cwd, so a `cd clients/web` + * in the npm script would NOT make it resolvable. See smoke-web-browser.mjs's + * header for the full gotcha. + */ + +import { createRequire } from "node:module"; +import { join } from "node:path"; + +/** + * Console messages that are the async half of the uncaught-crash class (an + * unhandled rejection or a failed dynamic import). Hard failures; every other + * console error is a diagnostic, so benign font-CDN / React-warning noise can't + * flake CI. smoke-web-browser.mjs's header documents the reasoning at length. + */ +export const FATAL_CONSOLE = + /^Uncaught\b|Failed to fetch dynamically imported module/; + +/** + * Attach the two error channels a headless page reports on. + * + * An uncaught *synchronous* page error fires `pageerror`; its async twin — an + * unhandled rejection or a failed dynamic import — is not a `pageerror` at all + * and arrives on the console channel instead. Both are captured, and both are + * read on every engine rather than branching per browser: which channel an + * engine picks is exactly the kind of detail that differs between them, and + * reading both makes that difference irrelevant. Only `fatal()` is a failure, so + * ordinary console noise stays a diagnostic. + */ +export function attachPageDiagnostics(page) { + const pageErrors = []; + const consoleErrors = []; + page.on("pageerror", (err) => + pageErrors.push(err instanceof Error ? err.message : String(err)), + ); + page.on("console", (msg) => { + if (msg.type() === "error") consoleErrors.push(msg.text()); + }); + return { + pageErrors, + consoleErrors, + fatalConsole: () => consoleErrors.filter((m) => FATAL_CONSOLE.test(m)), + benignConsole: () => consoleErrors.filter((m) => !FATAL_CONSOLE.test(m)), + fatal: () => [ + ...pageErrors, + ...consoleErrors.filter((m) => FATAL_CONSOLE.test(m)), + ], + }; +} + +/** The supported engine set. Anything else is a typo, not a request. */ +export const SUPPORTED_BROWSERS = ["chromium", "firefox", "webkit"]; + +/** The engine used when nothing asks for another one. */ +export const DEFAULT_BROWSER = "chromium"; + +/** The env var each smoke reads to pick its engine. */ +export const BROWSER_ENV_VAR = "SMOKE_BROWSER"; + +/** + * Resolve the engine name from the environment, deny-by-default. + * + * An unrecognized value FAILS rather than falling back to Chromium: a silent + * fallback would report a green Chromium run under a job labelled "webkit", + * which is worse than no coverage — it claims coverage that never ran. + */ +export function resolveBrowserName(env = process.env) { + const raw = env[BROWSER_ENV_VAR]; + if (raw === undefined || raw.trim() === "") return DEFAULT_BROWSER; + const name = raw.trim().toLowerCase(); + if (!SUPPORTED_BROWSERS.includes(name)) { + throw new Error( + `${BROWSER_ENV_VAR}="${raw}" is not a supported browser — expected one of ${SUPPORTED_BROWSERS.join(", ")}`, + ); + } + return name; +} + +/** + * Launch a headless browser of `browserName`, resolved from the web client's + * install. + * + * The launch-failure message names the engine that failed and the + * `playwright install` invocation that fixes it — naming "chromium" while + * WebKit was the thing missing sends the reader off to install a browser they + * already have. + */ +export async function loadBrowser(repoRoot, browserName = DEFAULT_BROWSER) { + if (!SUPPORTED_BROWSERS.includes(browserName)) { + throw new Error( + `unsupported browser "${browserName}" — expected one of ${SUPPORTED_BROWSERS.join(", ")}`, + ); + } + const requireFromWeb = createRequire( + join(repoRoot, "clients", "web", "package.json"), + ); + let playwright; + try { + playwright = requireFromWeb("playwright"); + } catch (err) { + // Not resolvable means devDependencies are missing — fixed by `npm install` + // at the repo root, NOT by `playwright install` (which fetches binaries). + throw new Error( + `could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, + ); + } + try { + return await playwright[browserName].launch({ headless: true }); + } catch (err) { + throw new Error( + `${browserName} failed to launch — run \`npx playwright install --with-deps ${browserName}\`, ` + + `which fetches the browser and (on a bare Linux box) its system libraries ` + + `(${err instanceof Error ? err.message : String(err)})`, + ); + } +} diff --git a/scripts/lib/headless-browser.test.mjs b/scripts/lib/headless-browser.test.mjs new file mode 100644 index 00000000..187d02e7 --- /dev/null +++ b/scripts/lib/headless-browser.test.mjs @@ -0,0 +1,148 @@ +/** + * Unit tests for the headless-browser engine selection (#2086). + * + * The smokes cannot check this themselves: each one resolves exactly one engine + * per process and then spends its whole run inside the happy path, so the branch + * that matters most — an unrecognized `SMOKE_BROWSER` — is dead code from their + * point of view. It is also the branch whose failure is silent rather than loud: + * a fallback to Chromium there would report a green run under a job labelled + * "webkit", claiming coverage that never ran. + * + * `loadBrowser`'s own launch path is deliberately not covered here — it takes a + * real browser binary, which is what the smokes are for. Only its argument + * validation, which fails before any of that, is. + */ + +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + BROWSER_ENV_VAR, + DEFAULT_BROWSER, + SUPPORTED_BROWSERS, + attachPageDiagnostics, + loadBrowser, + resolveBrowserName, +} from "./headless-browser.mjs"; + +describe("resolveBrowserName", () => { + it("defaults to chromium when the variable is unset", () => { + assert.equal(resolveBrowserName({}), DEFAULT_BROWSER); + }); + + it("treats an empty or whitespace-only value as unset", () => { + // A CI expression that resolves to nothing (`SMOKE_BROWSER: ${{ … }}` with + // an undefined matrix key) sets the variable to "" rather than removing it. + for (const raw of ["", " "]) { + assert.equal( + resolveBrowserName({ [BROWSER_ENV_VAR]: raw }), + DEFAULT_BROWSER, + ); + } + }); + + it("accepts every supported engine", () => { + for (const name of SUPPORTED_BROWSERS) { + assert.equal(resolveBrowserName({ [BROWSER_ENV_VAR]: name }), name); + } + }); + + it("normalizes surrounding whitespace and case", () => { + assert.equal( + resolveBrowserName({ [BROWSER_ENV_VAR]: " WebKit " }), + "webkit", + ); + }); + + it("rejects an unrecognized engine rather than falling back", () => { + assert.throws( + () => resolveBrowserName({ [BROWSER_ENV_VAR]: "safari" }), + // The message must quote what was asked for AND list what is accepted — + // "safari" is the most likely typo here and its fix is not guessable. + (err) => + /safari/.test(err.message) && + SUPPORTED_BROWSERS.every((name) => err.message.includes(name)), + ); + }); +}); + +describe("loadBrowser", () => { + it("rejects an unsupported engine before touching Playwright", async () => { + await assert.rejects( + // A repo root that does not exist: reaching the `createRequire` would + // throw a different (resolution) error, so this also pins the ORDER — + // validation first, so the message names the real mistake. + () => loadBrowser("/nonexistent-repo-root", "safari"), + /unsupported browser "safari"/, + ); + }); +}); + +describe("attachPageDiagnostics", () => { + /** Minimal Playwright `page` stand-in: records handlers, replays events. */ + function fakePage() { + const handlers = {}; + return { + on: (event, fn) => { + (handlers[event] ??= []).push(fn); + }, + emit: (event, arg) => handlers[event]?.forEach((fn) => fn(arg)), + }; + } + + const consoleMessage = (type, text) => ({ + type: () => type, + text: () => text, + }); + + it("splits fatal console errors from benign noise", () => { + const page = fakePage(); + const diagnostics = attachPageDiagnostics(page); + + page.emit("console", consoleMessage("error", "Uncaught (in promise) boom")); + page.emit( + "console", + consoleMessage("error", "Failed to fetch dynamically imported module x"), + ); + // The two shapes that used to flake CI, and must stay diagnostics. + page.emit( + "console", + consoleMessage("error", "Failed to load resource: net::ERR_FAILED"), + ); + page.emit( + "console", + consoleMessage("error", "Warning: each child needs a key"), + ); + // Non-error console output is ignored entirely. + page.emit("console", consoleMessage("log", "Uncaught looking but a log")); + + assert.deepEqual(diagnostics.fatalConsole(), [ + "Uncaught (in promise) boom", + "Failed to fetch dynamically imported module x", + ]); + assert.deepEqual(diagnostics.benignConsole(), [ + "Failed to load resource: net::ERR_FAILED", + "Warning: each child needs a key", + ]); + }); + + it("counts every pageerror as fatal, whatever it was thrown as", () => { + const page = fakePage(); + const diagnostics = attachPageDiagnostics(page); + + page.emit("pageerror", new Error("sync boom")); + page.emit("pageerror", "thrown as a string"); + page.emit("console", consoleMessage("error", "benign")); + + assert.deepEqual(diagnostics.pageErrors, [ + "sync boom", + "thrown as a string", + ]); + assert.deepEqual(diagnostics.fatal(), ["sync boom", "thrown as a string"]); + }); + + it("reports nothing on a clean page", () => { + const diagnostics = attachPageDiagnostics(fakePage()); + assert.deepEqual(diagnostics.fatal(), []); + assert.deepEqual(diagnostics.benignConsole(), []); + }); +}); diff --git a/scripts/lib/mcp-app-flow.mjs b/scripts/lib/mcp-app-flow.mjs index 23233f61..99a21e3c 100644 --- a/scripts/lib/mcp-app-flow.mjs +++ b/scripts/lib/mcp-app-flow.mjs @@ -22,14 +22,12 @@ * **test server** is always a repo fixture — it is not in the tarball and * should not be. * - * Playwright is resolved with a `createRequire` based at - * clients/web/package.json rather than a bare `import("playwright")`: a bare ESM - * specifier resolves relative to `scripts/`, not the cwd, so a `cd clients/web` - * in the npm script would NOT make it resolvable. Same gotcha as - * smoke-web-browser.mjs; see its header. + * Launching the browser is NOT this module's job — `lib/headless-browser.mjs` + * owns that, so the engine is a parameter rather than a hard-coded Chromium + * (#2086) and so `smoke:web:browser`, which does not drive an App at all, can + * share it without importing this file. */ -import { createRequire } from "node:module"; import { join } from "node:path"; import { startAnnouncedChild } from "./announced-child.mjs"; import { @@ -40,16 +38,6 @@ import { /** The App tool the `mcp-app-http.json` fixture serves. */ export const APP_TOOL = "mcp_app_demo"; -/** - * Console messages that are the async half of the uncaught-crash class (an - * unhandled rejection or a failed dynamic import). Hard failures; every other - * console error is a diagnostic, so benign font-CDN / React-warning noise can't - * flake CI. Kept identical to smoke-web-browser.mjs, which documents the - * reasoning at length. - */ -export const FATAL_CONSOLE = - /^Uncaught\b|Failed to fetch dynamically imported module/; - /** Path to the composable test server build, relative to the repo root. */ export function composableServerPath(repoRoot) { return testServerEntryPath(repoRoot, "composable"); @@ -143,59 +131,6 @@ export function buildAppDeepLink({ ); } -/** - * Attach the two error channels a headless page reports on. - * - * An uncaught *synchronous* page error fires `pageerror`; its async twin — an - * unhandled rejection or a failed dynamic import — is not a `pageerror` at all, - * Chromium reports it on the console channel instead. Both are captured; only - * `fatal()` is a failure, so ordinary console noise stays a diagnostic. - */ -export function attachPageDiagnostics(page) { - const pageErrors = []; - const consoleErrors = []; - page.on("pageerror", (err) => - pageErrors.push(err instanceof Error ? err.message : String(err)), - ); - page.on("console", (msg) => { - if (msg.type() === "error") consoleErrors.push(msg.text()); - }); - return { - pageErrors, - consoleErrors, - fatalConsole: () => consoleErrors.filter((m) => FATAL_CONSOLE.test(m)), - benignConsole: () => consoleErrors.filter((m) => !FATAL_CONSOLE.test(m)), - fatal: () => [ - ...pageErrors, - ...consoleErrors.filter((m) => FATAL_CONSOLE.test(m)), - ], - }; -} - -/** Launch headless Chromium, resolved from the web client's install. */ -export async function loadChromium(repoRoot) { - const requireFromWeb = createRequire( - join(repoRoot, "clients", "web", "package.json"), - ); - let chromium; - try { - ({ chromium } = requireFromWeb("playwright")); - } catch (err) { - // Not resolvable means devDependencies are missing — fixed by `npm install` - // at the repo root, NOT by `playwright install` (which fetches binaries). - throw new Error( - `could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, - ); - } - try { - return await chromium.launch({ headless: true }); - } catch (err) { - throw new Error( - `chromium failed to launch — on a bare Linux box run \`npx playwright install --with-deps chromium\` for the system libraries (${err instanceof Error ? err.message : String(err)})`, - ); - } -} - /** * Drive **connect → open app → widget ready** on an already-open page. * diff --git a/scripts/pack-and-verify.mjs b/scripts/pack-and-verify.mjs index 260eeb48..1145f4b2 100644 --- a/scripts/pack-and-verify.mjs +++ b/scripts/pack-and-verify.mjs @@ -60,12 +60,11 @@ import { ensureTestServers, testServerEntryPath, } from "./lib/ensure-test-servers.mjs"; +import { attachPageDiagnostics, loadBrowser } from "./lib/headless-browser.mjs"; import { APP_TOOL, - attachPageDiagnostics, buildAppDeepLink, driveAppFlow, - loadChromium, startMcpAppServer, } from "./lib/mcp-app-flow.mjs"; import { winShellArgs } from "./lib/win-shell-args.mjs"; @@ -567,7 +566,11 @@ async function verifyAppRender(baseUrl, token, whenWebServerExits) { }, label: LABEL, }); - browser = await loadChromium(repoRoot); + // Chromium explicitly, not `resolveBrowserName()`: this is a *packaging* + // check, and the engine matrix (#2086) belongs to the smokes, where the + // sandbox surface is what is under test. Pinning it also means `pack:verify` + // cannot be pointed at an engine its npm script never installed. + browser = await loadBrowser(repoRoot, "chromium"); const page = await browser.newPage(); const diagnostics = attachPageDiagnostics(page); diff --git a/scripts/smoke-web-app.mjs b/scripts/smoke-web-app.mjs index d4ea5c7d..81ba3199 100644 --- a/scripts/smoke-web-app.mjs +++ b/scripts/smoke-web-app.mjs @@ -41,6 +41,16 @@ * renamed* without its reader being updated, a repo-tree failure pack:verify * would only find later. * + * ── Which engine ──────────────────────────────────────────────────────────── + * + * `SMOKE_BROWSER` picks the engine (`chromium` — the default — `firefox`, or + * `webkit`); CI runs all three (#2086). This smoke is one of the two places the + * MCP Apps sandbox is genuinely exercised, and the sandbox is built out of the + * primitives that actually diverge between engines — `srcdoc` CSP inheritance, + * nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. See + * `lib/headless-browser.mjs`, including why a green WebKit run is not a Safari + * guarantee. + * * 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, as in smoke:cli — see `scripts/lib/ensure-test-servers.mjs` for why @@ -54,17 +64,34 @@ import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; import { - APP_TOOL, attachPageDiagnostics, + loadBrowser, + resolveBrowserName, +} from "./lib/headless-browser.mjs"; +import { + APP_TOOL, buildAppDeepLink, driveAppFlow, - loadChromium, sandboxProxyPageFor, startMcpAppServer, } from "./lib/mcp-app-flow.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); -const LABEL = "smoke:web:app"; + +// Resolved before anything is started, so an unsupported SMOKE_BROWSER fails +// immediately rather than after a web server and two MCP servers are up. Every +// message this smoke prints carries the engine, so a matrix failure names which +// one broke without the reader having to match it to a job title. +let BROWSER; +try { + BROWSER = resolveBrowserName(); +} catch (err) { + console.error( + `smoke:web:app FAILED — ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); +} +const LABEL = `smoke:web:app [${BROWSER}]`; // Resolved exactly as the runtime does, from the built runner's directory. const sandboxProxyPage = sandboxProxyPageFor( @@ -141,7 +168,7 @@ try { label: LABEL, }); await server.waitForReady(); - browser = await loadChromium(repoRoot); + browser = await loadBrowser(repoRoot, BROWSER); const page = await browser.newPage(); const diagnostics = attachPageDiagnostics(page); diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index ade88c54..aa50a5a3 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -29,12 +29,13 @@ * Two channels carry the failure. A *synchronous* uncaught exception (the * CASE-1 shape above — a stub call during module init) fires `pageerror`. Its * *async* twin — the same `TypeError` reached through an `await`/`.then()`, or a - * failed dynamic import (this app lazy-loads chunks) — is NOT a `pageerror`; - * Chromium logs it on the **console** channel as `Uncaught (in promise) …` / - * `Failed to fetch dynamically imported module`. Both are hard failures. + * failed dynamic import (this app lazy-loads chunks) — is NOT a `pageerror`; it + * arrives on the **console** channel as `Uncaught (in promise) …` / + * `Failed to fetch dynamically imported module`. Both are hard failures, and + * both channels are read on every engine rather than branching per browser. * - * Every *other* `console.error` is NOT a hard failure: the console is where - * Chromium also reports benign things a boot smoke shouldn't fail on — a failed + * Every *other* `console.error` is NOT a hard failure: the console is also where + * benign things a boot smoke shouldn't fail on land — a failed * subresource load (e.g. the Google-Fonts `` in index.html on a * network-restricted box) or a React key/prop warning. Those are printed as * diagnostics. The `Uncaught` / dynamic-import prefixes are unambiguous — a @@ -42,29 +43,44 @@ * never starts with `Uncaught` — so hard-failing on them can't reintroduce that * flake. * - * Playwright lives in clients/web's node_modules, so it's resolved with a - * `createRequire` based at clients/web/package.json rather than a bare - * `import("playwright")`. A bare ESM specifier resolves relative to *this - * script's* directory (scripts/), not the cwd — so `cd clients/web` in the npm - * script would NOT make it resolvable (it only appeared to work locally when an - * ancestor node_modules happened to carry playwright; it fails in CI, which has - * none). createRequire pins resolution to clients/web regardless of cwd. + * Launching the browser (and resolving Playwright from clients/web, which has + * its own gotcha — see `lib/headless-browser.mjs`) is delegated to that module, + * which is also where `SMOKE_BROWSER` picks the engine: `chromium` (the + * default), `firefox`, or `webkit`, all three of which CI runs (#2086). The + * engine question here is narrower than in the App smokes — this asserts a clean + * first paint, i.e. that the shipped bundle's syntax and API level are + * *reachable* on the engine at all, rather than anything about the sandbox — but + * it is real, and it is nearly free to include. * * Expects `clients/web/dist` and `clients/launcher/build` to be built first — * the validate / CI ordering guarantees this. */ -import { createRequire } from "node:module"; import { setTimeout as delay } from "node:timers/promises"; import { fileURLToPath } from "node:url"; import { dirname, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; +import { + attachPageDiagnostics, + loadBrowser, + resolveBrowserName, +} from "./lib/headless-browser.mjs"; -const scriptDir = dirname(fileURLToPath(import.meta.url)); -// Resolve playwright from clients/web (where it's installed) no matter the cwd. -const requireFromWeb = createRequire( - resolve(scriptDir, "..", "clients/web/package.json"), -); +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +// Resolved before the web server is started, so an unsupported SMOKE_BROWSER +// fails immediately. Every message carries the engine, so a matrix failure names +// which one broke. +let BROWSER; +try { + BROWSER = resolveBrowserName(); +} catch (err) { + console.error( + `smoke:web:browser FAILED — ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); +} +const LABEL = `smoke:web:browser [${BROWSER}]`; const HOST = "127.0.0.1"; // Distinct from smoke:web's SMOKE_WEB_PORT so overriding one doesn't make both @@ -72,17 +88,11 @@ const HOST = "127.0.0.1"; const PORT = process.env.SMOKE_WEB_BROWSER_PORT ?? "6298"; const TOKEN = "smoke-web-browser-token"; -// Console messages that are the async half of the uncaught-crash class (an -// unhandled promise rejection or a failed dynamic import). Hard failures, unlike -// benign console noise. See the header comment for why this can't reintroduce -// the font/CDN flake. -const FATAL_CONSOLE = /^Uncaught\b|Failed to fetch dynamically imported module/; - const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN, - label: "smoke:web:browser", + label: LABEL, }); let browser = null; @@ -99,51 +109,22 @@ async function shutdown() { } async function fail(message) { - console.error(`smoke:web:browser FAILED — ${message}`); + console.error(`${LABEL} FAILED — ${message}`); await shutdown(); process.exit(1); } -async function loadChromium() { - let chromium; - try { - ({ chromium } = requireFromWeb("playwright")); - } catch (err) { - // A failure here means the playwright *npm package* isn't resolvable from - // clients/web — fixed by installing devDependencies, NOT by `playwright - // install` (which fetches browser binaries). `npm install` at the repo root - // cascades into clients/web via postinstall. - throw new Error( - `could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, - ); - } - try { - return await chromium.launch({ headless: true }); - } catch (err) { - throw new Error( - `chromium failed to launch — on a bare Linux box run \`npx playwright install --with-deps chromium\` for the system libraries (${err instanceof Error ? err.message : String(err)})`, - ); - } -} - try { await server.waitForReady(); - browser = await loadChromium(); + browser = await loadBrowser(repoRoot, BROWSER); const page = await browser.newPage(); // Uncaught (synchronous) page errors are a hard failure — a Node-only module // reaching the browser bundle surfaces here as a TypeError when its empty stub - // is called during module init. - const pageErrors = []; - // Console errors are diagnostic only EXCEPT those matching FATAL_CONSOLE (the - // async half of the same crash class) — see the header comment. - const consoleErrors = []; - page.on("pageerror", (err) => - pageErrors.push(err instanceof Error ? err.message : String(err)), - ); - page.on("console", (msg) => { - if (msg.type() === "error") consoleErrors.push(msg.text()); - }); + // is called during module init. Console errors are diagnostic only EXCEPT the + // async half of that same crash class; see the header comment, and the shared + // helper for the split. + const diagnostics = attachPageDiagnostics(page); const render = async () => { // Token is injected into index.html by the prod server, so a bare `/` load @@ -175,39 +156,35 @@ try { try { await Promise.race([server.whenChildExits(), render()]); } catch (err) { - const diagnostics = [ - ...pageErrors, - ...consoleErrors.map((m) => `console: ${m}`), + const notes = [ + ...diagnostics.pageErrors, + ...diagnostics.consoleErrors.map((m) => `console: ${m}`), ]; await fail( `${err instanceof Error ? err.message : String(err)}${ - diagnostics.length - ? ` — page diagnostics: ${diagnostics.join("; ")}` - : "" + notes.length ? ` — page diagnostics: ${notes.join("; ")}` : "" }`, ); } // Hard failures: any uncaught (sync) page error, plus console errors that are // the async half of the class (unhandled rejection / failed dynamic import). - const fatalConsole = consoleErrors.filter((m) => FATAL_CONSOLE.test(m)); - if (pageErrors.length > 0 || fatalConsole.length > 0) { - await fail( - `app logged uncaught error(s): ${[...pageErrors, ...fatalConsole].join("; ")}`, - ); + const fatal = diagnostics.fatal(); + if (fatal.length > 0) { + await fail(`app logged uncaught error(s): ${fatal.join("; ")}`); } // Non-fatal console errors: surface them so a real problem isn't invisible, // without failing the smoke on benign subresource/warning noise. - const benignConsole = consoleErrors.filter((m) => !FATAL_CONSOLE.test(m)); - if (benignConsole.length > 0) { + const benign = diagnostics.benignConsole(); + if (benign.length > 0) { console.log( - `smoke:web:browser note — ${benignConsole.length} non-fatal console error(s): ${benignConsole.join("; ")}`, + `${LABEL} note — ${benign.length} non-fatal console error(s): ${benign.join("; ")}`, ); } console.log( - `smoke:web:browser OK — app booted at ${server.baseUrl}, rendered "Add Servers" with no uncaught errors (sync page error or unhandled rejection)`, + `${LABEL} OK — app booted at ${server.baseUrl}, rendered "Add Servers" with no uncaught errors (sync page error or unhandled rejection)`, ); await shutdown(); process.exit(0); diff --git a/scripts/smoke-web-elicitation.mjs b/scripts/smoke-web-elicitation.mjs index 42599c0a..19ba2c5d 100644 --- a/scripts/smoke-web-elicitation.mjs +++ b/scripts/smoke-web-elicitation.mjs @@ -22,9 +22,14 @@ * therefore needs `frameLocator(...).frameLocator(...)`, not one hop. * * Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of each state (used to attach - * proof to a PR); unset, it asserts only. Playwright is resolved with a - * `createRequire` based at clients/web/package.json for the reason documented at - * length in smoke-web-browser.mjs. + * proof to a PR); unset, it asserts only. + * + * `SMOKE_BROWSER` picks the engine (`chromium` — the default — `firefox`, or + * `webkit`); CI runs all three (#2086). Along with `smoke:web:app` this is one + * of the two places the MCP Apps sandbox is actually loaded, and the two nested + * frames below are precisely the surface that diverges between engines. See + * `lib/headless-browser.mjs`, including why a green WebKit run is not a Safari + * guarantee. * * Expects `clients/web/dist` and `clients/launcher/build` to be built first. * `test-servers/build` is rebuilt on every run, as in smoke:web:app — see @@ -33,20 +38,35 @@ import { spawn } from "node:child_process"; import { mkdirSync } from "node:fs"; -import { createRequire } from "node:module"; import { setTimeout as delay } from "node:timers/promises"; import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; +import { + attachPageDiagnostics, + loadBrowser, + resolveBrowserName, +} from "./lib/headless-browser.mjs"; import { ensureTestServers, testServerEntryPath, } from "./lib/ensure-test-servers.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); -const requireFromWeb = createRequire( - resolve(repoRoot, "clients/web/package.json"), -); + +// Resolved before anything is started, so an unsupported SMOKE_BROWSER fails +// immediately rather than after a web server and two MCP servers are up. Every +// message carries the engine, so a matrix failure names which one broke. +let BROWSER; +try { + BROWSER = resolveBrowserName(); +} catch (err) { + console.error( + `smoke:web:elicit FAILED — ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); +} +const LABEL = `smoke:web:elicit [${BROWSER}]`; const composableServer = testServerEntryPath(repoRoot, "composable"); const configPath = (name) => @@ -59,9 +79,6 @@ const PORT = process.env.SMOKE_WEB_ELICIT_PORT ?? "6296"; const TOKEN = "smoke-web-elicit-token"; const TOOL = "app_choose_option"; const SHOT_DIR = process.env.SMOKE_SCREENSHOT_DIR; -// The async half of the uncaught-crash class. Kept identical to -// smoke-web-browser.mjs / smoke-web-app.mjs. -const FATAL_CONSOLE = /^Uncaught\b|Failed to fetch dynamically imported module/; const servers = []; let browser = null; @@ -69,7 +86,7 @@ const web = startProdWebServer({ host: HOST, port: PORT, token: TOKEN, - label: "smoke:web:elicit", + label: LABEL, }); async function shutdown() { @@ -84,14 +101,14 @@ async function shutdown() { await web.stop(); while (servers.length) { await stopChild(servers.pop(), { - label: "smoke:web:elicit", + label: LABEL, what: "MCP test server", }); } } async function fail(message) { - console.error(`smoke:web:elicit FAILED — ${message}`); + console.error(`${LABEL} FAILED — ${message}`); await shutdown(); process.exit(1); } @@ -134,24 +151,6 @@ async function startMcpServer(configName) { throw new Error(`MCP test server did not start within 30s:\n${out}`); } -async function loadChromium() { - let chromium; - try { - ({ chromium } = requireFromWeb("playwright")); - } catch (err) { - throw new Error( - `could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, - ); - } - try { - return await chromium.launch({ headless: true }); - } catch (err) { - throw new Error( - `chromium failed to launch — on a bare Linux box run \`npx playwright install --with-deps chromium\` for the system libraries (${err instanceof Error ? err.message : String(err)})`, - ); - } -} - async function shot(page, name) { if (!SHOT_DIR) return; mkdirSync(SHOT_DIR, { recursive: true }); @@ -159,7 +158,7 @@ async function shot(page, name) { path: join(SHOT_DIR, `${name}.png`), fullPage: false, }); - console.log(`smoke:web:elicit — captured ${name}.png`); + console.log(`${LABEL} — captured ${name}.png`); } /** Connect to `mcpUrl` through the deep link and wait for the Tools list. */ @@ -207,31 +206,24 @@ try { // Rebuilt on every run — presence is not freshness (#2111). ensureTestServers({ repoRoot, - label: "smoke:web:elicit", + label: LABEL, requires: ["composable"], }); const appUrl = await startMcpServer("app-elicitation-http"); const nativeUrl = await startMcpServer("app-elicitation-native-http"); await web.waitForReady(); - browser = await loadChromium(); + browser = await loadBrowser(repoRoot, BROWSER); const page = await browser.newPage({ viewport: { width: 1280, height: 900 }, }); - // Uncaught *synchronous* page errors, and their *async* twin (an unhandled - // rejection or a failed dynamic import), which Chromium reports on the - // console channel instead. Both are hard failures; every other console error - // is only a diagnostic, so benign font/React noise cannot flake CI. Same - // split as smoke:web:browser and smoke:web:app, which document it at length. - const pageErrors = []; - const consoleErrors = []; - page.on("pageerror", (err) => - pageErrors.push(err instanceof Error ? err.message : String(err)), - ); - page.on("console", (msg) => { - if (msg.type() === "error") consoleErrors.push(msg.text()); - }); - const fatalConsole = () => consoleErrors.filter((m) => FATAL_CONSOLE.test(m)); + // Uncaught *synchronous* page errors, plus their *async* twin (an unhandled + // rejection or a failed dynamic import), which arrives on the console channel + // instead. Both are hard failures; every other console error is only a + // diagnostic, so benign font/React noise cannot flake CI. Shared with + // smoke:web:app rather than re-hand-rolled — the split is subtle enough that + // two copies would drift, and it is documented at length on the helper. + const diagnostics = attachPageDiagnostics(page); const drive = async () => { // ── 1. Negotiated: the server's app answers the elicitation ──────────── @@ -297,27 +289,25 @@ try { try { await Promise.race([web.whenChildExits(), drive()]); } catch (err) { - const diagnostics = [ - ...pageErrors, - ...fatalConsole().map((m) => `console: ${m}`), + const notes = [ + ...diagnostics.pageErrors, + ...diagnostics.fatalConsole().map((m) => `console: ${m}`), ]; await fail( `${err instanceof Error ? err.message : String(err)}${ - diagnostics.length - ? ` — page diagnostics: ${diagnostics.join("; ")}` - : "" + notes.length ? ` — page diagnostics: ${notes.join("; ")}` : "" }`, ); } // A drive that reached all of its assertions still fails if the page threw on // the way: without this the smoke prints OK over a broken bundle. - const fatal = [...pageErrors, ...fatalConsole()]; + const fatal = diagnostics.fatal(); if (fatal.length > 0) { await fail(`page logged uncaught error(s): ${fatal.join("; ")}`); } - console.log("smoke:web:elicit OK — app-rendered and native paths both drive"); + console.log(`${LABEL} OK — app-rendered and native paths both drive`); await shutdown(); } catch (err) { await fail(err instanceof Error ? err.message : String(err));