fix(smoke): reject an empty SMOKE_BROWSER; test the installer's argument logic

Two findings from Copilot's suppressed block.

1. resolveBrowserName treated a present-but-empty SMOKE_BROWSER as unset
   and returned chromium. The test that covered it even documented WHY
   that is dangerous — GitHub Actions renders an undefined matrix key as
   "" rather than omitting the variable — and then asserted the unsafe
   behavior. A misspelled matrix key would run chromium inside a job named
   for another engine and report green: the same false-coverage outcome
   the unrecognized-value branch exists to prevent, reached by a typo
   instead of a bad value.

   Only an absent variable now selects the default. Empty throws, naming
   the likely cause and the actual remedy ('unset it', which is not the
   same as setting it to nothing). The workflow always renders a real
   value, so nothing in CI is affected.

2. install-smoke-browser.mjs had no tests. Its I/O half runs on every
   smoke and fails loudly; its argument half does not — every caller
   passes nothing or the literal 'chromium', so the precedence rule is
   asserted by nothing at runtime. It matters: pack:verify passes chromium
   BECAUSE it launches chromium, and if the environment won,
   SMOKE_BROWSER=webkit npm run pack:verify would install one engine and
   launch another, surfacing as a misleading "Executable doesn't exist".

   resolveRequestedBrowser is now exported and pure; execution moved
   behind main() with the entrypoint guard verify-typecheck-coverage.mjs
   uses, so importing the module for tests cannot trigger an install.

Both mutation-verified: restoring either behavior fails the tests that
claim to cover it.

Addresses the suppressed comments on Copilot's fifth review of #2133.

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

scripts/install-smoke-browser.mjs

@@ -21,31 +21,51 @@
  * engine comes from `SMOKE_BROWSER` (default `chromium`); pass one explicitly
  * where the consumer's engine is fixed regardless of that variable, as
  * `pack:verify` does.
+ *
+ * Execution lives behind `main()` so importing this module for tests does not
+ * run an install — the same shape `verify-typecheck-coverage.mjs` uses.
  */
 
 import { spawnSync } from "node:child_process";
 import { join, resolve } from "node:path";
+import { pathToFileURL } from "node:url";
 import {
   SUPPORTED_BROWSERS,
   resolveBrowserName,
 } from "./lib/headless-browser.mjs";
 import { resolveNodeBin } from "./lib/resolve-node-bin.mjs";
 
-const repoRoot = resolve(import.meta.dirname, "..");
-const webDir = join(repoRoot, "clients", "web");
-
-const requested = process.argv[2];
-let browserName;
-try {
-  if (requested === undefined) {
-    browserName = resolveBrowserName();
-  } else if (SUPPORTED_BROWSERS.includes(requested)) {
-    browserName = requested;
-  } else {
+/**
+ * Which engine to install: the explicit argument if given, else `SMOKE_BROWSER`.
+ *
+ * The precedence is the load-bearing part, and it is why this is a function
+ * rather than three lines inline. `pack:verify` passes `chromium` explicitly
+ * *because* it launches Chromium explicitly — if the argument ever lost to the
+ * environment, `SMOKE_BROWSER=webkit npm run pack:verify` would install WebKit
+ * and then launch Chromium, and the mismatch would surface as a confusing
+ * "Executable doesn't exist" rather than as the wiring error it is.
+ *
+ * An explicit argument is validated here rather than deferred, so a typo in an
+ * npm script fails naming itself instead of reaching the Playwright CLI.
+ */
+export function resolveRequestedBrowser(argv = [], env = process.env) {
+  const requested = argv[0];
+  if (requested === undefined) return resolveBrowserName(env);
+  if (!SUPPORTED_BROWSERS.includes(requested)) {
     throw new Error(
       `unsupported browser "${requested}" — expected one of ${SUPPORTED_BROWSERS.join(", ")}`,
     );
   }
+  return requested;
+}
+
+function main() {
+  const repoRoot = resolve(import.meta.dirname, "..");
+  const webDir = join(repoRoot, "clients", "web");
+
+  let browserName;
+  try {
+    browserName = resolveRequestedBrowser(process.argv.slice(2), process.env);
   } catch (err) {
     console.error(
       `install-smoke-browser: ${err instanceof Error ? err.message : String(err)}`,
@@ -82,3 +102,9 @@ if (result.status !== 0) {
     );
     process.exit(result.status ?? 1);
   }
+}
+
+// Only when run as a script, never on import (see the header). Same guard as
+// verify-typecheck-coverage.mjs — `pathToFileURL` so it holds on Windows paths.
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href)
+  main();

scripts/install-smoke-browser.test.mjs

@@ -0,0 +1,88 @@
+/**
+ * Unit tests for the smoke browser installer's argument resolution (#2086).
+ *
+ * The installer's I/O half — resolving the Playwright CLI and spawning it — is
+ * exercised on every `npm run smoke:web:*`, so it fails loudly the moment it
+ * breaks. Its *argument* half is not: every caller in the repo passes either
+ * nothing or the literal `chromium`, so the precedence rule below is asserted by
+ * nothing at runtime, and getting it wrong produces a mismatch (install one
+ * engine, launch another) that surfaces as a misleading "Executable doesn't
+ * exist" rather than as the wiring error it is.
+ *
+ * Importing this module must not run an install; `main()` is behind an
+ * entrypoint guard for exactly that reason, and these tests are also what would
+ * catch that guard regressing — an unguarded module would try to install a
+ * browser the moment the suite imported it.
+ */
+
+import assert from "node:assert/strict";
+import { describe, it } from "node:test";
+import { resolveRequestedBrowser } from "./install-smoke-browser.mjs";
+import {
+  BROWSER_ENV_VAR,
+  DEFAULT_BROWSER,
+  SUPPORTED_BROWSERS,
+} from "./lib/headless-browser.mjs";
+
+describe("resolveRequestedBrowser", () => {
+  it("falls back to the environment when no argument is given", () => {
+    assert.equal(resolveRequestedBrowser([], {}), DEFAULT_BROWSER);
+    assert.equal(
+      resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "webkit" }),
+      "webkit",
+    );
+  });
+
+  it("lets an explicit argument beat the environment", () => {
+    // The `pack:verify` invariant. It passes `chromium` because it LAUNCHES
+    // chromium; if the environment won here, `SMOKE_BROWSER=webkit npm run
+    // pack:verify` would install WebKit and then launch Chromium.
+    assert.equal(
+      resolveRequestedBrowser(["chromium"], { [BROWSER_ENV_VAR]: "webkit" }),
+      "chromium",
+    );
+    for (const name of SUPPORTED_BROWSERS) {
+      assert.equal(
+        resolveRequestedBrowser([name], { [BROWSER_ENV_VAR]: "firefox" }),
+        name,
+      );
+    }
+  });
+
+  it("rejects an unsupported argument, naming it and the allowed set", () => {
+    assert.throws(
+      () => resolveRequestedBrowser(["safari"], {}),
+      (err) => {
+        assert.match(err.message, /unsupported browser "safari"/);
+        SUPPORTED_BROWSERS.forEach((name) =>
+          assert.ok(err.message.includes(name)),
+        );
+        return true;
+      },
+    );
+  });
+
+  it("does not let a bad argument fall through to the environment", () => {
+    // Silently using SMOKE_BROWSER after ignoring a typo'd argument would
+    // install an engine nobody asked for, and the npm script's typo would
+    // survive unnoticed.
+    assert.throws(
+      () =>
+        resolveRequestedBrowser(["chrome"], { [BROWSER_ENV_VAR]: "firefox" }),
+      /unsupported browser "chrome"/,
+    );
+  });
+
+  it("propagates the environment's own validation, empty included", () => {
+    // Delegation, not a second copy of the rules — so the deny-by-default
+    // behavior cannot drift between the two entry points.
+    assert.throws(
+      () => resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "safari" }),
+      /is not a supported browser/,
+    );
+    assert.throws(
+      () => resolveRequestedBrowser([], { [BROWSER_ENV_VAR]: "" }),
+      /set but empty/,
+    );
+  });
+});

scripts/lib/headless-browser.mjs

@@ -99,11 +99,27 @@ export const BROWSER_ENV_VAR = "SMOKE_BROWSER";
  * An unrecognized value FAILS rather than falling back to Chromium: a silent
  * fallback would report a green Chromium run under a job labelled "webkit",
  * 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.
  */
 export function resolveBrowserName(env = process.env) {
   const raw = env[BROWSER_ENV_VAR];
-  if (raw === undefined || raw.trim() === "") return DEFAULT_BROWSER;
+  if (raw === undefined) return DEFAULT_BROWSER;
   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. ` +
+        `Unset it to use ${DEFAULT_BROWSER}, or set one of ${SUPPORTED_BROWSERS.join(", ")}.`,
+    );
+  }
   if (!SUPPORTED_BROWSERS.includes(name)) {
     throw new Error(
       `${BROWSER_ENV_VAR}="${raw}" is not a supported browser — expected one of ${SUPPORTED_BROWSERS.join(", ")}`,

scripts/lib/headless-browser.test.mjs

@@ -35,13 +35,25 @@ describe("resolveBrowserName", () => {
     assert.equal(resolveBrowserName({}), DEFAULT_BROWSER);
   });
 
-  it("treats an empty or whitespace-only value as unset", () => {
-    // A CI expression that resolves to nothing (`SMOKE_BROWSER: ${{ … }}` with
-    // an undefined matrix key) sets the variable to "" rather than removing it.
-    for (const raw of ["", "   "]) {
-      assert.equal(
-        resolveBrowserName({ [BROWSER_ENV_VAR]: raw }),
-        DEFAULT_BROWSER,
+  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.
+    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/);
+          assert.match(err.message, /Unset it/);
+          return true;
+        },
+        `expected ${JSON.stringify(raw)} to be rejected`,
       );
     }
   });