From 49e72c03e143ae0668f8a622be1641ecf9ff3893 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 15:02:53 -0400 Subject: [PATCH 01/13] build(web): fail vite build on a browser-externalized Node built-in (#1769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vite 8 (rolldown) externalizes a node:* / Node built-in that reaches the browser graph with a warning and ships a `module.exports = {}` stub — the build still succeeds, shipping a broken bundle. The runtime browser smoke (smoke:web:browser, #1615) only catches the subset where that stub is *called* at module init; a stub imported but never called ships silently and nothing gates it. Add a Vite build plugin that turns that specific browser-externalization warning into a hard `vite build` error, moving detection upstream into `npm run build` / `validate` and additionally catching the never-called case the runtime smoke can't see. - clients/web/server/browser-externalized-builtin-gate.ts: Vite-agnostic detection + error + per-build state machine (unit-tested to the ≥90 gate). - vite.config.ts: thin Vite wiring. A throw inside rolldown's onLog is swallowed (the one hook where it doesn't abort — verified against vite@8.0.0), so the gate records the warning in onLog and re-throws in buildEnd. Scoped to `vite build` (apply: 'build'); the Node runner build (tsup) is unaffected. - scripts/verify-build-gate.mjs + verify:build-gate (in npm run ci and the GitHub workflow): runs a real build with a node:fs probe and asserts it fails via the gate — the only check that catches the warning phrasing drifting in a future Vite bump and silently disabling the message-keyed gate. There is no stable log `code` on the rolldown warning (only message + plugin), so the gate keys off the documented message phrasing; the drift guard above is what keeps that safe across Vite versions. smoke:web:browser stays as the runtime backstop. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .github/workflows/main.yml | 9 ++ AGENTS.md | 1 + README.md | 3 +- .../browser-externalized-builtin-gate.ts | 78 ++++++++++++++++ .../browser-externalized-builtin-gate.test.ts | 83 +++++++++++++++++ clients/web/vite.config.ts | 40 ++++++++- package.json | 3 +- scripts/verify-build-gate.mjs | 88 +++++++++++++++++++ 8 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 clients/web/server/browser-externalized-builtin-gate.ts create mode 100644 clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts create mode 100644 scripts/verify-build-gate.mjs diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c136918cf..9259214a0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -53,6 +53,15 @@ jobs: # running the integration suite twice. run: npm run coverage + - name: Verify the browser-externalized-builtin build gate (#1769) + # Runs a real `vite build` with a Node built-in forced into the browser + # graph and asserts the build FAILS via the #1769 gate. The unit tests + # cover the detection logic against a captured message; only this real + # build catches the risk the issue calls out — that the Vite warning + # phrasing drifts across releases, silently disabling the message-keyed + # gate. It restores the mutated entry afterward (see the script). + run: npm run verify:build-gate + # Playwright chromium is installed BEFORE the smokes because # `smoke:web:browser` (the headless-browser boot smoke, #1615) drives the # prod web bundle in chromium — restoring/installing it here lets that diff --git a/AGENTS.md b/AGENTS.md index ed03359f1..690115e44 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -220,6 +220,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with `smoke:web:browser`, so the two can't drift. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a *class* (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first *call* into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A *synchronous* such throw fires `pageerror`; its *async* twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never *called* ships a harmless `{}` and is invisible here by design. Every *other* console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). +- **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's *browser-externalization warning* (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the *called-at-init* case (which `smoke:web:browser` also catches, but later/at runtime) **and** the *imported-but-never-called* case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin *records* the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — and the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. - `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. diff --git a/README.md b/README.md index 709637e5f..b0076bb0a 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,8 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run validate` | Runs `validate:core` (the shared `core/` `format:check` + `lint` gate) first, then per client: `format:check` + `lint` + **`typecheck`** (cli/tui only) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus a headless-Chromium boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests). | -| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `smoke` → Storybook. A true superset of GitHub CI. | +| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | +| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package. Run `npm run format` before committing — the root `format` fixes `core/` and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts new file mode 100644 index 000000000..24566523e --- /dev/null +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -0,0 +1,78 @@ +/** + * Build-gate logic for #1769: fail a browser `vite build` when a Node built-in + * reaches the browser graph. + * + * Vite 8 (rolldown) *externalizes* a `node:*` / bare Node built-in that leaks + * into the browser bundle — it emits a warning and ships a `module.exports = {}` + * stub, and the build still succeeds. That green build ships a broken bundle + * (#1615). The runtime browser smoke (`smoke:web:browser`) only catches the + * subset where the stub is *called* at module init (CASE 1); a stub that's + * imported but never called ships silently (CASE 2) and nothing gates it. + * + * Promoting that specific warning to a hard build error moves CASE 1 detection + * upstream into `npm run build` / `validate` and additionally catches CASE 2. + * + * This module holds the Vite-agnostic pieces so they can be unit-tested without + * standing up a build; the thin Vite `Plugin` that wires them into `onLog` / + * `buildEnd` lives in `vite.config.ts`. The reason the throw can't happen in + * `onLog` directly (rolldown swallows it there) is documented at that call site. + */ + +// The phrase Vite 8 (rolldown) emits when a Node built-in was externalized for +// the browser. There is no stable rollup `code` on this rolldown log — verified +// against the pinned vite@8.0.0, the log carries only `message` + +// `plugin: "rolldown:vite-resolve"` — so the gate keys off this documented, +// user-facing phrasing, whose troubleshooting anchor +// (`module-externalized-for-browser-compatibility`) Vite treats as stable. +// The `verify:build-gate` script exercises a real build so a phrasing drift in a +// future Vite bump fails CI here rather than silently disabling the gate. +export const BROWSER_EXTERNALIZED_BUILTIN_PHRASE = + 'has been externalized for browser compatibility'; + +/** True for a build log announcing a browser-externalized Node built-in. */ +export function isBrowserExternalizedBuiltinLog( + message: string | undefined, +): boolean { + return message?.includes(BROWSER_EXTERNALIZED_BUILTIN_PHRASE) ?? false; +} + +/** The actionable error a browser-externalized Node built-in fails the build with. */ +export function browserExternalizedBuiltinError(message: string): Error { + return new Error( + 'Build failed (#1769): a Node built-in reached the browser bundle and was ' + + 'externalized to an empty stub, which ships a broken bundle. Remove the ' + + 'node:* / Node built-in import from the browser graph (or gate it behind ' + + 'the Node-only dev backend).\n\nOriginal Vite warning: ' + + message, + ); +} + +/** Records browser-externalization build logs and fails the build if any were seen. */ +export interface BrowserExternalizedBuiltinGate { + /** Feed each build log's message here; the first matching one is retained. */ + recordLog(message: string | undefined): void; + /** Throw {@link browserExternalizedBuiltinError} if a match was recorded. */ + assertClean(): void; +} + +/** + * A single-build detector: `recordLog` is called for every build log (from the + * plugin's `onLog`), `assertClean` is called once resolution is complete (from + * the plugin's `buildEnd`) and throws if a Node built-in was externalized. State + * is per-instance so each build gets a fresh gate. + */ +export function createBrowserExternalizedBuiltinGate(): BrowserExternalizedBuiltinGate { + let externalizedWarning: string | undefined; + return { + recordLog(message) { + if (externalizedWarning === undefined && isBrowserExternalizedBuiltinLog(message)) { + externalizedWarning = message; + } + }, + assertClean() { + if (externalizedWarning !== undefined) { + throw browserExternalizedBuiltinError(externalizedWarning); + } + }, + }; +} diff --git a/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts new file mode 100644 index 000000000..f649dde98 --- /dev/null +++ b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts @@ -0,0 +1,83 @@ +import { describe, it, expect } from "vitest"; +import { + BROWSER_EXTERNALIZED_BUILTIN_PHRASE, + isBrowserExternalizedBuiltinLog, + browserExternalizedBuiltinError, + createBrowserExternalizedBuiltinGate, +} from "../../../../server/browser-externalized-builtin-gate.js"; + +// A real message captured from vite@8.0.0's build log (see the gate module). +const REAL_MESSAGE = + 'Module "node:fs" has been externalized for browser compatibility, ' + + 'imported by "/app/src/main.tsx". See https://vite.dev/guide/troubleshooting.html' + + "#module-externalized-for-browser-compatibility for more details."; + +describe("isBrowserExternalizedBuiltinLog", () => { + it("matches the real browser-externalization build message", () => { + expect(isBrowserExternalizedBuiltinLog(REAL_MESSAGE)).toBe(true); + // Sanity: the captured message actually contains the phrase we key off. + expect(REAL_MESSAGE).toContain(BROWSER_EXTERNALIZED_BUILTIN_PHRASE); + }); + + it("does not match an unrelated build warning", () => { + expect( + isBrowserExternalizedBuiltinLog( + "Some chunks are larger than 500 kB after minification.", + ), + ).toBe(false); + }); + + it("does not match an absent message", () => { + expect(isBrowserExternalizedBuiltinLog(undefined)).toBe(false); + }); +}); + +describe("browserExternalizedBuiltinError", () => { + it("builds an actionable #1769 error embedding the original warning", () => { + const err = browserExternalizedBuiltinError(REAL_MESSAGE); + expect(err).toBeInstanceOf(Error); + expect(err.message).toContain("#1769"); + expect(err.message).toContain("externalized to an empty stub"); + // The original Vite warning is preserved so the offending module is visible. + expect(err.message).toContain(REAL_MESSAGE); + }); +}); + +describe("createBrowserExternalizedBuiltinGate", () => { + it("does not throw when no externalization was recorded", () => { + const gate = createBrowserExternalizedBuiltinGate(); + expect(() => gate.assertClean()).not.toThrow(); + }); + + it("throws #1769 in assertClean when a matching log was recorded", () => { + const gate = createBrowserExternalizedBuiltinGate(); + gate.recordLog(REAL_MESSAGE); + expect(() => gate.assertClean()).toThrow(/#1769/); + // The recorded message is surfaced in the failure. + expect(() => gate.assertClean()).toThrow(REAL_MESSAGE); + }); + + it("ignores non-matching and absent logs", () => { + const gate = createBrowserExternalizedBuiltinGate(); + gate.recordLog("unrelated warning"); + gate.recordLog(undefined); + expect(() => gate.assertClean()).not.toThrow(); + }); + + it("retains the first matching message when several are recorded", () => { + const gate = createBrowserExternalizedBuiltinGate(); + const first = REAL_MESSAGE.replace("node:fs", "node:path"); + gate.recordLog(first); + gate.recordLog(REAL_MESSAGE); + // The first offender is the one reported (the `=== undefined` guard). + expect(() => gate.assertClean()).toThrow(first); + }); + + it("keeps state per instance", () => { + const dirty = createBrowserExternalizedBuiltinGate(); + dirty.recordLog(REAL_MESSAGE); + const clean = createBrowserExternalizedBuiltinGate(); + expect(() => clean.assertClean()).not.toThrow(); + expect(() => dirty.assertClean()).toThrow(/#1769/); + }); +}); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 126031d34..a0caa8256 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -1,5 +1,5 @@ /// -import { defineConfig } from 'vite'; +import { defineConfig, type Plugin } from 'vite'; import react from '@vitejs/plugin-react'; // https://vite.dev/config/ @@ -10,6 +10,7 @@ import { playwright } from '@vitest/browser-playwright'; import { honoMiddlewarePlugin } from './server/vite-hono-plugin'; import { getViteBaseConfig, getViteDevOptimizeDeps } from './server/vite-base-config'; import { buildWebServerConfigFromEnv } from './server/web-server-config'; +import { createBrowserExternalizedBuiltinGate } from './server/browser-externalized-builtin-gate'; import { vitestSharedPaths } from '../../vitest.shared.mts'; const dirname = typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url)); const { repoRoot, sharedDedupe, nodeModulesAliases, projectResolve, sharedAliases } = @@ -27,6 +28,35 @@ const { repoRoot, sharedDedupe, nodeModulesAliases, projectResolve, sharedAliase // include AND fail to be excluded from unit, silently landing under happy-dom. const integrationGlob = 'clients/web/src/test/integration/**/*.test.{ts,tsx}'; +// Fail `vite build` when a Node built-in lands in the browser bundle (#1769). +// Vite 8 otherwise only *warns* and ships a `module.exports = {}` stub, so a +// broken bundle builds green; the runtime browser smoke (`smoke:web:browser`, +// #1615) only catches the subset where that stub is *called* at module init. +// The detection + error logic lives in `browser-externalized-builtin-gate.ts` +// (Vite-agnostic, unit-tested); this is only the Vite wiring. +// +// Throwing directly in `onLog` does NOT abort a rolldown build (it's the one +// hook where a thrown error is swallowed — verified against vite@8.0.0), so the +// gate *records* the warning in `onLog` and re-throws in `buildEnd`, which runs +// after module resolution (by when the warning has fired) and where a throw +// aborts the build with a non-zero exit. `apply: 'build'` scopes it to +// `vite build` only — never `vite dev` or the vitest projects — and the Node +// runner build (tsup, `build:runner`) uses a separate config where built-ins +// are legitimate. +function browserExternalizedBuiltinGate(): Plugin { + const gate = createBrowserExternalizedBuiltinGate(); + return { + name: 'inspector:fail-on-browser-externalized-builtin', + apply: 'build', + onLog(_level, log) { + gate.recordLog(log.message); + }, + buildEnd() { + gate.assertClean(); + }, + }; +} + // More info at: https://storybook.js.org/docs/next/writing-tests/integrations/vitest-addon export default defineConfig(({ command }) => { const isDevServer = command === "serve" && !process.env.VITEST; @@ -46,7 +76,13 @@ export default defineConfig(({ command }) => { // resolve it from core/, reviving the benign `UNRESOLVED_IMPORT` warnings // that #1491 eliminated at the source (by removing the old stream filter and // build-time onwarn suppressions rather than re-hiding the symptom). - plugins: [react(), honoMiddlewarePlugin(buildWebServerConfigFromEnv())], + // `browserExternalizedBuiltinGate` fails `vite build` if a Node built-in + // reaches the browser bundle (#1769) — see its definition above. + plugins: [ + react(), + honoMiddlewarePlugin(buildWebServerConfigFromEnv()), + browserExternalizedBuiltinGate(), + ], // Shared optimizeDeps exclusions so node-only packages // (`@modelcontextprotocol/client/stdio`, `cross-spawn`, `which`) // consumed by the dev backend aren't scanned for browser pre-bundling. diff --git a/package.json b/package.json index 8707fd2f1..126be1a6f 100644 --- a/package.json +++ b/package.json @@ -37,8 +37,9 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "ci": "npm run validate && npm run coverage && npm run smoke && npm run ci:storybook", + "ci": "npm run validate && npm run coverage && npm run verify:build-gate && npm run smoke && npm run ci:storybook", "ci:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", + "verify:build-gate": "node scripts/verify-build-gate.mjs", "validate": "npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", "validate:core": "npm run format:check:core && npm run lint:core", "lint:core": "eslint \"core/**/*.{ts,tsx}\"", diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs new file mode 100644 index 000000000..959878921 --- /dev/null +++ b/scripts/verify-build-gate.mjs @@ -0,0 +1,88 @@ +#!/usr/bin/env node +/** + * Verifies the browser-externalized-builtin build gate (#1769). + * + * The gate (clients/web/vite.config.ts + + * clients/web/server/browser-externalized-builtin-gate.ts) fails `vite build` + * when a Node built-in reaches the browser graph — Vite 8 otherwise only warns + * and ships a `{}` stub, so the broken bundle builds green. The unit tests cover + * the detection logic against a *captured* message string; only a real build can + * prove the gate still fires against the *live* Vite version, catching the one + * risk the issue calls out: the warning phrasing drifts across Vite releases + * (8.0.x → 8.1.x), silently disabling a message-keyed gate. + * + * This temporarily injects a `node:fs` import into the browser entry + * (src/main.tsx), runs `vite build`, and asserts the build FAILS with the #1769 + * error, then restores the entry. Run from `npm run ci`. + */ + +import { spawnSync } from "node:child_process"; +import { readFileSync, writeFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const webDir = path.join(repoRoot, "clients/web"); +const entryPath = path.join(webDir, "src/main.tsx"); +const viteCache = path.join(webDir, "node_modules/.vite"); + +// A namespace import + guarded use so the built-in isn't tree-shaken before Vite +// externalizes it (a bare side-effect import can be dropped). `__never__` is +// never truthy, so the reference survives to build time without running. +const PROBE = + 'import * as __nodeBuiltinProbe from "node:fs";\n' + + "if (globalThis.__never__) console.log(__nodeBuiltinProbe);\n"; + +function fail(message, detail) { + console.error(`verify:build-gate FAILED — ${message}`); + if (detail) console.error(detail); + process.exit(1); +} + +const original = readFileSync(entryPath, "utf8"); + +let result; +try { + writeFileSync(entryPath, PROBE + original); + // Clear the transform cache so the externalization warning is re-emitted + // rather than served from a prior build's cache. + spawnSync("rm", ["-rf", viteCache], { cwd: webDir }); + result = spawnSync("npx", ["vite", "build"], { + cwd: webDir, + encoding: "utf8", + }); +} finally { + // Always restore the entry, even if the build spawn threw. + writeFileSync(entryPath, original); +} + +// Guard against a botched restore leaving the tree dirty. +if (readFileSync(entryPath, "utf8") !== original) { + fail( + `failed to restore ${entryPath} — run 'git checkout -- ${path.relative(repoRoot, entryPath)}'`, + ); +} + +const output = `${result.stdout ?? ""}\n${result.stderr ?? ""}`; + +if (result.status === 0) { + fail( + "vite build SUCCEEDED with a node:fs import in the browser graph — 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).", + output, + ); +} + +if (!output.includes("#1769")) { + fail( + "vite build failed, but not via the #1769 gate — the build broke for another " + + "reason, so this check no longer proves the gate works.", + output, + ); +} + +console.log( + "verify:build-gate OK — vite build fails on a Node built-in in the browser graph (#1769 gate fired).", +); +process.exit(0); From 011efbabe2a46e05334e15b868c974ecac5cb24f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 15:27:15 -0400 Subject: [PATCH 02/13] build(web): harden the browser-externalized-builtin gate per review (#1769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on PR #1777: - Per-build state: add gate.reset() called from the plugin's buildStart, so a `vite build --watch` rebuild doesn't inherit a prior build's recorded warning (the plugin instance is reused across rebuilds). Makes the docblock true. - buildEnd(error) now only asserts on the success path — rolldown also calls buildEnd(error) when the build already failed for another reason, and throwing then would mask that real error with the #1769 message. - verify-build-gate.mjs: drop the shell `rm -rf` cache-clear entirely (verified the gate fires reliably without it — the earlier "caching" symptom was the onLog-swallow behavior, not a stale cache); add a result.error spawn-failure check so a missing `npx` isn't misdiagnosed as "not via the gate"; add SIGINT/SIGTERM handlers so an interrupt during the multi-minute build still restores the mutated entry; add a progress log before the spawn. - Match the double-quote style of the sibling server/ files. - Docs: list browser-externalized-builtin-gate.ts in the AGENTS.md server/ block and the clients/web/README server/ list (with why a build helper lives under server/), and add verify-build-gate to the root README scripts/ line. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 4 ++- README.md | 2 +- clients/web/README.md | 1 + .../browser-externalized-builtin-gate.ts | 27 ++++++++++----- .../browser-externalized-builtin-gate.test.ts | 11 ++++++ clients/web/vite.config.ts | 13 +++++-- scripts/verify-build-gate.mjs | 34 +++++++++++++++---- 7 files changed, 73 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 690115e44..ff0e9aa8c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,9 @@ inspector/ │ │ │ # web-server-config.ts (env parsing + initial-config payload + banner), │ │ │ # sandbox-controller.ts (MCP Apps sandbox HTTP server), │ │ │ # inject-auth-token.ts (embeds the API token into served index.html), -│ │ │ # vite-base-config.ts (shared optimizeDeps exclusions) +│ │ │ # vite-base-config.ts (shared optimizeDeps exclusions), +│ │ │ # browser-externalized-builtin-gate.ts (build-gate logic that fails +│ │ │ # `vite build` on a browser-externalized Node built-in — #1769) │ │ └── static/ # sandbox_proxy.html (served by sandbox-controller for MCP Apps tab) │ ├── cli/ # CLI client (tsup bundle, @inspector/core alias) │ ├── tui/ # TUI client (Ink + React, tsup bundle) diff --git a/README.md b/README.md index b0076bb0a..63fc5cf81 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ inspector/ │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests -├── scripts/ # Root build/verify tooling (install cascade, smokes, pack:verify) +├── scripts/ # Root build/verify tooling (install cascade, smokes, verify-build-gate, pack:verify) ├── specification/ # Design/build specifications ├── AGENTS.md # Contribution rules for agents AND humans (see below) └── README.md # You are here diff --git a/clients/web/README.md b/clients/web/README.md index a836f4329..29c2f1f58 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -19,6 +19,7 @@ The `server/` directory holds the Node-only backend: - **`web-server-config.ts`** — env parsing, the `GET /api/config` payload, the startup banner. - **`inject-auth-token.ts`** — embeds the API token into the served `index.html` (see [Auth token](#auth-token)). - **`sandbox-controller.ts`** — the MCP Apps sandbox HTTP server; **`ensure-web-build.ts`** — builds `dist/` on demand for prod `--web`; **`vite-base-config.ts`** — shared `optimizeDeps` exclusions. +- **`browser-externalized-builtin-gate.ts`** — Vite-agnostic build-gate logic that fails `vite build` when a Node built-in reaches the browser bundle (#1769); the thin Vite plugin wiring lives in `vite.config.ts`. It sits under `server/` (rather than `src/`) as the home for Node-only, build-time tooling — it's imported by the Vite config, never by the browser — alongside the other `vite-*` config helpers here. ## Development diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index 24566523e..a7f0cde2a 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -27,7 +27,7 @@ // The `verify:build-gate` script exercises a real build so a phrasing drift in a // future Vite bump fails CI here rather than silently disabling the gate. export const BROWSER_EXTERNALIZED_BUILTIN_PHRASE = - 'has been externalized for browser compatibility'; + "has been externalized for browser compatibility"; /** True for a build log announcing a browser-externalized Node built-in. */ export function isBrowserExternalizedBuiltinLog( @@ -39,10 +39,10 @@ export function isBrowserExternalizedBuiltinLog( /** The actionable error a browser-externalized Node built-in fails the build with. */ export function browserExternalizedBuiltinError(message: string): Error { return new Error( - 'Build failed (#1769): a Node built-in reached the browser bundle and was ' + - 'externalized to an empty stub, which ships a broken bundle. Remove the ' + - 'node:* / Node built-in import from the browser graph (or gate it behind ' + - 'the Node-only dev backend).\n\nOriginal Vite warning: ' + + "Build failed (#1769): a Node built-in reached the browser bundle and was " + + "externalized to an empty stub, which ships a broken bundle. Remove the " + + "node:* / Node built-in import from the browser graph (or gate it behind " + + "the Node-only dev backend).\n\nOriginal Vite warning: " + message, ); } @@ -53,19 +53,25 @@ export interface BrowserExternalizedBuiltinGate { recordLog(message: string | undefined): void; /** Throw {@link browserExternalizedBuiltinError} if a match was recorded. */ assertClean(): void; + /** Clear recorded state so a rebuild (e.g. `vite build --watch`) starts fresh. */ + reset(): void; } /** - * A single-build detector: `recordLog` is called for every build log (from the + * A per-build detector: `recordLog` is called for every build log (from the * plugin's `onLog`), `assertClean` is called once resolution is complete (from - * the plugin's `buildEnd`) and throws if a Node built-in was externalized. State - * is per-instance so each build gets a fresh gate. + * the plugin's `buildEnd`) and throws if a Node built-in was externalized. The + * plugin `reset`s it in `buildStart` so a watch-mode rebuild doesn't inherit a + * previous build's recorded warning. */ export function createBrowserExternalizedBuiltinGate(): BrowserExternalizedBuiltinGate { let externalizedWarning: string | undefined; return { recordLog(message) { - if (externalizedWarning === undefined && isBrowserExternalizedBuiltinLog(message)) { + if ( + externalizedWarning === undefined && + isBrowserExternalizedBuiltinLog(message) + ) { externalizedWarning = message; } }, @@ -74,5 +80,8 @@ export function createBrowserExternalizedBuiltinGate(): BrowserExternalizedBuilt throw browserExternalizedBuiltinError(externalizedWarning); } }, + reset() { + externalizedWarning = undefined; + }, }; } diff --git a/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts index f649dde98..cbf00c184 100644 --- a/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts +++ b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts @@ -73,6 +73,17 @@ describe("createBrowserExternalizedBuiltinGate", () => { expect(() => gate.assertClean()).toThrow(first); }); + it("reset() clears a recorded warning so a rebuild starts clean", () => { + const gate = createBrowserExternalizedBuiltinGate(); + gate.recordLog(REAL_MESSAGE); + gate.reset(); + // Post-reset the gate is clean... + expect(() => gate.assertClean()).not.toThrow(); + // ...and can record + fail again on the next build. + gate.recordLog(REAL_MESSAGE); + expect(() => gate.assertClean()).toThrow(/#1769/); + }); + it("keeps state per instance", () => { const dirty = createBrowserExternalizedBuiltinGate(); dirty.recordLog(REAL_MESSAGE); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index a0caa8256..8da1acf17 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -43,16 +43,25 @@ const integrationGlob = 'clients/web/src/test/integration/**/*.test.{ts,tsx}'; // `vite build` only — never `vite dev` or the vitest projects — and the Node // runner build (tsup, `build:runner`) uses a separate config where built-ins // are legitimate. +// +// `buildStart` resets the gate so a `vite build --watch` rebuild doesn't inherit +// a prior build's recorded warning (the plugin instance is reused across +// rebuilds). `buildEnd` only asserts on the *success* path: rolldown also calls +// `buildEnd(error)` when the build already failed for another reason, and +// throwing then would mask that real error with the #1769 message. function browserExternalizedBuiltinGate(): Plugin { const gate = createBrowserExternalizedBuiltinGate(); return { name: 'inspector:fail-on-browser-externalized-builtin', apply: 'build', + buildStart() { + gate.reset(); + }, onLog(_level, log) { gate.recordLog(log.message); }, - buildEnd() { - gate.assertClean(); + buildEnd(error) { + if (!error) gate.assertClean(); }, }; } diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 959878921..473d03f02 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -24,7 +24,6 @@ import { fileURLToPath } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const webDir = path.join(repoRoot, "clients/web"); const entryPath = path.join(webDir, "src/main.tsx"); -const viteCache = path.join(webDir, "node_modules/.vite"); // A namespace import + guarded use so the built-in isn't tree-shaken before Vite // externalizes it (a bare side-effect import can be dropped). `__never__` is @@ -33,27 +32,43 @@ const PROBE = 'import * as __nodeBuiltinProbe from "node:fs";\n' + "if (globalThis.__never__) console.log(__nodeBuiltinProbe);\n"; +const original = readFileSync(entryPath, "utf8"); +let restored = false; + +function restoreEntry() { + if (restored) return; + writeFileSync(entryPath, original); + restored = true; +} + function fail(message, detail) { console.error(`verify:build-gate FAILED — ${message}`); if (detail) console.error(detail); process.exit(1); } -const original = readFileSync(entryPath, "utf8"); +// A `finally` doesn't run on Ctrl-C during the multi-minute build; restore the +// mutated entry on a signal too so an interrupt never leaves the tree dirty. +for (const signal of ["SIGINT", "SIGTERM"]) { + process.on(signal, () => { + restoreEntry(); + process.exit(130); + }); +} let result; try { writeFileSync(entryPath, PROBE + original); - // Clear the transform cache so the externalization warning is re-emitted - // rather than served from a prior build's cache. - spawnSync("rm", ["-rf", viteCache], { cwd: webDir }); + console.log( + "verify:build-gate: running a real `vite build` with a node:fs probe (takes a minute)…", + ); result = spawnSync("npx", ["vite", "build"], { cwd: webDir, encoding: "utf8", }); } finally { // Always restore the entry, even if the build spawn threw. - writeFileSync(entryPath, original); + restoreEntry(); } // Guard against a botched restore leaving the tree dirty. @@ -63,6 +78,13 @@ if (readFileSync(entryPath, "utf8") !== original) { ); } +// A spawn failure (e.g. `npx` missing) leaves `status` null with no output — +// surface it as itself rather than falling through to the "not via the gate" +// diagnosis, which would send someone chasing a build regression that isn't real. +if (result.error) { + fail(`could not run \`vite build\` (${result.error.message})`); +} + const output = `${result.stdout ?? ""}\n${result.stderr ?? ""}`; if (result.status === 0) { From a8a368a76f1a088adeed744952ca1ab06b5c852d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 15:49:06 -0400 Subject: [PATCH 03/13] build(web): round-2 review hardening for the #1769 build gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Scope the gate to the browser (`client`) environment via applyToEnvironment, so a future SSR/node environment built from this config isn't failed for a legitimate node:* import — browser-only intent is now structural. - Report ALL externalized built-ins in one build, deduped, instead of only the first offender (N leaks no longer means N fix-and-rebuild cycles). browserExternalizedBuiltinError now takes string[]; gate uses a Set. - verify-build-gate.mjs: pin Vite with `npx --no-install` so the drift check can never silently run a registry-fetched Vite instead of the repo-pinned one (the whole point is version fidelity); append the probe instead of prepending (ES imports hoist, and appending won't demote a future leading directive); clarify the signal-handler comment (handlers are a backstop — a blocked spawnSync tears down via the process group first). - AGENTS.md: update both `ci` pipeline enumerations to include verify:build-gate and drop the now-false "Storybook is the only CI step left out" claim. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 4 +- .../browser-externalized-builtin-gate.ts | 45 ++++++++++--------- .../browser-externalized-builtin-gate.test.ts | 42 +++++++++++++---- clients/web/vite.config.ts | 14 +++--- scripts/verify-build-gate.mjs | 18 ++++++-- 5 files changed, 84 insertions(+), 39 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ff0e9aa8c..425a6d460 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -212,13 +212,13 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i ### Mandatory pre-push gate - ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`) and every client's scope in one shot. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. -- **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). +- **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). - ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs the **`core/` gate first** (`validate:core`) and then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the shared-code format + lint gate (#1689).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/` before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`). Use `npm run format:core` to auto-fix. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. -- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only CI step left out (see below). +- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only step in `npm run ci` that GitHub CI runs as its own tail step (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with `smoke:web:browser`, so the two can't drift. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a *class* (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first *call* into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A *synchronous* such throw fires `pageerror`; its *async* twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never *called* ships a harmless `{}` and is invisible here by design. Every *other* console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index a7f0cde2a..92c7c58bd 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -14,8 +14,9 @@ * * This module holds the Vite-agnostic pieces so they can be unit-tested without * standing up a build; the thin Vite `Plugin` that wires them into `onLog` / - * `buildEnd` lives in `vite.config.ts`. The reason the throw can't happen in - * `onLog` directly (rolldown swallows it there) is documented at that call site. + * `buildEnd` lives in `vite.config.ts`, scoped there to the browser (`client`) + * environment. The reason the throw can't happen in `onLog` directly (rolldown + * swallows it there) is documented at that call site. */ // The phrase Vite 8 (rolldown) emits when a Node built-in was externalized for @@ -36,22 +37,27 @@ export function isBrowserExternalizedBuiltinLog( return message?.includes(BROWSER_EXTERNALIZED_BUILTIN_PHRASE) ?? false; } -/** The actionable error a browser-externalized Node built-in fails the build with. */ -export function browserExternalizedBuiltinError(message: string): Error { +/** + * The actionable error a browser-externalized Node built-in fails the build + * with. Takes every matched warning so a build that leaks several built-ins + * reports them all in one pass, rather than one-per-rebuild. + */ +export function browserExternalizedBuiltinError(messages: string[]): Error { + const list = messages.map((m) => ` - ${m}`).join("\n"); return new Error( "Build failed (#1769): a Node built-in reached the browser bundle and was " + "externalized to an empty stub, which ships a broken bundle. Remove the " + - "node:* / Node built-in import from the browser graph (or gate it behind " + - "the Node-only dev backend).\n\nOriginal Vite warning: " + - message, + "node:* / Node built-in import(s) from the browser graph (or gate them " + + "behind the Node-only dev backend).\n\nOriginal Vite warning(s):\n" + + list, ); } /** Records browser-externalization build logs and fails the build if any were seen. */ export interface BrowserExternalizedBuiltinGate { - /** Feed each build log's message here; the first matching one is retained. */ + /** Feed each build log's message here; matching ones are collected (deduped). */ recordLog(message: string | undefined): void; - /** Throw {@link browserExternalizedBuiltinError} if a match was recorded. */ + /** Throw {@link browserExternalizedBuiltinError} if any match was recorded. */ assertClean(): void; /** Clear recorded state so a rebuild (e.g. `vite build --watch`) starts fresh. */ reset(): void; @@ -60,28 +66,25 @@ export interface BrowserExternalizedBuiltinGate { /** * A per-build detector: `recordLog` is called for every build log (from the * plugin's `onLog`), `assertClean` is called once resolution is complete (from - * the plugin's `buildEnd`) and throws if a Node built-in was externalized. The - * plugin `reset`s it in `buildStart` so a watch-mode rebuild doesn't inherit a - * previous build's recorded warning. + * the plugin's `buildEnd`) and throws — listing every offender — if any Node + * built-in was externalized. The plugin `reset`s it in `buildStart` so a + * watch-mode rebuild doesn't inherit a previous build's recorded warnings. */ export function createBrowserExternalizedBuiltinGate(): BrowserExternalizedBuiltinGate { - let externalizedWarning: string | undefined; + const externalized = new Set(); return { recordLog(message) { - if ( - externalizedWarning === undefined && - isBrowserExternalizedBuiltinLog(message) - ) { - externalizedWarning = message; + if (message !== undefined && isBrowserExternalizedBuiltinLog(message)) { + externalized.add(message); } }, assertClean() { - if (externalizedWarning !== undefined) { - throw browserExternalizedBuiltinError(externalizedWarning); + if (externalized.size > 0) { + throw browserExternalizedBuiltinError([...externalized]); } }, reset() { - externalizedWarning = undefined; + externalized.clear(); }, }; } diff --git a/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts index cbf00c184..51fbf1307 100644 --- a/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts +++ b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts @@ -6,6 +6,18 @@ import { createBrowserExternalizedBuiltinGate, } from "../../../../server/browser-externalized-builtin-gate.js"; +// Run `fn`, expect it to throw an Error, and return the thrown message — +// typed, so assertions on the message don't reach through an `any` catch var. +function messageFromThrow(fn: () => void): string { + try { + fn(); + } catch (err) { + if (err instanceof Error) return err.message; + throw err; + } + throw new Error("expected the call to throw, but it did not"); +} + // A real message captured from vite@8.0.0's build log (see the gate module). const REAL_MESSAGE = 'Module "node:fs" has been externalized for browser compatibility, ' + @@ -33,13 +45,15 @@ describe("isBrowserExternalizedBuiltinLog", () => { }); describe("browserExternalizedBuiltinError", () => { - it("builds an actionable #1769 error embedding the original warning", () => { - const err = browserExternalizedBuiltinError(REAL_MESSAGE); + it("builds an actionable #1769 error embedding every original warning", () => { + const second = REAL_MESSAGE.replace("node:fs", "node:path"); + const err = browserExternalizedBuiltinError([REAL_MESSAGE, second]); expect(err).toBeInstanceOf(Error); expect(err.message).toContain("#1769"); expect(err.message).toContain("externalized to an empty stub"); - // The original Vite warning is preserved so the offending module is visible. + // Every offending warning is listed so all leaks are visible in one pass. expect(err.message).toContain(REAL_MESSAGE); + expect(err.message).toContain(second); }); }); @@ -64,13 +78,25 @@ describe("createBrowserExternalizedBuiltinGate", () => { expect(() => gate.assertClean()).not.toThrow(); }); - it("retains the first matching message when several are recorded", () => { + it("reports every distinct offender when several are recorded", () => { + const gate = createBrowserExternalizedBuiltinGate(); + const other = REAL_MESSAGE.replace("node:fs", "node:path"); + gate.recordLog(other); + gate.recordLog(REAL_MESSAGE); + // Both leaks surface in one failure rather than one-per-rebuild. + const message = messageFromThrow(() => gate.assertClean()); + expect(message).toContain(other); + expect(message).toContain(REAL_MESSAGE); + }); + + it("dedupes a repeated offender message", () => { const gate = createBrowserExternalizedBuiltinGate(); - const first = REAL_MESSAGE.replace("node:fs", "node:path"); - gate.recordLog(first); gate.recordLog(REAL_MESSAGE); - // The first offender is the one reported (the `=== undefined` guard). - expect(() => gate.assertClean()).toThrow(first); + gate.recordLog(REAL_MESSAGE); + // Listed once, not twice. + const message = messageFromThrow(() => gate.assertClean()); + const occurrences = message.split(REAL_MESSAGE).length - 1; + expect(occurrences).toBe(1); }); it("reset() clears a recorded warning so a rebuild starts clean", () => { diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 8da1acf17..b0a34342c 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -39,13 +39,16 @@ const integrationGlob = 'clients/web/src/test/integration/**/*.test.{ts,tsx}'; // hook where a thrown error is swallowed — verified against vite@8.0.0), so the // gate *records* the warning in `onLog` and re-throws in `buildEnd`, which runs // after module resolution (by when the warning has fired) and where a throw -// aborts the build with a non-zero exit. `apply: 'build'` scopes it to -// `vite build` only — never `vite dev` or the vitest projects — and the Node -// runner build (tsup, `build:runner`) uses a separate config where built-ins -// are legitimate. +// aborts the build with a non-zero exit. +// +// `apply: 'build'` scopes it to `vite build` (never `vite dev` or the vitest +// projects), and `applyToEnvironment` narrows it to the **browser** (`client`) +// environment so a future SSR/node environment built from this config isn't +// failed for a legitimate `node:*` import — the browser-only intent is +// structural, not incidental. (The Node runner build is a separate tsup config.) // // `buildStart` resets the gate so a `vite build --watch` rebuild doesn't inherit -// a prior build's recorded warning (the plugin instance is reused across +// a prior build's recorded warnings (the plugin instance is reused across // rebuilds). `buildEnd` only asserts on the *success* path: rolldown also calls // `buildEnd(error)` when the build already failed for another reason, and // throwing then would mask that real error with the #1769 message. @@ -54,6 +57,7 @@ function browserExternalizedBuiltinGate(): Plugin { return { name: 'inspector:fail-on-browser-externalized-builtin', apply: 'build', + applyToEnvironment: (environment) => environment.name === 'client', buildStart() { gate.reset(); }, diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 473d03f02..0627eb2a2 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -28,8 +28,11 @@ const entryPath = path.join(webDir, "src/main.tsx"); // A namespace import + guarded use so the built-in isn't tree-shaken before Vite // externalizes it (a bare side-effect import can be dropped). `__never__` is // never truthy, so the reference survives to build time without running. +// Appended (not prepended): ES imports hoist, so this still externalizes at +// resolve time, and appending won't demote a leading directive (e.g. a future +// `"use client"`) the way prepending would. const PROBE = - 'import * as __nodeBuiltinProbe from "node:fs";\n' + + '\nimport * as __nodeBuiltinProbe from "node:fs";\n' + "if (globalThis.__never__) console.log(__nodeBuiltinProbe);\n"; const original = readFileSync(entryPath, "utf8"); @@ -49,6 +52,10 @@ function fail(message, detail) { // A `finally` doesn't run on Ctrl-C during the multi-minute build; restore the // mutated entry on a signal too so an interrupt never leaves the tree dirty. +// While `spawnSync` blocks, a Ctrl-C reaches `vite` via the shared process +// group (the child dies, `spawnSync` returns, the `finally` restores) and these +// handlers run afterward as a backstop — e.g. for a `kill ` that targets +// only this process, where the queued handler is the sole restore path. for (const signal of ["SIGINT", "SIGTERM"]) { process.on(signal, () => { restoreEntry(); @@ -58,11 +65,16 @@ for (const signal of ["SIGINT", "SIGTERM"]) { let result; try { - writeFileSync(entryPath, PROBE + original); + writeFileSync(entryPath, original + PROBE); console.log( "verify:build-gate: running a real `vite build` with a node:fs probe (takes a minute)…", ); - result = spawnSync("npx", ["vite", "build"], { + // `--no-install` pins to the locally installed (repo-pinned) Vite: the whole + // point is proving the message-keyed gate fires against THIS Vite, so `npx` + // must never silently fetch a different version from the registry when + // clients/web/node_modules is missing/partial. A missing local bin then + // surfaces via the `result.error` check below. + result = spawnSync("npx", ["--no-install", "vite", "build"], { cwd: webDir, encoding: "utf8", }); From f4db356a902c948ac46e934231843fc6e960ebae Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 16:13:56 -0400 Subject: [PATCH 04/13] build(web): round-3 review polish for the #1769 build gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Error message: name the transitive-dependency remedy (a resolve.alias to a browser shim) — "remove the import / gate behind the backend" doesn't apply when the node:* import lives in node_modules. - isBrowserExternalizedBuiltinLog is now a `message is string` type guard, so recordLog drops its redundant `!== undefined` clause while still typing the Set.add() — a real one-liner, not a coverage-only removal. - verify-build-gate.mjs: docblock now states WHY it builds the real config + entry (catches config-level regressions — plugin removed, onwarn suppression added — that a temp config would miss), so the minute-long build isn't later "optimized" into a temp entry and silently lose that coverage. Guard the entry read so a renamed main.tsx fails through fail() with an actionable message. On a botched restore, save the captured original to a sidecar .bak and point there instead of recommending `git checkout --`, which would discard uncommitted edits. - AGENTS.md: fix the muddled CI-steps sentence (GitHub CI runs the whole chain as separate steps, Storybook last) and note the applyToEnvironment client-only scoping in the gate prose. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 4 +- .../browser-externalized-builtin-gate.ts | 13 ++++-- scripts/verify-build-gate.mjs | 45 +++++++++++++++---- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 425a6d460..273657d14 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -218,11 +218,11 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script (`tsc --noEmit -p tsconfig.json`) folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib *resolution* options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. -- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). Storybook is the only step in `npm run ci` that GitHub CI runs as its own tail step (see below). +- **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. - `smoke:web` (`scripts/smoke-web.mjs`) starts `mcp-inspector --web` (prod, no `--dev`) against the built `clients/web/dist` and asserts `GET /` serves the SPA (HTTP 200) with the injected `__INSPECTOR_API_TOKEN__`. Prod `--web` serves from `clients/web/dist`, which ships in the published package but is absent in a fresh checkout — the runner builds it on demand (`build:client` = `vite build`) on first launch, or exits with an actionable error if that build can't run (see `clients/web/server/ensure-web-build.ts` and the launcher README). `--dev` runs Vite directly and never needs `dist`. It shares the spawn/readiness/teardown helper (`scripts/lib/prod-web-server.mjs`) with `smoke:web:browser`, so the two can't drift. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a *class* (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first *call* into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A *synchronous* such throw fires `pageerror`; its *async* twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never *called* ships a harmless `{}` and is invisible here by design. Every *other* console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). -- **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's *browser-externalization warning* (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the *called-at-init* case (which `smoke:web:browser` also catches, but later/at runtime) **and** the *imported-but-never-called* case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin *records* the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — and the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. +- **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's *browser-externalization warning* (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the *called-at-init* case (which `smoke:web:browser` also catches, but later/at runtime) **and** the *imported-but-never-called* case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin *records* the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. - `smoke:cli` (`scripts/smoke-cli.mjs`) drives `mcp-inspector --cli` through the built launcher against the bundled stdio test server via a temp `--catalog`: it asserts `tools/list` returns the server's tools (real connect over stdio), the default writable catalog is seeded empty on first run, a missing read-only `--config` errors without seeding, and `--catalog` + `--config` is rejected. `smoke:tui` (`scripts/smoke-tui.mjs`) launches `mcp-inspector --tui --catalog ` and asserts the Ink app renders its first frame (the "MCP Servers" panel) within a timeout, then SIGTERMs it — a shallow boot/render check, not full interaction. **`smoke:tui` is local-only: it self-skips when `process.env.CI` is set**, because the Ink TUI needs a real TTY (raw mode) that headless CI lacks — so run it (via `npm run smoke`) on your own machine before pushing. Both build `test-servers/build` on demand if it's missing. - Storybook play-function tests (`clients/web` `test:storybook`) run in headless Chromium via `@vitest/browser-playwright` (~10s). They are part of `npm run ci` (which installs Playwright chromium first); kept out of `validate` because they need the browser binary and are slower than the unit suite. diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index 92c7c58bd..d587de54e 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -30,10 +30,12 @@ export const BROWSER_EXTERNALIZED_BUILTIN_PHRASE = "has been externalized for browser compatibility"; -/** True for a build log announcing a browser-externalized Node built-in. */ +// True for a build log announcing a browser-externalized Node built-in. Typed +// as a `message is string` guard: a match implies a defined string, so callers +// can add it to a `Set` without a separate `undefined` check. export function isBrowserExternalizedBuiltinLog( message: string | undefined, -): boolean { +): message is string { return message?.includes(BROWSER_EXTERNALIZED_BUILTIN_PHRASE) ?? false; } @@ -48,7 +50,10 @@ export function browserExternalizedBuiltinError(messages: string[]): Error { "Build failed (#1769): a Node built-in reached the browser bundle and was " + "externalized to an empty stub, which ships a broken bundle. Remove the " + "node:* / Node built-in import(s) from the browser graph (or gate them " + - "behind the Node-only dev backend).\n\nOriginal Vite warning(s):\n" + + "behind the Node-only dev backend). If an import comes from a dependency " + + "(not first-party code — see the module path in the warning below), add a " + + "`resolve.alias` in clients/web/vite.config.ts pointing it at a browser " + + "shim.\n\nOriginal Vite warning(s):\n" + list, ); } @@ -74,7 +79,7 @@ export function createBrowserExternalizedBuiltinGate(): BrowserExternalizedBuilt const externalized = new Set(); return { recordLog(message) { - if (message !== undefined && isBrowserExternalizedBuiltinLog(message)) { + if (isBrowserExternalizedBuiltinLog(message)) { externalized.add(message); } }, diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 0627eb2a2..7246e9218 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -14,6 +14,16 @@ * This temporarily injects a `node:fs` import into the browser entry * (src/main.tsx), runs `vite build`, and asserts the build FAILS with the #1769 * error, then restores the entry. Run from `npm run ci`. + * + * Why the REAL config + entry (and not a fast throwaway temp entry / generated + * config): building the actual `clients/web` config is what catches config-level + * regressions — the plugin being deleted from the `plugins` array, or a + * `build.rollupOptions.onwarn` suppression added 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 the live Vite." That fidelity is the + * point, and it's why this accepts a full (~minute) app build and a + * source-mutation-with-restore rather than something cheaper. Do NOT "optimize" + * it into a temp entry — that silently loses the config-regression coverage. */ import { spawnSync } from "node:child_process"; @@ -23,6 +33,8 @@ import { fileURLToPath } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const webDir = path.join(repoRoot, "clients/web"); +// Hardcoded browser entry. If it's ever renamed, the guarded read below fails +// with an actionable message rather than a raw ENOENT stack. const entryPath = path.join(webDir, "src/main.tsx"); // A namespace import + guarded use so the built-in isn't tree-shaken before Vite @@ -35,7 +47,22 @@ const PROBE = '\nimport * as __nodeBuiltinProbe from "node:fs";\n' + "if (globalThis.__never__) console.log(__nodeBuiltinProbe);\n"; -const original = readFileSync(entryPath, "utf8"); +function fail(message, detail) { + console.error(`verify:build-gate FAILED — ${message}`); + if (detail) console.error(detail); + process.exit(1); +} + +let original; +try { + original = readFileSync(entryPath, "utf8"); +} catch (err) { + fail( + `could not read the browser entry ${path.relative(repoRoot, entryPath)} ` + + `(${err.message}) — if it was renamed, update entryPath in this script`, + ); +} + let restored = false; function restoreEntry() { @@ -44,12 +71,6 @@ function restoreEntry() { restored = true; } -function fail(message, detail) { - console.error(`verify:build-gate FAILED — ${message}`); - if (detail) console.error(detail); - process.exit(1); -} - // A `finally` doesn't run on Ctrl-C during the multi-minute build; restore the // mutated entry on a signal too so an interrupt never leaves the tree dirty. // While `spawnSync` blocks, a Ctrl-C reaches `vite` via the shared process @@ -83,10 +104,16 @@ try { restoreEntry(); } -// Guard against a botched restore leaving the tree dirty. +// Guard against a botched restore leaving the tree dirty. Write the captured +// original to a sidecar `.bak` and point there — NOT `git checkout --`, which +// would also discard any uncommitted edits the developer had in the entry. if (readFileSync(entryPath, "utf8") !== original) { + const backupPath = `${entryPath}.verify-build-gate.bak`; + writeFileSync(backupPath, original); fail( - `failed to restore ${entryPath} — run 'git checkout -- ${path.relative(repoRoot, entryPath)}'`, + `failed to restore ${path.relative(repoRoot, entryPath)} — its pre-run ` + + `contents were saved to ${path.relative(repoRoot, backupPath)}; restore ` + + `from there (it preserves any uncommitted edits, unlike 'git checkout --')`, ); } From d34cdddb89cb76f676ccdfb365d970c509bf92d4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 16:37:03 -0400 Subject: [PATCH 05/13] =?UTF-8?q?build(web):=20round-4=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20sharper=20verify-build-gate=20diagnostics=20(#17?= =?UTF-8?q?69)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All in scripts/verify-build-gate.mjs's failure paths: - Split the "build SUCCEEDED" failure into three diagnoses keyed off the captured output, so it names the right file: warning present but build passed → plugin not applying (removed from `plugins`, `applyToEnvironment` env-name mismatch, or an `onwarn` suppression); node:fs present but phrase absent → phrasing drift, update the constant; neither → probe never reached the graph. The old message hard-coded "phrasing drifted", which misdirects for the config-regression class this script exists to catch. Added applyToEnvironment to the docblock's config-regression list too. - Bound the build with a 10-min spawnSync timeout (+ SIGKILL); a timeout sets result.error (ETIMEDOUT) and reports via the existing branch, instead of burning to the GitHub job's 360-min default with no output. - Guard the probe write so a read-only/EACCES checkout fails through fail() with an actionable message instead of escaping as a raw stack. - SIGTERM now exits 143 (was 130, which is SIGINT's code). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/verify-build-gate.mjs | 61 +++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 7246e9218..9681f86e7 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -17,7 +17,8 @@ * * Why the REAL config + entry (and not a fast throwaway temp entry / generated * config): building the actual `clients/web` config is what catches config-level - * regressions — the plugin being deleted from the `plugins` array, or a + * regressions — the plugin being deleted from the `plugins` array, its + * `applyToEnvironment` no longer matching the browser environment's name, or a * `build.rollupOptions.onwarn` suppression added 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 the live Vite." That fidelity is the @@ -37,6 +38,12 @@ const webDir = path.join(repoRoot, "clients/web"); // with an actionable message rather than a raw ENOENT stack. const entryPath = path.join(webDir, "src/main.tsx"); +// Mirrors BROWSER_EXTERNALIZED_BUILTIN_PHRASE in +// clients/web/server/browser-externalized-builtin-gate.ts; kept as a literal +// because this plain .mjs script can't import the TS source. Used to tell apart +// the ways a passing build can mean the gate broke (see the diagnoses below). +const KNOWN_PHRASE = "has been externalized for browser compatibility"; + // A namespace import + guarded use so the built-in isn't tree-shaken before Vite // externalizes it (a bare side-effect import can be dropped). `__never__` is // never truthy, so the reference survives to build time without running. @@ -80,13 +87,24 @@ function restoreEntry() { for (const signal of ["SIGINT", "SIGTERM"]) { process.on(signal, () => { restoreEntry(); - process.exit(130); + // Conventional 128 + signal number: SIGINT → 130, SIGTERM → 143. + process.exit(signal === "SIGINT" ? 130 : 143); }); } -let result; +// Mutate the entry (guarded), THEN wrap only the build in the restore-`finally` +// — so a write failure (read-only checkout, EACCES) fails actionably before any +// mutation, rather than escaping the finally as a raw stack. try { writeFileSync(entryPath, original + PROBE); +} catch (err) { + fail( + `could not write the probe into ${path.relative(repoRoot, entryPath)} (${err.message})`, + ); +} + +let result; +try { console.log( "verify:build-gate: running a real `vite build` with a node:fs probe (takes a minute)…", ); @@ -94,10 +112,15 @@ try { // point is proving the message-keyed gate fires against THIS Vite, so `npx` // must never silently fetch a different version from the registry when // clients/web/node_modules is missing/partial. A missing local bin then - // surfaces via the `result.error` check below. + // surfaces via the `result.error` check below. `timeout` bounds a hung build: + // spawnSync sets `result.error` (ETIMEDOUT) on timeout, so the same branch + // reports it — otherwise a hang would burn to the GitHub job's 360-min default + // with no output (this step captures rather than inherits stdio). result = spawnSync("npx", ["--no-install", "vite", "build"], { cwd: webDir, encoding: "utf8", + timeout: 10 * 60_000, + killSignal: "SIGKILL", }); } finally { // Always restore the entry, even if the build spawn threw. @@ -127,10 +150,34 @@ if (result.error) { const output = `${result.stdout ?? ""}\n${result.stderr ?? ""}`; if (result.status === 0) { + // A passing build with a Node built-in in the browser graph means the gate + // broke — but in three distinct ways, each pointing at a different file. The + // captured output distinguishes them (Vite prints the warning at the default + // log level, and no build script passes `--logLevel`, so its presence is + // reliable). + if (output.includes(KNOWN_PHRASE)) { + fail( + "vite build SUCCEEDED but Vite DID emit the externalization warning — the " + + "gate plugin isn't applying. In clients/web/vite.config.ts the plugin may " + + "have been removed from `plugins`, its `applyToEnvironment` may no longer " + + "match the browser environment's name, or a `build.rollupOptions.onwarn` " + + "suppression was added above it.", + output, + ); + } + if (output.includes("node:fs")) { + fail( + "vite build SUCCEEDED and the warning phrasing drifted — the probe reached " + + "the graph (node:fs is named) but the known phrase is absent. Update " + + "BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts " + + "to match the new Vite wording.", + output, + ); + } fail( - "vite build SUCCEEDED with a node:fs import in the browser graph — 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).", + "vite build SUCCEEDED and the probe never reached the browser graph (neither " + + "the known phrase nor node:fs appears in the output) — the entry may have " + + "moved or the probe was tree-shaken. Check this script's PROBE / entryPath.", output, ); } From 371472f36d7f40feae2f1bde3c31c7243647e499 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 17:00:06 -0400 Subject: [PATCH 06/13] =?UTF-8?q?build(web):=20round-5=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20self-consistent=20verify-build-gate=20(#1769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All in scripts/verify-build-gate.mjs: - Drift guard: assert the gate source file still contains the mirrored KNOWN_PHRASE literal, failing fast if the two drift. Without it, a stale KNOWN_PHRASE after a phrasing update would make the three-way diagnosis misreport a plugin-not-applying regression as a phrasing drift — the exact misdirection the diagnosis exists to avoid. - Make the .bak safety net reachable on the path that needs it most: restoreEntry now catches its own write failure and routes to a shared saveBackupAndFail helper, instead of letting the write escape the finally as a raw stack (which skipped the .bak fallback and left the probe injected in the entry). - Add the fourth "plugin isn't applying" cause to the diagnosis: a future Vite emitting the warning before the client env's buildStart reset clears it (same symptom as the other three). - Flag once, on the failing-check path, that clients/web/dist holds a probe build so a local debugger doesn't serve it unaware. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/verify-build-gate.mjs | 75 ++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 15 deletions(-) diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 9681f86e7..05328c379 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -37,11 +37,16 @@ const webDir = path.join(repoRoot, "clients/web"); // Hardcoded browser entry. If it's ever renamed, the guarded read below fails // with an actionable message rather than a raw ENOENT stack. const entryPath = path.join(webDir, "src/main.tsx"); +const gatePath = path.join( + webDir, + "server/browser-externalized-builtin-gate.ts", +); // Mirrors BROWSER_EXTERNALIZED_BUILTIN_PHRASE in // clients/web/server/browser-externalized-builtin-gate.ts; kept as a literal // because this plain .mjs script can't import the TS source. Used to tell apart // the ways a passing build can mean the gate broke (see the diagnoses below). +// The drift guard below keeps this literal honest against that source. const KNOWN_PHRASE = "has been externalized for browser compatibility"; // A namespace import + guarded use so the built-in isn't tree-shaken before Vite @@ -60,6 +65,23 @@ function fail(message, detail) { process.exit(1); } +// Write the captured original to a sidecar `.bak` and fail — the honest remedy +// when the in-place restore can't be trusted, since it preserves any +// uncommitted edits the developer had (unlike `git checkout --`). +function saveBackupAndFail(reason) { + const backupPath = `${entryPath}.verify-build-gate.bak`; + try { + writeFileSync(backupPath, original); + } catch { + // Best effort: if even the sidecar can't be written, the reason below (and + // the injected probe still in the entry) is all we can offer. + } + fail( + `${reason} — pre-run contents were saved to ${path.relative(repoRoot, backupPath)}; ` + + `restore from there (it preserves uncommitted edits, unlike 'git checkout --')`, + ); +} + let original; try { original = readFileSync(entryPath, "utf8"); @@ -70,12 +92,33 @@ try { ); } +// Fail fast if the mirrored literal drifted from the source of truth: a stale +// KNOWN_PHRASE would make the three-way diagnosis below misreport a +// plugin-not-applying regression as a phrasing drift (the exact misdirection the +// diagnosis exists to avoid). +if (!readFileSync(gatePath, "utf8").includes(KNOWN_PHRASE)) { + fail( + `KNOWN_PHRASE no longer appears in ${path.relative(repoRoot, gatePath)} — the ` + + `mirrored literals drifted; re-sync KNOWN_PHRASE with ` + + `BROWSER_EXTERNALIZED_BUILTIN_PHRASE`, + ); +} + let restored = false; +// Restore the entry in place. On failure the sidecar-`.bak` path is the reachable +// safety net — letting the write escape the `finally` would skip it and leave the +// probe injected in the entry (the worst outcome this script can produce). function restoreEntry() { if (restored) return; - writeFileSync(entryPath, original); - restored = true; + try { + writeFileSync(entryPath, original); + restored = true; + } catch (err) { + saveBackupAndFail( + `failed to restore ${path.relative(repoRoot, entryPath)} (${err.message})`, + ); + } } // A `finally` doesn't run on Ctrl-C during the multi-minute build; restore the @@ -127,16 +170,11 @@ try { restoreEntry(); } -// Guard against a botched restore leaving the tree dirty. Write the captured -// original to a sidecar `.bak` and point there — NOT `git checkout --`, which -// would also discard any uncommitted edits the developer had in the entry. +// Guard against a botched restore that wrote *something* other than the original +// (distinct from restoreEntry's write throwing, which it handles itself). if (readFileSync(entryPath, "utf8") !== original) { - const backupPath = `${entryPath}.verify-build-gate.bak`; - writeFileSync(backupPath, original); - fail( - `failed to restore ${path.relative(repoRoot, entryPath)} — its pre-run ` + - `contents were saved to ${path.relative(repoRoot, backupPath)}; restore ` + - `from there (it preserves any uncommitted edits, unlike 'git checkout --')`, + saveBackupAndFail( + `failed to restore ${path.relative(repoRoot, entryPath)} (contents differ)`, ); } @@ -151,17 +189,24 @@ const output = `${result.stdout ?? ""}\n${result.stderr ?? ""}`; if (result.status === 0) { // A passing build with a Node built-in in the browser graph means the gate - // broke — but in three distinct ways, each pointing at a different file. The + // broke — but in distinct ways, each pointing at a different file. The // captured output distinguishes them (Vite prints the warning at the default // log level, and no build script passes `--logLevel`, so its presence is - // reliable). + // reliable). All three paths completed the build, so `clients/web/dist` now + // holds a probe bundle — harmless (the probe never runs) and overwritten by + // the next `validate`/`build`; flagged once here so a local debugger doesn't + // serve it via `npm run web` unaware. + console.error( + "verify:build-gate: note — clients/web/dist now holds a probe build; run a normal build before serving it.", + ); if (output.includes(KNOWN_PHRASE)) { fail( "vite build SUCCEEDED but Vite DID emit the externalization warning — the " + "gate plugin isn't applying. In clients/web/vite.config.ts the plugin may " + "have been removed from `plugins`, its `applyToEnvironment` may no longer " + - "match the browser environment's name, or a `build.rollupOptions.onwarn` " + - "suppression was added above it.", + "match the browser environment's name, a `build.rollupOptions.onwarn` " + + "suppression was added above it, or a future Vite emitted the warning " + + "before the client environment's `buildStart` reset (which then cleared it).", output, ); } From 223ec0fad70d289429d5ffa33939e323296f93a7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 17:21:52 -0400 Subject: [PATCH 07/13] =?UTF-8?q?build(web):=20round-6=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20verify-build-gate=20literal=20contracts=20(#1769?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All in scripts/verify-build-gate.mjs; one theme — the literals the script asserts on can each go stale independently of what they mirror: - Success key: assert on the gate's distinctive thrown-error prefix "Build failed (#1769)", not a bare "#1769". The issue number also appears in vite.config.ts comments, so a code frame from an unrelated build error there could otherwise make the check report OK with the gate dead — the one direction it must never get wrong (a false pass hiding a dead gate). - Drift guard now anchors on the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment (regex) instead of a whole-file substring match, so the old wording lingering in a comment can't mask a changed constant; and its readFileSync of the gate module is guarded so a moved file fails through fail() actionably. - Extract PROBE_MODULE ("node:fs") used to build both the PROBE and the diagnosis check + messages, so changing the probe module can't silently misroute a phrasing-drift failure into "probe never reached the graph". - Comment the load-bearing diagnosis order (phrase before module-name). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/verify-build-gate.mjs | 53 +++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 05328c379..99a499fc1 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -49,6 +49,12 @@ const gatePath = path.join( // The drift guard below keeps this literal honest against that source. const KNOWN_PHRASE = "has been externalized for browser compatibility"; +// The Node built-in the probe imports. Single source of truth: the diagnosis +// below tests the output for this exact name, so deriving both from one constant +// keeps a phrasing-drift failure from being misrouted as "probe never reached +// the graph" if the probe module ever changes. +const PROBE_MODULE = "node:fs"; + // A namespace import + guarded use so the built-in isn't tree-shaken before Vite // externalizes it (a bare side-effect import can be dropped). `__never__` is // never truthy, so the reference survives to build time without running. @@ -56,7 +62,7 @@ const KNOWN_PHRASE = "has been externalized for browser compatibility"; // resolve time, and appending won't demote a leading directive (e.g. a future // `"use client"`) the way prepending would. const PROBE = - '\nimport * as __nodeBuiltinProbe from "node:fs";\n' + + `\nimport * as __nodeBuiltinProbe from "${PROBE_MODULE}";\n` + "if (globalThis.__never__) console.log(__nodeBuiltinProbe);\n"; function fail(message, detail) { @@ -65,6 +71,11 @@ function fail(message, detail) { process.exit(1); } +// Escape a literal for safe interpolation into a RegExp. +function escapeRegExp(literal) { + return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + // Write the captured original to a sidecar `.bak` and fail — the honest remedy // when the in-place restore can't be trusted, since it preserves any // uncommitted edits the developer had (unlike `git checkout --`). @@ -95,12 +106,25 @@ try { // Fail fast if the mirrored literal drifted from the source of truth: a stale // KNOWN_PHRASE would make the three-way diagnosis below misreport a // plugin-not-applying regression as a phrasing drift (the exact misdirection the -// diagnosis exists to avoid). -if (!readFileSync(gatePath, "utf8").includes(KNOWN_PHRASE)) { +// diagnosis exists to avoid). Anchor on the *assignment* — matching the whole +// file would let the old wording lingering in a comment mask a changed constant. +let gateSource; +try { + gateSource = readFileSync(gatePath, "utf8"); +} catch (err) { + fail( + `could not read the gate module ${path.relative(repoRoot, gatePath)} ` + + `(${err.message}) — if it moved, update gatePath in this script`, + ); +} +const phraseAssignment = new RegExp( + `BROWSER_EXTERNALIZED_BUILTIN_PHRASE\\s*=\\s*["'\`]${escapeRegExp(KNOWN_PHRASE)}["'\`]`, +); +if (!phraseAssignment.test(gateSource)) { fail( - `KNOWN_PHRASE no longer appears in ${path.relative(repoRoot, gatePath)} — the ` + - `mirrored literals drifted; re-sync KNOWN_PHRASE with ` + - `BROWSER_EXTERNALIZED_BUILTIN_PHRASE`, + `KNOWN_PHRASE here no longer matches the BROWSER_EXTERNALIZED_BUILTIN_PHRASE ` + + `assignment in ${path.relative(repoRoot, gatePath)} — the mirrored literals ` + + `drifted; re-sync them.`, ); } @@ -199,6 +223,8 @@ if (result.status === 0) { console.error( "verify:build-gate: note — clients/web/dist now holds a probe build; run a normal build before serving it.", ); + // Order matters: a phrase match implies the probe reached the graph, so it + // must be checked before the module-name fallback — don't reorder these. if (output.includes(KNOWN_PHRASE)) { fail( "vite build SUCCEEDED but Vite DID emit the externalization warning — the " + @@ -210,10 +236,10 @@ if (result.status === 0) { output, ); } - if (output.includes("node:fs")) { + if (output.includes(PROBE_MODULE)) { fail( "vite build SUCCEEDED and the warning phrasing drifted — the probe reached " + - "the graph (node:fs is named) but the known phrase is absent. Update " + + `the graph (${PROBE_MODULE} is named) but the known phrase is absent. Update ` + "BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts " + "to match the new Vite wording.", output, @@ -221,13 +247,18 @@ if (result.status === 0) { } fail( "vite build SUCCEEDED and the probe never reached the browser graph (neither " + - "the known phrase nor node:fs appears in the output) — the entry may have " + - "moved or the probe was tree-shaken. Check this script's PROBE / entryPath.", + `the known phrase nor ${PROBE_MODULE} appears in the output) — the entry may ` + + "have moved or the probe was tree-shaken. Check this script's PROBE / entryPath.", output, ); } -if (!output.includes("#1769")) { +// Assert on the gate's distinctive thrown-error prefix, NOT a bare "#1769": +// that issue number also appears in clients/web/vite.config.ts comments, so a +// code frame from an unrelated build error there could otherwise make this +// report OK with the gate dead. This prefix + KNOWN_PHRASE are the script's +// contract with browser-externalized-builtin-gate.ts's exported error. +if (!output.includes("Build failed (#1769)")) { fail( "vite build failed, but not via the #1769 gate — the build broke for another " + "reason, so this check no longer proves the gate works.", From d01fe18d76fdf71e9d6b89155040714dd041ff68 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 17:45:01 -0400 Subject: [PATCH 08/13] =?UTF-8?q?build(web):=20round-7=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20drift-guard=20both=20mirrored=20literals=20(#176?= =?UTF-8?q?9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract ERROR_PREFIX ("Build failed (#1769)") and drift-guard it against the gate module too, and use it as the success key. Previously only KNOWN_PHRASE was guarded; the prefix is mirrored across the same TS/.mjs boundary for the same reason, so rewording the gate's thrown error would make a correctly-firing gate report "the build broke for another reason" (benign direction, but the same misdirection the other guards close). - Write the restore-failure .bak under the OS temp dir, not next to the entry: a sidecar inside src/ is untracked and a `git add -A` (likely while recovering) would sweep it into a commit. - Branch-(b) remediation now says update BOTH the gate constant AND KNOWN_PHRASE, so a phrasing-drift fix lands in one pass instead of tripping the drift guard on the next run. - Note next to BROWSER_EXTERNALIZED_BUILTIN_PHRASE that it must stay a single string literal (the script's drift guard anchors a regex on the assignment). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../browser-externalized-builtin-gate.ts | 5 +- scripts/verify-build-gate.mjs | 62 ++++++++++++------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index d587de54e..167eda289 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -26,7 +26,10 @@ // user-facing phrasing, whose troubleshooting anchor // (`module-externalized-for-browser-compatibility`) Vite treats as stable. // The `verify:build-gate` script exercises a real build so a phrasing drift in a -// future Vite bump fails CI here rather than silently disabling the gate. +// future Vite bump fails CI here rather than silently disabling the gate. That +// script also drift-guards its mirrored copy by regex against this assignment — +// keep it a single string literal (no concatenation/template) or the guard will +// report a false drift. export const BROWSER_EXTERNALIZED_BUILTIN_PHRASE = "has been externalized for browser compatibility"; diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 99a499fc1..517f03b6e 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -29,6 +29,7 @@ import { spawnSync } from "node:child_process"; import { readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -42,12 +43,14 @@ const gatePath = path.join( "server/browser-externalized-builtin-gate.ts", ); -// Mirrors BROWSER_EXTERNALIZED_BUILTIN_PHRASE in -// clients/web/server/browser-externalized-builtin-gate.ts; kept as a literal -// because this plain .mjs script can't import the TS source. Used to tell apart -// the ways a passing build can mean the gate broke (see the diagnoses below). -// The drift guard below keeps this literal honest against that source. +// The two literals this .mjs mirrors from the gate module (it can't import the +// TS source). KNOWN_PHRASE tells apart the ways a passing build can mean the gate +// broke (see the diagnoses below); ERROR_PREFIX is the success key — the +// distinctive lead of the gate's thrown error, so an unrelated build error whose +// output merely mentions "#1769" (it's in vite.config.ts comments) can't report +// OK. The drift guard below keeps BOTH honest against the source. const KNOWN_PHRASE = "has been externalized for browser compatibility"; +const ERROR_PREFIX = "Build failed (#1769)"; // The Node built-in the probe imports. Single source of truth: the diagnosis // below tests the output for this exact name, so deriving both from one constant @@ -76,11 +79,16 @@ function escapeRegExp(literal) { return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } -// Write the captured original to a sidecar `.bak` and fail — the honest remedy -// when the in-place restore can't be trusted, since it preserves any -// uncommitted edits the developer had (unlike `git checkout --`). +// Write the captured original to a `.bak` and fail — the honest remedy when the +// in-place restore can't be trusted, since it preserves any uncommitted edits +// the developer had (unlike `git checkout --`). Written under the OS temp dir, +// NOT next to the entry: a sidecar inside `src/` is untracked and a `git add -A` +// (likely while recovering) would sweep it into a commit. function saveBackupAndFail(reason) { - const backupPath = `${entryPath}.verify-build-gate.bak`; + const backupPath = path.join( + tmpdir(), + `${path.basename(entryPath)}.verify-build-gate.bak`, + ); try { writeFileSync(backupPath, original); } catch { @@ -88,8 +96,8 @@ function saveBackupAndFail(reason) { // the injected probe still in the entry) is all we can offer. } fail( - `${reason} — pre-run contents were saved to ${path.relative(repoRoot, backupPath)}; ` + - `restore from there (it preserves uncommitted edits, unlike 'git checkout --')`, + `${reason} — pre-run contents were saved to ${backupPath}; restore from ` + + `there (it preserves uncommitted edits, unlike 'git checkout --')`, ); } @@ -103,11 +111,14 @@ try { ); } -// Fail fast if the mirrored literal drifted from the source of truth: a stale +// Fail fast if either mirrored literal drifted from the source of truth: a stale // KNOWN_PHRASE would make the three-way diagnosis below misreport a -// plugin-not-applying regression as a phrasing drift (the exact misdirection the -// diagnosis exists to avoid). Anchor on the *assignment* — matching the whole -// file would let the old wording lingering in a comment mask a changed constant. +// plugin-not-applying regression as a phrasing drift, and a stale ERROR_PREFIX +// would make a correctly-firing gate report "the build broke for another +// reason" (both exactly the misdirection this script exists to avoid). Anchor +// the phrase on the *assignment* — matching the whole file would let the old +// wording lingering in a comment mask a changed constant; the prefix is +// distinctive enough that a plain substring is fine. let gateSource; try { gateSource = readFileSync(gatePath, "utf8"); @@ -117,14 +128,20 @@ try { `(${err.message}) — if it moved, update gatePath in this script`, ); } +const gateRel = path.relative(repoRoot, gatePath); const phraseAssignment = new RegExp( `BROWSER_EXTERNALIZED_BUILTIN_PHRASE\\s*=\\s*["'\`]${escapeRegExp(KNOWN_PHRASE)}["'\`]`, ); if (!phraseAssignment.test(gateSource)) { fail( `KNOWN_PHRASE here no longer matches the BROWSER_EXTERNALIZED_BUILTIN_PHRASE ` + - `assignment in ${path.relative(repoRoot, gatePath)} — the mirrored literals ` + - `drifted; re-sync them.`, + `assignment in ${gateRel} — the mirrored literals drifted; re-sync them.`, + ); +} +if (!gateSource.includes(ERROR_PREFIX)) { + fail( + `ERROR_PREFIX here ("${ERROR_PREFIX}") no longer appears in ${gateRel} — the ` + + `gate's thrown error was reworded; re-sync ERROR_PREFIX with it.`, ); } @@ -240,8 +257,9 @@ if (result.status === 0) { fail( "vite build SUCCEEDED and the warning phrasing drifted — the probe reached " + `the graph (${PROBE_MODULE} is named) but the known phrase is absent. Update ` + - "BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts " + - "to match the new Vite wording.", + "BOTH BROWSER_EXTERNALIZED_BUILTIN_PHRASE in browser-externalized-builtin-gate.ts " + + "AND KNOWN_PHRASE in this script to the new Vite wording (the drift guard " + + "requires they stay in sync).", output, ); } @@ -256,9 +274,9 @@ if (result.status === 0) { // Assert on the gate's distinctive thrown-error prefix, NOT a bare "#1769": // that issue number also appears in clients/web/vite.config.ts comments, so a // code frame from an unrelated build error there could otherwise make this -// report OK with the gate dead. This prefix + KNOWN_PHRASE are the script's -// contract with browser-externalized-builtin-gate.ts's exported error. -if (!output.includes("Build failed (#1769)")) { +// report OK with the gate dead. ERROR_PREFIX + KNOWN_PHRASE are the script's +// contract with the gate module, and both are drift-guarded above. +if (!output.includes(ERROR_PREFIX)) { fail( "vite build failed, but not via the #1769 gate — the build broke for another " + "reason, so this check no longer proves the gate works.", From d2a16de3c563267e1619df18aa4bdc6b43b4e324 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 18:08:14 -0400 Subject: [PATCH 09/13] =?UTF-8?q?build(web):=20round-8=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20safe=20recovery=20path=20+=20structural=20onLog?= =?UTF-8?q?=20order=20(#1769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - saveBackupAndFail no longer claims a backup it may not have written: it tracks whether the write succeeded and only points the user at the .bak in that case (otherwise it says the probe is still injected and to restore from VCS). Uses mkdtempSync for a unique dir instead of a fixed tmp filename, so a pre-existing/foreign-owned /tmp file can't make the recovery message point at someone else's content. - Guard the restore-verification readFileSync too, routing a failure through saveBackupAndFail so the whole restore path is uniformly safe (a raw throw there would skip the backup net). - vite.config.ts: enforce: 'pre' on the gate plugin so it runs ahead of the normal-plugin group in the onLog chain — a plugin whose onLog returns false filters the log for later plugins, so trailing the array would let a future log-filtering plugin silently blind the gate. Makes "the gate sees every log" structural rather than array-position-dependent. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- clients/web/vite.config.ts | 6 +++++ scripts/verify-build-gate.mjs | 50 ++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index b0a34342c..9c42d7def 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -58,6 +58,12 @@ function browserExternalizedBuiltinGate(): Plugin { name: 'inspector:fail-on-browser-externalized-builtin', apply: 'build', applyToEnvironment: (environment) => environment.name === 'client', + // `enforce: 'pre'` runs this ahead of the normal-plugin group in the `onLog` + // chain: a plugin whose `onLog` returns `false` filters that log for every + // later plugin, so trailing the array would let a future log-filtering plugin + // silently blind the gate. Ordering the gate first makes "it sees every log" + // structural rather than dependent on array position. + enforce: 'pre', buildStart() { gate.reset(); }, diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 517f03b6e..71573b890 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -28,7 +28,7 @@ */ import { spawnSync } from "node:child_process"; -import { readFileSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -79,26 +79,30 @@ function escapeRegExp(literal) { return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } -// Write the captured original to a `.bak` and fail — the honest remedy when the +// Write the captured original to a backup and fail — the honest remedy when the // in-place restore can't be trusted, since it preserves any uncommitted edits -// the developer had (unlike `git checkout --`). Written under the OS temp dir, -// NOT next to the entry: a sidecar inside `src/` is untracked and a `git add -A` -// (likely while recovering) would sweep it into a commit. +// the developer had (unlike `git checkout --`). The backup goes in a fresh +// `mkdtempSync` dir, NOT next to the entry (a sidecar inside `src/` is untracked +// and a recovery-time `git add -A` would commit it) and NOT a fixed tmp filename +// (a pre-existing/foreign-owned file would make us point the user at someone +// else's content). The message only claims a backup when the write succeeded — +// this path runs during recovery, so it must not misdirect. function saveBackupAndFail(reason) { - const backupPath = path.join( - tmpdir(), - `${path.basename(entryPath)}.verify-build-gate.bak`, - ); + let savedTo; try { - writeFileSync(backupPath, original); + const dir = mkdtempSync(path.join(tmpdir(), "verify-build-gate-")); + savedTo = path.join(dir, path.basename(entryPath)); + writeFileSync(savedTo, original); } catch { - // Best effort: if even the sidecar can't be written, the reason below (and - // the injected probe still in the entry) is all we can offer. + savedTo = undefined; } - fail( - `${reason} — pre-run contents were saved to ${backupPath}; restore from ` + - `there (it preserves uncommitted edits, unlike 'git checkout --')`, - ); + const remedy = savedTo + ? `pre-run contents were saved to ${savedTo}; restore from there ` + + `(it preserves uncommitted edits, unlike 'git checkout --')` + : `a backup could NOT be written — the entry still has the probe injected; ` + + `restore it from version control (this discards uncommitted edits to it) ` + + `once the filesystem is writable`; + fail(`${reason} — ${remedy}`); } let original; @@ -212,8 +216,18 @@ try { } // Guard against a botched restore that wrote *something* other than the original -// (distinct from restoreEntry's write throwing, which it handles itself). -if (readFileSync(entryPath, "utf8") !== original) { +// (distinct from restoreEntry's write throwing, which it handles itself). The +// verifying read is itself routed through saveBackupAndFail so the restore path +// is uniformly safe — a raw throw here would skip the backup net. +let afterRestore; +try { + afterRestore = readFileSync(entryPath, "utf8"); +} catch (err) { + saveBackupAndFail( + `could not re-read ${path.relative(repoRoot, entryPath)} to verify the restore (${err.message})`, + ); +} +if (afterRestore !== original) { saveBackupAndFail( `failed to restore ${path.relative(repoRoot, entryPath)} (contents differ)`, ); From cbe2ab5b01ba8ce3e2052dff21eecb426a21db5f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 18:26:00 -0400 Subject: [PATCH 10/13] =?UTF-8?q?build(web):=20round-9=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20prove=20the=20gate=20fired=20on=20the=20probe=20?= =?UTF-8?q?(#1769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The success key confirmed the #1769 gate fired, but not that OUR probe tripped it. If the repo already leaked a Node built-in, the gate would fire on that and the script would report OK even if the probe was tree-shaken or entryPath went stale — branch (c)'s failure silently inverted into a pass. Add an assertion that PROBE_MODULE appears among the gate's embedded offender warnings. Unreachable inside `npm run ci` (validate's build:web fails first on a pre-existing leak), but this script is a documented standalone command — run exactly when debugging a leak, the case where this mattered. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- scripts/verify-build-gate.mjs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 71573b890..c46c4a212 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -298,6 +298,22 @@ if (!output.includes(ERROR_PREFIX)) { ); } +// The gate fired — but confirm it fired on OUR probe, not a pre-existing leak. +// The gate's error embeds every original Vite warning, and those name the module, +// so PROBE_MODULE must appear. Without this, a repo that already leaks a built-in +// would report OK even if the probe was tree-shaken or entryPath went stale — +// branch (c)'s failure, silently inverted into a pass. (Unreachable inside +// `npm run ci`: `validate`'s `build:web` fails first on a pre-existing leak — but +// this script is a documented standalone command, run exactly when debugging one.) +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 (was it tree-shaken, or has entryPath gone stale?).`, + output, + ); +} + console.log( "verify:build-gate OK — vite build fails on a Node built-in in the browser graph (#1769 gate fired).", ); From 50eb49f43d52f9d44ac7aeadad6c8a689269c1d7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 18:47:11 -0400 Subject: [PATCH 11/13] =?UTF-8?q?build(web):=20round-10=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20roll=20back=20a=20partial=20probe=20write=20(#17?= =?UTF-8?q?69)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe writeFileSync uses the default 'w' flag (truncates at open), so a mid-write ENOSPC/EIO could leave clients/web/src/main.tsx empty or half-written — and its catch called fail() directly, the one path on this script that could destroy the entry rather than merely dirty it (every other read/write is already routed into the recovery net). Call restoreEntry() in that catch to roll the partial write back (falling through to the .bak net if the restore itself can't write), and correct the comment that claimed a write failure happens "before any mutation". Also widen the enforce:'pre' comment to note it orders ahead of Vite's core plugins too (harmless — the gate defines no resolve/load/transform). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- clients/web/vite.config.ts | 13 ++++++++----- scripts/verify-build-gate.mjs | 8 ++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index 9c42d7def..8ea021be3 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -58,11 +58,14 @@ function browserExternalizedBuiltinGate(): Plugin { name: 'inspector:fail-on-browser-externalized-builtin', apply: 'build', applyToEnvironment: (environment) => environment.name === 'client', - // `enforce: 'pre'` runs this ahead of the normal-plugin group in the `onLog` - // chain: a plugin whose `onLog` returns `false` filters that log for every - // later plugin, so trailing the array would let a future log-filtering plugin - // silently blind the gate. Ordering the gate first makes "it sees every log" - // structural rather than dependent on array position. + // `enforce: 'pre'` runs this ahead of the normal-plugin group (and Vite's + // core plugins) in the `onLog` chain: a plugin whose `onLog` returns `false` + // filters that log for every later plugin, so trailing the array would let a + // future log-filtering plugin silently blind the gate. Ordering the gate + // first makes "it sees every log" structural rather than dependent on array + // position. Harmless to the emitting `rolldown:vite-resolve` plugin — the gate + // defines no resolve/load/transform, and `onLog` delivery doesn't depend on + // the emitter's position. enforce: 'pre', buildStart() { gate.reset(); diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index c46c4a212..05f078b7a 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -181,11 +181,15 @@ for (const signal of ["SIGINT", "SIGTERM"]) { } // Mutate the entry (guarded), THEN wrap only the build in the restore-`finally` -// — so a write failure (read-only checkout, EACCES) fails actionably before any -// mutation, rather than escaping the finally as a raw stack. +// — so a write failure fails actionably before the build, and any partial write +// is rolled back rather than escaping as a raw stack. `writeFileSync`'s default +// `'w'` flag truncates at open, so a mid-write ENOSPC/EIO can leave the entry +// empty or half-written; `restoreEntry()` puts the captured original back (and +// itself falls through to the `.bak` net if it can't write). try { writeFileSync(entryPath, original + PROBE); } catch (err) { + restoreEntry(); fail( `could not write the probe into ${path.relative(repoRoot, entryPath)} (${err.message})`, ); From ec8117f7406967934680d6a5ba45700f2c405fda Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 19:09:05 -0400 Subject: [PATCH 12/13] =?UTF-8?q?build(web):=20round-11=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20don't=20let=20the=20rollback=20swallow=20the=20w?= =?UTF-8?q?rite=20error=20(#1769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10's restoreEntry() in the probe-write catch swallowed the primary error: restoreEntry() on its own write failure calls saveBackupAndFail → fail → exit, so the "could not write the probe" diagnosis never ran when the rollback also failed — which is exactly the read-only-checkout / ENOSPC case (both writes fail at open). On a read-only checkout nothing was ever written, yet the developer was told the restore failed and pointed at a /tmp backup of an untouched file. Roll back inline instead: attempt the restore without exiting, track whether it succeeded, then always report the PRIMARY write error, appending "restore from version control" only when the rollback didn't take. Also pin the "Build failed (#1769)" error prefix as contiguous in a single string fragment (a comment symmetric with the phrase pin), since verify-build-gate.mjs drift-guards on it as ERROR_PREFIX. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../browser-externalized-builtin-gate.ts | 4 ++++ scripts/verify-build-gate.mjs | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index 167eda289..48db97cb5 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -49,6 +49,10 @@ export function isBrowserExternalizedBuiltinLog( */ export function browserExternalizedBuiltinError(messages: string[]): Error { const list = messages.map((m) => ` - ${m}`).join("\n"); + // Keep the "Build failed (#1769)" lead contiguous in a single fragment: + // scripts/verify-build-gate.mjs mirrors it as ERROR_PREFIX (its success key) + // and drift-guards on that substring — a reflow splitting the prefix across + // string fragments would trip the guard with a false drift. return new Error( "Build failed (#1769): a Node built-in reached the browser bundle and was " + "externalized to an empty stub, which ships a broken bundle. Remove the " + diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index 05f078b7a..daa3f61d4 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -189,9 +189,23 @@ for (const signal of ["SIGINT", "SIGTERM"]) { try { writeFileSync(entryPath, original + PROBE); } catch (err) { - restoreEntry(); + // Roll back a possible partial write ('w' truncates at open), but inline — + // NOT via restoreEntry(), whose own saveBackupAndFail→exit on a failing + // rollback would swallow this primary write error (the actual diagnosis) and, + // on a read-only checkout where nothing was ever written, misdirect the + // developer to a /tmp backup of an untouched file. + let rolledBack = false; + try { + writeFileSync(entryPath, original); + restored = rolledBack = true; + } catch { + // Reported via the not-rolled-back clause below. + } fail( - `could not write the probe into ${path.relative(repoRoot, entryPath)} (${err.message})`, + `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"), ); } From acf10c74a1a958c27a28d818428efea5be6a6804 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 19:32:28 -0400 Subject: [PATCH 13/13] =?UTF-8?q?build(web):=20round-12=20review=20polish?= =?UTF-8?q?=20=E2=80=94=20don't=20let=20the=20pin=20comment=20defeat=20its?= =?UTF-8?q?=20own=20guard=20(#1769)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-11 pin comment quoted "Build failed (#1769)" verbatim, so the prefix appeared twice in the gate module and verify-build-gate.mjs's whole-file includes(ERROR_PREFIX) drift guard passed on the comment alone — rewording the thrown error would have kept the guard green while a correctly-firing gate reported "the build broke for another reason." Reworded the comment to not repeat the full lead (matching how the phrase pin avoids repeating its phrase); the literal now occurs exactly once, restoring the guard's meaning. Also make the probe-write recovery message factual instead of inferred: read the entry back and only warn "restore from version control" when it actually differs from the original. On a read-only checkout both writes fail at open, so the file is untouched and the old hedge needlessly implied damage. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../browser-externalized-builtin-gate.ts | 10 ++++++---- scripts/verify-build-gate.mjs | 20 +++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/clients/web/server/browser-externalized-builtin-gate.ts b/clients/web/server/browser-externalized-builtin-gate.ts index 48db97cb5..48305a715 100644 --- a/clients/web/server/browser-externalized-builtin-gate.ts +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -49,10 +49,12 @@ export function isBrowserExternalizedBuiltinLog( */ export function browserExternalizedBuiltinError(messages: string[]): Error { const list = messages.map((m) => ` - ${m}`).join("\n"); - // Keep the "Build failed (#1769)" lead contiguous in a single fragment: - // scripts/verify-build-gate.mjs mirrors it as ERROR_PREFIX (its success key) - // and drift-guards on that substring — a reflow splitting the prefix across - // string fragments would trip the guard with a false drift. + // Keep the error's `#1769` lead contiguous in a single string fragment: + // scripts/verify-build-gate.mjs mirrors that lead as ERROR_PREFIX (its success + // key) and drift-guards on it as a whole-file substring, so it must occur + // exactly ONCE here — do not quote the full lead in this comment, and don't + // reflow it across fragments, or the guard would pass on a copy and mask a + // reworded error. return new Error( "Build failed (#1769): a Node built-in reached the browser bundle and was " + "externalized to an empty stub, which ships a broken bundle. Remove the " + diff --git a/scripts/verify-build-gate.mjs b/scripts/verify-build-gate.mjs index daa3f61d4..31f29b37c 100644 --- a/scripts/verify-build-gate.mjs +++ b/scripts/verify-build-gate.mjs @@ -199,13 +199,25 @@ try { writeFileSync(entryPath, original); restored = rolledBack = true; } catch { - // Reported via the not-rolled-back clause below. + // Reported via the damage check below. + } + // Only warn of damage when the entry actually differs from the original — on a + // read-only checkout (EACCES/EROFS, the likeliest trigger) both writes fail at + // open so the file is untouched, and "restore from version control" would + // needlessly discard uncommitted edits. Read failing → assume the worst. + let damaged = false; + if (!rolledBack) { + try { + damaged = readFileSync(entryPath, "utf8") !== original; + } catch { + damaged = true; + } } 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"), + (damaged + ? " — the entry is truncated or partially written; restore it from version control" + : ""), ); }