mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: handle npx with package@version syntax on Windows for MCP servers
- Special handling for npx commands with @ in package names (e.g., chrome-devtools-mcp@latest) - On Windows, combine npx and package@version into a single command string before wrapping with cmd.exe - This ensures proper package resolution when using npx with versioned packages - Added comprehensive tests for the new behavior Fixes #9167
This commit is contained in:
parent
6e6341346e
commit
14a8021f4d
2 changed files with 152 additions and 7 deletions
|
|
@ -677,11 +677,30 @@ export class McpHub {
|
|||
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
|
||||
// Special handling for npx with package@version syntax
|
||||
const isNpxWithPackage =
|
||||
configInjected.command.toLowerCase() === "npx" &&
|
||||
configInjected.args &&
|
||||
configInjected.args.length > 0 &&
|
||||
configInjected.args[0].includes("@")
|
||||
|
||||
let command = configInjected.command
|
||||
let args = configInjected.args
|
||||
|
||||
if (isWindows && !isAlreadyWrapped) {
|
||||
if (isNpxWithPackage) {
|
||||
// For npx with package@version, combine npx and package name into a single command string
|
||||
// This ensures proper package resolution on Windows
|
||||
const packageName = configInjected.args![0]
|
||||
const remainingArgs = configInjected.args!.slice(1)
|
||||
command = "cmd.exe"
|
||||
args = ["/c", `npx ${packageName}`, ...remainingArgs]
|
||||
} else {
|
||||
// Standard wrapping for other commands
|
||||
command = "cmd.exe"
|
||||
args = ["/c", configInjected.command, ...(configInjected.args || [])]
|
||||
}
|
||||
}
|
||||
|
||||
transport = new StdioClientTransport({
|
||||
command,
|
||||
|
|
@ -1378,8 +1397,8 @@ export class McpHub {
|
|||
await this.deleteConnection(serverName, serverSource)
|
||||
// Re-add as a disabled connection
|
||||
// Re-read config from file to get updated disabled state
|
||||
const updatedConfig = await this.readServerConfigFromFile(serverName, serverSource)
|
||||
await this.connectToServer(serverName, updatedConfig, serverSource)
|
||||
const updatedConfig = await this.readServerConfigFromFile(serverName, serverSource)
|
||||
await this.connectToServer(serverName, updatedConfig, serverSource)
|
||||
} else if (!disabled && connection.server.status === "disconnected") {
|
||||
// If enabling a disabled server, connect it
|
||||
// Re-read config from file to get updated disabled state
|
||||
|
|
|
|||
|
|
@ -2146,5 +2146,131 @@ describe("McpHub", () => {
|
|||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("should handle npx with package@version syntax on Windows", 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 npx with package@version is properly wrapped
|
||||
expect(config.command).toBe("cmd.exe")
|
||||
// The package@version should be combined with npx in a single string
|
||||
expect(config.args).toEqual(["/c", "npx chrome-devtools-mcp@latest"])
|
||||
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 chrome-devtools-mcp@latest
|
||||
vi.mocked(fs.readFile).mockResolvedValue(
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
"chrome-devtools": {
|
||||
command: "npx",
|
||||
args: ["chrome-devtools-mcp@latest"],
|
||||
},
|
||||
},
|
||||
}),
|
||||
)
|
||||
|
||||
// Initialize servers (this will trigger connectToServer)
|
||||
await mcpHub["initializeGlobalMcpServers"]()
|
||||
|
||||
// Verify StdioClientTransport was called with properly wrapped command
|
||||
expect(StdioClientTransport).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
command: "cmd.exe",
|
||||
args: ["/c", "npx chrome-devtools-mcp@latest"],
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it("should handle npx with package@version and additional args on Windows", 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 npx with package@version and additional args is properly wrapped
|
||||
expect(config.command).toBe("cmd.exe")
|
||||
// The package@version should be combined with npx, and additional args follow
|
||||
expect(config.args).toEqual(["/c", "npx some-package@1.2.3", "--config", "test.json"])
|
||||
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 package@version and additional args
|
||||
vi.mocked(fs.readFile).mockResolvedValue(
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
"test-server": {
|
||||
command: "npx",
|
||||
args: ["some-package@1.2.3", "--config", "test.json"],
|
||||
},
|
||||
},
|
||||
}),
|
||||
)
|
||||
|
||||
// Initialize servers (this will trigger connectToServer)
|
||||
await mcpHub["initializeGlobalMcpServers"]()
|
||||
|
||||
// Verify StdioClientTransport was called with properly wrapped command
|
||||
expect(StdioClientTransport).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
command: "cmd.exe",
|
||||
args: ["/c", "npx some-package@1.2.3", "--config", "test.json"],
|
||||
}),
|
||||
)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue