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/); });