fix(proxy): proxy /api/fetch, and detect entry-inlined externals

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>
This commit is contained in:
cliffhall 2026-08-23 21:33:31 -04:00
commit 9a084f49e5
7 changed files with 203 additions and 77 deletions

View file

@ -101,7 +101,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
## Gates and PR hygiene
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
- **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged.
- **Build output is never a gate target.** Lint, format, and typecheck read first-party source only; everything a build writes (`clients/*/build`, `clients/web/dist`, `storybook-static`, `coverage`, `test-servers/build`, `core/**/{build,dist}`, `*.tsbuildinfo`) stays out via each scope's `globalIgnores`, `format` globs, and tsconfig `include`. Gating generated code reports defects in vendored third-party source that nobody can fix, and a rule promotion turns that warning into a `validate` failure (#2043). Flag a PR that adds a build location without ignoring it in the same change, that widens an ignore to silence a finding in first-party code, or that adds a build directory to a tsconfig `include` to make a generated `.d.ts` resolve. Note the coverage guards don't catch this — they assert source is _covered_, not that output is _excluded_.

File diff suppressed because one or more lines are too long

View file

@ -367,12 +367,12 @@ Each client self-validates from its own folder; the root scripts chain them. The
| `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). |
| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. |
| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. |
| `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that client declares `external` was inlined anyway (an emitted `<pkg>-HASH.js` chunk, or a rewritten `./<pkg>-HASH.js` specifier in the entry). `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. |
| `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that client declares `external` was inlined anyway. Detection is via esbuild's `// <path>` module banners, so it covers both shapes — a separate `<pkg>-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run ci`. |
| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. |
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. |
| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. |
| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`.
| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. |
| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → Storybook. A true superset of GitHub CI. |
| `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). |
Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file.

View file

@ -341,6 +341,62 @@ describe("server.ts supplemental coverage", () => {
await stop(h);
});
it("routes the outbound request through HTTP_PROXY (#2067)", async () => {
// /api/fetch 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 here. On the bare global `fetch` a
// corporate-proxy user could connect to a server but never authorize
// against it, and Node's native NODE_USE_ENV_PROXY does not cover them
// (unsupported at the 22.19 engine floor).
const { createServer } = await import("node:http");
const { request } = await import("node:http");
const seen: string[] = [];
const proxy = createServer((req, res) => {
seen.push(req.url ?? "");
const u = new URL(req.url ?? "");
const up = request(
{
host: u.hostname,
port: u.port,
path: u.pathname + u.search,
method: req.method,
headers: req.headers,
},
(r) => {
res.writeHead(r.statusCode ?? 502, r.headers);
r.pipe(res);
},
);
up.on("error", () => {
res.writeHead(502);
res.end();
});
req.pipe(up);
});
await new Promise<void>((r) => proxy.listen(0, "127.0.0.1", () => r()));
const proxyAddr = proxy.address();
const proxyPort =
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 {
const res = await fetch(`${h.baseUrl}/api/fetch`, {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({ url: `${targetUrl}/plain` }),
});
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;
await new Promise<void>((r) => proxy.close(() => r()));
}
});
it("forwards method + headers and returns the response body", async () => {
const res = await fetch(`${h.baseUrl}/api/fetch`, {
method: "POST",

View file

@ -26,6 +26,7 @@ import type { Context, Env, Next } from "hono";
import { streamSSE } from "hono/streaming";
import { watch as chokidarWatch, type FSWatcher } from "chokidar";
import { createTransportNode } from "../../node/transport.js";
import { createProxyFetch } from "../../node/proxyFetch.js";
import type {
RemoteConnectRequest,
RemoteSendRequest,
@ -1054,7 +1055,14 @@ export function createRemoteApp(
}
try {
const res = await fetch(url, {
// Proxy-aware, not the bare global. This 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 here. Left on the global `fetch`, a corporate-proxy user could
// connect to a server but never authorize against it — and Node's native
// `NODE_USE_ENV_PROXY` does not cover them either, being unsupported at
// our 22.19 engine floor (#2067).
const res = await (createProxyFetch() ?? fetch)(url, {
method,
headers: new Headers(headers),
body: reqBody,

View file

@ -92,38 +92,45 @@ export function parseExternals(source) {
}
/**
* 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.
* Every module esbuild inlines is emitted under a `// <path>` banner naming its
* source file, so a package that was bundled despite being declared external
* leaves `// ../../node_modules/<pkg>/…` lines behind. Matching on those covers
* **both** shapes this can take: a separate `<pkg>-HASH.js` chunk (what a
* dynamically `import()`ed CommonJS package produces, which is what #2067 was)
* and a statically-imported package folded straight into `index.js`, which
* emits no chunk at all and would otherwise pass unnoticed.
*
* Reading banners means the guard depends on unminified output. That is true of
* every client bundle today and is asserted rather than assumed: a build with no
* banners at all fails as `noBanners` instead of silently reporting a clean
* result, so turning on `minify` surfaces here rather than quietly retiring the
* check.
*/
export function findInlinedExternals({ externals, buildFiles, entrySource }) {
export function findInlinedExternals({ externals, files }) {
const banners = files.flatMap((file) =>
[...file.source.matchAll(/^\/\/ (\S+)$/gm)].map((m) => ({
file: file.name,
path: m[1],
})),
);
if (banners.length === 0) {
return { noBanners: true, violations: [] };
}
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}-`)
) {
// The trailing slash is load-bearing: without it `undici` matches
// `undici-types`, a @types transitive that is legitimately present.
const needle = `node_modules/${pkg}/`;
const hit = banners.find((b) => b.path.includes(needle));
if (hit) {
violations.push({
pkg,
reason: "entry imports a rewritten relative chunk",
reason: `inlined into ${hit.file} (e.g. ${hit.path})`,
});
}
}
return violations;
}
function escapeRegExp(s) {
return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
return { noBanners: false, violations };
}
function main() {
@ -140,11 +147,23 @@ function main() {
const externals = parseExternals(
readFileSync(join(repoRoot, client.config), "utf8"),
);
const violations = findInlinedExternals({
const files = readdirSync(buildDir)
.filter((f) => f.endsWith(".js"))
.map((name) => ({
name,
source: readFileSync(join(buildDir, name), "utf8"),
}));
const { noBanners, violations } = findInlinedExternals({
externals,
buildFiles: readdirSync(buildDir),
entrySource: readFileSync(entry, "utf8"),
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}).`,

View file

@ -51,61 +51,104 @@ describe("parseExternals", () => {
});
describe("findInlinedExternals", () => {
test("flags an emitted chunk named after the package", () => {
// The exact shape #2067 shipped: `undici-HXPKCIY3.js` beside index.js.
const v = findInlinedExternals({
const banner = (p) => `// ${p}\nvar x = 1;\n`;
test("flags a package inlined as its own chunk", () => {
// The exact shape #2067 shipped: a dynamically import()ed CommonJS package
// emitted as `undici-HXPKCIY3.js` beside index.js.
const { violations } = findInlinedExternals({
externals: ["undici"],
buildFiles: ["index.js", "undici-HXPKCIY3.js"],
entrySource: "",
files: [
{
name: "index.js",
source: banner("../../core/mcp/node/transport.ts"),
},
{
name: "undici-HXPKCIY3.js",
source: banner("../../node_modules/undici/lib/core/symbols.js"),
},
],
});
assert.equal(v.length, 1);
assert.equal(v[0].pkg, "undici");
assert.match(v[0].reason, /undici-HXPKCIY3\.js/);
assert.equal(violations.length, 1);
assert.equal(violations[0].pkg, "undici");
assert.match(violations[0].reason, /undici-HXPKCIY3\.js/);
});
test("flags a rewritten relative specifier in the entry", () => {
// Belt to the chunk-name brace: if esbuild ever names the chunk differently,
// the rewritten import in the entry still gives the inlining away.
const v = findInlinedExternals({
externals: ["undici"],
buildFiles: ["index.js"],
entrySource: 'await import("./undici-ABC123.js")',
test("flags a package inlined straight into the entry, with no chunk", () => {
// A *statically* imported package is folded into index.js and emits no
// chunk at all, so a chunk-name check would miss it entirely. The banner is
// what gives it away.
const { violations } = findInlinedExternals({
externals: ["pino"],
files: [
{
name: "index.js",
source:
banner("../../core/mcp/index.ts") +
banner("../../node_modules/pino/pino.js"),
},
],
});
assert.equal(v.length, 1);
assert.match(v[0].reason, /rewritten relative chunk/);
});
test("matches a scoped package on its last segment", () => {
const v = findInlinedExternals({
externals: ["@napi-rs/keyring"],
buildFiles: ["index.js", "keyring-QQ11.js"],
entrySource: "",
});
assert.equal(v.length, 1);
assert.equal(v[0].pkg, "@napi-rs/keyring");
assert.equal(violations.length, 1);
assert.equal(violations[0].pkg, "pino");
assert.match(violations[0].reason, /index\.js/);
});
test("passes a genuinely external package", () => {
assert.deepEqual(
findInlinedExternals({
externals: ["undici", "pino"],
buildFiles: ["index.js"],
entrySource: 'await import("undici"); import "pino";',
}),
[],
);
const { noBanners, violations } = findInlinedExternals({
externals: ["undici", "pino"],
files: [
{
name: "index.js",
source:
banner("../../core/mcp/node/proxyFetch.ts") +
'await import("undici");\nimport "pino";\n',
},
],
});
assert.equal(noBanners, false);
assert.deepEqual(violations, []);
});
test("does not mistake an unrelated chunk for the package", () => {
// `undici-types` is a @types transitive; a substring match would flag it.
assert.deepEqual(
findInlinedExternals({
externals: ["undici"],
buildFiles: ["index.js", "undici-types-AA22.js"],
entrySource: "",
}),
[],
);
test("does not mistake a same-prefix package for the external", () => {
// `undici-types` is a @types transitive and is legitimately inlined; the
// trailing slash in the needle is what keeps it from reading as `undici`.
const { violations } = findInlinedExternals({
externals: ["undici"],
files: [
{
name: "index.js",
source: banner("../../node_modules/undici-types/index.js"),
},
],
});
assert.deepEqual(violations, []);
});
test("matches a scoped package on its full name", () => {
const { violations } = findInlinedExternals({
externals: ["@napi-rs/keyring"],
files: [
{
name: "index.js",
source: banner("../../node_modules/@napi-rs/keyring/index.js"),
},
],
});
assert.equal(violations.length, 1);
assert.equal(violations[0].pkg, "@napi-rs/keyring");
});
test("reports noBanners rather than a clean pass when banners are absent", () => {
// The guard reads esbuild's `// <path>` module banners, so minified output
// would make every bundle look clean. That must fail loudly instead —
// otherwise enabling `minify` silently retires the check.
const { noBanners, violations } = findInlinedExternals({
externals: ["undici"],
files: [{ name: "index.js", source: "var a=1;var b=2;" }],
});
assert.equal(noBanners, true);
assert.deepEqual(violations, []);
});
});