diff --git a/AGENTS.md b/AGENTS.md index 4a4b142e..335407fc 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. 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 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: - ⚠️ **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/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 795e7e3c..1763644f 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -12,7 +12,16 @@ * - GET fast-path re-check when a concurrent write removed the plaintext */ -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -292,6 +301,37 @@ describe("server.ts supplemental coverage", () => { let target: ServerType; let targetUrl: string; + // Isolate the WHOLE suite from the developer's (or CI's) proxy environment. + // Setting only uppercase HTTP_PROXY in the one proxy test would not be + // enough: undici's EnvHttpProxyAgent prefers lowercase `http_proxy`, and + // honors NO_PROXY dynamically, so an ambient value could route the request + // elsewhere or bypass the proxy and fail the assertion. Suite-level rather + // than per-test because the agent is memoized process-wide once built. + const PROXY_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", + ] as const; + const savedProxyEnv: Partial> = {}; + + beforeAll(() => { + for (const name of PROXY_VARS) { + savedProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + }); + + afterAll(() => { + for (const name of PROXY_VARS) { + const previous = savedProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } + }); + beforeEach(async () => { h = await start(); // A tiny upstream HTTP server we can point /api/fetch at. @@ -379,7 +419,6 @@ describe("server.ts supplemental coverage", () => { typeof proxyAddr === "object" && proxyAddr !== null ? proxyAddr.port : 0; - const previous = process.env.HTTP_PROXY; process.env.HTTP_PROXY = `http://127.0.0.1:${proxyPort}`; try { @@ -391,8 +430,7 @@ describe("server.ts supplemental coverage", () => { expect(res.status).toBe(200); expect(seen).toEqual([`${targetUrl}/plain`]); } finally { - if (previous === undefined) delete process.env.HTTP_PROXY; - else process.env.HTTP_PROXY = previous; + delete process.env.HTTP_PROXY; await new Promise((r) => proxy.close(() => r())); } }); diff --git a/scripts/verify-bundle-externals.mjs b/scripts/verify-bundle-externals.mjs index 2b245e2b..818157f6 100644 --- a/scripts/verify-bundle-externals.mjs +++ b/scripts/verify-bundle-externals.mjs @@ -133,6 +133,40 @@ export function findInlinedExternals({ externals, files }) { return { noBanners: false, violations }; } +/** + * 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. + */ +export function evaluateClient({ name, build, externals, files }) { + if (externals.length === 0) { + return [ + `${name}: no \`external\` entries could be parsed from its tsup config. ` + + `Every bundled client has some, so this means the config no longer matches ` + + `what parseExternals() reads (it keys off the literal text "external: ["). ` + + `Update the parser rather than leaving the guard checking nothing.`, + ]; + } + + const { noBanners, violations } = findInlinedExternals({ externals, files }); + if (noBanners) { + return [ + `${name}: ${build} has no esbuild module banners, so this guard cannot see ` + + `what was inlined. Has minification been enabled for this bundle?`, + ]; + } + + return violations.map( + (v) => + `${name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`, + ); +} + function main() { const failures = []; for (const client of BUNDLED_CLIENTS) { @@ -153,34 +187,31 @@ function main() { name, source: readFileSync(join(buildDir, name), "utf8"), })); - const { noBanners, violations } = findInlinedExternals({ - externals, - files, - }); - if (noBanners) { - failures.push( - `${client.name}: ${client.build} has no esbuild module banners, so this guard ` + - `cannot see what was inlined. Has minification been enabled for this bundle?`, - ); - continue; - } - for (const v of violations) { - failures.push( - `${client.name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`, - ); - } + failures.push( + ...evaluateClient({ + name: client.name, + build: client.build, + externals, + files, + }), + ); } if (failures.length > 0) { console.error("verify:bundle-externals FAILED\n"); for (const f of failures) console.error(` - ${f}`); - console.error( - "\nAn externalized package must resolve from the consumer's install. If it was\n" + - "inlined, esbuild rewrote its specifier to a relative chunk that no user-side\n" + - "install can satisfy — and a CommonJS package inlined into an ESM bundle throws\n" + - '`Dynamic require of "..." is not supported` on first use (#2067).\n' + - "Add the package to that client's tsup `external` list.", - ); + // Only for a real inlining — the other two failure modes (nothing parsed, + // no banners) are about the guard losing sight of the bundle, and "add the + // package to `external`" would be the wrong instruction for either. + if (failures.some((f) => f.includes("was inlined"))) { + console.error( + "\nAn externalized package must resolve from the consumer's install. If it was\n" + + "inlined, esbuild rewrote its specifier to a relative chunk that no user-side\n" + + "install can satisfy — and a CommonJS package inlined into an ESM bundle throws\n" + + '`Dynamic require of "..." is not supported` on first use (#2067).\n' + + "Add the package to that client's tsup `external` list.", + ); + } process.exit(1); } diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs index bac8b53b..7d03ac53 100644 --- a/scripts/verify-bundle-externals.test.mjs +++ b/scripts/verify-bundle-externals.test.mjs @@ -12,6 +12,7 @@ import assert from "node:assert/strict"; import { BUNDLED_CLIENTS, + evaluateClient, parseExternals, findInlinedExternals, } from "./verify-bundle-externals.mjs"; @@ -152,6 +153,71 @@ describe("findInlinedExternals", () => { }); }); +describe("evaluateClient", () => { + const banner = (p) => `// ${p}\nvar x = 1;\n`; + const clean = [ + { name: "index.js", source: banner("../../core/mcp/index.ts") }, + ]; + + test("fails closed when no externals could be parsed", () => { + // A tsup refactor — `external: EXTERNALS`, or a line break after the colon — + // makes parseExternals return nothing. Checking zero packages and reporting + // success would silently disable the guard on a change that looks harmless. + const failures = evaluateClient({ + name: "web", + build: "clients/web/build", + externals: [], + files: clean, + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /no `external` entries could be parsed/); + }); + + test("fails when the bundle carries no module banners", () => { + const failures = evaluateClient({ + name: "cli", + build: "clients/cli/build", + externals: ["undici"], + files: [{ name: "index.js", source: "var a=1;" }], + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /no esbuild module banners/); + }); + + test("reports one failure per inlined package", () => { + const failures = evaluateClient({ + name: "tui", + build: "clients/tui/build", + externals: ["undici", "pino"], + files: [ + { + name: "index.js", + source: + banner("../../node_modules/undici/index.js") + + banner("../../node_modules/pino/pino.js"), + }, + ], + }); + assert.equal(failures.length, 2); + assert.match( + failures[0], + /tui: "undici" is declared external but was inlined/, + ); + }); + + test("passes a clean bundle", () => { + assert.deepEqual( + evaluateClient({ + name: "web", + build: "clients/web/build", + externals: ["undici"], + files: clean, + }), + [], + ); + }); +}); + describe("BUNDLED_CLIENTS", () => { test("covers every tsup-bundled client", () => { // `clients/launcher` is plain tsc and emits no bundle, so it is deliberately