ci: add headless-browser boot smoke for the web client (#1615)

[cliffhall]

Closes #1615

What

Adds smoke:web:browser — a lightweight headless-Chromium boot smoke for the prod web client — and wires it into npm run smoke (and thus npm run ci / GitHub CI).

Why

smoke:web only asserts that GET / serves the SPA HTML with the injected auth token — it never runs the React app. That's how #1612 slipped through: the browser bundle transitively value-imported a Node-only module, pulling node:* built-ins into the build and crashing the app to a blank page at runtime (Module "node:*" has been externalized for browser compatibility). Unit/integration tests run in node/happy-dom, never a real browser bundle, so none of them caught it either.

The PR that fixed #1612 added a source-level scan of the known browser-reachable OAuth-storage modules — but that only closes the hole one import list at a time. A Node dep pulled in through some other module (or a dependency) would still ship a broken bundle.

How

scripts/smoke-web-browser.mjs boots the same prod mcp-inspector --web server as smoke:web, then opens the served page in headless Chromium (Playwright, already a clients/web devDependency for the Storybook tests) and asserts:

• the app renders its first meaningful frame — the always-present "Add Servers" control — within 30s, and
• no uncaught page errors were logged (pageerror + console.error), calling out the Module "node:*" has been externalized signature explicitly.

This catches "Node code reached the browser bundle" as a class of regression, not one import at a time.

Notes:

• The npm script does cd clients/web && npx playwright install chromium first so import("playwright") resolves and chromium is present (a no-op when already installed). Repo-root paths inside the script come from import.meta.url, so the cwd change doesn't affect which build tree is exercised.
• In .github/workflows/main.yml, the Playwright cache-restore + install steps now run before the smokes so smoke:web:browser reuses the cache instead of downloading its own copy; the Storybook step still reuses the same install.
• Kept lightweight — a boot/render assertion, not a full interaction suite.

Verification

• Clean prod build → smoke passes (renders "Add Servers", no page errors), exits 0.
• Injected a fake Module "node:fs" has been externalized … throw into main.tsx, rebuilt: the smoke failed with Node built-in reached the browser bundle: … and exited 1. Reverted.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 6s —— View job

---

Review: headless-browser boot smoke (#1768)

• Gather context (read diff + related scripts)
• Review scripts/smoke-web-browser.mjs
• Review workflow / package.json / docs changes
• Post review

Good addition — this closes a real gap that source-level import scans can't (#1612 as a class, not one import list at a time), and the docs (AGENTS.md + README.md) and CI ordering rationale are both updated properly. The verification method (inject a fake Module "node:fs" has been externalized throw, confirm exit 1) is exactly the right way to prove a smoke actually smokes.

A few things I'd change before merge — the first two are the ones that matter.

---

1. Treating every console.error as a hard failure will make this flaky (and couples CI to a third-party CDN)

scripts/smoke-web-browser.mjs:122-124 pushes any console message of type error into pageErrors, and line 150 fails the smoke on a non-empty list. Chromium logs failed subresource loads to the console as error-type messages, and clients/web/index.html:7-9 loads Google Fonts:

<link rel="preconnect" href="https://fonts.googleapis.com" />
<link href="https://fonts.googleapis.com/css2?family=Fredoka..." rel="stylesheet" />

So a transient fonts.googleapis.com failure (or running the smoke on a network-restricted / air-gapped machine, which every other smoke tolerates) surfaces as console.error: Failed to load resource: net::ERR_... and fails a smoke that has nothing to do with fonts. Same for any benign React warning (console.error is React's channel for key/prop warnings) or a 404 on an asset.

The regression this script exists for arrives as an uncaught pageerror — Vite's __vite-browser-external proxy throws on property access. Suggest making pageerror the failure condition and treating console errors as either (a) diagnostic context printed on failure, or (b) failure only when they match the externalization/uncaught signature. That keeps the #1612 coverage while removing the flake surface.

Fix this →

2. The node:* signature regex misses non-prefixed built-ins

scripts/smoke-web-browser.mjs:107:

if (/Module "node:.*" has been externalized/.test(message)) {

Vite's proxy message is Module "${id}" has been externalized for browser compatibility… where id is whatever was imported — import fs from "fs" (no node: prefix, still perfectly common in transitive deps) yields Module "fs" has been externalized…. That still fails as a generic page error, but loses the diagnostic label that makes the failure self-explanatory — which is most of this script's value. /Module "[^"]+" has been externalized/ covers both.

Related: describeError() is only applied on the pageerror channel, so if the signature ever arrives via the console channel it gets the bare console.error: … prefix instead. Running the signature match over both channels (or at report time on the joined list) fixes that.

Fix this →

3. SMOKE_WEB_PORT is reused with a different default

scripts/smoke-web-browser.mjs:41 reads process.env.SMOKE_WEB_PORT ?? "6298" while scripts/smoke-web.mjs:23 reads the same var with default "6299". Unset, they don't collide — but the whole point of the override is a machine where the default port is taken, and setting it makes both smokes bind the same port. They run back-to-back and shutdown() (line 75) SIGTERMs without awaiting exit before process.exit, so the second boot can hit EADDRINUSE and report the confusing launcher exited (code …) before serving. A distinct SMOKE_WEB_BROWSER_PORT avoids it.

Fix this →

Smaller notes (take or leave)

• No settle window after first paint. The pageErrors check (line 150) runs the instant "Add Servers" becomes visible, so an externalization throw from a lazily-loaded chunk that evaluates a tick later is missed. A short await page.waitForLoadState("networkidle") (or a ~500ms delay) before the final check would harden it cheaply.
• Fail-fast if the launcher dies mid-page-load. exited is only consulted inside waitForServer(). If the server dies after that, the waitFor burns its full 30s before reporting a render timeout rather than the real cause. Racing the waitFor against a child-exit promise would surface it immediately.
• Duplication with smoke-web.mjs. The spawn / readiness-poll / shutdown / fail boilerplate (~45 lines) is now copy-pasted. Either extract a scripts/lib/prod-web-server.mjs helper, or — since both scripts boot the same prod server — consider one script that boots once and asserts both the injected-token HTML and the clean browser render, halving the CI boot cost. Separate scripts are a defensible choice; just flagging that two copies of the readiness logic will drift.
• npx playwright install chromium in the npm script omits --with-deps (correct — it needs sudo), so on a bare Linux box chromium.launch() fails with a missing-shared-library error. Worth catching that and printing "run npx playwright install --with-deps chromium". Also note npm run ci now runs playwright install twice (smoke:web:browser + ci:storybook) — harmless no-op, just redundant.
• Workflow reordering looks right: the cache restore + --with-deps install now precede the smokes, Storybook reuses the same install, and the cache key is unchanged. scripts/ isn't in the root format/lint scope, but that's pre-existing and consistent with the other smokes; and the script correctly stays out of the files allowlist.

---

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading rather than executing the smoke — installing + building the web/launcher bundles to run it wasn't in scope for a review pass. Findings 1 and 4 (console-error noise, late-chunk errors) are the ones I'd want confirmed empirically; CI on this PR should exercise 1 directly.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Thanks — thorough review. Pushed fixes in bb80837. Point by point:

1. console.error as hard failure → flaky (CDN/offline). Fixed. pageerror is now the hard-failure signal (that's how the #1612 class actually arrives — the Vite proxy throws). Console errors are collected and printed as non-fatal diagnostics, and only hard-fail when they carry the externalization signature. So a fonts.googleapis.com hiccup, an offline run, or a React key/prop warning no longer fails the smoke.

2. Regex misses non-prefixed built-ins. Fixed. Broadened to /Module "[^"]+" has been externalized/, and the signature-labeling now runs over both the pageerror and console channels (a shared labelExternalized() helper). Verified empirically: injected a bare import fs from "fs"-style throw (Module "fs" has been externalized…) and the smoke failed with Node built-in reached the browser bundle: Module "fs" ….

3. SMOKE_WEB_PORT reused with a different default. Fixed — the browser smoke now reads SMOKE_WEB_BROWSER_PORT (default 6298), distinct from smoke:web's SMOKE_WEB_PORT (6299), so overriding one no longer makes both back-to-back smokes bind the same port.

Smaller notes:

• No settle window. Added await page.waitForLoadState("networkidle", { timeout: 5000 }).catch(() => {}) + a 500ms delay after first paint, so a lazily-evaluated chunk that throws a tick later is caught. networkidle is best-effort (won't hang the smoke if a font request never idles).
• Fail-fast on launcher death. Added server.whenChildExits() and race it against the render, so a mid-load server crash reports the real cause instead of burning the full 30s render timeout.
• Duplication with smoke-web.mjs. Extracted the spawn / readiness-poll / teardown boilerplate into scripts/lib/prod-web-server.mjs; both smokes now consume it, so the readiness logic can't drift. Kept them as two scripts (the reviewer noted that's defensible) rather than merging.
• --with-deps hint. loadChromium() now catches both the import("playwright") failure and the chromium.launch() failure and prints "run npx playwright install --with-deps chromium". Left the double playwright install in npm run ci as-is (harmless no-op, as noted).
• scripts/ not in format/lint scope. Confirmed pre-existing and consistent with the other smokes; ran prettier on the new/changed files manually anyway.

Full npm run ci (validate + coverage + smoke + Storybook) is green with these changes.

@claude review

[claude]

Claude finished @cliffhall's task in 2m 34s —— View job

---

Re-review: headless-browser boot smoke (#1768)

• Read the follow-up commit (bb80837)
• Verify fixes 1–3 and the smaller notes
• Post review

All three main findings are addressed correctly, and the helper extraction is a genuine improvement over two copies. One small regression came in with the extraction (finding A below) — everything else is nits.

Verified fixes

#	Fix	Status
1	pageerror is the hard failure; console errors are diagnostics unless they carry the signature	✅ smoke-web-browser.mjs:104-113, :160-179. Font/offline/React-warning noise now prints via console.log as a note instead of failing.
2	Broadened regex + labeling on both channels	✅ :51 /Module "[^"]+" has been externalized/ and a shared labelExternalized() applied to pageErrors and consoleErrors in all three report paths.
3	Distinct port	✅ :45 SMOKE_WEB_BROWSER_PORT ?? "6298" vs smoke-web.mjs:27 SMOKE_WEB_PORT ?? "6299".
—	Settle window	✅ :134-137 best-effort networkidle + 500 ms.
—	Fail-fast on launcher death	✅ whenChildExits() raced against render() (:143). No unhandled-rejection hazard: Promise.race subscribes to both inputs, so the later rejection at SIGTERM time is considered handled.
—	--with-deps hint	✅ loadChromium() wraps both the import("playwright") and chromium.launch() failures with the actionable message.
—	Duplication	✅ scripts/lib/prod-web-server.mjs consumed by both smokes.

Nice unplanned improvement: the render-failure path (:144-156) now prints the labelled page diagnostics, so a #1612-style regression reports Node built-in reached the browser bundle: … alongside the render timeout rather than just "timed out" — that's the exact failure mode this script exists for, and it now self-explains.

I also checked the locator can't hit a Playwright strict-mode violation: Add Servers is rendered from exactly one site (ServerAddMenu.tsx:19 ← ServerListControls.tsx:36 ← ServerListScreen.tsx:264), so the getByRole resolves to a single node.

---

A. The extraction made smoke:web's non-200 assertion dead code (and turned a hard failure into a 60 s timeout)

The old fetchRoot() returned the first response it got, whatever the status:

try { return await fetch(`${BASE_URL}/`); } catch { await delay(500); }

The new waitForReady() (scripts/lib/prod-web-server.mjs:66-72) only returns on res.ok:

const res = await fetch(`${baseUrl}/`);
if (res.ok) return res;

Two consequences for scripts/smoke-web.mjs:43-45:

• if (res.status !== 200) is now unreachable in practice — res.ok guarantees 2xx, so only a 201–299 would trip it.
• A server that boots but consistently answers, say, 500 (broken dist, injection failure throwing in the / handler) used to fail in ~1 s with GET / returned HTTP 500. It now polls for the full 60 s and reports server did not start within 60s, which points the reader at the wrong thing.

Cheapest fix that keeps the readiness semantics (a warming server legitimately answering 503 shouldn't end the poll): remember the last non-ok response and surface it — either return it on timeout, or include its status in the timeout message (server did not start within 60s (last response: HTTP 500)). Fix this →

Nits (take or leave)

• isExited / exitCode are exported but unused by either consumer (prod-web-server.mjs:101-102) — both read exit state through waitForReady() / whenChildExits(). Dead API on a brand-new helper; drop them unless you expect a third caller.
• No child.on("error") in the helper. A spawn-level failure emits 'error' on the child; with no listener that throws an uncaught exception with a raw stack instead of the smoke:web… FAILED — line. Pre-existing behavior, but now it's centralized in one place where a two-line reject-style handler would fix it for both smokes.
• SMOKE_WEB_BROWSER_PORT isn't documented — it's explained in the script header, but AGENTS.md's smoke:web:browser bullet doesn't mention either port override. Fine to leave; just noting the escape hatch is discoverable only by reading the script.
• npm run smoke now boots the prod web server twice back-to-back (once per script). You explicitly chose two scripts over one combined boot, which is defensible — just the cost of that choice.

Verdict: LGTM once (A) is addressed — and even as-is it's a strict improvement over the previous revision; (A) only degrades a diagnostic in smoke:web, it can't produce a false pass.

Caveat on verification: this checkout still has no node_modules and no client builds, so I reviewed by reading and syntax-checking (node --check passes on all three scripts) rather than executing the smoke or prettier --check. Your local npm run ci being green covers the empirical side, including the console-error-noise behavior I flagged last round.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Note: CI on the previous push failed in smoke:web:browser with Cannot find package 'playwright'. Root cause: the bare import("playwright") resolved relative to the script's own directory (scripts/), not the cwd, so the cd clients/web in the npm script never helped — it only passed on my machine because an ancestor node_modules carried playwright. Fixed in d12c3a6 by resolving playwright explicitly from clients/web via createRequire (cwd- and ancestor-independent; verified it resolves to clients/web/node_modules/playwright). New CI run in progress.

[cliffhall]

Thanks — addressed in 45e828e.

A. waitForReady made the non-200 assertion dead code / 60s timeout on persistent 500. Fixed. waitForReady keeps polling on a non-ok status (a warming server may answer 503) but now records the last non-ok status and includes it in the timeout error: server did not start within 60s (last response: HTTP 500). So a booted-but-broken dist/injection failure points the reader at the right thing instead of a bare timeout. (Kept smoke:web's res.status !== 200 check — with res.ok it only trips on a non-200 2xx, which is a cheap, still-meaningful assertion, not harmful.)

Nit — unused exports. Dropped isExited / exitCode (and child, also unused) from the helper's returned API; it's now just { baseUrl, waitForReady, whenChildExits, stop }.

Nit — no child.on("error"). Added. A spawn-level failure ('error', not 'exit') now sets a spawnError that both waitForReady and the whenChildExits race surface as the smoke's … FAILED — line (launcher failed to spawn: …) instead of an uncaught raw stack.

Nit — SMOKE_WEB_BROWSER_PORT not in AGENTS.md. Left as-is for consistency — smoke:web's SMOKE_WEB_PORT isn't documented there either; both are discoverable in the script headers.

Double boot — kept as the deliberate two-script choice, as noted.

Separately: the first CI run on this PR caught a real bug the static reviews couldn't — the bare import("playwright") resolved against scripts/, not the cwd, so it failed in CI with Cannot find package 'playwright' (passed locally only via an ancestor node_modules). Fixed in d12c3a6 by resolving playwright from clients/web via createRequire. Nice validation that the smoke — and the CI wiring — actually exercise the bundle.

Local npm run ci is green. @claude review

[claude]

Claude finished @cliffhall's task in 5m 47s —— View job

---

Third-pass review: headless-browser boot smoke (#1768)

• Read the follow-up commits (d12c3a6, 45e828e)
• Verify finding A + the three nits
• Check what Vite 8 actually emits for a browser-externalized module
• Post review

Finding A and all three nits are correctly addressed. One new thing surfaced while checking the externalization signature against the repo's actual Vite version — it doesn't break the smoke, but it does mean the docs promise a string the prod bundle can't produce.

Verified fixes (45e828e)

#	Fix	Status
A	waitForReady records the last non-ok status	✅ prod-web-server.mjs:79-95 — keeps polling on non-ok (warming 503 still tolerated) and reports server did not start within 60s (last response: HTTP 500). Correct fix for the diagnostic regression.
nit	Unused exports dropped	✅ Returned API is now { baseUrl, waitForReady, whenChildExits, stop }.
nit	child.on("error")	✅ :57-68 — bootFailure() surfaces launcher failed to spawn: … through both waitForReady and whenChildExits, instead of an uncaught raw stack. (See 2 below for one edge.)
—	playwright resolution (d12c3a6)	✅ createRequire(resolve(scriptDir, "..", "clients/web/package.json")) pins resolution to clients/web/node_modules independent of cwd and ancestor dirs, and playwright ships a CJS require entry, so requireFromWeb("playwright") is valid. Good catch by CI — a bare ESM specifier really does resolve from scripts/, not the cwd.

CI on this head (45e828e9) is still in progress as I write this, so the playwright-resolution fix isn't confirmed green yet.

---

1. The Module "…" has been externalized signature can't fire against a Vite 8 production bundle

I fetched the Vite the web client resolves (clients/web/package.json → "vite": "^8.0.0", latest 8.1.5) and read its dist. The string this smoke keys on appears in exactly one place in the whole package (dist/node/chunks/node.js, the vite:dep-pre-bundle load handler):

if (id.startsWith(browserExternalNamespace)) {
  const path = id.slice(17);
  if (isProduction) return { code: "module.exports = {}" };      // ← prod: silent stub
  else return { code: `module.exports = Object.create(new Proxy({}, {
    get(_, key) { … console.warn(`Module "${path}" has been externalized …`) }  // ← dev: console.WARN
  }))` };
}

Three consequences:

• Prod (vite build, what this smoke runs): the stub is module.exports = {} with no message at all. The externalized signature is unreachable here.
• Dev: Vite 8 replaced the old throwing proxy with a console.warn. Playwright reports that as msg.type() === "warning", and the collector only keeps type() === "error" (smoke-web-browser.mjs:125) — so even on the dev leg the signature would be filtered out.
• Node built-ins specifically: for a client environment resolve.builtins defaults to [] (node.js:35515), and clients/web/vite.config.ts sets no build.rollupOptions.external, so an import … from "node:fs" reaching the browser graph now fails the build: [vite]: Rolldown failed to resolve import "node:fs" from "…" (node.js:33151-33154). That's caught by validate/build, upstream of this smoke.

None of that makes the smoke worthless — the load-bearing assertion is still exactly right: the prod bundle boots and paints with no uncaught page error. A silently-stubbed {} module blowing up on first use (the realistic remaining shape of #1612 under Vite 8 — a dep whose browser field maps fs: false) arrives as a TypeError pageerror and fails the smoke. But:

• EXTERNALIZED / labelExternalized() is effectively dead code against the bundle it runs on, and
• AGENTS.md/README.md/the script header promise the specific string ("in particular no Module "node:*" has been externalized for browser compatibility"), which the prod build cannot emit.

Also worth noting for the record: the empirical verification in both commits injected a hand-written throw carrying that string, which proves the reporting plumbing but not the real signature. Re-verify by making a browser-reachable module actually value-import a Node-only path and reporting what the smoke prints.

Suggested minimum: reword the claim in the docs + header to what's actually asserted ("boots and renders with no uncaught page error — which is how a Node-only module reaching the browser bundle manifests"), and either drop the signature machinery or also match msg.type() === "warning" so the dev-flavored message isn't silently dropped if this ever runs against a dev server. Fix this →

2. child.on("error") sets exited = true, so stop() can skip the SIGTERM

prod-web-server.mjs:57-60 treats 'error' as death, and stop() (:120) is if (!exited) child.kill("SIGTERM"). Node emits 'error' not only when the process could not be spawned, but also when it could not be killed or a message couldn't be sent — cases where the child is still alive. In that path the smoke exits without killing the launcher, leaving an orphan holding port 6298/6299, and the next smoke run reports the (now misleading) server did not start within 60s.

Cheap fix: keep exited for the 'exit' event only, and let bootFailure() key off spawnError (it already does), so stop() still SIGTERMs when 'error' fired but the process lives. Fix this →

Nits

• AGENTS.md still documents the disproven playwright mechanism. The new bullet says the npm script's cd clients/web is "so import("playwright") resolves" and that "repo-root paths inside the script come from import.meta.url, so the cwd change doesn't affect which build tree is exercised" — d12c3a6 established the cd does not make a bare specifier resolve (that's why createRequire exists); the cd now only makes npx playwright install find the local bin. The script headers were updated; the doc wasn't. Per the repo's own doc-accuracy rule this is worth a one-line correction, and it's the sentence a future reader would trust. Fix this →
• The new scripts/lib/ folder isn't mentioned in the docs. README.md's tree describes scripts/ generically and neither doc mentions scripts/lib/prod-web-server.mjs, though both smoke bullets in AGENTS.md describe the boot behavior it now owns. A clause on the smoke:web bullet ("both smokes share the spawn/readiness helper in scripts/lib/prod-web-server.mjs") would keep the next reader from re-discovering it.
• whenChildExits() registers both once("exit") and once("error") and never removes the sibling when one fires. Harmless here (called once per run, process exits shortly after), just noting it.
• Re-confirmed no unhandled rejection from Promise.race([server.whenChildExits(), render()]): the race attaches handlers to both inputs, so the rejection at SIGTERM time is considered handled even though the race already settled.
• Re-confirmed the locator is unambiguous — Add Servers is rendered from a single site (ServerAddMenu.tsx:19), so no Playwright strict-mode violation.
• Workflow ordering still reads correctly: cache restore → install --with-deps chromium → smokes → Storybook, with the cache key unchanged.

Verdict: the helper and the smoke are in good shape — (2) is a narrow robustness edge and (1) is about the claim, not the coverage. Neither can produce a false pass. I'd fix the AGENTS.md playwright sentence and the overstated signature claim before merge, since both are documentation that a future reader will act on; (2) at your discretion.

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading and by inspecting the Vite 8.1.5 tarball directly (npm pack vite@8) rather than executing the smoke. Finding 1 rests on that dist read plus clients/web/vite.config.ts having no build.rollupOptions.external; if the installed Vite is an earlier 8.x its dep-pre-bundle branch could differ, so treat the exact prod/dev split as version-specific to what I read.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Excellent review — the Vite-8 point is the important one. Addressed in b97b407, and I verified it empirically against the installed vite@8.0.0 rather than trusting a version-specific dist read, since you flagged the exact split as version-dependent:

1. Signature can't fire against a Vite 8 prod bundle. Confirmed end-to-end. In this repo's vite@8.0.0, a node:fs import reaching the browser graph is a build-time warning (plugin rolldown:vite-resolve, surfaced by vite build) and the build still succeeds, shipping a silent stub. Then:

• CASE 1 — a browser-reachable module that calls the externalized builtin at init (readFileSync(...)): the smoke fails with page diagnostics: (0 , Lv.readFileSync) is not a function — a TypeError pageerror, no "externalized" string anywhere. This is the real refactor(auth): remove Zustand from OAuth persistence, clean up storage IO (#1549) #1612 shape and it's caught.
• CASE 2 — imported but never called: ships module.exports = {}, typeof is harmless, app boots → smoke passes. Exactly your point: an unused externalized import is invisible by design (and harmless).

So the EXTERNALIZED/labelExternalized machinery was dead against the prod bundle — dropped it. The smoke now asserts precisely what it verifies: the prod bundle boots and paints its first meaningful frame with no uncaught page error — which is how a Node-only module reaching the browser bundle manifests (a TypeError when its empty stub is called). Reworded the script header, the AGENTS.md smoke:web:browser bullet, and the README smoke row to say that instead of promising the specific string. Console errors remain diagnostic-only.

2. child.on(\"error\") set exited, so stop() could skip SIGTERM. Fixed. The error is now tracked in a separate childError; bootFailed() = exited || childError (so waitForReady/whenChildExits still surface a spawn failure), while stop() stays guarded on exited alone — so an 'error' from a failed kill/send on a still-alive child no longer orphans the launcher on the port.

Nit — stale AGENTS.md playwright sentence. Fixed. The bullet no longer claims cd clients/web makes import(\"playwright\") resolve; it now states playwright is resolved via createRequire from clients/web (cwd-independent) and the cd exists only so npx playwright install finds the local bin.

Nit — scripts/lib/ undocumented. Added a clause to both the smoke:web and smoke:web:browser bullets noting they share scripts/lib/prod-web-server.mjs.

Nit — whenChildExits leaves a sibling listener; double boot. Left as-is (both are harmless as you noted; the process exits right after).

Local npm run ci is green, and the prior CI run confirmed the playwright-resolution fix works in real CI.

@claude review

[claude]

Claude finished @cliffhall's task in 3m 6s —— View job

---

Fourth-pass review: headless-browser boot smoke (#1768)

• Read the follow-up commit (b97b407)
• Verify findings 1 & 2 + the doc nits
• Check the residual failure-signal coverage (pageerror vs unhandled rejection)
• Post review

Both findings are correctly addressed, and dropping the signature machinery was the right call rather than papering over it — the script now claims exactly what it asserts. Your empirical result also supersedes my dist read in a way that strengthens the PR (see the note under 1). One residual gap in the failure signal is worth three lines; everything else is nits.

Verified fixes (b97b407)

#	Fix	Status
1	Signature machinery dropped; reframed on uncaught pageerror	✅ EXTERNALIZED / labelExternalized() gone; smoke-web-browser.mjs:164-166 fails on pageErrors only; :170-174 prints console errors as a note. Header :14-32, AGENTS.md bullet, and the README.md smoke row all reworded to "no uncaught page error — which is how a Node-only module reaching the browser bundle manifests". The header even documents the CASE 2 corollary (unused externalized import ships {}, invisible by design), which is the honest statement of scope.
2	child.on("error") no longer sets exited	✅ prod-web-server.mjs:49-65 — childError is separate, bootFailed() = exited || childError !== null feeds waitForReady/whenChildExits, while stop() (:125) still guards on exited alone, so an 'error' from a failed kill/send can't orphan a live launcher on the port. Side benefit: stop()'s child.kill() on an unspawned child can no longer produce an uncaught 'error' — the listener catches it.
nit	Stale AGENTS.md playwright sentence	✅ Now states playwright is resolved via createRequire from clients/web regardless of cwd, and that the cd exists only so npx playwright install chromium finds the local bin.
nit	scripts/lib/ undocumented	✅ Noted on both bullets (AGENTS.md), and smoke-web.mjs:12-15 cross-references the helper and its companion.

Also re-verified: Add Servers is still a single render site (ServerAddMenu.tsx:19, a Mantine Button → role=button), so the locator can't hit a strict-mode violation; node --check passes on all three scripts; the launcher runs the web server in-process (clients/launcher/src/index.ts:48-50 await import(clientEntry("web"))), so the SIGTERM in stop() reaches the actual listener — no grandchild to orphan.

Your Vite finding upgrades the smoke's importance. I had read 8.1.5's dist and concluded a node:* import would fail the client build outright (resolve.builtins: [] → "Rolldown failed to resolve import"). You measured the installed 8.0.0 and got a build warning plus a shipped silent stub. If that's the behavior in the pinned version, then nothing upstream of this smoke gates the regression at all — npm run build prints a warning CI doesn't fail on, and the crash only appears when the bundle runs. That makes smoke:web:browser the only gate for the CASE 1 shape, not a belt-and-braces check. (Possible complementary follow-up, out of scope here: make vite build treat that specific warning as an error, which would also catch CASE 2 — the unused import that this smoke can't see by design.)

---

1. pageerror doesn't fire for unhandled promise rejections — the async half of the class still exits 0

page.on("pageerror") fires for uncaught synchronous exceptions. Chromium reports an unhandled promise rejection on the console channel instead, as Uncaught (in promise) TypeError: … — and console errors are now diagnostic-only (:117-119, :170-174). So the same empty-stub TypeError reached through an async path prints a note and the smoke passes:

• a then/await inside module init,
• a failed dynamic import — this app has real ones (CodeHighlight.tsx:85-86 await import("react-syntax-highlighter/…")), which also surface as Failed to fetch dynamically imported module on the same channel.

This doesn't weaken the shape you verified (CASE 1 is sync module init → pageerror → caught, confirmed empirically). It's a narrowing of the class the docs now claim.

The fix keeps the anti-flake property that motivated the last round, because Chromium's prefix for this class is unambiguous: Uncaught … can never be a font 404 (Failed to load resource: net::ERR_…) or a React key/prop warning. Hard-fail console errors matching /^Uncaught\b/ (and optionally Failed to fetch dynamically imported module); everything else stays a printed note.

Fix this →

Nits (take or leave)

• AGENTS.md's "not a runtime message" is absolute where it should be prod-scoped. The bullet now says the Module "…" has been externalized text "is a build-time warning (vite build / npm run build), not a runtime message, so the browser never sees it". True for the prod bundle this smoke runs; under npm run dev Vite's browser-external stub is a Proxy that console.warns that string at runtime, so a dev-server browser does see it. Since AGENTS.md is read as general guidance, a two-word qualifier ("not a runtime message in a prod build") keeps a future reader from concluding it can never appear at runtime.
• smoke-web.mjs:43-45's res.status !== 200 is still only reachable for a non-200 2xx now that waitForReady() returns on res.ok. You called it a cheap, still-meaningful assertion — agreed, no change needed; noting it stays as documented dead-ish code.
• bootFailed() folding childError in means an 'error' from a failed kill on a still-live child would be reported by waitForReady/whenChildExits as a boot failure — the exact case the new comment says the child is alive. Unreachable in practice (the only kill() happens after both have settled), so this is a consistency observation, not a bug.
• whenChildExits()'s sibling listener and the double prod-web boot in npm run smoke remain as you deliberately left them — agreed both are harmless.
• CI on this head (b97b407) is still in progress as I write this, so the reframed smoke isn't confirmed green in CI yet.

Verdict: LGTM. Finding 1 is a ~3-line addition that closes the async half of the class the docs now claim to cover; it can't produce a false pass in the shape you verified, so it's fine to take before merge or as a follow-up — your call.

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading plus node --check (all three scripts pass); no prettier --check and no execution of the smoke. Finding 1 rests on Playwright's documented pageerror semantics (sync uncaught exceptions only) — worth a 60-second empirical confirmation on your machine by throwing from inside a .then() during module init and checking whether the smoke exits 0.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Thanks — took finding 1; it's a real gap. Fixed in 4382322.

1. pageerror misses the async half of the class. Added a narrow hard-failure signature over the collected console errors: /^Uncaught\b|Failed to fetch dynamically imported module/. Any match hard-fails; every other console error stays a printed non-fatal note — so a font/CDN 404 (Failed to load resource: net::ERR_…) or a React warning still can't flake it.

Empirically verified against vite@8.0.0:

• Injected Promise.reject(new TypeError(...)) at module init (app still paints, so waitFor succeeds) — the smoke now fails: app logged uncaught error(s): smoke async probe — unhandled rejection. Before this change it exited 0. (Interesting wrinkle: in this Playwright build the rejection also fires pageerror, so it's caught on both channels — but the FATAL_CONSOLE signature is what guarantees the console-only variants are covered regardless of Playwright version.)
• The failed-dynamic-import case is even better handled upstream than expected: a statically-analyzable missing chunk (import(\"./__nope__.js\")) fails the vite build outright, so it never reaches the smoke. The signature covers the runtime dynamic-import failure (a chunk that 404s at fetch time), which only ever appears on the console channel.

Updated the script header and the AGENTS.md bullet to document both the sync (pageerror) and async (console Uncaught …) failure signals.

Nit — "not a runtime message" was absolute. Fixed — now scoped: "in a prod build … not a runtime message (under npm run dev Vite's stub is instead a Proxy that console.warns that string at runtime)."

Nits — res.status !== 200 dead-ish / bootFailed() folding childError / sibling listener / double boot. Left as-is — all acknowledged as unreachable-in-practice or deliberate, none can produce a false pass.

Your point about this being the only gate for CASE 1 is right and worth recording: under vite@8.0.0 a node:* import reaching the browser graph is a build warning CI doesn't fail on, so smoke:web:browser is the sole gate for the used-stub crash. The complementary follow-up you suggested (make vite build treat that warning as an error, which would also catch the unused-import CASE 2) is a good idea — I'll file it as a separate issue rather than expand this PR's scope.

Local npm run ci is green. @claude review

[claude]

Claude finished @cliffhall's task in 2m 51s —— View job

---

Fifth-pass review: headless-browser boot smoke (#1768)

• Read the follow-up commit (4382322)
• Verify the async hard-failure signature + doc nit
• Corroborate against the last green CI run
• Post review

Finding 1 is correctly implemented, and I found empirical corroboration in CI that it carries no flake risk here (below). No blocking findings this round.

Verified (4382322)

#	Fix	Status
1	Async half hard-fails	✅ smoke-web-browser.mjs:79 FATAL_CONSOLE = /^Uncaught\b|Failed to fetch dynamically imported module/; :184-189 fails on pageErrors ∪ fatalConsole; :193-198 prints only the complement as a note. Alternation precedence is right (the ^ anchor binds the first branch only) — I ran the regex over representative messages: Uncaught (in promise) TypeError: …, Uncaught TypeError: …, Failed to fetch dynamically imported module: … → FATAL; Failed to load resource: net::ERR_NAME_NOT_RESOLVED, a React key warning, Unable to preload CSS for … → note.
nit	AGENTS.md "not a runtime message" prod-scoped	✅ Now "in a prod build … (under npm run dev Vite's stub is instead a Proxy that console.warns that string at runtime)", and the bullet documents both the sync pageerror and async console signals.

Header comment :29-43 states the two channels and why the prefixes can't collide with benign noise, which is the part a future reader needs to not re-widen it. node --check passes on all three scripts.

Empirical corroboration from CI (stronger than my last round's caveat)

The last green run on the previous head (b97b407, run 30131260246) printed:

smoke:web:browser OK — app booted at http://127.0.0.1:6298, rendered "Add Servers" with no uncaught page errors

…and no smoke:web:browser note — N non-fatal console error(s) line at all. So in the CI environment the boot collects zero console errors — the new hard-fail signature has no false-positive surface there, not merely a narrow one. That's the flake question from round 1 answered with data rather than reasoning.

Also, your "interesting wrinkle" is expected rather than build-specific, which makes the belt-and-braces framing exactly right: Chromium reports unhandled rejections through Runtime.exceptionThrown (text Uncaught (in promise) …), which Playwright maps to pageerror — so that channel usually catches it. Playwright's console channel additionally forwards browser-level Log.entryAdded entries (that's why a subresource failure like a font 404 can arrive as a console message of type error — round 1's concern was real). FATAL_CONSOLE is the branch that survives either mapping changing.

Nits (take or leave)

• Success line + README.md row still say "page error" where the assertion now spans both channels. :201 prints "with no uncaught page errors" and README.md:145 reads "no uncaught page error — how a Node built-in reaching the browser bundle manifests". AGENTS.md was updated to the two-channel wording; these two weren't. Cosmetic, but it's the same doc-accuracy class as the last two rounds. Fix this →
• The vite build follow-up needs a board item, not just a mention. You said you'd file "make the externalized-module warning an error" separately — worth doing before merge so the CASE 2 gap (unused externalized import, invisible to this smoke by design) is tracked rather than living only in this thread. Per AGENTS.md that's issue + v2 label + board Add tab and approval flow for server -> client sampling #28 with a Status, all three steps.
• Duplication in the failure message when the same rejection lands on both channels (pageErrors and fatalConsole both carry it) — cosmetic, and arguably useful signal.
• Everything you explicitly left as-is (res.status !== 200 reachable only for a non-200 2xx, bootFailed() folding childError, whenChildExits's sibling listener, the double prod-web boot) — agreed on all four; none can produce a false pass.

Verdict: LGTM. Five rounds in, the script now asserts exactly what it verifies, on both channels the class can arrive through, with the anti-flake property confirmed by a real CI run. CI on this head (4382322) is still in progress as I write this; the previous head was green.

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading, node --check, a standalone regex harness, and the CI log of the prior green run — not by executing the smoke.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Thanks — both nits handled in 9c576d7.

Nit — success line + README said "page error". Fixed. The OK line now reads … with no uncaught errors (sync page error or unhandled rejection) and the README row … no uncaught error — sync exception or unhandled rejection …, matching the two-channel wording already in the AGENTS.md bullet.

Nit — vite build follow-up needs a board item. Already done — filed #1769 ("Build gate: fail vite build on a browser-externalized Node built-in"), v2-labeled, assigned, and on board #28 with Status Todo. It captures exactly the CASE 2 gap (unused externalized import, invisible to this smoke by design) plus moving CASE 1 detection upstream of the runtime. (You filed the review just before I posted it, so it wasn't visible in-thread yet.)

Remaining items left as-is (res.status !== 200 reachable only for a non-200 2xx, bootFailed() folding childError, the sibling listener, both-channel message duplication, the double prod-web boot) — all acknowledged, none can produce a false pass.

Thanks for the five-round pass — the CI-corroborated anti-flake point and the async-channel gap were both real catches. Local npm run ci green.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 53s —— View job

---

Sixth-pass review: headless-browser boot smoke (#1768)

• Read the follow-up commit (9c576d7)
• Verify the wording fixes + Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615) #1769 tracking
• Final read of the full diff (7 files)
• Post review

Both nits are correctly applied and the diff is in good shape. No blocking findings; one small message-accuracy nit that's new this round, plus one thing I couldn't verify from here.

Verified (9c576d7)

#	Fix	Status
nit	Success line two-channel wording	✅ smoke-web-browser.mjs:201 → …rendered "Add Servers" with no uncaught errors (sync page error or unhandled rejection).
nit	README smoke row	✅ README.md:145 → …no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests. Now consistent with the AGENTS.md bullet (which already documents both pageerror and the console Uncaught … / Failed to fetch dynamically imported module signals) and with the script header :29-43.
nit	vite build follow-up tracked	✅ #1769 exists — "Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615)", v2-labeled, assigned to you, open. See the caveat below on the board step.

Full-diff re-read (git diff origin/v2/main...HEAD, 7 files) — nothing else changed since the fifth pass, and the pieces still line up:

• package.json:57 adds smoke:web:browser and appends it last in smoke, so a failure lands after the cheaper smokes have already passed.
• .github/workflows/main.yml — cache restore → playwright install --with-deps chromium → npm run smoke → Storybook; cache key unchanged, comment explains the reordering.
• scripts/lib/prod-web-server.mjs — childError separate from exited, bootFailed() feeding waitForReady/whenChildExits, stop() still guarded on exited alone; waitForReady records the last non-ok status.
• scripts/smoke-web.mjs shrinks to the HTML assertions and delegates boot to the helper, with a header cross-reference to the companion.

Nit (take or leave)

loadChromium()'s import-failure hint points at the wrong command. smoke-web-browser.mjs:105-109 catches a requireFromWeb("playwright") failure and advises npx playwright install --with-deps chromium. But a failure there means the playwright npm package isn't resolvable from clients/web — and playwright install installs browser binaries, not the package, so the suggested command can't fix it (the npm script already ran npx playwright install chromium one step earlier, which would have transiently fetched playwright from the registry and still left the require failing). The right advice for that branch is npm install at the repo root (the postinstall cascade installs clients/web's devDependencies). The chromium.launch() branch at :113-116 is correctly advised — that one really is the missing-system-libraries case.

Fix this →

Notes

• Everything you left as-is (res.status !== 200 reachable only for a non-200 2xx, bootFailed() folding childError, whenChildExits's sibling listener, the both-channel message duplication, the double prod-web boot in npm run smoke) — agreed on all five; none can produce a false pass.
• CI: the run on this head (9c576d72, 30132201889) is still in progress, as is the one on 43823223. The last completed run on this branch — b97b407b, 30131260246 — was green, and this head only changes two comment/doc strings plus one console.log literal, so nothing behavioral is unproven by it. Worth a glance that the async-signature commit (4382322) goes green before merge, since that's the last functional change.
• Board step for Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615) #1769 unverified: this job's token can't read project Add tab and approval flow for server -> client sampling #28 (Could not resolve to a ProjectV2 with the number 28), so I confirmed the issue exists with the v2 label and an assignee but couldn't confirm it's on the board with Status Todo. Taking your word that it is.

Verdict: LGTM — ship it. Six rounds in, the smoke asserts exactly what it verifies on both channels the crash class can arrive through, the shared helper removes the drift risk, the docs match the behavior, and the CASE 2 gap that this smoke can't see by design is tracked in #1769. The loadChromium hint is a one-line message tweak, fine as a follow-up or a drive-by.

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading the diff and node --check (passes on all three scripts) — no prettier --check, and I did not execute the smoke.
· branch v2/ci-headless-web-boot-smoke-1615

[cliffhall]

Thanks — fixed the last nit in 0c27159.

loadChromium() import-failure hint pointed at the wrong command. Right — a requireFromWeb("playwright") failure means the playwright npm package isn't resolvable from clients/web, which playwright install (browser binaries) can't fix. The message now advises npm install at the repo root (the postinstall cascade installs clients/web's devDependencies), with a comment explaining the distinction. The chromium.launch() catch keeps --with-deps — correct for the missing-system-libraries case.

On your notes:

• CI: local npm run ci is green on this head, and CI on 4382322 (the last functional change — the async signature) went green earlier; this head and 9c576d7 only touch comment/doc/message strings.
• Board: confirmed on my side — Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615) #1769 is on board Add tab and approval flow for server -> client sampling #28 with Status Todo (the review token can't read the org project, hence it couldn't verify).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 29s —— View job

---

Seventh-pass review: headless-browser boot smoke (#1768)

• Read the follow-up commit (0c27159)
• Verify the loadChromium() hint fix
• Final full-diff pass (7 files)
• Corroborate CI state across the branch
• Post review

The last nit is correctly fixed and I have no further findings. Full diff re-read; ship it.

Verified (0c27159)

#	Fix	Status
nit	loadChromium() import-failure hint	✅ smoke-web-browser.mjs:105-114 — the requireFromWeb("playwright") catch now reads could not resolve the Playwright package from clients/web — run \npm install` at the repo root, with a comment stating why playwright installcan't fix that branch (binaries ≠ package). Thechromium.launch()catch at:115-121keeps--with-deps` and now names the cause explicitly ("for the system libraries"), so the two branches are no longer confusable.

Both messages are now actionable for the failure they actually describe: a bare box missing devDependencies gets npm install; a box with the package but no system libs gets --with-deps. Worth noting the ordering is right too — npx playwright install chromium in the npm script runs before the script, so if the package genuinely isn't installed, npx fetches it transiently, installs the binaries, and the script's requireFromWeb still fails — landing on exactly the message that tells you to run npm install.

CI state (corrected/completed picture)

0c271598  in_progress   ← this head (comment/message strings only)
9c576d72  in_progress
43823223  success       ← the last FUNCTIONAL change (async console signature)
b97b407b  success
45e828e9  success
d12c3a69  success       ← playwright createRequire fix
bb808371  failure       ← the pre-fix playwright resolution failure
acd3f98c  failure

Your claim checks out: 4382322 — the async-signature commit, the last behavioral change on the branch — is green in CI, and everything after it touches only comments, docs, and one error/console.log string. So the functional surface of this PR is CI-proven; the two in-progress runs can only regress on a formatting/parse issue, and node --check passes on all three scripts.

Final diff pass — nothing further

• scripts/lib/prod-web-server.mjs — childError separate from exited; bootFailed() feeds waitForReady/whenChildExits while stop() still guards on exited alone (no orphaned launcher on the port); waitForReady keeps polling non-ok but reports the last status on timeout.
• scripts/smoke-web-browser.mjs — pageerror ∪ FATAL_CONSOLE-matching console errors are the hard failure; the complement prints as a note; settle window (networkidle best-effort + 500 ms) before the assertion; render raced against launcher death. Re-checked FATAL_CONSOLE has no /g flag, so the two .test() passes at :188 and :197 can't interact through lastIndex — a real hazard in this exact shape, and it's absent here.
• scripts/smoke-web.mjs — down to the HTML assertions, with a header cross-reference to the companion and the shared helper.
• .github/workflows/main.yml — cache restore → playwright install --with-deps chromium → npm run smoke → Storybook; cache key unchanged, comment explains the reorder.
• AGENTS.md / README.md / package.json — two-channel wording consistent across the script header, the smoke:web:browser bullet, and the smoke row; smoke:web:browser appended last in smoke so it fails after the cheaper checks have passed; helper documented on both smoke bullets.
• Deliberately-left items (res.status !== 200 reachable only for a non-200 2xx, bootFailed() folding childError, whenChildExits's sibling listener, both-channel message duplication, the double prod-web boot) — agreed on all five; none can produce a false pass.

Verdict: LGTM — approved to merge once the in-progress run on this head goes green (the functional commit 4382322 already did). Seven rounds in: the smoke asserts exactly what it verifies, on both channels the crash class can arrive through; the shared helper removes the drift risk between the two web smokes; the docs match the behavior; and the CASE-2 gap this smoke can't see by design is tracked in #1769.

Caveat on verification: this checkout has no node_modules and no client builds, so I reviewed by reading the diff, node --check, and the branch's CI history — I did not execute the smoke or prettier --check.
· branch v2/ci-headless-web-boot-smoke-1615