diff --git a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs index 2e44ee34f..03b7f68a9 100644 --- a/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/StdioClientTransport.cs @@ -63,11 +63,13 @@ public async Task ConnectAsync(CancellationToken cancellationToken = string command = _options.Command; IList? arguments = _options.Arguments; - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && - !string.Equals(Path.GetFileName(command), "cmd.exe", StringComparison.OrdinalIgnoreCase)) + bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + bool isCmd = string.Equals(Path.GetFileName(command), "cmd.exe", StringComparison.OrdinalIgnoreCase); + bool needsCmdEscaping = isWindows && isCmd; + if (isWindows && !isCmd && !ShouldLaunchDirectly(command)) { - // 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. + // Use cmd.exe for commands that require shell handling or resolution through PATH/PATHEXT. + needsCmdEscaping = true; arguments = arguments is null or [] ? ["/c", command] : ["/c", command, ..arguments]; command = "cmd.exe"; } @@ -98,13 +100,13 @@ public async Task ConnectAsync(CancellationToken cancellationToken = #if NET foreach (string arg in arguments) { - startInfo.ArgumentList.Add(EscapeArgumentString(arg)); + startInfo.ArgumentList.Add(needsCmdEscaping ? EscapeArgumentString(arg) : arg); } #else StringBuilder argsBuilder = new(); foreach (string arg in arguments) { - PasteArguments.AppendArgument(argsBuilder, EscapeArgumentString(arg)); + PasteArguments.AppendArgument(argsBuilder, needsCmdEscaping ? EscapeArgumentString(arg) : arg); } startInfo.Arguments = argsBuilder.ToString(); @@ -288,6 +290,23 @@ internal static bool HasExited(Process process) } } + private static bool ShouldLaunchDirectly(string command) + { + // CreateProcess can only launch real executable images directly, + // which in practice means .exe/.com. Anything else (.bat/.cmd) relies on cmd.exe + // resolving it via file association / PATHEXT, so it must keep going through the cmd.exe wrapper. + string extension = Path.GetExtension(command); + if (!extension.Equals(".exe", StringComparison.OrdinalIgnoreCase) && + !extension.Equals(".com", StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + // Match Process resolution on Unix: rooted paths are used as supplied, while relative + // paths are probed against the parent process's current directory. + return Path.IsPathRooted(command) || File.Exists(command); + } + private static string EscapeArgumentString(string argument) => RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && !ContainsWhitespaceRegex.IsMatch(argument) ? WindowsCliSpecialArgumentsRegex.Replace(argument, static match => "^" + match.Value) : diff --git a/tests/ModelContextProtocol.TestServer/Program.cs b/tests/ModelContextProtocol.TestServer/Program.cs index 6812fe5d5..ee51bbae7 100644 --- a/tests/ModelContextProtocol.TestServer/Program.cs +++ b/tests/ModelContextProtocol.TestServer/Program.cs @@ -41,6 +41,13 @@ private static async Task Main(string[] args) return; } + if (args.Contains("--echo-cwd-and-exit")) + { + Console.Error.WriteLine($"CWD:{Environment.CurrentDirectory}"); + Console.Error.Flush(); + return; + } + Log.Logger.Information("Starting server..."); string? cliArg = ParseCliArgument(args); diff --git a/tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj b/tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj index 677d77357..2a21c0093 100644 --- a/tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj +++ b/tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj @@ -93,6 +93,10 @@ PreserveNewest + + ExecutableWithSpacesHandledCorrectly\%(RecursiveDir)%(Filename)%(Extension) + PreserveNewest + PreserveNewest diff --git a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs index 60ce9cf5a..9c3538f33 100644 --- a/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs +++ b/tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs @@ -165,13 +165,13 @@ public async Task EscapesCliArgumentsCorrectly(string? cliArgumentValue) Command = (PlatformDetection.IsMonoRuntime, PlatformDetection.IsWindows) switch { (true, _) => "mono", - (_, true) => testServerExecutable, + (_, true) => "cmd.exe", _ => "dotnet", }, Arguments = (PlatformDetection.IsMonoRuntime, PlatformDetection.IsWindows) switch { (true, _) => [testServerExecutable, "--echo-cli-arg-and-exit", cliArgument], - (_, true) => ["--echo-cli-arg-and-exit", cliArgument], + (_, true) => ["/c", testServerExecutable, "--echo-cli-arg-and-exit", cliArgument], _ => [testServerDll, "--echo-cli-arg-and-exit", cliArgument], }, StandardErrorLines = line => @@ -192,12 +192,128 @@ public async Task EscapesCliArgumentsCorrectly(string? cliArgumentValue) JsonElement parsedArgument = JsonElement.Parse(serializedArgument); Assert.Equal(cliArgumentValue ?? "", parsedArgument.GetString()); - var exception = await Assert.ThrowsAsync( - async () => await session.MessageReader.Completion.WaitAsync( + await AssertServerExitedCleanlyAsync(session); + } + + [Theory] + [InlineData("My Test Server.exe", true)] + [InlineData("My Test Server.com", false)] + [InlineData("my directory/my test server.exe", false)] + public async Task ExecutableWithSpacesHandledCorrectly(string relativeExecutablePath, bool useRootedPath) + { + const string OutputPrefix = "CLI_ARG:"; + const string CliArgumentValue = "42"; + + string rootDirectoryName = $"BypassCmd-{Guid.NewGuid():N}"; + string rootDirectory = Path.Combine(Environment.CurrentDirectory, rootDirectoryName); + string executablePath = Path.Combine(rootDirectory, relativeExecutablePath); + string executableDirectory = Path.GetDirectoryName(executablePath)!; + + try + { + string sourceDirectory = Path.Combine(AppContext.BaseDirectory, nameof(ExecutableWithSpacesHandledCorrectly)) + Path.DirectorySeparatorChar; + foreach (string sourcePath in Directory.EnumerateFiles(sourceDirectory, "*", SearchOption.AllDirectories)) + { + string destinationPath = Path.Combine(executableDirectory, sourcePath[sourceDirectory.Length..]); + Directory.CreateDirectory(Path.GetDirectoryName(destinationPath)!); + File.Copy(sourcePath, destinationPath); + } + + string sourceExecutablePath = Path.Combine(sourceDirectory, PlatformDetection.IsWindows ? "TestServer.exe" : "TestServer"); + File.Copy(sourceExecutablePath, executablePath, overwrite: true); + + // .NET Framework loads binding redirects from a configuration file matching the executable's name. + string sourceConfigurationPath = sourceExecutablePath + ".config"; + if (File.Exists(sourceConfigurationPath)) + { + File.Copy(sourceConfigurationPath, executablePath + ".config"); + } + + var capturedArgument = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var transport = new StdioClientTransport(new() + { + Name = "TestServer", + Command = useRootedPath ? executablePath : Path.Combine(rootDirectoryName, relativeExecutablePath), + Arguments = ["--echo-cli-arg-and-exit", $"--cli-arg={CliArgumentValue}"], + StandardErrorLines = line => + { + if (line.StartsWith(OutputPrefix, StringComparison.Ordinal)) + { + capturedArgument.TrySetResult(line[OutputPrefix.Length..]); + } + }, + }, LoggerFactory); + + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + string serializedArgument = await capturedArgument.Task.WaitAsync( TestConstants.DefaultTimeout, - TestContext.Current.CancellationToken)); - var completionDetails = Assert.IsType(exception.Details); - Assert.Equal(0, completionDetails.ExitCode); + TestContext.Current.CancellationToken); + Assert.Equal(CliArgumentValue, JsonElement.Parse(serializedArgument).GetString()); + + await AssertServerExitedCleanlyAsync(session); + } + finally + { + try + { + Directory.Delete(rootDirectory, recursive: true); + } + catch (UnauthorizedAccessException) { } + } + } + + [Fact] + public async Task WorkingDirectory_IsUsedAsChildProcessCurrentDirectory() + { + const string OutputPrefix = "CWD:"; + string testServerExecutable = Path.Combine( + AppContext.BaseDirectory, + PlatformDetection.IsWindows ? "TestServer.exe" : "TestServer"); + + // A directory that does not contain the executable, so a successful launch also demonstrates the + // executable is located independently of WorkingDirectory. + string workingDirectory = Path.Combine(Path.GetTempPath(), $"McpStdioWorkingDir-{Guid.NewGuid():N}"); + Directory.CreateDirectory(workingDirectory); + try + { + var capturedCwd = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + StdioClientTransportOptions options = new() + { + Name = "TestServer", + Command = testServerExecutable, + Arguments = ["--echo-cwd-and-exit"], + WorkingDirectory = workingDirectory, + StandardErrorLines = line => + { + if (line.StartsWith(OutputPrefix, StringComparison.Ordinal)) + { + capturedCwd.TrySetResult(line[OutputPrefix.Length..]); + } + }, + }; + + var transport = new StdioClientTransport(options, LoggerFactory); + await using var session = await transport.ConnectAsync(TestContext.Current.CancellationToken); + + string reportedCwd = (await capturedCwd.Task.WaitAsync( + TestConstants.DefaultTimeout, + TestContext.Current.CancellationToken)).Trim(); + + // The child reports the working directory we set. Compare with EndsWith because some platforms + // (e.g. macOS) report the temp path through its resolved location (/var -> /private/var). + char[] separators = [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar]; + Assert.EndsWith( + workingDirectory.TrimEnd(separators), + reportedCwd.TrimEnd(separators), + PlatformDetection.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); + + await AssertServerExitedCleanlyAsync(session); + } + finally + { + Directory.Delete(workingDirectory, recursive: true); + } } [Fact(Skip = "Platform not supported by this test.", SkipUnless = nameof(IsStdErrCallbackSupported))] @@ -431,4 +547,14 @@ public async Task ReadMessagesAsync_Should_Accept_CRLF_Delimited_Messages() Assert.IsType(readMessage); Assert.Equal("44", ((JsonRpcRequest)readMessage).Id.ToString()); } + + private static async Task AssertServerExitedCleanlyAsync(ITransport session) + { + var exception = await Assert.ThrowsAsync( + async () => await session.MessageReader.Completion.WaitAsync( + TestConstants.DefaultTimeout, + TestContext.Current.CancellationToken)); + var completionDetails = Assert.IsType(exception.Details); + Assert.Equal(0, completionDetails.ExitCode); + } }