Skip to content

Commit a091768

Browse files
committed
fix: filter out empty string arguments for Playwright MCP server
- Fixes issue #8251 where Playwright MCP server fails to start with empty string arguments - Filters out both empty strings and arguments ending with = (like --browser=) - Added comprehensive tests to verify the fix works correctly - Does not affect other MCP servers, only applies to Playwright
1 parent 12f94fc commit a091768

File tree

2 files changed

+186
-0
lines changed

2 files changed

+186
-0
lines changed

src/services/marketplace/SimpleInstaller.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,20 @@ export class SimpleInstaller {
226226
const filePath = await this.getMcpFilePath(target)
227227
const mcpData = JSON.parse(contentToUse)
228228

229+
// Fix for Playwright MCP server: filter out empty string arguments and arguments with empty values
230+
// Issue #8251: Empty string arguments and arguments ending with '=' cause the Playwright MCP server to fail
231+
if (item.id === "playwright" && mcpData.args && Array.isArray(mcpData.args)) {
232+
// Filter out empty string arguments and arguments that end with '=' (empty value arguments)
233+
mcpData.args = mcpData.args.filter((arg: any) => {
234+
if (typeof arg !== "string") return true
235+
// Remove empty strings
236+
if (arg === "") return false
237+
// Remove arguments that end with '=' (like --browser=, --headless=, --viewport-size=)
238+
if (arg.endsWith("=")) return false
239+
return true
240+
})
241+
}
242+
229243
// Read existing file or create new structure
230244
let existingData: any = { mcpServers: {} }
231245
try {
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
// npx vitest services/marketplace/__tests__/SimpleInstaller.playwright.spec.ts
2+
3+
import { describe, it, expect, vi, beforeEach } from "vitest"
4+
import * as vscode from "vscode"
5+
import * as fs from "fs/promises"
6+
import * as path from "path"
7+
import { SimpleInstaller } from "../SimpleInstaller"
8+
import type { MarketplaceItem } from "@roo-code/types"
9+
10+
// Mock vscode
11+
vi.mock("vscode", () => ({
12+
workspace: {
13+
workspaceFolders: [{ uri: { fsPath: "/test/workspace" } }],
14+
},
15+
ExtensionContext: vi.fn(),
16+
}))
17+
18+
// Mock fs/promises
19+
vi.mock("fs/promises", () => ({
20+
readFile: vi.fn(),
21+
writeFile: vi.fn(),
22+
mkdir: vi.fn(),
23+
}))
24+
25+
// Mock globalContext
26+
vi.mock("../../../utils/globalContext", () => ({
27+
ensureSettingsDirectoryExists: vi.fn().mockResolvedValue("/test/settings"),
28+
}))
29+
30+
describe("SimpleInstaller - Playwright MCP Fix", () => {
31+
let installer: SimpleInstaller
32+
let mockContext: vscode.ExtensionContext
33+
34+
beforeEach(() => {
35+
vi.clearAllMocks()
36+
mockContext = {} as vscode.ExtensionContext
37+
installer = new SimpleInstaller(mockContext)
38+
})
39+
40+
it("should filter out empty string arguments for Playwright MCP server", async () => {
41+
const playwrightItem: MarketplaceItem = {
42+
id: "playwright",
43+
name: "Playwright MCP Server",
44+
type: "mcp",
45+
description: "Browser automation MCP server",
46+
author: "Playwright Team",
47+
url: "https://github.com/playwright/mcp",
48+
content: JSON.stringify({
49+
command: "npx",
50+
args: ["-y", "@playwright/mcp@latest", "--browser=", "--headless=", "--viewport-size="],
51+
}),
52+
}
53+
54+
// Mock file doesn't exist (new installation)
55+
vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" })
56+
vi.mocked(fs.mkdir).mockResolvedValue(undefined)
57+
vi.mocked(fs.writeFile).mockResolvedValue(undefined)
58+
59+
await installer.installItem(playwrightItem, { target: "global" })
60+
61+
// Check that writeFile was called with filtered args
62+
expect(fs.writeFile).toHaveBeenCalledWith(
63+
expect.stringContaining("mcp_settings.json"),
64+
expect.any(String),
65+
"utf-8",
66+
)
67+
68+
// Get the actual content that was written
69+
const writeCall = vi.mocked(fs.writeFile).mock.calls[0]
70+
const writtenContent = JSON.parse(writeCall[1] as string)
71+
72+
// Verify that empty string arguments were filtered out
73+
expect(writtenContent.mcpServers.playwright.args).toEqual(["-y", "@playwright/mcp@latest"])
74+
75+
// Verify command is preserved
76+
expect(writtenContent.mcpServers.playwright.command).toBe("npx")
77+
})
78+
79+
it("should not modify args for non-Playwright MCP servers", async () => {
80+
const otherItem: MarketplaceItem = {
81+
id: "other-server",
82+
name: "Other MCP Server",
83+
type: "mcp",
84+
description: "Another MCP server",
85+
author: "Other Team",
86+
url: "https://example.com/other-server",
87+
content: JSON.stringify({
88+
command: "node",
89+
args: ["server.js", "--option=", ""],
90+
}),
91+
}
92+
93+
// Mock file doesn't exist (new installation)
94+
vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" })
95+
vi.mocked(fs.mkdir).mockResolvedValue(undefined)
96+
vi.mocked(fs.writeFile).mockResolvedValue(undefined)
97+
98+
await installer.installItem(otherItem, { target: "global" })
99+
100+
// Get the actual content that was written
101+
const writeCall = vi.mocked(fs.writeFile).mock.calls[0]
102+
const writtenContent = JSON.parse(writeCall[1] as string)
103+
104+
// Verify that args were NOT modified for non-Playwright servers
105+
expect(writtenContent.mcpServers["other-server"].args).toEqual(["server.js", "--option=", ""])
106+
})
107+
108+
it("should handle Playwright MCP server with no args gracefully", async () => {
109+
const playwrightItem: MarketplaceItem = {
110+
id: "playwright",
111+
name: "Playwright MCP Server",
112+
type: "mcp",
113+
description: "Browser automation MCP server",
114+
author: "Playwright Team",
115+
url: "https://github.com/playwright/mcp",
116+
content: JSON.stringify({
117+
command: "npx",
118+
// No args property
119+
}),
120+
}
121+
122+
// Mock file doesn't exist (new installation)
123+
vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" })
124+
vi.mocked(fs.mkdir).mockResolvedValue(undefined)
125+
vi.mocked(fs.writeFile).mockResolvedValue(undefined)
126+
127+
await installer.installItem(playwrightItem, { target: "global" })
128+
129+
// Should not throw an error
130+
expect(fs.writeFile).toHaveBeenCalled()
131+
132+
// Get the actual content that was written
133+
const writeCall = vi.mocked(fs.writeFile).mock.calls[0]
134+
const writtenContent = JSON.parse(writeCall[1] as string)
135+
136+
// Verify command is preserved and no args property exists
137+
expect(writtenContent.mcpServers.playwright.command).toBe("npx")
138+
expect(writtenContent.mcpServers.playwright.args).toBeUndefined()
139+
})
140+
141+
it("should handle Playwright MCP server with mixed empty and non-empty args", async () => {
142+
const playwrightItem: MarketplaceItem = {
143+
id: "playwright",
144+
name: "Playwright MCP Server",
145+
type: "mcp",
146+
description: "Browser automation MCP server",
147+
author: "Playwright Team",
148+
url: "https://github.com/playwright/mcp",
149+
content: JSON.stringify({
150+
command: "npx",
151+
args: ["-y", "", "@playwright/mcp@latest", "--browser=", "--headless=false", "", "--viewport-size="],
152+
}),
153+
}
154+
155+
// Mock file doesn't exist (new installation)
156+
vi.mocked(fs.readFile).mockRejectedValue({ code: "ENOENT" })
157+
vi.mocked(fs.mkdir).mockResolvedValue(undefined)
158+
vi.mocked(fs.writeFile).mockResolvedValue(undefined)
159+
160+
await installer.installItem(playwrightItem, { target: "project" })
161+
162+
// Check that writeFile was called
163+
expect(fs.writeFile).toHaveBeenCalledWith(expect.stringContaining("mcp.json"), expect.any(String), "utf-8")
164+
165+
// Get the actual content that was written
166+
const writeCall = vi.mocked(fs.writeFile).mock.calls[0]
167+
const writtenContent = JSON.parse(writeCall[1] as string)
168+
169+
// Verify that only empty string arguments were filtered out
170+
expect(writtenContent.mcpServers.playwright.args).toEqual(["-y", "@playwright/mcp@latest", "--headless=false"])
171+
})
172+
})

0 commit comments

Comments
 (0)