fix(guard): derive must-not-bundle candidates independently of external

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 `<pkg>-HASH.js` chunk at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 22:02:54 -04:00
commit dbe95e58ed

View file

@ -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 `// <path>` module banners, which covers both shapes an inlining can take: a separate `<pkg>-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 `// <path>` module banners, which covers both shapes an inlining can take: a separate `<pkg>-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.