diff --git a/src/services/marketplace/SimpleInstaller.ts b/src/services/marketplace/SimpleInstaller.ts index be002e2f1d..49cd8fe8d4 100644 --- a/src/services/marketplace/SimpleInstaller.ts +++ b/src/services/marketplace/SimpleInstaller.ts @@ -226,6 +226,20 @@ export class SimpleInstaller { const filePath = await this.getMcpFilePath(target) const mcpData = JSON.parse(contentToUse) + // Fix for Playwright MCP server: filter out empty string arguments and arguments with empty values + // Issue #8251: Empty string arguments and arguments ending with '=' cause the Playwright MCP server to fail + if (item.id === "playwright" && mcpData.args && Array.isArray(mcpData.args)) { + // Filter out empty string arguments and arguments that end with '=' (empty value arguments) + mcpData.args = mcpData.args.filter((arg: any) => { + if (typeof arg !== "string") return true + // Remove empty strings + if (arg === "") return false + // Remove arguments that end with '=' (like --browser=, --headless=, --viewport-size=) + if (arg.endsWith("=")) return false + return true + }) + } + // Read existing file or create new structure let existingData: any = { mcpServers: {} } try { diff --git a/src/services/marketplace/__tests__/SimpleInstaller.playwright.spec.ts b/src/services/marketplace/__tests__/SimpleInstaller.playwright.spec.ts new file mode 100644 index 0000000000..b15d7ff82c --- /dev/null +++ b/src/services/marketplace/__tests__/SimpleInstaller.playwright.spec.ts @@ -0,0 +1,172 @@ +// npx vitest services/marketplace/__tests__/SimpleInstaller.playwright.spec.ts + +import { describe, it, expect, vi, beforeEach } from "vitest" +import * as vscode from "vscode" +import * as fs from "fs/promises" +import * as path from "path" +import { SimpleInstaller } from "../SimpleInstaller" +import type { MarketplaceItem } from "@roo-code/types" + +// Mock vscode +vi.mock("vscode", () => ({ + workspace: { + workspaceFolders: [{ uri: { fsPath: "/test/workspace" } }], + }, + ExtensionContext: vi.fn(), +})) + +// Mock fs/promises +vi.mock("fs/promises", () => ({ + readFile: vi.fn(), + writeFile: vi.fn(), + mkdir: vi.fn(), +})) + +// Mock globalContext +vi.mock("../../../utils/globalContext", () => ({ + ensureSettingsDirectoryExists: vi.fn().mockResolvedValue("/test/settings"), +})) + +describe("SimpleInstaller - Playwright MCP Fix", () => { + let installer: SimpleInstaller + let mockContext: vscode.ExtensionContext + + beforeEach(() => { + vi.clearAllMocks() + mockContext = {} as vscode.ExtensionContext + installer = new SimpleInstaller(mockContext) + }) + + it("should filter out empty string arguments for Playwright MCP server", async () => { + const playwrightItem: MarketplaceItem = { + id: "playwright", + name: "Playwright MCP Server", + type: "mcp", + description: "Browser automation MCP server", + author: "Playwright Team", + url: "https://github.com/playwright/mcp", + content: JSON.stringify({ + command: "npx", + args: ["-y", "@playwright/mcp@latest", "--browser=", "--headless=", "--viewport-size="], + }), + } + + // Mock file doesn't exist (new installation) + vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" }) + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.writeFile).mockResolvedValue(undefined) + + await installer.installItem(playwrightItem, { target: "global" }) + + // Check that writeFile was called with filtered args + expect(fs.writeFile).toHaveBeenCalledWith( + expect.stringContaining("mcp_settings.json"), + expect.any(String), + "utf-8", + ) + + // Get the actual content that was written + const writeCall = vi.mocked(fs.writeFile).mock.calls[0] + const writtenContent = JSON.parse(writeCall[1] as string) + + // Verify that empty string arguments were filtered out + expect(writtenContent.mcpServers.playwright.args).toEqual(["-y", "@playwright/mcp@latest"]) + + // Verify command is preserved + expect(writtenContent.mcpServers.playwright.command).toBe("npx") + }) + + it("should not modify args for non-Playwright MCP servers", async () => { + const otherItem: MarketplaceItem = { + id: "other-server", + name: "Other MCP Server", + type: "mcp", + description: "Another MCP server", + author: "Other Team", + url: "https://example.com/other-server", + content: JSON.stringify({ + command: "node", + args: ["server.js", "--option=", ""], + }), + } + + // Mock file doesn't exist (new installation) + vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" }) + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.writeFile).mockResolvedValue(undefined) + + await installer.installItem(otherItem, { target: "global" }) + + // Get the actual content that was written + const writeCall = vi.mocked(fs.writeFile).mock.calls[0] + const writtenContent = JSON.parse(writeCall[1] as string) + + // Verify that args were NOT modified for non-Playwright servers + expect(writtenContent.mcpServers["other-server"].args).toEqual(["server.js", "--option=", ""]) + }) + + it("should handle Playwright MCP server with no args gracefully", async () => { + const playwrightItem: MarketplaceItem = { + id: "playwright", + name: "Playwright MCP Server", + type: "mcp", + description: "Browser automation MCP server", + author: "Playwright Team", + url: "https://github.com/playwright/mcp", + content: JSON.stringify({ + command: "npx", + // No args property + }), + } + + // Mock file doesn't exist (new installation) + vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" }) + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.writeFile).mockResolvedValue(undefined) + + await installer.installItem(playwrightItem, { target: "global" }) + + // Should not throw an error + expect(fs.writeFile).toHaveBeenCalled() + + // Get the actual content that was written + const writeCall = vi.mocked(fs.writeFile).mock.calls[0] + const writtenContent = JSON.parse(writeCall[1] as string) + + // Verify command is preserved and no args property exists + expect(writtenContent.mcpServers.playwright.command).toBe("npx") + expect(writtenContent.mcpServers.playwright.args).toBeUndefined() + }) + + it("should handle Playwright MCP server with mixed empty and non-empty args", async () => { + const playwrightItem: MarketplaceItem = { + id: "playwright", + name: "Playwright MCP Server", + type: "mcp", + description: "Browser automation MCP server", + author: "Playwright Team", + url: "https://github.com/playwright/mcp", + content: JSON.stringify({ + command: "npx", + args: ["-y", "", "@playwright/mcp@latest", "--browser=", "--headless=false", "", "--viewport-size="], + }), + } + + // Mock file doesn't exist (new installation) + vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" }) + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.writeFile).mockResolvedValue(undefined) + + await installer.installItem(playwrightItem, { target: "project" }) + + // Check that writeFile was called + expect(fs.writeFile).toHaveBeenCalledWith(expect.stringContaining("mcp.json"), expect.any(String), "utf-8") + + // Get the actual content that was written + const writeCall = vi.mocked(fs.writeFile).mock.calls[0] + const writtenContent = JSON.parse(writeCall[1] as string) + + // Verify that only empty string arguments were filtered out + expect(writtenContent.mcpServers.playwright.args).toEqual(["-y", "@playwright/mcp@latest", "--headless=false"]) + }) +})