-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: filter out empty string arguments for Playwright MCP server #8252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider extracting this filtering logic into a separate private method like |
||
| if (typeof arg !== "string") return true | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good defensive programming checking for non-string arguments. Consider logging a warning when non-string arguments are encountered, as this might indicate a data quality issue from the marketplace API. |
||
| // 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 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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="], | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a test case for arguments that contain only whitespace (e.g., |
||
| }), | ||
| } | ||
|
|
||
| // 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"]) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this empty argument filtering more generic. While the fix correctly targets the Playwright MCP server, other MCP servers might face similar issues with empty value arguments. The test shows that other servers with similar patterns are left untouched.
Consider whether this filtering logic should be: