fix(proxy): route through undici's own fetch and keep undici external
Corporate-proxy users could not connect to any remote MCP server from v2. Two independent defects, both reported in #2067. 1. The web and TUI bundles inlined undici. It was declared in the root and `clients/cli` manifests but named in no client's tsup `external` list, and tsup auto-externalizes only what the *nearest* manifest declares — so esbuild inlined 1.05MB of it into `clients/{web,tui}/build/`. undici is CommonJS, so the emitted chunk's `require("assert")` hit esbuild's ESM `__require` shim and threw `Dynamic require of "assert" is not supported`. The rejected `import()` surfaced as "the `undici` package is not available", telling users to install a package that was already present — and could not have helped, because the specifier had been rewritten to a relative chunk at build time. 2. `withProxyDispatcher` injected a *userland* undici `EnvHttpProxyAgent` into Node's *built-in* fetch. That couples two different copies of undici at the dispatcher handler interface, which is not stable across majors: Node 22 embeds undici 6, Node 24 embeds 7, Node 26 embeds 8. A userland undici 8 agent handed a built-in undici 7 handler is rejected with `fetch failed: invalid onRequestStart method`, and the request never leaves the process. So fixing the bundling alone would only have converted the web/TUI failure into the CLI's. Both halves of the pair now come from userland undici — its `fetch` and its `EnvHttpProxyAgent` — which keeps the handler inside one copy and works unchanged from the Node 22.19 engine floor through Node 26. Deleting the wrapper in favor of Node's native `NODE_USE_ENV_PROXY` was not an option: measured, it is unsupported on 22.19 (the floor) and only lands mid-22.x. Because undici's `Response` is a different class from `globalThis.Response`, the proxied fetch re-wraps each response as a genuine global `Response`, streaming preserved. Without that, `res instanceof Response` is false for callers that test it — including the SDK's OAuth error formatter, which would degrade every message to `Raw body: [object Response]`. The proxy now sits at the bottom of the fetch stack (`environment.fetch` for the Node clients, and the `createTransportNode` default for the web backend, its one caller that supplies no `fetchFn`) rather than wrapping the transport's fetch. Proxying means *substituting* the fetch, not decorating one, so wrapping a supplied `fetchFn` would discard it; installing it underneath lets InspectorClient's own wrappers compose over it. A side effect worth having: OAuth discovery and token requests go through `environment.fetch` too, and were never proxied before. Guarding it: - `undici` is added to all three tsup `external` lists and removed from `clients/cli/package.json`, so it is root-declared only, per the placement rule in AGENTS.md. The two installs had already drifted (root 8.5.0 vs cli 8.9.0). - New `npm run verify:bundle-externals` fails if any package a client declares external was inlined into that client's bundle anyway. It reads the *built output*, not the config — those 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. Verified against the real pre-fix artifact, which it flags. - The proxy tests now drive a real forwarding proxy rather than a mock. That matters: defect 2 was a runtime interface mismatch, which no mock can catch because no mock ever dispatches. AGENTS.md's must-not-bundle rule (added by #2082) is extended with why undici slipped past it, and notes the `node -e` trap — probing the bundled chunk with `node -e` falsely succeeds, because it exposes a global `require` that satisfies esbuild's guard. Closes #2067 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
fd1fa06cf5
commit
d90a955164
1 changed files with 758 additions and 133 deletions
173
scripts/verify-bundle-externals.mjs
Normal file
173
scripts/verify-bundle-externals.mjs
Normal file
|
|
@ -0,0 +1,173 @@
|
|||
/**
|
||||
* Guards the mirror of the "must be bundled" rule: a dependency a client
|
||||
* declares **external** must still be external in the built bundle.
|
||||
*
|
||||
* AGENTS.md has a rule for dependencies that MUST be inlined — the
|
||||
* React-rendering ones (#1952) — and nothing at all for the opposite direction,
|
||||
* which is how #2067 shipped. `undici` was declared only in the root and
|
||||
* `clients/cli` manifests, and tsup auto-externalizes just what the *nearest*
|
||||
* manifest declares, so esbuild inlined 1.05 MB of it into the web and TUI
|
||||
* bundles. Inlining a CommonJS package into an ESM bundle is not merely wasteful:
|
||||
* esbuild rewrites `import("undici")` to a relative chunk whose `require("assert")`
|
||||
* hits its `__require` shim and throws `Dynamic require of "assert" is not
|
||||
* supported`. Because the specifier was rewritten at build time, no user-side
|
||||
* install can ever satisfy it — the package is unloadable by construction, and
|
||||
* every proxied connection failed with a message telling users to install a
|
||||
* package that was already there.
|
||||
*
|
||||
* The check is deliberately on the **built output**, not the config: the config
|
||||
* is what a reviewer reads, but the emitted chunk is what ships, and the two
|
||||
* disagreed for four releases. It is also derived from each client's own
|
||||
* `external` list rather than a list maintained here, so a newly externalized
|
||||
* package is covered without editing this file.
|
||||
*
|
||||
* Nothing else catches this class. The unit and integration tests run against
|
||||
* source with a real `undici` on the resolution path; the smokes run the built
|
||||
* tree but never set a proxy env var, so the lazy `import()` never executes.
|
||||
*/
|
||||
|
||||
import { readdirSync, readFileSync, existsSync } from "node:fs";
|
||||
import { dirname, join, resolve } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..");
|
||||
|
||||
/**
|
||||
* Clients that ship a tsup bundle, with the config to read `external` from and
|
||||
* the build directory to inspect. `clients/launcher` is plain `tsc` — it emits
|
||||
* no bundle and inlines nothing — so it has nothing to check.
|
||||
*/
|
||||
export const BUNDLED_CLIENTS = [
|
||||
{
|
||||
name: "web",
|
||||
config: "clients/web/tsup.runner.config.ts",
|
||||
build: "clients/web/build",
|
||||
},
|
||||
{
|
||||
name: "cli",
|
||||
config: "clients/cli/tsup.config.ts",
|
||||
build: "clients/cli/build",
|
||||
},
|
||||
{
|
||||
name: "tui",
|
||||
config: "clients/tui/tsup.config.ts",
|
||||
build: "clients/tui/build",
|
||||
},
|
||||
];
|
||||
|
||||
/**
|
||||
* Extracts the string literals of a tsup config's `external: [...]` array.
|
||||
*
|
||||
* Reading the config as *text* rather than importing it keeps this guard free of
|
||||
* the client's own toolchain (tsup, esbuild, and — for the TUI — a plugin that
|
||||
* reads `ink-form` off disk at module load). Only bare string entries are
|
||||
* collected; a regex entry such as `/^@inspector\/core/` is a `noExternal`
|
||||
* pattern in practice and has no package name to look for.
|
||||
*/
|
||||
export function parseExternals(source) {
|
||||
const start = source.indexOf("external: [");
|
||||
if (start === -1) return [];
|
||||
let depth = 0;
|
||||
let end = -1;
|
||||
for (let i = source.indexOf("[", start); i < source.length; i++) {
|
||||
if (source[i] === "[") depth++;
|
||||
else if (source[i] === "]") {
|
||||
depth--;
|
||||
if (depth === 0) {
|
||||
end = i;
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
if (end === -1) return [];
|
||||
const body = source.slice(start, end);
|
||||
// Strip comments first: the entries are documented at length, and a package
|
||||
// name quoted inside prose would otherwise read as an entry.
|
||||
const withoutComments = body
|
||||
.replace(/\/\*[\s\S]*?\*\//g, "")
|
||||
.replace(/\/\/[^\n]*/g, "");
|
||||
return [...withoutComments.matchAll(/"([^"]+)"|'([^']+)'/g)]
|
||||
.map((m) => m[1] ?? m[2])
|
||||
.filter((name) => name !== "external: [");
|
||||
}
|
||||
|
||||
/**
|
||||
* The chunk esbuild emits for an inlined package is named after the package's
|
||||
* last path segment plus a hash — `undici-HXPKCIY3.js`,
|
||||
* `@scope/name` → `name-XXXXXXXX.js` — and the rewritten specifier in the entry
|
||||
* is the matching relative path. Either one present means the package was
|
||||
* inlined despite being declared external.
|
||||
*/
|
||||
export function findInlinedExternals({ externals, buildFiles, entrySource }) {
|
||||
const violations = [];
|
||||
for (const pkg of externals) {
|
||||
const base = pkg.split("/").pop();
|
||||
const chunk = buildFiles.find((f) =>
|
||||
new RegExp(`^${escapeRegExp(base)}-[A-Za-z0-9_]+\\.js$`).test(f),
|
||||
);
|
||||
if (chunk) {
|
||||
violations.push({ pkg, reason: `emitted chunk ${chunk}` });
|
||||
continue;
|
||||
}
|
||||
if (
|
||||
entrySource.includes(`"./${base}-`) ||
|
||||
entrySource.includes(`'./${base}-`)
|
||||
) {
|
||||
violations.push({
|
||||
pkg,
|
||||
reason: "entry imports a rewritten relative chunk",
|
||||
});
|
||||
}
|
||||
}
|
||||
return violations;
|
||||
}
|
||||
|
||||
function escapeRegExp(s) {
|
||||
return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
}
|
||||
|
||||
function main() {
|
||||
const failures = [];
|
||||
for (const client of BUNDLED_CLIENTS) {
|
||||
const buildDir = join(repoRoot, client.build);
|
||||
const entry = join(buildDir, "index.js");
|
||||
if (!existsSync(entry)) {
|
||||
failures.push(
|
||||
`${client.name}: ${client.build}/index.js is missing — run \`npm run build\` first.`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
const externals = parseExternals(
|
||||
readFileSync(join(repoRoot, client.config), "utf8"),
|
||||
);
|
||||
const violations = findInlinedExternals({
|
||||
externals,
|
||||
buildFiles: readdirSync(buildDir),
|
||||
entrySource: readFileSync(entry, "utf8"),
|
||||
});
|
||||
for (const v of violations) {
|
||||
failures.push(
|
||||
`${client.name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
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.",
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
console.log(
|
||||
`verify:bundle-externals OK — ${BUNDLED_CLIENTS.length} bundles, no externalized package inlined.`,
|
||||
);
|
||||
}
|
||||
|
||||
if (process.argv[1] === fileURLToPath(import.meta.url)) main();
|
||||
Loading…
Reference in a new issue