From acd3f98c0e073cb81a7a76c5ee83123681f74d3a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 17:33:13 -0400 Subject: [PATCH 1/8] ci: add headless-browser boot smoke for the web client (#1615) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit smoke:web only asserts GET / serves the SPA HTML with the injected token; it never runs the React app, so a Node built-in reaching the browser bundle (which crashes the app to a blank page at runtime, e.g. #1612) slips through. Add smoke:web:browser, which boots the same prod --web server and runs the bundle in headless Chromium (Playwright, already a clients/web devDependency), asserting the app renders its first frame (the "Add Servers" control) with no uncaught page errors — in particular no `Module "node:*" has been externalized`. This catches "Node code reached the browser bundle" as a class, not one import at a time. Wire it into the root smoke chain (and thus npm run ci / GitHub CI), and move the Playwright install/cache steps ahead of the smoke step so the new smoke reuses the cache. Update AGENTS.md and README. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .github/workflows/main.yml | 24 +++-- AGENTS.md | 3 +- README.md | 2 +- package.json | 3 +- scripts/smoke-web-browser.mjs | 161 ++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+), 12 deletions(-) create mode 100644 scripts/smoke-web-browser.mjs diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index fbdd32a0f..c136918cf 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -53,15 +53,11 @@ jobs: # running the integration suite twice. run: npm run coverage - - name: Run cross-client smokes - # End-to-end smokes through the built launcher (--help dispatch + prod - # CLI/web). Not part of any client's `validate`: it needs the - # cli/tui/launcher bundles, which `validate` above already built - # (smoke:web builds clients/web/dist on demand — #1486). smoke:tui - # self-skips here — the Ink TUI needs a real TTY (raw mode) that headless - # CI lacks, so its boot/render check is local-only. - run: npm run smoke - + # Playwright chromium is installed BEFORE the smokes because + # `smoke:web:browser` (the headless-browser boot smoke, #1615) drives the + # prod web bundle in chromium — restoring/installing it here lets that + # smoke reuse the cache instead of downloading its own copy. The Storybook + # step below reuses the same install. - name: Cache Playwright browsers uses: actions/cache@v4 with: @@ -72,6 +68,16 @@ jobs: working-directory: ./clients/web run: npx playwright install --with-deps chromium + - name: Run cross-client smokes + # End-to-end smokes through the built launcher (--help dispatch + prod + # CLI/web). Not part of any client's `validate`: it needs the + # cli/tui/launcher bundles, which `validate` above already built + # (smoke:web builds clients/web/dist on demand — #1486). smoke:web:browser + # boots the prod web bundle in headless chromium (#1615). smoke:tui + # self-skips here — the Ink TUI needs a real TTY (raw mode) that headless + # CI lacks, so its boot/render check is local-only. + run: npm run smoke + - name: Run Storybook play-function tests working-directory: ./clients/web run: npm run test:storybook diff --git a/AGENTS.md b/AGENTS.md index bdbb641d7..72e998504 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -216,9 +216,10 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. -- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke`. Storybook is the only CI step left out (see below). +- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only CI step left out (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. +- `smoke: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 page errors** — in particular no `Module "node:*" has been externalized for browser compatibility`. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle (which crashes the app to a blank page at runtime, e.g. #1612) slipped through it; this smoke catches that regression as a *class*. The npm script runs `cd clients/web && npx playwright install chromium` first so `import("playwright")` resolves and chromium is present (a no-op when already installed); repo-root paths inside the script come from `import.meta.url`, so the cwd change doesn't affect which build tree is exercised. - `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. diff --git a/README.md b/README.md index 8b3b2eb1e..71a74003f 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs `validate:core` (the shared `core/` `format:check` + `lint` gate) first, then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `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). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no `node:*` in the browser bundle). | | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | diff --git a/package.json b/package.json index 26f97f0ae..8707fd2f1 100644 --- a/package.json +++ b/package.json @@ -54,10 +54,11 @@ "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", + "smoke": "npm run smoke:launcher && npm run smoke:cli && npm run smoke:tui && npm run smoke:web && npm run smoke:web:browser", "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:launcher": "node scripts/smoke-launcher.mjs", "pack:verify": "node scripts/pack-and-verify.mjs", "prepack": "npm run build", diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs new file mode 100644 index 000000000..530495f77 --- /dev/null +++ b/scripts/smoke-web-browser.mjs @@ -0,0 +1,161 @@ +#!/usr/bin/env node +/** + * Headless-browser boot smoke for the prod web client (#1615). + * + * `smoke:web` (scripts/smoke-web.mjs) only asserts that `GET /` serves the SPA + * HTML with the injected auth token — it never executes the React app, so a + * regression that only manifests when the bundle *runs* slips through. The + * canonical example (#1612): the browser bundle transitively value-imported a + * Node-only module, pulling `node:*` built-ins into the browser build and + * crashing the app to a blank page with + * `Module "node:*" has been externalized for browser compatibility`. + * Unit/integration tests run in node / happy-dom, never a real browser bundle, + * so none of them caught it. + * + * This script closes that gap as a *class* of bug rather than one import at a + * time: it launches `mcp-inspector --web` (prod, no `--dev`) against the built + * `clients/web/dist`, opens the served page in headless Chromium (Playwright, + * already a clients/web devDependency for the Storybook tests), and asserts the + * app renders its first meaningful frame (the "Add Servers" control) with **no + * uncaught page errors** — in particular no `node:*` externalization error. + * + * Run from the clients/web directory (`npm run smoke:web:browser` does + * `cd clients/web` first) so `import("playwright")` resolves against that + * client's node_modules. Repo-root paths below are derived from import.meta.url, + * so the cwd change doesn't affect which launcher/build tree is exercised. + * + * Expects `clients/web/dist` and `clients/launcher/build` to be built first — + * the validate / CI ordering guarantees this. + */ + +import { spawn } from "node:child_process"; +import { setTimeout as delay } from "node:timers/promises"; +import { fileURLToPath } from "node:url"; +import { dirname, resolve } from "node:path"; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const repoRoot = resolve(scriptDir, ".."); +const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js"); + +const HOST = "127.0.0.1"; +const PORT = process.env.SMOKE_WEB_PORT ?? "6298"; +const TOKEN = "smoke-web-browser-token"; +const BASE_URL = `http://${HOST}:${PORT}`; + +const child = spawn(process.execPath, [launcherEntry, "--web"], { + env: { + ...process.env, + CLIENT_PORT: PORT, + HOST, + MCP_INSPECTOR_API_TOKEN: TOKEN, + // Don't pop a browser in CI. + MCP_AUTO_OPEN_ENABLED: "false", + }, + stdio: ["ignore", "inherit", "inherit"], +}); + +let exited = false; +let exitCode = null; +child.on("exit", (code) => { + exited = true; + exitCode = code; +}); + +let browser = null; + +async function shutdown() { + if (browser) { + try { + await browser.close(); + } catch { + // best-effort + } + browser = null; + } + if (!exited) child.kill("SIGTERM"); +} + +async function fail(message) { + console.error(`smoke:web:browser FAILED — ${message}`); + await shutdown(); + process.exit(1); +} + +async function waitForServer() { + for (let attempt = 0; attempt < 120; attempt++) { + if (exited) { + throw new Error( + `launcher exited (code ${exitCode}) before serving — see output above`, + ); + } + try { + const res = await fetch(`${BASE_URL}/`); + if (res.ok) return; + } catch { + // not listening yet + } + await delay(500); + } + throw new Error("server did not start within 60s"); +} + +// A page error whose message mentions a node: built-in being externalized is +// the #1612 signature; treat *any* uncaught page error as a failure, but call +// this class out explicitly since it's the regression this smoke exists for. +function describeError(err) { + const message = err instanceof Error ? err.message : String(err); + if (/Module "node:.*" has been externalized/.test(message)) { + return `Node built-in reached the browser bundle: ${message}`; + } + return message; +} + +try { + const { chromium } = await import("playwright"); + await waitForServer(); + + browser = await chromium.launch({ headless: true }); + const page = await browser.newPage(); + + const pageErrors = []; + page.on("pageerror", (err) => pageErrors.push(describeError(err))); + page.on("console", (msg) => { + if (msg.type() === "error") pageErrors.push(`console.error: ${msg.text()}`); + }); + + // Token is injected into index.html by the prod server, so a bare `/` load + // authenticates without a query param. + const response = await page.goto(BASE_URL, { + waitUntil: "domcontentloaded", + timeout: 30_000, + }); + if (!response || !response.ok()) { + await fail( + `GET / returned HTTP ${response ? response.status() : "no response"}`, + ); + } + + // First meaningful frame: the always-present "Add Servers" control. + await page + .getByRole("button", { name: /Add Servers/ }) + .waitFor({ state: "visible", timeout: 30_000 }) + .catch(async () => { + await fail( + `app did not render the "Add Servers" control within 30s${ + pageErrors.length ? ` — page errors: ${pageErrors.join("; ")}` : "" + }`, + ); + }); + + if (pageErrors.length > 0) { + await fail(`app rendered but logged page errors: ${pageErrors.join("; ")}`); + } + + console.log( + `smoke:web:browser OK — app booted at ${BASE_URL}, rendered "Add Servers" with no page errors`, + ); + await shutdown(); + process.exit(0); +} catch (err) { + await fail(err instanceof Error ? err.message : String(err)); +} From bb8083717aceb14984d14b1fe6f74887ea3f7433 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 17:47:26 -0400 Subject: [PATCH 2/8] ci: address review on the headless-browser boot smoke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Don't fail on benign console.error (CDN font loads, React warnings) — the #1612 class arrives as an uncaught pageerror. console errors are now diagnostics, hard-failing only when they carry the externalization signature. - Broaden the externalization regex to non-prefixed built-ins ("fs" as well as "node:fs") and label the signature on both the pageerror and console channels. - Rename the port override to SMOKE_WEB_BROWSER_PORT so it doesn't collide with smoke:web's SMOKE_WEB_PORT. - Add a settle window (networkidle + 500ms) so a late-evaluating lazy chunk that throws is caught, and race the render against launcher death so a mid-load crash reports the real cause instead of a render timeout. - Print an actionable hint when Playwright/chromium can't launch (missing --with-deps system libs). - Extract the shared spawn/readiness boilerplate into scripts/lib/prod-web-server.mjs so smoke-web and smoke-web-browser can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/lib/prod-web-server.mjs | 109 ++++++++++++++++++ scripts/smoke-web-browser.mjs | 195 ++++++++++++++++++-------------- scripts/smoke-web.mjs | 68 +++-------- 3 files changed, 235 insertions(+), 137 deletions(-) create mode 100644 scripts/lib/prod-web-server.mjs diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs new file mode 100644 index 000000000..a7d2b2b9c --- /dev/null +++ b/scripts/lib/prod-web-server.mjs @@ -0,0 +1,109 @@ +/** + * Shared boot/readiness helper for the prod web smokes. + * + * Both `scripts/smoke-web.mjs` (serves-the-HTML check) and + * `scripts/smoke-web-browser.mjs` (runs-the-bundle check, #1615) boot the *same* + * prod `mcp-inspector --web` server, so the spawn + readiness-poll boilerplate + * lives here once instead of being copy-pasted (and drifting) in each script. + * + * Repo-root paths are derived from import.meta.url, so a caller's cwd (e.g. + * `smoke:web:browser` does `cd clients/web` first so `import("playwright")` + * resolves) doesn't affect which launcher/build tree is exercised. + */ + +import { spawn } from "node:child_process"; +import { setTimeout as delay } from "node:timers/promises"; +import { fileURLToPath } from "node:url"; +import { dirname, resolve } from "node:path"; + +const libDir = dirname(fileURLToPath(import.meta.url)); +const repoRoot = resolve(libDir, "..", ".."); +const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js"); + +/** + * Spawn `mcp-inspector --web` (prod, no `--dev`) against the built + * `clients/web/dist` and return handles for readiness + teardown. + * + * @param {object} opts + * @param {string} opts.host + * @param {string} opts.port + * @param {string} opts.token value injected as MCP_INSPECTOR_API_TOKEN + */ +export function startProdWebServer({ host, port, token }) { + const baseUrl = `http://${host}:${port}`; + + const child = spawn(process.execPath, [launcherEntry, "--web"], { + env: { + ...process.env, + CLIENT_PORT: port, + HOST: host, + MCP_INSPECTOR_API_TOKEN: token, + // Don't pop a browser in CI. + MCP_AUTO_OPEN_ENABLED: "false", + }, + stdio: ["ignore", "inherit", "inherit"], + }); + + let exited = false; + let exitCode = null; + child.on("exit", (code) => { + exited = true; + exitCode = code; + }); + + /** + * Poll `GET /` until the server answers with an ok status. Rejects early if + * the launcher exits before serving (so a boot failure surfaces immediately + * instead of after the full timeout). Resolves with the first ok Response. + */ + async function waitForReady({ attempts = 120, intervalMs = 500 } = {}) { + for (let attempt = 0; attempt < attempts; attempt++) { + if (exited) { + throw new Error( + `launcher exited (code ${exitCode}) before serving — see output above`, + ); + } + try { + const res = await fetch(`${baseUrl}/`); + if (res.ok) return res; + } catch { + // not listening yet + } + await delay(intervalMs); + } + throw new Error( + `server did not start within ${(attempts * intervalMs) / 1000}s`, + ); + } + + /** + * A promise that rejects when the launcher process exits — race it against + * page-load work so a mid-flight server death is reported as the real cause + * instead of a downstream render timeout. Never resolves; if the server stays + * up it simply stays pending until the process exits. + */ + function whenChildExits() { + return new Promise((_resolve, reject) => { + const onExit = (code) => + reject( + new Error( + `launcher exited (code ${code ?? exitCode}) mid-run — see output above`, + ), + ); + if (exited) onExit(exitCode); + else child.once("exit", onExit); + }); + } + + return { + baseUrl, + child, + isExited: () => exited, + exitCode: () => exitCode, + waitForReady, + whenChildExits, + stop: () => { + if (!exited) child.kill("SIGTERM"); + }, + }; +} diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index 530495f77..bb05041f7 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -16,51 +16,47 @@ * time: it launches `mcp-inspector --web` (prod, no `--dev`) against the built * `clients/web/dist`, opens the served page in headless Chromium (Playwright, * already a clients/web devDependency for the Storybook tests), and asserts the - * app renders its first meaningful frame (the "Add Servers" control) with **no - * uncaught page errors** — in particular no `node:*` externalization error. + * app renders its first meaningful frame (the "Add Servers" control). + * + * Failure signal: an **uncaught `pageerror`** — that's how the #1612 class + * arrives (Vite's `__vite-browser-external` proxy *throws* on property access). + * A `console.error` is NOT treated as a hard failure by itself, because the + * console is also where Chromium reports benign things a boot smoke shouldn't + * fail on: a failed subresource load (e.g. the Google-Fonts `` in + * index.html on a network-restricted box) or a React key/prop warning. Console + * errors are collected and printed as diagnostics, and only fail the run when + * they carry the externalized-module signature. * * Run from the clients/web directory (`npm run smoke:web:browser` does * `cd clients/web` first) so `import("playwright")` resolves against that - * client's node_modules. Repo-root paths below are derived from import.meta.url, - * so the cwd change doesn't affect which launcher/build tree is exercised. + * client's node_modules. Repo-root paths are derived inside the shared helper + * from import.meta.url, so the cwd change doesn't affect which build tree runs. * * Expects `clients/web/dist` and `clients/launcher/build` to be built first — * the validate / CI ordering guarantees this. */ -import { spawn } from "node:child_process"; import { setTimeout as delay } from "node:timers/promises"; -import { fileURLToPath } from "node:url"; -import { dirname, resolve } from "node:path"; - -const scriptDir = dirname(fileURLToPath(import.meta.url)); -const repoRoot = resolve(scriptDir, ".."); -const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js"); +import { startProdWebServer } from "./lib/prod-web-server.mjs"; const HOST = "127.0.0.1"; -const PORT = process.env.SMOKE_WEB_PORT ?? "6298"; +// Distinct from smoke:web's SMOKE_WEB_PORT so overriding one doesn't make both +// back-to-back smokes bind the same port (→ EADDRINUSE on the second). +const PORT = process.env.SMOKE_WEB_BROWSER_PORT ?? "6298"; const TOKEN = "smoke-web-browser-token"; -const BASE_URL = `http://${HOST}:${PORT}`; -const child = spawn(process.execPath, [launcherEntry, "--web"], { - env: { - ...process.env, - CLIENT_PORT: PORT, - HOST, - MCP_INSPECTOR_API_TOKEN: TOKEN, - // Don't pop a browser in CI. - MCP_AUTO_OPEN_ENABLED: "false", - }, - stdio: ["ignore", "inherit", "inherit"], -}); +// Vite emits `Module "${id}" has been externalized for browser compatibility…` +// where `id` is whatever was imported — `node:fs` OR a bare `fs` (both common +// in transitive deps). Match either so the diagnostic label always attaches. +const EXTERNALIZED = /Module "[^"]+" has been externalized/; -let exited = false; -let exitCode = null; -child.on("exit", (code) => { - exited = true; - exitCode = code; -}); +function labelExternalized(message) { + return EXTERNALIZED.test(message) + ? `Node built-in reached the browser bundle: ${message}` + : message; +} +const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN }); let browser = null; async function shutdown() { @@ -72,7 +68,7 @@ async function shutdown() { } browser = null; } - if (!exited) child.kill("SIGTERM"); + server.stop(); } async function fail(message) { @@ -81,78 +77,109 @@ async function fail(message) { process.exit(1); } -async function waitForServer() { - for (let attempt = 0; attempt < 120; attempt++) { - if (exited) { - throw new Error( - `launcher exited (code ${exitCode}) before serving — see output above`, - ); - } - try { - const res = await fetch(`${BASE_URL}/`); - if (res.ok) return; - } catch { - // not listening yet - } - await delay(500); +async function loadChromium() { + let playwright; + try { + playwright = await import("playwright"); + } catch (err) { + throw new Error( + `could not load Playwright — run \`npx playwright install --with-deps chromium\` (${err instanceof Error ? err.message : String(err)})`, + ); } - throw new Error("server did not start within 60s"); -} - -// A page error whose message mentions a node: built-in being externalized is -// the #1612 signature; treat *any* uncaught page error as a failure, but call -// this class out explicitly since it's the regression this smoke exists for. -function describeError(err) { - const message = err instanceof Error ? err.message : String(err); - if (/Module "node:.*" has been externalized/.test(message)) { - return `Node built-in reached the browser bundle: ${message}`; + try { + return await playwright.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)})`, + ); } - return message; } try { - const { chromium } = await import("playwright"); - await waitForServer(); - - browser = await chromium.launch({ headless: true }); + await server.waitForReady(); + browser = await loadChromium(); const page = await browser.newPage(); + // Uncaught page errors are the hard failure (the #1612 signature lands here). const pageErrors = []; - page.on("pageerror", (err) => pageErrors.push(describeError(err))); + // Console errors are diagnostic unless they carry the externalization + // signature (see the header comment for why they're not a blanket failure). + const consoleErrors = []; + page.on("pageerror", (err) => + pageErrors.push(err instanceof Error ? err.message : String(err)), + ); page.on("console", (msg) => { - if (msg.type() === "error") pageErrors.push(`console.error: ${msg.text()}`); + if (msg.type() === "error") consoleErrors.push(msg.text()); }); - // Token is injected into index.html by the prod server, so a bare `/` load - // authenticates without a query param. - const response = await page.goto(BASE_URL, { - waitUntil: "domcontentloaded", - timeout: 30_000, - }); - if (!response || !response.ok()) { + const render = async () => { + // Token is injected into index.html by the prod server, so a bare `/` load + // authenticates without a query param. + const response = await page.goto(server.baseUrl, { + waitUntil: "domcontentloaded", + timeout: 30_000, + }); + if (!response || !response.ok()) { + throw new Error( + `GET / returned HTTP ${response ? response.status() : "no response"}`, + ); + } + // First meaningful frame: the always-present "Add Servers" control. + await page + .getByRole("button", { name: /Add Servers/ }) + .waitFor({ state: "visible", timeout: 30_000 }); + // Settle window: let lazily-evaluated chunks that throw a tick after first + // paint surface before we assert a clean boot. networkidle is best-effort + // (the Google-Fonts request may never idle on a restricted network). + await page + .waitForLoadState("networkidle", { timeout: 5_000 }) + .catch(() => {}); + await delay(500); + }; + + // Race the render against launcher death so a mid-load server crash is + // reported as the real cause instead of a 30s render timeout. + try { + await Promise.race([server.whenChildExits(), render()]); + } catch (err) { + const diagnostics = [ + ...pageErrors.map(labelExternalized), + ...consoleErrors.map((m) => `console: ${labelExternalized(m)}`), + ]; await fail( - `GET / returned HTTP ${response ? response.status() : "no response"}`, + `${err instanceof Error ? err.message : String(err)}${ + diagnostics.length + ? ` — page diagnostics: ${diagnostics.join("; ")}` + : "" + }`, ); } - // First meaningful frame: the always-present "Add Servers" control. - await page - .getByRole("button", { name: /Add Servers/ }) - .waitFor({ state: "visible", timeout: 30_000 }) - .catch(async () => { - await fail( - `app did not render the "Add Servers" control within 30s${ - pageErrors.length ? ` — page errors: ${pageErrors.join("; ")}` : "" - }`, - ); - }); + // Hard failures: any uncaught page error, plus console errors that carry the + // externalization signature. + const hardErrors = [ + ...pageErrors.map(labelExternalized), + ...consoleErrors + .filter((m) => EXTERNALIZED.test(m)) + .map((m) => `console: ${labelExternalized(m)}`), + ]; + if (hardErrors.length > 0) { + await fail( + `app logged uncaught / externalization errors: ${hardErrors.join("; ")}`, + ); + } - if (pageErrors.length > 0) { - await fail(`app rendered but logged page errors: ${pageErrors.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) => !EXTERNALIZED.test(m)); + if (benignConsole.length > 0) { + console.log( + `smoke:web:browser note — ${benignConsole.length} non-fatal console error(s): ${benignConsole.join("; ")}`, + ); } console.log( - `smoke:web:browser OK — app booted at ${BASE_URL}, rendered "Add Servers" with no page errors`, + `smoke:web:browser OK — app booted at ${server.baseUrl}, rendered "Add Servers" with no uncaught page errors`, ); await shutdown(); process.exit(0); diff --git a/scripts/smoke-web.mjs b/scripts/smoke-web.mjs index ca0ee37cc..143ba925d 100644 --- a/scripts/smoke-web.mjs +++ b/scripts/smoke-web.mjs @@ -9,15 +9,19 @@ * returns HTTP 200 with the injected auth-token global. It exits non-zero * (failing CI / `npm run validate`) on any failure, then shuts the server down. * + * This checks the served HTML only — it does not execute the bundle. The + * companion `smoke:web:browser` (scripts/smoke-web-browser.mjs, #1615) boots the + * same server and actually runs the app in headless Chromium; both share the + * spawn/readiness helper in ./lib/prod-web-server.mjs. + * * Expects `clients/web/dist` and `clients/launcher/build` to be built first — - * the validate / CI ordering guarantees this, so the readiness wait below is - * sized for an already-built `dist`. (If `dist` happened to be missing, the - * launcher's build-on-demand path would build it on startup, which a cold - * `vite build` could push past that wait — not a scenario this script targets.) + * the validate / CI ordering guarantees this, so the readiness wait is sized for + * an already-built `dist`. (If `dist` happened to be missing, the launcher's + * build-on-demand path would build it on startup, which a cold `vite build` + * could push past that wait — not a scenario this script targets.) */ -import { spawn } from "node:child_process"; -import { setTimeout as delay } from "node:timers/promises"; +import { startProdWebServer } from "./lib/prod-web-server.mjs"; const HOST = "127.0.0.1"; const PORT = process.env.SMOKE_WEB_PORT ?? "6299"; @@ -25,59 +29,17 @@ const TOKEN = "smoke-web-token"; // Mirrors INSPECTOR_API_TOKEN_GLOBAL in core/mcp/remote/constants.ts; kept as a // literal because this plain .mjs script can't import the TS source. const TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__"; -const BASE_URL = `http://${HOST}:${PORT}`; - -const child = spawn( - process.execPath, - ["clients/launcher/build/index.js", "--web"], - { - env: { - ...process.env, - CLIENT_PORT: PORT, - HOST, - MCP_INSPECTOR_API_TOKEN: TOKEN, - // Don't pop a browser in CI. - MCP_AUTO_OPEN_ENABLED: "false", - }, - stdio: ["ignore", "inherit", "inherit"], - }, -); -let exited = false; -let exitCode = null; -child.on("exit", (code) => { - exited = true; - exitCode = code; -}); - -function shutdown() { - if (!exited) child.kill("SIGTERM"); -} +const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN }); function fail(message) { console.error(`smoke:web FAILED — ${message}`); - shutdown(); + server.stop(); process.exit(1); } -async function fetchRoot() { - for (let attempt = 0; attempt < 120; attempt++) { - if (exited) { - throw new Error( - `launcher exited (code ${exitCode}) before serving — see output above`, - ); - } - try { - return await fetch(`${BASE_URL}/`); - } catch { - await delay(500); - } - } - throw new Error("server did not start within 60s"); -} - try { - const res = await fetchRoot(); + const res = await server.waitForReady(); if (res.status !== 200) { fail(`GET / returned HTTP ${res.status}, expected 200`); } @@ -91,9 +53,9 @@ try { fail("served HTML is missing the injected auth-token value"); } console.log( - `smoke:web OK — GET / => 200 with injected ${TOKEN_GLOBAL} at ${BASE_URL}`, + `smoke:web OK — GET / => 200 with injected ${TOKEN_GLOBAL} at ${server.baseUrl}`, ); - shutdown(); + server.stop(); process.exit(0); } catch (err) { fail(err instanceof Error ? err.message : String(err)); From d12c3a69b48f07d4d8490be37a5fa14ddaf39207 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:14:05 -0400 Subject: [PATCH 3/8] ci: fix playwright resolution in the browser smoke (CI failure) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bare `import("playwright")` resolved relative to the script's own directory (scripts/), not the cwd — so `cd clients/web` in the npm script never made it resolvable. It only passed locally because an ancestor node_modules happened to carry playwright; CI has none, so it failed with "Cannot find package 'playwright'". Resolve it explicitly from clients/web via createRequire, independent of cwd and ancestor dirs. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/smoke-web-browser.mjs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index bb05041f7..72bb24b2c 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -27,18 +27,31 @@ * errors are collected and printed as diagnostics, and only fail the run when * they carry the externalized-module signature. * - * Run from the clients/web directory (`npm run smoke:web:browser` does - * `cd clients/web` first) so `import("playwright")` resolves against that - * client's node_modules. Repo-root paths are derived inside the shared helper - * from import.meta.url, so the cwd change doesn't affect which build tree runs. + * 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, and 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 or ancestor dirs. * * 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"; +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 HOST = "127.0.0.1"; // Distinct from smoke:web's SMOKE_WEB_PORT so overriding one doesn't make both // back-to-back smokes bind the same port (→ EADDRINUSE on the second). @@ -78,16 +91,16 @@ async function fail(message) { } async function loadChromium() { - let playwright; + let chromium; try { - playwright = await import("playwright"); + ({ chromium } = requireFromWeb("playwright")); } catch (err) { throw new Error( - `could not load Playwright — run \`npx playwright install --with-deps chromium\` (${err instanceof Error ? err.message : String(err)})`, + `could not load Playwright from clients/web — run \`npx playwright install --with-deps chromium\` (${err instanceof Error ? err.message : String(err)})`, ); } try { - return await playwright.chromium.launch({ headless: true }); + 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)})`, From 45e828e92415106551ae3cac3600ca2ca0258845 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:16:01 -0400 Subject: [PATCH 4/8] ci: address second review on the prod-web-server helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - waitForReady now records the last non-ok status and includes it in the timeout message, so a server that boots but persistently answers 500 (broken dist / injection failure) reports "server did not start … (last response: HTTP 500)" instead of a bare timeout that points at the wrong thing. Keeps polling on non-ok (a warming server may answer 503). - Add a child 'error' handler so a spawn-level failure surfaces as the smoke's FAILED line instead of an uncaught raw stack; readiness and the child-exit race both report it. - Drop the unused isExited/exitCode/child exports from the helper API. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/lib/prod-web-server.mjs | 60 ++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index a7d2b2b9c..1d394ef1b 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/scripts/lib/prod-web-server.mjs @@ -7,8 +7,8 @@ * lives here once instead of being copy-pasted (and drifting) in each script. * * Repo-root paths are derived from import.meta.url, so a caller's cwd (e.g. - * `smoke:web:browser` does `cd clients/web` first so `import("playwright")` - * resolves) doesn't affect which launcher/build tree is exercised. + * `smoke:web:browser` does `cd clients/web` first so its `npx playwright + * install` finds the local bin) doesn't affect which launcher/build tree runs. */ import { spawn } from "node:child_process"; @@ -46,60 +46,74 @@ export function startProdWebServer({ host, port, token }) { let exited = false; let exitCode = null; + let spawnError = null; child.on("exit", (code) => { exited = true; exitCode = code; }); + // A spawn-level failure (e.g. a missing launcher entry) emits 'error', not + // 'exit'; without a listener Node throws it uncaught with a raw stack instead + // of the smoke's `… FAILED —` line. Capture it so readiness surfaces it. + child.on("error", (err) => { + exited = true; + spawnError = err; + }); + + function bootFailure(phase) { + return new Error( + spawnError + ? `launcher failed to spawn: ${spawnError.message}` + : `launcher exited (code ${exitCode}) ${phase} — see output above`, + ); + } /** - * Poll `GET /` until the server answers with an ok status. Rejects early if - * the launcher exits before serving (so a boot failure surfaces immediately - * instead of after the full timeout). Resolves with the first ok Response. + * Poll `GET /` until the server answers with an ok status. Keeps polling on a + * non-ok status (a warming server may legitimately answer 503), but records + * the last one so a server that boots yet never returns ok (e.g. a broken + * `dist` answering 500) reports that status instead of a bare timeout. Rejects + * early if the launcher exits before serving. Resolves with the first ok + * Response. */ async function waitForReady({ attempts = 120, intervalMs = 500 } = {}) { + let lastStatus = null; for (let attempt = 0; attempt < attempts; attempt++) { - if (exited) { - throw new Error( - `launcher exited (code ${exitCode}) before serving — see output above`, - ); - } + if (exited) throw bootFailure("before serving"); try { const res = await fetch(`${baseUrl}/`); if (res.ok) return res; + lastStatus = res.status; } catch { // not listening yet } await delay(intervalMs); } throw new Error( - `server did not start within ${(attempts * intervalMs) / 1000}s`, + `server did not start within ${(attempts * intervalMs) / 1000}s${ + lastStatus !== null ? ` (last response: HTTP ${lastStatus})` : "" + }`, ); } /** - * A promise that rejects when the launcher process exits — race it against + * A promise that rejects when the launcher process dies — race it against * page-load work so a mid-flight server death is reported as the real cause * instead of a downstream render timeout. Never resolves; if the server stays * up it simply stays pending until the process exits. */ function whenChildExits() { return new Promise((_resolve, reject) => { - const onExit = (code) => - reject( - new Error( - `launcher exited (code ${code ?? exitCode}) mid-run — see output above`, - ), - ); - if (exited) onExit(exitCode); - else child.once("exit", onExit); + const onDeath = () => reject(bootFailure("mid-run")); + if (exited) onDeath(); + else { + child.once("exit", onDeath); + child.once("error", onDeath); + } }); } return { baseUrl, - child, - isExited: () => exited, - exitCode: () => exitCode, waitForReady, whenChildExits, stop: () => { From b97b407b2c8bd73f4dcf243fe86fd9898b141aca Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:32:42 -0400 Subject: [PATCH 5/8] ci: reframe browser smoke around uncaught pageerror; fix stop() edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-review findings. 1. The `Module "…" has been externalized` string can't fire against a Vite 8 *prod* bundle — verified end-to-end against the installed vite@8.0.0: a node: import reaching the browser graph is a BUILD-time warning, and the shipped stub is a silent `module.exports = {}`. The real #1612 manifestation is the first *call* into that empty stub throwing a TypeError at module init (confirmed: injecting a used `node:fs` import fails the smoke with "readFileSync is not a function"; an unused one ships {} and is harmless). So the signature machinery was dead against the bundle it runs on — dropped it. The smoke now asserts exactly what it verifies: the prod bundle boots and paints with no uncaught page error. Reworded the script header, the AGENTS.md bullet, and the README row to match. Console errors stay diagnostic-only. 2. The helper's child 'error' handler set `exited = true`, but Node emits 'error' for a failed kill/send too (child still alive) — so `stop()` (guarded on `exited`) would skip the SIGTERM and orphan the launcher on the port. Track the error separately (`childError`); `bootFailed()` = exited || childError, while `stop()` still SIGTERMs a live child. Docs: fixed the stale AGENTS.md claim that `cd clients/web` makes `import("playwright")` resolve (createRequire does, cwd-independently; the cd is only for `npx playwright install`), and noted the shared scripts/lib/prod-web-server.mjs helper on both smoke bullets. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 4 +- README.md | 2 +- scripts/lib/prod-web-server.mjs | 25 ++++++---- scripts/smoke-web-browser.mjs | 88 +++++++++++++-------------------- 4 files changed, 53 insertions(+), 66 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 72e998504..d06fadb78 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -218,8 +218,8 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only CI step left out (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. -- `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. -- `smoke: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 page errors** — in particular no `Module "node:*" has been externalized for browser compatibility`. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle (which crashes the app to a blank page at runtime, e.g. #1612) slipped through it; this smoke catches that regression as a *class*. The npm script runs `cd clients/web && npx playwright install chromium` first so `import("playwright")` resolves and chromium is present (a no-op when already installed); repo-root paths inside the script come from `import.meta.url`, so the cwd change doesn't affect which build tree is exercised. +- `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`, so the two can't drift. +- `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 page 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 `pageerror` the smoke fails on. The literal `Module "…" has been externalized` text is a **build-time** warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it; and an externalized import that is never *called* ships a harmless `{}` and is invisible here by design. Console errors are printed as diagnostics, not failures (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: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. diff --git a/README.md b/README.md index 71a74003f..7353c32cd 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs `validate:core` (the shared `core/` `format:check` + `lint` gate) first, then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `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 a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no `node:*` in the browser bundle). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught page error — how a Node built-in reaching the browser bundle manifests). | | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index 1d394ef1b..254410efd 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/scripts/lib/prod-web-server.mjs @@ -46,23 +46,28 @@ export function startProdWebServer({ host, port, token }) { let exited = false; let exitCode = null; - let spawnError = null; + let childError = null; child.on("exit", (code) => { exited = true; exitCode = code; }); - // A spawn-level failure (e.g. a missing launcher entry) emits 'error', not - // 'exit'; without a listener Node throws it uncaught with a raw stack instead - // of the smoke's `… FAILED —` line. Capture it so readiness surfaces it. + // A child 'error' event covers a spawn failure (e.g. a missing launcher + // entry) but ALSO a failed kill / failed send where the process is still + // alive. Without a listener Node throws it uncaught with a raw stack instead + // of the smoke's `… FAILED —` line. Record it, but deliberately do NOT set + // `exited` — otherwise `stop()` (which guards on `exited`) would skip the + // SIGTERM and orphan a still-running launcher on the port. child.on("error", (err) => { - exited = true; - spawnError = err; + childError = err; }); + // Boot has failed if the process exited or emitted an error. + const bootFailed = () => exited || childError !== null; + function bootFailure(phase) { return new Error( - spawnError - ? `launcher failed to spawn: ${spawnError.message}` + childError + ? `launcher process error: ${childError.message}` : `launcher exited (code ${exitCode}) ${phase} — see output above`, ); } @@ -78,7 +83,7 @@ export function startProdWebServer({ host, port, token }) { async function waitForReady({ attempts = 120, intervalMs = 500 } = {}) { let lastStatus = null; for (let attempt = 0; attempt < attempts; attempt++) { - if (exited) throw bootFailure("before serving"); + if (bootFailed()) throw bootFailure("before serving"); try { const res = await fetch(`${baseUrl}/`); if (res.ok) return res; @@ -104,7 +109,7 @@ export function startProdWebServer({ host, port, token }) { function whenChildExits() { return new Promise((_resolve, reject) => { const onDeath = () => reject(bootFailure("mid-run")); - if (exited) onDeath(); + if (bootFailed()) onDeath(); else { child.once("exit", onDeath); child.once("error", onDeath); diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index 72bb24b2c..af9eff60e 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -6,35 +6,38 @@ * HTML with the injected auth token — it never executes the React app, so a * regression that only manifests when the bundle *runs* slips through. The * canonical example (#1612): the browser bundle transitively value-imported a - * Node-only module, pulling `node:*` built-ins into the browser build and - * crashing the app to a blank page with - * `Module "node:*" has been externalized for browser compatibility`. - * Unit/integration tests run in node / happy-dom, never a real browser bundle, - * so none of them caught it. + * Node-only module (`store-io` → `atomically` → `stubborn-fs` → `node:process`), + * pulling a Node built-in into the browser graph and crashing the app to a blank + * page at runtime. Unit/integration tests run in node / happy-dom, never a real + * browser bundle, so none of them caught it. * - * This script closes that gap as a *class* of bug rather than one import at a - * time: it launches `mcp-inspector --web` (prod, no `--dev`) against the built - * `clients/web/dist`, opens the served page in headless Chromium (Playwright, - * already a clients/web devDependency for the Storybook tests), and asserts the - * app renders its first meaningful frame (the "Add Servers" control). + * The assertion: the prod bundle **boots and paints its first meaningful frame + * (the "Add Servers" control) with no uncaught page error.** That is how a + * Node-only module reaching the browser bundle actually manifests under Vite: + * the excluded module is replaced by 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 — an uncaught `pageerror` here. * - * Failure signal: an **uncaught `pageerror`** — that's how the #1612 class - * arrives (Vite's `__vite-browser-external` proxy *throws* on property access). - * A `console.error` is NOT treated as a hard failure by itself, because the - * console is also where Chromium reports benign things a boot smoke shouldn't - * fail on: a failed subresource load (e.g. the Google-Fonts `` in - * index.html on a network-restricted box) or a React key/prop warning. Console - * errors are collected and printed as diagnostics, and only fail the run when - * they carry the externalized-module signature. + * What this does NOT rely on: the literal `Module "…" has been externalized` + * string. Under Vite 8 that is a **build-time** warning (surfaced by + * `vite build`, i.e. `npm run build`), not a runtime message — the shipped stub + * is a silent `module.exports = {}`. So the browser never sees that string; the + * load-bearing signal is the uncaught `TypeError`. (Corollary: an externalized + * import that is never *called* ships a harmless empty object and is invisible + * to this smoke by design — an unused Node import doesn't crash the app.) + * + * `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 subresource + * load (e.g. the Google-Fonts `` in index.html on a network-restricted + * box) or a React key/prop warning. Console errors are printed as diagnostics. * * 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, and 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 or ancestor dirs. + * 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. * * Expects `clients/web/dist` and `clients/launcher/build` to be built first — * the validate / CI ordering guarantees this. @@ -58,17 +61,6 @@ const HOST = "127.0.0.1"; const PORT = process.env.SMOKE_WEB_BROWSER_PORT ?? "6298"; const TOKEN = "smoke-web-browser-token"; -// Vite emits `Module "${id}" has been externalized for browser compatibility…` -// where `id` is whatever was imported — `node:fs` OR a bare `fs` (both common -// in transitive deps). Match either so the diagnostic label always attaches. -const EXTERNALIZED = /Module "[^"]+" has been externalized/; - -function labelExternalized(message) { - return EXTERNALIZED.test(message) - ? `Node built-in reached the browser bundle: ${message}` - : message; -} - const server = startProdWebServer({ host: HOST, port: PORT, token: TOKEN }); let browser = null; @@ -113,10 +105,11 @@ try { browser = await loadChromium(); const page = await browser.newPage(); - // Uncaught page errors are the hard failure (the #1612 signature lands here). + // Uncaught page errors are the hard failure — a Node-only module reaching the + // browser bundle surfaces here as a TypeError when its empty stub is called. const pageErrors = []; - // Console errors are diagnostic unless they carry the externalization - // signature (see the header comment for why they're not a blanket failure). + // Console errors are diagnostic only (see the header comment for why they're + // not a blanket failure). const consoleErrors = []; page.on("pageerror", (err) => pageErrors.push(err instanceof Error ? err.message : String(err)), @@ -156,8 +149,8 @@ try { await Promise.race([server.whenChildExits(), render()]); } catch (err) { const diagnostics = [ - ...pageErrors.map(labelExternalized), - ...consoleErrors.map((m) => `console: ${labelExternalized(m)}`), + ...pageErrors, + ...consoleErrors.map((m) => `console: ${m}`), ]; await fail( `${err instanceof Error ? err.message : String(err)}${ @@ -168,26 +161,15 @@ try { ); } - // Hard failures: any uncaught page error, plus console errors that carry the - // externalization signature. - const hardErrors = [ - ...pageErrors.map(labelExternalized), - ...consoleErrors - .filter((m) => EXTERNALIZED.test(m)) - .map((m) => `console: ${labelExternalized(m)}`), - ]; - if (hardErrors.length > 0) { - await fail( - `app logged uncaught / externalization errors: ${hardErrors.join("; ")}`, - ); + if (pageErrors.length > 0) { + await fail(`app logged uncaught page error(s): ${pageErrors.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) => !EXTERNALIZED.test(m)); - if (benignConsole.length > 0) { + if (consoleErrors.length > 0) { console.log( - `smoke:web:browser note — ${benignConsole.length} non-fatal console error(s): ${benignConsole.join("; ")}`, + `smoke:web:browser note — ${consoleErrors.length} non-fatal console error(s): ${consoleErrors.join("; ")}`, ); } From 438232237b0688d6fd461999c218f8e8838d2220 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:43:29 -0400 Subject: [PATCH 6/8] ci: also hard-fail the async half of the crash class (fourth review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit page.on("pageerror") fires for uncaught SYNCHRONOUS exceptions only. The async twin of the #1612 shape — the same empty-stub TypeError reached through an await/.then(), or a failed dynamic import (this app lazy-loads chunks) — lands on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module`, which were diagnostic-only, so the smoke would have passed. Hard-fail console errors matching /^Uncaught\b|Failed to fetch dynamically imported module/; every other console error stays a printed non-fatal note. The prefixes are unambiguous (a font/CDN miss reads "Failed to load resource: net::ERR_…"; React warnings never start with "Uncaught"), so this can't reintroduce the flake the earlier round fixed. Verified: an unhandled rejection at module init now fails the smoke where it previously wouldn't. Also prod-scope the AGENTS.md note that the "externalized" string isn't a runtime message (under `npm run dev` Vite's stub is a Proxy that console.warns it), and document both the sync and async failure signals. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 2 +- scripts/smoke-web-browser.mjs | 48 ++++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d06fadb78..df3da320c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -219,7 +219,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only CI step left out (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with `smoke:web:browser`, so the two can't drift. -- `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 page 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 `pageerror` the smoke fails on. The literal `Module "…" has been externalized` text is a **build-time** warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it; and an externalized import that is never *called* ships a harmless `{}` and is invisible here by design. Console errors are printed as diagnostics, not failures (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: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: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. diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index af9eff60e..bebbc365d 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -26,10 +26,21 @@ * import that is never *called* ships a harmless empty object and is invisible * to this smoke by design — an unused Node import doesn't crash the app.) * - * `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 subresource - * load (e.g. the Google-Fonts `` in index.html on a network-restricted - * box) or a React key/prop warning. Console errors are printed as diagnostics. + * 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. + * + * 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 + * 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 + * font/CDN miss reads `Failed to load resource: net::ERR_…` and a React warning + * 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 @@ -61,6 +72,12 @@ 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 }); let browser = null; @@ -105,11 +122,12 @@ try { browser = await loadChromium(); const page = await browser.newPage(); - // Uncaught page errors are the hard failure — a Node-only module reaching the - // browser bundle surfaces here as a TypeError when its empty stub is called. + // 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 (see the header comment for why they're - // not a blanket failure). + // 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)), @@ -161,15 +179,21 @@ try { ); } - if (pageErrors.length > 0) { - await fail(`app logged uncaught page error(s): ${pageErrors.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("; ")}`, + ); } // Non-fatal console errors: surface them so a real problem isn't invisible, // without failing the smoke on benign subresource/warning noise. - if (consoleErrors.length > 0) { + const benignConsole = consoleErrors.filter((m) => !FATAL_CONSOLE.test(m)); + if (benignConsole.length > 0) { console.log( - `smoke:web:browser note — ${consoleErrors.length} non-fatal console error(s): ${consoleErrors.join("; ")}`, + `smoke:web:browser note — ${benignConsole.length} non-fatal console error(s): ${benignConsole.join("; ")}`, ); } From 9c576d72530e81463e4b6810d90b9633be920888 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:52:00 -0400 Subject: [PATCH 7/8] docs: reword smoke success line + README to two-channel wording (fifth review) After the async hard-failure signature, the assertion spans both the pageerror (sync) and console (unhandled rejection / failed dynamic import) channels. The OK console line and the README smoke row still said "page error"; match the two-channel wording already in the AGENTS.md bullet. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- README.md | 2 +- scripts/smoke-web-browser.mjs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7353c32cd..709637e5f 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `npm run validate` | Runs `validate:core` (the shared `core/` `format:check` + `lint` gate) first, then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `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 a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught page error — how a Node built-in reaching the browser bundle manifests). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium 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). | | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index bebbc365d..7951a2114 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -198,7 +198,7 @@ try { } console.log( - `smoke:web:browser OK — app booted at ${server.baseUrl}, rendered "Add Servers" with no uncaught page errors`, + `smoke:web:browser OK — app booted at ${server.baseUrl}, rendered "Add Servers" with no uncaught errors (sync page error or unhandled rejection)`, ); await shutdown(); process.exit(0); From 0c271598e0b6d95b6173f2c6a7ed4f20a65a4701 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 18:59:39 -0400 Subject: [PATCH 8/8] ci: correct the playwright-package-resolution hint (sixth review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch around requireFromWeb("playwright") advised `playwright install --with-deps chromium`, but that installs browser binaries, not the npm package — and the npm script already ran `playwright install` a step earlier. A failure there means the package isn't resolvable from clients/web, so the actionable fix is `npm install` at the repo root (the postinstall cascade installs clients/web's devDependencies). The chromium.launch() catch keeps its --with-deps advice (correct for the missing-system-libraries case). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/smoke-web-browser.mjs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index 7951a2114..1188865cf 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -104,8 +104,12 @@ async function loadChromium() { 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 load Playwright from clients/web — run \`npx playwright install --with-deps chromium\` (${err instanceof Error ? err.message : String(err)})`, + `could not resolve the Playwright package from clients/web — run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, ); } try {