[Bugfix] Fix local Chrome CDP attach validation - #12968
Conversation
|
Hi Hermes team @teknium1 , I’ve really enjoyed following the project and working with the codebase, and I’d love to become a contributor. I’m interested in helping with bug fixes, testing, documentation, and small feature improvements, and I’m happy to start with issues that are most useful to the project. I’d like to contribute consistently, learn the project’s standards, and gradually take on more responsibility over time.☺ |
|
Likely duplicate of #12198 — both harden local Chrome CDP discovery to handle /json/version unavailability and validate actual CDP endpoint. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for investigating the Chrome built-in remote-debugging path. The underlying compatibility gap remains on current main: tools/browser_tool.py:422–433 returns the raw shorthand after discovery fails, and the CLI rejects the connection at hermes_cli/cli_commands_mixin.py:1847 and :1882–:1886 before it can attach.
Problems
tools/browser_tool.py:269on this PR readsparsed.portoutside the exception handler. For an invalid port,urllib.parseraisesValueError, changing a failed CDP discovery into an uncaught resolver failure.- The patch does not cover the TUI
browser.managepath, which independently probes only/json/versionand/jsonintui_gateway/server.py:13707–:13708. - The second commit includes unrelated
run_agent.py,tools/approval.py, model-catalog, and release-map changes. These should not travel with a CDP attach fix.
Suggested changes
- Salvage the fallback into the current shared CLI/TUI connection flow, with tests for
404 + DevToolsActivePorton both surfaces. - Guard invalid
parsed.portaccess and add the regression test. - Keep the browser fix scoped to browser files/tests.
This is an automated hermes-sweeper review.
| return "" | ||
|
|
||
| host = parsed.hostname or "" | ||
| port = parsed.port |
There was a problem hiding this comment.
parsed.port can raise ValueError for a malformed port (for example, http://127.0.0.1:abc), but it is outside the surrounding try. Please catch that case and return the normal empty fallback so an invalid configured endpoint cannot turn discovery failure into an uncaught exception.
| detector for local endpoints. | ||
| """ | ||
| cfg = get_provider_stale_timeout(self.provider, self.model) | ||
| cfg = get_provider_stale_timeout( |
There was a problem hiding this comment.
This stale-timeout change is unrelated to local Chrome CDP discovery. Please split it from this browser-focused PR so the CDP fix can be evaluated and salvaged independently.
Summary
DevToolsActivePortwhen local Chrome exposes a debug port but/json/versionis unavailable/browser connectvalidate the resolved CDP endpoint before reporting successRoot cause
Recent Chrome built-in remote debugging flows can leave port
9222open and write a real browser websocket toDevToolsActivePortwhile returning404from classic discovery endpoints like/json/version. Hermes treated a listening port as success and relied too heavily on/json/version, so/browser connectcould report success even though later CDP usage still failed.Validation
source .venv-ci/bin/activate && python -m pytest tests/tools/test_browser_cdp_override.py tests/tools/test_browser_cdp_tool.py tests/tools/test_browser_cloud_fallback.py tests/cli/test_cli_browser_connect.py -q40 passedsource .venv-ci/bin/activate && python -m pytest tests/ -q -n 0tests/gateway/*,tests/hermes_cli/test_gateway_wsl.py,tests/hermes_cli/test_gemini_provider.py, andtests/run_agent/test_*interrupt*