diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 34855be7..de5ff9f4 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -102,10 +102,10 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo ## Gates and PR hygiene -- `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. +- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook; the Firefox step is local-only and not mirrored in GitHub CI). `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). **CI runs Chromium only**; the others are an on-demand tool, deliberately not a gate (#2086 — a Firefox job was trialled and removed for never disagreeing with Chromium). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check. +- **The headless web smokes are engine-parameterized, not Chromium-only.** `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit` take their engine from `SMOKE_BROWSER` (chromium — the default — firefox, webkit). **GitHub CI runs Chromium only; Firefox runs in the local pre-push gate** via `smoke:web:firefox` (#2086 — a CI job was trialled and removed for never disagreeing with Chromium across a dozen runs, and moved into `npm run ci` instead). They exist because the MCP Apps sandbox is built out of the primitives that diverge between engines: `srcdoc` CSP inheritance, nested sandboxed iframes, `Permissions-Policy`, cross-frame `postMessage`. Nothing else covers that — `sandbox-csp.test.ts` asserts a policy *string*, and no Storybook story reaches the sandbox at all. Flag a new browser-driven script that launches Playwright itself instead of going through `scripts/lib/headless-browser.mjs`, an unrecognized-engine path that falls back to Chromium rather than failing (it would claim coverage that never ran), or a launch-failure message naming the wrong engine. `pack:verify` is pinned to Chromium on purpose — it is a packaging check. - **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. diff --git a/AGENTS.md b/AGENTS.md index 54706314..f1e79934 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -886,7 +886,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab ### Mandatory pre-push gate - ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. -- **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `verify:bundle-externals` (the #2067 must-not-bundle gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). +- **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `verify:bundle-externals` (the #2067 must-not-bundle gate) → `smoke` → **`smoke:web:firefox`** (the three browser-driven smokes again under Firefox — #2086; see below) → Storybook play-function tests (installs Playwright chromium if needed). Note `smoke:web:firefox` is the one step that is **not** in GitHub CI: it is a superset, not a mirror, and deliberately so. It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). - ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency that reaches a single `tsc` program from two installs resolves to two different versions across them), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script 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`. @@ -895,7 +895,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **One version per install-crossing dependency (#1896).** Because v2 is not a workspace, the root and each `clients/*` carry their own `node_modules` — and a client's `tsconfig.test.json` compiles first-party sources that live *outside* the client (`test-servers/src`, `core/`), which resolve their dependencies from the **root** install while the client's own sources resolve from the client install. So the same package can appear **twice in one `tsc` program**. At the same version that duplication is harmless; on a skew, TypeScript must relate two structurally-distinct declarations of the same type. For a deeply recursive-generic surface that is exponential: zod `4.3.6` (root) against zod `4.4.3` (`clients/web`) made `clients/web`'s `tsc -b` exhaust the 4GB default heap outright via `TS2589 Type instantiation is excessively deep`, because every `@modelcontextprotocol/*` schema is built out of zod generics. **Raising the heap with `--max-old-space-size` hides this class rather than fixing it — align the versions instead.** `npm run verify:dep-lockstep` (`scripts/verify-dep-lockstep.mjs`, in `validate`) is the durable guard: it **derives** the candidate set from **what actually enters each `tsc` program** (#1965) — every client tsconfig project is listed with `tsc --listFilesOnly` through the shared `scripts/lib/tsc-program.mjs` helper (the same machinery `verify:typecheck-coverage` reads a program through, so the two guards can't disagree about what one contains), each resolved `node_modules` file is mapped to its owning install and package, and a package reaching **one** program from **two** installs is a candidate. That is precisely the set that can put two structurally-distinct copies of a type in front of one checker — and it needs no editing when a new dependency arrives. It replaced a derivation that read the packages the shared sources named *directly*, which could not see one whose declarations arrive only through another package's `.d.ts`: `@modelcontextprotocol/sdk` is never written in first-party code (the shared sources import the split `@modelcontextprotocol/client|core|…`) yet 16 of its `.d.ts` files land in `clients/web`'s test program, so a second copy under `clients/web/node_modules` skewed unseen. Two properties are worth knowing: a package present in two installs but reached from only one in a given program is correctly **not** a candidate, and TypeScript's package-identity redirect collapses two copies at the *same* name@version (so an aligned package's own transitive dependencies load once) — the redirect stops applying the moment they skew, which is exactly when the guard needs to see both. Versions still come from the committed lockfiles, but from the entry for the **exact install path the program resolved** (`node_modules/zod`, `node_modules/a/node_modules/zod`) rather than from the install's top-level entry: a *nested* duplicate inside one install is still not a candidate on its own — folding it onto its outermost install is what keeps the set small — but once a program has loaded one, pricing it from a top-level entry that may be absent or differently versioned would let a real pair pass (Copilot). Only the installs that actually **met in one program** are compared, so a third install's copy that no program loads beside another is not evidence of anything. Any co-occurrence whose copies disagree fails, **deny-by-default**; a resolved copy with no lockfile entry fails too, since the tree and the lockfile then disagree about what was loaded. The escape hatch is `TOLERATED_SKEW` in that file, an allowlist of *names* (not version pairs, so an ordinary patch float doesn't churn it), each entry carrying why that package's types can't blow up; it is **empty today** — the four names it used to carry (`react`, `hono`, `jose`, `@modelcontextprotocol/ext-apps`) were admitted under the old derivation and none is a candidate under this one, so each would be a rationale for a skew that cannot occur. **Being listed is not a blanket exemption** — it tolerates skew only *within a major version*, since a rationale about patch-level differences says nothing about a React 18-vs-19 split, where the type surface itself changes; a cross-major skew fails even for a listed package. **When bumping a dependency that the shared sources pull in, bump it in every install that declares it** — that's the root plus whichever clients list it, not all four unconditionally (launcher declares no zod, for instance, and a package absent from an install can't skew, so the guard ignores it there). Don't add a dependency to a client just to satisfy this. Its pure helpers are unit-tested via `test:scripts` (its own, plus `scripts/lib/tsc-program.test.mjs` for the shared derivation), and it vouches — with `verify:format-coverage` and `verify:typecheck-coverage` — that its siblings are still wired into `validate`. It runs the same `tsc --listFilesOnly` pass its sibling does, in its own process, costing ~14s: the listing is deliberately **not** cached to disk between the two, because a fingerprint that missed an input would make a guard measure a program that no longer exists and pass on a real miss. - **`npm run verify:bundle-externals`** runs after `verify:build-gate` in `npm run ci`. It asserts that no package a client declares `external` was inlined into that client's bundle anyway — the must-not-bundle invariant above (#2067). It reads `clients/*/build`, so it needs a build to have run (`validate` provides one). - **`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` / `smoke:web:app` / `smoke:web:elicit`, 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 `verify:build-gate` (the #1769 build gate — see below), then `verify:bundle-externals` (the #2067 must-not-bundle gate), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). +- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`, as is `smoke:web:firefox` (the same browser-driven smokes again under Firefox, #2086 — CI does not run that step). It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser` / `smoke:web:app` / `smoke:web:elicit`, 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 `verify:build-gate` (the #1769 build gate — see below), then `verify:bundle-externals` (the #2067 must-not-bundle gate), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (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`, `smoke:web:app` and `smoke:web:elicit`**, so the four can't drift. @@ -908,9 +908,17 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - `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 are engine-parameterized; CI runs Chromium only (#2086) +### The web smokes are engine-parameterized; Firefox is in the pre-push gate, not CI (#2086) -**`smoke:web:browser`, `smoke:web:app` and `smoke:web:elicit` support Chromium, Firefox and WebKit; `SMOKE_BROWSER` picks one, unset means `chromium`.** **CI runs Chromium only** — the other engines are an on-demand tool, not a gate (see the CI bullet for why). `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. +**`smoke:web:browser`, `smoke:web:app` and `smoke:web:elicit` support Chromium, Firefox and WebKit; `SMOKE_BROWSER` picks one, unset means `chromium`.** + +```bash +npm run smoke:web:firefox # part of `npm run ci` +SMOKE_BROWSER=webkit npm run smoke:web:engine # any engine, on demand +SMOKE_BROWSER=firefox npm run smoke:web:app # one smoke, one engine +``` + +**Firefox runs in the local pre-push gate (`npm run ci`) and NOT in GitHub CI.** That split is the whole design — see the gate bullet below. `ENGINE_SMOKES` in `scripts/run-engine-smokes.mjs` is the single list of which smokes are engine-sensitive; add a fourth there and every engine picks it up. `smoke:web:firefox` passes the engine as an argument rather than relying on the ambient `SMOKE_BROWSER`, so the gate cannot be silently redirected to another engine by a stray variable — verified, an explicit argument beats it. 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. @@ -918,7 +926,8 @@ Everything about the selection lives in **`scripts/lib/headless-browser.mjs`** - **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 is Chromium-only, deliberately, and a Firefox job was trialled and removed.** The job was cheap — ~2 minutes, parallel with the 15-minute `build` job, so zero added wall-clock — and it still did not earn its place: across a dozen runs Firefox never once disagreed with Chromium, so it spent runner time and carried a real flake surface (`playwright install --with-deps` runs `apt-get update`, which fails whenever a third-party repo in the runner image breaks) to re-confirm a result already in hand. **Don't re-add it on the argument that cross-engine coverage is good in principle** — that argument was already accepted, and it is what the on-demand path serves. Re-add it on evidence: an actual cross-engine regression that a gate would have caught. It is a one-job diff when that happens. +- **Firefox is gated locally, not in CI, and that asymmetry is deliberate.** A GitHub Actions job was trialled and removed. It was cheap — ~2 minutes, parallel with the 15-minute `build` job, so zero added wall-clock — but across a dozen runs Firefox never once disagreed with Chromium, so it spent runner minutes on every push, from every branch, carrying a real flake surface (`playwright install --with-deps` runs `apt-get update`, which fails whenever a third-party repo in the runner image breaks) to re-confirm a result already in hand. Moving it into `npm run ci` keeps the check where a human is about to push a change they can still reason about, and pays for it once rather than on every push. **Don't re-add the CI job on the argument that cross-engine coverage is good in principle** — that argument was accepted, and the pre-push gate is what serves it. Re-add it on evidence: a cross-engine regression that reached `v2/main` because someone skipped the gate. +- **This makes `npm run ci` a strict superset of GitHub CI rather than a mirror of it.** That was already the direction (`smoke:tui` self-skips on CI and runs only locally); Firefox is the second such step. The invariant that still holds, and the one that matters, is the useful direction: **passing `npm run ci` locally means CI's gates will pass.** - **`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 fails the two App smokes**, for reasons nobody has identified — the failure stage even varies between runs. The investigation was dropped as not worth the effort once it became clear it does not reproduce in real Safari, and that an isolated repro of the suspected mechanism did not reproduce it under Playwright's WebKit either. Treat a WebKit failure as unexplained rather than as a defect until someone has actually looked. - ⚠️ **A red WebKit run is not a Safari indictment, for the same reason a green one is not a Safari guarantee.** That failure was first written up as a Safari bug on the strength of a Playwright-WebKit run alone; a manual check in Safari did not reproduce it, and neither did an isolated repro of the mechanism it was blamed on. The caveat below cuts both ways, and only one direction of it was applied. **Reproduce in the real browser before claiming user impact in one** — the divergence between Playwright's WebKit build and Safari is largest in exactly the layer (networking) where that bug lives. diff --git a/README.md b/README.md index 092fa837..d7004aec 100644 --- a/README.md +++ b/README.md @@ -459,7 +459,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. -| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → Storybook. A true superset of GitHub CI. | +| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. @@ -479,11 +479,13 @@ 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, and **CI runs Chromium only**. The other engines are a tool you reach for, not a gate: running the sandbox smokes under Firefox before touching the MCP Apps sandbox, the CSP builder, or the proxy page is cheap and worth doing, but nothing runs it for you. An unrecognized value is an error, not a fallback: a silent fallback would report a green Chromium run for a command that asked for `webkit`. A missing browser binary fails naming the engine and its `npx playwright install --with-deps `. +Unset, the engine is `chromium`, so `npm run smoke` is unchanged. **`npm run ci` — the mandatory pre-push gate — additionally runs all three smokes under Firefox** via `smoke:web:firefox`; **GitHub CI does not.** An unrecognized `SMOKE_BROWSER` is an error, not a fallback: a silent fallback would report a green Chromium run for a command that asked for `webkit`. A missing browser binary fails naming the engine and its `npx playwright install --with-deps `. **Firefox passes all three smokes. WebKit fails the two App smokes**, for reasons nobody has identified. Two things are known: it does **not** reproduce in real Safari (an MCP App opens there normally), and an isolated repro of the mechanism it was first blamed on did not reproduce it under Playwright's WebKit either. So it reads as a property of that particular build rather than a bug users hit, and chasing it further was judged not worth the effort — treat a WebKit failure as unexplained rather than as a defect until someone has looked. -**Why none of this is gated in CI.** A Firefox job was trialled and was cheap — about two minutes, running in parallel with the 15-minute `build` job, so no added wall-clock. It was dropped anyway, on the honest count: across a dozen runs it never once disagreed with Chromium, so it was paying real runner time and a real flake surface (`playwright install --with-deps` runs `apt-get update`, which fails whenever a third-party repo in the runner image breaks) to re-confirm a result we already had. Making it available on demand keeps the value — a cross-engine check right when you are changing engine-sensitive code — without a permanent tax on every push. If a cross-engine regression ever does turn up, that is the evidence for putting the job back; it is a one-job diff. +**Why Firefox is in the pre-push gate rather than in CI.** A GitHub Actions job was trialled and was cheap — about two minutes, in parallel with the 15-minute `build` job, so no added wall-clock. It was dropped anyway, on the honest count: across a dozen runs it never once disagreed with Chromium, so it spent runner minutes on every push from every branch to re-confirm a result already in hand, and carried a real flake surface (`playwright install --with-deps` runs `apt-get update`, which fails whenever a third-party repo in the runner image breaks). + +Putting it in `npm run ci` instead keeps the check where it is worth most — in front of a human about to push a change they can still reason about — and pays for it once per push rather than once per CI run. If a cross-engine regression ever reaches `v2/main` because someone skipped the gate, that is the evidence for restoring the CI job; it is a one-job diff. **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. diff --git a/package.json b/package.json index 9337b5a1..eba1ca4c 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run ci:storybook", + "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run ci:storybook", "ci:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", @@ -73,7 +73,8 @@ "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:web:engine": "node scripts/run-engine-smokes.mjs", + "smoke:web:firefox": "node scripts/run-engine-smokes.mjs firefox", "smoke:launcher": "node scripts/smoke-launcher.mjs", "pack:verify": "node scripts/install-smoke-browser.mjs chromium && node scripts/pack-and-verify.mjs", "prepack": "npm run build", diff --git a/scripts/run-engine-smokes.mjs b/scripts/run-engine-smokes.mjs new file mode 100644 index 00000000..29c041a7 --- /dev/null +++ b/scripts/run-engine-smokes.mjs @@ -0,0 +1,83 @@ +#!/usr/bin/env node +/** + * Run the three headless web smokes in one browser engine (#2086). + * + * `SMOKE_BROWSER=firefox npm run smoke:web:engine` needs the variable to reach + * three child processes, and a POSIX `VAR=x npm run …` prefix does not work + * under Windows' `cmd.exe`, which npm uses there. Setting it in the child env + * from Node does, on every platform — the same reason `install-smoke-browser` + * exists rather than an inline shell expansion. + * + * It also gives the smoke list ONE home. It used to be an `&&` chain in + * package.json alongside a second chain in the workflow; adding a fourth smoke + * meant remembering both. Now `ENGINE_SMOKES` is the list, and both the + * on-demand command and the pre-push gate run through it. + * + * Usage: `node scripts/run-engine-smokes.mjs [engine]`. With no argument the + * engine comes from `SMOKE_BROWSER` (default `chromium`). + */ + +import { spawnSync } from "node:child_process"; +import { join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; +import { resolveRequestedBrowser } from "./install-smoke-browser.mjs"; + +/** + * The smokes that are engine-sensitive, in run order. + * + * Cheapest first, so a bundle that cannot even boot on the engine fails in + * seconds rather than after two full App flows have timed out. + */ +export const ENGINE_SMOKES = [ + "smoke-web-browser.mjs", + "smoke-web-app.mjs", + "smoke-web-elicitation.mjs", +]; + +function main() { + const scriptDir = resolve(import.meta.dirname); + + let browserName; + try { + browserName = resolveRequestedBrowser(process.argv.slice(2), process.env); + } catch (err) { + console.error( + `run-engine-smokes: ${err instanceof Error ? err.message : String(err)}`, + ); + process.exit(1); + } + + // Children inherit this rather than reading the ambient variable, so an + // explicit argument wins over SMOKE_BROWSER all the way down. + const env = { ...process.env, SMOKE_BROWSER: browserName }; + + const install = spawnSync( + process.execPath, + [join(scriptDir, "install-smoke-browser.mjs"), browserName], + { stdio: "inherit", env }, + ); + if (install.status !== 0) process.exit(install.status ?? 1); + + for (const smoke of ENGINE_SMOKES) { + const result = spawnSync(process.execPath, [join(scriptDir, smoke)], { + stdio: "inherit", + env, + }); + if (result.error) { + console.error( + `run-engine-smokes: could not run ${smoke}: ${result.error.message}`, + ); + process.exit(1); + } + if (result.status !== 0) { + // The smoke has already printed its own diagnosis; add only the engine, + // so a failure in the pre-push gate names which run it came from. + console.error(`run-engine-smokes: ${smoke} failed under ${browserName}`); + process.exit(result.status ?? 1); + } + } +} + +// Only when run as a script, never on import — see install-smoke-browser.mjs. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) + main(); diff --git a/scripts/run-engine-smokes.test.mjs b/scripts/run-engine-smokes.test.mjs new file mode 100644 index 00000000..ca8516c6 --- /dev/null +++ b/scripts/run-engine-smokes.test.mjs @@ -0,0 +1,45 @@ +/** + * The engine-smoke runner's list (#2086). + * + * `ENGINE_SMOKES` is the single home for "which smokes are engine-sensitive", + * and the thing that can silently rot about it is a smoke going missing — the + * suite still passes, just covering less. Nothing at runtime notices, because a + * shorter list is a *successful* run. + * + * Importing the module must not spawn anything; `main()` is behind an entrypoint + * guard, and this test would catch that regressing by running three browser + * smokes the moment the suite imported it. + */ + +import assert from "node:assert/strict"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; +import { describe, it } from "node:test"; +import { ENGINE_SMOKES } from "./run-engine-smokes.mjs"; + +const scriptDir = import.meta.dirname; + +describe("ENGINE_SMOKES", () => { + it("names every browser-driven smoke, and each one exists", () => { + // Deliberately spelled out rather than derived: this list IS the coverage + // claim, so a test that computed it from disk would agree with any mistake. + assert.deepEqual(ENGINE_SMOKES, [ + "smoke-web-browser.mjs", + "smoke-web-app.mjs", + "smoke-web-elicitation.mjs", + ]); + for (const smoke of ENGINE_SMOKES) { + assert.ok( + existsSync(join(scriptDir, smoke)), + `${smoke} is listed but missing — a rename would silently shrink the run`, + ); + } + }); + + it("runs the cheapest smoke first", () => { + // smoke-web-browser only boots the bundle; the other two drive a whole App + // flow with 45s waits. Ordering it first is what makes "this engine cannot + // run the bundle at all" fail in seconds rather than after two timeouts. + assert.equal(ENGINE_SMOKES[0], "smoke-web-browser.mjs"); + }); +});