Conversation
…ent session - Introduce OutputFormat (Text/Json/Toon) with --toon flag alongside --json. TOON produces compact tabular output for LLM agents (toon-format crate). - Add `console`, `network`, and `sw-logs` commands for inspecting page console messages, network requests, and extension service worker logs. - Add persistent CDP session that stays attached to the current page so events accumulate across commands; `console`/`network` drain on demand instead of using blocking --duration windows. - Add --block-url/--allow-url under `emulate` (+ as global flags) for URL blocking via Network.setBlockedURLs; patterns persist in the daemon. - Add `kill-daemon` subcommand for stopping the background daemon cleanly. - Rewrite skill/chrome-devtools/SKILL.md with correct target-name workflow, pattern-based usage examples, and critical gotchas.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces an ChangesCDP CLI Expansion: OutputFormat, Persistent Sessions, New Commands, URL Blocklist
Sequence Diagram(s)sequenceDiagram
participant Executor
participant CdpClient
participant CDP as Chrome CDP
participant Format as format_structured
Executor->>CdpClient: ensure_persistent_session(target_id)
CdpClient->>CDP: Target.attachToTarget (flatten)
CdpClient->>CDP: Network.enable / Runtime.enable
CdpClient->>CDP: Network.setBlockedURLs(blocklist)
alt console/network with duration_ms == 0 (drain)
Executor->>CdpClient: drain_console_events() / drain_network_events()
CdpClient-->>Executor: Vec from persistent buffer
else console/network with duration_ms > 0 (live)
Executor->>CdpClient: read_events_for(duration_ms)
loop until timeout
CDP-->>CdpClient: Runtime/Network event
CdpClient->>CdpClient: push_event → buffer
end
Executor->>CdpClient: drain_*_events()
else sw-logs
Executor->>CdpClient: get_all_targets()
CdpClient-->>Executor: all targets including service_worker
loop per service_worker
Executor->>CDP: Target.attachToTarget
Executor->>CDP: Runtime.enable
end
Executor->>CdpClient: read_events_for(duration_ms)
end
Executor->>Executor: process_*_events(raw CDP)
Executor->>Format: format_structured(events, req.format())
Format-->>Executor: formatted String
Executor-->>Executor: CommandResult::output(formatted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions for continuous event collection, adds new commands for inspecting console logs, network requests, and extension service worker logs, and implements a compact, LLM-friendly TOON output format. The review feedback highlights critical issues that need to be addressed before merging: specifically, the event router in push_event and the service worker log processor both lack session ID filtering, which leads to event pollution from unrelated sessions. Additionally, several commands (sw-logs, console, and network) fail to disable domains or detach sessions if an error occurs during event reading, resulting in potential resource and session leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@src/cdp.rs`:
- Around line 560-570: The match expression in the `read_events_for` function
combines timeout errors and socket/read errors into the same case, causing real
WebSocket failures to be silently treated as normal timeout completion and
partial data returned as success. Differentiate the two error cases: handle the
timeout case (when tokio::time::timeout returns Err) by breaking the loop
normally, and handle the actual read/socket error case (when self.read_text()
returns Ok(Err)) separately to either propagate the error, log it, or return an
appropriate result instead of silently breaking with partial data.
- Around line 146-152: When detaching from an old target in the
`ensure_persistent_session` method, the event buffers for `network_events` and
`console_events` are not being cleared, causing stale events from the previous
target to persist and be drained under the new page context. In the block where
`persistent_session.take()` is called and we detach from the target, add
clearing of both `network_events` and `console_events` buffers alongside the
`persistent_target_id = None` assignment. Apply this same fix at the other
affected location mentioned in the comment (lines 174-176).
- Around line 212-233: The persistent event buffering logic is capturing Network
and Runtime events without verifying they belong to the active persistent
session, causing events from different sessions to be mixed together. Modify the
event handling in the block where you check for Network.requestWillBeSent,
Network.responseReceived, Network.loadingFinished, Network.loadingFailed,
Runtime.consoleAPICalled, and Runtime.exceptionThrown to extract the sessionId
from the incoming event and compare it against the sessionId stored in
self.persistent_session before appending to self.network_events or
self.console_events. Only push events to these buffers if the sessionIds match.
- Around line 166-172: The Network.enable and Runtime.enable calls for
persistent session setup are being silently ignored with let _ = discards, which
means failures go undetected and the session is still marked as persistent even
when critical domain initialization fails. Remove the let _ = pattern and
instead check the results of both send_to_target calls for "Network.enable" and
"Runtime.enable" - either propagate any errors returned or at minimum log them
and prevent the session from being marked as persistent if either call fails.
This ensures downstream collectors cannot skip per-command enables and miss
events due to uninitialized domains.
In `@src/commands/emulation.rs`:
- Around line 102-114: The allow_url processing block removes patterns from
client.blocklist but never adds them to client.allowlist, so allowed rules are
not persisted as daemon state. When iterating through params.allow_url, add each
pattern to client.allowlist to ensure the allow rules are durably stored and
properly included in status output. Apply the same fix to any other location
where allow_url patterns are processed (such as the similar code block around
line 193-207).
In `@src/commands/executor.rs`:
- Around line 412-457: The executor uses inconsistent default values for the
duration parameter compared to the CLI. In both the "console" and "network"
command handlers, change the unwrap_or(3000) calls to unwrap_or(0) to match the
CLI defaults defined in src/lib.rs, ensuring that direct daemon callers sending
raw JSON requests without a duration field get 0ms (instant drain mode) instead
of 3000ms, which aligns with the documented behavior that omitting the duration
field means "drain instantly".
In `@src/commands/sw_logs.rs`:
- Around line 68-79: The code processes Runtime events for all sessionIds
without filtering out events that don't belong to attached service-worker
sessions. Add a validation check immediately after determining the source
variable (after the find operation on the sessions iterator) to skip processing
events where the sessionId is not found in the attached sessions. Specifically,
if source resolves to "unknown" (indicating the sessionId was not matched),
continue to the next event iteration to prevent unrelated page/session logs from
being included in the sw-logs output.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af8ddc19-cdd3-4738-88ef-c3cb09704232
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/console.rssrc/commands/emulation.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/mod.rssrc/commands/network.rssrc/commands/pages.rssrc/commands/snapshot.rssrc/commands/sw_logs.rssrc/commands/third_party.rssrc/format.rssrc/lib.rssrc/protocol.rstests/executor_tests.rs
Correctness - cdp::push_event now gates persistent-buffer routing on the event's sessionId matching the persistent page session, so sw-logs and ad-hoc live sessions no longer leak into the page's console_events / network_events buffers. - sw_logs now filters out events that don't match an attached service worker session, preventing page/other-session events from appearing as SW logs. Cleanup (URL blocking) - Removed the dead `allowlist` field on CdpClient, the `--clear-allows` flag, and the unreachable "Allowed URLs:" display branch. The doc comments claimed allowlist semantics that were never implemented; --allow-url already just removed entries from the blocklist. - Renamed --allow-url to --unblock-url to match its actual semantics (and reword --block-url's help to no longer reference --allow-url). Robustness - sw-logs / console / network now capture read_events_for's result, run session detach / Runtime.disable / Network.disable cleanup, and only then `?`-propagate the error. read_events_for is currently infallible, so this is dead-code hygiene — but removes a latent trap the moment it gains a fallible path. - kill-daemon now uses libc::kill(SIGTERM) directly instead of shelling out to /usr/bin/kill, and distinguishes ESRCH (stale PID file) from real signalling failures. Tests (19 → 31 tests) - format::from_flags precedence (toon wins over json) - DaemonRequest::format() precedence (output_format wins over legacy json_output bool) - format_structured round-trips Json / Toon / Text - process_console_events: arg joining, description fallback, exception mapping, type-filter include/exclude, `error` filter includes exceptions, ignores unknown methods - process_network_events: request+response merge by requestId, loadingFailed status, type filter, URL sort, ignores unknown methods, orphan response dropped - extract_extension_id: with path, no path, trailing slash, non-extension URL, empty string, prefix-only edge case Misc - Executor duration fallback for console/network changed 3000 → 0 so hand-crafted / malformed requests default to drain mode consistent with the CLI's documented contract. - Console/Network --duration help clarified: events collected in live mode remain available to a later drain. - skill/chrome-devtools/SKILL.md updated for --unblock-url rename.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the Chrome DevTools CLI, including a persistent CDP session for continuous Network and Console event collection, new commands (console, network, sw-logs, and kill-daemon), network URL blocking capabilities, and support for the compact TOON output format. The review feedback highlights critical issues regarding duplicate event delivery in live mode, a failure of URL blocking in direct/fallback mode, and opportunities to make the code more idiomatic and efficient by avoiding unnecessary cloning of session variables.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the Chrome DevTools CLI by introducing persistent CDP sessions for continuous network and console event collection, adding new commands (console, network, sw-logs, and kill-daemon), implementing URL blocking, and supporting a compact TOON output format. The code review identified several important issues to address: redundant subcommand arguments shadowing global flags, a potential CDP session leak if initialization fails, fragile parsing of non-string primitive values in console and service worker logs, event buffer pollution from service worker logs, and premature deletion of socket/PID files when daemon termination fails due to permission errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…k-url flags Page-level commands previously attached a throwaway per-command CDP session while blocklist/emulation state lived on the long-lived persistent session, so that state never governed the commands that actually ran. Unify on the persistent session (falling back to a per-command attach only when it is unavailable), and never detach the persistent session during cleanup. This fixes `emulate --clear-viewport` / `--clear-geolocation` being silent no-ops: the override was set on one short-lived session and the clear ran on a different one. Set and clear now run on the same session, so clearing works. It also lets `--block-url` govern the page's subresource requests during navigation. Also dedupe the network flags. `--block-url` / `--unblock-url` were declared both as global flags and as `emulate` subcommand flags, which shadowed each other and forced a workaround in the executor. They are now defined once (global) and inherited by `emulate` via clap; the emulate handler still applies them itself, ordered before `--clear-blocks` (clear, then add), which is why the generic merge keeps skipping "emulate". cdp: store the persistent session id before enabling Network/Runtime so events arriving during enable are routed to the persistent buffers; reset the session id and detach if either enable fails. docs: - Document that `--block-url` blocks subresources only (images, scripts, fetch/XHR, CDN, trackers) and not top-level navigations — a Chrome `Network.setBlockedURLs` limitation, not a CLI bug. - Add README reference for console/network/sw-logs/kill-daemon. - Correct the live-mode description: `--duration` consumes events, so they do not reappear on a later drain. Verified against live (headless) Chrome on google.com: navigate, evaluate, snapshot, network, console, fill, screenshot, dialog handling, subresource blocking, and the `--clear-viewport` fix (756 -> 800 -> 756). cargo build clean, cargo test 31 passed.
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions to continuously collect network and console events, adds new commands (console, network, sw-logs, and kill-daemon), and implements URL blocking capabilities along with a compact TOON output format for LLM agents. The reviewer feedback highlights critical issues with URL blocking in direct/fallback mode, where the blocklist is silently ignored when a persistent session is unavailable. To resolve this, the reviewer suggests refactoring apply_network_rules to accept an optional session ID and applying it during fallback session attachment. Additionally, the reviewer recommends preserving the chronological order of network requests instead of sorting them alphabetically by URL to improve sequence analysis.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cdp.rs (1)
591-601:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat WebSocket read failures as normal timeout completion.
At Line 600,
Ok(Err(_)) | Err(_) => breakmasks transport errors and returns partial results as success.Proposed fix
- Ok(Err(_)) | Err(_) => break, + Ok(Err(e)) => return Err(e), + Err(_) => break,🤖 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 `@src/cdp.rs` around lines 591 - 601, The current match statement at the timeout call site is masking transport errors by treating both timeout expiration (Err(_)) and read failures (Ok(Err(_))) identically by just breaking out of the loop. Refactor the match statement to distinguish between these two cases: when the timeout expires (Err(_)), you can safely break to indicate normal timeout completion; however, when read_text() actually fails (Ok(Err(_))), this represents a real transport error that should be propagated or logged as an error rather than silently returning partial results as success. This ensures that legitimate WebSocket failures are not hidden from the caller.
🤖 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 `@src/cdp.rs`:
- Around line 203-211: The `apply_network_rules_internal` method is currently
discarding the result of the `send_to_target` call using `let _ = `, which means
failures to apply the Network.setBlockedURLs command to Chrome are silently
ignored and the caller cannot detect failures. Remove the `let _ = ` discard
pattern and instead propagate or handle the error result appropriately, either
by returning the Result type or checking and logging failures. Apply this same
fix to the other similar location mentioned at lines 189-193 where network rule
application also discards results.
In `@src/lib.rs`:
- Around line 685-689: The direct fallback mode for page navigation is
hardcoding empty vectors for block_url and unblock_url instead of using the
global blocklist configuration, causing command-line flags like --block-url and
--unblock-url to be silently ignored. Replace the Vec::new() calls with the
appropriate values from the global blocklist configuration so that the direct
fallback mode respects the same URL filtering rules as daemon mode. This issue
occurs at two locations: around line 685 (for new-page fallback) and around line
787 (for navigate fallback), both of which need to be updated consistently to
use the actual blocklist values instead of empty vectors.
- Around line 556-567: The PID is parsed as u32 but then cast to libc::pid_t (a
signed integer type) before being passed to the libc::kill function. On systems
where pid_t is i32, u32 values exceeding i32::MAX will wrap to negative numbers,
causing unexpected behavior. Add a validation check after parsing the PID string
into the pid variable to ensure the parsed value does not exceed the maximum
value representable by libc::pid_t (typically i32::MAX). If the PID is out of
range, return an error similar to the existing invalid PID error handling. This
validation should occur before the unsafe call to libc::kill to prevent
incorrect signal delivery.
---
Duplicate comments:
In `@src/cdp.rs`:
- Around line 591-601: The current match statement at the timeout call site is
masking transport errors by treating both timeout expiration (Err(_)) and read
failures (Ok(Err(_))) identically by just breaking out of the loop. Refactor the
match statement to distinguish between these two cases: when the timeout expires
(Err(_)), you can safely break to indicate normal timeout completion; however,
when read_text() actually fails (Ok(Err(_))), this represents a real transport
error that should be propagated or logged as an error rather than silently
returning partial results as success. This ensures that legitimate WebSocket
failures are not hidden from the caller.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f330c19-8245-4c64-a916-c502a65b96fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlREADME.mdskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/console.rssrc/commands/emulation.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/input.rssrc/commands/mod.rssrc/commands/navigate.rssrc/commands/network.rssrc/commands/pages.rssrc/commands/snapshot.rssrc/commands/sw_logs.rssrc/commands/third_party.rssrc/format.rssrc/lib.rssrc/protocol.rstests/executor_tests.rstests/telemetry_tests.rs
…onologically Executor - When the persistent CDP session is unavailable (failed to attach or lost), the fallback fresh session now re-applies `Network.setBlockedURLs` using the daemon's blocklist. Without this, `--block-url` was silently ignored on the degraded path because ensure_persistent_session is what normally calls setBlockedURLs. - `apply_network_rules_internal` promoted to pub(crate) so executor.rs can call it on the fresh session. Network command - Drop the alphabetical-by-URL sort on `network` output. Events now appear in first-seen (chronological) order, with redirects reusing the existing requestId in place rather than being repositioned. - Test `test_process_network_events_sorts_by_url` renamed to `test_process_network_events_preserves_chronological_order` and expectations flipped (z.example.com before a.example.com).
…ndling Network rule application used to swallow failures. `apply_network_rules` and `apply_network_rules_internal` now return `Result` and propagate `Network.setBlockedURLs` errors; the callers (`emulate`, the executor's global block-flag handler, and the fallback-session apply) propagate with `?` instead of discarding the result, so a failed blocklist update surfaces to the user. `read_events_for` no longer treats a real WebSocket read error the same as the collection-window timeout. A timeout is the normal completion path; an `Ok(Err(_))` from `read_text()` is a transport failure and is now logged as a warning before stopping, so a partial result isn't silently presented as a clean end-of-window. `kill-daemon` now validates that the PID from the PID file fits in `libc::pid_t` (i32) before signaling. A corrupted PID file could otherwise wrap to a negative value via `as`, and `kill(-pid)` signals an entire process group — potentially hitting unrelated processes. The guard lives inside `#[cfg(unix)]` so non-unix builds stay warning-clean. Also document, via comments, that direct-mode `new-page` / `navigate` intentionally do not carry the URL blocklist: blocking is daemon-only state and there is no persistent session to apply it to in the one-shot direct path.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Chrome DevTools CLI by introducing a persistent CDP session in the background daemon for continuous event tracking. It adds new commands to drain and live-monitor console logs (console), network requests (network), and extension service worker logs (sw-logs), alongside a kill-daemon command. It also implements URL blocking capabilities and introduces a compact, LLM-friendly TOON output format. The review feedback identifies that while the documentation states that running emulate with no flags displays all active overrides, the current implementation only shows blocked URLs. The reviewer provides actionable suggestions to store and display active viewport and geolocation overrides within the persistent CdpClient session.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…tion per tab `emulate` with no flags is documented to show all active overrides, but only the blocklist was tracked — viewport and geolocation weren't stored, so they could never be displayed. Store them as structured overrides (`ViewportOverride`, `GeolocationOverride`) on the client: set on apply, reset on `--clear-viewport` / `--clear-geolocation` / `--clear-all`, and render alongside the blocklist in the no-flags output. Make all emulation (viewport, geolocation, URL blocklist) **per-tab** instead of a single daemon-wide value. The active tab's state lives in the `blocklist`/`viewport`/`geolocation` fields; `ensure_persistent_session` saves that state under the outgoing target id and restores the incoming target's saved state (or empty) on every target switch, then applies it to the new session. Each tab keeps its own overrides — they persist across navigation within a tab and don't leak to other tabs, so e.g. a mobile viewport with images blocked on one tab and a desktop baseline on another can be held at the same time. State is kept in exactly one place per tab (active fields when current, the saved map otherwise); restore removes from the map and save writes the leaving tab's own state, so there's no duplication. - Re-apply viewport/geolocation on target switch via `apply_emulation_internal`, mirroring the existing blocklist re-apply, so overrides survive switches. - `close-page` calls `forget_target` to drop a closed tab's saved state (and reset the active fields + invalid persistent session if it was the active tab). - Only save tabs that actually have overrides, so the saved-state map doesn't accumulate an empty entry for every tab visited. Docs (README, SKILL) updated from daemon-wide to per-tab semantics. Verified on live Chrome: tab A held 375px + `*.png` blocked (fetch blocked) while tab B held 1440px + no block (fetch OK), concurrently, while interleaving commands; `--clear-*` removes the right override; target_id is stable across cross-origin navigation so a tab's state stays attached to it. cargo build clean, cargo test 31 passed.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions to continuously collect network and console events, adding new commands (console, network, sw-logs, and kill-daemon) and per-tab emulation state isolation (viewport, geolocation, and URL blocking). It also adds support for the compact TOON output format. Feedback focuses on a security vulnerability in kill-daemon where a PID of 0 could terminate the entire process group, and performance optimizations in src/cdp.rs to replace Vec with VecDeque for more efficient event buffer capping.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
kill-daemon: refuse a PID of 0 before signaling. `libc::kill(0, SIGTERM)` signals every process in the caller's process group — it would take down the CLI itself and its siblings — and a corrupted/truncated PID file can read "0", which slips past the existing `i32` overflow guard (0 is a valid i32). The check sits beside that guard inside `#[cfg(unix)]`, so the two now cover both kill() footguns: 0 (process group) and negative/overflow (`-pid` process group). cdp: switch the persistent `network_events` / `console_events` buffers from `Vec` to `VecDeque`. The size cap runs on every push, so once a buffer is full the old `Vec` did `drain(..1)` — an O(N) front-shift of ~5000 elements per event. With `VecDeque` it's `push_back` + `pop_front`, both O(1). `drain_*` still returns `Vec` via `.into()` (one O(N) conversion per drain command, not per event). This also matches the generic `events` buffer, which was already a `VecDeque`. Both verified on live Chrome: console/network still collect and drain correctly; kill-daemon with a PID file of "0" reports "refusing to signal" instead of signaling. cargo build clean, cargo test 31 passed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions to continuously collect console and network events, adds new commands (console, network, sw-logs, and kill-daemon), supports per-tab emulation and URL blocking isolation, and adds a compact --toon output format. The review feedback highlights several high-severity issues, recommending that both network and console type filters be normalized to lowercase for case-insensitive matching, with warn mapped to warning for robustness. Additionally, the reviewer notes that URL blocking rules should be applied directly to the active session ID in direct/fallback mode to ensure they take effect when the persistent session is unavailable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/commands/emulation.rs`:
- Around line 104-106: The issue is that apply_network_rules() is called without
verifying that it can actually apply the rules, and it silently skips execution
when persistent_session is None, causing the operation to report success even
though no rules were applied. Before calling apply_network_rules() in the
network_changed conditional block, add a check to ensure that either a
persistent session exists or that an active command session is available. If
neither exists, return an error indicating that a session is required to apply
network rules. This prevents the silent failure and ensures blocklist updates
are only reported as successful when they are actually applied to an available
session.
In `@src/commands/executor.rs`:
- Around line 145-147: The new-page command is hardcoding empty vectors for
block_url and unblock_url fields, which causes the global URL blocklist flags to
be dropped since the command returns through the browser-level path before a
later merge can run. Instead of hardcoding block_url and unblock_url as
Vec::new(), populate these fields with the corresponding global blocklist
configuration values so that when new-page returns through the browser-level
path, the global blocklist flags are properly included and not lost.
In `@src/commands/sw_logs.rs`:
- Around line 41-57: The current code in the match block for
`client.attach_to_target` ignores failures from the `Runtime.enable` call using
`let _ =` but still adds the session to the `sessions` vector even when
`Runtime.enable` fails. This causes the session to be used for event collection
despite not being properly initialized, leading to empty results without
signaling setup failure. Fix this by checking the result of the `send_to_target`
call for `Runtime.enable` and only adding the session to `sessions` if it
succeeds; if it fails, log a warning and skip adding that session to the vector.
In `@src/lib.rs`:
- Around line 591-604: The kill-daemon error handling in the non-ESRCH branch
only logs the failure to stderr but still returns Ok(()) to the caller, causing
the process to exit with code 0 despite the signaling failure. This breaks
automation semantics. Modify the non-ESRCH error branch (the else block that
logs "Failed to signal daemon") to return an Err() containing the actual error
instead of returning Ok(()), while keeping the ESRCH case returning Ok(()) since
that represents a successful cleanup of stale files. This ensures signaling
failures propagate as actual errors up the call stack.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5420ee1-74a1-406e-b5dd-5791410f9d10
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlREADME.mdskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/console.rssrc/commands/emulation.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/input.rssrc/commands/mod.rssrc/commands/navigate.rssrc/commands/network.rssrc/commands/pages.rssrc/commands/snapshot.rssrc/commands/sw_logs.rssrc/commands/third_party.rssrc/format.rssrc/lib.rssrc/protocol.rstests/executor_tests.rstests/telemetry_tests.rs
Blocklist merge+apply now runs after ensure_persistent_session so --block-url flags target the resolved tab, not a stale one from a previous command. Browser-level commands (list-pages, sw-logs, new-page) are intentional no-ops for --block-url since they have no target tab. Emulation falls back to the command's session when persistent_session is None (direct mode, degraded daemon path) instead of silently skipping network rule application. Service worker logs checks Runtime.enable result and skips sessions that fail initialization, preventing empty results without diagnostics. kill-daemon returns Err on non-ESRCH signal failures (e.g., EPERM) so automation gets correct exit codes. Removed duplicate error output. Added TabEmulation struct and unit tests for per-tab isolation invariant.
Network --type filter normalizes both sides to lowercase, closing the gap between documented lowercase examples (fetch, xhr) and CDP's capitalized resource types (Fetch, XHR). Silent mismatches are now caught. Console --type filter adds warn→warning alias (matches console.warn() intuition) and case-insensitive matching. Exception path literals documented as lowercase-contracted against the normalized set. CLI help text updated to reflect case-insensitive behavior.
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions to the background daemon to enable continuous collection of network and console events, as well as per-tab emulation and URL blocking isolation. It adds new commands (console, network, sw-logs, and kill-daemon), introduces a compact, LLM-friendly TOON output format, and updates documentation and tests accordingly. The reviewer feedback suggests ensuring that global URL blocking flags (--block-url and --unblock-url) are correctly passed and applied during new-page and navigate commands even when running in direct (non-daemon) mode, so that subresources are blocked during the initial page load.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skill/chrome-devtools/SKILL.md`:
- Around line 242-255: The documentation in the "Pattern 7: Output Formats"
section incorrectly references a `--text` flag that does not exist in the
implementation. Plain text is the default output format (as defined by
`#[default]` on `OutputFormat::Text` in src/format.rs), not a selectable flag.
Update the documentation to clarify that all commands default to human-readable
text output when neither flag is specified, and that only `--json` and `--toon`
are the available flag options for alternate output formats. Remove any mention
of `--text` as a flag option.
In `@src/lib.rs`:
- Around line 615-617: The error handling at the Err branch treats all
read_to_string errors identically, incorrectly reporting any failure as "daemon
not running". Instead, match on the specific error kind: only
io::ErrorKind::NotFound should result in the "No daemon running" message, while
other error kinds (such as permission denied, invalid data, or other IO errors)
should be handled explicitly with proper error logging that includes the actual
error details. This ensures permission and IO failures are not silently
misreported as the daemon not being present.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b37a441-05cb-4beb-8d75-8f019f8a0efb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlREADME.mdskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/console.rssrc/commands/emulation.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/input.rssrc/commands/mod.rssrc/commands/navigate.rssrc/commands/network.rssrc/commands/pages.rssrc/commands/snapshot.rssrc/commands/sw_logs.rssrc/commands/third_party.rssrc/format.rssrc/lib.rssrc/protocol.rstests/executor_tests.rstests/telemetry_tests.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces persistent CDP sessions to continuously collect network and console events, per-tab emulation isolation (viewport, geolocation, and URL blocking), and new commands for console, network, and extension service worker log inspection, alongside a compact TOON output format. The reviewer feedback highlights several key improvements: resolving event duplication in the event loop by refactoring persistent event routing, returning early in sw-logs if no sessions are attached to prevent blocking, falling back to params["text"] for console events with empty arguments, and preserving HTTP redirect history in network event processing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn push_event(&mut self, event: Value) { | ||
| // Only route events into the persistent buffers when the event's | ||
| // sessionId matches the persistent page session (flatten-mode events | ||
| // are tagged with sessionId). Events from ad-hoc sessions (sw-logs | ||
| // attach, per-command live sessions) fall through and go into the | ||
| // general events buffer — otherwise a subsequent page `console` drain | ||
| // would return extension service worker logs. | ||
| let from_persistent_session = self.persistent_session.as_deref().is_some_and(|s| { | ||
| event | ||
| .get("sessionId") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|session_id| session_id == s) | ||
| .unwrap_or(false) | ||
| }); | ||
|
|
||
| if from_persistent_session { | ||
| let method = event.get("method").and_then(|v| v.as_str()).unwrap_or(""); | ||
| match method { | ||
| "Network.requestWillBeSent" | ||
| | "Network.responseReceived" | ||
| | "Network.loadingFinished" | ||
| | "Network.loadingFailed" => { | ||
| self.network_events.push_back(event); | ||
| if self.network_events.len() > MAX_PERSISTENT_EVENTS { | ||
| self.network_events.pop_front(); | ||
| } | ||
| return; | ||
| } | ||
| "Runtime.consoleAPICalled" | "Runtime.exceptionThrown" => { | ||
| self.console_events.push_back(event); | ||
| if self.console_events.len() > MAX_PERSISTENT_EVENTS { | ||
| self.console_events.pop_front(); | ||
| } | ||
| return; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| Self::push_to_buffer(&mut self.events, event); | ||
| } |
There was a problem hiding this comment.
The push_event method currently routes persistent session events to network_events or console_events, but falls back to pushing all other events to self.events. During read_events_for, this causes non-persistent events (such as extension service worker logs or direct-mode console/network events) to be pushed to self.events and returned in the result vector. Consequently, these events remain in self.events and are duplicated/re-processed in subsequent event-collecting commands.
To fix this, we should split the persistent routing logic into a separate helper method route_persistent_event and call it directly in read_events_for without pushing non-persistent events to self.events.
fn route_persistent_event(&mut self, event: Value) -> bool {
let from_persistent_session = self.persistent_session.as_deref().is_some_and(|s| {
event
.get("sessionId")
.and_then(|v| v.as_str())
.map(|session_id| session_id == s)
.unwrap_or(false)
});
if from_persistent_session {
let method = event.get("method").and_then(|v| v.as_str()).unwrap_or("");
match method {
"Network.requestWillBeSent"
| "Network.responseReceived"
| "Network.loadingFinished"
| "Network.loadingFailed" => {
self.network_events.push_back(event);
if self.network_events.len() > MAX_PERSISTENT_EVENTS {
self.network_events.pop_front();
}
return true;
}
"Runtime.consoleAPICalled" | "Runtime.exceptionThrown" => {
self.console_events.push_back(event);
if self.console_events.len() > MAX_PERSISTENT_EVENTS {
self.console_events.pop_front();
}
return true;
}
_ => {}
}
}
false
}
fn push_event(&mut self, event: Value) {
if !self.route_persistent_event(event.clone()) {
Self::push_to_buffer(&mut self.events, event);
}
}| Ok(Ok(text)) => { | ||
| if let Ok(resp) = serde_json::from_str::<Value>(&text) { | ||
| if resp.get("method").is_some() { | ||
| self.push_event(resp.clone()); | ||
| events.push(resp); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Update read_events_for to use the new route_persistent_event helper instead of push_event. This ensures that events collected during the duration window are routed to persistent buffers if they belong to the persistent session, but are not pushed to self.events if they are non-persistent (preventing them from being duplicated in subsequent commands).
match tokio::time::timeout(remaining, self.read_text()).await {
Ok(Ok(text)) => {
if let Ok(resp) = serde_json::from_str::<Value>(&text) {
if resp.get("method").is_some() {
self.route_persistent_event(resp.clone());
events.push(resp);
}
}
}| } | ||
|
|
||
| let events_result = client.read_events_for(duration_ms).await; |
There was a problem hiding this comment.
If the CLI fails to attach to any of the discovered extension service workers (e.g., due to target closure or permission issues), the sessions vector will be empty. In this case, calling read_events_for will block the CLI for the entire duration_ms (default 3 seconds) without any possibility of collecting logs.
We should check if sessions is empty and return early with an appropriate message to improve responsiveness and UX.
}
if sessions.is_empty() {
return Ok(CommandResult::output(
"Failed to attach to any extension service workers.".to_string(),
));
}
let events_result = client.read_events_for(duration_ms).await;| let text = args.join(" "); | ||
| let timestamp = params["timestamp"].as_f64().unwrap_or(0.0); |
There was a problem hiding this comment.
In CDP, Runtime.consoleAPICalled events can sometimes have an empty args array but contain a text field directly on the event parameters (e.g., for certain console call types or when no arguments are passed).
To ensure we don't miss these console messages, we should fall back to params["text"] if the formatted text from args is empty.
let mut text = args.join(" ");
if text.is_empty() {
if let Some(t) = params["text"].as_str() {
text = t.to_string();
}
}
let timestamp = params["timestamp"].as_f64().unwrap_or(0.0);| if !requests.contains_key(&request_id) { | ||
| order.push(request_id.clone()); | ||
| } | ||
| requests.insert( | ||
| request_id, | ||
| json!({ | ||
| "url": url, | ||
| "method": http_method, | ||
| "resourceType": resource_type, | ||
| "status": null, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
When an HTTP redirect occurs, Chrome's Network domain fires another Network.requestWillBeSent event with the same requestId but containing a redirectResponse object.
Currently, the implementation overwrites the existing request entry in requests with the new redirected URL and resets the status to null. This completely discards the redirect history (such as the intermediate URLs and their 301/302 status codes), which is critical for debugging network redirect loops or incorrect redirect configurations.
Consider preserving redirects by checking if params contains redirectResponse and, if so, stashing the previous request under a modified ID (e.g., appending a redirect suffix) before inserting the new request.
if requests.contains_key(&request_id) {
if params.get("redirectResponse").is_some() {
let mut redirect_id = format!("{}-redirect", request_id);
let mut counter = 1;
while requests.contains_key(&redirect_id) {
redirect_id = format!("{}-redirect-{}", request_id, counter);
counter += 1;
}
if let Some(prev) = requests.get(&request_id).cloned() {
requests.insert(redirect_id.clone(), prev);
if let Some(pos) = order.iter().position(|id| id == &request_id) {
order.insert(pos, redirect_id);
}
}
}
} else {
order.push(request_id.clone());
}
requests.insert(
request_id,
json!({
"url": url,
"method": http_method,
"resourceType": resource_type,
"status": null,
}),
);
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…pty sessions Network: stash intermediate redirect hops (with their real 3xx status) as separate entries instead of overwriting them when Chrome re-fires requestWillBeSent with the same requestId. The redirect chain is now visible in output rather than silently collapsed to the final URL. sw_logs: return early when all service worker attaches fail instead of blocking for the full collection window with no sessions to collect from.
1 participant
console,network, andsw-logscommands for inspecting page console messages, network requests, and extension service worker logs.console/networkdrain on demand instead of using blocking --duration windows.emulate(+ as global flags) for URL blocking via Network.setBlockedURLs; patterns persist in the daemon.kill-daemonsubcommand for stopping the background daemon cleanly.Summary by CodeRabbit
Release Notes
console,network, andsw-logscommands with optional filtering and duration-based live mode (plus quick drain mode).kill-daemonand enhanced persistent background inspection that reattaches across targets.