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

ci: drop the Firefox smoke job; keep the engines as an on-demand tool

Browse source 
The job was trialled and did not earn its place. It was genuinely cheap —
~2 minutes, running in parallel with the 15-minute build job, so zero
added wall-clock — but across a dozen runs Firefox never once disagreed
with Chromium. It was paying runner time and 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; that is what the
one red run was) to re-confirm a result already in hand.

Everything that made cross-engine checking possible stays: SMOKE_BROWSER,
the shared headless-browser helper, the installer, and the tests. So
"SMOKE_BROWSER=firefox npm run smoke:web:engine" is there for the moment
it is worth running — before touching the sandbox, the CSP builder or the
proxy page — without taxing every push to re-derive the same answer.

The workflow is now byte-identical to v2/main again; the publish jobs go
back to needing only build.

Docs say what is true rather than what was intended, and AGENTS.md
records the reasoning so the job is not re-added on the general argument
that cross-engine coverage is good — that argument was already accepted,
and the on-demand path is what serves it. Re-add it on evidence: a real
cross-engine regression a gate would have caught.

Also swept the now-stale "matrix" language out of the smokes and the
resolver, including the empty-SMOKE_BROWSER rationale, which no longer
leans on a matrix key that does not exist.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-25 23:54:51 -04:00
commit 8a7ade7b08
6 changed files with 64 additions and 140 deletions
Download patch file Download diff file Expand all files Collapse all files

19
scripts/lib/headless-browser.mjs
View file

@ -102,13 +102,14 @@ export const BROWSER_ENV_VAR = "SMOKE_BROWSER";
* which is worse than no coverage — it claims coverage that never ran.
*
* **Only an ABSENT variable selects the default. A present-but-empty one is an
* error**, because that is what an unresolved CI expression looks like: GitHub
* Actions renders `SMOKE_BROWSER: ${{ matrix.browsr }}` (or any undefined key)
* as the empty string rather than omitting the variable. Treating that as "not
* set" would run Chromium inside a job named for another engine — precisely the
* false-coverage failure this resolver exists to prevent, arriving through a
* typo instead of a bad value. Unsetting the variable is how you ask for the
* default; setting it to nothing is not.
* error**, because empty is what a variable set from something that did not
* resolve looks like — `SMOKE_BROWSER="$ENGINE"` with `ENGINE` unset, or a CI
* expression naming a key that does not exist (GitHub Actions renders those as
* the empty string rather than omitting the variable). Treating that as "not
* set" would silently run Chromium for a caller who asked for something else,
* which is the same false-coverage failure the unrecognized-value branch exists
* to prevent, arriving through a typo instead of a bad value. Unsetting the
* variable is how you ask for the default; setting it to nothing is not.
*/
export function resolveBrowserName(env = process.env) {
const raw = env[BROWSER_ENV_VAR];
@ -116,8 +117,8 @@ export function resolveBrowserName(env = process.env) {
const name = raw.trim().toLowerCase();
if (name === "") {
throw new Error(
`${BROWSER_ENV_VAR} is set but empty — an unresolved CI expression ` +
`(a misspelled \`matrix\` key renders as "") looks exactly like this. ` +
`${BROWSER_ENV_VAR} is set but empty — a variable set from something ` +
`that did not resolve looks exactly like this. ` +
`Unset it to use ${DEFAULT_BROWSER}, or set one of ${SUPPORTED_BROWSERS.join(", ")}.`,
);
}

22
scripts/lib/headless-browser.test.mjs
View file

@ -5,8 +5,8 @@
* per process and then spends its whole run inside the happy path, so the branch
* that matters most — an unrecognized `SMOKE_BROWSER` — is dead code from their
* point of view. It is also the branch whose failure is silent rather than loud:
* a fallback to Chromium there would report a green run under a job labelled
* "webkit", claiming coverage that never ran.
* a fallback to Chromium there would report a green run to a caller who asked
* for another engine — coverage claimed that never ran.
*
* `loadBrowser`'s failure branches are covered here too, through its injectable
* `loadPlaywright`. The smokes cannot reach them by construction —
@ -36,20 +36,20 @@ describe("resolveBrowserName", () => {
});
it("rejects a present-but-empty value instead of defaulting", () => {
// This is the case worth being strict about. A CI expression that resolves
// to nothing (`SMOKE_BROWSER: ${{ matrix.browsr }}`, an undefined key) sets
// the variable to "" rather than removing it — so defaulting here would run
// Chromium inside a job named for another engine, which is the very
// false-coverage outcome the unrecognized-value branch exists to stop. Only
// an ABSENT variable may select the default.
// This is the case worth being strict about. `SMOKE_BROWSER="$ENGINE"` with
// ENGINE unset — or a CI expression naming a key that does not exist — sets
// the variable to "" rather than removing it, so defaulting here would
// silently run Chromium for a caller who asked for something else. That is
// the same false-coverage outcome the unrecognized-value branch exists to
// stop. Only an ABSENT variable may select the default.
for (const raw of ["", " ", "\t\n"]) {
assert.throws(
() => resolveBrowserName({ [BROWSER_ENV_VAR]: raw }),
(err) => {
assert.match(err.message, /set but empty/);
// Names the likely cause and the actual remedy — "unset it", which is
// different from "set it to nothing" and is the whole point here.
assert.match(err.message, /matrix/);
// Names the actual remedy — "unset it", which is different from
// "set it to nothing" and is the whole point here.
assert.match(err.message, /did not resolve/);
assert.match(err.message, /Unset it/);
return true;
},

2
scripts/pack-and-verify.mjs
View file

@ -567,7 +567,7 @@ async function verifyAppRender(baseUrl, token, whenWebServerExits) {
label: LABEL,
});
// Chromium explicitly, not `resolveBrowserName()`: this is a *packaging*
// check, and the engine matrix (#2086) belongs to the smokes, where the
// check, and the engine question (#2086) belongs to the smokes, where the
// sandbox surface is what is under test. Pinning it also means `pack:verify`
// cannot be pointed at an engine its npm script never installed.
browser = await loadBrowser(repoRoot, "chromium");

21
scripts/smoke-web-app.mjs
View file

@ -44,14 +44,15 @@
* ── Which engine ────────────────────────────────────────────────────────────
*
* `SMOKE_BROWSER` picks the engine (`chromium` — the default — `firefox`, or
* `webkit`). CI gates **Chromium** (in the `build` job's `npm run smoke`) and
* **Firefox** (in the `Sandbox smokes` matrix job); WebKit runs here but is not
* gated: this smoke fails under Playwright's WebKit for reasons nobody has
* identified, and nobody is currently investigating. It does NOT reproduce in
* real Safari, and an isolated SSE repro did not reproduce it in Playwright's
* WebKit either — so it is a property of that build, not a browser bug, and it
* was judged not worth chasing. Run WebKit locally if you want the signal; do
* not read a failure here as a defect until someone has looked (#2086).
* `webkit`). **CI runs Chromium only.** The other engines are an on-demand tool,
* not a gate: `SMOKE_BROWSER=firefox npm run smoke:web:engine` before touching
* the sandbox is cheap and worth doing, but nothing runs it for you (#2086).
*
* Firefox passes. WebKit fails this smoke for reasons nobody has identified and
* nobody is investigating: it does not reproduce in real Safari, and an isolated
* SSE repro did not reproduce it under Playwright's WebKit either, so it reads
* as a property of that build rather than a browser bug. Do not read a WebKit
* failure here as a defect until someone has actually looked.
*
* This smoke is one of the two places the
* MCP Apps sandbox is genuinely exercised, and the sandbox is built out of the
@ -89,8 +90,8 @@ const repoRoot = resolve(import.meta.dirname, "..");
// Resolved before anything is started, so an unsupported SMOKE_BROWSER fails
// immediately rather than after a web server and two MCP servers are up. Every
// message this smoke prints carries the engine, so a matrix failure names which
// one broke without the reader having to match it to a job title.
// message this smoke prints carries the engine, so a failure names which one
// broke rather than leaving the reader to remember what they invoked it with.
let BROWSER;
try {
BROWSER = resolveBrowserName();

11
scripts/smoke-web-browser.mjs
View file

@ -46,10 +46,9 @@
* Launching the browser (and resolving Playwright from clients/web, which has
* its own gotcha — see `lib/headless-browser.mjs`) is delegated to that module,
* which is also where `SMOKE_BROWSER` picks the engine: `chromium` (the
* default), `firefox`, or `webkit` (#2086). CI gates Chromium (in the `build`
* job's `npm run smoke`) and Firefox (in the `Sandbox smokes` matrix job);
* WebKit runs but is not gated — note this smoke PASSES in WebKit, it is the two
* App smokes that do not (see their headers).
* default), `firefox`, or `webkit` (#2086). **CI runs Chromium only**; the other
* engines are an on-demand tool rather than a gate. This smoke passes in all
* three — it is the two App smokes that fail under WebKit (see their headers).
*
* The engine question here is narrower than in the App smokes — this asserts a clean
* first paint, i.e. that the shipped bundle's syntax and API level are
@ -73,8 +72,8 @@ import {
const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..");
// Resolved before the web server is started, so an unsupported SMOKE_BROWSER
// fails immediately. Every message carries the engine, so a matrix failure names
// which one broke.
// fails immediately. Every message carries the engine, so a failure names which
// one broke.
let BROWSER;
try {
BROWSER = resolveBrowserName();

19
scripts/smoke-web-elicitation.mjs
View file

@ -25,14 +25,15 @@
* proof to a PR); unset, it asserts only.
*
* `SMOKE_BROWSER` picks the engine (`chromium` — the default — `firefox`, or
* `webkit`). CI gates **Chromium** (in the `build` job's `npm run smoke`) and
* **Firefox** (in the `Sandbox smokes` matrix job); WebKit runs here but is not
* gated: this smoke fails under Playwright's WebKit for reasons nobody has
* identified, and nobody is currently investigating. It does NOT reproduce in
* real Safari, and an isolated SSE repro did not reproduce it in Playwright's
* WebKit either — so it is a property of that build, not a browser bug, and it
* was judged not worth chasing. Run WebKit locally if you want the signal; do
* not read a failure here as a defect until someone has looked (#2086).
* `webkit`). **CI runs Chromium only.** The other engines are an on-demand tool,
* not a gate: `SMOKE_BROWSER=firefox npm run smoke:web:engine` before touching
* the sandbox is cheap and worth doing, but nothing runs it for you (#2086).
*
* Firefox passes. WebKit fails this smoke for reasons nobody has identified and
* nobody is investigating: it does not reproduce in real Safari, and an isolated
* SSE repro did not reproduce it under Playwright's WebKit either, so it reads
* as a property of that build rather than a browser bug. Do not read a WebKit
* failure here as a defect until someone has actually looked.
*
* Along with `smoke:web:app` this is one
* of the two places the MCP Apps sandbox is actually loaded, and the two nested
@ -65,7 +66,7 @@ const repoRoot = resolve(import.meta.dirname, "..");
// Resolved before anything is started, so an unsupported SMOKE_BROWSER fails
// immediately rather than after a web server and two MCP servers are up. Every
// message carries the engine, so a matrix failure names which one broke.
// message carries the engine, so a failure names which one broke.
let BROWSER;
try {
BROWSER = resolveBrowserName();