feat(inspector): stdio MCP server transport support - #132
Conversation
Add stdio transport alongside existing HTTP for connecting to MCP servers
that communicate over stdin/stdout.
## Architecture
- ConnectionParams discriminated union: { transport: 'http', url } | { transport: 'stdio', command, args?, env?, cwd? }
- ONE branch point at createTestClient() — all other code is transport-agnostic
- Auto-restart for stdio with exponential backoff (1s, 2s, 4s, max 3 retries)
## Changes
### Types & Transport (Phase 1)
- Add ConnectionParams type to @mcp-apps-kit/testing
- createTestClient() accepts ConnectionParams, branches to StdioClientTransport or StreamableHTTPClientTransport
- Add onTransportClose callback to TestClientOptions
- Add connectionParams field to ConnectionState
### Connection Chain (Phase 2)
- ConnectionManager.connect() accepts ConnectionParams with input validation
- Auto-restart logic: onTransportClose → exponential backoff reconnect (stdio only)
- ConnectionRegistry.createConnection() accepts ConnectionParams
- All callers updated (28+ test files, 4 source files)
### Tool API (Phase 3)
- connect_to_server tool supports both transports via Zod union schema
- Backward compatible: plain { url } still works (defaults to HTTP)
### Dashboard API (Phase 4)
- POST /dashboard/connections accepts ConnectionParams body
- Backward compat: { url } without transport field defaults to HTTP
- Response includes transport type
### Dashboard UI (Phase 5)
- Transport dropdown (HTTP/stdio) in ConnectionBar
- stdio mode: command + args inputs replace URL input
- Advanced Settings toggle: env vars + cwd (stdio only)
- Server history stores transport type, shows stdio: badge
- Selecting stdio history entry populates command/args fields
…nconsistency
- Add connectionGeneration counter to ConnectionManager to prevent
stale restart attempts after explicit disconnect()
- Align label format in test-client.ts to match connection.ts
('stdio: command' with space)
- Add stdio-transport behavioral test suite (18 tests)
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughConnection APIs moved from a positional URL string to a structured ConnectionParams object (transport + fields). Stdio transport support and onTransportClose hooks added, with auto-restart/backoff scaffolding. Dashboard, UI, tools, testing utilities, and many tests updated to accept and normalize the new param shape. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionManager
participant Transport
participant MCPServer
rect rgba(100, 200, 150, 0.5)
note over Client,ConnectionManager: HTTP transport flow
Client->>ConnectionManager: connect({transport:"http", url:"http://..."})
ConnectionManager->>Transport: instantiate StreamableHTTPClientTransport(url)
Transport->>MCPServer: open HTTP stream/request
MCPServer-->>Transport: response / ready
Transport-->>ConnectionManager: onReady()
ConnectionManager-->>Client: connected
end
rect rgba(150, 150, 200, 0.5)
note over Client,ConnectionManager: Stdio transport with auto-restart
Client->>ConnectionManager: connect({transport:"stdio", command:"cmd", args:[]})
ConnectionManager->>Transport: spawn StdioClientTransport(command,args,env)
Transport->>MCPServer: child process spawned
MCPServer-->>Transport: ready / then close
Transport-->>ConnectionManager: onClose()
alt closed unexpectedly
ConnectionManager->>ConnectionManager: handleStdioProcessExit() (backoff & attempts++)
ConnectionManager->>Transport: spawn (reconnect attempt)
Transport->>MCPServer: new child process
MCPServer-->>Transport: ready
else max attempts exceeded
ConnectionManager-->>Client: emit error / abort
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| it("should return empty array when no UI widgets", async () => { | ||
| const client = await manager.connect("http://localhost:3000/mcp"); | ||
| const client = await manager.connect({ transport: "http", url: "http://localhost:3000/mcp" }); |
Code Review: PR #132 - Stdio Transport SupportSummaryThis PR adds stdio transport support to the MCP Inspector, enabling connections to MCP servers via stdin/stdout alongside the existing HTTP transport. The implementation is well-structured with proper type safety and comprehensive test coverage. Verdict: ✅ Approve with Minor Fixes Recommended Strengths
Issues FoundHigh Priority1. Race Condition in Auto-Restart Logic ( The auto-restart mechanism has a potential race condition: this.autoRestartTimer = setTimeout(() => {
if (this.connectionGeneration !== generationAtStart) {
return;
}
this.connect(params, options)
.then(() => {
if (this.connectionGeneration !== generationAtStart) {
void this.disconnect(); // Called after successful reconnect
}
})
}, delay);Issue: If Recommendation: Add a flag to track whether disconnect was requested during reconnect, or use AbortController pattern. 2. Error Handling in Auto-Restart ( .catch(() => {
void this.disconnect(); // Could throw, not caught
});Issue: If Recommendation: Wrap in try-catch or use Medium Priority3. Missing Path Validation ( The Security Consideration: While not a direct vulnerability (stdio processes run in the inspector's security context), invalid paths will cause confusing errors. Consider adding validation or documenting that the inspector should not be exposed to untrusted users. Recommendation: if (params.transport === "stdio" && params.cwd) {
// Validate cwd is a valid directory
if (!fs.existsSync(params.cwd) || !fs.statSync(params.cwd).isDirectory()) {
throw new Error(`Invalid working directory: ${params.cwd}`);
}
}4. Test Coverage Gaps Missing tests for:
Low Priority5. Type Safety Enhancement ( transport.onclose = options.onTransportClose;Issue: The Recommendation: Add explicit type to onTransportClose?: () => void;6. Complex Environment Variable Filtering ( The filter pattern is correct but complex: Object.fromEntries(
Object.entries({ ...process.env, ...params.env }).filter(
(entry): entry is [string, string] => entry[1] !== undefined
)
)Recommendation: Add a comment explaining that Code Quality ObservationsFollows Repository Standards: ✅
Architecture: ✅
Error Messages: ✅
Performance ConsiderationsAuto-Restart Backoff: ✅
Memory Management: ✅
Final RecommendationsBefore merging:
Optional improvements:
Testing Checklist
Great work on this feature! The stdio transport is a valuable addition that's well-implemented. The identified issues are mostly edge cases that are unlikely to cause problems in practice, but addressing them would improve robustness. cc: @Reviewer - please verify CI passes and consider addressing the high-priority items before merge. |
Code Review: PR #132 - stdio MCP Server Transport SupportExecutive SummaryThis PR adds stdio (stdin/stdout) transport support to the MCP Inspector, enabling connection to MCP servers via child process spawning in addition to the existing HTTP transport. The implementation is well-architected, thoroughly tested, and production-ready with strong type safety and backward compatibility. Recommendation: ✅ APPROVE with minor suggestions Strengths1. Excellent Architecture & Design ⭐⭐⭐⭐⭐
2. Comprehensive Test Coverage ⭐⭐⭐⭐⭐
3. Strong Type Safety ⭐⭐⭐⭐⭐
4. Security & Input Validation ⭐⭐⭐⭐
Issues & SuggestionsCritical IssuesNone - No blocking issues found. High Priority1. Potential Command Injection Risk 🔴Location: The stdio transport accepts arbitrary
Current code: transport = new StdioClientTransport({
command: params.command, // Could be "sh -c 'malicious code'"
args: params.args, // Could contain shell metacharacters
env: mergedEnv,
cwd: params.cwd,
stderr: "pipe",
});Recommendations:
Mitigation: The 2. Environment Variable Merging Could Leak SecretsLocation: The code merges user-provided const mergedEnv = params.env
? Object.fromEntries(
Object.entries({ ...process.env, ...params.env }).filter(
(entry): entry is [string, string] => entry[1] !== undefined
)
)
: undefined;Issue: If Recommendations:
Example fix: const env = params.env
? params.env // Use only user-provided env (isolated)
: undefined; // Use default env (or allowlist specific vars)Medium Priority3. Missing Timeout Handling for Stdio ConnectionsLocation: The Recommendation: Add timeout to 4. Auto-restart Counter Not Reset on Successful ReconnectLocation: The Recommendation: this.connect(params, options)
.then(() => {
// Reset attempt counter on successful reconnect
this.autoRestartAttempts = 0; // Add this line
if (this.connectionGeneration !== generationAtStart) {
// ...
}
})5. Inconsistent Display LabelsLocation: Multiple files The stdio connection label format varies:
Recommendation: Standardize on one format (suggest with space for readability). Low Priority (Code Quality)6. Type Assertions Could Be SaferLocation: Uses type assertions 7. Magic NumbersLocation:
Recommendation: Extract to named constants with JSDoc explaining the backoff strategy. 8. Dashboard HTML Path ResolutionLocation: The Performance Considerations
DocumentationStrengths:
Suggestions:
Alignment with Repo Standards✅ Follows AGENTS.md requirements:
✅ Code style:
Final VerdictThis is excellent work that demonstrates strong engineering practices:
The security concerns (command injection, environment variable leakage) are the only blocking items that should be addressed before merge. Once mitigated with documentation and safer defaults, this PR is ready to ship. Score: 8.5/10 (would be 9.5/10 with security hardening) Recommended Actions Before Merge
Great work @gabrypavanello! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/inspector/src/connection.ts`:
- Around line 183-188: The stdio display label in the const label generation
uses "stdio: " (with a space) which is inconsistent with connectionDisplayLabel
in connect.ts that uses "stdio:"; update the label generation in the
params-handling code (the const label that checks params.transport === "http")
to use "stdio:" (no space) when building the string from params.command and
params.args so the format matches connectionDisplayLabel across the codebase.
In `@packages/inspector/src/dashboard/react/hooks/useServerHistory.ts`:
- Around line 20-25: The current matching logic in useServerHistory (where
transport, command and args are compared) uses args.join(" ").toLowerCase(),
which can collide different arrays and throws if persisted args contain
non-strings; change the comparison to perform a strict element-wise array
equality for args and add a type-guard that confirms every element is a string
before doing any toLowerCase/command comparisons. Update the matching code paths
referenced (the args comparison in the blocks around lines shown) to first
verify Array.isArray(entry.args) and entry.args.every(a => typeof a ===
"string"), then compare lengths and each element via normalized equality (e.g.,
compare lowercased strings per element) instead of join, and ensure you apply
the same guard in all places noted (the other blocks around 95-106 and 129-135).
🧹 Nitpick comments (6)
examples/minimal/tests/integration.test.ts (1)
14-15: Consider usingport: 0for random port assignment.The hardcoded port
3004can cause port conflicts when running multiple test suites in parallel. Usingport: 0allows the OS to assign an available port dynamically, and the actual port can be retrieved from the server instance.♻️ Suggested refactor
- const testPort = 3004; - const server = await startTestServer(app, { port: testPort }); + const server = await startTestServer(app, { port: 0 }); + const testPort = server.port;Based on learnings: "Use
port: 0for random port assignment in test servers to avoid port conflicts when running multiple test suites."packages/testing/tests/unit/server/test-client.test.ts (1)
21-26: Consider clarifying test description.The test description says "invalid URL format" but port 99999 is actually an out-of-range port number (valid range is 0-65535), not technically an invalid URL format. The URL itself is syntactically valid.
Consider renaming to better reflect what's being tested:
📝 Suggested clarification
- it("should throw ConnectionError for invalid URL format", async () => { - // Invalid URLs should throw connection errors + it("should throw ConnectionError for out-of-range port", async () => { + // Out-of-range ports (>65535) should throw connection errors await expect( createTestClient({ transport: "http", url: "http://localhost:99999/mcp" }) ).rejects.toThrow(ConnectionError);examples/minimal/tests/greet-v1.test.ts (1)
14-16: Consider usingport: 0for dynamic port assignment.Using a hardcoded port (3001) can cause conflicts when running multiple test suites in parallel. Using
port: 0lets the OS assign an available port, which the test server should expose viaserver.port.Based on learnings: "Use
port: 0for random port assignment in test servers to avoid port conflicts when running multiple test suites".♻️ Suggested refactor for dynamic port assignment
beforeAll(async () => { - const testPort = 3001; - const server = await startTestServer(app, { port: testPort }); + const server = await startTestServer(app, { port: 0 }); await new Promise((resolve) => setTimeout(resolve, 100)); const client = await createTestClient( - { transport: "http", url: `http://localhost:${testPort}/v1/mcp` }, + { transport: "http", url: `http://localhost:${server.port}/v1/mcp` }, { trackHistory: true, } );packages/inspector/tests/dom-events-full-flow.test.ts (1)
33-33: Consider usingport: 0for random port assignment.The hardcoded port 16274 could cause conflicts when running multiple test suites in parallel. Using
port: 0allows the OS to assign an available port automatically.♻️ Suggested change
- const port = 16274; // Use a different port to avoid conflicts + const port = 0; // Let the OS assign an available portNote: You would need to retrieve the actual port from the server after it starts (e.g., via
server.getPort()or similar method if available).Based on learnings: "Use
port: 0for random port assignment in test servers to avoid port conflicts when running multiple test suites"packages/inspector/tests/connection-target-schema.test.ts (1)
44-67: Consider addingafterEachcleanup for the connection manager.While each test creates a fresh
ConnectionManagerinstance inbeforeEach, the connected managers from previous tests may not be explicitly disconnected. Adding anafterEachhook to disconnect would ensure proper resource cleanup and prevent potential leaks.♻️ Suggested addition after line 67
}); + + afterEach(async () => { + try { + await manager.disconnect(); + } catch { + // Ignore disconnect errors during cleanup + } + }); describe("target schema capture", () => {Based on learnings: "Clean up servers in tests to avoid resource leaks and hanging processes"
packages/inspector/src/dashboard/dashboard-server.ts (1)
19-19: Use a Zod v4 schema for ConnectionParams validation (incl. stdio args/env types).The current manual checks allow non-string
args/envvalues through and bypass the repo’s Zod-based validation requirement. A schema also centralizes legacy{ url }normalization. (You can still map Zod errors to your existing messages if you need parity.)♻️ Suggested refactor (Zod parsing & normalization)
-import type { ConnectionParams } from "@mcp-apps-kit/testing"; +import type { ConnectionParams } from "@mcp-apps-kit/testing"; +import { z } from "zod"; @@ +const urlSchema = z + .string() + .trim() + .min(1, "Missing url") + .refine((value) => { + try { + const parsed = new URL(value); + return ["http:", "https:", "ws:", "wss:"].includes(parsed.protocol); + } catch { + return false; + } + }, "Unsupported URL protocol. Use http, https, ws, or wss."); + +const connectionParamsSchema = z.union([ + z.object({ + transport: z.literal("stdio"), + command: z.string().trim().min(1, "Missing or empty command for stdio transport"), + args: z.array(z.string()).optional(), + env: z.record(z.string()).optional(), + cwd: z.string().optional(), + }), + z.object({ transport: z.literal("http"), url: urlSchema }), + z.object({ url: urlSchema }).transform((value) => ({ + transport: "http", + url: value.url, + })), +]); @@ - // Normalize to ConnectionParams (backward compat: { url } → { transport: "http", url }) - let params: ConnectionParams; - const transport = (body.transport as string | undefined) ?? (body.url ? "http" : undefined); - - if (transport === "stdio") { - // Validate stdio params - const command = body.command; - if (typeof command !== "string" || command.trim().length === 0) { - res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Missing or empty command for stdio transport" })); - return true; - } - params = { - transport: "stdio", - command: command.trim(), - ...(Array.isArray(body.args) ? { args: body.args as string[] } : {}), - ...(body.env && typeof body.env === "object" - ? { env: body.env as Record<string, string> } - : {}), - ...(typeof body.cwd === "string" ? { cwd: body.cwd } : {}), - }; - } else if (transport === "http") { - // Validate HTTP/WS URL - const urlStr = body.url; - if (typeof urlStr !== "string" || urlStr.trim().length === 0) { - res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Missing url" })); - return true; - } - try { - const parsedUrl = new URL(urlStr); - const allowedProtocols = new Set(["http:", "https:", "ws:", "wss:"]); - if (!allowedProtocols.has(parsedUrl.protocol)) { - res.writeHead(400, { "Content-Type": "application/json" }); - res.end( - JSON.stringify({ - error: "Unsupported URL protocol. Use http, https, ws, or wss.", - }) - ); - return true; - } - } catch { - res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Invalid URL format" })); - return true; - } - params = { transport: "http", url: urlStr }; - } else { - res.writeHead(400, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ error: "Missing transport type or url" })); - return true; - } + // Normalize + validate via Zod (backward compat: { url } → { transport: "http", url }) + const parsed = connectionParamsSchema.safeParse(body); + if (!parsed.success) { + const message = parsed.error.issues[0]?.message ?? "Invalid connection parameters"; + res.writeHead(400, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: message })); + return true; + } + const params = parsed.data as ConnectionParams;As per coding guidelines: Always use Zod v4 for schema/validation; do not use Zod v3 assumptions.
Also applies to: 167-229
| /** Transport type — defaults to HTTP when absent (backward compat). */ | ||
| transport?: "http" | "stdio"; | ||
| /** stdio command (only when transport === "stdio"). */ | ||
| command?: string; | ||
| /** stdio args (only when transport === "stdio"). */ | ||
| args?: string[]; |
There was a problem hiding this comment.
Avoid stdio args collisions and guard non-string args during matching.
join(" ") can treat different arg arrays as identical (e.g., ["a b"] vs ["a","b"]), and toLowerCase() will throw if a persisted history entry contains non-string args. A strict array comparison plus a type guard avoids both issues.
🔧 Suggested fix
+const argsEqual = (left?: string[], right?: string[]): boolean => {
+ if (!left && !right) return true;
+ if (!left || !right) return false;
+ return left.length === right.length && left.every((value, index) => value === right[index]);
+};
@@
- const filtered =
- entry.transport === "stdio"
- ? prev.filter(
- (e) =>
- !(
- e.transport === "stdio" &&
- e.command === entry.command &&
- (e.args?.join(" ") ?? "") === (entry.args?.join(" ") ?? "")
- )
- )
- : prev.filter((e) => e.url !== entry.url);
+ const filtered =
+ entry.transport === "stdio"
+ ? prev.filter(
+ (e) =>
+ !(
+ e.transport === "stdio" &&
+ e.command === entry.command &&
+ argsEqual(e.args, entry.args)
+ )
+ )
+ : prev.filter((e) => e.url !== entry.url);
@@
- if (entry.args?.some((a) => a.toLowerCase().includes(lowerFilter))) return true;
+ if (entry.args?.some((a) => typeof a === "string" && a.toLowerCase().includes(lowerFilter)))
+ return true;Also applies to: 95-106, 129-135
🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/hooks/useServerHistory.ts` around
lines 20 - 25, The current matching logic in useServerHistory (where transport,
command and args are compared) uses args.join(" ").toLowerCase(), which can
collide different arrays and throws if persisted args contain non-strings;
change the comparison to perform a strict element-wise array equality for args
and add a type-guard that confirms every element is a string before doing any
toLowerCase/command comparisons. Update the matching code paths referenced (the
args comparison in the blocks around lines shown) to first verify
Array.isArray(entry.args) and entry.args.every(a => typeof a === "string"), then
compare lengths and each element via normalized equality (e.g., compare
lowercased strings per element) instead of join, and ensure you apply the same
guard in all places noted (the other blocks around 95-106 and 129-135).
Add mocked unit tests covering stdio/HTTP transport creation, env merging, cwd forwarding, onTransportClose wiring, connection error labels, and client operations (callTool, history, disconnect). Branches coverage: 49.73% → 52.41% (above 50% threshold).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/inspector/src/tools/connect.ts`:
- Around line 115-118: The fallback to an empty string for url in the HTTP
branch can mask missing-url bugs; update the branch in connect.ts (the code that
currently does const url = "url" in input ? input.url : "" and returns {
transport: "http", url }) to defensively ensure url exists: either assert the
narrowed type (e.g., use a non-null assertion on input.url) or explicitly throw
an error if "url" is not present, so createConnection receives a guaranteed
valid URL rather than an empty string. Ensure the change targets the HTTP
transport branch where transport is set to "http" and affects the object
returned to createConnection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/testing/tests/unit/server/test-client.test.ts`:
- Around line 268-276: The test currently asserts the tool call result with
plain expect calls; update it to use the fluent tool assertion API by replacing
the assertions after calling client.callTool("test", {}) with
expectToolResult(result) and its fluent checks (e.g., .toBeDefined(),
.toHaveContent([]) or equivalent on the expectToolResult chain). Locate the test
around createTestClient(...) and client.callTool(...) and swap the plain
expect(result) / expect(result.content) assertions to the expectToolResult(...)
fluent assertions.
| it("should call tools", async () => { | ||
| const client = await createTestClient({ | ||
| transport: "http", | ||
| url: "http://localhost:3000/mcp", | ||
| }); | ||
| const result = await client.callTool("test", {}); | ||
| expect(result).toBeDefined(); | ||
| expect(result.content).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Use expectToolResult() for tool execution assertions.
The tool call result is validated with plain expect, but the test guideline requires the fluent expectToolResult() API for tool results.
As per coding guidelines, "Use the fluent assertion API with expectToolResult() for validating tool execution results".
🤖 Prompt for AI Agents
In `@packages/testing/tests/unit/server/test-client.test.ts` around lines 268 -
276, The test currently asserts the tool call result with plain expect calls;
update it to use the fluent tool assertion API by replacing the assertions after
calling client.callTool("test", {}) with expectToolResult(result) and its fluent
checks (e.g., .toBeDefined(), .toHaveContent([]) or equivalent on the
expectToolResult chain). Locate the test around createTestClient(...) and
client.callTool(...) and swap the plain expect(result) / expect(result.content)
assertions to the expectToolResult(...) fluent assertions.
- Throw error for missing HTTP URL instead of falling back to empty
string (connect.ts)
- Use element-wise args comparison in server history dedup instead of
join(' ') which can collide different arrays (useServerHistory.ts)
- Reset autoRestartAttempts on successful reconnect so subsequent
crashes get full retry budget (connection.ts)
- Wrap disconnect() in auto-restart catch with .catch() to prevent
swallowed errors (connection.ts)
- Standardize stdio label format with space after colon (connect.ts)
Code Review: stdio MCP Server Transport SupportI've completed a comprehensive review of PR #132. This is an excellent implementation that adds stdio transport support to the MCP Inspector with strong architecture, robust error handling, and comprehensive test coverage. ✅ StrengthsArchitecture & Design
Error Handling
Type Safety
Test Coverage
Frontend Implementation
🔍 Areas for Improvement1. Security: Command Injection Prevention
|
- Replace inline collapsible with floating Popover component - Settings gear button on right side of connection bar - Popover: dark bg, shadow, z-1000, dismiss on click-outside/Escape - Reusable Popover accepts panelStyle for future settings - Button order: connect/disconnect → settings → close - No header layout shift when settings are open
Pull Request Review: stdio MCP Server Transport SupportSummaryThis PR adds stdio transport support to the MCP Inspector, enabling connection to MCP servers via stdin/stdout in addition to the existing HTTP transport. The implementation is well-architected with a discriminated union pattern, comprehensive test coverage (18 behavioral tests + existing suite), and thoughtful auto-restart logic for stdio connections. Code Quality Assessment: ⭐⭐⭐⭐⭐ ExcellentStrengths
Areas of ExcellenceAuto-Restart Implementation (connection.ts:439-494)The auto-restart logic is exceptionally well-designed:
Transport Abstraction (test-client.ts:44-62)Clean transport initialization with proper environment variable merging. Tool Schema Design (connect.ts:64-68)The discriminated union schema is perfectly structured with backward compatibility. Potential Issues and Concerns1. Security: Environment Variable Injection
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/inspector/src/dashboard/react/components/ConnectionBar.tsx (2)
463-503:⚠️ Potential issue | 🟡 MinorGuard history-triggered connects while
isCreatingis true.The dropdown-triggered paths in
handleKeyDown(Enter with selection) andhandleSelectHistorycallonCreateConnectiondirectly without checkingisCreating, allowing duplicate requests when a user selects history while a connection is already being created. The mainhandleCreatepath properly gates onisCreating, but these two paths bypass that check. Add guards and includeisCreatingin the dependency arrays.🔧 Suggested fix
const handleKeyDown = useCallback( (e: React.KeyboardEvent) => { if (e.key === "Enter" && showDropdown && hoveredIndex >= 0) { e.preventDefault(); + if (isCreating) return; const entry = filteredHistory[hoveredIndex]; if (entry) { const params = applyHistoryEntry(entry); setShowDropdown(false); void onCreateConnection(params); } } else if (e.key === "Enter") { void handleCreate(); } else if (e.key === "Escape") { setShowDropdown(false); inputRef.current?.blur(); } else if (e.key === "ArrowDown" && showDropdown) { e.preventDefault(); setHoveredIndex((prev) => Math.min(prev + 1, filteredHistory.length - 1)); } else if (e.key === "ArrowUp" && showDropdown) { e.preventDefault(); setHoveredIndex((prev) => Math.max(prev - 1, 0)); } }, - [handleCreate, showDropdown, hoveredIndex, filteredHistory, onCreateConnection, applyHistoryEntry] + [ + handleCreate, + showDropdown, + hoveredIndex, + filteredHistory, + onCreateConnection, + applyHistoryEntry, + isCreating, + ] ); const handleSelectHistory = useCallback( (entry: ServerHistoryEntry) => { + if (isCreating) return; const params = applyHistoryEntry(entry); setShowDropdown(false); void onCreateConnection(params); }, - [onCreateConnection, applyHistoryEntry] + [onCreateConnection, applyHistoryEntry, isCreating] );
363-569:⚠️ Potential issue | 🟡 MinorUse a timeout ref to prevent blur from closing dropdown when switching between stdio inputs.
In stdio mode, when transitioning between the command and args inputs, the blur timeout from the first input can fire after the second is focused, unexpectedly closing the dropdown. Track the timeout with a ref and clear any pending timeout on focus.
🔧 Suggested fix
const inputRef = useRef<HTMLInputElement>(null); const containerRef = useRef<HTMLDivElement>(null); const settingsButtonRef = useRef<HTMLButtonElement>(null); + const blurTimeoutRef = useRef<number | null>(null); + + const handleInputFocus = useCallback(() => { + if (blurTimeoutRef.current !== null) { + window.clearTimeout(blurTimeoutRef.current); + blurTimeoutRef.current = null; + } + setIsFocused(true); + }, []); + + const handleInputBlur = useCallback(() => { + if (blurTimeoutRef.current !== null) { + window.clearTimeout(blurTimeoutRef.current); + } + blurTimeoutRef.current = window.setTimeout(() => setIsFocused(false), 200); + }, []); + + useEffect(() => { + return () => { + if (blurTimeoutRef.current !== null) { + window.clearTimeout(blurTimeoutRef.current); + } + }; + }, []);- onFocus={() => setIsFocused(true)} - onBlur={() => setTimeout(() => setIsFocused(false), 200)} + onFocus={handleInputFocus} + onBlur={handleInputBlur}- onFocus={() => setIsFocused(true)} - onBlur={() => setTimeout(() => setIsFocused(false), 200)} + onFocus={handleInputFocus} + onBlur={handleInputBlur}- onFocus={() => setIsFocused(true)} - onBlur={() => setTimeout(() => setIsFocused(false), 200)} + onFocus={handleInputFocus} + onBlur={handleInputBlur}
🧹 Nitpick comments (1)
packages/inspector/src/dashboard/react/components/ConnectionBar.tsx (1)
13-13: Consider relocatingConnectionParamsto a shared package instead of@mcp-apps-kit/testing.Pulling a production UI type from a testing package creates an upward dependency and risks circular or dev-only coupling. Prefer moving the shared type into a dedicated shared package and importing it from there.
As per coding guidelines: "packages/**/*.{ts,tsx}: Avoid cross-package circular dependencies; move shared types/utilities down or into a dedicated package instead of importing up".
…sistency, UI guards - Standardize stdio display label to 'stdio:command args' (no space after colon) across connection.ts, connect.ts, test-client.ts and all tests - Hoist argsEqual() to module scope in useServerHistory.ts for proper element-wise array comparison (prevents join collision) - Add type guard for non-string args in history filter - Add isCreating guard on handleKeyDown and handleSelectHistory in ConnectionBar.tsx to prevent duplicate connection requests - Add JSDoc security warning on ConnectionParams.env about process.env merge - Add inline SECURITY NOTE in test-client.ts env merging code
Pull Request Review: stdio MCP Server Transport SupportOverviewThis PR adds stdio transport support to the MCP Inspector, allowing connections to MCP servers via stdin/stdout in addition to HTTP. The implementation is well-structured with comprehensive tests and follows the repository's strict TypeScript and architectural standards. ✅ StrengthsArchitecture & Design
Code Quality
Security & Best Practices
🔍 Issues & Concerns1. Security: Environment Variable Merging (MEDIUM PRIORITY)Location: Issue: The current implementation merges user-provided env vars with the entire Risk:
Recommendation: // Option 1: Explicit allowlist approach
const SAFE_ENV_VARS = ['PATH', 'HOME', 'USER', 'LANG'];
const safeParentEnv = Object.fromEntries(
Object.entries(process.env).filter(([key]) => SAFE_ENV_VARS.includes(key))
);
const mergedEnv = params.env
? { ...safeParentEnv, ...params.env }
: undefined;Consider adding a 2. Error Handling: Silent Reconnect Failures (MEDIUM PRIORITY)Location: Issue: Auto-restart failures are silently swallowed without notifying the user or emitting events. Impact: Users won't know why their stdio connection disappeared. Recommendation:
3. Type Safety: Missing Zod Validation for Dashboard POST (LOW PRIORITY)Location: The dashboard POST endpoint parses JSON body manually but doesn't validate it with Zod before passing to 4. UI: Environment Variables Input Parsing (LOW PRIORITY)Location: Issues:
Recommendation: Consider using a more robust parser or document the expected format clearly in the UI placeholder text. 5. Race Condition: Auto-restart Generation Counter (LOW PRIORITY)Location: The generation counter pattern is good, but there's a subtle race between disconnect and timer firing. Current mitigation with 📋 Test CoverageExcellent Coverage
Missing Tests (Suggested for follow-up)
🔐 Security ReviewApproved Patterns
Concerns
Recommendation: Document security model for stdio connections:
📝 DocumentationPresent
Missing (Suggested for follow-up)
✨ Best Practices Followed
🚀 RecommendationsMust Address (Before Merge)
Should Fix (Follow-up PR)
Nice to Have
📊 Summary
🎯 Final VerdictAPPROVE with minor security documentation required This is a well-engineered feature with excellent architecture and comprehensive testing. The discriminated union pattern and transport-agnostic design are textbook examples of good TypeScript practices. The main concern is the environment variable merging behavior in
The code quality is excellent and follows all repository standards (strict TypeScript, Zod v4, no Estimated Risk: MEDIUM → LOW (after addressing env security) Files Reviewed: 54 changed files (2037 additions, 377 deletions)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/inspector/src/connection.ts`:
- Around line 459-484: The auto-restart aborts because connect() calls
disconnect() which increments connectionGeneration and mismatches
generationAtStart; fix by adding a way to suppress bumping connectionGeneration
when disconnect() is invoked as part of an internal reconnect flow: update
disconnect() to accept an options flag (e.g. preserveGeneration or
suppressGenerationIncrement) and change connect() (and handleStdioProcessExit()
if it triggers reconnect) to call disconnect({preserveGeneration: true}) during
the auto-restart path so connectionGeneration remains equal to generationAtStart
and the post-connect generation check in the autoRestartTimer callback will not
incorrectly abort; ensure autoRestartAttempts reset logic remains unchanged.
🧹 Nitpick comments (1)
packages/inspector/src/dashboard/react/components/ConnectionBar.tsx (1)
409-426: Consider preserving quoted stdio args.
Whitespace splitting drops quoted/escaped args (e.g.,--name "a b"). If you expect such cases, a minimal quoted-arg parser keeps intent.♻️ Optional tweak
- ...(stdioArgs.trim() ? { args: stdioArgs.trim().split(/\s+/) } : {}), + ...(stdioArgs.trim() ? { args: parseQuotedArgs(stdioArgs.trim()) } : {}),function parseQuotedArgs(raw: string): string[] { const matches = raw.match(/(?:[^\s"]+|"[^"]*")+/g); return matches ? matches.map((arg) => arg.replace(/^"|"$/g, "")) : []; }
| const generationAtStart = this.connectionGeneration; | ||
|
|
||
| this.autoRestartTimer = setTimeout(() => { | ||
| this.autoRestartTimer = null; | ||
|
|
||
| // Abort if disconnect() was called while we were waiting | ||
| if (this.connectionGeneration !== generationAtStart) { | ||
| if (this.debug) { | ||
| console.log(`[inspector] Auto-restart aborted: disconnect called during backoff`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| this.connect(params, options) | ||
| .then(() => { | ||
| // Abort if disconnect() was called while connect() was in-flight | ||
| if (this.connectionGeneration !== generationAtStart) { | ||
| if (this.debug) { | ||
| console.log(`[inspector] Auto-restart aborted: disconnect called during reconnect`); | ||
| } | ||
| void this.disconnect(); | ||
| return; | ||
| } | ||
| // Reset attempt counter on successful reconnect | ||
| this.autoRestartAttempts = 0; | ||
| }) |
There was a problem hiding this comment.
Auto-restart will always abort after a successful reconnect.
When stdio exits, handleStdioProcessExit() calls connect() while state.connected is still true. connect() then calls disconnect(), which bumps connectionGeneration; the subsequent generation check treats that as a user disconnect and immediately disconnects, so restarts never stick.
🛠️ Suggested fix (avoid generation bump during auto-restart)
this.autoRestartTimer = setTimeout(() => {
this.autoRestartTimer = null;
// Abort if disconnect() was called while we were waiting
if (this.connectionGeneration !== generationAtStart) {
if (this.debug) {
console.log(`[inspector] Auto-restart aborted: disconnect called during backoff`);
}
return;
}
+ const previousClient = this.state.client;
+ this.state.connected = false;
+ this.state.client = null;
+ void previousClient?.disconnect().catch(() => {
+ /* best-effort cleanup */
+ });
+
this.connect(params, options)
.then(() => {
// Abort if disconnect() was called while connect() was in-flight
if (this.connectionGeneration !== generationAtStart) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const generationAtStart = this.connectionGeneration; | |
| this.autoRestartTimer = setTimeout(() => { | |
| this.autoRestartTimer = null; | |
| // Abort if disconnect() was called while we were waiting | |
| if (this.connectionGeneration !== generationAtStart) { | |
| if (this.debug) { | |
| console.log(`[inspector] Auto-restart aborted: disconnect called during backoff`); | |
| } | |
| return; | |
| } | |
| this.connect(params, options) | |
| .then(() => { | |
| // Abort if disconnect() was called while connect() was in-flight | |
| if (this.connectionGeneration !== generationAtStart) { | |
| if (this.debug) { | |
| console.log(`[inspector] Auto-restart aborted: disconnect called during reconnect`); | |
| } | |
| void this.disconnect(); | |
| return; | |
| } | |
| // Reset attempt counter on successful reconnect | |
| this.autoRestartAttempts = 0; | |
| }) | |
| const generationAtStart = this.connectionGeneration; | |
| this.autoRestartTimer = setTimeout(() => { | |
| this.autoRestartTimer = null; | |
| // Abort if disconnect() was called while we were waiting | |
| if (this.connectionGeneration !== generationAtStart) { | |
| if (this.debug) { | |
| console.log(`[inspector] Auto-restart aborted: disconnect called during backoff`); | |
| } | |
| return; | |
| } | |
| const previousClient = this.state.client; | |
| this.state.connected = false; | |
| this.state.client = null; | |
| void previousClient?.disconnect().catch(() => { | |
| /* best-effort cleanup */ | |
| }); | |
| this.connect(params, options) | |
| .then(() => { | |
| // Abort if disconnect() was called while connect() was in-flight | |
| if (this.connectionGeneration !== generationAtStart) { | |
| if (this.debug) { | |
| console.log(`[inspector] Auto-restart aborted: disconnect called during reconnect`); | |
| } | |
| void this.disconnect(); | |
| return; | |
| } | |
| // Reset attempt counter on successful reconnect | |
| this.autoRestartAttempts = 0; | |
| }) |
🤖 Prompt for AI Agents
In `@packages/inspector/src/connection.ts` around lines 459 - 484, The
auto-restart aborts because connect() calls disconnect() which increments
connectionGeneration and mismatches generationAtStart; fix by adding a way to
suppress bumping connectionGeneration when disconnect() is invoked as part of an
internal reconnect flow: update disconnect() to accept an options flag (e.g.
preserveGeneration or suppressGenerationIncrement) and change connect() (and
handleStdioProcessExit() if it triggers reconnect) to call
disconnect({preserveGeneration: true}) during the auto-restart path so
connectionGeneration remains equal to generationAtStart and the post-connect
generation check in the autoRestartTimer callback will not incorrectly abort;
ensure autoRestartAttempts reset logic remains unchanged.
Replace full process.env merge with explicit safe-var allowlist (PATH, HOME, LANG, NODE_ENV, XDG dirs, etc.) when spawning stdio child processes. Sensitive variables (API keys, credentials, tokens) are never inherited automatically. Add inheritEnv flag to ConnectionParams for explicit control: - When env is provided: only safe parent vars + user overrides (default) - When inheritEnv=false: only safe parent vars - When neither env nor inheritEnv set: Node.js default (full process.env) Addresses PR #132 security review comment on env var leakage.
Code Review: stdio MCP Transport SupportSummaryThis PR adds stdio transport support to the MCP Inspector, enabling connections to MCP servers via stdin/stdout in addition to HTTP. The implementation is well-architected with strong type safety, comprehensive testing, and thoughtful error handling. ✅ Strengths1. Excellent Architecture & Type Safety
2. Security-First Design ⭐The environment variable filtering in const SAFE_ENV_VARS = new Set([
"PATH", "HOME", "LANG", "NODE_ENV", ...
]);
const safeParentEnv = Object.fromEntries(
Object.entries(process.env).filter(
(entry): entry is [string, string] =>
entry[1] !== undefined && SAFE_ENV_VARS.has(entry[0])
)
);This prevents accidental credential leakage to child processes. Clear documentation explains the security model. 3. Robust Auto-Restart Logic
4. Comprehensive Test Coverage
5. User Experience
🔍 Code Quality ObservationsPositive
Areas for Consideration1. Environment Variable Inheritance Logic (Minor Complexity)
const mergedEnv = params.env
? { ...safeParentEnv, ...params.env }
: params.inheritEnv === false
? safeParentEnv
: undefined;The ternary logic here is correct but dense. Consider extracting to a named function for clarity: function buildChildEnv(params: StdioParams, safeParentEnv: Record<string, string>) {
if (params.env) {
// User provided explicit env: merge with safe parent vars
return { ...safeParentEnv, ...params.env };
}
if (params.inheritEnv === false) {
// Explicitly disabled: only safe parent vars
return safeParentEnv;
}
// Default Node.js behavior: inherit full process.env
return undefined;
}Impact: Low - current code works correctly, just harder to parse at a glance. 2. Race Condition Edge Case in Auto-Restart (Theoretical)
The generation check happens at two points:
There's a tiny window between lines 472-475 where Impact: Very Low - works correctly, just slightly non-obvious control flow. 3. Frontend Input Validation (Missing Client-Side Check)
Recommendation: Add client-side validation: const isValid = transport === 'http'
? urlInput.trim() !== ''
: commandInput.trim() !== '';Impact: Low - server-side validation catches it, but poor UX for empty input. 4. Error Message Consistency (Minor Polish)
if (message.includes("ENOENT") || message.includes("spawn")) {
throw new Error(`Failed to spawn process: ${message}`);
}ENOENT typically means "command not found". Consider more specific message: throw new Error(`Command not found or failed to spawn: ${message}`);Impact: Very Low - nice-to-have for better user feedback. 🔒 Security Analysis✅ No Concerns Identified
🚀 Performance Considerations✅ Good Practices
Potential OptimizationFrontend server history could grow unbounded. Consider adding a Impact: Very Low - unlikely to be a real-world issue. 📋 Test Coverage AssessmentCovered ✅
Missing (Optional Enhancements)
Verdict: Coverage is excellent for core functionality. Missing tests are edge cases. 🎯 Adherence to Repository Standards✅ Compliant
|
1 participant
Summary
Add stdio transport support to the MCP Inspector, enabling connection to MCP servers via stdin/stdout in addition to the existing HTTP transport.
Changes
Backend
createTestClientacceptsConnectionParamswithtransport: "stdio" | "http"discriminated unionConnectionManagerstoresconnectionParamsin state, builds stdio display labels (stdio: command args)connect_to_servertool: accepts stdio params, backward-compatible with legacy{ url }input/connections: validates stdio params (command required), returns transport typeConnectionRegistry: passes through both transport typesFrontend
Tests
@modelcontextprotocol/server-basic-react --stdioTesting