Repository navigation
Conversation
A Chrome that had debugging enabled at runtime, through the toggle on chrome://inspect/#remote-debugging, serves a different transport than one started with --remote-debugging-port. Measured on Chrome 153: the browser socket upgrades (101), a per-target socket is refused (403), and /json/version and /json/list both 404. That is what a user gets when they attach to the browser they were already using rather than relaunching it, and the toolkit could not reach it at all. Discovery now falls back to Target.getTargets over that one socket, and each call reaches its page through a flat Target.attachToTarget session on it. The HTTP path is unchanged: listTargets issues its original GET /json/list first and unconditionally, and only a request that did not answer reaches the fallback, so there is no added round-trip and no added failure mode. The one-target-per-call guarantee holds. Target.getTargets is metadata-only and attaches to nothing, so a 200-tab browser costs one round trip rather than one attach per tab (measured: 201 pages, 411 targets, 0.03s). The per-command timeout is bounded per command id rather than per socket, so sharing the socket does not let one stuck tab block the rest — browser-ws:wedge blackholes a tab and shows it rejecting at the 5s bound while a witness tab, list_pages, and a freshly opened tab all answer in under 1ms on that same socket. Nothing is auto-discovered. A runtime-toggled Chrome raises a modal consent prompt for each new client, granting full access to a logged-in session, so the browser is named explicitly with CDP_BROWSER_WS or CDP_USER_DATA_DIR and the default profile is never scanned. Co-Authored-By: Claude Opus 5 <[email protected]>
Three problems review found in the new code: detectEndpoint cached the promise, not the result, so the first attempt losing a race with a still-starting Chrome left every later call holding that same rejection — including listTargets' fallback, which would then report a dead endpoint for the life of the process. Evict on rejection; a resolved detection is still cached exactly once. The smoke test's fail() called process.exit, which does not unwind, so the catch that kills Chrome and removes the mkdtemp profile never ran. Every assertion failure leaked a browser and a profile directory. Throw instead and let the existing handler do its job. The wedge test waited the full 30s for an endpoint Chrome would never print if it had already died. Reject on exit, as the smoke test does. Co-Authored-By: Claude Opus 5 <[email protected]>
…r runs The handshake is bounded because a dead port would otherwise stall the whole detection, but the /json/version fetch right below it was not, so a host that accepts TCP and never answers hung detectEndpoint — and with it listTargets' fallback. It now settles in 5s. The failure message said no socket was found in "the default Chrome profile(s)", asserting a search the module deliberately never performs: scanning a default profile is what raises Chrome's consent dialog on the user's screen. Say what actually happened instead. Also in the fixtures: the proxy's open() awaited a promise only onopen resolved, so a failed upstream handshake queued client messages forever; the smoke test computed the page-socket probe but never asserted on it, so a proxy that upgraded /devtools/page/* would have passed the very check that establishes the endpoint is browser-ws-only; and freePort was dead code. Co-Authored-By: Claude Opus 5 <[email protected]>
prxref automated review: ApprovedPR: Reach a Chrome that serves only the browser WebSocket (from #9) · files reviewed: 17 🟥 0 error · 🟧 7 warning · 🔍 0 spec · ⬜ 1 outofscope
Inline comments: 5 of 8 findings (3 anchors rejected by the forge). Reviewed by prxref · model=z-ai/glm-5.3-flash · 48373 tok · 42.8s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| const profile = await mkdtemp(join(tmpdir(), "cdp-browser-ws-smoke-")); |
There was a problem hiding this comment.
🤖 🟧 [WARNING] Chrome and profile leak when startup fails before the try block (test/browser-ws-smoke.ts:50)
The await of the upstreamWs promise (lines 65-77) and the proxy start (line 81) run before the try at line 90, and cleanup() is only called inside try/catch. If Chrome exits early or the 30s endpoint timeout rejects, the top-level await throws with no cleanup: the spawned Chrome (line 50) keeps running and the mkdtemp profile (line 49) is never removed.
Reviewed by prxref · model=z-ai/glm-5.3-flash
| }); | ||
| chrome.on("exit", (code) => reject(new Error(`Chrome exited (${code}) before reporting an endpoint`))); | ||
| }); | ||
| const proxy = await startBrowserWsOnlyProxy(`http://${new URL(upstreamWs).host}`); |
There was a problem hiding this comment.
🤖 🟧 [WARNING] Chrome and profile leak when startup fails before the try/finally (test/browser-ws-wedge.ts:87)
The upstreamWs await (lines 74-86) and proxy start (line 87) precede the try/finally, whose finally is the only place chrome.kill/rm happen. A Chrome startup failure (exit or 30s timeout) rejects the top-level await before finally exists, leaking the spawned Chrome process and the temp profile from line 60.
Reviewed by prxref · model=z-ai/glm-5.3-flash
| * and a dead port would otherwise stall the whole detection on a connect that | ||
| * never resolves. | ||
| */ | ||
| function handshake(url: string, timeoutMs: number): Promise<boolean> { |
There was a problem hiding this comment.
🤖 🟧 [WARNING] Raw WebSocket opened outside client.ts primitives (src/cdp/endpoint.ts:107)
Spec: "Build on src/client.ts primitives: never open a WebSocket yourself." (CONTRIBUTING.md:20). handshake constructs new WebSocket(url) directly instead of going through CdpConnection/openBrowser, the only in-repo standards document provided for this chunk forbids opening a WebSocket outside client.ts.
Reviewed by prxref · model=z-ai/glm-5.3-flash
| // lookup came up empty. | ||
| throw httpError; | ||
| }); | ||
| knownTransport = "browser-ws"; |
There was a problem hiding this comment.
🤖 🟧 [WARNING] Transient /json/list failure permanently latches browser-ws transport (src/client.ts:277)
Any single /json/list failure (e.g. a transient 5xx or network blip on a --remote-debugging-port Chrome) falls into getTargetsOverBrowserWs, which can succeed over the browser socket, and then sets knownTransport = "browser-ws" for the process lifetime. From then on every listing skips /json/list and reports empty webSocketDebuggerUrl, silently switching openPage and all callers to session transport after a one-off error.
Reviewed by prxref · model=z-ai/glm-5.3-flash
| * Resolved at connect time for that reason, never memoized across a restart. | ||
| */ | ||
| import { readFile } from "node:fs/promises"; | ||
| import { homedir } from "node:os"; |
There was a problem hiding this comment.
🤖 ⬜ [OUTOFSCOPE] Unused homedir import (src/cdp/endpoint.ts:45)
import { homedir } from "node:os"; is added but nothing in the module references it — profile lookup reads CDP_USER_DATA_DIR directly. Dead import under a strict-tsc repo rule.
Reviewed by prxref · model=z-ai/glm-5.3-flash
Carries over #9 by @srizzo unchanged: same three commits, same tree, his authorship kept.
#9 had to be re-opened from this repo. This repo's history was rewritten, which moved
mainunder the PR, and GitHub does not let maintainers push history that touches workflow files into a fork branch. Full description, measurements and review discussion are on #9.Co-authored-by: Samuel Rizzo [email protected]