fix(smoke): assert the requested engine is the one launched; fix the remedy

Three findings, all from Copilot's suppressed block.

1. The tests never checked that loadBrowser launches the engine it was
   asked for. The failure-path stand-in rejects identically for every
   engine, so loadBrowser(root, "firefox") could have called
   chromium.launch() and still produced a correctly Firefox-labelled
   error. The smokes could not catch it either: the Firefox gate runs
   after Chromium is already installed, so a mis-dispatched launch would
   SUCCEED and report Firefox coverage that never happened.

   Added a success-path test with distinct per-engine spies asserting the
   requested engine is invoked, that nothing else is, and that its handle
   is what comes back. Mutation-verified: hard-coding playwright.chromium
   fails it.

2. The launch-failure message told the reader to run
   `npx playwright install --with-deps <engine>`, which is wrong now that
   the npm scripts run from the repo root rather than clients/web -- I
   introduced that when I removed the `cd`. Playwright is pinned in
   clients/web, so a bare root-level npx can fetch a different version and
   install a browser revision the pinned one still cannot launch. It now
   points at `npm run smoke:web:<engine>` first, and scopes the raw
   command to clients/web with the reason.

   That message named smoke:web:webkit, which did not exist -- only the
   chromium and firefox scripts did. Added it, and a test asserting every
   supported engine has one, since a remedy nobody can run is worse than
   no remedy. Mutation-verified.

3. AGENTS.md said npm run ci "mirrors" the workflow and then, two
   sentences later, that it is "a superset, not a mirror". It now leads
   with strict superset, which is the topology this PR is documenting.

The README and AGENTS.md remediation wording is corrected the same way as
the runtime message, so a reader gets the same advice wherever they find
it.

Addresses Copilot review 5030668906 on #2133.

Signed-off-by: cliffhall <cliff@futurescale.com>

AGENTS.md

@@ -914,7 +914,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` → **`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).
+- **`npm run ci` is the mandatory pre-push command** — it is a **strict superset of `.github/workflows/main.yml`** (which additionally runs `npm install`, done separately here): `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). `smoke:web:firefox` is the step with no GitHub CI counterpart, deliberately (`smoke:tui` is the other, by self-skipping there). It also runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so the direction that matters holds: 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`.
@@ -953,7 +953,7 @@ Everything about the selection lives in **`scripts/lib/headless-browser.mjs`**
 - **The engine matters for one surface, and it is the MCP Apps sandbox.** Most of the web client is React and Mantine, where a second engine buys little. The sandbox is built out of the primitives that genuinely diverge: 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.
 - **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 `<meta>` 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 <engine>`. Naming `chromium` while WebKit was the missing one sends the reader to install a browser they already have.
+- **A launch failure names the engine that failed** and a remedy that works **from where the caller actually is**: `npm run smoke:web:<engine>`, or `npx playwright install --with-deps <engine>` run *from `clients/web`*. Naming `chromium` while WebKit was missing would send the reader to install a browser they already have — and naming a bare root-level `npx playwright` would send them to a version that is not the pinned one, which can install a browser revision the pinned Playwright still cannot launch.
 - **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.

README.md

@@ -479,7 +479,7 @@ 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 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>`.
+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 a remedy that works from where you are — `npm run smoke:web:<engine>`, or `npx playwright install --with-deps <engine>` run from `clients/web`, where Playwright is pinned.
 
 **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.
 

package.json

@@ -76,6 +76,7 @@
     "smoke:web:engine": "node scripts/run-engine-smokes.mjs",
     "smoke:web:chromium": "node scripts/run-engine-smokes.mjs chromium",
     "smoke:web:firefox": "node scripts/run-engine-smokes.mjs firefox",
+    "smoke:web:webkit": "node scripts/run-engine-smokes.mjs webkit",
     "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",

scripts/lib/headless-browser.mjs

@@ -187,8 +187,11 @@ export async function loadBrowser(
     return await playwright[browserName].launch({ headless: true });
   } catch (err) {
     throw new Error(
-      `${browserName} failed to launch — run \`npx playwright install --with-deps ${browserName}\`, ` +
-        `which fetches the browser and (on a bare Linux box) its system libraries ` +
+      `${browserName} failed to launch — run \`npm run smoke:web:${browserName}\`, ` +
+        `or from clients/web run \`npx playwright install --with-deps ${browserName}\` ` +
+        `if you also need its system libraries. Playwright is pinned in ` +
+        `clients/web, so running that from the repo root can fetch a different ` +
+        `version and install a browser revision this one still cannot launch ` +
         `(${err instanceof Error ? err.message : String(err)})`,
     );
   }

scripts/lib/headless-browser.test.mjs

@@ -16,7 +16,10 @@
  * naming *that engine* and its own `playwright install` command, which is
  * exactly the kind of string that rots into naming the wrong one.
  *
- * Only the successful launch is left to the smokes, since it needs a real binary.
+ * The successful path is covered here too, with distinct per-engine spies — the
+ * failure-path stand-in rejects identically for every engine, so it cannot tell
+ * a correct dispatch from a mis-dispatch that happens to print the right label.
+ * Only a launch against a REAL binary is left to the smokes.
  */
 
 import assert from "node:assert/strict";
@@ -108,6 +111,44 @@ describe("loadBrowser", () => {
     assert.equal(loaded, false);
   });
 
+  it("launches the engine that was asked for, and returns it", async () => {
+    // The load-bearing assertion, and the one the failure-path tests below
+    // CANNOT make (Copilot, #2133): their stand-in rejects identically for every
+    // engine, so `loadBrowser(root, "firefox")` could call `chromium.launch()`
+    // and still produce a correctly Firefox-labelled error. The smokes cannot
+    // catch that either — the Firefox gate runs after Chromium is already
+    // installed, so a mis-dispatched launch would succeed and report Firefox
+    // coverage that never happened. Only distinct per-engine spies pin it.
+    const launched = [];
+    const spyPlaywright = () =>
+      Object.fromEntries(
+        SUPPORTED_BROWSERS.map((name) => [
+          name,
+          {
+            launch: async (options) => {
+              launched.push({ name, options });
+              return { engine: name };
+            },
+          },
+        ]),
+      );
+
+    for (const name of SUPPORTED_BROWSERS) {
+      launched.length = 0;
+      const browser = await loadBrowser("/repo", name, {
+        loadPlaywright: spyPlaywright,
+      });
+      assert.deepEqual(
+        launched.map((l) => l.name),
+        [name],
+        `loadBrowser(…, "${name}") must launch ${name} and nothing else`,
+      );
+      // The returned handle is that engine's, not some other engine's.
+      assert.deepEqual(browser, { engine: name });
+      assert.deepEqual(launched[0].options, { headless: true });
+    }
+  });
+
   it("names the engine that failed, and its own install command", async () => {
     // The whole point of #2086's acceptance criterion: a reader whose WebKit
     // binary is missing must not be sent to install chromium.
@@ -119,10 +160,16 @@ describe("loadBrowser", () => {
           }),
         (err) => {
           assert.match(err.message, new RegExp(`^${name} failed to launch`));
+          // Names a remedy that works from where the caller is: the npm
+          // script, and the raw command scoped to clients/web (Playwright is
+          // pinned there; a bare root-level `npx playwright` can fetch a
+          // different version and install a revision this cannot launch).
+          assert.match(err.message, new RegExp(`npm run smoke:web:${name}`));
+          assert.match(err.message, /from clients\/web/);
           assert.match(
             err.message,
             new RegExp(
-              `npx playwright install --with-deps ${name}\\\`(?![\\s\\S]*--with-deps (?!${name}))`,
+              `--with-deps ${name}\\\`(?![\\s\\S]*--with-deps (?!${name}))`,
             ),
           );
           // The underlying cause survives, so the reader can tell a missing

scripts/run-engine-smokes.test.mjs

@@ -16,6 +16,7 @@ import { existsSync, readFileSync } from "node:fs";
 import { join } from "node:path";
 import { describe, it } from "node:test";
 import { ENGINE_SMOKES } from "./run-engine-smokes.mjs";
+import { SUPPORTED_BROWSERS } from "./lib/headless-browser.mjs";
 
 const scriptDir = import.meta.dirname;
 const scripts = JSON.parse(
@@ -75,6 +76,20 @@ describe("every engine tier consumes ENGINE_SMOKES", () => {
     assert.match(scripts.ci, /smoke:web:firefox/);
   });
 
+  it("every supported engine has a `smoke:web:<engine>` script", () => {
+    // loadBrowser's launch-failure message tells the reader to run
+    // `npm run smoke:web:<engine>`. If a supported engine has no such script,
+    // that remedy is unrunnable — which is how the message shipped naming
+    // `smoke:web:webkit` before it existed (Copilot, #2133).
+    for (const name of SUPPORTED_BROWSERS) {
+      assert.match(
+        scripts[`smoke:web:${name}`] ?? "",
+        new RegExp(`run-engine-smokes\\.mjs ${name}$`),
+        `npm run smoke:web:${name} is promised by the launch-failure message`,
+      );
+    }
+  });
+
   it("each gated tier names its engine explicitly rather than reading the env", () => {
     // An ambient SMOKE_BROWSER must not be able to redirect a gate: without the
     // literal engine, `SMOKE_BROWSER=firefox npm run ci` would run Firefox twice