feat: CDP attach mode — observe any DevTools-protocol browser - #6
Conversation
📝 WalkthroughWalkthroughChangesCDP attach mode
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer
participant CDPBrowser
participant NavigationHandoff
User->>Renderer: attach to endpoint
Renderer->>CDPBrowser: discover and connect
CDPBrowser-->>Renderer: targets, frames, and events
Renderer-->>User: render frame and display logs
User->>Renderer: click or navigate
Renderer->>CDPBrowser: dispatch input or navigation
User->>NavigationHandoff: open URL
NavigationHandoff-->>Renderer: provide URL handoff
Renderer->>CDPBrowser: navigate attached page
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
bin/cdp.mjs (2)
456-463: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
restartScreencastwhen no target is pinned.The staleness watchdog can call
restartScreencast()afterTarget.targetDestroyedclearedpageSessionId. Both calls then go out browser-level, andPage.startScreencastrejects out of the function. Return early instead.♻️ Proposed change
async restartScreencast() { + if (!session || !pageSessionId) return; try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/cdp.mjs` around lines 456 - 463, Update restartScreencast to return immediately when pageSessionId is unset, before sending Page.stopScreencast or calling startScreencast; retain the existing restart behavior when a target is pinned.
230-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
close()leaves pending requests to their own timeouts.
close()setsdead = truefirst. The resultingonclosecallsfailAll, which returns immediately becausedeadis already true. Every in-flightsendthen waits for its own timer, up to 10 s, and those timers hold the event loop open. Reject the pending requests directly.♻️ Proposed change
close() { - dead = true; + failAll("connection closed"); try { ws.close(); } catch { /* already closed */ } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/cdp.mjs` around lines 230 - 237, Update close() to reject all pending requests directly before or while marking the connection dead, rather than relying on onclose/failAll after dead is set. Ensure each in-flight send is settled immediately and its timeout is cleared, while preserving the existing ws.close() attempt and already-closed handling.docs/plans/2026-08-04-001-feat-cdp-attach-mode-plan.md (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: silence MD038 on the
✖prefix code spans.markdownlint flags the trailing space inside the code spans on Line 36 and Line 62. Keep the visible space by padding both sides of the span content.
♻️ Proposed change
-- R8. Log-source network failures render with the same `✖ ` prefix and pass through the same recent-window dedupe map as the polling feed, so a retry loop paints once in either mode. Log entries carry text + URL, not structured method/status — lines show Chrome's error text; no fake method/status parity is synthesized. +- R8. Log-source network failures render with the same `` `✖ ` `` prefix and pass through the same recent-window dedupe map as the polling feed, so a retry loop paints once in either mode. Log entries carry text + URL, not structured method/status — lines show Chrome's error text; no fake method/status parity is synthesized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-08-04-001-feat-cdp-attach-mode-plan.md` at line 36, Update the inline code spans containing the `✖ ` prefix in the plan so the trailing visible space is preserved while avoiding MD038 by padding the span content on both sides. Apply the same formatting adjustment to both occurrences referenced by the comment, including the network-failure entry and the other matching occurrence.Source: Linters/SAST tools
tests/cdp.test.mjs (1)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
https://endpoint path and for repeatedclose().The suite asserts junk and
file:rejection, but nothttps://. That is exactly the schemediscoverEndpointaccepts and then dials over cleartextnode:http. The plan also listsclose()idempotency as a U1 scenario, and no test asserts it.💚 Proposed test additions
test("discoverEndpoint rejects junk and non-endpoint schemes", async () => { await assert.rejects(discoverEndpoint("not a url"), /endpoint URL/); await assert.rejects(discoverEndpoint("file:///etc/passwd"), /endpoint URL/); + // https cannot be dialed by the node:http discovery path. + await assert.rejects(discoverEndpoint("https://127.0.0.1:9222"), /endpoint URL/); });test("close is idempotent", async () => { const ws = makeFakeWs(); const s = makeCdpSession("ws://x/devtools/browser/1", { wsFactory: () => ws }); ws.open(); await s.opened; let closes = 0; s.onClose(() => closes++); s.close(); s.close(); assert.equal(s.dead, true); assert.equal(closes, 1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cdp.test.mjs` around lines 86 - 89, Add tests in the existing cdp test suite for the accepted https:// discoverEndpoint path, verifying it proceeds to the endpoint connection behavior rather than being rejected, and add a close-idempotency test around makeCdpSession that calls close() twice and asserts dead is true with the close callback invoked once.tests/renderer.test.mjs (1)
1979-1988: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage that sets the render mode before the attach tick.
Every attach test drives
tick()on a freshly constructed Renderer, sothis.modestill holds"attach". The real pane assignsthis.modefrompickRenderMode()inrun()first. Setr.mode = "symbols"beforeawait r.tick()in one test to pin the backend gate independently of the render mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/renderer.test.mjs` around lines 1979 - 1988, Update the attach-mode test around attachRenderer and tick to assign r.mode = "symbols" before the first await r.tick(). Keep the existing assertions and liveness-check behavior unchanged.README.md (1)
226-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso list the new keys in the Configuration tables.
This section introduces
cdp-urlandHERDR_BROWSER_CDP_URL. The Configuration tables (lines 314-328) still list onlysessionandrender. Add one row to each table, so readers who go to Configuration find the attach-mode keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 226 - 233, Update the Configuration tables in README.md to add rows for the newly introduced cdp-url configuration key and HERDR_BROWSER_CDP_URL environment variable, alongside the existing session and render entries. Use the same descriptions and formatting conventions as the surrounding tables.bin/renderer.mjs (1)
750-758: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the same endpoint validation at startup as at runtime.
attachTorejects any value that does not match/^(wss?|https?):\/\//i.resolveCdpEndpointaccepts any non-empty string, soHERDR_BROWSER_CDP_URL=localhost:9222selects attach mode with an endpoint the prompt would refuse. The pane then reports only a connect failure. Extract one validator and use it in both places, so a malformed configuration reports the same clear message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/renderer.mjs` around lines 750 - 758, Extract the endpoint scheme validation currently enforced by attachTo into a shared validator, and call it from both attachTo and resolveCdpEndpoint. Ensure startup rejects non-empty CDP URLs that do not match the accepted http, https, ws, or wss scheme pattern, producing the same clear validation message as runtime.scripts/open.sh (1)
26-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate the URL once before the branch.
Lines 27-30 and 38-41 repeat the same check and the same message. Move the
validate_urlguard above theif, so the two delivery paths cannot drift.♻️ Proposed refactor
+if [ -n "$url" ] && ! validate_url "$url"; then + echo "herdr-browser: refusing URL (must start with http:// or https://, no credentials): $url" >&2 + exit 2 +fi + if [ -n "$url" ] && [ -n "$cdp_endpoint" ]; then - if ! validate_url "$url"; then - echo "herdr-browser: refusing URL (must start with http:// or https://, no credentials): $url" >&2 - exit 2 - fi # Attach mode: hand the URL to the pane, which navigates the attached # target. The renderer watches this file, so pickup does not wait for a # backed-off poll tick. handoff="$(state_dir)/navigate-$(ws_id)" umask 077 printf '%s\n' "$url" > "$handoff" elif [ -n "$url" ]; then - if ! validate_url "$url"; then - echo "herdr-browser: refusing URL (must start with http:// or https://, no credentials): $url" >&2 - exit 2 - fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/open.sh` around lines 26 - 41, Move the validate_url guard and its refusal message in scripts/open.sh above the if [ -n "$url" ] && [ -n "$cdp_endpoint" ] branch, applying it once whenever url is provided; leave the attach and non-attach delivery paths unchanged after removing their duplicate checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/cdp.mjs`:
- Around line 420-430: Update connect() to close and discard any existing
session before creating a replacement, and reset pinnedTargetId before
reconnection so failed attempts cannot retain a dead target. Capture the newly
created session in the onClose callback and emit endpoint_gone only when that
callback still belongs to the active session, preventing stale sockets from
affecting the new connection.
- Around line 88-96: Update discoverEndpoint to reject https: URLs, allowing
only the supported http: scheme before resolving the host and calling getJson.
Preserve the existing HTTP discovery behavior and validation for other
unsupported protocols.
In `@bin/renderer.mjs`:
- Around line 876-884: Update the frame branch in the frame handler to reset the
stale-frame latch and remove the stale banner whenever a new frame arrives.
Preserve the existing lastFrameAt update, metadata handling, stream
notification, and frame acknowledgment flow.
- Around line 1444-1449: Update the key allowlist in the unattached input
handling near the switch containing the "a" case to retain the "a" key alongside
"u", "q", and "\x03". Preserve the existing filtering behavior for all other
keys so users can invoke attachTo when streamCooldownUntil prevents rediscovery.
- Around line 561-573: Separate backend identity from render mode in the
renderer initialization around `this.cdpEndpoint`, `this.mode`, `this.browser`,
and `this.backendName`; introduce or reuse a dedicated backend symbol and update
attach-specific navigation, staleness, click-scaling, cleanup, `attachTo()`, and
`renderImage()` checks to use it. Keep `this.mode` exclusively for
`pickRenderMode()` results so Kitty output and CDP behavior remain correct after
render-mode selection, and add a regression test that selects a render mode
before `tick()`.
- Around line 918-928: Update pushLogEntry to prune stale entries from
networkState.recent before or while handling network log entries, removing
records older than the existing 60-second deduplication window. Keep the current
key-based suppression behavior intact, including the immediate return for
recently seen entries.
In `@README.md`:
- Around line 254-257: Update the README section describing the console feed so
it no longer instructs users to set the inaccessible consoleTier option.
Describe the current behavior as implemented by makeCdpBrowser(endpoint), or
expose a supported configuration path before documenting user configuration;
keep the guidance consistent with the renderer’s existing invocation.
In `@scripts/open.sh`:
- Around line 34-36: Update the handoff-file write in the open flow to use a
temporary file in the same state directory, write the URL with the existing
restrictive umask, then atomically rename it to $handoff. Ensure the renderer
only observes the completed file and clean up the temporary file if writing
fails.
---
Nitpick comments:
In `@bin/cdp.mjs`:
- Around line 456-463: Update restartScreencast to return immediately when
pageSessionId is unset, before sending Page.stopScreencast or calling
startScreencast; retain the existing restart behavior when a target is pinned.
- Around line 230-237: Update close() to reject all pending requests directly
before or while marking the connection dead, rather than relying on
onclose/failAll after dead is set. Ensure each in-flight send is settled
immediately and its timeout is cleared, while preserving the existing ws.close()
attempt and already-closed handling.
In `@bin/renderer.mjs`:
- Around line 750-758: Extract the endpoint scheme validation currently enforced
by attachTo into a shared validator, and call it from both attachTo and
resolveCdpEndpoint. Ensure startup rejects non-empty CDP URLs that do not match
the accepted http, https, ws, or wss scheme pattern, producing the same clear
validation message as runtime.
In `@docs/plans/2026-08-04-001-feat-cdp-attach-mode-plan.md`:
- Line 36: Update the inline code spans containing the `✖ ` prefix in the plan
so the trailing visible space is preserved while avoiding MD038 by padding the
span content on both sides. Apply the same formatting adjustment to both
occurrences referenced by the comment, including the network-failure entry and
the other matching occurrence.
In `@README.md`:
- Around line 226-233: Update the Configuration tables in README.md to add rows
for the newly introduced cdp-url configuration key and HERDR_BROWSER_CDP_URL
environment variable, alongside the existing session and render entries. Use the
same descriptions and formatting conventions as the surrounding tables.
In `@scripts/open.sh`:
- Around line 26-41: Move the validate_url guard and its refusal message in
scripts/open.sh above the if [ -n "$url" ] && [ -n "$cdp_endpoint" ] branch,
applying it once whenever url is provided; leave the attach and non-attach
delivery paths unchanged after removing their duplicate checks.
In `@tests/cdp.test.mjs`:
- Around line 86-89: Add tests in the existing cdp test suite for the accepted
https:// discoverEndpoint path, verifying it proceeds to the endpoint connection
behavior rather than being rejected, and add a close-idempotency test around
makeCdpSession that calls close() twice and asserts dead is true with the close
callback invoked once.
In `@tests/renderer.test.mjs`:
- Around line 1979-1988: Update the attach-mode test around attachRenderer and
tick to assign r.mode = "symbols" before the first await r.tick(). Keep the
existing assertions and liveness-check behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 93f010f3-d6d3-47a8-bc2b-ab9bff286d9b
📒 Files selected for processing (7)
README.mdbin/cdp.mjsbin/renderer.mjsdocs/plans/2026-08-04-001-feat-cdp-attach-mode-plan.mdscripts/open.shtests/cdp.test.mjstests/renderer.test.mjs
| if (u.protocol !== "http:" && u.protocol !== "https:") | ||
| throw new Error("not an endpoint URL — use http://host:port or ws://…"); | ||
| let host = u.hostname.replace(/^\[|\]$/g, ""); | ||
| if (!net.isIP(host)) { | ||
| const resolve = lookup ?? (await import("node:dns/promises")).lookup; | ||
| host = (await resolve(host)).address; | ||
| } | ||
| const port = u.port || "9222"; | ||
| const version = await getJson(host, port, "/json/version"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js node:http request https URL TLS not supported node:https
💡 Result:
In Node.js, the node:http module does not support TLS/HTTPS by default [1]. To make requests to an https:// URL, you must use the node:https module [2][3]. If you attempt to use node:http to connect to an https:// URL, the request will fail because node:http lacks the TLS/SSL capabilities required to perform the handshake [4]. You must explicitly use https.request or https.get, which are designed to wrap the HTTP request within a TLS connection [2][4][5]. Additionally, it is possible for a Node.js binary to be built without support for the node:crypto module [2][6]. If your Node.js environment is built this way, any attempt to import or use the node:https module will throw an error because the underlying cryptographic functions required for TLS are unavailable [2][7]. In such cases, you will not be able to perform HTTPS requests using the built-in module [2]. To correctly perform an HTTPS request, use the following approach: const https = require('node:https'); const options = { hostname: 'example.com', port: 443, path: '/', method: 'GET' }; const req = https.request(options, (res) => { // handle response }); req.on('error', (e) => { console.error(e); }); req.end; If you are working in an environment where node:https might be unavailable, you can use a try-catch block or dynamic import to handle the potential error gracefully [2][6].
Citations:
- 1: https://nodejs.org/api/http.html
- 2: https://nodejs.org/api/https.html
- 3: https://nodejs.org/docs/latest-v23.x/api/https.html
- 4: https://www.thenodebook.com/http2-tls/https-servers-clients
- 5: https://nodejs.org/api/https.md
- 6: https://github.com/nodejs/node/blob/main/doc/api/https.md
- 7: https://nodejs.org/dist/latest/docs/api/https.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cdp outline ---'
ast-grep outline bin/cdp.mjs --match getJson --view expanded || true
printf '%s\n' '--- cdp lines 1-130 ---'
cat -n bin/cdp.mjs | sed -n '1,130p'
printf '%s\n' '--- getJson and endpoint references ---'
rg -n -C 3 'getJson|protocol !==|webSocketDebuggerUrl|endpoint URL|node:http|node:https|new URL' --glob '!node_modules' .Repository: StructuPath/herdr-browser
Length of output: 19898
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cdp tests ---'
cat -n tests/cdp.test.mjs | sed -n '1,115p'
printf '%s\n' '--- endpoint scheme references ---'
rg -n -i -C 2 'https://|wss://|http://host:port|CDP_URL|cdp-url' \
README.md docs bin scripts tests --glob '!node_modules' || true
printf '%s\n' '--- read-only scheme/default-port probe ---'
python3 - <<'PY'
from pathlib import Path
import re
source = Path("bin/cdp.mjs").read_text()
get_json = re.search(r"function getJson\(.*?\n\}", source, re.S)
discover = re.search(r"export async function discoverEndpoint\(.*", source, re.S)
assert get_json and discover
get_json_text = get_json.group(0)
discover_text = discover.group(0)
print("getJson uses node:http:", "http.request(" in get_json_text)
print("getJson uses node:https:", "https.request(" in get_json_text)
print("discover accepts https:", 'u.protocol !== "http:" && u.protocol !== "https:"' in discover_text)
print("default port expression:", re.search(r'const port = .*', discover_text).group(0))
print("scheme is passed to getJson:", bool(re.search(r"getJson\(\s*(?:u\.protocol|scheme)", discover_text)))
PYRepository: StructuPath/herdr-browser
Length of output: 36363
Reject https: CDP discovery endpoints. discoverEndpoint accepts https: but getJson always uses node:http and ignores the scheme. This sends cleartext HTTP and uses port 9222 by default. Reject https: or add node:https support with port 443.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/cdp.mjs` around lines 88 - 96, Update discoverEndpoint to reject https:
URLs, allowing only the supported http: scheme before resolving the host and
calling getJson. Preserve the existing HTTP discovery behavior and validation
for other unsupported protocols.
| async connect() { | ||
| endpoint = await discoverEndpoint(endpointInput, opts); | ||
| session = makeCdpSession(endpoint.wsUrl, opts); | ||
| await session.opened; | ||
| session.onEvent(onCdpEvent); | ||
| session.onClose(() => emit({ type: "endpoint_gone" })); | ||
| await session.send("Target.setDiscoverTargets", { discover: true }); | ||
| const pages = await pageTargets(); | ||
| if (!pages.length) throw new Error("endpoint has no page targets"); | ||
| attachTimeMs = Date.now(); | ||
| await pinTarget(pages[0].targetId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reattach leaks the previous session and lets it emit endpoint_gone against the new one.
connect() overwrites session without closing the previous one. The renderer calls attachCdp() again after sessionExists() returns false (bin/renderer.mjs:1197-1211), and a ping timeout does not close a half-open socket. Two effects follow:
- The old WebSocket and its timers stay alive for the process lifetime.
- When the old socket finally closes, its
onClosehandler from Line 425 still runs and emitsendpoint_gone, so the renderer reports endpoint loss while the new session is healthy.
Close the previous session and bind the onClose emit to the session that registered it. Reset the pinned-target state as well, because a failed connect() otherwise leaves pinnedTargetId pointing at a dead target.
🐛 Proposed fix
async connect() {
+ // A reattach must not leave the previous socket alive: its close
+ // event would otherwise report endpoint loss for the new session.
+ if (session) {
+ try {
+ session.close();
+ } catch {
+ /* already gone */
+ }
+ }
+ pinnedTargetId = null;
+ pageSessionId = null;
+ lastMeta = null;
endpoint = await discoverEndpoint(endpointInput, opts);
session = makeCdpSession(endpoint.wsUrl, opts);
await session.opened;
- session.onEvent(onCdpEvent);
- session.onClose(() => emit({ type: "endpoint_gone" }));
+ const mine = session;
+ session.onEvent(onCdpEvent);
+ session.onClose(() => {
+ if (session === mine) emit({ type: "endpoint_gone" });
+ });
await session.send("Target.setDiscoverTargets", { discover: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async connect() { | |
| endpoint = await discoverEndpoint(endpointInput, opts); | |
| session = makeCdpSession(endpoint.wsUrl, opts); | |
| await session.opened; | |
| session.onEvent(onCdpEvent); | |
| session.onClose(() => emit({ type: "endpoint_gone" })); | |
| await session.send("Target.setDiscoverTargets", { discover: true }); | |
| const pages = await pageTargets(); | |
| if (!pages.length) throw new Error("endpoint has no page targets"); | |
| attachTimeMs = Date.now(); | |
| await pinTarget(pages[0].targetId); | |
| async connect() { | |
| // A reattach must not leave the previous socket alive: its close | |
| // event would otherwise report endpoint loss for the new session. | |
| if (session) { | |
| try { | |
| session.close(); | |
| } catch { | |
| /* already gone */ | |
| } | |
| } | |
| pinnedTargetId = null; | |
| pageSessionId = null; | |
| lastMeta = null; | |
| endpoint = await discoverEndpoint(endpointInput, opts); | |
| session = makeCdpSession(endpoint.wsUrl, opts); | |
| await session.opened; | |
| const mine = session; | |
| session.onEvent(onCdpEvent); | |
| session.onClose(() => { | |
| if (session === mine) emit({ type: "endpoint_gone" }); | |
| }); | |
| await session.send("Target.setDiscoverTargets", { discover: true }); | |
| const pages = await pageTargets(); | |
| if (!pages.length) throw new Error("endpoint has no page targets"); | |
| attachTimeMs = Date.now(); | |
| await pinTarget(pages[0].targetId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/cdp.mjs` around lines 420 - 430, Update connect() to close and discard
any existing session before creating a replacement, and reset pinnedTargetId
before reconnection so failed attempts cannot retain a dead target. Capture the
newly created session in the onClose callback and emit endpoint_gone only when
that callback still belongs to the active session, preventing stale sockets from
affecting the new connection.
| // Backend arbitration: an explicitly configured CDP endpoint is the more | ||
| // deliberate act than an ambient agent-browser session, so it wins — and | ||
| // it wins deterministically at start, never by racing discovery. | ||
| this.cdpEndpoint = this.resolveCdpEndpoint(env); | ||
| this.mode = this.cdpEndpoint ? "attach" : "agent-browser"; | ||
| this.browser = | ||
| this.mode === "attach" | ||
| ? makeCdpBrowser(this.cdpEndpoint) | ||
| : makeBrowser(this.session, this.bin); | ||
| // Attach mode observes a browser someone else owns: ownership is never | ||
| // claimed, so the quit path can never close a stranger's session. | ||
| this.ownershipEnabled = this.mode !== "attach"; | ||
| this.backendName = this.mode === "attach" ? "browser endpoint" : "agent-browser"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm this.mode is assigned by both the backend selection and the render probe.
rg -n --type=js -C2 '\bthis\.mode\s*=' bin/renderer.mjs
rg -n --type=js -C3 'function pickRenderMode' bin/renderer.mjs
rg -n --type=js "this\.mode ===" bin/renderer.mjsRepository: StructuPath/herdr-browser
Length of output: 221
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path("bin/renderer.mjs")
raw = p.read_bytes()
text = raw.replace(b"\x00", b"").decode("utf-8", "replace")
lines = text.splitlines()
terms = ("this.mode", "pickRenderMode", "attachTo", "tick()", "startNavigateWatch",
"checkFrameStaleness", "renderImage", "cleanup")
for i, line in enumerate(lines, 1):
if any(term in line for term in terms):
lo, hi = max(1, i-3), min(len(lines), i+4)
print(f"\n--- lines {lo}-{hi} ---")
for n in range(lo, hi + 1):
print(f"{n}:{lines[n-1]}")
PY
printf '\n--- structural occurrences ---\n'
python3 - <<'PY'
from pathlib import Path
text = Path("bin/renderer.mjs").read_bytes().replace(b"\x00", b"").decode("utf-8", "replace")
for n, line in enumerate(text.splitlines(), 1):
if "this.mode" in line or "pickRenderMode" in line:
print(f"{n}:{line}")
PY
printf '\n--- candidate tests and backend identifiers ---\n'
rg -n -i --hidden --glob '!node_modules' 'attachTo|pickRenderMode|backendName|ownershipEnabled|cdpEndpoint|renderImage' . | head -200Repository: StructuPath/herdr-browser
Length of output: 21309
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
text = Path("bin/renderer.mjs").read_bytes().replace(b"\x00", b"").decode()
lines = text.splitlines()
def line_containing(fragment):
return next(i for i, line in enumerate(lines, 1) if fragment in line)
backend_assign = line_containing('this.mode = this.cdpEndpoint ? "attach" : "agent-browser";')
render_assign = line_containing('this.mode = pickRenderMode(')
first_loop_tick = line_containing('await this.enqueue(() => this.tick());')
attach_runtime_assign = line_containing('this.mode = "attach";')
assert backend_assign < render_assign < first_loop_tick
assert attach_runtime_assign != backend_assign
assert all(
f'this.mode {op} "attach"' in lines[i - 1]
for i, op in ((838, "!=="), (952, "!=="), (1188, "==="), (1806, "==="))
)
assert 'this.mode === "attach" ? this.cdpPagePoint(pt, dims) : pt;' in text
assert 'this.mode === "kitty" ? "kitty" : "symbols"' in text
print("constructor backend assignment precedes render-mode assignment, which precedes the first loop tick")
print("runtime attachTo also overwrites the render-mode field")
print("attach-only gates and render-only branches share this.mode")
PYRepository: StructuPath/herdr-browser
Length of output: 376
Separate backend state from render mode.
run() overwrites this.mode with pickRenderMode() before the first polling tick. CDP attach gates then fail, so attach navigation, staleness checks, click scaling, and cleanup do not run. attachTo() also makes renderImage() use the symbols path instead of Kitty output.
Store the backend separately. Use it for backend checks and keep this.mode exclusively for render modes. Add a regression test that selects a render mode before tick().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/renderer.mjs` around lines 561 - 573, Separate backend identity from
render mode in the renderer initialization around `this.cdpEndpoint`,
`this.mode`, `this.browser`, and `this.backendName`; introduce or reuse a
dedicated backend symbol and update attach-specific navigation, staleness,
click-scaling, cleanup, `attachTo()`, and `renderImage()` checks to use it. Keep
`this.mode` exclusively for `pickRenderMode()` results so Kitty output and CDP
behavior remain correct after render-mode selection, and add a regression test
that selects a render mode before `tick()`.
| if (m.type === "frame") { | ||
| this.lastFrameAt = Date.now(); | ||
| this.lastFrameMeta = m.metadata ?? null; | ||
| this.onStreamMessage({ type: "frame", data: m.data }); | ||
| // Ack after the paint queue drains. A skipped paint (blocked stdout, | ||
| // chafa cooldown) still acks — a frozen stream is worse than a | ||
| // dropped frame, and quality/max-dimension knobs bound the cost. | ||
| this.enqueue(() => {}).then(() => this.browser.ackFrame?.(m.ackId, m.gen)); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the stale-frame latch when frames resume.
checkFrameStaleness sets staleHandled = true and writes the banner. The frame handler updates lastFrameAt but leaves staleHandled set and the banner on screen. After one stall the pane reports "frame stale" permanently, and a later stall gets no restart attempt.
🐛 Proposed fix
if (m.type === "frame") {
this.lastFrameAt = Date.now();
this.lastFrameMeta = m.metadata ?? null;
+ if (this.staleHandled) {
+ this.staleHandled = false;
+ if (this.banner.startsWith("frame stale")) {
+ this.banner = "";
+ this.header();
+ }
+ }
this.onStreamMessage({ type: "frame", data: m.data });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (m.type === "frame") { | |
| this.lastFrameAt = Date.now(); | |
| this.lastFrameMeta = m.metadata ?? null; | |
| this.onStreamMessage({ type: "frame", data: m.data }); | |
| // Ack after the paint queue drains. A skipped paint (blocked stdout, | |
| // chafa cooldown) still acks — a frozen stream is worse than a | |
| // dropped frame, and quality/max-dimension knobs bound the cost. | |
| this.enqueue(() => {}).then(() => this.browser.ackFrame?.(m.ackId, m.gen)); | |
| return; | |
| if (m.type === "frame") { | |
| this.lastFrameAt = Date.now(); | |
| this.lastFrameMeta = m.metadata ?? null; | |
| if (this.staleHandled) { | |
| this.staleHandled = false; | |
| if (this.banner.startsWith("frame stale")) { | |
| this.banner = ""; | |
| this.header(); | |
| } | |
| } | |
| this.onStreamMessage({ type: "frame", data: m.data }); | |
| // Ack after the paint queue drains. A skipped paint (blocked stdout, | |
| // chafa cooldown) still acks — a frozen stream is worse than a | |
| // dropped frame, and quality/max-dimension knobs bound the cost. | |
| this.enqueue(() => {}).then(() => this.browser.ackFrame?.(m.ackId, m.gen)); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/renderer.mjs` around lines 876 - 884, Update the frame branch in the
frame handler to reset the stale-frame latch and remove the stale banner
whenever a new frame arrives. Preserve the existing lastFrameAt update, metadata
handling, stream notification, and frame acknowledgment flow.
| pushLogEntry(entry) { | ||
| const text = entry.url | ||
| ? `${entry.text} ${truncate(sanitizeText(entry.url), 200)}` | ||
| : entry.text; | ||
| if (entry.source === "network") { | ||
| const key = `log ${entry.url} ${entry.text}`; | ||
| const now = Date.now(); | ||
| const last = this.networkState.recent.get(key); | ||
| this.networkState.recent.set(key, now); | ||
| if (last !== undefined && now - last <= 60_000) return; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate newNetworkState and any pruning of the `recent` map.
rg -n --type=js -C6 'function newNetworkState' bin/
rg -n --type=js -C4 '\brecent\b' bin/Repository: StructuPath/herdr-browser
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate definitions and recent-map references ---'
rg -n -C8 'newNetworkState|networkState\.recent|recent\.|pollNetwork|diffNetworkFailures|attach' bin/renderer.mjs
printf '%s\n' '--- relevant source range ---'
sed -n '860,1010p' bin/renderer.mjsRepository: StructuPath/herdr-browser
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked bin files ---'
git ls-files bin
printf '%s\n' '--- renderer files ---'
fd -i 'renderer' .
printf '%s\n' '--- recent/network references ---'
rg -n -C5 'networkState|diffNetworkFailures|pollNetwork|pushLogEntry|newNetworkState|recent' .Repository: StructuPath/herdr-browser
Length of output: 30646
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- renderer size and outline ---'
wc -l bin/renderer.mjs
ast-grep outline bin/renderer.mjs
printf '%s\n' '--- renderer exports and network-related text ---'
rg -n -C4 'export|Network|network|Log|push|recent|diff|poll' bin/renderer.mjs
printf '%s\n' '--- renderer beginning and end ---'
sed -n '1,80p' bin/renderer.mjs
tail -n 100 bin/renderer.mjsRepository: StructuPath/herdr-browser
Length of output: 2075
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- network helpers ---'
sed -n '310,410p' bin/renderer.mjs
printf '%s\n' '--- renderer network methods ---'
rg -n -C8 'pushLogEntry|pollNetwork|diffNetworkFailures|startNetworkTimer|networkState|recent' bin/renderer.mjs || true
printf '%s\n' '--- attach and browser initialization ---'
sed -n '539,700p' bin/renderer.mjsRepository: StructuPath/herdr-browser
Length of output: 10261
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- pollNetwork implementation ---'
sed -n '690,790p' bin/renderer.mjs
printf '%s\n' '--- pushLogEntry and nearby CDP handlers ---'
sed -n '880,980p' bin/renderer.mjs
printf '%s\n' '--- all direct recent-map operations ---'
grep -n -C4 'recent' bin/renderer.mjs || trueRepository: StructuPath/herdr-browser
Length of output: 7840
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
for name in ("bin/renderer.mjs", "bin/cdp.mjs"):
data = Path(name).read_text()
print(f"--- {name} ---")
for needle in ("makeCdpBrowser", "network", "pushLogEntry", "diffNetworkFailures"):
print(f"{needle}: {len(re.findall(re.escape(needle), data))} matches")
for m in re.finditer(r"makeCdpBrowser|pushLogEntry|diffNetworkFailures|network", data):
line = data.count("\n", 0, m.start()) + 1
if name == "bin/renderer.mjs" and 680 <= line <= 950:
print(f"{line}: {data.splitlines()[line-1][:180]}")
PY
node - <<'JS'
const fs = require("fs");
const src = fs.readFileSync("bin/renderer.mjs", "utf8");
const start = src.indexOf("\tpushLogEntry(entry) {");
const end = src.indexOf("\n\t}", start) + 3;
const pushLog = src.slice(start, end);
const poll = src.slice(src.indexOf("\tasync pollNetwork"), src.indexOf("\n\t// Endpoint sources", src.indexOf("\tasync pollNetwork")));
console.log("--- pushLogEntry ---");
console.log(pushLog);
console.log("--- pollNetwork guard/call order ---");
console.log(poll.match(/if \(this\.networkOff[\s\S]*?diffNetworkFailures[\s\S]*?\n\t\t}/)?.[0] ?? poll);
JSRepository: StructuPath/herdr-browser
Length of output: 3074
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- CDP backend outline ---'
ast-grep outline bin/cdp.mjs
printf '%s\n' '--- CDP backend exports and returned surface ---'
grep -n -C10 'makeCdpBrowser\|return {\|network' bin/cdp.mjs || true
printf '%s\n' '--- exact map-growth model ---'
python3 - <<'PY'
WINDOW = 60_000
recent = {}
for i in range(10_000):
url = f"https://page.invalid/{i}"
text = "net::ERR_FAILED"
key = f"log {url} {text}"
now = i
last = recent.get(key)
recent[key] = now
if last is not None and now - last <= WINDOW:
continue
print(f"distinct entries after 10,000 distinct page keys: {len(recent)}")
# The existing pruning condition removes old entries only when diffNetworkFailures runs.
pruned = {k: t for k, t in recent.items() if 10_000 - t <= WINDOW}
print(f"entries surviving a pruning pass at t=10,000 ms: {len(pruned)}")
PYRepository: StructuPath/herdr-browser
Length of output: 7907
Prune stale networkState.recent entries in pushLogEntry. Attach mode omits browser.network, so diffNetworkFailures never prunes this map. Distinct page-controlled URL/text pairs therefore remain for the entire attach session.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/renderer.mjs` around lines 918 - 928, Update pushLogEntry to prune stale
entries from networkState.recent before or while handling network log entries,
removing records older than the existing 60-second deduplication window. Keep
the current key-based suppression behavior intact, including the immediate
return for recently seen entries.
| // Attach gets its own key: "localhost:9222" is already a valid | ||
| // navigation target, so overloading the URL prompt would force a | ||
| // heuristic that guesses wrong on exactly the common case. | ||
| case "a": | ||
| this.openPrompt("attach to endpoint: ", (v) => this.attachTo(v)); | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The a key is unreachable exactly when it is needed.
Line 1439 drops every key except u, q, and \x03 while this.attached is false. A refused or dead endpoint leaves this.attached === false, and the non-rediscoverable branch sets streamCooldownUntil = Number.MAX_SAFE_INTEGER. The user then cannot press a to attach to a corrected endpoint. Add a to the allowlist.
🐛 Proposed fix
- if (!this.attached && !["u", "q", "\x03"].includes(ch)) return;
+ if (!this.attached && !["u", "a", "q", "\x03"].includes(ch)) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/renderer.mjs` around lines 1444 - 1449, Update the key allowlist in the
unattached input handling near the switch containing the "a" case to retain the
"a" key alongside "u", "q", and "\x03". Preserve the existing filtering behavior
for all other keys so users can invoke attachTo when streamCooldownUntil
prevents rediscovery.
| - **One honest footprint:** the console feed calls `Runtime.enable`, which is | ||
| observable by the page and is avoided by stealth automation stacks. Set | ||
| `consoleTier` to `log-only` to skip it — network failures and violations | ||
| still surface through the Log domain. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
consoleTier is not user-settable.
The text tells the reader to set consoleTier to log-only. consoleTier is only an option of makeCdpBrowser, and the renderer calls makeCdpBrowser(endpoint) with no options at bin/renderer.mjs lines 568 and 778. No environment variable or config file reaches it. Either expose it (for example HERDR_BROWSER_CDP_CONSOLE_TIER plus a cdp-console-tier config file) or describe the current behavior without an instruction the reader cannot perform.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 254 - 257, Update the README section describing the
console feed so it no longer instructs users to set the inaccessible consoleTier
option. Describe the current behavior as implemented by
makeCdpBrowser(endpoint), or expose a supported configuration path before
documenting user configuration; keep the guidance consistent with the renderer’s
existing invocation.
| handoff="$(state_dir)/navigate-$(ws_id)" | ||
| umask 077 | ||
| printf '%s\n' "$url" > "$handoff" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Write the handoff file atomically.
> creates and truncates $handoff before the URL is written. The renderer watches the state directory and, on the create event, reads the first line and unlinks the file (bin/renderer.mjs lines 843-857). If the watcher wins the race, it reads an empty line, deletes the file, and the click navigates nothing. Write to a temporary name in the same directory, then rename.
🐛 Proposed fix
handoff="$(state_dir)/navigate-$(ws_id)"
umask 077
- printf '%s\n' "$url" > "$handoff"
+ printf '%s\n' "$url" > "$handoff.$$"
+ mv -f "$handoff.$$" "$handoff"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handoff="$(state_dir)/navigate-$(ws_id)" | |
| umask 077 | |
| printf '%s\n' "$url" > "$handoff" | |
| handoff="$(state_dir)/navigate-$(ws_id)" | |
| umask 077 | |
| printf '%s\n' "$url" > "$handoff.$$" | |
| mv -f "$handoff.$$" "$handoff" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/open.sh` around lines 34 - 36, Update the handoff-file write in the
open flow to use a temporary file in the same state directory, write the URL
with the existing restrictive umask, then atomically rename it to $handoff.
Ensure the renderer only observes the completed file and clean up the temporary
file if writing fails.
1 participant
Adds a second backend: the pane can attach to any Chrome DevTools Protocol endpoint — a Playwright, Puppeteer, or Browser Use run, or any Chrome started with
--remote-debugging-port— and render it, stream its console and network failures, and forward clicks/keys, while the automation client keeps driving. agent-browser mode remains the default and is untouched.Stacked on #5 (failed-network-requests). Plan:
docs/plans/2026-08-04-001-feat-cdp-attach-mode-plan.md.Why this shape
ogulcancelik/herdr-browserwon attention by accepting any CDP client, but it does that by owning a Chromium behind an unauthenticated loopback control gateway, with pixels-only observability. Attaching read-mostly to the stack the user already runs takes the universality without the control surface — and CDP'sLog/Runtimedomains carry failure detail the agent-browser daemon drops (net::ERR_CONNECTION_REFUSED, not a bare status).Guarantees (each one has a test)
Target.createTarget/closeTarget, noEmulation.*ever (asserted over the adapter's whole lifecycle). The automation client owns viewport and emulation; a pane that overrode them would fight the client it exists to observe.selfCreatedand theagent-browser closebranch are gated on the active backend; attach-mode cleanup isPage.stopScreencast+ socket close only.host:portis ever displayed or logged.Runtime.enable, which is page-observable and avoided by stealth automation stacks. Alog-onlytier skips it; documented rather than hidden.Implementation notes worth review
bin/cdp.mjsis a from-scratch CDP client on Node's global WebSocket (Node 22 floor for attach mode only; agent-browser mode stays Node 20, with a clear banner otherwise).node:http, notfetch— WHATWG fetch silently drops a customHostheader (verified), and Chrome 111+ rejects DNS-name Hosts on/json/*.Page.screencastFrameAckechoes the frame's integer id while routing over the flat-session string id; conflating them makes Chrome ignore acks and freezes the stream at quota. Generation-guarded so a pre-restart ack can't reach a new screencast.deviceWidthvs frame width); a cached scale lands every click wrong on retina or after a window resize.targetCreated. Event subscription is broader than rendering: an events-onlyTarget.setAutoAttachpicks up OOPIF/worker console and failures so an embedded frame can't break silently.scripts/open.shdecides attach mode from the same static sources the renderer reads (env /cdp-urlconfig file), not a runtime marker that cannot exist before the first pane start. Otherwise the first Cmd+click would spawn exactly the invisible agent-browser session attach mode forbids.LogandRuntime— Chrome flushes buffered console history onenable, which would otherwise paint a wall of stale lines on every attach.Testing
tests/cdp.test.mjs20, attach-mode cases intests/renderer.test.mjs14). Suite: 183/184. The one failure is the pre-existinge2e: goLive…test, which also fails on unmodifiedmain(agent-browser 0.33.2 vs the 0.28.x this was built against).net::ERR_CONNECTION_REFUSED, HTTP 404s,console.warn, and an uncaughtTypeError— plus the Renderer wired to a real endpoint confirmingsetViewport/networkare absent and ownership is off.Known blind spot
A request that hangs without failing produces no CDP event, so attach mode can't report it the way agent-browser mode's 15 s timeout heuristic does. Documented in the README rather than papered over.
Post-Deploy Monitoring & Validation
No server-side deploy. After
herdr plugin link/update, close and reopen the pane. Start any Chrome with--remote-debugging-port=9222, setHERDR_BROWSER_CDP_URL, and expect frames within a second and failure lines as the page misbehaves.frame stale (tab hidden or contended)is expected when the observed tab is backgrounded. Rollback: reinstall the previous plugin version; the feature holds no persistent state.Follow-ups (deferred)
dumpaction (screenshot + console tail + failures as a post-run artifact an agent can read).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
acommand.Documentation
Bug Fixes