From 32cb4c63a7548542b815f657b4e6de9846aac614 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 22:52:02 -0400 Subject: [PATCH] fix(smoke): reject an empty SMOKE_BROWSER; test the installer's argument logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/install-smoke-browser.mjs | 112 +++++++++++++++---------- scripts/install-smoke-browser.test.mjs | 88 +++++++++++++++++++ scripts/lib/headless-browser.mjs | 18 +++- scripts/lib/headless-browser.test.mjs | 26 ++++-- 4 files changed, 193 insertions(+), 51 deletions(-) create mode 100644 scripts/install-smoke-browser.test.mjs diff --git a/scripts/install-smoke-browser.mjs b/scripts/install-smoke-browser.mjs index d052528c..16502ec1 100644 --- a/scripts/install-smoke-browser.mjs +++ b/scripts/install-smoke-browser.mjs @@ -21,64 +21,90 @@ * 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(", ")}`, ); } -} catch (err) { - console.error( - `install-smoke-browser: ${err instanceof Error ? err.message : String(err)}`, - ); - process.exit(1); + return requested; } -let cli; -try { - cli = resolveNodeBin("playwright", "playwright", webDir); -} catch (err) { - console.error( - "install-smoke-browser: could not resolve the Playwright CLI from clients/web — " + - `run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, - ); - process.exit(1); +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)}`, + ); + process.exit(1); + } + + let cli; + try { + cli = resolveNodeBin("playwright", "playwright", webDir); + } catch (err) { + console.error( + "install-smoke-browser: could not resolve the Playwright CLI from clients/web — " + + `run \`npm install\` at the repo root (${err instanceof Error ? err.message : String(err)})`, + ); + process.exit(1); + } + + // `install` is a no-op when the binary is already present, so this is cheap on + // a warm machine and on a CI runner whose Playwright cache was restored. + const result = spawnSync(process.execPath, [cli, "install", browserName], { + cwd: webDir, + stdio: "inherit", + }); + if (result.error) { + console.error( + `install-smoke-browser: could not run the Playwright CLI: ${result.error.message}`, + ); + process.exit(1); + } + if (result.status !== 0) { + console.error( + `install-smoke-browser: \`playwright install ${browserName}\` exited ${result.status}`, + ); + process.exit(result.status ?? 1); + } } -// `install` is a no-op when the binary is already present, so this is cheap on -// a warm machine and on a CI runner whose Playwright cache was restored. -const result = spawnSync(process.execPath, [cli, "install", browserName], { - cwd: webDir, - stdio: "inherit", -}); -if (result.error) { - console.error( - `install-smoke-browser: could not run the Playwright CLI: ${result.error.message}`, - ); - process.exit(1); -} -if (result.status !== 0) { - console.error( - `install-smoke-browser: \`playwright install ${browserName}\` exited ${result.status}`, - ); - 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(); diff --git a/scripts/install-smoke-browser.test.mjs b/scripts/install-smoke-browser.test.mjs new file mode 100644 index 00000000..0b7bd589 --- /dev/null +++ b/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/, + ); + }); +}); diff --git a/scripts/lib/headless-browser.mjs b/scripts/lib/headless-browser.mjs index 50ec7d9b..84f148c8 100644 --- a/scripts/lib/headless-browser.mjs +++ b/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(", ")}`, diff --git a/scripts/lib/headless-browser.test.mjs b/scripts/lib/headless-browser.test.mjs index c6b2d263..bc9c2645 100644 --- a/scripts/lib/headless-browser.test.mjs +++ b/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`, ); } });