fix: filter out empty string arguments for Playwright MCP server #8252
Conversation
- 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
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
|
|
||
| // 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)) { |
There was a problem hiding this comment.
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:
- Applied to all MCP servers (with an opt-out list if needed)
- Configurable via a property in the MarketplaceItem
- At minimum, documented why it's Playwright-specific
| 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 |
There was a problem hiding this comment.
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.
| // 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) => { |
There was a problem hiding this comment.
Consider extracting this filtering logic into a separate private method like filterEmptyArguments(args: any[]): any[] for better readability and potential reuse.
| url: "https://github.com/playwright/mcp", | ||
| content: JSON.stringify({ | ||
| command: "npx", | ||
| args: ["-y", "", "@playwright/mcp@latest", "--browser=", "--headless=false", "", "--viewport-size="], |
There was a problem hiding this comment.
Consider adding a test case for arguments that contain only whitespace (e.g., " ") to ensure they're handled appropriately.
hannesrudolph
added
the
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Summary
This PR fixes issue #8251 where the Playwright MCP server fails to start due to empty string arguments in its configuration.
Problem
The Playwright MCP server configuration from the marketplace API includes arguments with empty values:
--browser=--headless=--viewport-size=These empty value arguments cause the Playwright MCP server to fail during startup.
Solution
Added a targeted fix in the
SimpleInstallerthat:item.id === 'playwright')'') and arguments ending with=(empty value arguments)-yand@playwright/mcp@latestTesting
Impact
Fixes #8251
Important
Fixes Playwright MCP server startup issue by filtering out empty string and empty value arguments in
SimpleInstaller.SimpleInstaller, filter out empty string arguments and arguments ending with=for Playwright MCP server.-yand@playwright/mcp@latest.SimpleInstaller.playwright.spec.tswith 4 test cases for filtering empty arguments, non-Playwright server handling, missing args, and mixed args.This description was created by
for a091768. You can customize this summary. It will automatically update as commits are pushed.