fix(stdio): handle spaces in Command path on Windows (cmd /d /s /c)#1703
Open
yayayouyou wants to merge 2 commits into
Open
fix(stdio): handle spaces in Command path on Windows (cmd /d /s /c)#1703yayayouyou wants to merge 2 commits into
yayayouyou wants to merge 2 commits into
Conversation
On Windows, StdioClientTransport wraps non-cmd.exe commands as `cmd.exe /c <command> <args...>` and adds the arguments through ProcessStartInfo.ArgumentList. When the command is an absolute path containing a space (e.g. under "Program Files") and there is at least one argument, ArgumentList quotes both the path and the argument, so more than two quote characters follow /c. cmd then strips the first and last quote and re-parses the middle, tries to launch "C:\Program", and the server never starts. Build the cmd.exe command line by hand for the wrapping case and assign it to startInfo.Arguments directly, bypassing ArgumentList: - Invoke `cmd.exe /d /s /c "<inner>"`. /s makes cmd strip exactly the single outer pair of quotes and take the rest verbatim; /d skips AutoRun. - Quote the command and each argument (Win32 CommandLineToArgvW rules) only when it contains whitespace, a quote, or a cmd metacharacter (& | < > ^ ( )), which also makes those metacharacters literal. A bare command name is left unquoted so cmd still resolves it against the current directory and PATH. Non-Windows and the explicit cmd.exe command path are unchanged. Adds a unit test asserting the exact generated command line and a Windows-only integration test that launches a server from a directory whose path contains a space and round-trips arguments (spaces, & ^ < > |, quotes, trailing backslash). Fixes modelcontextprotocol#1601 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific StdioClientTransport startup failure when the configured Command is an absolute path containing spaces and at least one argument is present, by avoiding ProcessStartInfo.ArgumentList for the cmd.exe wrapping case and instead emitting a cmd.exe /d /s /c "<inner...>" command line directly.
Changes:
- Reworks Windows
cmd.exewrapping inStdioClientTransportto build a single pre-quoted arguments string (viaGetWindowsCmdArguments) and assign it toProcessStartInfo.Arguments. - Adds Windows-only regression tests that validate the exact
cmd.execommand line shape and a round-trip integration test using a copied test server under a space-containing directory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ModelContextProtocol.Core/Client/StdioClientTransport.cs | Builds the Windows cmd.exe wrapper invocation as a single /d /s /c "<inner>" argument string to preserve space-containing executable paths. |
| tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs | Adds Windows-only regression coverage for the wrapper command line and for launching a server from a path containing spaces. |
Comment on lines
+67
to
+69
| // On Windows, for stdio, we need to wrap non-shell commands with cmd.exe /c {command} (usually npx or uvicorn). | ||
| // The stdio transport will not work correctly if the command is not run in a shell. | ||
| string? cmdArguments = null; |
Comment on lines
+204
to
+222
| const string spacedCommand = @"C:\Program Files\My Server\server.exe"; | ||
|
|
||
| var cases = new (string command, string[]? arguments, string expectedArguments)[] | ||
| { | ||
| // Space in the command path with a trailing argument — the exact shape that failed in #1601. | ||
| (spacedCommand, ["--flag"], Wrap(Quote(spacedCommand) + " --flag")), | ||
| // Space in the command path with an argument that itself contains spaces. | ||
| (spacedCommand, ["arg with spaces"], Wrap(Quote(spacedCommand) + " " + Quote("arg with spaces"))), | ||
| // Space in the command path with no arguments. | ||
| (spacedCommand, null, Wrap(Quote(spacedCommand))), | ||
| // A space-free command path is left unquoted; cmd.exe metacharacters in arguments are quoted so | ||
| // cmd treats them literally rather than as operators. | ||
| (@"C:\tools\server.exe", ["a&b", "c|d", "e>f", "g<h", "i^j"], | ||
| Wrap(@"C:\tools\server.exe" + " " + Quote("a&b") + " " + Quote("c|d") + " " + Quote("e>f") + " " + Quote("g<h") + " " + Quote("i^j"))), | ||
| // A bare command name and ordinary arguments without special characters are passed through unquoted. | ||
| ("server.exe", ["--key=value", "plain"], Wrap("server.exe" + " --key=value plain")), | ||
| // An empty argument is preserved as an empty quoted token. | ||
| ("server.exe", [""], Wrap("server.exe" + " " + Quote(""))), | ||
| }; |
- Update the wrapping comment to reflect the hand-built `cmd.exe /d /s /c`
command line instead of the old `cmd.exe /c {command}` description.
- Use guaranteed-nonexistent, GUID-based command names in
WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePair so the wrapper's cmd.exe
never launches a real program if a name like `server.exe` happens to
resolve from PATH; the test only inspects the logged command line.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Thanks @copilot — addressed both points in 0426932:
(AI-assisted, reviewed and tested on Windows.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1601.
Problem
On Windows,
StdioClientTransportwraps every non-cmd.execommand ascmd.exe /c <command> <args...>, adding the arguments throughProcessStartInfo.ArgumentList. When the command is an absolute path that contains a space (e.g. anything underC:\Program Files\...) and there is at least one argument,ArgumentListquotes both the space-containing path and the argument, so more than two quote characters follow/c. Percmd /?, cmd then strips the first and last quote and re-parses the middle, so it tries to launchC:\Programand the MCP server never starts. The same executable at a space-free path works, which is why this is easy to miss (npx,uvicorn,dotnetall live on space-free PATH entries).Fix
Build the
cmd.execommand line by hand for the wrapping case and assign it tostartInfo.Argumentsdirectly, bypassingArgumentList:cmd.exe /d /s /c "<inner>"./smakes cmd strip exactly the single outer pair of quotes and take the rest verbatim (avoiding the multi-quote re-parsing rule);/dskips AutoRun.<inner>is the command followed by the arguments, each quoted with the Win32CommandLineToArgvWrules only when it contains whitespace, a quote, or a cmd metacharacter (& | < > ^ ( )) — quoting also makes those metacharacters literal. A bare command name is left unquoted so cmd still resolves it against the current directory / PATH.^-escaping used on theArgumentListpath (EscapeArgumentString) is not applied here, since those characters are already literal inside the quoted/s /cregion.Non-Windows and the explicit
cmd.execommand path are unchanged.Testing
Verified on Windows 11 with .NET 10:
WindowsCmdWrapper_UsesDsCWithSingleOuterQuotePairasserts the exact generated command line for: space-in-path + arg, arg with spaces, no-args, metacharacters, plain args, and an empty arg.CommandPathWithSpaces_RoundTripsArgumentsThroughCmdcopies the test server to a directory whose path contains a space, launches it by absolute path, and asserts the argument echoes back unchanged for spaces,& ^ < > |, quotes, trailing backslash, and a URL with&.EscapesCliArgumentsCorrectlyround-trip test (no-space command + tricky args) still passes, confirming no regression for the common case.dotnet test tests/ModelContextProtocol.Tests/is green.AI-assistance disclosure: I used AI assistance to investigate and implement this change, and I reviewed and tested it myself on Windows.