Add stdio transport hardening and shared CLI transport flags#1784
Add stdio transport hardening and shared CLI transport flags#1784chelojimenez merged 6 commits intomainfrom
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
dosubot
Bot
added
the
size:L
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
dosubot
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an explicit CLI Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/lib/server-config.ts`:
- Around line 295-303: The returned spawn options currently set stderr: "pipe"
unconditionally which leaves the child process blocked once the one-time startup
capture in MCPClientManager (startup capture listener at
MCPClientManager.ts:1276) is removed; change the object returned by the function
in server-config.ts so stderr is conditional rather than always "pipe" (for
example, expose or use an options flag like captureStartupLogs or similar and
set stderr to "pipe" only when that flag is true, otherwise use "inherit"), and
update any callers (e.g., the code that triggers MCPClientManager's startup
capture) to pass that flag so startup capture still works but the long-running
child uses "inherit" to avoid pipe buffer stalls.
In `@sdk/src/mcp-client-manager/MCPClientManager.ts`:
- Around line 1087-1096: When client.connect(transport, { timeout }) fails,
ensure the spawned stdio transport is closed before rethrowing: in the catch
block call stopStderrCapture() once into a local variable (e.g., stderrCapture),
then attempt to close the stdio transport (e.g., await transport.close() or
transport.kill(), wrapped in its own try/catch to avoid masking the original
error), and finally throw this.annotateStdioConnectError(serverId, error,
stderrCapture); keep a single stopStderrCapture() invocation (remove the
duplicate in finally or guard it so it’s a no-op if already called) so stderr is
captured exactly once and the child process is not left running.
- Around line 1244-1250: The current getProcessEnvironment() returns the entire
parent process.env (filtered only for string) which can leak secrets to spawned
MCP server processes; replace this with a curated whitelist pattern (like the
SDK's getDefaultEnvironment/getDefaultProcessEnv) or explicitly merge only
allowed keys (e.g., HOME, PATH, USER, SHELL, TMPDIR, LANG, NODE_ENV, etc.) when
building the child env in the spawn call (the merge currently done with {
...this.getProcessEnvironment(), ...(config.env ?? {}) }); alternatively, if
full inheritance is intended, add an explicit justification comment and document
it in the MCPClientManager API docs, but do not leave broad environment
inheritance unguarded—use the getProcessEnvironment or rename it to reflect
whitelist behavior and ensure it returns only the approved keys.
In `@sdk/tests/MCPClientManager.test.ts`:
- Around line 165-175: The afterEach teardown currently always deletes
process.env[inheritedEnvKey] and process.env[overrideEnvKey], which can clobber
pre-existing environment values; modify the test to capture the prior values
(e.g., const prevInherited = process.env[inheritedEnvKey]; const prevOverride =
process.env[overrideEnvKey]) before tests run (in the top-level scope or
beforeEach) and in afterEach restore them (set back to prev value or delete only
if they were undefined originally) while still calling await
manager.disconnectAllServers(); update references to inheritedEnvKey and
overrideEnvKey and the beforeEach/afterEach hooks in MCPClientManager.test.ts to
implement this save-and-restore behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 146b81d1-7bad-4b33-9ffb-d6976bd0ea54
📒 Files selected for processing (9)
cli/src/lib/server-config.tscli/tests/server-config.test.tsdocs/cli/apps-conformance.mdxdocs/cli/overview.mdxdocs/cli/reference.mdxdocs/cli/server-inspection.mdxdocs/cli/tools-resources-prompts.mdxsdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.test.ts
| return { | ||
| command, | ||
| args: parseCommandArgs(options.commandArgs), | ||
| args: parseCommandArgs(options.args, options.commandArgs), | ||
| env: parseEnvironmentOption(options.env), | ||
| ...(cwd ? { cwd } : {}), | ||
| ...(clientCapabilities ? { clientCapabilities } : {}), | ||
| stderr: "ignore", | ||
| stderr: "pipe", | ||
| timeout: options.timeout, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect stderr capture lifecycle and listener cleanup.
rg -n -C5 'captureStdioStartupStderr|stderrStream\.on\("data"|removeListener|stopped' sdk/src/mcp-client-manager/MCPClientManager.ts
# Inspect all call sites to confirm whether capture cleanup stops reading permanently.
rg -n -C8 'captureStdioStartupStderr\(' sdk/src/mcp-client-manager/MCPClientManager.tsRepository: MCPJam/inspector
Length of output: 2655
🏁 Script executed:
# Check if there are other listeners attached to stderr after startup capture cleanup
rg -n "stderrStream|\.stderr" sdk/src/mcp-client-manager/MCPClientManager.ts | head -30
# Look for any listener reattachment or additional stderr handling
rg -n "\.on\(" sdk/src/mcp-client-manager/MCPClientManager.ts | grep -i stderrRepository: MCPJam/inspector
Length of output: 345
🏁 Script executed:
# Check StdioClientTransport stderr handling
rg -n "stderr" sdk/src/mcp-client-manager/MCPClientManager.ts -B2 -A2 | head -40
# Look for any code that continues reading stderr after connect
rg -n "transport\.stderr" .Repository: MCPJam/inspector
Length of output: 1320
🏁 Script executed:
# Check if stopStderrCapture is called in the success path
rg -n "stopStderrCapture" sdk/src/mcp-client-manager/MCPClientManager.ts -A1 -B1Repository: MCPJam/inspector
Length of output: 284
Unmanaged stderr pipe will eventually stall the child process.
Line 301 sets stderr: "pipe" unconditionally, but the startup capture listener (MCPClientManager.ts:1276) is removed after connect completes—and it's the only consumer in the codebase. Without a drain, the pipe buffer fills and blocks the child's stderr writes, degrading or halting runtime behavior for verbose servers. Either attach a persistent drain listener or switch to "inherit" after startup capture finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/server-config.ts` around lines 295 - 303, The returned spawn
options currently set stderr: "pipe" unconditionally which leaves the child
process blocked once the one-time startup capture in MCPClientManager (startup
capture listener at MCPClientManager.ts:1276) is removed; change the object
returned by the function in server-config.ts so stderr is conditional rather
than always "pipe" (for example, expose or use an options flag like
captureStartupLogs or similar and set stderr to "pipe" only when that flag is
true, otherwise use "inherit"), and update any callers (e.g., the code that
triggers MCPClientManager's startup capture) to pass that flag so startup
capture still works but the long-running child uses "inherit" to avoid pipe
buffer stalls.
| try { | ||
| await client.connect(transport, { timeout }); | ||
| } catch (error) { | ||
| throw this.annotateStdioConnectError( | ||
| serverId, | ||
| error, | ||
| stopStderrCapture() | ||
| ); | ||
| } finally { | ||
| stopStderrCapture(); |
There was a problem hiding this comment.
Close the stdio transport on connect failure.
Unlike the HTTP path, this branch rethrows without closing the spawned transport. A failed initialize/timeout can leave the child process running after resetState() drops the handle.
Suggested fix
try {
await client.connect(transport, { timeout });
} catch (error) {
+ await this.safeCloseTransport(underlying);
throw this.annotateStdioConnectError(
serverId,
error,
stopStderrCapture()
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/mcp-client-manager/MCPClientManager.ts` around lines 1087 - 1096,
When client.connect(transport, { timeout }) fails, ensure the spawned stdio
transport is closed before rethrowing: in the catch block call
stopStderrCapture() once into a local variable (e.g., stderrCapture), then
attempt to close the stdio transport (e.g., await transport.close() or
transport.kill(), wrapped in its own try/catch to avoid masking the original
error), and finally throw this.annotateStdioConnectError(serverId, error,
stderrCapture); keep a single stopStderrCapture() invocation (remove the
duplicate in finally or guard it so it’s a no-op if already called) so stderr is
captured exactly once and the child process is not left running.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f539db8. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sdk/src/mcp-client-manager/MCPClientManager.ts (1)
1092-1101:⚠️ Potential issue | 🟠 MajorClose the spawned stdio transport before rethrowing.
Line 1094 still exits the failure path without closing
underlying, so a timed-out or rejected initialize can leave the child process running afterresetState()drops the handle.Suggested fix
try { await client.connect(transport, { timeout }); } catch (error) { + await this.safeCloseTransport(underlying); const stderrOutput = stderrDrain.getCapturedOutput(); stderrDrain.cleanup(); throw this.annotateStdioConnectError( serverId, error, stderrOutput ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/mcp-client-manager/MCPClientManager.ts` around lines 1092 - 1101, The catch block for await client.connect(...) can leave the spawned stdio process running because the transport's underlying handle isn't closed before rethrowing; update the catch to first close the spawned transport (call and await underlying.close() or underlying?.close() as appropriate) and then cleanup stderrDrain and throw the annotated error via this.annotateStdioConnectError(serverId, error, stderrOutput) so the child process is terminated before resetState() drops the handle (refer to client.connect, stderrDrain.getCapturedOutput, annotateStdioConnectError, and underlying).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/lib/server-config.ts`:
- Around line 74-77: The variadic option ".option(\"--args <arg...>\", ...)" in
server-config.ts fails to collect values that begin with '-' because commander
interprets them as new options; change this to a safer API: replace the variadic
"--args <arg...>" with either a single string flag like "--command-args <args>"
(accept a quoted space-separated string and split it into an array at parse
time) or rename to a non-ambiguous flag "--command-args <arg...>" and mark
"--args" deprecated; update parsing logic where command args are consumed to use
the new symbol (e.g., parse and split the string or use the new "--command-args"
array) so dash-prefixed child args are preserved.
In `@sdk/src/mcp-client-manager/MCPClientManager.ts`:
- Around line 1290-1302: Wrap the original error unconditionally so caller
always retains server context: in the MCPClientManager error handling block
(where stderrOutput, serverId and error are used), always construct a new Error
whose message starts with `Failed to connect to MCP server "${serverId}" via
stdio:` and conditionally append the `Child process stderr:` section only when
`stderrOutput` is non-empty; set the original `error` as the `cause` when it's
an Error (or include it as cause regardless) so callers keep the original stack
while preserving the server id and "via stdio" context even on silent timeouts.
---
Duplicate comments:
In `@sdk/src/mcp-client-manager/MCPClientManager.ts`:
- Around line 1092-1101: The catch block for await client.connect(...) can leave
the spawned stdio process running because the transport's underlying handle
isn't closed before rethrowing; update the catch to first close the spawned
transport (call and await underlying.close() or underlying?.close() as
appropriate) and then cleanup stderrDrain and throw the annotated error via
this.annotateStdioConnectError(serverId, error, stderrOutput) so the child
process is terminated before resetState() drops the handle (refer to
client.connect, stderrDrain.getCapturedOutput, annotateStdioConnectError, and
underlying).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 702e44ab-0bba-40ad-bc3a-f8cd04d006fd
📒 Files selected for processing (5)
cli/src/lib/server-config.tscli/tests/server-config.test.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/src/mcp-client-manager/types.tssdk/tests/MCPClientManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/tests/MCPClientManager.test.ts
- cli/tests/server-config.test.ts
| .option( | ||
| "--args <arg...>", | ||
| "Preferred stdio command arguments. Pass multiple values or repeat the flag.", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the official Commander.js docs, what are the parsing rules for variadic options declared as .option("--args <arg...>"), especially when the child argument list contains tokens starting with -?
💡 Result:
In the official Commander.js documentation, variadic options declared as .option("--args <arg...>") collect multiple option-arguments into an array. The first argument is parsed normally. Subsequent arguments for this option are collected until an argument starting with a dash (-) is encountered, which is treated as a new option rather than part of --args. The special -- terminates option processing entirely, with remaining arguments becoming positional arguments. For child argument lists containing tokens starting with - (e.g., --args foo -bar baz), -bar stops collection for --args (so --args gets ["foo"]), -bar is parsed as an option (error if unknown), and baz becomes a positional argument. This behavior aligns with optional argument parsing rules: arguments starting with - or -- are not consumed as option-arguments.
Citations:
- 1: https://github.com/tj/commander.js/blob/master/docs/options-in-depth.md
- 2: https://github.com/tj/commander.js/
- 3: Add support for variadic options tj/commander.js#1250
--args cannot reliably pass child process arguments that start with dashes.
Commander variadic options cease consuming values upon encountering a token beginning with -, treating it as a new option instead. This breaks common patterns like --args -y convex@latest mcp start, where -y halts collection. Consider retaining --command-args as the primary syntax, or implement raw token consumption beyond the variadic option to preserve complete argument lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/server-config.ts` around lines 74 - 77, The variadic option
".option(\"--args <arg...>\", ...)" in server-config.ts fails to collect values
that begin with '-' because commander interprets them as new options; change
this to a safer API: replace the variadic "--args <arg...>" with either a single
string flag like "--command-args <args>" (accept a quoted space-separated string
and split it into an array at parse time) or rename to a non-ambiguous flag
"--command-args <arg...>" and mark "--args" deprecated; update parsing logic
where command args are consumed to use the new symbol (e.g., parse and split the
string or use the new "--command-args" array) so dash-prefixed child args are
preserved.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/tests/server-config.test.ts (1)
136-147: Assertion incomplete—consider verifying the error message.This test validates the exit code but omits message verification, unlike its siblings. Adding a message check (even partial) would strengthen the test and clarify intent.
💡 Suggested enhancement
assert.throws( () => parseServerConfig({ transport: "http", command: "node", url: "https://example.com/mcp", header: ["X-Test: yes"], }), (error) => error instanceof CliError && - error.exitCode === 2, + error.exitCode === 2 && + error.message.includes("--command"), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/server-config.test.ts` around lines 136 - 147, The test currently asserts that parseServerConfig throws a CliError with exitCode 2 but doesn't check the error message; update the assertion callback for parseServerConfig to also verify the error.message content (for example using includes(...) or a regex) to assert the expected error text is produced; reference the parseServerConfig call and CliError type in the assertion so the test ensures both exitCode and a matching error message are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/tests/server-config.test.ts`:
- Around line 136-147: The test currently asserts that parseServerConfig throws
a CliError with exitCode 2 but doesn't check the error message; update the
assertion callback for parseServerConfig to also verify the error.message
content (for example using includes(...) or a regex) to assert the expected
error text is produced; reference the parseServerConfig call and CliError type
in the assertion so the test ensures both exitCode and a matching error message
are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c4438a1-3585-49fc-bf8b-dc9c5f4976cd
📒 Files selected for processing (1)
cli/tests/server-config.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sdk/src/mcp-client-manager/MCPClientManager.ts (1)
1091-1101:⚠️ Potential issue | 🔴 CriticalTransport remains open on connect failure.
The catch block drains stderr and annotates the error, yet the spawned
underlyingtransport is never closed. Unlike the HTTP path (line 1193), a failed or timed-outclient.connectleaves the child process running afterresetState()discards the handle.Suggested fix
try { await client.connect(transport, { timeout }); } catch (error) { const stderrOutput = stderrDrain.getCapturedOutput(); stderrDrain.cleanup(); + await this.safeCloseTransport(underlying); throw this.annotateStdioConnectError( serverId, error, stderrOutput ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/mcp-client-manager/MCPClientManager.ts` around lines 1091 - 1101, The catch block around client.connect leaves the spawned stdio transport running when connect fails; after capturing and cleaning stderr (stderrDrain.getCapturedOutput() and stderrDrain.cleanup()) ensure the underlying transport/child process is closed before throwing: explicitly close or dispose the provided transport (the same transport passed into client.connect) and/or call the manager's resetState() path used by the HTTP flow, then rethrow the annotated error via this.annotateStdioConnectError(serverId, error, stderrOutput) so the child process is not leaked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@sdk/src/mcp-client-manager/MCPClientManager.ts`:
- Around line 1091-1101: The catch block around client.connect leaves the
spawned stdio transport running when connect fails; after capturing and cleaning
stderr (stderrDrain.getCapturedOutput() and stderrDrain.cleanup()) ensure the
underlying transport/child process is closed before throwing: explicitly close
or dispose the provided transport (the same transport passed into
client.connect) and/or call the manager's resetState() path used by the HTTP
flow, then rethrow the annotated error via
this.annotateStdioConnectError(serverId, error, stderrOutput) so the child
process is not leaked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a004bec3-98a5-459e-8cb9-6268f94e0764
📒 Files selected for processing (4)
cli/src/lib/server-config.tscli/tests/server-config.test.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/src/lib/server-config.ts
1 participant
Summary
--transport,--args,-e/--env, and--cwd--command-argsas a legacy alias while normalizing stdio args and tightening HTTP vs stdio validationstderr, and surface startupstderron connection failurescwd, and startup error reportingTesting
npm --prefix ./cli test -- tests/server-config.test.tsnpm --prefix ./sdk test -- MCPClientManager.test.tsnpm --prefix ./cli run typechecknpm --prefix ./sdk run typechecktools listandget-envagainstnpx @modelcontextprotocol/server-everythingtools listagainstnpx -y convex@latest mcp startNote
Medium Risk
Changes CLI target parsing and SDK stdio connection behavior (env inheritance, cwd, stderr piping/error annotation), which can affect how subprocess-based servers start and how failures are reported.
Overview
Adds explicit transport selection/validation to the CLI via
--transport http|stdio, plus modern stdio flags--args,-e/--env, and--cwd(keeping--command-argsas a legacy alias) and tighter rejection of stdio-only flags on HTTP targets.Hardens SDK stdio connections by inheriting the parent process environment, defaulting stdio
stderrtopipe, continuously draining/capturing startup stderr, and surfacing that stderr in connection errors while ensuring drain cleanup on disconnect/reset.Updates CLI docs and tests to use the new stdio syntax, document env inheritance/overrides, and cover transport validation, args merging, cwd handling, and improved stdio failure reporting.
Reviewed by Cursor Bugbot for commit 05f3b9c. Bugbot is set up for automated code reviews on this repo. Configure here.