fix: address review feedback - prevent double-wrapping and add tests

Author: hannesrudolph

src/services/mcp/McpHub.ts

@@ -587,11 +587,20 @@ export class McpHub {
 
 			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"
-				const command = isWindows ? "cmd.exe" : configInjected.command
-				const args = isWindows
-					? ["/c", configInjected.command, ...(configInjected.args || [])]
-					: configInjected.args
+
+				// 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,

src/services/mcp/__tests__/McpHub.test.ts

@@ -842,5 +842,209 @@ describe("McpHub", () => {
 				}),
 			)
 		})
+
+		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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				stderr: {
+					on: jest.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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				getInstructions: jest.fn().mockReturnValue("test instructions"),
+				request: jest.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
+			;(fs.readFile as jest.Mock).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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				stderr: {
+					on: jest.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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				getInstructions: jest.fn().mockReturnValue("test instructions"),
+				request: jest.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
+			;(fs.readFile as jest.Mock).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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				stderr: {
+					on: jest.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: jest.fn().mockResolvedValue(undefined),
+				close: jest.fn().mockResolvedValue(undefined),
+				getInstructions: jest.fn().mockReturnValue("test instructions"),
+				request: jest.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
+			;(fs.readFile as jest.Mock).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"],
+				}),
+			)
+		})
 	})
 })