From dbe95e58edfd77121f0e4a31025009d95a9b7150 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 22:02:54 -0400 Subject: [PATCH] fix(guard): derive must-not-bundle candidates independently of `external` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the suppressed review comment on #2099, which was the sharpest of the three rounds: the guard stopped looking at a package the moment its `external` entry was removed — so deleting `"undici"` from the web config would have made the pre-fix bundle pass. That is self-referential, and #2067 was precisely a MISSING declaration, so the guard covered its regression but not the bug. Candidates are now the union of two independently-derived lists: each client's `external` array, and the root manifest's `dependencies` — root-declared by rule, reached through `core/`, and required to be external in every client bundle. Removing a package from an `external` list can no longer remove it from scrutiny. That immediately surfaced one latent instance of the same rule violation: `@modelcontextprotocol/ext-apps` is root-declared and reached through `core/mcp/apps.ts`, but was absent from the CLI's `external` list and was being inlined into `index.js` — dragging the transitive v1 SDK and zod-to-json-schema in with it. It is ESM, so it was not failing the way `undici` did; what it violated was the rule. Adding it drops the CLI bundle from 590KB to 518KB. It is also a real example of the shape the previous revision could not see: it was inlined into the entry, emitting no `-HASH.js` chunk at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- AGENTS.md | 2 +- README.md | 2 +- clients/cli/tsup.config.ts | 6 ++++ scripts/verify-bundle-externals.mjs | 43 ++++++++++++++++++----- scripts/verify-bundle-externals.test.mjs | 44 ++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 335407fc..48831c45 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,7 +253,7 @@ The same **placement** rule covers anything reached only through **root-owned co **That rule has now been broken twice, so it is enforced by a guard rather than by review.** `undici` predates #2082 and was missed by it: it *was* declared in `clients/cli/package.json`, so tsup externalized it for the CLI and the CLI looked fine, while the web and TUI bundles silently inlined 1.05MB of it (#2067). The failure mode is slightly worse than the `proper-lockfile` one, because the package is `import()`ed lazily rather than at startup: the binary boots normally and only the proxied code path dies — with esbuild's rewritten specifier (`import("./undici-HXPKCIY3.js")`) meaning **no user-side install can ever satisfy it**, so the published clients told every proxy user to install a package that was already present and could not have helped. -**`npm run verify:bundle-externals`** (`scripts/verify-bundle-externals.mjs`, in `npm run ci` and the GitHub workflow) is that guard. It reads the **built output**, not the config — the two disagreed for four releases — and derives what to check from each client's own `external` array, so a newly externalized package is covered without editing the guard. Detection keys off esbuild's `// ` module banners, which covers both shapes an inlining can take: a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces, i.e. #2067) and a statically-imported package folded straight into `index.js`, which emits no chunk and a chunk-name check would miss. Reading banners means the guard needs unminified output, which it asserts rather than assumes — a bundle with no banners fails loudly instead of reporting clean, so turning on `minify` surfaces here. It **fails closed** on the other way it could lose sight of a bundle, too: parsing the `external` array keys off literal config text, so if a tsup refactor leaves it parsing nothing, that is an error rather than a vacuous pass. Two more things worth knowing: +**`npm run verify:bundle-externals`** (`scripts/verify-bundle-externals.mjs`, in `npm run ci` and the GitHub workflow) is that guard. It reads the **built output**, not the config — the two disagreed for four releases — and checks the union of two independently-derived candidate lists: each client's own `external` array, **and the root manifest's `dependencies`**. The second is what makes it cover #2067 rather than only its regression: `undici` was *missing* from the web and TUI `external` lists — that omission was the bug — so a guard driven by those lists alone would have inspected the inlined 1.05MB bundle and reported success. Removing a package from an `external` list must not remove it from scrutiny. Deriving both ways also means a newly added root dependency, or a newly externalized package, is covered without editing the guard. Detection keys off esbuild's `// ` module banners, which covers both shapes an inlining can take: a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces, i.e. #2067) and a statically-imported package folded straight into `index.js`, which emits no chunk and a chunk-name check would miss. Reading banners means the guard needs unminified output, which it asserts rather than assumes — a bundle with no banners fails loudly instead of reporting clean, so turning on `minify` surfaces here. It **fails closed** on the other way it could lose sight of a bundle, too: parsing the `external` array keys off literal config text, so if a tsup refactor leaves it parsing nothing, that is an error rather than a vacuous pass. Two more things worth knowing: - ⚠️ **Nothing in the ordinary test tiers catches this class.** Unit and integration tests run against **source**, where the real package is on the resolution path; the smokes run the built tree but never take the code path (for `undici`, no proxy env var is set), so the lazy `import()` never executes. The defect exists only in bundled output, only on a path the smokes don't walk. - ⚠️ **Probing it by hand with `node -e` falsely succeeds.** `node -e 'import("./undici-XXXX.js")'` resolves fine, because `node -e` exposes a global `require` that satisfies esbuild's `typeof require !== "undefined"` guard. Reproduce from a real `.mjs` file, or you will conclude the chunk is loadable when it is not. diff --git a/README.md b/README.md index 25a2555d..51a0a4f7 100644 --- a/README.md +++ b/README.md @@ -367,7 +367,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `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 three headless-Chromium smokes: a 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), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. | | `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 verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that client declares `external` was inlined anyway. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. | +| `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index 5518a7f1..e0463a3b 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -38,6 +38,12 @@ export default defineConfig({ "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", + // Root-declared and reached through `core/mcp/apps.ts`, so it must be + // external here like every other root runtime dependency — it was being + // inlined (dragging the transitive v1 SDK and zod-to-json-schema with it), + // which the #2067 guard surfaced. ESM, so it was not failing the way + // `undici` did; the rule is what it violated. + "@modelcontextprotocol/ext-apps", "commander", "pino", ], diff --git a/scripts/verify-bundle-externals.mjs b/scripts/verify-bundle-externals.mjs index 818157f6..930bec36 100644 --- a/scripts/verify-bundle-externals.mjs +++ b/scripts/verify-bundle-externals.mjs @@ -136,14 +136,32 @@ export function findInlinedExternals({ externals, files }) { /** * The per-client verdict, as pure data so every branch is unit-testable. * - * Note it **fails closed on an empty `external` list**. `parseExternals` keys - * off the literal text `external: [`, so an ordinary tsup refactor — hoisting - * the array to a constant, or a line break after the colon — would make it - * return nothing, and a guard that checks zero packages reports success. Every - * enrolled bundle has externals today, so "none parsed" means the parser lost - * track of the config, not that the invariant holds. + * The candidate set is the **union of two independently-derived lists**, and the + * second is what makes this guard cover #2067 rather than merely its regression: + * + * - `externals` — what the client's tsup config declares. Catches a package that + * was meant to stay out and got pulled in anyway. + * - `rootDependencies` — the root manifest's runtime dependencies. These are + * root-declared *by rule* (see AGENTS.md), reached through `core/`, and must be + * external in every client bundle. Deriving them independently is essential, + * because #2067 was a **missing** declaration: `undici` was absent from the web + * and TUI `external` lists, so a check driven only by those lists would have + * looked at the inlined 1.05MB bundle and reported success. Removing a package + * from an `external` list must not be able to remove it from scrutiny. + * + * It also **fails closed on an empty `external` list**. `parseExternals` keys off + * the literal text `external: [`, so an ordinary tsup refactor — hoisting the + * array to a constant, or a line break after the colon — would make it return + * nothing. Every enrolled bundle has externals today, so "none parsed" means the + * parser lost track of the config, not that the invariant holds. */ -export function evaluateClient({ name, build, externals, files }) { +export function evaluateClient({ + name, + build, + externals, + rootDependencies = [], + files, +}) { if (externals.length === 0) { return [ `${name}: no \`external\` entries could be parsed from its tsup config. ` + @@ -153,7 +171,11 @@ export function evaluateClient({ name, build, externals, files }) { ]; } - const { noBanners, violations } = findInlinedExternals({ externals, files }); + const candidates = [...new Set([...externals, ...rootDependencies])]; + const { noBanners, violations } = findInlinedExternals({ + externals: candidates, + files, + }); if (noBanners) { return [ `${name}: ${build} has no esbuild module banners, so this guard cannot see ` + @@ -169,6 +191,10 @@ export function evaluateClient({ name, build, externals, files }) { function main() { const failures = []; + const rootDependencies = Object.keys( + JSON.parse(readFileSync(join(repoRoot, "package.json"), "utf8")) + .dependencies ?? {}, + ); for (const client of BUNDLED_CLIENTS) { const buildDir = join(repoRoot, client.build); const entry = join(buildDir, "index.js"); @@ -192,6 +218,7 @@ function main() { name: client.name, build: client.build, externals, + rootDependencies, files, }), ); diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs index 7d03ac53..ec744e07 100644 --- a/scripts/verify-bundle-externals.test.mjs +++ b/scripts/verify-bundle-externals.test.mjs @@ -205,12 +205,56 @@ describe("evaluateClient", () => { ); }); + test("flags a root dependency even when the external list omits it", () => { + // THE #2067 SHAPE, and the reason the candidate set is not just `externals`. + // `undici` was missing from the web and TUI external lists — that omission + // WAS the bug — so a guard driven only by those lists would have inspected + // the inlined 1.05MB bundle and reported success. Removing a package from an + // `external` list must not remove it from scrutiny. + const failures = evaluateClient({ + name: "web", + build: "clients/web/build", + externals: ["pino"], + rootDependencies: ["undici", "pino"], + files: [ + { + name: "undici-HXPKCIY3.js", + source: banner("../../node_modules/undici/lib/core/symbols.js"), + }, + ], + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /"undici" is declared external but was inlined/); + }); + + test("does not flag a deliberately inlined package that is not a root dependency", () => { + // The TUI inlines `ink-form` / `ink-scroll-view` on purpose (#1952). They are + // declared in the TUI's own manifest, never at the root, which is exactly + // what keeps them out of the candidate set. + assert.deepEqual( + evaluateClient({ + name: "tui", + build: "clients/tui/build", + externals: ["react", "ink"], + rootDependencies: ["react"], + files: [ + { + name: "index.js", + source: banner("../../node_modules/ink-form/lib/Form.js"), + }, + ], + }), + [], + ); + }); + test("passes a clean bundle", () => { assert.deepEqual( evaluateClient({ name: "web", build: "clients/web/build", externals: ["undici"], + rootDependencies: ["undici"], files: clean, }), [],