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..273657d14 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) @@ -210,16 +212,17 @@ 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). 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** 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/README.md b/README.md index 709637e5f..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 @@ -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/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 new file mode 100644 index 000000000..48305a715 --- /dev/null +++ b/clients/web/server/browser-externalized-builtin-gate.ts @@ -0,0 +1,104 @@ +/** + * 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`, 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 +// 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. 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"; + +// 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, +): message is string { + return message?.includes(BROWSER_EXTERNALIZED_BUILTIN_PHRASE) ?? false; +} + +/** + * 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"); + // 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 " + + "node:* / Node built-in import(s) from the browser graph (or gate them " + + "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, + ); +} + +/** Records browser-externalization build logs and fails the build if any were seen. */ +export interface BrowserExternalizedBuiltinGate { + /** Feed each build log's message here; matching ones are collected (deduped). */ + recordLog(message: string | undefined): void; + /** 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; +} + +/** + * 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 — 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 { + const externalized = new Set(); + return { + recordLog(message) { + if (isBrowserExternalizedBuiltinLog(message)) { + externalized.add(message); + } + }, + assertClean() { + if (externalized.size > 0) { + throw browserExternalizedBuiltinError([...externalized]); + } + }, + reset() { + 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 new file mode 100644 index 000000000..51fbf1307 --- /dev/null +++ b/clients/web/src/test/integration/server/browser-externalized-builtin-gate.test.ts @@ -0,0 +1,120 @@ +import { describe, it, expect } from "vitest"; +import { + BROWSER_EXTERNALIZED_BUILTIN_PHRASE, + isBrowserExternalizedBuiltinLog, + browserExternalizedBuiltinError, + 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, ' + + '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 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"); + // Every offending warning is listed so all leaks are visible in one pass. + expect(err.message).toContain(REAL_MESSAGE); + expect(err.message).toContain(second); + }); +}); + +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("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(); + gate.recordLog(REAL_MESSAGE); + 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", () => { + 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); + 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..8ea021be3 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,57 @@ 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` (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 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. +function browserExternalizedBuiltinGate(): Plugin { + const gate = createBrowserExternalizedBuiltinGate(); + return { + name: 'inspector:fail-on-browser-externalized-builtin', + apply: 'build', + applyToEnvironment: (environment) => environment.name === 'client', + // `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(); + }, + onLog(_level, log) { + gate.recordLog(log.message); + }, + buildEnd(error) { + if (!error) 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 +98,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..31f29b37c --- /dev/null +++ b/scripts/verify-build-gate.mjs @@ -0,0 +1,350 @@ +#!/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`. + * + * 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, 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 + * 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"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +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"); +// 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", +); + +// 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 +// 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. +// 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 = + `\nimport * as __nodeBuiltinProbe from "${PROBE_MODULE}";\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); +} + +// Escape a literal for safe interpolation into a RegExp. +function escapeRegExp(literal) { + return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +// 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 --`). 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) { + let savedTo; + try { + const dir = mkdtempSync(path.join(tmpdir(), "verify-build-gate-")); + savedTo = path.join(dir, path.basename(entryPath)); + writeFileSync(savedTo, original); + } catch { + savedTo = undefined; + } + 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; +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`, + ); +} + +// 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, 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"); +} catch (err) { + fail( + `could not read the gate module ${path.relative(repoRoot, gatePath)} ` + + `(${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 ${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.`, + ); +} + +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; + 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 +// 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(); + // Conventional 128 + signal number: SIGINT → 130, SIGTERM → 143. + process.exit(signal === "SIGINT" ? 130 : 143); + }); +} + +// Mutate the entry (guarded), THEN wrap only the build in the restore-`finally` +// — 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) { + // 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 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})` + + (damaged + ? " — the entry is truncated or partially written; restore it from version control" + : ""), + ); +} + +let result; +try { + console.log( + "verify:build-gate: running a real `vite build` with a node:fs probe (takes a minute)…", + ); + // `--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. `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. + restoreEntry(); +} + +// Guard against a botched restore that wrote *something* other than the 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)`, + ); +} + +// 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) { + // A passing build with a Node built-in in the browser graph means the gate + // 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). 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.", + ); + // 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 " + + "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, 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, + ); + } + if (output.includes(PROBE_MODULE)) { + 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 ` + + "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, + ); + } + fail( + "vite build SUCCEEDED and the probe never reached the browser graph (neither " + + `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, + ); +} + +// 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. 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.", + output, + ); +} + +// 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).", +); +process.exit(0);