build(web): fail vite build on a browser-externalized Node built-in (#1769)

[cliffhall]

Closes #1769

Problem

Follow-up from #1615 / PR #1768. Vite 8 (rolldown), when a node:* / bare Node built-in reaches the browser graph, externalizes it with a warning and still succeeds the build, shipping a module.exports = {} stub:

• CASE 1 — the stub is called at module init → crashes at runtime → caught by smoke:web:browser (CI: add a headless-browser boot smoke for the web client #1615), but only there, and only at runtime.
• CASE 2 — the stub is imported but never called → ships a harmless-looking {} and is invisible to the runtime smoke by design → nothing gates it.

Fix

A Vite build plugin turns that specific browser-externalization warning into a hard vite build error, so a Node built-in in the browser graph now fails npm run build / validate — upstream of the smoke, and covering both cases.

• clients/web/server/browser-externalized-builtin-gate.ts — the Vite-agnostic logic (phrase match, error, per-build state machine). Unit-tested to the ≥90 gate; no Vite import, so it's trivially testable.
• clients/web/vite.config.ts — the thin Vite wiring. Key subtlety: throwing inside rolldown's onLog is swallowed (it's the one hook where a thrown error doesn't abort the build — verified against vite@8.0.0). So the gate records the warning in onLog and re-throws in buildEnd, where a throw aborts the build with a non-zero exit. Scoped to vite build (apply: 'build') — never vite dev or the vitest projects; the Node runner build (tsup, build:runner) is a separate config where built-ins are legitimate.

Guarding the version-drift risk the issue calls out

There is no stable log code on the rolldown warning (it carries only message + plugin: "rolldown:vite-resolve" — captured against vite@8.0.0), so the gate keys off the documented message phrasing (…has been externalized for browser compatibility). The issue notes this phrasing differs across Vite releases and could silently disable a message-keyed gate.

scripts/verify-build-gate.mjs (npm run verify:build-gate, wired into npm run ci and the GitHub workflow) closes that: it forces a node:fs import into src/main.tsx, runs a real vite build, and asserts the build fails via the #1769 gate — then restores the entry. The unit tests cover the detection logic against a captured string; only this real build catches a live-Vite phrasing drift. smoke:web:browser stays as the runtime backstop for crashes the build can't reason about.

Verification

• vite build with a Node built-in in the browser graph → fails with the actionable Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615) #1769 error (exit 1). ✓
• Clean vite build of the real app → succeeds, other warnings (chunk-size) still print. ✓
• npm run verify:build-gate → OK, and leaves src/main.tsx clean. ✓
• Full npm run ci green locally (validate, coverage gate incl. the new module, verify:build-gate, smoke, Storybook).

Docs

Updated AGENTS.md (build-gate section next to smoke:web:browser), the root README.md testing table, and the CI workflow.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777 (build gate for browser-externalized Node built-ins)

• Gather context (read diff + changed files)
• Review gate logic + Vite wiring
• Review verify-build-gate.mjs + CI wiring
• Review tests & docs
• Post review

---

Summary

Good shape overall. The split — Vite-agnostic detection in browser-externalized-builtin-gate.ts, thin wiring in vite.config.ts — is the right call and makes the logic trivially unit-testable. The onLog-records / buildEnd-throws workaround for rolldown swallowing throws in onLog is documented at the call site, apply: 'build' correctly keeps it off vite dev and the vitest projects, and verify:build-gate is the piece that actually earns the message-keyed match by proving it against a live Vite. Tests are thorough (first-offender retention and per-instance state are both covered), and file placement matches the existing src/test/integration/server/ convention with clients/web/server/** already in the coverage include.

Findings below, most significant first. Nothing here is a blocker for the gate's primary purpose.

---

1. Gate state is per plugin instance, not per build — sticky failure across rebuilds

clients/web/vite.config.ts:74 creates the gate in the plugin factory, which runs once per config evaluation. The docblock at clients/web/server/browser-externalized-builtin-gate.ts:61 says "State is per-instance so each build gets a fresh gate" — that's only true when each build gets a fresh plugin instance. Under vite build --watch, or any flow that builds twice in one process with the same resolved config, the recorded warning persists: once tripped, every subsequent rebuild fails even after the offending import is removed, and the reported message points at the already-fixed module.

Today's scripts don't use --watch (ensure-web-build.ts:31 spawns a subprocess, so no in-process reuse), so this is latent rather than live — but it's a one-line fix and it makes the docblock true:

buildStart() { gate.reset(); },   // plus a reset() on the gate interface

Fix this →

2. buildEnd() ignores its error argument — can mask the real build failure

clients/web/vite.config.ts:81. Rollup/rolldown call buildEnd(error?) on the failure path too, then rethrow the original error — but a throw from the hook propagates first, so the original never surfaces. If a build both fails for an unrelated reason (syntax error, unresolved import) and externalized a built-in, the developer sees the #1769 message instead of the actual cause.

buildEnd(error) { if (!error) gate.assertClean(); },

Note this doesn't weaken verify:build-gate — the probe build has no other error, so the gate is still the thing that fails it.

Fix this →

3. scripts/verify-build-gate.mjs:49 — shell rm -rf instead of rmSync

Every other script in scripts/ uses rmSync(..., { recursive: true, force: true }) (smoke-cli.mjs:414, pack-and-verify.mjs:73, smoke-tui.mjs:102). Spawning rm is Windows-hostile and its failure is silently swallowed (the result isn't checked), so on a platform without rm the cache-clear silently no-ops. node:fs is already imported in this file — swapping is a two-token change.

Related question: is clearing node_modules/.vite actually needed for vite build? That's the dev dep-optimizer cache; if the build doesn't consult it, the delete just costs the next npm run dev a full re-optimize (a real papercut for anyone running npm run ci locally). If it is needed for the warning to re-fire, the comment at :48 is worth strengthening to say which cache and why.

Fix this →

4. scripts/verify-build-gate.mjs:68 — a spawn failure is reported as the wrong diagnosis

If npx can't be spawned (ENOENT), result.status is null and output is empty, so the script falls through to "vite build failed, but not via the #1769 gate — the build broke for another reason". That sends someone hunting a build regression that doesn't exist. clients/web/server/ensure-web-build.ts:40 has the precedent — surface result.error first with its own message.

Fix this →

5. Docs: the new module isn't in the two server/ file listings

AGENTS.md and the root README.md testing table are nicely updated, but the AGENTS.md maintenance rule ("When adding … any file or folder, update the corresponding entry in the main README.md and/or the related clients/*/README.md") also covers:

• AGENTS.md:12 — the clients/web/server/ inline listing in the project-structure block.
• clients/web/README.md:14-21 — the bulleted server/ file list.
• Optionally the root README.md scripts/ line ("install cascade, smokes, pack:verify") could gain verify:build-gate.

Worth a sentence in the web README noting why a build-config helper lives under server/ — that README currently defines server/ as "the dev/prod backend wiring (never imported by the browser)", and this module is neither backend nor browser.

Fix this →

6. Nits

• Interrupt safety (scripts/verify-build-gate.mjs:54): the finally restores src/main.tsx, but a Ctrl-C during the multi-minute build skips it and leaves the entry mutated. A SIGINT/SIGTERM handler that restores-and-exits would close that. The post-restore verification at :60 is a nice touch either way.
• Build output is captured, not inherited (:52): a several-minute vite build prints nothing until it finishes. Other smokes use stdio: "inherit"; here output is needed for the assertions, so this is a fair trade — but a console.log before the spawn saying "running a real vite build, this takes a minute" would help.
• Quote style: the new module uses single quotes; every other file in clients/web/server/ uses double. Not caught by anything today (clients/web's format:check is prettier --check src, so server/ and vite.config.ts aren't format-gated) — pre-existing gap, just noting the drift.
• browser-externalized-builtin-gate.ts:68 runs past the wrap width the rest of the file keeps; same un-gated-formatting reason.

What I checked and found fine

• Coverage: all four exports exercised, both sides of the ?? and && branches hit — should clear the ≥90 gate without any v8 ignore.
• Test placement matches the seven existing src/test/integration/server/*.test.ts files.
• apply: 'build' means the Storybook build also gets the gate, which is a bonus rather than a problem.
• The probe at :32-34 is robust: the externalization warning fires at resolve time, so even a tree-shaken reference would trip it, and npx vite build (not npm run build) correctly skips tsc -b, which would reject globalThis.__never__.
• CI step ordering is sensible (after coverage, before the Playwright install + smokes).
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Thanks — thorough review. Addressed all of 1–5 plus the nits in 011efbab.

1. Per-build state / sticky failure under watch — fixed. Added reset() to the gate and call it from a buildStart hook, so a vite build --watch rebuild starts fresh instead of failing forever on an already-fixed import. Added a reset() unit test, and the docblock now describes the plugin resetting in buildStart rather than claiming per-instance freshness.

2. buildEnd masking a real build error — fixed. buildEnd(error) now only calls assertClean() on the success path (if (!error)), so an unrelated failure (syntax error, unresolved import) surfaces as itself instead of being overwritten by the #1769 message. verify:build-gate is unaffected — the probe build has no other error, so the gate is still what fails it (re-verified).

3. rm -rf — removed entirely. I tested the reviewer's question directly: the gate fires reliably without any cache clear (two consecutive probe builds both failed with #1769, no clearing). My original "caching" concern was a misdiagnosis — it was actually the onLog-swallow behavior before I moved the throw to buildEnd. So rather than swap to rmSync, I dropped the cache-clear (and the viteCache var) altogether: no Windows-hostile rm, and no re-optimize papercut for the next npm run dev.

4. Spawn-failure misdiagnosis — fixed. Added an explicit result.error check (mirroring ensure-web-build.ts) that fails with the spawn error itself, before the status === 0 / #1769 branches.

5. Docs — fixed. Added browser-externalized-builtin-gate.ts to the AGENTS.md project-structure server/ block and the clients/web/README.md server/ bullet list — the latter with a sentence on why a build-config helper lives under server/ (Node-only, build-time tooling imported by the Vite config, never the browser, alongside the other vite-* helpers). Added verify-build-gate to the root README scripts/ line.

6. Nits:

• Interrupt safety — added SIGINT/SIGTERM handlers that restore src/main.tsx and exit, so a Ctrl-C during the build no longer leaves the entry mutated.
• Progress log — added a "running a real vite build (takes a minute)…" line before the spawn.
• Quote style — switched the new module to double quotes to match its server/ siblings. (Agreed the underlying gap is that clients/web's format:check only covers src/, leaving server/ + vite.config.ts ungated — I've left that broader gap alone here.)
• Wrap width — reflowed the over-long line.

Good catch on the apply: 'build' → Storybook build also getting the gate; that's intended and welcome.

npm run ci is green locally (validate, coverage incl. the new module, verify:build-gate, smoke, Storybook). Will re-request review once CI clears on the new commit.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 2 (after 011efbab)

• Gather context (read full diff + changed files)
• Verify the six round-1 findings are actually addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All six round-1 findings are genuinely fixed, and a couple of them better than suggested — dropping the rm -rf entirely (after testing that the gate fires without any cache clear, and correctly re-attributing the original "caching" symptom to the onLog swallow) is the right call over swapping in rmSync. CI is green on 011efbab, which is meaningful here: it means the new verify:build-gate step passed with the buildStart reset in place, so the reset demonstrably doesn't erase a warning recorded earlier in the build.

What's left is one docs gap and one robustness point on the verify script; the rest are nits. No blockers.

---

1. AGENTS.md still enumerates the old ci chain in two places

The root README.md:147 was updated to validate → coverage → verify:build-gate → smoke → Storybook, and a good new bullet was added at AGENTS.md:225. But the two places in AGENTS.md that actually enumerate the pipeline weren't:

• AGENTS.md:215 — "npm run ci is the mandatory pre-push command — it mirrors .github/workflows/main.yml (minus npm install): validate → coverage → smoke → Storybook…"
• AGENTS.md:221 — "CI runs validate, then the coverage gate …, then smoke … Storybook is the only CI step left out."

Both now under-describe the pipeline, and the second one's "Storybook is the only CI step left out" is simply false with the new step present. AGENTS.md is the file agents load every session and the one that claims to mirror the workflow — a stale enumeration there is exactly how someone later "corrects" package.json's ci back to the documented chain.

Fix this →

2. npx vite build can silently run a different Vite than the pinned one

scripts/verify-build-gate.mjs:65. The entire purpose of this script is proving the message-keyed gate still fires against the live, pinned Vite. But npx falls back to fetching a package from the registry when the bin isn't resolvable locally — so if clients/web/node_modules is missing or partial (a fresh clone where only the root install ran, INSPECTOR_SKIP_CLIENT_INSTALL=1, an interrupted install), the check quietly runs against whatever vite npm hands it. A pass then proves nothing about this repo's Vite, and a fail sends someone chasing a phrasing drift that doesn't exist in the version they ship.

npx --no-install vite build turns that into an explicit failure (caught by the new result.error branch, which is nice), or spawn clients/web/node_modules/.bin/vite directly. The existing npx tsc calls in smoke-cli.mjs:64 / smoke-tui.mjs:46 are the same convention, but they don't carry version-fidelity semantics — this one does.

Fix this →

3. The gate isn't environment-scoped (forward-looking)

clients/web/vite.config.ts:56 — apply: 'build' scopes to vite build, but every environment built from this config, not just the browser one. Today that's fine (the client env is the only one here; the Node runner is a separate tsup config), so this is latent — but the module docblock and the AGENTS.md prose both frame this as "the browser graph", and if an SSR/node environment is ever added to this config its legitimate node:* imports would fail the build with a #1769 message. Vite 6+'s applyToEnvironment: (env) => env.name === 'client' (or a this.environment.name check in onLog) makes the browser-only intent structural rather than incidental.

Fix this →

4. Only the first offender is reported

browser-externalized-builtin-gate.ts:70-77 keeps the first match and drops the rest — deliberate, and the test at :67 pins it. Worth reconsidering the cost though: with a multi-minute vite build, N leaked built-ins means N fix-and-rebuild cycles. Collecting every matched message and listing them in one error is a few lines and strictly better DX; the "first offender" framing was more about state hygiene than about wanting to hide the others.

Fix this →

5. Nits

• Signal handlers can't run while spawnSync blocks (verify-build-gate.mjs:52-57). The Ctrl-C case works, but by way of the process group rather than the handler: SIGINT reaches vite too, the child dies, spawnSync returns, the finally restores, and only then does the queued handler run (and find restored === true). A kill <parent-pid> alone leaves vite building and the handler pending until it finishes. The handlers are a correct backstop — worth one clarifying line in the comment so a future reader doesn't assume prompt teardown.
• PROBE is prepended (:31-33, :61). Harmless today, but it would demote a leading directive if main.tsx ever gains one ("use client"). ES imports hoist, so appending would be equally effective and directive-safe.
• scripts/ isn't format-gated either. Same class as the server/ gap you noted: the root format covers core/ plus each client's own scope, so nothing prettier-checks scripts/*.mjs — verify-build-gate.mjs:24 already runs past the width prettier would enforce. Pre-existing, not introduced here.

What I checked and found fine

• All six round-1 items land correctly: reset() + buildStart + a dedicated test; buildEnd(error) guarded to the success path with the masking rationale in the comment; cache-clear removed outright with the misdiagnosis explained; result.error checked before the status/#1769 branches, mirroring ensure-web-build.ts; docs added in all three places (the "why it lives under server/" sentence in clients/web/README.md:22 is a good addition); quotes/wrap/progress-log/signal nits done.
• The dist ordering hazard I went looking for isn't one. verify:build-gate sits between coverage and smoke in both package.json and the workflow, and it runs a build that intentionally fails — but the throw is in buildEnd, i.e. before output generation and emptyOutDir, so clients/web/dist is untouched and the following smoke:web / smoke:web:browser still serve the real bundle from validate. (And if it ever were wiped, ensure-web-build.ts rebuilds on demand.)
• CI green on 011efbab is real evidence, not just a formality: the workflow now runs verify:build-gate, so the gate provably still fires with the buildStart reset in place — the one ordering risk the reset introduced (a warning logged before buildStart being erased) is disproven for the actual path, since externalization is a resolve-time log.
• Test placement and the .js-extension import match the seven sibling src/test/integration/server/*.test.ts files; clients/web/server/** is in the coverage include (vite.config.ts:151) and not excluded; all four exports plus reset() are exercised on both branches of the ?? and && — should clear ≥90 with no v8 ignore.
• The new module is lint-gated (clients/web's lint is eslint ., which reaches server/), and vite.config.ts typechecks under tsconfig.node.json, so the Plugin typing and the buildEnd(error) signature are both statically checked.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-2 addressed in a8a368a7 — took all four substantive points plus the nits.

1. Stale ci enumerations in AGENTS.md — fixed. Updated both pipeline enumerations (the "mandatory pre-push" line and the "CI runs…" line) to include verify:build-gate, and dropped the now-false "Storybook is the only CI step left out" claim. Good catch that this is the exact file someone would use to "correct" package.json back.

2. npx running a different Vite than pinned — fixed. Switched to npx --no-install vite build, so the check can never silently fetch a registry Vite when clients/web/node_modules is missing/partial — a missing local bin now surfaces via the result.error branch instead. Added a comment explaining why version fidelity specifically matters here (it's the whole point of the script). Agreed the plain-npx convention in the other smokes is fine because they don't carry version-fidelity semantics.

3. Environment scoping — fixed. Added applyToEnvironment: (env) => env.name === 'client' so the gate is structurally browser-only; a future SSR/node environment built from this config won't be failed for a legitimate node:* import. Verified the gate still fires for the client build (verify:build-gate green) and the clean build still passes (smoke). Noted the browser scoping in the module docblock too.

4. Report all offenders — done. The gate now collects every matched warning (deduped, via a Set) and lists them together in the error, so a build leaking several built-ins reports them in one pass rather than one-per-rebuild. browserExternalizedBuiltinError takes string[]; updated the tests (multi-offender + dedupe cases).

5. Nits:

• Signal-handler comment — clarified that while spawnSync blocks, a Ctrl-C tears down vite via the shared process group first (child dies → spawnSync returns → finally restores), and the handlers are the backstop for a kill <pid> that targets only this process.
• Probe prepend → append — appended instead (ES imports hoist, so it still externalizes at resolve time, and appending won't demote a future leading directive like "use client").
• scripts/ format gap — left as-is; it's the same pre-existing "not format-gated" class as server/, out of scope for this PR.

On the two "what I checked and found fine" notes: yes — the dist ordering is safe (the throw is in buildEnd, before output/emptyOutDir), and CI-green-on-the-commit is the real evidence that buildStart's reset doesn't erase a resolve-time warning.

npm run ci is green locally. Will re-request review once CI clears on a8a368a7.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 4m 39s —— View job

---

Code review — PR #1777, round 3 (after a8a368a7)

• Gather context (read diff + changed files)
• Verify the round-2 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All four round-2 substantive items plus the nits are genuinely addressed, and CI is green on a8a368a7 — which matters more than usual here, because the workflow now runs verify:build-gate, so the newly-added applyToEnvironment: (env) => env.name === 'client' narrowing is proven not to have silently switched the gate off. That's the exact failure mode an environment filter could have introduced, and it's covered by evidence rather than by argument.

The multi-offender collection (Set + listed together) is a real DX win over one-per-rebuild, --no-install closes the version-fidelity hole properly, and the signal-handler comment now describes what actually happens rather than what one would hope happens.

What's left is one docs-wording regression, one design consideration on the verify script's cost/fidelity tradeoff, and nits. No blockers — this is mergeable as-is.

---

1. AGENTS.md:222 — the replacement wording is muddled, and arguably still wrong

The stale enumeration is fixed (good), but the sentence that replaced "Storybook is the only CI step left out" reads:

Storybook is the only step in npm run ci that GitHub CI runs as its own tail step (see below).

GitHub CI runs every step of the ci chain as its own step — validate, coverage, verify:build-gate, smoke, and Storybook are five separate run: steps in .github/workflows/main.yml. So "the only step … that GitHub CI runs as its own step" is false, and the only thing salvaging it is the word "tail," which makes it a tautology (the last step is the last step). The original sentence was trying to say something about validate scope, and the rewrite inherited its confusion instead of resolving it.

Since this bullet's whole job is to let a reader confirm package.json's ci matches the workflow, it should just say that plainly — e.g. "GitHub CI runs the same chain as separate steps, with the Storybook play-function tests last."

Fix this →

2. verify:build-gate costs a full app build and mutates a tracked source file

scripts/verify-build-gate.mjs:68 writes into clients/web/src/main.tsx and then runs a full vite build of the real app — by the script's own admission, "takes a minute" — on every CI run and every local npm run ci. All of that just to assert one string survives a Vite bump.

The cheaper shape: export the plugin factory alongside the pure logic (import { type Plugin } from "vite" is type-only, so it wouldn't pull Vite into the unit-test path or cost the current testability split), and have the verify script build a throwaway temp entry with a minimal generated config. That's seconds instead of minutes, and it deletes the source-mutation hazard entirely — no probe in a tracked file, no restore path, no finally, no signal handlers, no post-restore verification. Roughly half this script's complexity exists to manage a mutation that a temp entry wouldn't need.

The counter-argument is real and worth stating, because it may well win: building the actual config is what catches config-level regressions — someone deleting the plugin from the plugins array, or adding a build.rollupOptions.onwarn suppression above it. A temp config would keep passing through all of those, degrading this from "the gate works in this repo" to "the gate's string still matches." If that fidelity is the intent (and I think it is), it deserves a line in the script docblock saying so — otherwise the next person to notice the minute-long build will "optimize" it into a temp config and quietly lose the property you built it for.

Fix this →

3. No escape hatch for a legitimately externalized built-in

browser-externalized-builtin-gate.ts:45 — the gate is all-or-nothing: any matching warning fails the build for every contributor, with no allowlist and no opt-out env var. Today the repo builds clean so this is latent, but the realistic trigger isn't first-party code — it's a transitive dependency that does import fs from "node:fs" behind a runtime branch it never takes in the browser. Under the old behavior that shipped a harmless {} (CASE 2, by construction the case this PR exists to catch); under the gate it's a hard npm run build failure that a contributor can't fix by editing their own code.

The error text does give direction ("Remove the node:* imports … or gate them behind the Node-only dev backend"), but neither of those applies when the import is inside node_modules. Naming the actual remedy for that case — a resolve.alias to a browser shim, or adding the module to an allowlist — would turn a dead-end error into an actionable one. Worth at minimum a sentence in the error; an optional createBrowserExternalizedBuiltinGate({ allow: [...] }) is the fuller version if you'd rather not wait for someone to hit it.

Fix this →

4. Two small robustness gaps in the verify script

• scripts/verify-build-gate.mjs:38 — if src/main.tsx is ever renamed or moved, readFileSync throws a raw ENOENT stack instead of going through fail(). The rest of the script is careful about actionable diagnostics; this one path isn't. A guarded read naming the entry (and the fact that the path is hardcoded here) is two lines.
• :89 — the restore-failure remedy is git checkout -- clients/web/src/main.tsx, which discards any uncommitted edits the developer had in that file. The script is holding the correct content in original, so the honest remedy is to write it somewhere recoverable (a .bak next to the entry) and point at that, rather than suggesting a command that can destroy work. This message only ever prints when something already went wrong, which is exactly when the advice shouldn't compound it.

Fix this →

5. Nits

• Redundant guard (browser-externalized-builtin-gate.ts:77): message !== undefined && isBrowserExternalizedBuiltinLog(message) — the callee already handles undefined via ?? false, so the first clause is dead weight and an extra branch to keep covered (the test at :77 currently carries it). Dropping it makes recordLog a one-liner with no coverage cost.
• The Storybook "bonus" is narrower than round 1 suggested. applyToEnvironment is right, but combined with apply: 'build' the gate now only reaches a manual storybook build (test:storybook is vitest/serve, so apply: 'build' already excluded it) — and that path isn't in CI, so whether Storybook's builder names its environment client is untested. No action needed; just don't count Storybook as covered.
• AGENTS.md:224 describes the scoping as apply: 'build' only — the new applyToEnvironment client-only narrowing is documented in the code comment and the module docblock but not in the prose that agents actually load. Half a sentence.
• scripts/ still isn't format-gated — agreed this is the pre-existing server/ class of gap and out of scope; noting only that this PR adds a 122-line file to the ungated set, so the gap grows slightly.

What I checked and found fine

• All four round-2 items verified in the code, not just in the reply: applyToEnvironment at vite.config.ts:56; npx --no-install at verify-build-gate.mjs:77 with the version-fidelity rationale in-comment; Set-based multi-offender collection at :74–:83 with dedupe + multi-offender tests at :81/:92; both AGENTS.md enumerations now carry verify:build-gate; probe appended not prepended with the directive-safety reasoning at :31–:33; signal-handler comment now accurately describes process-group teardown as the real path and the handler as the kill <pid> backstop.
• No --logLevel anywhere in the build scripts, so nothing starves rolldown's onLog of the warning before the gate sees it — a --logLevel silent in a future script would be a silent gate-disabler, and today there's none.
• False-positive coverage is handled by ordering, not luck: ci runs validate (which does a real build:web) before verify:build-gate, so "the gate doesn't fire on a clean app" is already gated — this script only needs to prove the no-false-negative direction, which it does.
• Coverage should clear ≥90 with no v8 ignore: all four exports plus reset() exercised, and both sides of ?? false, !== undefined &&, and size > 0 are hit.
• Test import depth (../../../../server/…js) and placement match the sibling src/test/integration/server/*.test.ts files; clients/web/server/** is in the coverage include at vite.config.ts:154 and not excluded.
• Docs land in all four places this time — AGENTS.md structure block, clients/web/README.md:22 (with the good "why it lives under server/" justification), the root README scripts/ line, and the root README testing table.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-3 addressed in f4db356a — took all five findings.

1. Muddled AGENTS.md CI-steps sentence — fixed. You're right, "the only step … as its own tail step" was false-or-tautological. Replaced with a plain statement: GitHub CI runs the same chain as separate workflow steps, with the Storybook play-function tests last.

2. Why the real build, not a temp entry — documented. Agreed the fidelity is the point (a temp config keeps passing when someone deletes the plugin or adds an onwarn suppression above it — the exact regressions this exists to catch). Added a docblock paragraph saying so explicitly, ending with "Do NOT optimize it into a temp entry — that silently loses the config-regression coverage," so the next person eyeing the minute-long build doesn't quietly trade the property away. Kept the real-config approach.

3. Transitive-dependency escape hatch — error message extended. The remedy text now covers the case where the node:* import comes from a dependency, not first-party code: add a resolve.alias in vite.config.ts pointing the module at a browser shim (referencing the module path in the warning). I went with the message rather than the allow: [...] option — it's latent (repo builds clean today), and adding config API + tests for an unhit path felt like speculative surface; the actionable error turns the dead-end into a fix, and the allowlist is a clean follow-up if someone actually hits it.

4. Verify-script robustness — both fixed.

• Guarded the entry read: a renamed/missing main.tsx now fails through fail() with an actionable message (and a note that the path is hardcoded here) instead of a raw ENOENT stack.
• Restore-failure remedy no longer recommends git checkout -- (which discards uncommitted edits). It now writes the captured original to a sidecar .bak and points there — the honest remedy, since the script is holding the correct content and this message only prints when something already went wrong.

5. Nits:

• Redundant guard — removed, properly. Rather than just dropping message !== undefined (which would break the Set<string>.add() typing), I made isBrowserExternalizedBuiltinLog a message is string type guard, so recordLog narrows and the clause is genuinely unnecessary. One-liner now, still type-safe.
• Storybook "bonus" — noted, not counting it as covered; apply: 'build' already excludes the vitest/serve test:storybook path, and a manual storybook build isn't in CI.
• AGENTS.md scoping prose — added the applyToEnvironment client-only narrowing to the gate bullet (it was only in the code comment/docblock before).
• scripts/ format gap — agreed, pre-existing and out of scope; noted that this PR does grow the ungated set.

Thanks for the three passes — the transitive-dep remedy and the .bak-over-git checkout fix in particular are real improvements I'd have shipped without.

npm run ci is green locally. Will re-request review once CI clears on f4db356a.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 4 (after f4db356a)

• Gather context (read diff + changed files)
• Verify round-3 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All five round-3 items landed, and #5's fix is better than what I suggested — turning isBrowserExternalizedBuiltinLog into a message is string predicate (browser-externalized-builtin-gate.ts:36-40) removes the redundant clause and keeps Set<string>.add() type-safe, instead of just deleting the guard and papering over the typing. The .bak-over-git checkout -- restore and the transitive-dependency remedy in the error text are both real improvements.

This is mergeable. The remaining findings are all in scripts/verify-build-gate.mjs's failure diagnostics — the gate itself and its Vite wiring I have nothing left on.

---

1. The status === 0 failure message names the wrong cause for the regression class the script was just documented to catch

scripts/verify-build-gate.mjs:129-136. When the probe build succeeds, the script says:

the gate did not fire (the warning phrasing likely drifted in a Vite bump; update BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts)

But the round-3 docblock (:18-26) now states a second reason this script builds the real config: catching config-level regressions — "the plugin being deleted from the plugins array, or a build.rollupOptions.onwarn suppression added above it." Add applyToEnvironment to that list: if a future Vite renames the browser environment from client, the plugin silently stops applying (vite.config.ts:53) — the most realistic silent-disabler now that the env filter exists.

In all of those cases the phrasing is still correct, and the script tells the reader to go edit the constant. That's not just unhelpful; it invites someone to "fix" a constant that already matches, and if they change it to something else that matches, the check goes green with the plugin still unwired.

The script already holds output, so the three causes are cheaply distinguishable:

output contains	actual cause
the known phrase	warning fired as expected but the build passed → plugin not applied (deleted from plugins, applyToEnvironment env-name mismatch, onwarn suppression)
node:fs but not the phrase	phrasing drift → update BROWSER_EXTERNALIZED_BUILTIN_PHRASE
neither	the probe never reached the browser graph (entry moved, probe tree-shaken)

Vite prints the externalization warning at default log level and nothing in these scripts passes --logLevel, so the phrase-presence check is reliable. This is the same class of fix as the round-2 result.error one: a failure that misdiagnoses itself sends someone at the wrong file.

Fix this →

2. No spawn timeout, and output is captured — a hung vite build gives zero diagnostics for 6 hours

scripts/verify-build-gate.mjs:98. Output is deliberately captured (needed for the assertions), which is a fair trade — but it means a build that hangs prints nothing, and the build job in .github/workflows/main.yml has no timeout-minutes, so it burns to GitHub's 360-minute default and then dies with an empty log for this step.

spawnSync takes timeout directly, and on timeout it sets result.error (ETIMEDOUT) — which the round-2 result.error branch at :123 already reports. So this is one option away from a bounded failure with a real message:

result = spawnSync("npx", ["--no-install", "vite", "build"], {
  cwd: webDir, encoding: "utf8", timeout: 10 * 60_000, killSignal: "SIGKILL",
});

Fix this →

3. The probe write is the one path not routed through fail()

scripts/verify-build-gate.mjs:89. Round 3 guarded the entry read (:57-64) so a renamed main.tsx fails actionably — but the write immediately after is unguarded. On a read-only checkout or an EACCES, it throws a raw stack out of the try/finally (the finally restores, then the exception escapes), which is exactly the raw-ENOENT-stack shape you just removed from the read path. Wrapping it to fail("could not write the probe into <entry> (…)") makes the script's diagnostics uniform.

Fix this →

4. Nits

• The gate's tests don't run in the fast loop. browser-externalized-builtin-gate.test.ts sits under src/test/integration/, so the integrationGlob excludes it from the unit project (vite.config.ts:29, :260) — npm run test and therefore validate never run it; it only executes under coverage. That matches the seven sibling server/*.test.ts files, so convention wins, but note those are there because they're Node-only I/O tests, whereas this module is pure and would run fine in unit. Fine as-is; just don't expect validate to catch a gate-logic regression.
• SIGTERM exits 130 (:83) — 130 is SIGINT's conventional code; SIGTERM's is 143. Cosmetic, and nothing reads it.
• browserExternalizedBuiltinError([]) produces an error with an empty "Original Vite warning(s):" list. Unreachable through the gate (size > 0 guards it at :87), but the function is exported and unguarded.

What I checked and found fine

• All five round-3 items verified in the code: the AGENTS.md CI-steps sentence now plainly says GitHub CI runs the same chain as separate steps with Storybook last; the docblock's "why the REAL config + entry … Do NOT optimize it into a temp entry" paragraph at :18-26 is exactly the guard that was missing; the transitive-dependency resolve.alias remedy is in the error text (gate.ts:53-56); the guarded read + .bak restore are in (:57-64, :110-118); the type-predicate refactor removes the redundant clause without weakening typing; AGENTS.md's gate bullet now documents the applyToEnvironment client-only narrowing.
• dist is still safe across the intentionally-failing build. The throw is in buildEnd, which precedes renderStart (where Vite's emptyOutDir runs), so clients/web/dist from validate survives into smoke:web. Worth knowing the safety net cuts both ways: ensure-web-build.ts would silently rebuild a wiped dist, so CI green wouldn't prove this — the ordering argument is what does.
• The #1769 marker is a sound assertion key: it's in the thrown message, Vite prints plugin errors, and nothing else in the build output emits it.
• No --logLevel anywhere in the build scripts, so nothing starves onLog — still true after this round's changes.
• Coverage: clients/web/server/** is in the include (vite.config.ts:154) and not excluded; all four exports plus reset() are exercised, both sides of ?? false and size > 0 hit, and the new multi-offender/dedupe/reset tests cover the round-2 additions. No v8 ignore needed.
• apply: 'build' + applyToEnvironment are proven live by the CI verify:build-gate step passing on a8a368a7/f4db356a — the env filter demonstrably didn't switch the gate off, which is the one thing that couldn't be established by reading.
• Docs land in all four places, and package.json's ci chain matches the workflow step order.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-4 addressed in d34cdddb — took the three findings plus the SIGTERM nit.

1. status === 0 message named the wrong cause — fixed. Split it into three diagnoses keyed off the captured output, so it points at the right file:

• output contains the known phrase → the plugin isn't applying (removed from plugins, applyToEnvironment no longer matching the browser env name, or an onwarn suppression) — pointing at vite.config.ts, not the constant.
• output has node:fs but not the phrase → phrasing drift → update BROWSER_EXTERNALIZED_BUILTIN_PHRASE.
• neither → probe never reached the graph (entry moved / tree-shaken).

Added a KNOWN_PHRASE literal for the check (kept as a literal with a pointer comment, like smoke-web.mjs does for TOKEN_GLOBAL, since this .mjs can't import the TS source), and added applyToEnvironment to the docblock's config-regression list. Good call that this is the same "a failure that misdiagnoses itself sends someone at the wrong file" class as the round-2 result.error fix.

2. No spawn timeout — fixed. Added timeout: 10 * 60_000 + killSignal: "SIGKILL". A hang now sets result.error (ETIMEDOUT) and reports through the existing branch with a bounded failure, instead of burning to the job's 360-min default with an empty log. Noted in the comment that spawnSync surfaces the timeout via result.error.

3. Unguarded probe write — fixed. Moved the probe write out of the restore-finally and guarded it: a read-only/EACCES checkout now fails through fail() with an actionable message before any mutation, matching the guarded read above it (and there's nothing to restore since nothing was written).

Nits:

• SIGTERM exit code — now 143 (SIGINT stays 130); 128 + signo.
• browserExternalizedBuiltinError([]) — left as-is: unreachable through the gate (size > 0 guards it), and adding an empty-input guard to an internally-only-non-empty caller is dead code. Noted rather than fixed.
• Gate tests run under coverage, not validate — left in src/test/integration/server/ to match the seven sibling server/*.test.ts; you called this "convention wins," and I'd rather not special-case one pure module out of that folder. Worth knowing validate won't catch a gate-logic regression, but coverage (and CI) do.

Thanks again for the four rounds — the three-way diagnosis and the timeout are genuine robustness wins.

npm run ci is green locally (including the new diagnostic paths). Will re-request review once CI clears on d34cdddb.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 5 (after d34cdddb)

• Gather context (read diff + changed files)
• Verify round-4 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All three round-4 findings plus the SIGTERM nit landed, and the three-way diagnosis (verify-build-gate.mjs:152-183) is a real improvement — a failing check now points at vite.config.ts, at the phrase constant, or at the probe, depending on what the captured output actually says.

The gate module and its Vite wiring I have nothing left on — browser-externalized-builtin-gate.ts is 95 lines of pure, fully-tested logic, and the plugin's four hooks (apply / applyToEnvironment / buildStart reset / buildEnd(error)-guarded assert) are each justified in-comment and proven live by the CI verify:build-gate step.

Everything below is in scripts/verify-build-gate.mjs. No blockers — this is mergeable.

---

1. The mirrored KNOWN_PHRASE can drift from the source constant, silently reintroducing the exact misdiagnosis round 4 removed

scripts/verify-build-gate.mjs:41-45 duplicates BROWSER_EXTERNALIZED_BUILTIN_PHRASE as a literal, with a comment saying the two mirror each other. Nothing enforces that. Walk the scenario the script exists for:

1. Vite bumps, the phrasing drifts, the check fires, someone updates BROWSER_EXTERNALIZED_BUILTIN_PHRASE in the gate to the new wording — and (very plausibly) not KNOWN_PHRASE, since the script is a different file and its failure message names only the gate constant.
2. Later, someone unwires the plugin (deletes it from plugins, or a Vite rename breaks the applyToEnvironment env-name match).
3. The probe build now succeeds. Output contains the new phrase; KNOWN_PHRASE holds the old one, so branch (a) misses and the script falls into branch (b): "the warning phrasing drifted — update BROWSER_EXTERNALIZED_BUILTIN_PHRASE."

That's the wrong file, and worse, it's the specific failure mode round 4 was about: someone edits a constant that already matches, and if they land on any string that matches, the check goes green with the plugin still unwired.

The precedent you cited (smoke-web.mjs:29-31, pack-and-verify.mjs:50-52 mirroring __INSPECTOR_API_TOKEN__) doesn't carry this risk — there a drift makes the smoke fail loudly; here it makes the diagnosis lie. A three-line guard closes it without needing to import the TS:

if (!readFileSync(gatePath, "utf8").includes(KNOWN_PHRASE)) {
  fail(`KNOWN_PHRASE here no longer appears in ${gateRel} — the mirrored literals drifted; re-sync them.`);
}

Fix this →

2. A failed restore escapes as a raw stack, so the .bak safety net never runs

scripts/verify-build-gate.mjs:125-128. Round 4 guarded the probe write; the restore write is still bare. If writeFileSync throws inside the finally (EACCES, disk full, the file became read-only mid-build), the exception propagates straight out — the restore-verification block at :133-141, which is the code that writes the .bak and prints the careful "restore from there, unlike git checkout --" message, never executes. The developer gets a raw stack and a tree left with node:fs injected into main.tsx, which is the worst combination this script can produce and precisely the one those 9 lines were written to prevent.

restoreEntry() already holds original, so routing the catch into the same .bak path (rather than letting it escape) makes the safety net actually reachable on the path that needs it most.

Fix this →

3. The "plugin isn't applying" cause list omits the one cause this PR introduced

scripts/verify-build-gate.mjs:158-166 lists three causes for warning present, build green: plugin removed from plugins, applyToEnvironment env-name mismatch, onwarn suppression. There's a fourth now — the buildStart reset added in round 1. If a future Vite emits the externalization warning before the client environment's buildStart (dep scan, a resolve during a prior phase), the gate records it and then reset() wipes it, and the symptom is byte-for-byte identical to the three listed causes. Today's ordering disproves it for this Vite (CI green), but that's exactly the kind of thing a Vite bump changes, and this message is the only place a reader will look. Half a clause.

Fix this →

4. Nits

• The failed-check path leaves a probe build in dist. On the status === 0 branch the build completed, so clients/web/dist now holds a bundle containing the node:fs probe and its {} stub. Harmless at runtime (the probe is never called) and the next validate overwrites it — but someone debugging a verify:build-gate failure locally and then running npm run web is serving a probe build with no indication. One clause in those three fail() messages would cover it. (The expected path is unaffected: the throw is in buildEnd, before output generation.)
• readFileSync at :65 inside try, original possibly undefined. Only reachable if fail() ever stopped exiting — fine today, just noting the implicit contract that fail() is noreturn isn't expressible in a .mjs.
• The gate matches at any log level (onLog(_level, log) ignores _level). Correct as written — the externalization log is a warning today, and not keying on level makes it robust to a level change — but it does mean an info-level log quoting the phrase would trip the build. Not worth guarding, just worth knowing.
• Gate tests still run only under coverage, not validate — unchanged from round 4, and I agree convention wins over special-casing one pure module out of src/test/integration/server/.

What I checked and found fine

• All four round-4 items verified in the code: the three-way diagnosis with distinct remediation per branch (:152-183); applyToEnvironment added to the docblock's config-regression list (:21); timeout: 10 * 60_000 + killSignal: "SIGKILL" with the in-comment note that spawnSync surfaces a timeout via result.error (:119-124); the probe write moved out of the restore-finally and guarded so an EACCES fails before any mutation (:95-104); SIGTERM → 143 with the 128 + signo rationale (:90-91).
• browserExternalizedBuiltinError([]) — agreed, leave it: size > 0 guards the only caller, and an empty-input branch would be dead code that then needs its own coverage.
• Coverage will clear ≥90 with no v8 ignore: clients/web/server/** is in the include (vite.config.ts:154) and the new module isn't in the exclude list; all four exports plus reset() are exercised, and both sides of ?? false and size > 0 are hit. The test helper's unreachable throw err / trailing-throw branches don't count — **/*.test.{ts,tsx} is excluded (:166).
• The type-predicate refactor is sound: message is string on a ?? false return is a correct narrowing (a match implies a defined string), and it's what lets recordLog be a guard-free one-liner while Set<string>.add() stays typed.
• npx --no-install vite build skips tsc -b (the web build is tsc -b && vite build && build:runner), which is required — globalThis.__never__ would fail typecheck. Confirmed against clients/web/package.json:14.
• Appending the probe is safe for main.tsx as it stands (no leading directive, imports hoist), and the directive-safety reasoning at :50-52 is the right thing to have written down.
• The round-3 AGENTS.md sentence reads correctly now — "GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last" is both true and useful; package.json's ci chain and the workflow step order still match.
• No --logLevel anywhere in the build scripts, so nothing starves onLog of the warning before the gate sees it — still true after this round.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-5 addressed in 371472f3 — took all three findings plus the dist nit.

1. KNOWN_PHRASE drifting from the source constant — fixed with a self-check. Added a drift guard that reads the gate module and asserts it still contains the mirrored KNOWN_PHRASE literal, failing fast if not. You're right this is a different risk class than the smoke-web.mjs token mirror (there a drift fails loudly; here it would make the diagnosis lie). Now a phrasing update that forgets to re-sync the script fails immediately with "the mirrored literals drifted; re-sync" rather than silently misdirecting a later plugin-unwiring to the wrong file.

2. Failed restore escaping the finally and skipping the .bak — fixed. restoreEntry now catches its own write failure and routes to a shared saveBackupAndFail helper (also used by the post-build content-mismatch check), so an EACCES/read-only/disk-full restore writes the sidecar .bak and fails actionably instead of throwing a raw stack out of the finally and leaving the probe injected — exactly the worst-case those lines exist to prevent.

3. buildStart-reset cause missing from the diagnosis — added. The "plugin isn't applying" branch now lists a fourth cause: a future Vite emitting the externalization warning before the client environment's buildStart reset runs (which would then clear it) — byte-identical symptom to the other three, and the only place a reader looks.

Nits:

• Probe build left in dist — added a single console.error note on the failing-check path (covering all three branches without repeating it) so a local debugger doesn't npm run web a probe bundle unaware. The expected path is unaffected (throw is in buildEnd, before output).
• original possibly-undefined after a non-exiting fail() / any-log-level match / gate tests only under coverage — noted, no change: fail() is noreturn (just not expressible in .mjs), matching at any level is deliberate robustness, and the test location follows the server/* convention you endorsed.

Really appreciate the five passes — the drift guard in particular turns a "keep these in sync by hand" comment into something enforced.

npm run ci is green locally. Will re-request review once CI clears on 371472f3.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 6 (after 371472f3)

• Gather context (read diff + changed files)
• Verify round-5 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All three round-5 findings plus the dist nit landed, and the drift guard (verify-build-gate.mjs:95-105) is the right shape — it converts a "keep these in sync by hand" comment into something enforced, which is exactly what a message-keyed gate needs.

The gate module, its tests, the Vite wiring, and the docs I have nothing left on. Everything below is in scripts/verify-build-gate.mjs, and it's all one theme: the script now defends carefully against false negatives, but three literals it asserts on can each go stale independently of what they mirror. No blockers — mergeable.

---

1. #1769 is too weak an assertion key — an unrelated build failure can report OK

scripts/verify-build-gate.mjs:230. The success condition is "build failed and the output mentions #1769". But #1769 also appears three times in clients/web/vite.config.ts comments (:31, :54, :93) — the file that surrounds the plugin, and therefore the file most likely to be mid-edit when someone breaks the gate. If a build error there produces a code frame spanning those lines, the output contains #1769 without the gate ever having fired, and the script prints:

verify:build-gate OK — vite build fails on a Node built-in in the browser graph (#1769 gate fired).

That's the one direction the script must never get wrong: every other diagnosis in here is a false alarm that misdirects, but this is a false pass that hides a dead gate. Narrow, but it's free to close — the thrown error's own prefix is distinctive and unambiguous:

if (!output.includes("Build failed (#1769)")) {

browserExternalizedBuiltinError (browser-externalized-builtin-gate.ts:50) leads with exactly that string, so nothing about the passing path changes. (Same argument for KNOWN_PHRASE: worth a comment noting these two literals are the script's contract with the gate module.)

Fix this →

2. The drift guard checks the whole file, not the constant

scripts/verify-build-gate.mjs:99 asserts browser-externalized-builtin-gate.ts contains KNOWN_PHRASE anywhere. Today the only occurrence is the constant at gate.ts:30-31, so the guard is sound. But the scenario the guard exists for is someone updating the phrase after a Vite bump — and the natural way to do that is to change the constant while leaving the old wording in the surrounding comment (gate.ts:22-29 is already a paragraph explaining where the phrase comes from; "previously …" is a very ordinary edit). The guard passes, KNOWN_PHRASE is stale, and the three-way diagnosis is back to lying — precisely the outcome the guard was added to prevent.

Anchoring on the assignment makes the guard actually mean "the constant matches":

const gateSource = readFileSync(gatePath, "utf8");
if (!new RegExp(`BROWSER_EXTERNALIZED_BUILTIN_PHRASE\\s*=\\s*["'\`]${KNOWN_PHRASE}["'\`]`).test(gateSource))

Also: this readFileSync is unguarded, so a renamed/moved gate module throws a raw ENOENT stack — the same class you routed through fail() for the entry read at :86-93. It happens before any mutation so the tree stays clean, but the diagnostics should be uniform.

Fix this →

3. The probe module name is duplicated between PROBE and its diagnosis

PROBE hardcodes node:fs at :59; the branch-(b) diagnosis independently tests output.includes("node:fs") at :213. Change the probe to node:path (entirely plausible — say node:fs becomes legitimately aliased in the config someday) and branch (b) silently stops matching, so a genuine phrasing drift falls through to branch (c) and reports "the probe never reached the browser graph — check PROBE / entryPath." Wrong file again, and the same shape as findings 1 and 2. A const PROBE_MODULE = "node:fs" interpolated into both (and into the two message strings at :216/:224) makes it structural.

Fix this →

4. Nits

• Diagnosis ordering is right, and worth a comment. The three branches are checked phrase → node:fs → neither, which is the correct precedence (a phrase match implies the probe reached the graph, so it must win). It's load-bearing but not stated; one line above :202 would keep a future reader from reordering them.
• dist note prints via console.error before the diagnosis (:199) — correct channel and correct placement (all three branches fail() after it), no change needed.
• scripts/ still isn't format-gated — unchanged, pre-existing, and this file is now 241 lines of it. Not this PR's problem; worth a follow-up issue at some point given how much diagnostic prose now lives here.

What I checked and found fine

• All four round-5 items verified in the code: the drift guard at :95-105; restoreEntry's own try/catch routing to the shared saveBackupAndFail helper (:112-122, :71-83) so a failed restore reaches the .bak path instead of escaping the finally; the buildStart-reset cause added as the fourth entry in the plugin-isn't-applying diagnosis (:208-209); the probe-build-in-dist note (:199-201).
• The gate module is done. 95 lines, four exports, zero Vite import, message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed. The seven test cases cover both branches of ?? false and size > 0, plus dedupe / multi-offender / reset() / per-instance — ≥90 on all four dimensions with no v8 ignore, and clients/web/server/** is in the coverage include (vite.config.ts:154).
• The plugin's four hooks are each justified in-comment and each proven live: apply: 'build', applyToEnvironment: (env) => env.name === 'client', buildStart reset, buildEnd(error)-guarded assert. CI's verify:build-gate step passing on a8a368a7/f4db356a/d34cdddb is what establishes the env filter and the reset didn't quietly switch the gate off — that couldn't be settled by reading.
• onLog returns nothing, so the original Vite warning still prints alongside the gate's error — the developer sees both the module path and the remediation.
• dist survives the expected path: the throw is in buildEnd, which precedes output generation and emptyOutDir, so smoke:web / smoke:web:browser still serve the real bundle from validate.
• No --logLevel anywhere in the build scripts, so nothing starves onLog before the gate sees the warning.
• Docs are consistent across all five places — AGENTS.md structure block + the gate bullet (with the applyToEnvironment narrowing) + both pipeline enumerations, clients/web/README.md:22 with the "why it lives under server/" justification, the root README scripts/ line and testing table. package.json's ci chain and the workflow step order match.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-6 addressed in 223ec0fa — took all three findings plus the ordering-comment nit.

1. #1769 too weak a success key — fixed. Now asserts on the distinctive thrown-error prefix Build failed (#1769) (which browserExternalizedBuiltinError leads with), not a bare #1769. You're right this is the one direction the script must never get wrong — a false pass hiding a dead gate — and the bare number appears in vite.config.ts comments, so a code frame from an unrelated error there could have masked it. Added a comment noting this prefix + KNOWN_PHRASE are the script's contract with the gate module.

2. Drift guard checked the file, not the constant — fixed. Anchored it on the BROWSER_EXTERNALIZED_BUILTIN_PHRASE = "…" assignment via regex (with a small escapeRegExp), so the old wording lingering in the surrounding comment paragraph can't mask a changed constant — which was exactly the plausible edit that would have defeated the whole-file match. Also guarded the gate module's readFileSync so a moved/renamed file fails through fail() instead of a raw ENOENT stack, matching the entry read. Verified the regex matches the current constant and rejects a stale value.

3. Probe module duplicated — fixed. Extracted PROBE_MODULE = "node:fs" and derived both the PROBE string and the branch-(b) output.includes(...) check + its messages from it, so changing the probe module can't silently misroute a phrasing-drift failure into "probe never reached the graph."

Nits:

• Diagnosis order — added the "don't reorder these" comment (phrase before module-name, since a phrase match implies the probe reached the graph).
• dist note channel/placement — unchanged, you confirmed it's right.
• scripts/ not format-gated — agreed this has grown enough to deserve its own fix; I filed a follow-up issue for closing the server/ + scripts/ + vite.config.ts prettier/eslint gap (it's the same class you flagged in rounds 2–3), rather than expand this PR's scope. [link in a follow-up comment]

Six passes in and the findings have converged on "make the script's literal contracts self-enforcing," which they now are. Thanks for the thoroughness.

npm run ci is green locally. Will re-request review once CI clears on 223ec0fa.

[cliffhall]

Follow-up issue for the scripts/ + server/ + vite.config.ts prettier-gate gap: #1778 (labeled v2, on the board as Todo). Kept out of this PR to avoid scope creep.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 7 (after 223ec0fa)

• Gather context (read diff + changed files)
• Verify round-6 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All three round-6 findings plus the ordering-comment nit landed. The success key is now the distinctive Build failed (#1769) prefix (verify-build-gate.mjs:261), the drift guard is anchored on the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment rather than a whole-file match (:120-129), and PROBE_MODULE is the single source for both the probe and branch (b) of the diagnosis.

This should merge. The gate module, the plugin wiring, the tests, and the docs I have nothing on — and the findings have converged to the point where the marginal value of another round is below the cost of one. I'd take #1 (three lines, closes the last unguarded literal in the script's own stated contract) and treat the rest as optional.

---

1. The contract has two mirrored literals; only one is drift-guarded

scripts/verify-build-gate.mjs:256-260 says outright:

This prefix + KNOWN_PHRASE are the script's contract with browser-externalized-builtin-gate.ts's exported error.

KNOWN_PHRASE got the round-5/6 drift guard. "Build failed (#1769)" didn't — and it's mirrored the same way, across the same TS/.mjs boundary, for the same reason (the script can't import the source). Only two occurrences exist in the repo:

clients/web/server/browser-externalized-builtin-gate.ts:50:    "Build failed (#1769): a Node built-in reached the browser bundle and was "
scripts/verify-build-gate.mjs:261:if (!output.includes("Build failed (#1769)")) {

Nothing pins them together. The unit test asserts only expect(err.message).toContain("#1769") (browser-externalized-builtin-gate.test.ts:52), which a reworded prefix ("#1769: build failed — …") still satisfies. So a routine edit to that error message turns a correctly firing gate into:

vite build failed, but not via the #1769 gate — the build broke for another reason, so this check no longer proves the gate works.

Wrong file, wrong diagnosis — the same class round 4, 5, and 6 each closed one instance of. It's the benign direction (a false alarm, not a false pass, so it can't hide a dead gate), which is why it's the only item here — but the fix is three lines against gateSource, which is already read at :113:

const ERROR_PREFIX = "Build failed (#1769)";
if (!gateSource.includes(ERROR_PREFIX)) {
  fail(`ERROR_PREFIX here no longer appears in ${gateRel} — the mirrored literals drifted; re-sync them.`);
}

Fix this →

2. Nits

• Branch (b) costs an extra CI cycle (:240-246). On a genuine phrasing drift it says "update BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts" — and someone who does exactly that trips the new drift guard on the very next run ("the mirrored literals drifted"). Correct behavior, but the remediation should just say update both (the gate constant and KNOWN_PHRASE in this script) so the fix lands in one pass.
• The drift-guard regex assumes a single contiguous string literal (:120-122). If the constant is ever written as a concatenation or a template with an interpolation, the guard reports "the mirrored literals drifted" when they haven't. Unlikely (prettier doesn't split string literals), and cheapest to close from the other side: a // keep this a single string literal — scripts/verify-build-gate.mjs anchors on this assignment next to gate.ts:30.
• The .bak sidecar isn't gitignored. saveBackupAndFail writes clients/web/src/main.tsx.verify-build-gate.bak (:83); no .gitignore in this repo covers *.bak, so it lands as an untracked file inside src/ and a git add -A sweeps it into a commit — right when the developer is already recovering from a failed restore. Either add the pattern to clients/web/.gitignore or write it under os.tmpdir() and print the absolute path.
• browserExternalizedBuiltinError([]) still produces an empty warning list — agreed with your round-4 call to leave it (size > 0 guards the only caller; a guard would be dead code needing its own coverage).

What I checked and found fine

• All four round-6 items verified in the code: the Build failed (#1769) success key with its rationale comment (:256-261); the assignment-anchored drift guard with escapeRegExp and a guarded readFileSync of the gate module (:74-77, :111-129); PROBE_MODULE derived into both PROBE and branch (b) plus its messages (:56, :64-66, :239-251); the "don't reorder these" precedence comment (:226-227).
• The gate module is complete: 95 lines, four exports, zero Vite import, the message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed, and eight test cases covering both branches of ?? false and size > 0 plus dedupe / multi-offender / reset() / per-instance. clients/web/server/** is in the coverage include — ≥90 on all four dimensions with no v8 ignore.
• The plugin's four hooks each carry their justification in-comment (vite.config.ts:31-68) and are proven live rather than by argument: CI's verify:build-gate step has passed on a8a368a7 / f4db356a / d34cdddb / 371472f3, which is what establishes that applyToEnvironment: (env) => env.name === 'client' and the buildStart reset didn't quietly switch the gate off — the one thing reading can't settle.
• onLog returns nothing, so the original Vite warning still prints alongside the gate's error; the developer sees the module path and the remediation (including the round-3 resolve.alias advice for a transitive-dependency import).
• dist is safe on the expected path — the throw is in buildEnd, before output generation and emptyOutDir, so smoke:web / smoke:web:browser still serve the real bundle from validate; the status === 0 path (build completed) now flags the probe bundle explicitly (:223-225).
• No --logLevel anywhere in the build scripts, so nothing starves onLog of the warning before the gate sees it.
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (with the applyToEnvironment narrowing), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README scripts/ line and testing table. package.json's ci chain matches the workflow step order, and the workflow step's comment explains the restore.
• Follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778 is the right call for the scripts/ + server/ + vite.config.ts prettier gap — genuinely out of scope here.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-7 addressed in d01fe18d — took finding #1 and all three nits.

1. Second mirrored literal now drift-guarded too. Extracted ERROR_PREFIX = "Build failed (#1769)" and added it to the drift guard against the gate module (right beside the KNOWN_PHRASE check, reusing the gateSource already read), and use it as the success key. You're right it was the last unguarded literal in the script's own stated contract — a reworded gate error would've made a correctly-firing gate report "the build broke for another reason." Benign direction, but the same class the last three rounds each closed one instance of, so worth finishing.

Nits:

• .bak sweeping into git — fixed. saveBackupAndFail now writes under os.tmpdir() and prints the absolute path, instead of a sidecar inside src/ that a recovery-time git add -A would commit. Cleaner than gitignoring — it keeps the repo untouched entirely.
• Branch-(b) remediation — fixed. It now says update BOTH the gate constant AND KNOWN_PHRASE in one pass, so following the advice doesn't immediately trip the drift guard on the next run.
• Single-literal assumption — pinned from the other side. Added a note next to BROWSER_EXTERNALIZED_BUILTIN_PHRASE that it must stay a single string literal (no concat/template) or the regex-anchored drift guard reports a false drift.
• browserExternalizedBuiltinError([]) — left as-is per the round-4 call (unreachable through the gate; a guard would be dead code needing its own coverage).

Agreed the findings have converged — the script's two literal contracts with the gate module are now both self-enforcing, and I think this is the natural stopping point. Thanks for the seven passes; the gate is materially more robust for them.

npm run ci is green locally. Will re-request review once CI clears on d01fe18d.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 8 (after d01fe18d)

• Gather context (read diff + changed files)
• Verify round-7 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

Round-7's item and all three nits landed: ERROR_PREFIX is extracted and drift-guarded beside KNOWN_PHRASE (verify-build-gate.mjs:53, :141-146, :279), the .bak moved to os.tmpdir(), branch (b) now says update both copies, and gate.ts:30-32 pins the constant as a single string literal from the other side. CI is green on d01fe18d — which is the evidence that matters for this round, since tightening the success key from #1769 to Build failed (#1769) could only be validated by a real build actually emitting that prefix, and verify:build-gate passed with it.

I also ran the drift guard's regex against the current gate source locally: it matches the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment, ERROR_PREFIX is present, and the phrase appears exactly once in the file — so the anchoring is doing what it claims.

Ship it. Eight rounds in, the gate module, its tests, the plugin wiring, and the docs are done. The three items below are all in verify-build-gate.mjs; I'd take #1 (a recovery-path message that can state something false) and treat #2/#3 as optional.

---

1. saveBackupAndFail tells you to restore from a .bak it may never have written

scripts/verify-build-gate.mjs:87-102. The write is best-effort with an empty catch, but the message that follows is unconditional:

try { writeFileSync(backupPath, original); } catch { /* best effort */ }
fail(`${reason} — pre-run contents were saved to ${backupPath}; restore from there …`);

So on an EACCES/full-disk temp dir the developer is told to restore from a path that doesn't exist — in the one code path that only ever runs when the restore already failed and main.tsx still has node:fs injected. This is the same "a failure that misdiagnoses itself" class rounds 4–7 each closed one instance of, except here the misdirection lands during recovery.

The fixed filename compounds it: path.join(tmpdir(), "main.tsx.verify-build-gate.bak") is a shared-namespace path. On a multi-user box or a CI runner with a persistent /tmp, that file can already exist owned by someone else — the write fails, the catch swallows it, and the message now points at another run's (or another user's) main.tsx while saying "restore from there." Following that advice overwrites the entry with foreign content.

Both close with a few lines: track whether the write succeeded and word the message accordingly, and use mkdtempSync(path.join(tmpdir(), "verify-build-gate-")) for a collision-free directory.

Fix this →

2. The gate is last in plugins, so an earlier plugin's onLog could filter the warning before it arrives

clients/web/vite.config.ts:92-97. In Rollup's onLog contract (which rolldown mirrors), returning false from a plugin's onLog filters the log — no later plugin's onLog runs. The gate is registered third, after react() and honoMiddlewarePlugin(...); neither defines onLog today (I grepped — no onLog/onwarn anywhere else in the config or server/), so this is latent, not live.

What makes it worth a line rather than nothing is how it would fail. A filtered log isn't printed either, so the captured output would contain neither the phrase nor node:fs — and verify-build-gate.mjs:266-271 would report "the probe never reached the browser graph … Check this script's PROBE / entryPath." Wrong file, and the one diagnosis that points away from the plugin. enforce: 'pre' on the plugin puts it ahead of the normal-plugin group in the onLog chain and makes "the gate sees every log" structural rather than dependent on array position — same move as applyToEnvironment in round 2.

Fix this →

3. The restore-verification read is the last unguarded readFileSync

scripts/verify-build-gate.mjs:216. Rounds 3–6 routed the entry read, the gate-module read, and the probe write through fail(); this one still throws a raw stack. It's the least likely to fire — but it's the read that decides whether the restore worked, so a throw there skips saveBackupAndFail entirely and leaves the probe injected with no .bak and no actionable message. Routing it into saveBackupAndFail (rather than fail) makes the whole restore path uniformly safe.

Fix this →

4. Nits

• Signal-handler exit code degrades on a failed restore (:153-176): if restoreEntry() inside a SIGINT handler hits a write failure, saveBackupAndFail → fail → process.exit(1), so the carefully-chosen 130/143 never applies. Arguably correct (a failed restore is an error), just not what the 128 + signo comment implies.
• The unit test carries a third copy of the phrase (browser-externalized-builtin-gate.test.ts:22-25) — but it self-checks against the exported constant at :31, so it can't drift silently. Good as-is; worth knowing the mirror count is three, not two.
• escapeRegExp is fine for the current phrase (no metacharacters), and correctly escapes the set that matters outside character classes.
• browserExternalizedBuiltinError([]) — still agree with the round-4 call to leave it (size > 0 guards the only caller).

What I checked and found fine

• All four round-7 items verified in the code, not just the reply: ERROR_PREFIX defined at :53 with the contract comment at :46-51, drift-guarded at :141-146 reusing the already-read gateSource, and used as the success key at :279; the .bak under tmpdir() (:88-91); branch (b) naming both constants (:260-262); the single-string-literal pin at gate.ts:30-32.
• The drift guard actually works — I executed its regex against the current gate source: matches the assignment, rejects a whole-file-only occurrence, and the phrase appears exactly once in the file so there's no comment copy to mask a change today.
• Vite is effectively pinned despite ^8.0.0. clients/web/package-lock.json locks vite@8.0.0, and the workflow's npm install honors it — so the gate module's "verified against the pinned vite@8.0.0" is accurate, and a future 8.x bump necessarily lands as a lockfile change that verify:build-gate then exercises against the new phrasing. That's the design working end to end.
• CI green on d01fe18d proves the tightened success key. Build failed (#1769) is a stricter assertion than the previous bare #1769; the only way to know Vite surfaces the full prefix in plugin-error output was a real run, and the workflow's verify:build-gate step passing is that run.
• The gate module is complete: 98 lines, four exports, zero Vite import, the message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed. Eight test cases cover both branches of ?? false and size > 0 plus dedupe / multi-offender / reset() / per-instance; clients/web/server/** is in the coverage include and unexcluded — ≥90 on all four dimensions with no v8 ignore.
• onLog returns nothing, so the original Vite warning still prints alongside the gate's error — the developer sees the offending module path and the remediation (including the round-3 resolve.alias advice for a transitive-dependency import).
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (with both the apply: 'build' and applyToEnvironment scoping), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README scripts/ line and the testing table. package.json's ci chain matches the workflow's step order, and the workflow step's comment explains the entry mutation and restore.
• Follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778 remains the right home for the scripts/ + server/ + vite.config.ts prettier gap.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-8 addressed in d2a16de3 — took all three findings.

1. saveBackupAndFail claiming a backup it may not have written — fixed. It now tracks whether the write succeeded and only tells the user to restore from the .bak in that case; otherwise it says the probe is still injected and to restore from version control. And it writes into a fresh mkdtempSync(path.join(tmpdir(), "verify-build-gate-")) dir instead of a fixed tmp filename, so a pre-existing or foreign-owned /tmp/main.tsx.verify-build-gate.bak can't make the recovery message point at someone else's content. Agreed this was the sharpest one — a misdirection that lands during recovery is the worst place for it.

2. enforce: 'pre' on the gate plugin — added. You're right that a later-registered plugin returning false from onLog would filter the log for the gate and make verify-build-gate misreport it as "the probe never reached the browser graph" (the one diagnosis pointing away from the plugin). No plugin defines onLog today, so it was latent, but enforce: 'pre' makes "the gate sees every log" structural rather than array-position-dependent — same spirit as the round-2 applyToEnvironment. Verified the gate still fires with it (verify:build-gate green, typecheck clean).

3. Restore-verification read guarded — fixed. Routed it through saveBackupAndFail too, so the entire restore path is uniformly safe; a raw throw there would previously have skipped the backup net.

Nits:

• Signal-handler exit code on a failed restore — left as-is: a failed restore is an error, so saveBackupAndFail → exit 1 overriding the 130/143 is the right precedence; the 128 + signo comment describes the normal path.
• Third phrase copy in the test — it self-checks against the exported constant, so it can't drift silently; leaving it.
• escapeRegExp / empty-array — no change (both sound as noted).

That's eight rounds; the recovery path is now uniformly guarded and the onLog ordering is structural. I believe this is done.

npm run ci is green locally. Will re-request review once CI clears on d2a16de3.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 9 (after d2a16de3)

• Gather context (read diff + changed files)
• Verify round-8 findings are addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

All three round-8 findings landed: saveBackupAndFail now tracks whether the backup write actually succeeded and words the remedy accordingly, into a collision-free mkdtempSync dir (verify-build-gate.mjs:90-106); enforce: 'pre' makes the gate's position in the onLog chain structural (vite.config.ts:56-62); and the restore-verification read routes through saveBackupAndFail (:222-229), so every read and write on the mutation path is now guarded.

CI is green on d2a16de3, which is the evidence that matters this round — enforce: 'pre' reorders the plugin relative to react() and the Hono plugin, and the only way to know that didn't perturb the build was a real verify:build-gate run against it.

I also re-ran the drift guard's own logic locally against the current gate source: the assignment regex matches, ERROR_PREFIX is present, and the phrase occurs exactly once in the file — so both mirrored literals are genuinely pinned, not just nominally.

Ship it. The gate module, the plugin wiring, the tests, and the docs are done. One optional item below, which only bites in standalone use; I'd take it or not, but I wouldn't hold the PR for it.

---

1. The success key proves a gate firing, not the probe's — a false pass when run standalone

scripts/verify-build-gate.mjs:293. The pass condition is "build failed and output contains Build failed (#1769)". That's now well-guarded against an unrelated error masquerading as the gate (round 6) — but it doesn't establish that the probe is what tripped it. If the repo already leaks a built-in from first-party or dependency code, the gate fires on that, and the script reports:

verify:build-gate OK — vite build fails on a Node built-in in the browser graph (#1769 gate fired).

…even if the probe was tree-shaken or entryPath had gone stale — i.e. exactly the condition branch (c) at :280-285 exists to catch, silently inverted into a pass.

Inside npm run ci and the workflow this is unreachable by ordering: validate runs build:web first, so a pre-existing leak already fails the job before this step. But the script is documented as a standalone command in both README.md:146 and AGENTS.md, and a developer debugging a leak locally is precisely the person who'd run it alone. The gate's error embeds every original warning (browser-externalized-builtin-gate.ts:59-60), and those warnings name the module — so PROBE_MODULE already gives you the check for free:

if (!output.includes(PROBE_MODULE)) {
  fail(`the #1769 gate fired but ${PROBE_MODULE} isn't among the offenders — it tripped on a pre-existing leak, so this run doesn't prove the probe reached the graph.`, output);
}

That closes the last direction where this check can report OK without having proven what it claims.

Fix this →

2. Nits

• A synchronous spawnSync throw escapes as a raw stack (:207). spawnSync reports spawn failures via result.error (handled), but it throws on an invalid options object — e.g. if someone later passes a bad timeout. The finally still restores the entry, so the tree stays clean; it's only the diagnostic that regresses to a stack. Bottom of the barrel given every other path is now guarded, and arguably a programmer error that should show a stack.
• enforce: 'pre' puts the gate's buildEnd first among plugins, so on a build that both leaks a built-in and has another plugin's buildEnd error, the Build gate: fail vite build on a browser-externalized Node built-in (SEP of #1615) #1769 message wins the race. That's the intended precedence (the if (!error) guard already handles the case where the build failed before buildEnd), and the alternative ordering is worse — noting it only because enforce: 'pre' was added for onLog and quietly affects buildEnd too.
• The phrase now has three copies — the gate constant, KNOWN_PHRASE, and REAL_MESSAGE in the test (browser-externalized-builtin-gate.test.ts:22-25). The third self-checks against the exported constant at :31, so it can't drift silently; unchanged from round 8, still fine.

What I checked and found fine

• All three round-8 items verified in the code, not just the reply: the savedTo tracking with a distinct "no backup was written — restore from version control" remedy (:99-104); mkdtempSync(path.join(tmpdir(), "verify-build-gate-")) replacing the fixed tmp filename, with the rationale in-comment (:84-89); enforce: 'pre' with a comment explaining that a later plugin returning false from onLog would filter the log for the gate (:56-62); the guarded restore-verification read (:222-229).
• The drift guards actually work — executed against the live gate source: the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment regex matches, ERROR_PREFIX is present, and the phrase appears exactly once, so no comment copy could mask a changed constant today. The // keep it a single string literal pin at gate.ts:30-32 protects the regex from the other side.
• dist ordering is still safe. The expected path throws in buildEnd, before output generation and emptyOutDir, so the real bundle from validate survives into the workflow's smoke step — and the workflow confirms the order (validate → coverage → verify:build-gate → Playwright install → smoke → Storybook), matching package.json's ci chain exactly. The status === 0 paths that do complete a build now flag the probe bundle explicitly (:254-256).
• The gate module is complete: 98 lines, four exports, zero Vite import, the message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed. Eight test cases cover both branches of ?? false and size > 0 plus dedupe / multi-offender / reset() / per-instance; clients/web/server/** is in the coverage include and unexcluded — ≥90 on all four dimensions with no v8 ignore.
• onLog returns nothing, so the original Vite warning still prints alongside the gate's error — the developer sees the offending module path and the remediation, including the transitive-dependency resolve.alias advice.
• No --logLevel anywhere in the build scripts, so nothing starves onLog of the warning before the gate sees it — still true after enforce: 'pre'.
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (both apply: 'build' and applyToEnvironment documented), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README scripts/ line and the testing table. The workflow step carries its own explanatory comment.
• Follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778 remains the right home for the scripts/ + server/ + vite.config.ts prettier gap — genuinely out of scope here.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-9 addressed in cbe2ab5b — took finding #1.

Success key now proves the probe fired the gate, not just a gate firing. Added an assertion that PROBE_MODULE (node:fs) appears among the gate's embedded offender warnings after the ERROR_PREFIX check. You're right this was the last direction the check could report OK without proving what it claims — a repo already leaking a built-in would trip the gate on that, masking a tree-shaken probe or a stale entryPath (branch (c)'s failure, inverted into a pass). Unreachable inside npm run ci (validate's build:web fails first on any pre-existing leak), but the script is a documented standalone command run exactly when someone's debugging a leak — the case where it mattered.

Nits — all left as-is with agreement:

• spawnSync synchronous throw on a bad options object — a programmer error that should show a stack; the finally still restores, so the tree stays clean.
• enforce: 'pre' also orders buildEnd first — the intended precedence (the if (!error) guard already handles a build that failed before buildEnd), and the alternative is worse.
• Third phrase copy in the test — self-checks against the exported constant, can't drift silently.

Nine rounds in, every read/write on the mutation path is guarded, both mirrored literals are drift-guarded, the onLog ordering is structural, and the success key now proves the probe specifically. I believe this is genuinely done.

npm run ci is green locally.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 10 (after cbe2ab5b)

• Gather context (read diff + changed files)
• Verify round-9 finding is addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

Round 9's item landed: the success path now asserts PROBE_MODULE appears among the gate's embedded offender warnings (verify-build-gate.mjs:308-315), so a gate firing on a pre-existing leak can no longer report OK while the probe was tree-shaken or entryPath went stale. That was the last direction the check could pass without proving what it claims.

This is converged and should merge. The gate module (98 lines, four exports, zero Vite import, eight tests), the plugin's five hooks, and the docs are done — I have nothing on any of them. One real finding below, in the verify script's one remaining unguarded mutation path; it's low-probability but it's the exact class rounds 4–8 each closed one instance of, and the fix is one line.

---

1. A partial probe write destroys main.tsx with no restore and no backup

scripts/verify-build-gate.mjs:186-192. The comment above it (:183-185) says the guarded write means a failure "fails actionably before any mutation" — that's true for EACCES/EROFS, where the open itself fails. It is not true in general: writeFileSync defaults to flag 'w' (O_WRONLY | O_CREAT | O_TRUNC), so the file is truncated at open and the data written after. An ENOSPC, EDQUOT, or EIO between those two steps leaves main.tsx empty or half-written — and this catch calls fail() directly:

try {
  writeFileSync(entryPath, original + PROBE);
} catch (err) {
  fail(`could not write the probe into ${...} (${err.message})`);   // no restore, no .bak
}

So the script exits 1 having wiped the browser entry, and the developer gets a message saying it couldn't write the probe — implying nothing happened. Every other write and read on this path is now routed into the recovery net: restoreEntry catches its own write failure and falls back to saveBackupAndFail (:157-167), the entry read is guarded (:109-116), the gate-module read is guarded (:126-134), and the restore-verification read routes through saveBackupAndFail (:222-229). This one path is the exception, and it's the only one that can leave the tree destroyed rather than merely dirty.

original is already in memory, so the fix is to attempt the in-place restore first — which itself falls back to the .bak if it can't write:

} catch (err) {
  restoreEntry();  // truncating open may have already emptied the entry
  fail(`could not write the probe into ${path.relative(repoRoot, entryPath)} (${err.message})`);
}

and soften the comment from "before any mutation" to "before the build, and any partial write is rolled back."

Fix this →

2. Nits

• enforce: 'pre' also moves the gate ahead of Vite's core plugins, not just the two user plugins in the array — including rolldown:vite-resolve, the plugin that emits the externalization warning. That's harmless (the gate defines no resolveId/load/transform, and onLog delivery doesn't depend on the emitting plugin's position), and CI green on d2a16de3/cbe2ab5b confirms it. The in-comment rationale at vite.config.ts:56-62 frames it purely as ordering within the user-plugin array, which slightly understates the reach.
• result.status === null with no result.error — a child killed by a signal (an OOM-killed rolldown, say) leaves status null, error unset, and probably no ERROR_PREFIX in the output, so it reports "the build broke for another reason." Accurate enough, and result.signal would let it say so precisely. Bottom of the barrel.
• scripts/ isn't in the published files allowlist (only scripts/install-clients.mjs is), so the new 320-line script adds nothing to the tarball. Confirmed, not a concern.

What I checked and found fine

• Round 9's item verified in the code: the PROBE_MODULE assertion at :308-315 sits after the ERROR_PREFIX check, with the reasoning (the gate error embeds every original warning, and those name the module) and the "unreachable inside npm run ci because validate's build:web fails first" caveat both written down.
• Both drift guards still hold against the live source — I re-ran their logic: the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment regex matches, ERROR_PREFIX is present in the gate module, and the phrase occurs exactly once in the file, so no comment copy could mask a changed constant. The // keep it a single string literal pin at gate.ts:30-32 protects the regex from the other side.
• No other onLog/onwarn handler exists anywhere in clients/web — grepped; the only two hits are the gate module and its wiring. So enforce: 'pre' is guarding a latent risk, as described, not fixing a live one.
• Vite is effectively pinned: clients/web/package.json declares ^8.0.0 but the lockfile pins 8.0.0, so a bump necessarily lands as a lockfile change that verify:build-gate then exercises against the new phrasing. That's the whole design working end to end.
• dist ordering is safe. The expected path throws in buildEnd, before output generation and emptyOutDir, so the real bundle from validate survives into smoke; the three status === 0 paths (which did complete a build) flag the probe bundle once at :254-256.
• The gate module is complete and fully covered: clients/web/server/** is in the coverage include and unexcluded; all four exports plus reset() are exercised, both sides of ?? false and size > 0 are hit, and the dedupe / multi-offender / reset / per-instance cases are pinned. ≥90 on all four dimensions with no v8 ignore.
• onLog returns nothing, so Vite's original warning still prints alongside the gate's error — the developer sees the offending module path and the remediation, including the transitive-dependency resolve.alias advice.
• No --logLevel anywhere in the build scripts, so nothing starves onLog before the gate sees the warning.
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (both apply: 'build' and applyToEnvironment documented), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README scripts/ line and testing table. package.json's ci chain matches the workflow's step order, and the workflow step carries its own explanatory comment.
• Follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778 remains the right home for the scripts/ + server/ + vite.config.ts prettier gap.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-10 addressed in 50eb49f4 — took finding #1.

Partial probe write can no longer destroy main.tsx — fixed. You're right the "before any mutation" comment was wrong: writeFileSync's default 'w' truncates at open, so a mid-write ENOSPC/EIO leaves the entry empty or half-written. The catch now calls restoreEntry() before fail(), rolling the partial write back (and falling through to the .bak net if the restore itself can't write) — closing the one path that could leave the tree destroyed rather than merely dirty. Corrected the comment to "before the build, and any partial write is rolled back."

Nits:

• enforce: 'pre' reach — widened the comment to note it orders the gate ahead of Vite's core plugins too (including the emitting rolldown:vite-resolve), and why that's harmless (the gate defines no resolve/load/transform, and onLog delivery doesn't depend on the emitter's position).
• Signal-killed child (status === null, no error) — left as-is; "the build broke for another reason" is accurate enough, and it's genuinely bottom-of-the-barrel.
• scripts/ not in the files allowlist — confirmed non-issue, no change.

Ten rounds in, every read and write on the mutation path — including a truncating partial write — is now routed into the recovery net, both mirrored literals are drift-guarded, the onLog ordering is structural, and the success key proves the probe specifically. This is done.

npm run ci is green locally.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 11 (after 50eb49f4)

• Gather context (read diff + changed files)
• Verify round-10 finding is addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

Round 10's item landed: the probe write's catch now calls restoreEntry() before fail() (verify-build-gate.mjs:189-196), and the comment above it correctly says "before the build, and any partial write is rolled back" rather than the old, false "before any mutation." CI is green on 50eb49f4.

The gate module, the plugin's five hooks, the eight unit tests, and the docs are done — I have nothing on any of them, and haven't for several rounds now.

One finding below, and it's a direct consequence of round 10's fix: the rollback is correct, but it swallows the primary error message in the two most likely ways the write fails. I verified this by executing the control flow, not by reading it. It's a two-line fix, and after it I'd merge without another pass.

---

1. The round-10 rollback makes the round-4 diagnostic unreachable on a read-only checkout

scripts/verify-build-gate.mjs:189-196:

try {
  writeFileSync(entryPath, original + PROBE);
} catch (err) {
  restoreEntry();
  fail(`could not write the probe into ${...} (${err.message})`);
}

restoreEntry() doesn't return on failure — it calls saveBackupAndFail() → fail() → process.exit(1) (:157-167, :90-106). So whenever the rollback write also fails, the fail() on the next line never executes. I ran the exact control flow to confirm:

$ node -e '<the same try/catch/restoreEntry/saveBackupAndFail shape>'
FAIL -> failed to restore main.tsx (EACCES: permission denied, open main.tsx) — <backup remedy>

The "could not write the probe" message — added in round 4 precisely for the read-only-checkout case — is gone.

That matters because the two realistic triggers both fail both writes:

trigger	probe write	rollback write	what the developer sees
read-only checkout / EACCES / EROFS	fails at open	fails at open	"failed to restore main.tsx … pre-run contents were saved to /tmp/…; restore from there"
ENOSPC / EDQUOT	fails mid-write	fails (disk still full)	same
transient EIO	fails mid-write	succeeds	correct message ✓

In the top row nothing was ever written — the entry is byte-identical to what the developer had — yet they're told the restore failed and pointed at a /tmp backup to recover from, which is the recovery-misdirection class rounds 8 and 10 each closed one instance of, reintroduced from the other side. It also writes a mkdtemp backup of an unmodified file as a side effect.

Keeping the rollback but not letting it swallow the diagnosis:

} catch (err) {
  // Roll back a possible partial write ('w' truncates at open) — but don't let
  // a failing rollback swallow the primary error, which is the actual diagnosis.
  let rolledBack = false;
  try {
    writeFileSync(entryPath, original);
    restored = rolledBack = true;
  } catch { /* reported in the message below */ }
  fail(
    `could not write the probe into ${path.relative(repoRoot, entryPath)} (${err.message})` +
      (rolledBack
        ? ""
        : " — the entry may be truncated or partially written; restore it from version control"),
  );
}

Fix this →

2. Nits

• ERROR_PREFIX isn't pinned as contiguous the way KNOWN_PHRASE is. The drift guard at :145 is a plain gateSource.includes(ERROR_PREFIX), and in the gate module the prefix sits inside the first fragment of a concatenated message (gate.ts:53). BROWSER_EXTERNALIZED_BUILTIN_PHRASE carries a "keep it a single string literal" pin (gate.ts:30-32) protecting its regex; the prefix has no equivalent, so a reflow like "Build failed " + "(#1769): …" would trip the guard with a false drift. Prettier doesn't split string literals, so this is theoretical — a half-line comment next to the error message would close it symmetrically.
• Signal handlers are live before the probe write (:175 vs :190). A SIGINT in that window runs restoreEntry(), writing original over an unmodified file — a harmless no-op on a writable tree, but on a read-only one it takes the saveBackupAndFail path for a file that was never touched. Same root cause as Progress notifications #1; fixing Progress notifications #1 doesn't cover this one, and it's rare enough not to be worth its own guard.
• browserExternalizedBuiltinError([]) — still agree with the round-4 call to leave it (size > 0 guards the only caller; a guard would be dead code needing its own coverage).

What I checked and found fine

• Round 10's item verified in the code: restoreEntry() in the probe-write catch (:192), and the comment corrected to name the 'w'-truncates-at-open mechanism and the rollback (:183-188). The enforce: 'pre' comment was also widened to note it orders the gate ahead of Vite's core plugins including the emitting rolldown:vite-resolve, and why that's harmless (vite.config.ts:56-67).
• Both drift guards still hold against the live source — re-ran their logic: the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment regex matches, ERROR_PREFIX is present in the gate module, and the phrase occurs exactly once in the file, so no comment copy could mask a changed constant.
• The round-9 probe assertion is sound in practice: Vite's warning is Module "node:fs" has been externalized…, the gate embeds every original warning verbatim (gate.ts:59-60), and output.includes(PROBE_MODULE) therefore matches — confirmed by CI's verify:build-gate step passing on cbe2ab5b and 50eb49f4, which is the only way that assertion could be validated.
• The gate module is complete: 98 lines, four exports, zero Vite import, the message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed. Eight tests cover both branches of ?? false and size > 0 plus dedupe / multi-offender / reset() / per-instance; clients/web/server/** is in the coverage include and unexcluded — ≥90 on all four dimensions with no v8 ignore. The test's third copy of the phrase self-checks against the exported constant (:31), so it can't drift silently.
• dist ordering is safe. The expected path throws in buildEnd, before output generation and emptyOutDir, so the real bundle from validate survives into smoke; the three status === 0 paths that did complete a build flag the probe bundle once (:258-260).
• onLog returns nothing, so Vite's original warning still prints alongside the gate's error — the developer sees the offending module path and the remediation, including the transitive-dependency resolve.alias advice.
• No --logLevel anywhere in the build scripts, so nothing starves onLog before the gate sees the warning.
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (both apply: 'build' and applyToEnvironment documented), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README scripts/ line and testing table. package.json's ci chain matches the workflow's step order, and the workflow step carries its own explanatory comment.
• Follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778 remains the right home for the scripts/ + server/ + vite.config.ts prettier gap.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-11 addressed in ec8117f7 — took finding #1 (a regression my round-10 fix introduced, good catch).

Rollback no longer swallows the primary write error — fixed. You're right: restoreEntry() in the catch exits via saveBackupAndFail→fail on its own failure, so the "could not write the probe" diagnosis was unreachable exactly when both writes fail (read-only checkout / ENOSPC) — and on a read-only tree nothing was ever written, so pointing at a /tmp backup was pure misdirection. Switched to an inline rollback that doesn't exit: it attempts the restore, tracks rolledBack, then always reports the PRIMARY write error, appending "restore it from version control" only when the rollback didn't take. Confirmed the control flow reports the write error now, not the restore one.

Nits:

• ERROR_PREFIX contiguity — pinned. Added a comment next to the gate's error message saying to keep "Build failed (#1769)" in a single fragment, symmetric with the BROWSER_EXTERNALIZED_BUILTIN_PHRASE pin, since the script drift-guards on it as a substring.
• SIGINT in the pre-write window — same root cause, rare, and (as you said) not worth its own guard; left it.
• browserExternalizedBuiltinError([]) — left per the round-4 call.

npm run ci is green locally.

[cliffhall]

@claude review

[claude]

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

---

Code review — PR #1777, round 12 (after ec8117f7)

• Gather context (read diff + changed files)
• Verify round-11 finding is addressed
• Fresh review of gate logic, Vite wiring, verify script, tests, docs
• Post review

---

Summary

Round 11's item landed correctly: the probe-write catch now rolls back inline with a rolledBack flag instead of calling restoreEntry() (verify-build-gate.mjs:189-210), so the primary write error is always the one reported.

One finding, and it's a real regression introduced by the round-11 commit — the comment added to pin ERROR_PREFIX contains the literal it pins, which defeats the round-7 drift guard that checks for it. I verified this by executing the guard's logic against the live source, not by reading. It's a one-line fix on the comment side.

After that, ship it. The gate module, the plugin's five hooks, the eight unit tests, and the docs are done and have been for several rounds.

---

1. The round-11 pin comment defeats the ERROR_PREFIX drift guard it was written to protect

clients/web/server/browser-externalized-builtin-gate.ts:52 (added in ec8117f7):

  // Keep the "Build failed (#1769)" lead contiguous in a single fragment:
  ...
  return new Error(
    "Build failed (#1769): a Node built-in reached the browser bundle and was " +

The prefix now appears twice in the file, and the guard at verify-build-gate.mjs:145 is a plain whole-file substring test (gateSource.includes(ERROR_PREFIX)). So the comment satisfies the guard on its own. Executed against the live source:

$ node /tmp/chk.mjs
52:   // Keep the "Build failed (#1769)" lead contiguous in a single fragment:
57:     "Build failed (#1769): a Node built-in reached the browser bundle and was " +
---- simulate: reword the THROWN error, leave the pin comment ----
thrown-error prefix still emitted?  false
drift guard gateSource.includes(ERROR_PREFIX) -> true

Reword the thrown error ("#1769 build failure: …", or drop "Build failed") and leave the comment alone — the drift guard passes, and then a correctly-firing gate reports:

vite build failed, but not via the #1769 gate — the build broke for another reason, so this check no longer proves the gate works.

That's the exact misdirection round 7 added ERROR_PREFIX to prevent, and it's the same shape as the round-6 finding about the phrase guard ("the old wording lingering in a comment masks a changed constant") — reintroduced from the other side.

The asymmetry is the tell, and it's the fix: BROWSER_EXTERNALIZED_BUILTIN_PHRASE's pin comment (:28-32) deliberately doesn't repeat the phrase — I counted, it occurs exactly once in the file — which is what makes its assignment-anchored regex meaningful. Do the same here: reword :52 so it refers to the lead without quoting it verbatim (e.g. "Keep the error's #1769 lead contiguous in a single fragment…"), restoring the guard to one occurrence. The structural alternative — anchoring the check on new Error(\s*"Build failed (#1769) — also works and is robust to future comments, but is more machinery than the comment reword needs.

[Fix this →](https://claude.ai/code?q=On%20branch%20v2%2Ffix-1769-build-gate-externalized-builtin%2C%20in%20clients%2Fweb%2Fserver%2Fbrowser-externalized-builtin-gate.ts%20line%2052%20the%20pin%20comment%20quotes%20the%20literal%20%22Build%20failed%20(%231769)%22%20verbatim%2C%20so%20it%20appears%20twice%20in%20the%20file%20and%20scripts%2Fverify-build-gate.mjs%27s%20whole-file%20gateSource.includes(ERROR_PREFIX)%20drift%20guard%20at%20line%20145%20passes%20on%20the%20comment%20alone%20—%20rewording%20the%20thrown%20error%20would%20silently%20keep%20the%20guard%20green%20while%20a%20correctly-firing%20gate%20reports%20%22the%20build%20broke%20for%20another%20reason%22.%20Reword%20the%20comment%20so%20it%20does%20not%20repeat%20the%20literal%20(matching%20how%20the%20BROWSER_EXTERNALIZED_BUILTIN_PHRASE%20pin%20comment%20avoids%20repeating%20its%20phrase%2C%20which%20occurs%20exactly%20once%20in%20the%20file)%2C%20or%20anchor%20the%20ERROR_PREFIX%20guard%20on%20the%20new%20Error(%20string%20literal%20instead%20of%20a%20whole-file%20substring&repo=modelcontextprotocol/inspector)

2. Nit — the not-rolled-back clause still claims possible damage on the case where nothing was written

scripts/verify-build-gate.mjs:204-209. Round 11 fixed the misdirection to a /tmp backup; what remains is milder but the same shape. On a read-only checkout (EACCES/EROFS — the most likely trigger) both writes fail at open, so rolledBack is false and the message appends:

— the entry may be truncated or partially written; restore it from version control

The entry is byte-identical to what the developer had, and "restore it from version control" discards uncommitted edits to it. The hedge ("may be") makes it honest rather than false, so this is genuinely a nit — but the script already has a definitive signal available for two tokens: read the file back and compare with original, and only append the clause when it actually differs. That replaces the guess with a fact and drops the branch entirely on the common path.

Fix this →

3. Other nits

• saveBackupAndFail closes over original, a let declared after it (:90 vs :108). Safe purely by call ordering — every call site is downstream of the assignment. If a future guard ever calls it earlier it's a TDZ ReferenceError inside the recovery path. Not worth restructuring; worth knowing.
• restored = rolledBack = true (:200) — chained assignment across two variables with different meanings reads a little dense for a recovery path, but it's correct and the comment above carries the reasoning.
• scripts/ still isn't format-gated — tracked in follow-up chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) #1778, correctly out of scope here.

What I checked and found fine

• Round 11's item verified in the code and by control flow: the inline rollback at :197-203 no longer routes through restoreEntry() (whose saveBackupAndFail → fail → exit would swallow the primary error), and the fail() at :204 always reports the write error itself.
• The KNOWN_PHRASE guard is genuinely sound — I re-ran it against the live source: the assignment-anchored regex matches, and the phrase occurs exactly once in the file, so no comment copy can mask a changed constant. That's precisely the property finding Progress notifications #1 says ERROR_PREFIX has lost.
• node --check scripts/verify-build-gate.mjs passes.
• The gate module is complete: 102 lines, four exports, zero Vite import, the message is string predicate keeping recordLog a guard-free one-liner with Set<string> typed. Eight tests cover both branches of ?? false and size > 0 plus dedupe / multi-offender / reset() / per-instance; clients/web/server/** is in the coverage include and unexcluded — ≥90 on all four dimensions with no v8 ignore. The test's third copy of the phrase self-checks against the exported constant (:31), so it can't drift silently.
• The plugin's five hooks each carry their justification in-comment (vite.config.ts:31-83): apply: 'build', applyToEnvironment: (env) => env.name === 'client', enforce: 'pre' (with the widened round-10 note about core plugins and the emitting rolldown:vite-resolve), buildStart reset, buildEnd(error)-guarded assert. CI green across a8a368a7 → ec8117f7 is what establishes the env filter and the reset didn't quietly switch the gate off.
• onLog returns nothing, so Vite's original warning still prints alongside the gate's error — the developer sees the offending module path and the remediation, including the transitive-dependency resolve.alias advice.
• No --logLevel anywhere in the build scripts, so nothing starves onLog before the gate sees the warning.
• dist ordering is safe: the expected path throws in buildEnd, before output generation and emptyOutDir, so the real bundle from validate survives into smoke; the three status === 0 paths that did complete a build flag the probe bundle once (:272-274).
• Docs are consistent across all five places — AGENTS.md structure block, the gate bullet (both apply: 'build' and applyToEnvironment documented), both pipeline enumerations; clients/web/README.md:22 with the "why it lives under server/" justification; the root README.md scripts/ line and testing table. package.json's ci chain matches the workflow's step order, and the workflow step carries its own explanatory comment.
  · branch v2/fix-1769-build-gate-externalized-builtin

[cliffhall]

Round-12 addressed in acf10c74 — took finding #1 and nit #2.

1. Pin comment defeating its own drift guard — fixed. Good catch, and my regression: the round-11 comment quoted "Build failed (#1769)" verbatim, so it appeared twice and the whole-file includes(ERROR_PREFIX) guard passed on the comment alone. Reworded the comment to refer to the lead without quoting it in full (the way the BROWSER_EXTERNALIZED_BUILTIN_PHRASE pin avoids repeating its phrase) — confirmed the literal now occurs exactly once in the file, so the guard is meaningful again.

2. Recovery message inferring damage — made factual. Instead of guessing from whether the rollback write succeeded, it now reads the entry back and only appends "restore from version control" when the contents actually differ from the original (read failing → assume the worst). On a read-only checkout both writes fail at open, so the file is untouched and no longer gets a spurious damage warning. Also let me drop the "may be" hedge, since it's now a fact.

Nits #3 (TDZ closure over original, chained restored = rolledBack = true) — left as-is: safe by call ordering, and the reasoning is in-comment. #1778 still tracks the scripts/ format gap.

npm run ci is green locally.