From e31a20eec77117723b95fa6d15c3aa5edee4b0ac Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 23:00:28 -0400 Subject: [PATCH] test(proxy): isolate proxy env per file; word the guard failure by candidate source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H Signed-off-by: cliffhall --- clients/cli/__tests__/cli.test.ts | 67 +++++++++++-------- .../integration/mcp/node/transport.test.ts | 35 ++++++++++ core/mcp/node/proxyFetch.ts | 12 +++- scripts/verify-bundle-externals.mjs | 14 +++- scripts/verify-bundle-externals.test.mjs | 25 +++++++ 5 files changed, 120 insertions(+), 33 deletions(-) diff --git a/clients/cli/__tests__/cli.test.ts b/clients/cli/__tests__/cli.test.ts index bfb22c03..020f1446 100644 --- a/clients/cli/__tests__/cli.test.ts +++ b/clients/cli/__tests__/cli.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { afterAll, beforeAll, describe, it, expect, vi } from "vitest"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -26,23 +26,42 @@ import { import type { MCPServerConfig } from "@inspector/core/mcp/index.js"; import { createServer, request } from "node:http"; -/** - * Every proxy variable undici's EnvHttpProxyAgent consults. The proxy test - * clears all of them rather than only setting HTTP_PROXY: the agent prefers the - * lowercase forms and honors NO_PROXY dynamically, so an ambient value in the - * developer's or CI's environment could route the request elsewhere, or bypass - * the test's proxy entirely, and fail the assertion for the wrong reason. - */ -const PROXY_ENV_VARS = [ - "HTTP_PROXY", - "http_proxy", - "HTTPS_PROXY", - "https_proxy", - "NO_PROXY", - "no_proxy", -] as const; - describe("CLI Tests", () => { + /** + * Proxy variables are cleared for the WHOLE file, not just the proxy tests. + * + * `createProxyFetch`'s `EnvHttpProxyAgent` is memoized process-wide and captures + * its proxy URLs when it is constructed (only `NO_PROXY` is re-read per + * request). So on a machine with an ambient `HTTP_PROXY`, any earlier test that + * makes a real connection would build that agent against the ambient proxy, and + * a later test setting `HTTP_PROXY` to its own server would be talking to an + * agent that is not listening. Clearing up front means nothing can build the + * singleton before the proxy tests do. + */ + const AMBIENT_PROXY_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", + ] as const; + const ambientProxyEnv: Record = {}; + + beforeAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + ambientProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + }); + + afterAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + const previous = ambientProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } + }); describe("Basic CLI Mode", () => { it("should execute tools/list successfully", async () => { const { command, args } = getTestMcpServerCommand(); @@ -382,11 +401,6 @@ describe("CLI Tests", () => { }); req.pipe(upstream); }); - const savedProxyEnv: Record = {}; - for (const name of PROXY_ENV_VARS) { - savedProxyEnv[name] = process.env[name]; - delete process.env[name]; - } try { await server.start(); await new Promise((r) => proxy.listen(0, "127.0.0.1", () => r())); @@ -407,11 +421,10 @@ describe("CLI Tests", () => { expect(seen.length).toBeGreaterThan(0); expect(seen.every((u) => u.startsWith(server.url))).toBe(true); } finally { - for (const name of PROXY_ENV_VARS) { - const previous = savedProxyEnv[name]; - if (previous === undefined) delete process.env[name]; - else process.env[name] = previous; - } + // Only this test's own variable is cleared here. The ambient ones are + // restored by the file-level afterAll, so the memoized agent can never + // be left bound to this now-closed proxy for a later test. + delete process.env.HTTP_PROXY; await new Promise((r) => proxy.close(() => r())); await server.stop(); } diff --git a/clients/web/src/test/integration/mcp/node/transport.test.ts b/clients/web/src/test/integration/mcp/node/transport.test.ts index c30cec3c..cebf7c2d 100644 --- a/clients/web/src/test/integration/mcp/node/transport.test.ts +++ b/clients/web/src/test/integration/mcp/node/transport.test.ts @@ -28,6 +28,41 @@ import { createServer, request, type Server } from "node:http"; import type { AddressInfo } from "node:net"; describe("Transport", () => { + /** + * Proxy variables are cleared for the WHOLE file, not just the proxy tests. + * + * `createProxyFetch`'s `EnvHttpProxyAgent` is memoized process-wide and captures + * its proxy URLs when it is constructed (only `NO_PROXY` is re-read per + * request). So on a machine with an ambient `HTTP_PROXY`, any earlier test that + * makes a real connection would build that agent against the ambient proxy, and + * a later test setting `HTTP_PROXY` to its own server would be talking to an + * agent that is not listening. Clearing up front means nothing can build the + * singleton before the proxy tests do. + */ + const AMBIENT_PROXY_VARS = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "NO_PROXY", + "no_proxy", + ] as const; + const ambientProxyEnv: Record = {}; + + beforeAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + ambientProxyEnv[name] = process.env[name]; + delete process.env[name]; + } + }); + + afterAll(() => { + for (const name of AMBIENT_PROXY_VARS) { + const previous = ambientProxyEnv[name]; + if (previous === undefined) delete process.env[name]; + else process.env[name] = previous; + } + }); describe("getServerType", () => { it("should return stdio for stdio config", () => { const config: MCPServerConfig = { diff --git a/core/mcp/node/proxyFetch.ts b/core/mcp/node/proxyFetch.ts index 14e8a068..41190be4 100644 --- a/core/mcp/node/proxyFetch.ts +++ b/core/mcp/node/proxyFetch.ts @@ -202,9 +202,15 @@ function toUndiciRequest( * `fetch` slot, so undici is never imported and the built-in `fetch` is used as * before. * - * The env is read once, when this is called; undici's `EnvHttpProxyAgent` then - * owns per-request `NO_PROXY` matching. The agent itself is created lazily on - * the first request so that merely constructing a client costs nothing. + * The env is read once, when this is called, to decide *whether* to proxy at + * all. The agent itself is created lazily on the first request, so merely + * constructing a client costs nothing — and note that `EnvHttpProxyAgent` + * captures the proxy URLs at *its* construction (measured), evaluating only + * `NO_PROXY` per request. That is invisible in production, where the + * environment does not change mid-process, but it means a test that mutates + * `HTTP_PROXY` after any request has already been made is talking to an agent + * that is not listening: the proxy suites isolate these variables for a whole + * file so nothing can build the process-wide agent before they do. * * This belongs at the *bottom* of a client's fetch stack — the Node clients' * `environment.fetch` — because proxying now means substituting the fetch rather diff --git a/scripts/verify-bundle-externals.mjs b/scripts/verify-bundle-externals.mjs index 930bec36..8b2774f8 100644 --- a/scripts/verify-bundle-externals.mjs +++ b/scripts/verify-bundle-externals.mjs @@ -183,9 +183,17 @@ export function evaluateClient({ ]; } - return violations.map( - (v) => - `${name}: "${v.pkg}" is declared external but was inlined (${v.reason}).`, + // Word the failure for where the candidate came from. A root dependency that + // is missing from the `external` list — #2067's own shape — is not "declared + // external", and saying so would send the reader looking for a declaration + // that is precisely what is absent. + const declared = new Set(externals); + return violations.map((v) => + declared.has(v.pkg) + ? `${name}: "${v.pkg}" is declared external but was inlined (${v.reason}).` + : `${name}: "${v.pkg}" is a root runtime dependency, so it must stay ` + + `external, but it was inlined (${v.reason}). Add it to this client's ` + + `tsup \`external\` list.`, ); } diff --git a/scripts/verify-bundle-externals.test.mjs b/scripts/verify-bundle-externals.test.mjs index ed8d5010..36a81590 100644 --- a/scripts/verify-bundle-externals.test.mjs +++ b/scripts/verify-bundle-externals.test.mjs @@ -232,6 +232,31 @@ describe("evaluateClient", () => { ], }); assert.equal(failures.length, 1); + // Worded for where the candidate came from: `undici` is NOT in this + // client's `external` list here — that absence IS the bug — so a message + // saying it "is declared external" would send the reader looking for a + // declaration that is precisely what is missing. + assert.match( + failures[0], + /"undici" is a root runtime dependency, so it must stay external, but it was inlined/, + ); + assert.doesNotMatch(failures[0], /is declared external but was inlined/); + }); + + test("uses the declared-external wording when the package IS in the list", () => { + const failures = evaluateClient({ + name: "web", + build: "clients/web/build", + externals: ["undici"], + rootDependencies: ["undici"], + files: [ + { + name: "index.js", + source: banner("../../node_modules/undici/index.js"), + }, + ], + }); + assert.equal(failures.length, 1); assert.match(failures[0], /"undici" is declared external but was inlined/); });