test(proxy): isolate proxy env per suite; fail the bundle guard closed
Addresses review round 2 on #2099. - The /api/fetch proxy test set only uppercase HTTP_PROXY, so an ambient lowercase `http_proxy` (which undici's EnvHttpProxyAgent prefers) or a NO_PROXY could route the request elsewhere and fail the assertion. All six proxy vars are now saved and cleared for the whole /api/fetch suite and restored in afterAll — suite-level rather than per-test because the agent is memoized process-wide once built. Verified the test still passes under a deliberately hostile ambient env (`http_proxy=http://127.0.0.1:9 NO_PROXY='*'`). - `verify:bundle-externals` reported success when `parseExternals` returned nothing, so an ordinary tsup refactor — hoisting the array to a constant, or a line break after `external:` — would silently disable the guard. It now fails closed: every bundled client has externals, so "none parsed" means the parser lost track of the config. Verified against exactly that refactor. The per-client verdict moved into a pure `evaluateClient()` so all three failure modes are unit-tested, and the failure epilogue is now printed only for a real inlining, since "add the package to `external`" is the wrong instruction for the other two. 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
parent
9a084f49e5
commit
2f83549bc3
4 changed files with 163 additions and 28 deletions
|
|
@ -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. 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 `// <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.
|
||||
|
|
|
|||
|
|
@ -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<Record<string, string | undefined>> = {};
|
||||
|
||||
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<void>((r) => proxy.close(() => r()));
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue