Addresses review round 7 on #2099 (three suppressed comments).
- Proxy variables are now cleared for the WHOLE of `transport.test.ts` and
`cli.test.ts`, not just around the proxy tests. Measured: undici's
`EnvHttpProxyAgent` captures its proxy URLs when it is CONSTRUCTED and re-reads
only NO_PROXY per request, so with the process-wide memo, any earlier test that
connects on a machine with an ambient proxy would pin the agent to it and a
later test setting HTTP_PROXY would be talking to an agent that is not
listening. Clearing up front means nothing can build the singleton before the
proxy tests do, and restoring only in the file-level afterAll means the memo
cannot be left bound to a closed test proxy.
Both suites verified green under a hostile ambient env
(`HTTP_PROXY`/`http_proxy`/`HTTPS_PROXY` pointed at a dead port). In fairness
this is hardening rather than a fixed failure: the concrete cross-test break
did not reproduce with the current test order — but it depends on test order
and on the developer's environment, which is exactly what this removes.
- The guard's failure message said a package "is declared external but was
inlined" even for a candidate that came only from the root dependencies —
i.e. #2067's own shape, where the missing `external` entry IS the bug, so the
message contradicted the config it was asking the reader to check. It now
words the failure by candidate source, and says to add the entry.
The capture-at-construction behavior is documented on `createProxyFetch`, since
it is the reason the suites are structured this way.
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>
Addresses review round 4 on #2099 — three posted comments and three suppressed
ones, all valid.
- A rebuilt Response reported `url: ""`, `redirected: false`, and
`type: "default"` even after undici followed a redirect: a fetch-contract
regression visible only to proxy users, and one `core/auth/endpointOverrides.ts`
explicitly guards against by declining to rebuild at all. This adapter has no
such choice — the response arrives from the wrong realm — so it now reinstates
the three with `defineProperty`.
- Normalizing a `Request` input dropped every option the Request itself carried
beyond method/headers/body/signal. Most visibly `redirect`: a
`new Request(url, { redirect: "manual" })` started following redirects instead
of surfacing them. All of them are now carried through, with an explicit
`init` still winning per the fetch contract.
Both are covered by tests driving a real redirect through a real proxy, and
both were confirmed to fail without the fix.
- `@modelcontextprotocol/ext-apps` was added only to the CLI's `external` list.
AGENTS.md requires every root-declared runtime dependency in all three, since
which client reaches it is a function of what `core/` imports rather than of
the client's own code. Added to the web runner and TUI lists too.
- The `BUNDLED_CLIENTS` test compared the hard-coded list against a hard-coded
copy of itself, so a new tsup-bundled client would have left it green while
going unchecked. It now discovers clients from disk (a client is tsup-bundled
iff it ships a tsup config) and compares. Verified it fails when a stub client
is added.
- AGENTS.md's project tree still described `core/mcp/node/` as only the stdio
transport factory; it now carries `proxyFetch.ts` and its role.
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>
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>
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>
Addresses the Copilot review on #2099.
- `/api/fetch` used the bare global `fetch`. That route is the browser's only
way out to the network — the web client's `environment.fetch` is
`createRemoteFetch()`, which forwards OAuth discovery and token requests
there — so a corporate-proxy user could connect to a server but never
authorize against it, and the PR's "OAuth is proxied now" claim held for the
CLI and TUI but not for web. It now uses the same proxy-aware base fetch, with
an integration test driving a real forwarding proxy (confirmed failing without
the fix).
- `verify:bundle-externals` only recognized an inlining that produced a
`<pkg>-HASH.js` chunk. A *statically* imported package is folded into
`index.js` and emits no chunk, so it would have passed. Detection now keys off
esbuild's `// <path>` module banners, covering both shapes. Because that
depends on unminified output, a bundle with no banners now fails as such
rather than reporting clean — enabling `minify` cannot silently retire the
check. Tests cover the entry-inlined case, the same-prefix case
(`undici-types` must not read as `undici`), and the no-banner case.
- The pre-push pipeline summaries in AGENTS.md, README.md, and
.github/copilot-instructions.md still described `verify:build-gate → smoke`
and omitted the new gate.
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>
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>
cliffhall