[kernel-1116] browser debug: add cdp monitor#202
[kernel-1116] browser debug: add cdp monitor#202archandatta wants to merge 17 commits intoarchand/kernel-1116/cdp-pipelinefrom
Conversation
hiroTamada
left a comment
There was a problem hiding this comment.
reviewed incrementally—overall this is a solid foundation for cdp-based observability. left inline comments on a few areas:
- secrets/redaction: network events pass through raw headers, cookies, tokens with no scrubbing—worth documenting or gating
- disk growth: filewriter appends indefinitely with no rotation/cap—risky on constrained unikraft instances
- reconnect gap: readloop exits silently on ws error without same-url redial; depends on upstreammgr to always notify
- 500ms debounce: works well but rationale isn't documented for future readers
- duration: open question on whether to persist timing data on events for downstream analytics
no blockers for merge imo, but the disk growth and secrets points are worth resolving before wider rollout.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 69be86f. Configure here.
| func (m *Monitor) handleLoadEventFired(params json.RawMessage, sessionID string) { | ||
| m.publishEvent(EventPageLoad, events.DetailMinimal, events.Source{Kind: events.KindCDP}, "Page.loadEventFired", params, sessionID) | ||
| m.computed.onPageLoad() | ||
| go m.tryScreenshot(m.getLifecycleCtx()) |
There was a problem hiding this comment.
Subframe sessions prematurely advance computed state machine
Medium Severity
handleDOMContentLoaded and handleLoadEventFired unconditionally call computed.onDOMContentLoaded() and computed.onPageLoad() for events from any session, including auto-attached subframe targets. But handleFrameNavigated only resets computed state for top-level frames (ParentID == ""). This means a subframe session's Page.loadEventFired can start the layout_settled timer and set navDOMLoaded before the main page reaches those milestones. If the subframe's timer fires first, layoutFired becomes true and the main page's subsequent onPageLoad early-returns without restarting the timer, causing layout_settled and navigation_settled to fire based on subframe timing rather than main-page timing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 69be86f. Configure here.
| return "" | ||
| } | ||
| return truncateBody(resp.Body, bodyCapFor(state.mimeType)) | ||
| } |
There was a problem hiding this comment.
Response body base64 encoding flag is ignored
Low Severity
fetchResponseBody parses the base64Encoded field from Chrome's Network.getResponseBody response but never checks it. When Chrome returns base64Encoded: true, the body string is base64-encoded data that gets passed through truncateBody and published as-is. This produces garbled body content in the event and could also truncate mid-encoding, yielding invalid base64.
Reviewed by Cursor Bugbot for commit 69be86f. Configure here.


Tests
curl -v -X POST http://localhost:444/events/startto start the event capture streamI did some browsing and started a yt video and got these logs from it:
network.log was the largest - open to thoughts on filtering some of it to reduce the size
(14), image/avif (14), image/x-icon (12), image/webp (1)
application/javascript (4)
Other skipped:
the body fetch returned empty or the resource type was filtered
What we ARE capturing bodies for (30 responses):
Note
Medium Risk
Adds a new CDP WebSocket monitoring subsystem with reconnection, async network body capture, and screenshotting, plus changes to event categorization and log writing behavior. Risk is mainly around concurrency/reconnect edge cases and the
CategoryForunderscore parsing potentially re-categorizing existing event types.Overview
Introduces a full
cdpmonitorimplementation that connects to Chrome DevTools via WebSocket, auto-attaches to targets (including pre-existing ones), and publishes structured events for console output, page/navigation lifecycle, network activity (including optional truncated response bodies), interactions (click/key/scroll), layout shifts, and periodic screenshots.Adds computed meta-events (
network_idle,layout_settled,navigation_settled) with debounced timers and navigation-safe invalidation, and implements restart handling when the upstream DevTools URL changes, emittingmonitor_disconnected/monitor_reconnectedevents.Tightens the events pipeline by changing
CategoryForto split on_(and updating mappings) and by capping per-category log files at 64MB inFileWriter;CaptureSession.Publishnow publishes to the ring buffer under lock before doing file I/O to preserve sequence ordering. Comprehensive new tests cover monitor lifecycle, reconnects, handlers, computed events, and screenshot rate-limiting.Reviewed by Cursor Bugbot for commit 69be86f. Bugbot is set up for automated code reviews on this repo. Configure here.