modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

ci: put the Firefox smokes in the pre-push gate instead of GitHub CI

Browse source 
Dropping the CI job left cross-engine checking as something you had to
remember to run, which in practice means nobody runs it. This moves it
into npm run ci, so it happens once per push, in front of a human who can
still reason about the change -- rather than once per CI run, on every
branch, to re-derive an answer we already had.

scripts/run-engine-smokes.mjs is what makes that possible cross-platform:
SMOKE_BROWSER has 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. smoke:web:firefox passes the
engine as an ARGUMENT rather than relying on the ambient variable, so a
stray SMOKE_BROWSER cannot silently redirect the gate to another engine
-- verified: an explicit argument beats it.

It also gives the smoke list one home. It was previously an && chain in
package.json beside a second chain in the workflow, so adding a fourth
smoke meant remembering both. ENGINE_SMOKES is now the list, with a test
pinning its contents and its order -- a smoke going missing is invisible
at runtime, because a shorter list is still a passing run.

Firefox only. WebKit fails the App smokes for unidentified reasons and
must not be in a gate.

This makes npm run ci a strict superset of GitHub CI rather than a mirror.
That was already true of smoke:tui, which self-skips on CI; Firefox is the
second such step. The direction that matters still holds: passing
npm run ci locally means CI's gates will pass.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 00:12:55 -04:00
commit 7501dad675
6 changed files with 152 additions and 12 deletions
Download patch file Download diff file Expand all files Collapse all files

4
.github/copilot-instructions.md vendored
View file

@ -102,10 +102,10 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.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.

19
AGENTS.md
View file

File diff suppressed because one or more lines are too long

8
README.md
View file

@ -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 <engine>`.
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 <engine>`.
**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 `<meta>` injected as the first `<head>` 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 `<meta>` 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.

5
package.json
View file

@ -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",

83
scripts/run-engine-smokes.mjs Normal file
View file

@ -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();

45
scripts/run-engine-smokes.test.mjs Normal file
View file

@ -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");
});
});