diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 8b1f0ab2ae..0b6af7747d 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -579,9 +579,25 @@ export class McpHub { })) as typeof config if (configInjected.type === "stdio") { + // On Windows, wrap commands with cmd.exe to handle non-exe executables like npx.ps1 + // This is necessary for node version managers (fnm, nvm-windows, volta) that implement + // commands as PowerShell scripts rather than executables. + // Note: This adds a small overhead as commands go through an additional shell layer. + const isWindows = process.platform === "win32" + + // Check if command is already cmd.exe to avoid double-wrapping + const isAlreadyWrapped = + configInjected.command.toLowerCase() === "cmd.exe" || configInjected.command.toLowerCase() === "cmd" + + const command = isWindows && !isAlreadyWrapped ? "cmd.exe" : configInjected.command + const args = + isWindows && !isAlreadyWrapped + ? ["/c", configInjected.command, ...(configInjected.args || [])] + : configInjected.args + transport = new StdioClientTransport({ - command: configInjected.command, - args: configInjected.args, + command, + args, cwd: configInjected.cwd, env: { ...getDefaultEnvironment(), diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index f6f352961c..eaa9af908c 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -31,12 +31,23 @@ vi.mock("vscode", () => ({ vi.mock("fs/promises") vi.mock("../../../core/webview/ClineProvider") +// Mock the MCP SDK modules +vi.mock("@modelcontextprotocol/sdk/client/stdio.js", () => ({ + StdioClientTransport: vi.fn(), + getDefaultEnvironment: vi.fn().mockReturnValue({ PATH: "/usr/bin" }), +})) + +vi.mock("@modelcontextprotocol/sdk/client/index.js", () => ({ + Client: vi.fn(), +})) + describe("McpHub", () => { let mcpHub: McpHubType let mockProvider: Partial // Store original console methods const originalConsoleError = console.error + const originalPlatform = Object.getOwnPropertyDescriptor(process, "platform") beforeEach(() => { vi.clearAllMocks() @@ -111,6 +122,10 @@ describe("McpHub", () => { afterEach(() => { // Restore original console methods console.error = originalConsoleError + // Restore original platform + if (originalPlatform) { + Object.defineProperty(process, "platform", originalPlatform) + } }) describe("toggleToolAlwaysAllow", () => { @@ -682,4 +697,354 @@ describe("McpHub", () => { }) }) }) + + describe("Windows command wrapping", () => { + let StdioClientTransport: ReturnType + let Client: ReturnType + + beforeEach(async () => { + // Reset mocks + vi.clearAllMocks() + + // Get references to the mocked constructors + const stdioModule = await import("@modelcontextprotocol/sdk/client/stdio.js") + const clientModule = await import("@modelcontextprotocol/sdk/client/index.js") + StdioClientTransport = stdioModule.StdioClientTransport as ReturnType + Client = clientModule.Client as ReturnType + + // Mock Windows platform + Object.defineProperty(process, "platform", { + value: "win32", + writable: true, + enumerable: true, + configurable: true, + }) + }) + + it("should wrap commands with cmd.exe on Windows", async () => { + // Mock StdioClientTransport + const mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + StdioClientTransport.mockImplementation((config: any) => { + // Verify that cmd.exe wrapping is applied + expect(config.command).toBe("cmd.exe") + expect(config.args).toEqual([ + "/c", + "npx", + "-y", + "@modelcontextprotocol/server-filesystem", + "/test/path", + ]) + return mockTransport + }) + + // Mock Client + Client.mockImplementation(() => ({ + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + })) + + // Create a new McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Mock the config file read + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-npx-server": { + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/test/path"], + }, + }, + }), + ) + + // Initialize servers (this will trigger connectToServer) + await mcpHub["initializeGlobalMcpServers"]() + + // Verify StdioClientTransport was called with wrapped command + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: "cmd.exe", + args: ["/c", "npx", "-y", "@modelcontextprotocol/server-filesystem", "/test/path"], + }), + ) + }) + + it("should not wrap commands on non-Windows platforms", async () => { + // Mock non-Windows platform + Object.defineProperty(process, "platform", { + value: "darwin", + writable: true, + enumerable: true, + configurable: true, + }) + + // Mock StdioClientTransport + const mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + StdioClientTransport.mockImplementation((config: any) => { + // Verify that no cmd.exe wrapping is applied + expect(config.command).toBe("npx") + expect(config.args).toEqual(["-y", "@modelcontextprotocol/server-filesystem", "/test/path"]) + return mockTransport + }) + + // Mock Client + Client.mockImplementation(() => ({ + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + })) + + // Create a new McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Mock the config file read + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-npx-server": { + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/test/path"], + }, + }, + }), + ) + + // Initialize servers (this will trigger connectToServer) + await mcpHub["initializeGlobalMcpServers"]() + + // Verify StdioClientTransport was called without wrapping + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/test/path"], + }), + ) + }) + + it("should not double-wrap commands that are already cmd.exe", async () => { + // Mock Windows platform + Object.defineProperty(process, "platform", { + value: "win32", + writable: true, + enumerable: true, + configurable: true, + }) + + // Mock StdioClientTransport + const mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + StdioClientTransport.mockImplementation((config: any) => { + // Verify that cmd.exe is not double-wrapped + expect(config.command).toBe("cmd.exe") + expect(config.args).toEqual(["/c", "echo", "test"]) + return mockTransport + }) + + // Mock Client + Client.mockImplementation(() => ({ + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + })) + + // Create a new McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Mock the config file read with cmd.exe already as command + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-cmd-server": { + command: "cmd.exe", + args: ["/c", "echo", "test"], + }, + }, + }), + ) + + // Initialize servers (this will trigger connectToServer) + await mcpHub["initializeGlobalMcpServers"]() + + // Verify StdioClientTransport was called without double-wrapping + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: "cmd.exe", + args: ["/c", "echo", "test"], + }), + ) + }) + + it("should handle npx.ps1 scenario from node version managers", async () => { + // Mock Windows platform + Object.defineProperty(process, "platform", { + value: "win32", + writable: true, + enumerable: true, + configurable: true, + }) + + // Mock StdioClientTransport to simulate the ENOENT error without wrapping + const mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + let callCount = 0 + StdioClientTransport.mockImplementation((config: any) => { + callCount++ + // First call would fail with ENOENT if not wrapped + // Second call should be wrapped with cmd.exe + if (callCount === 1) { + // This simulates what would happen without wrapping + expect(config.command).toBe("cmd.exe") + expect(config.args[0]).toBe("/c") + expect(config.args[1]).toBe("npx") + } + return mockTransport + }) + + // Mock Client + Client.mockImplementation(() => ({ + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + })) + + // Create a new McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Mock the config file read - simulating fnm/nvm-windows scenario + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-fnm-npx-server": { + command: "npx", + args: ["-y", "@modelcontextprotocol/server-example"], + env: { + // Simulate fnm environment + FNM_DIR: "C:\\Users\\test\\.fnm", + FNM_NODE_DIST_MIRROR: "https://nodejs.org/dist", + FNM_ARCH: "x64", + }, + }, + }, + }), + ) + + // Initialize servers (this will trigger connectToServer) + await mcpHub["initializeGlobalMcpServers"]() + + // Verify that the command was wrapped with cmd.exe + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: "cmd.exe", + args: ["/c", "npx", "-y", "@modelcontextprotocol/server-example"], + env: expect.objectContaining({ + FNM_DIR: "C:\\Users\\test\\.fnm", + FNM_NODE_DIST_MIRROR: "https://nodejs.org/dist", + FNM_ARCH: "x64", + }), + }), + ) + }) + + it("should handle case-insensitive cmd command check", async () => { + // Mock Windows platform + Object.defineProperty(process, "platform", { + value: "win32", + writable: true, + enumerable: true, + configurable: true, + }) + + // Mock StdioClientTransport + const mockTransport = { + start: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + stderr: { + on: vi.fn(), + }, + onerror: null, + onclose: null, + } + + StdioClientTransport.mockImplementation((config: any) => { + // Verify that CMD (uppercase) is not double-wrapped + expect(config.command).toBe("CMD") + expect(config.args).toEqual(["/c", "echo", "test"]) + return mockTransport + }) + + // Mock Client + Client.mockImplementation(() => ({ + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn().mockReturnValue("test instructions"), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + })) + + // Create a new McpHub instance + const mcpHub = new McpHub(mockProvider as ClineProvider) + + // Mock the config file read with CMD (uppercase) as command + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "test-cmd-uppercase-server": { + command: "CMD", + args: ["/c", "echo", "test"], + }, + }, + }), + ) + + // Initialize servers (this will trigger connectToServer) + await mcpHub["initializeGlobalMcpServers"]() + + // Verify StdioClientTransport was called without double-wrapping + expect(StdioClientTransport).toHaveBeenCalledWith( + expect.objectContaining({ + command: "CMD", + args: ["/c", "echo", "test"], + }), + ) + }) + }) })