You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
⚠️ Decision revised during implementation — read this first
This issue was filed asking for a GitHub CI matrix across Chromium, Firefox and WebKit. Two findings during implementation changed that, and @cliffhall made the call:
WebKit fails the two App smokes for reasons nobody has identified. It does not reproduce in real Safari, 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 build, not a browser bug, and chasing it was judged not worth the effort. (An earlier revision of this issue and of SSE stream tail is not delivered under Playwright's WebKit, blocking the WebKit CI matrix entry (plus a cross-chunk parseSSE bug) #2132 wrongly described it as a Safari bug; that was retracted.)
A Firefox CI job was built, trialled, and removed. It was cheap — ~2 min, parallel with the 15-minute build job, so zero added wall-clock — but across a dozen runs Firefox never once disagreed with Chromium. 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).
Firefox moved into the local pre-push gate (npm run ci) instead of GitHub CI. The check lands in front of a human about to push a change they can still reason about, and costs once per push rather than once per CI run.
The Acceptance criteria below are updated to match. The original CI-matrix wording is kept struck through, so the change is visible rather than quietly rewritten.
Problem
Every browser-based check in this repo runs in Chromium only:
Firefox and WebKit appear nowhere — not in a script, not in the vitest browser config, not in the workflow.
That is fine for most of the web client, whose behavior is React and Mantine. It is not fine for the MCP Apps sandbox, which is the one part of the Inspector built out of the browser primitives that actually diverge between engines:
a CSP <meta> injected as the first <head> child of a document assigned to srcdoc — CSP inheritance into srcdoc / about:blank is historically divergent across engines;
a nested sandboxed iframe (trusted proxy → untrusted app) and the sandbox attribute's semantics;
a Permissions-Policyallow attribute built from the resource's permissions;
postMessage origin discipline across those two frames.
A regression in any of those is invisible to the current suite unless it also reproduces in Chromium.
Why the unit tests and Storybook do not cover this
sandbox-csp.test.ts tests string construction, not enforcement. It covers which sources survive approveCspSources and what policy text buildSandboxCspPolicy emits. That is environment-independent by construction — it would pass identically on an engine that ignores <meta> CSP entirely.
Storybook cannot cover it at all, in any browser. All three App stories (AppRenderer, AppsScreen, AppElicitationHost) point the iframe at PLACEHOLDER_SANDBOX = "data:text/html,…" and hand the renderer a mock bridge object. No sandbox proxy is served, no srcdoc assignment happens, no CSP is built or applied, and no ui/initialize handshake runs. sandbox-csp.ts is imported by exactly two things in the tree — its own unit test and createAppBridgeFactory.ts — so no story reaches it. Adding engines to the Storybook run would broaden coverage of modal chrome and focus behavior, which is worth something, but it is not this.
So the only place the sandbox is genuinely exercised is smoke:web:app and smoke:web:elicit, and both are Chromium-only.
Decision
Chromium, Firefox and WebKit. That is now the supported set; the work below is what makes it true rather than assumed.
Note Playwright's WebKit is a WebKit build, not Safari. It is close enough to catch engine-level CSP and iframe divergence, and not close enough to certify Safari specifically — worth saying so in whatever doc records the matrix, so nobody over-trusts a green run.
Scope
Parameterize the browser in the two smokes that actually load an app — scripts/smoke-web-app.mjs and scripts/smoke-web-elicitation.mjs. Both already isolate the launch in a loadChromium() helper, so this is a browser-name parameter (env var) plus a rename, not a restructure. Its error message names the missing-system-libraries fix and should name the right engine.
Decide whether smoke:web:browser joins them. It asserts a clean first paint of the bundle, which is a real cross-engine question (a syntax/API level the bundle targets) but not a CSP one — cheap to include, so probably yes.
Add the CI matrix in .github/workflows/main.yml, and install the engines it needs (npx playwright install --with-deps chromium firefox webkit). WebKit is the large download; expect the install step to dominate the added time.
Record the supported set somewhere a reader will find it — the root README's testing section is the natural home, next to the smoke table.
Consider adding firefox / webkit instances to the Storybook browser config as a separate decision. It covers a different (and much larger) surface, so it should be judged on its own cost rather than riding along with this.
Acceptance criteria
Revised — see the banner at the top.
smoke:web:app and smoke:web:elicit run in Chromium, Firefox and WebKit in CI → smoke:web:browser, smoke:web:app and smoke:web:elicit run in Chromium and Firefox in the local pre-push gate (npm run ci); GitHub CI stays Chromium-only; WebKit is reachable on demand. A failure names which engine failed.
Running them locally without a browser installed fails with an actionable message naming that engine's playwright install --with-deps <engine>. (unchanged)
The supported browser set is written down, with the Playwright-WebKit-is-not-Safari caveat — including that the caveat cuts both ways: a red WebKit run is no more a Safari indictment than a green one is a guarantee.
No change to what the smokes assert — this is an engine change, not new assertions. (unchanged)
The reasoning for keeping Firefox out of GitHub CI is recorded in AGENTS.md, aimed at how it would come back: not on the general argument that cross-engine coverage is good — that was accepted, and the pre-push gate serves it — but on evidence, i.e. a cross-engine regression that reached v2/main because someone skipped the gate.
Explicitly out of scope now
A GitHub CI matrix. Trialled and removed; see the banner.
Gating WebKit anywhere. It fails, and an engine is either green in a gate or absent from it — a continue-on-error job would report coverage nobody is held to.
Filed out of the review of #2083 (app-rendered form elicitations), which added smoke:web:elicit and followed the existing Chromium-only convention rather than introducing a matrix in a feature PR. The gap is not specific to that feature: it is the MCP Apps sandbox path generally, shared with the Apps tab and the #1565 sandbox hardening, so fixing it narrowly for elicitation would leave the larger surface uncovered.
Problem
Every browser-based check in this repo runs in Chromium only:
smoke:web:browserchromium.launch(),npx playwright install chromiumsmoke:web:appsmoke:web:elicitci:storybook)browser: "chromium"(clients/web/vite.config.ts)npx playwright install --with-deps chromium(.github/workflows/main.yml)Firefox and WebKit appear nowhere — not in a script, not in the vitest browser config, not in the workflow.
That is fine for most of the web client, whose behavior is React and Mantine. It is not fine for the MCP Apps sandbox, which is the one part of the Inspector built out of the browser primitives that actually diverge between engines:
<meta>injected as the first<head>child of a document assigned tosrcdoc— CSP inheritance intosrcdoc/about:blankis historically divergent across engines;sandboxattribute's semantics;Permissions-Policyallowattribute built from the resource'spermissions;postMessageorigin discipline across those two frames.A regression in any of those is invisible to the current suite unless it also reproduces in Chromium.
Why the unit tests and Storybook do not cover this
sandbox-csp.test.tstests string construction, not enforcement. It covers which sources surviveapproveCspSourcesand what policy textbuildSandboxCspPolicyemits. That is environment-independent by construction — it would pass identically on an engine that ignores<meta>CSP entirely.Storybook cannot cover it at all, in any browser. All three App stories (
AppRenderer,AppsScreen,AppElicitationHost) point the iframe atPLACEHOLDER_SANDBOX = "data:text/html,…"and hand the renderer a mock bridge object. No sandbox proxy is served, nosrcdocassignment happens, no CSP is built or applied, and noui/initializehandshake runs.sandbox-csp.tsis imported by exactly two things in the tree — its own unit test andcreateAppBridgeFactory.ts— so no story reaches it. Adding engines to the Storybook run would broaden coverage of modal chrome and focus behavior, which is worth something, but it is not this.So the only place the sandbox is genuinely exercised is
smoke:web:appandsmoke:web:elicit, and both are Chromium-only.Decision
Chromium, Firefox and WebKit. That is now the supported set; the work below is what makes it true rather than assumed.
Note Playwright's WebKit is a WebKit build, not Safari. It is close enough to catch engine-level CSP and iframe divergence, and not close enough to certify Safari specifically — worth saying so in whatever doc records the matrix, so nobody over-trusts a green run.
Scope
scripts/smoke-web-app.mjsandscripts/smoke-web-elicitation.mjs. Both already isolate the launch in aloadChromium()helper, so this is a browser-name parameter (env var) plus a rename, not a restructure. Its error message names the missing-system-libraries fix and should name the right engine.smoke:web:browserjoins them. It asserts a clean first paint of the bundle, which is a real cross-engine question (a syntax/API level the bundle targets) but not a CSP one — cheap to include, so probably yes..github/workflows/main.yml, and install the engines it needs (npx playwright install --with-deps chromium firefox webkit). WebKit is the large download; expect the install step to dominate the added time.firefox/webkitinstances to the Storybook browser config as a separate decision. It covers a different (and much larger) surface, so it should be judged on its own cost rather than riding along with this.Acceptance criteria
Revised — see the banner at the top.
smoke:web:appandsmoke:web:elicitrun in Chromium, Firefox and WebKit in CIsmoke:web:browser,smoke:web:appandsmoke:web:elicitrun in Chromium and Firefox in the local pre-push gate (npm run ci); GitHub CI stays Chromium-only; WebKit is reachable on demand. A failure names which engine failed.playwright install --with-deps <engine>. (unchanged)AGENTS.md, aimed at how it would come back: not on the general argument that cross-engine coverage is good — that was accepted, and the pre-push gate serves it — but on evidence, i.e. a cross-engine regression that reachedv2/mainbecause someone skipped the gate.Explicitly out of scope now
continue-on-errorjob would report coverage nobody is held to.Notes
Filed out of the review of #2083 (app-rendered form elicitations), which added
smoke:web:elicitand followed the existing Chromium-only convention rather than introducing a matrix in a feature PR. The gap is not specific to that feature: it is the MCP Apps sandbox path generally, shared with the Apps tab and the#1565sandbox hardening, so fixing it narrowly for elicitation would leave the larger surface uncovered.