Skip to content

feat(mcp): add playwright preset to hermes mcp add - #63771

Closed
ygd58 wants to merge 2 commits into
NousResearch:mainfrom
ygd58:fix/mcp-playwright-preset
Closed

feat(mcp): add playwright preset to hermes mcp add#63771
ygd58 wants to merge 2 commits into
NousResearch:mainfrom
ygd58:fix/mcp-playwright-preset

Conversation

@ygd58

@ygd58 ygd58 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Problem

hermes mcp add had no playwright preset (issue #19760). Previous PR #19768 added it but used a single /opt/google/chrome path check (classifying PATH installs, /usr/bin, macOS, Windows as browser absent) and dropped the codex preset.

Fix

  • codex preset preserved alongside playwright
  • _playwright_browser_args() uses shutil.which() across common names (google-chrome, google-chrome-stable, chromium-browser, chromium, chrome)
  • --no-sandbox only on Linux euid==0
  • --browser=chromium only when no system browser found on PATH

Verification

7 tests in TestPlaywrightMcpPreset: preset registered, npx command, --no-sandbox on Linux root, not on macOS, chromium fallback when absent, no fallback when found, /usr/bin/chromium-browser counts. 7/7 pass.

Fixes #19760

ygd58 added 2 commits July 13, 2026 15:44
Adds @playwright/mcp to the built-in MCP preset registry alongside
the existing codex preset (issue NousResearch#19760).

Key design decisions (addressing maintainer review of PR NousResearch#19768):
- codex preset is preserved; both entries coexist in _MCP_PRESETS
- _playwright_browser_args() uses platform-aware detection via
  shutil.which() across the common Chrome/Chromium binary names
  (google-chrome, google-chrome-stable, chromium-browser, chromium,
  chrome) instead of a single /opt/google/chrome/chrome path that
  would misclassify PATH installs, /usr/bin installs, snap packages,
  macOS, and Windows as 'browser absent'
- --no-sandbox added only on Linux when euid==0 (root/VPS/Docker)
- --browser=chromium fallback added only when NO system browser is
  found on the current PATH (so default Playwright behavior is
  preserved for normal user installs)

7 tests in TestPlaywrightMcpPreset:
- playwright registered alongside codex
- preset uses npx command
- --no-sandbox on Linux root
- --no-sandbox NOT added on macOS
- --browser=chromium added when no system browser found
- --browser=chromium NOT added when google-chrome on PATH
- /usr/bin/chromium-browser also counts as system browser

Fixes NousResearch#19760
Documents the new playwright preset in the built-in-presets table
at website/docs/user-guide/features/mcp.md, including the root/VPS
--no-sandbox behavior and the Playwright bundled Chromium fallback.
@alt-glitch alt-glitch added type/feature New feature or request comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth P3 Low — cosmetic, nice to have labels Jul 13, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for preserving the existing codex preset and adding focused tests and MCP documentation.

Problems

  • hermes_cli/mcp_config.py:68 only uses shutil.which() to decide whether a browser exists. That still misses standard macOS app bundles and Windows install directories; current main's hermes_cli/browser_connect.py:22-124 explicitly checks those platform locations. The forced fallback can therefore change behavior for ordinary non-PATH browser installs.
  • The PR body says Fixes #19760, but #19760 is the closed Nix npmDepsHash report, not a Playwright MCP request.

Suggested changes

  • Use an equivalent platform-aware resolver (and add macOS/Windows non-PATH tests), or retain the upstream standard npx @playwright/mcp@latest arguments rather than forcing a browser choice.
  • Correct the issue linkage.

Automated hermes-sweeper review.

Comment thread hermes_cli/mcp_config.py
"chromium-browser", "chromium",
"chrome",
)
_found_system_browser = any(shutil.which(name) for name in _chrome_names)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shutil.which() alone still reports no browser for standard macOS app-bundle installs and Windows install locations. hermes_cli/browser_connect.py:22-124 covers those paths; use equivalent platform-aware detection (with macOS/Windows regressions), or avoid forcing the fallback.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 2026
@ygd58

ygd58 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Ported forward in #68488: browser detection now reuses hermes_cli.browser_connect.get_chrome_debug_candidates() (fixes the macOS/Windows detection gap) and the wrong Fixes #19760 reference is dropped. Closing in favor of #68488.

@ygd58 ygd58 closed this Jul 21, 2026
ygd58 added a commit to ygd58/hermes-agent that referenced this pull request Jul 22, 2026
…eset

Per teknium1's review of NousResearch#63771 ('add macOS/Windows non-PATH tests'),
only the macOS .app-bundle case was added. Add the matching Windows
standard-install-directory case, confirming the platform-aware
get_chrome_debug_candidates() resolver -- not a bare shutil.which()
name list -- is what determines browser availability on Windows too.

68/68 tests pass in tests/hermes_cli/test_mcp_config.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/mcp MCP client and OAuth type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Build failure in Nix flake: npmDepsHash is out of date for hermes-tui

3 participants