[kernel-1116] Add CDP Monitor#213
Conversation
…ies and fix delimiter in CategoryFor
… and category validation
…alloc in filewriter, fix session doc
This binary is tracked on main and was incidentally deleted earlier on this branch. Restoring it keeps the 13.4MB binary out of this PR's diff. Removing the tracked binary from main should be done in a separate PR.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| _ = m.injectScript(ctx, p.SessionID) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Injected script never runs on already-loaded pages
Medium Severity
handleAttachedToTarget calls injectScript, which only uses Page.addScriptToEvaluateOnNewDocument. This registers interaction.js for future navigations but never evaluates it on the current document. Pages already loaded when the monitor attaches (via attachExistingTargets or after reconnect) won't have click, keydown, or scroll-settled tracking until their next navigation. A Runtime.evaluate call with the same script source is needed alongside the addScriptToEvaluateOnNewDocument registration to cover the current page.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bf4b04c. Configure here.
| _, err := m.send(ctx, "Page.addScriptToEvaluateOnNewDocument", map[string]any{ | ||
| "source": injectedJS, | ||
| }, sessionID) | ||
| return err |
There was a problem hiding this comment.
Interaction script not injected into current document
Medium Severity
injectScript only calls Page.addScriptToEvaluateOnNewDocument, which registers the interaction-tracking JS for future navigations. The already-loaded document in an attached target never receives the script. When the monitor attaches to existing pages (via attachExistingTargets at startup or after reconnect), clicks, keydowns, and scroll events on those pages won't be captured until the user navigates away. A companion Runtime.evaluate call is needed to inject into the current document.
Reviewed by Cursor Bugbot for commit 8e94162. Configure here.
| key: e.key, | ||
| selector: sel(t), tag: t.tagName || '' | ||
| })); | ||
| }, true); |
There was a problem hiding this comment.
Sensitive input detection bypassed by shadow DOM retargeting
Medium Severity
The keydown handler uses e.target to check isSensitiveInput, but e.target is retargeted across shadow DOM boundaries. When a <input type="password"> lives inside a web component's shadow DOM (common with Material UI, Lit, Shoelace, etc.), e.target at the document level resolves to the shadow host custom element — not the inner password input. Since the shadow host typically isn't an INPUT/TEXTAREA, isEditable returns false and isSensitiveInput returns false, allowing the actual e.key character to be captured. Using e.composedPath()[0] instead of e.target would resolve this, as it returns the real originating element even across shadow boundaries.
Reviewed by Cursor Bugbot for commit 5465e59. Configure here.
| // | ||
| // Lock ordering (outer → inner): | ||
| // | ||
| // restartMu → lifeMu → pendReqMu → computed.mu → pendMu → sessionsMu | ||
| // | ||
| // Never acquire a lock that appears later in this order while holding an | ||
| // earlier one, to prevent deadlock. |
There was a problem hiding this comment.
this is missing a lot of introductory context about what these locks control / what the Monitor struct is responsible for. This file is the entrypoint to the package and reading this I am very lost
maybe a lib/cdpmonitor/README.md would be helpful
There was a problem hiding this comment.
lmk if this is clear enough, or if there other details that would be worth adding! fd4d4d3
|
I did a manual pass against this branch with the review harness, doing a single navigation to A few observations/suggestions from that run: The event stream needs richer CDP identity metadata before this is reviewable as a browser logging foundation. The monitor already receives enough data to tie network events to a target/frame/navigation, but the published event drops most of it. Could we include at least:
Relatedly, the computed events ( Without this, consumers can’t reliably answer “which tab/page/navigation produced this event?”, which seems core to the usefulness of the CDP monitor. One note from the log: the first three document |
|
@rgarcia feedback addressed! the fields like |
archandatta
force-pushed
the
archand/kernel-1116/cdp-foundation
branch
from
16d37eb to
83a164b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 83a164b. Configure here.
| for _, ev := range evs { | ||
| s.publish(ev) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing dead check in onDOMContentLoaded
Low Severity
onDOMContentLoaded unconditionally sets s.navDOMLoaded = true without checking s.dead first. Every other state-mutating method (onRequest, onLoadingFinished, onPageLoad, onLayoutShift) guards with if s.dead { return } before modifying state. While pendingNavigationSettled independently checks dead preventing event emission, this inconsistency mutates a stopped state machine, which could mask bugs if future code reads navDOMLoaded without also checking dead.
Reviewed by Cursor Bugbot for commit 83a164b. Configure here.
|
I ran another manual pass with the review harness, this time navigating to Hacker News, then opening a new tab and navigating to kernel.sh. Event data: https://gist.github.com/rgarcia/0a60c17ece22e9394bdd9118475a0f5c Observations:
Overall the new loader/frame/request/target additions are a big improvement. I think the remaining work is mostly making the schema consistent and documented enough for downstream consumers. |
|
Couple of other things:
|
4 participants
Introduces the foundational layer of the CDP monitor as a standalone reviewablechunk. No Monitor struct wiring, just the primitives that everything else builds on.
types.go: CDP wire format (cdpMessage), all event type constants, internal state structs (networkReqState, targetInfo, CDP param shapes).
util.go: Console arg extraction, MIME allow-list (isCapturedMIME), resource type filter (isTextualResource), per-MIME body size caps (bodyCapFor), UTF-8-safe body truncation (truncateBody).
computed.go: State machine for the three derived events: network_idle (500ms debounce after all requests finish), layout_settled (1s after page_load with no layout shifts), navigation_settled (fires once all three flags converge). Timer invalidation via navSeq prevents stale AfterFunc callbacks from publishing for a previous navigation.
domains.go: isPageLikeTarget predicate (pages and iframes get Page.* / PerformanceTimeline.*; workers don't), bindingName constant, interaction.js embed.
interaction.js: Injected script tracking clicks, keydowns, and scroll-settled events via the __kernelEvent CDP binding.
Note
High Risk
Introduces a large new event-capture surface (network headers/bodies, console, interactions) plus complex concurrency/reconnect logic; mistakes here can leak sensitive data or destabilize long-lived capture sessions.
Overview
Implements a new
server/lib/cdpmonitorpackage that dials Chrome’s DevTools WebSocket, auto-attaches to targets, translates CDP notifications intoevents.Events (console/network/page/interaction), and emits computed events likenetwork_idle,layout_settled, andnavigation_settled.Adds robustness features including capped-backoff reconnect on upstream URL changes, per-session state machines, redirect-aware request tracking with TTL sweeps, response-body capture with MIME allow-list + truncation, and rate-limited screenshot capture via ffmpeg.
Wires the API service to construct the monitor with
slogand makesApiService.cdpMonitoran interface for easier stubbing; expands tests with extensive fixtures and lifecycle/handler coverage for the new monitor.Reviewed by Cursor Bugbot for commit 0139432. Bugbot is set up for automated code reviews on this repo. Configure here.