Repository navigation
feat(browser): attach passive tools to browsers without HTTP discovery - #2346
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
948a0ab to
fbac6ad
Compare
|
Update (head
The scope decision in #2288 is still pending; this should not merge until it covers the final diff. |
fbac6ad to
e245af5
Compare
|
Rebased to resolve the conflict with #2091 (head
|
|
@greptileai review |
|
This PR is set for a Plus review. TREX isn't supported with Plus or Apex reviews yet, so Greptile reviews this PR at Base while TREX is on. |
|
Accept a literal loopback browser WebSocket, ws://127.0.0.1:PORT/devtools/browser/ID, as the cdp_endpoint of the six passive browser tools. Discovery then reads Browser.getVersion and Target.getTargets over that socket, and the target session adopts the same socket, so a browser that asks for consent per connection prompts once per call. The socket identifier gates access, so Evidence records the endpoint as ws://127.0.0.1:PORT/devtools/browser/[REDACTED], and runtime reconciliation accepts that recorded form. HTTP endpoints and their Evidence are unchanged. Electron, web runtime, scenario and Inspector endpoints stay HTTP-only. Refs morluto#2288
Check in the real Chrome lane that DevToolsActivePort names the same browser socket as /json/version, and that inspecting the selected page through that socket leaves DOM structure and scripts unchanged compared with an inspection through HTTP discovery. Refs morluto#2288
e245af5 to
4706bb9
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Rebased on
The scope decision in #2288 is still open; this should not merge until it covers the final diff. |
…eouts Run the browser-socket WebSocket handshake and both discovery commands under one discovery deadline, so a port that accepts the connection but never completes the upgrade times out instead of waiting forever. Report a reached deadline as a timeout without carrying the internal abort as its cause, so CLI and MCP project provider_timeout rather than cancelled. Caller cancellation is still reported as cancelled. Refs morluto#2288
|
Addressed Greptile's review in
New tests cover a stalled reply and a handshake that never completes, including the projected error code. |
Summary
Implements option 2 from #2288. The passive browser tools can now attach to a browser that serves a loopback browser WebSocket but no HTTP discovery. Before this change, every passive call read
/json/versionand/json/listfirst, so such a browser failed withendpoint_unreachable.The maintainer scope decision for #2288 is still pending. This PR is open for review, but per CONTRIBUTING it should not be merged until that decision covers it. The proposal and evidence are in this comment.
Problem and expected behavior
Since Chrome 136,
--remote-debugging-portis ignored for the default profile (Chrome blog). Chrome 144+ can instead enable remote debugging on a running browser fromchrome://inspect/#remote-debugging(Chrome blog). A third-party report (trycua/cua#2916, Chrome 151) says this mode writes the port and a/devtools/browser/<id>path toDevToolsActivePortand returns 404 for/json/version.Expected: a caller can pass
ws://127.0.0.1:PORT/devtools/browser/<id>and get the same targets and inspection results as HTTP discovery.Change and scope
cdp_endpointon the six passive tools also acceptsws://127.0.0.1:PORT/devtools/browser/<id>orws://[::1]:PORT/.... The port must be explicit, the id one path segment, and credentials, query and fragment are rejected. The HTTP form is unchanged.localhost, LAN,wss:and/devtools/page/sockets stay invalid. The domain parses the endpoint once into{ transport, url, recorded }.list_browser_targets,inspect_web_page,analyze_web_bundle,observe_web_session,discover_webmcp_tools,capture_web_screenshot. Electron, web runtime, scenario attach and V8 Inspector endpoints keepbrowserEndpointSchemaand stay HTTP-only.Browser.getVersionandTarget.getTargets, then the target session adopts the same socket forTarget.attachToTarget(flat). Consent is reportedly requested per WebSocket upgrade, so a second socket would prompt twice. Unadopted sockets are closed on failure and on cancellation.ws://127.0.0.1:PORT/devtools/browser/[REDACTED]. JavaScript runtime reconciliation accepts that recorded form for browser captures. HTTP-endpoint Evidence is unchanged.CdpBrowserProviderrepeated the same discover → authorize → session → cleanup block in five operations. All five needed the new transport, so they now share onewithTargetSessionhelper.Not included: reading
DevToolsActivePortfor the caller, a pipe transport (option 3), or anything that launches a browser. #2287 separately documents option 1.Follow the contribution scope. The scope decision is pending in #2288.
Contract and boundary impact
browserObservationEndpointSchemainsrc/domain/browserObservation.ts; transport selection insrc/browser/CdpTargetSession.ts.cdp_endpointform. The MCP description and CLIendpointhelp name it. The advertised JSON Schema type is unchanged (string).browser.revisionisBrowser.getVersion's value (@<hash>) instead ofWebKit-Version(537.36 (@<hash>)).attachedis a boolean where Chrome's/json/listomits it and HTTP discovery reportsnull. Both are documented.parameters.cdp_endpointis the redacted socket URL for socket calls; unchanged for HTTP calls.Expected effects:
The attach is the existing passive CDP attach, over a different loopback transport.
Evidence and regression coverage
Tests added or updated:
src/domain/browserObservation.test.ts: accepted socket forms, recorded redaction, and rejected shapes (implicit port,localhost,wss:, credentials, query, fragment, empty or nested id, page socket).tests/boundary/browser/cdpBrowserProvider.browserSocket.test.ts: socket discovery matches HTTP discovery; inspection uses one socket and makes no HTTP requests; the socket closes on a missing target, malformedBrowser.getVersion, and cancellation; a refused socket isendpoint_unreachablewithout echoing the id.tests/boundary/cli/browserCli.test.ts,tests/boundary/mcp/browserMcp.test.ts: public CLI/MCP calls with a socket record the redacted endpoint. MCP reconciliation accepts that Evidence, and the advertised description names the socket form.scripts/lib/browser-socket-e2e.mjs, wired intonpm run verify:browser. On real Chrome it checks thatDevToolsActivePortnames the same socket as/json/version, and that socket discovery matches HTTP discovery. It also checks that a public CLI inspection through the socket records redacted Evidence and comparesunchangedfor DOM structure and scripts against an HTTP-discovery inspection.Browser.getVersionwith Chrome's reply shape and counts opened sockets.Issue success check ([Design] Attaching to Chromium browsers that expose no HTTP discovery endpoint #2288, option 2), run manually on the notes-web demo (
website/public/examples/notes-web/served on127.0.0.1). Chrome 138.0.7204.157 was started with an explicit--remote-debugging-portand a dedicated--user-data-dir. Only/json/version'swebSocketDebuggerUrlwas passed to the compiled CLI, and the result was compared with HTTP discovery:list-browser-targets: the same target, title, URL and origin. The only difference isattached(nullthrough HTTP, because Chrome's/json/listomits it;falsethrough the socket).inspect-web-page --include-script-sources --observation-ms 1000: identical apart fromtarget.attached. Both returned the same three scripts (assets/main.js,assets/i18n.js,examples/notes-web/export.js) with identical source SHA-256 digests, 72 DOM nodes, an emptynetwork.requests, and the sameattach_limited_sections.compare-web-captures(HTTP vs socket):dom_structure,scriptsandresourcesareunchanged.network,metadata,webmcp,accessibilityandstorageareunknown, as for any two attach-window captures.http://127.0.0.1:<port>andws://127.0.0.1:<port>/devtools/browser/[REDACTED]. The socket id appears in no CLI output.Base reproduction: on
upstream/main,rea list-browser-targets ws://127.0.0.1:PORT/devtools/browser/<id>fails input validation, becausecdp_endpointmust be an explicit-port HTTP URL. Without HTTP discovery, no accepted endpoint exists.User-visible CLI/MCP output: real Chrome 138,
rea list-browser-targets ws://127.0.0.1:<port>/devtools/browser/<id> --allowed-origins http://127.0.0.1:<site> --json(trimmed):{ "operation": "list_browser_targets", "parameters": { "cdp_endpoint": "ws://127.0.0.1:<port>/devtools/browser/[REDACTED]", "allowed_origins": ["http://127.0.0.1:<site>"] }, "normalized_result": { "browser": { "product": "Chrome/138.0.7204.157", "protocol_version": "1.3", "revision": "@e533e98b1267baa1f1c46d666b120e64e5146aa9", "js_version": "13.8.258.26" }, "targets": [ { "type": "page", "title": "REA browser verifier", "url": "http://127.0.0.1:<site>/app", "origin": "http://127.0.0.1:<site>", "attached": false } ], "excluded": { "disallowed_origin": 0, "unsupported_url": 0, "non_page": 4 } } }The socket id does not appear anywhere in stdout.
Remaining proof gaps:
chrome://inspecttoggle itself; the local Chrome is 138. The socket-only behavior and per-connection consent come from the third-party report above.For evidence-bearing changes:
Validation performed
616f8f84d): the browser-socket WebSocket handshake now runs inside the 10-second discovery deadline, and a reached deadline projects asprovider_timeoutrather thancancelled. New boundary tests cover a stalled discovery reply and a handshake that never completes.check:fastpasses;tests/boundary/browserplus the browser CLI and MCP suites pass (45 files, 417 tests, 1 skipped).4276aaf18(head4706bb9f4). The only conflict was with #951b095e1, which bounded HTTP discovery responses to 8 MiB and 10 seconds. Browser-socket discovery now shares that 10-second deadline (timeoutreason, caller cancellation preserved, boundary test added). Each socket reply stays bounded by the 64 MiB protocol message limit. After the rebase:check:fastpasses; alltests/boundary/browserfiles plus the browser/Electron CLI and MCP, Inspector and reconciliation suites pass (52 files, 467 tests, 1 skipped); the domain tests pass (6).npm run check: typecheck, lint (0 errors), knip,metadata:checkand architecture guards pass.format:checkflagged only untracked local build output outside this diff;oxfmt --checkon the 22 changed files passes.npm run docs:check: passes.npm run test:focused --alltests/boundary/browser/*.test.ts, pluscli/browserCli,cli/electronCli,cli/browserRedirectShapeCompleteness,mcp/browserMcp,mcp/electronMcp,mcp/browserComparisonInputandinspector/javascriptRuntimeObservation: 48 files, 431 tests pass.npm run test:focused --the two JavaScript runtime reconciliation boundary suites: 13 tests pass.npm run test:local --25 related browser, web, contract and Evidence source suites: 255 tests pass.REA_BROWSER_EXECUTABLE=/usr/bin/google-chrome npm run verify:browser(Chrome 138.0.7204.157, headless, Linux): the new browser-socket step passes. The run then stops in the existing scenario launch-storage step (browser-scenario-storage-e2e.mjs: expected seeded storage values, gotnull). Unmodifiedupstream/mainfails with the same assertion on this host, so the steps after it ran on neither revision. Run alone against the same Chrome, the browser-socket step returned{"targets":1,"revision":"@e533e98b1267baa1f1c46d666b120e64e5146aa9"}. Re-run after the capture-parity assertions were added: the step passes, and the lane stops at the same existing storage assertion.Compatibility, safety, and release
npm run verify:browser.tools/listpayload grows from 2,462,734 to 2,463,786 bytes (+1,052, about 0.04%), from thecdp_endpointdescription on the six tools (14 schema occurrences). Tokens were not measured; at about 4 bytes per token that is roughly 260 tokens, an estimate. HTTP discovery calls run the same code path as before. Elapsed time, CPU and memory for the socket path were not benchmarked.--allowed-originswhen listing targets. Chrome's own consent dialog still applies.Before merge
type(scope): outcome.Refs #2288