Skip to content

feat(core): honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY in the Node transport#1590

Merged
cliffhall merged 1 commit into
1579-wave-1from
v2/1563-node-proxy
Jul 2, 2026
Merged

feat(core): honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY in the Node transport#1590
cliffhall merged 1 commit into
1579-wave-1from
v2/1563-node-proxy

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1563

Summary

Makes the Node transport factory (core/mcp/node/transport.ts) honor the standard HTTPS_PROXY / HTTP_PROXY / NO_PROXY environment variables so the CLI and web backend can reach remote MCP servers from behind corporate proxies with zero new flags. Wave 1 of the PR #1510 decomposition (tracking issue #1579).

Changes

  • core/mcp/node/transport.ts: adds readProxyEnv() (checks HTTPS_PROXY/https_proxy/HTTP_PROXY/http_proxy in undici's precedence) and withProxyDispatcher(), which wraps the base fetch so outbound requests carry an undici EnvHttpProxyAgent dispatcher (covers both schemes and respects NO_PROXY). createTransportNode now wraps its baseFetch through it.
  • Lazy + zero-cost: undici is imported dynamically only when a proxy var is set, so non-proxy callers (TUI, web backend) pay nothing. If a proxy is configured but undici can't be loaded, the wrapper throws an actionable error instead of silently ignoring the env var.
  • Dependency: adds undici@^8.5.0 to the repo-root package.json (shared core/ resolution + web integration tests) and to clients/cli/package.json (the CLI ships proxy support at runtime). undici 8 requires Node >= 22.7.5, which matches the inspector's supported floor.
  • Docs: documents proxy behavior + the undici dependency/Node floor in the CLI README.
  • Tests: extends clients/web/src/test/integration/mcp/node/transport.test.ts with proxy coverage — readProxyEnv precedence/empty-string handling, withProxyDispatcher passthrough when unset, dispatcher injection + lazy singleton reuse, createTransportNode wrapping, and the actionable error when undici is unavailable.

Testing

  • clients/web test:integration: 106 passed (transport suite).
  • clients/web test:coverage (unit + integration): 3242 passed; transport.ts at 100% lines/statements/functions, 93.61% branches — clears the ≥90% per-file gate.
  • validate:web passes (unit flake in useServers.test.tsx under parallel load reproduces on the base branch and passes in isolation; unrelated to this change).

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 1, 2026
@cliffhall cliffhall linked an issue Jul 1, 2026 that may be closed by this pull request
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 37s —— View job


Code Review

  • Read the diff and changed files
  • Analyze core/mcp/node/transport.ts
  • Review tests
  • Review docs & dependency changes
  • Post review feedback

Nice, focused change. The lazy-import design is clean, the actionable error is a good touch, readProxyEnv precedence matches undici, NO_PROXY is respected via EnvHttpProxyAgent, no any, and the tests cover the meaningful branches. Two substantive items and a few nits below.


🔴 1. Node engine floor is misstated — undici 8.5.0 actually requires >= 22.19.0, not >= 22.7.5

The PR body and clients/cli/README.md:87 both claim undici 8 "requires Node >= 22.7.5, which matches the inspector's supported floor." The resolved lockfile says otherwise — in both package-lock.json and clients/cli/package-lock.json:

"node_modules/undici": {
  "version": "8.5.0",
  "engines": { "node": ">=22.19.0" }
}

Meanwhile the repo's declared floor is >= 22.7.5 (root package.json engines). So a user on Node 22.7.5 – 22.18.x satisfies the inspector's stated floor but not undici's — they'll hit an EBADENGINE warning on install and possibly a runtime break. This needs reconciling: either bump the repo engines.node (and the README/PR claim) to >= 22.19.0, or pin an undici version whose floor genuinely matches 22.7.5.

Fix this →

🟠 2. Web backend proxy support relies on module hoisting, not a declared dependency

The PR describes the feature as covering "the CLI and web backend," and indeed the web backend uses this transport (core/mcp/remote/node/server.ts:28createTransportNodewithProxyDispatcher). But undici was added only to the root package.json and clients/cli/package.jsonnot clients/web/package.json. Per AGENTS.md, each client keeps an isolated node_modules; clients/web's lockfile carries only undici-types (from @types/node), not the undici runtime.

It resolves today only because Node walks up the directory tree to the repo-root node_modules. That works in the monorepo dev tree and in the published root package, but it's fragile and inconsistent with the CLI, which correctly self-declares the dep. If a proxy is set and undici can't be found, users get the "undici not available" error even though they configured everything correctly. I'd add undici@^8.5.0 to clients/web/package.json to match the CLI and make the web surface self-contained.

Fix this →


Nits (optional)

  • core/mcp/node/transport.ts:55 — the dispatcher singleton is scoped per withProxyDispatcher() call, so every createTransportNode creates a fresh EnvHttpProxyAgent (each with its own connection pool). Harmless, but a module-level lazy singleton would avoid the redundant agents.
  • core/mcp/node/transport.ts:143 — in the SSE path, a caller-supplied eventSourceInit.fetch bypasses the proxy wrapper (only baseFetch/postFetch are wrapped). That's a defensible "explicit fetch wins" choice, but worth a one-line comment so it's not read as an oversight.
  • Docs — proxy behavior is documented only in the CLI README, but it applies to the web backend too. Since Request logging #2 makes web an explicit surface, a mention in clients/web/README.md would keep the docs consistent (per the AGENTS.md doc-maintenance rule).

None of the nits block; items 1 and 2 are the ones worth addressing before merge.
· v2/1563-node-proxy

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the thorough review. Addressed in the latest push:

🔴 1. Node engine floor — Fixed. You're right: the installed undici@8.5.0 declares engines.node: ">=22.19.0", so the old ">= 22.7.5" claim was wrong and left users on 22.7.5–22.18.x with an EBADENGINE/runtime gap. Reconciled by bumping the declared floor to >= 22.19.0: root package.json engines.node, the clients/cli/README.md note, and the specification/v2_cli_tui_launcher.md reference now all say >= 22.19.0. CI was also raised from Node 20.x → 22.x (the Node-20 job couldn't even load undici 8's CacheStorage — missing webidl.util.markAsUncloneable — which is what was failing the build).

🟠 2. Web backend self-declared dependency — Considered and intentionally not adding undici to clients/web/package.json. The web backend is never installed standalone — it ships inside the root mcp-inspector package (clients/web/build/), and the root package.json declares undici (kept), so Node resolves it from the root install at both dev and publish time. Adding a second physical copy to clients/web/node_modules (v2 clients are isolated, non-workspace) also breaks the "throws an actionable error when undici cannot be loaded" integration test: with two copies, vi.doMock("undici") (keyed on the test file's resolution) no longer intercepts the transport's import("undici") (resolved from the root copy), so the mock misses and the assertion fails. Keeping a single root-level copy avoids that split. I did add a proxy section to clients/web/README.md so the web surface is documented too.

Nits — Added the one-line comment on the SSE path noting that a caller-supplied eventSourceInit.fetch intentionally bypasses the proxy wrapper. Left the per-call dispatcher as-is: a module-level singleton would pin the first EnvHttpProxyAgent for the process lifetime even if proxy env changes between transports; the per-call agent is cheap and only created when a proxy is actually set.

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 1m 15s —— View job


Re-review

  • Verify item 1 fix (Node engine floor → >= 22.19.0)
  • Verify item 2 rationale (web dep intentionally not added)
  • Verify nit fix (SSE eventSourceInit.fetch comment)
  • Post feedback

Thanks for the thorough follow-up — the responses are all sound. Verification results below.

✅ 1. Node engine floor — resolved, with one leftover

The floor is now consistently >= 22.19.0 in package.json:85, clients/cli/README.md:87, and specification/v2_cli_tui_launcher.md:68, and CI moved to Node 22.x. Good.

One thing slipped through, though: the root package-lock.json is stale. The "" (root package) entry still declares the old floor:

// package-lock.json:38-40
"engines": {
  "node": ">=22.7.5"
}

package.json was bumped but the lockfile wasn't regenerated. I confirmed it locally — npm install --package-lock-only rewrites that line to >=22.19.0 (a real one-line drift, not a no-op). Worth committing so the lockfile matches package.json and stays consistent for npm ci. The clients/cli/package-lock.json is already in sync (it declares no engines), so only the root lockfile needs it.

Fix this →

✅ 2. Web dependency — rationale accepted

The reasoning holds: the web backend ships inside the root mcp-inspector package and the root package.json declares undici, so Node resolves it from the root install at both dev and publish time. And the point about a second physical copy defeating vi.doMock("undici") (which keys on the test file's module resolution) is a concrete, correct reason not to duplicate. Adding the clients/web/README.md proxy section closes the docs gap. Good call leaving the dep single-sourced.

✅ 3. SSE nit — addressed

The comment at core/mcp/node/transport.ts:143-144 makes the "explicit eventSourceInit.fetch wins, not re-wrapped for proxying" intent clear. And the per-call dispatcher justification (avoiding a process-lifetime-pinned agent when proxy env changes between transports) is reasonable — agreed on leaving it.


Only item 1's lockfile leftover is worth a commit before merge; everything else LGTM. 👍
· v2/1563-node-proxy

@cliffhall

Copy link
Copy Markdown
Member Author

Good catch on the stale lockfile. Fixed in 7e931ecf: ran npm install --package-lock-only so the root package-lock.json root-package entry now declares engines.node: ">=22.19.0", matching package.json. Confirmed the diff is exactly that one line (clients/cli/package-lock.json declares no engines, so it needed nothing). npm ci will now be consistent.

@claude review

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 26s —— View job


Re-review

  • Verify lockfile fix (7e931ecf) — root package-lock.json engines now >=22.19.0

Verified — the last leftover from the prior review is resolved. ✅

Commit 7e931ecf changes exactly the one line I flagged. The root package ("") entry in package-lock.json now reads:

// package-lock.json:38-40
"engines": {
  "node": ">=22.19.0"
}

which matches package.json:85 (>=22.19.0) and the resolved undici@8.5.0 floor (package-lock.json:3022). The commit diff is precisely -">=22.7.5"+">=22.19.0", nothing else — so npm ci is now internally consistent, and clients/cli/package-lock.json correctly needed no change (it declares no engines).

The Node floor is now consistent across all five touch-points: package.json, root package-lock.json, clients/cli/README.md, specification/v2_cli_tui_launcher.md, and CI (22.x).

Everything from the earlier rounds (engine floor reconciliation, the intentional single-sourced undici dep with its vi.doMock rationale, the SSE eventSourceInit.fetch comment) still holds. No open items remain from my side — LGTM. 👍
· v2/1563-node-proxy

@cliffhall

Copy link
Copy Markdown
Member Author

📋 Smoke test & AGENTS.md audit for this PR: #1563 (comment) (posted on the linked issue #1563).

@cliffhall cliffhall changed the base branch from v2/main to 1579-wave-1 July 2, 2026 16:45
@cliffhall cliffhall merged commit 42fb853 into 1579-wave-1 Jul 2, 2026
1 check passed
@cliffhall cliffhall deleted the v2/1563-node-proxy branch July 2, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core: honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY in the Node transport

1 participant