diff --git a/AGENTS.md b/AGENTS.md index 9d1e61b..5e40418 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ src/ ├── screenshot.rs ├── snapshot.rs ├── read_page.rs # read-page (Readability + HTML→Markdown) - ├── memory.rs # take-heapsnapshot (CDP streaming) + inspect-heapsnapshot-node (offline) + ├── memory.rs # take-heapsnapshot (CDP streaming) + inspect-heapsnapshot-node / compare-heapsnapshots (offline) ├── evaluate.rs ├── input.rs # click/fill/type/press/hover ├── emulation.rs # emulate (viewport/geolocation/blocklist) @@ -46,6 +46,18 @@ Detailed documentation for individual commands: - [read-page](wiki/read-page.md) — page content extraction as markdown +## Agent Skill + +`skill/chrome-devtools/SKILL.md` is the **source of truth** for the agent-facing +skill/documentation — it's what teaches an agent (e.g. Claude Code, opencode) how +to use this CLI (targeting, standard patterns, gotchas, failure handling, etc.). +It gets installed/copied into an agent's skills directory (e.g. +`~/.config/opencode/skills/chrome-devtools/SKILL.md`); that installed copy is a +**deployed artifact**, not the source — always edit `skill/chrome-devtools/SKILL.md` +in this repo, then re-sync/reinstall it, rather than editing the installed copy +directly. `skill/chrome-devtools/CUSTOM_SCRIPTING.md` documents `run-script` and +`adapter` in more depth. + ## Key Concepts ### Daemon Architecture @@ -54,6 +66,17 @@ A background daemon (`/tmp/chrome-devtools-daemon.sock`) keeps a persistent CDP WebSocket connection. First CLI invocation spawns it; subsequent commands reuse it. 5-minute idle timeout. +`CdpClient::connect` (`cdp.rs`) bounds the WebSocket handshake with a timeout +(`CHROME_CONNECT_TIMEOUT_SECS`, default 10s). Without it, a pending Chrome +remote-debugging consent dialog would hang the handshake indefinitely — and +since the daemon binds its socket *before* connecting to Chrome (see comments +in `daemon.rs`), the CLI's `wait_for_daemon()` would succeed immediately while +the actual request silently hung forever waiting on the daemon's response, with +no error and no way for a caller (especially an unattended agent) to tell what +was wrong. The timeout error message is written to be agent-actionable: retry +at most once, then stop and ask a human rather than looping or calling +`kill-daemon`. + ### Page Targeting Every page gets a deterministic friendly name (e.g. `warm-squid`) derived from @@ -73,9 +96,18 @@ LLM-friendly) produce structured output. Mutually exclusive. ### Offline Commands -`inspect-heapsnapshot-node` and `kill-daemon` are intercepted early in `run()` -before any Chrome connection or daemon spawn. `inspect-heapsnapshot-node` parses -a local `.heapsnapshot` file purely offline. +`inspect-heapsnapshot-node`, `compare-heapsnapshots`, and `kill-daemon` are +intercepted early in `run()` before any Chrome connection or daemon spawn. +`inspect-heapsnapshot-node` and `compare-heapsnapshots` parse local +`.heapsnapshot` files purely offline. Note that snapshot diffing matches nodes +by V8 heap object ID, which is only stable within a single Chrome session — +both snapshots must come from the same session to produce a meaningful diff. + +`kill-daemon` drops the daemon's already-approved Chrome connection, so it's +guarded (`kill_daemon_decision` in `lib.rs`): interactive (TTY) callers get a +`[y/N]` confirmation prompt; non-interactive callers (agents, scripts) are +refused outright unless `--force` is passed. It must never be used as a +"retry" step for connection failures — see the timeout note above. ### Path Resolution diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 3a9335f..3b850ea 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -333,6 +333,40 @@ See the dedicated [Custom Scripting Guide](./CUSTOM_SCRIPTING.md) for full docum chrome-devtools --target warm-squid adapter skill/chrome-devtools/examples/hn_adapter.js search -- "Rust" ``` +### Pattern 15: Memory Leak Debugging (Heap Snapshots) + +Take two heap snapshots around a suspected leak, diff them per class, then +drill into individual node IDs. `compare-heapsnapshots` and +`inspect-heapsnapshot-node` are fully offline (they parse local files — no +Chrome connection needed). + +```bash +# 1. Take a baseline snapshot +chrome-devtools --target warm-squid take-heapsnapshot --output /tmp/base.heapsnapshot + +# 2. Perform the leaky action in the page (click, navigate, etc.), then take a second snapshot +chrome-devtools --target warm-squid take-heapsnapshot --output /tmp/current.heapsnapshot + +# 3. Diff: one row per class with added/removed counts and sizes, sorted by size delta +chrome-devtools compare-heapsnapshots --base /tmp/base.heapsnapshot --current /tmp/current.heapsnapshot +# idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta +# 0,Detached HTMLDivElement,120,0,120,46080,0,46080 +# ... + +# 4. Detail for one summary row (use the idx column): per-node-id adds/removes +chrome-devtools compare-heapsnapshots --base /tmp/base.heapsnapshot --current /tmp/current.heapsnapshot --class-index 0 + +# 5. Inspect a specific node ID from the detail output +chrome-devtools inspect-heapsnapshot-node --file-path /tmp/current.heapsnapshot --node-id 12345 +``` + +**⚠️ Both snapshots must come from the same Chrome session.** The diff matches +nodes by V8 heap object ID, which is only stable within a single browser +session. Comparing snapshots taken across a Chrome restart (or from different +profiles/machines) produces a meaningless result where nearly everything is +reported as both added and removed — the CLI prints a warning on stderr when +it detects this. + ## Complete Command Reference ### Navigation @@ -359,6 +393,13 @@ chrome-devtools --target console [--duration ] [--type ] chrome-devtools sw-logs [--duration ] [--extension-id ] ``` +### Memory (heap snapshots) +```bash +chrome-devtools --target take-heapsnapshot --output +chrome-devtools compare-heapsnapshots --base --current [--class-index N] # offline +chrome-devtools inspect-heapsnapshot-node --file-path --node-id # offline +``` + ### Interaction ```bash chrome-devtools --target click "" @@ -396,9 +437,34 @@ chrome-devtools --target adapter [--arg key=v ### Daemon ```bash -chrome-devtools kill-daemon # stop the background daemon process +chrome-devtools kill-daemon # refuses when run non-interactively (agents) unless --force +chrome-devtools kill-daemon --force # kills unconditionally ``` +## Failure Handling: "Failed to connect to Chrome" / a command hangs + +Chrome's remote-debugging connection requires a one-time **human approval dialog** +in Chrome. If a command hangs or fails with a connection/timeout error, the most +likely cause is that this dialog is open and waiting for the human — not a bug +you can fix by retrying. + +**If a command hangs for a long time or errors with "Failed to connect to Chrome" +or "Timed out ... connecting to Chrome":** +1. Retry **at most once** (the human may have already approved it just now). +2. If it fails again, **STOP.** Do not keep retrying — the human is very likely + away from the keyboard and no amount of retrying will approve the dialog for + them. +3. Tell the user directly that Chrome is waiting for them to approve the + remote-debugging connection dialog, and wait for their response. + +**Never run `kill-daemon` as a way to "fix" a connection problem.** It does not +help — it destroys the daemon's already-approved connection (if one exists) and +guarantees the *next* attempt needs a fresh human approval, making things worse. +For this reason, `kill-daemon` refuses to run non-interactively without +`--force`. As an agent, only pass `--force` if the user has explicitly asked you +to kill the daemon for some other reason (e.g. it's stuck on unrelated JS +execution) — never as a reaction to a connection failure. + ## Critical Gotchas ### ✗ WRONG: Using invented target names @@ -459,6 +525,20 @@ chrome-devtools --target warm-squid console ### ✓ CORRECT: Each drain is a fresh window Run `console` / `network` right after the action that produces events, OR use `--duration` to collect for a window of time. +### ✗ WRONG: Retrying in a loop or running `kill-daemon` on connection failure +```bash +chrome-devtools list-pages # hangs / fails: "Failed to connect to Chrome" +chrome-devtools kill-daemon --force # ❌ WRONG: destroys the approved connection, makes it worse +chrome-devtools list-pages # retry again, and again... ❌ WRONG: human is likely away +``` + +### ✓ CORRECT: Retry once, then stop and ask the human +```bash +chrome-devtools list-pages # hangs / fails: "Failed to connect to Chrome" +chrome-devtools list-pages # ✓ one retry, in case the human just approved it +# still failing → STOP retrying, tell the user Chrome needs approval, and wait +``` + ## Output Format Summary | Flag | Description | diff --git a/src/cdp.rs b/src/cdp.rs index 0efaf2f..4a104d1 100644 --- a/src/cdp.rs +++ b/src/cdp.rs @@ -162,11 +162,56 @@ fn stash_tab_emulation( } } +/// Read the Chrome WebSocket connect timeout from `CHROME_CONNECT_TIMEOUT_SECS`, +/// defaulting to 10. Kept pure (value passed in) so the parsing policy is +/// unit-testable without mutating process-global env vars. +fn parse_connect_timeout(raw: Option<&str>) -> std::time::Duration { + const DEFAULT: std::time::Duration = std::time::Duration::from_secs(10); + match raw { + None => DEFAULT, + Some(v) => match v.parse::() { + // 0 would make every connect fail instantly with the misleading + // "Chrome may be waiting for approval" message; treat it as + // invalid rather than honoring it. + Ok(secs) if secs > 0 => std::time::Duration::from_secs(secs), + _ => { + eprintln!( + "[warning] Ignoring invalid CHROME_CONNECT_TIMEOUT_SECS value '{v}' \ + (expected a positive integer); using default of {}s", + DEFAULT.as_secs() + ); + DEFAULT + } + }, + } +} + +fn connect_timeout() -> std::time::Duration { + let raw = std::env::var("CHROME_CONNECT_TIMEOUT_SECS").ok(); + parse_connect_timeout(raw.as_deref()) +} + impl CdpClient { /// Connect to Chrome via WebSocket and return a CDP client. + /// + /// Bounded by a timeout: without it, a pending Chrome remote-debugging + /// consent dialog leaves the WebSocket handshake hanging indefinitely, + /// which (via the daemon) makes every command appear to hang forever + /// with no diagnostic. pub async fn connect(ws_url: &str) -> Result { - let (ws, _) = connect_async(ws_url) + let timeout_dur = connect_timeout(); + let (ws, _) = tokio::time::timeout(timeout_dur, connect_async(ws_url)) .await + .map_err(|_| { + anyhow!( + "Timed out after {}s connecting to Chrome at {ws_url}. Chrome may be \ + waiting for a human to approve the remote-debugging connection dialog. \ + If you are an automated agent: do not retry in a loop and do not run \ + kill-daemon (it will not fix this and will require a fresh approval) — \ + stop and ask the human to check Chrome, then retry once.", + timeout_dur.as_secs() + ) + })? .map_err(|e| anyhow!("Failed to connect to Chrome at {ws_url}: {e}"))?; let (write, read) = ws.split(); Ok(Self { @@ -884,6 +929,48 @@ mod tests { use super::*; use serde_json::json; + #[test] + fn test_parse_connect_timeout_default_when_unset() { + assert_eq!( + parse_connect_timeout(None), + std::time::Duration::from_secs(10) + ); + } + + #[test] + fn test_parse_connect_timeout_valid_value() { + assert_eq!( + parse_connect_timeout(Some("3")), + std::time::Duration::from_secs(3) + ); + } + + #[test] + fn test_parse_connect_timeout_rejects_zero() { + // 0 would make every connect fail instantly with a misleading + // "waiting for approval" error — must fall back to the default. + assert_eq!( + parse_connect_timeout(Some("0")), + std::time::Duration::from_secs(10) + ); + } + + #[test] + fn test_parse_connect_timeout_rejects_garbage() { + assert_eq!( + parse_connect_timeout(Some("fast")), + std::time::Duration::from_secs(10) + ); + assert_eq!( + parse_connect_timeout(Some("-5")), + std::time::Duration::from_secs(10) + ); + assert_eq!( + parse_connect_timeout(Some("")), + std::time::Duration::from_secs(10) + ); + } + #[test] fn test_event_buffer_capping() { let mut events = std::collections::VecDeque::new(); diff --git a/src/commands/evaluate.rs b/src/commands/evaluate.rs index d8ce78c..57ae555 100644 --- a/src/commands/evaluate.rs +++ b/src/commands/evaluate.rs @@ -248,7 +248,9 @@ pub async fn run_script( } } - let nav_url = if interpolated_url.starts_with("http://") || interpolated_url.starts_with("https://") { + let nav_url = if interpolated_url.starts_with("http://") + || interpolated_url.starts_with("https://") + { interpolated_url.clone() } else if is_local_host(&interpolated_url) { format!("http://{}", interpolated_url) @@ -258,7 +260,10 @@ pub async fn run_script( let current_url = client.current_url(session_id).await?; if current_url.trim_end_matches('/') != nav_url.trim_end_matches('/') { - eprintln!("[script] Current URL '{}' does not match target URL '{}'. Auto-navigating...", current_url, nav_url); + eprintln!( + "[script] Current URL '{}' does not match target URL '{}'. Auto-navigating...", + current_url, nav_url + ); crate::commands::navigate::navigate( client, @@ -327,7 +332,10 @@ fn parse_adapter_domains(content: &str) -> Vec { if trimmed.starts_with("import ") || trimmed.starts_with("import(") { continue; } - if matches!(trimmed, "\"use strict\";" | "'use strict';" | "\"use strict\"" | "'use strict'") { + if matches!( + trimmed, + "\"use strict\";" | "'use strict';" | "\"use strict\"" | "'use strict'" + ) { continue; } break; @@ -516,7 +524,9 @@ pub async fn run_adapter( let domains = parse_adapter_domains(&script_content); if !domains.is_empty() { let current_url = client.current_url(session_id).await?; - let matched = domains.iter().any(|domain| url_matches_domain(¤t_url, domain)); + let matched = domains + .iter() + .any(|domain| url_matches_domain(¤t_url, domain)); if !matched { let target_domain = &domains[0]; @@ -524,18 +534,19 @@ pub async fn run_adapter( // scheme when one is missing. Forcing a `www.` subdomain breaks apex // hosts and adapters that target an existing subdomain // (e.g. `creator.xiaohongshu.com`). - let target_url = if target_domain.starts_with("http://") || target_domain.starts_with("https://") { - // An explicit scheme always wins, so authors can force http/https - // by writing it in `@domain` (e.g. `@domain http://localhost:3000`). - target_domain.clone() - } else if is_local_host(target_domain) { - // Local dev servers generally speak http, not https. - format!("http://{}", target_domain) - } else { - format!("https://{}", target_domain) - }; + let target_url = + if target_domain.starts_with("http://") || target_domain.starts_with("https://") { + // An explicit scheme always wins, so authors can force http/https + // by writing it in `@domain` (e.g. `@domain http://localhost:3000`). + target_domain.clone() + } else if is_local_host(target_domain) { + // Local dev servers generally speak http, not https. + format!("http://{}", target_domain) + } else { + format!("https://{}", target_domain) + }; eprintln!("[adapter] Current URL '{}' does not match adapter domains {:?}. Auto-navigating to '{}'...", current_url, domains, target_url); - + crate::commands::navigate::navigate( client, session_id, @@ -549,7 +560,9 @@ pub async fn run_adapter( .await?; let post_nav_url = client.current_url(session_id).await?; - let post_matched = domains.iter().any(|domain| url_matches_domain(&post_nav_url, domain)); + let post_matched = domains + .iter() + .any(|domain| url_matches_domain(&post_nav_url, domain)); if !post_matched { anyhow::bail!( "Auto-navigation to '{}' resulted in URL '{}' which does not match adapter domains {:?}", @@ -720,7 +733,10 @@ mod tests { // (only declarations and bare re-exports are handled). let src = "export * from './x';\nexport const ok = 1;\nexport constants = 2;"; let out = strip_export_keywords(src); - assert_eq!(out, "export * from './x';\nconst ok = 1;\nexport constants = 2;"); + assert_eq!( + out, + "export * from './x';\nconst ok = 1;\nexport constants = 2;" + ); } #[test] @@ -748,9 +764,15 @@ mod tests { #[test] fn test_normalize_host() { - assert_eq!(normalize_host("http://user:pass@example.com/some/path"), "example.com"); + assert_eq!( + normalize_host("http://user:pass@example.com/some/path"), + "example.com" + ); assert_eq!(normalize_host("http://user:pass@[::1]:8080"), "[::1]"); - assert_eq!(normalize_host("http://user:pass@127.0.0.1:8080"), "127.0.0.1"); + assert_eq!( + normalize_host("http://user:pass@127.0.0.1:8080"), + "127.0.0.1" + ); assert_eq!(normalize_host("https://foo:bar@localhost"), "localhost"); assert_eq!(normalize_host("example.com"), "example.com"); assert_eq!(normalize_host("http://example.com:3000/"), "example.com"); @@ -772,9 +794,18 @@ mod tests { #[test] fn test_url_matches_domain() { - assert!(url_matches_domain("https://www.xiaohongshu.com/explore", "xiaohongshu.com")); - assert!(url_matches_domain("http://creator.xiaohongshu.com", "creator.xiaohongshu.com")); - assert!(url_matches_domain("https://xiaohongshu.com:8080/path", "xiaohongshu.com")); + assert!(url_matches_domain( + "https://www.xiaohongshu.com/explore", + "xiaohongshu.com" + )); + assert!(url_matches_domain( + "http://creator.xiaohongshu.com", + "creator.xiaohongshu.com" + )); + assert!(url_matches_domain( + "https://xiaohongshu.com:8080/path", + "xiaohongshu.com" + )); assert!(url_matches_domain("http://[::1]:3000", "[::1]")); assert!(!url_matches_domain("https://google.com", "xiaohongshu.com")); } @@ -782,9 +813,18 @@ mod tests { #[test] fn test_url_matches_domain_normalizes_domain() { // `@domain` written with a scheme and/or path still matches the host. - assert!(url_matches_domain("https://www.example.com/page", "https://example.com")); - assert!(url_matches_domain("https://example.com/explore", "example.com/path")); - assert!(url_matches_domain("https://example.com", "http://example.com:443/")); + assert!(url_matches_domain( + "https://www.example.com/page", + "https://example.com" + )); + assert!(url_matches_domain( + "https://example.com/explore", + "example.com/path" + )); + assert!(url_matches_domain( + "https://example.com", + "http://example.com:443/" + )); assert!(!url_matches_domain("https://example.com", "")); } @@ -800,7 +840,13 @@ mod tests { let ctx = build_ctx_object(r#"{"query":"hi"}"#); assert!(ctx.starts_with("const ctx = {")); assert!(ctx.contains(r#"args: {"query":"hi"}"#)); - for helper in ["wait:", "waitForText:", "waitForSelector:", "click:", "fill:"] { + for helper in [ + "wait:", + "waitForText:", + "waitForSelector:", + "click:", + "fill:", + ] { assert!(ctx.contains(helper), "missing helper: {helper}"); } // fill must special-case checkable inputs instead of setting `value`. diff --git a/src/commands/executor.rs b/src/commands/executor.rs index 7846bc5..75f4bcf 100644 --- a/src/commands/executor.rs +++ b/src/commands/executor.rs @@ -55,6 +55,7 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "read-page" => &["output"], "take-heapsnapshot" => &["output"], "inspect-heapsnapshot-node" => &["file_path", "node_id"], + "compare-heapsnapshots" => &["base", "current", "class_index"], "emulate" => &[ "viewport", "device_scale_factor", @@ -72,9 +73,22 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "console" => &["duration", "type"], "network" => &["duration", "type"], "sw-logs" => &["duration", "extension_id"], - "run-script" => &["file_path", "script_args", "raw_args", "output", "track_navigation"], - "adapter" => &["file_path", "function_name", "script_args", "raw_args", "output", "track_navigation"], - "kill-daemon" => &[], + "run-script" => &[ + "file_path", + "script_args", + "raw_args", + "output", + "track_navigation", + ], + "adapter" => &[ + "file_path", + "function_name", + "script_args", + "raw_args", + "output", + "track_navigation", + ], + "kill-daemon" => &["force"], _ => &[], } } @@ -325,7 +339,10 @@ fn script_exec_args(args: &serde_json::Value) -> Result> { .get("file_path") .and_then(|v| v.as_str()) .ok_or_else(|| anyhow!("file_path required"))?; - let script_args = args.get("script_args").cloned().unwrap_or_else(|| json!({})); + let script_args = args + .get("script_args") + .cloned() + .unwrap_or_else(|| json!({})); let output = args.get("output").and_then(|v| v.as_str()); let track_navigation = args .get("track_navigation") @@ -412,7 +429,10 @@ async fn inner_execute( client, session_id, commands::screenshot::ScreenshotOptions { - output: args.get("output").and_then(|v| v.as_str()).map(String::from), + output: args + .get("output") + .and_then(|v| v.as_str()) + .map(String::from), format: args .get("format") .and_then(|v| v.as_str()) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index e56185d..ec949a3 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -4,6 +4,7 @@ use std::fs::File; use std::io::BufReader; use crate::cdp::CdpClient; +use crate::error::{CliError, ErrorCode}; use crate::result::CommandResult; /// Take a heap snapshot of the page and save it to a file. @@ -22,7 +23,10 @@ pub async fn take_heapsnapshot( // can't collide, and rename is atomic (same filesystem). let temp_path = output_path.with_file_name(format!( ".{}.{}.tmp", - output_path.file_name().unwrap_or_default().to_string_lossy(), + output_path + .file_name() + .unwrap_or_default() + .to_string_lossy(), std::process::id(), )); // Drop guard ensures the temp file is removed under all termination paths @@ -43,13 +47,18 @@ pub async fn take_heapsnapshot( // Heap snapshots can be tens or hundreds of MB; buffer the writes to avoid a // syscall per streamed chunk. let mut file = tokio::io::BufWriter::new( - tokio::fs::File::create(&temp_path) - .await - .with_context(|| format!("Failed to create heap snapshot temp file: {}", temp_path.display()))?, + tokio::fs::File::create(&temp_path).await.with_context(|| { + format!( + "Failed to create heap snapshot temp file: {}", + temp_path.display() + ) + })?, ); - - // First, let's enable the HeapProfiler. - client.send_to_target(session_id, "HeapProfiler.enable", json!({})) + + // HeapProfiler must be enabled before takeHeapSnapshot — Chrome rejects + // the command otherwise. + client + .send_to_target(session_id, "HeapProfiler.enable", json!({})) .await .context("Failed to enable HeapProfiler via CDP")?; @@ -70,7 +79,7 @@ pub async fn take_heapsnapshot( .context("Failed to read WebSocket stream message during heap snapshot chunk collection")?; let event: serde_json::Value = serde_json::from_str(&text) .context("Failed to parse WebSocket text frame into JSON event")?; - + // Check if this is the completion response for our takeHeapSnapshot command if event.get("id").and_then(|v| v.as_u64()) == Some(msg_id) { if let Some(error) = event.get("error") { @@ -105,11 +114,11 @@ pub async fn take_heapsnapshot( } .await; - let _ = client.send_to_target(session_id, "HeapProfiler.disable", json!({})).await; + let _ = client + .send_to_target(session_id, "HeapProfiler.disable", json!({})) + .await; - if let Err(e) = snapshot_result { - return Err(e); - } + snapshot_result?; // Drop the writer (and its underlying file handle) before the rename: on // Windows an open handle blocks the move, and even on Unix releasing it @@ -132,13 +141,17 @@ pub async fn take_heapsnapshot( "output": output, "message": format!("Heap snapshot successfully saved to {}", output) }); - Ok(CommandResult::output(crate::format::format_structured(&details, format)?)) + Ok(CommandResult::output(crate::format::format_structured( + &details, format, + )?)) } } -#[derive(serde::Deserialize)] +#[derive(serde::Deserialize, Default)] struct MetaDetails { node_fields: Vec, + #[serde(default)] + node_types: Vec, } #[derive(serde::Deserialize)] @@ -153,45 +166,88 @@ struct HeapSnapshot { strings: Vec, } -/// Parse the JSON heap snapshot and locate details for the given node ID. -/// Returns a tuple of (node_name, self_size). -pub fn parse_node_from_snapshot( - file_path: &str, - node_id: u64, -) -> Result<(String, u64)> { - use anyhow::Context; - let file = File::open(file_path) - .with_context(|| format!("Failed to open heap snapshot file at: {}", file_path))?; - let reader = BufReader::new(file); - let val: HeapSnapshot = serde_json::from_reader(reader) - .context("Failed to deserialize heap snapshot file. Ensure it is valid JSON.")?; - - find_node_in_snapshot(&val, node_id) +/// Resolved offsets into the flat `nodes` array for the fields both +/// `find_node_in_snapshot` and `build_class_aggregates` need. +struct NodeFieldOffsets { + id_offset: usize, + name_offset: usize, + self_size_offset: usize, + node_size: usize, + type_offset: Option, } -/// Pure schema-validation + node-lookup logic, separated from I/O so it can be -/// unit-tested without writing a temp file. -fn find_node_in_snapshot(val: &HeapSnapshot, node_id: u64) -> Result<(String, u64)> { +/// Parse the snapshot meta's `node_fields` schema once and return the +/// offsets (and record stride) used to walk the flat `nodes` array. +/// +/// Also validates that `nodes_len` is a whole number of records so every +/// caller rejects truncated/malformed flat arrays identically, instead of one +/// walker erroring and another silently skipping a trailing partial record. +fn resolve_node_field_offsets( + node_fields: &[String], + nodes_len: usize, +) -> Result { use anyhow::Context; - let nodes = &val.nodes; - let node_fields = &val.snapshot.meta.node_fields; - - // Find fields offsets within the flat nodes array - let id_offset = node_fields.iter().position(|f| f == "id") + let id_offset = node_fields + .iter() + .position(|f| f == "id") .context("Invalid snapshot schema: 'id' node field meta is missing")?; - let name_offset = node_fields.iter().position(|f| f == "name") + let name_offset = node_fields + .iter() + .position(|f| f == "name") .context("Invalid snapshot schema: 'name' node field meta is missing")?; - let self_size_offset = node_fields.iter().position(|f| f == "self_size") + let self_size_offset = node_fields + .iter() + .position(|f| f == "self_size") .context("Invalid snapshot schema: 'self_size' node field meta is missing")?; let node_size = node_fields.len(); if node_size == 0 { bail!("Invalid snapshot: node_fields schema is empty"); } + if !nodes_len.is_multiple_of(node_size) { + bail!( + "Corrupt snapshot: nodes array length ({}) is not a multiple of node_size ({}); \ + the file is truncated or malformed", + nodes_len, + node_size + ); + } + let type_offset = node_fields.iter().position(|f| f == "type"); + Ok(NodeFieldOffsets { + id_offset, + name_offset, + self_size_offset, + node_size, + type_offset, + }) +} + +/// Parse the JSON heap snapshot and locate details for the given node ID. +/// Returns a tuple of (node_name, self_size). +pub fn parse_node_from_snapshot(file_path: &str, node_id: u64) -> Result<(String, u64)> { + let val = parse_snapshot_file(file_path)?; + find_node_in_snapshot(&val, node_id) +} + +/// Pure schema-validation + node-lookup logic, separated from I/O so it can be +/// unit-tested without writing a temp file. +fn find_node_in_snapshot(val: &HeapSnapshot, node_id: u64) -> Result<(String, u64)> { + use anyhow::Context; + let nodes = &val.nodes; + let offs = resolve_node_field_offsets(&val.snapshot.meta.node_fields, nodes.len())?; + let NodeFieldOffsets { + id_offset, + name_offset, + self_size_offset, + node_size, + .. + } = offs; - // Iterate over nodes using chunk sizes defined by the schema meta + // Iterate over nodes using chunk sizes defined by the schema meta. + // resolve_node_field_offsets already guaranteed nodes.len() is a whole + // number of records, so every full record is walked exactly once. let mut target_index = None; let mut current_idx = 0; - while current_idx + id_offset < nodes.len() { + while current_idx + node_size <= nodes.len() { let id = nodes[current_idx + id_offset]; if id == node_id { target_index = Some(current_idx); @@ -205,15 +261,16 @@ fn find_node_in_snapshot(val: &HeapSnapshot, node_id: u64) -> Result<(String, u6 None => bail!("Node with ID {} not found in snapshot file", node_id), }; - if target_node_index + node_size > nodes.len() { - bail!("Corrupted snapshot structure: target node index out of flat bounds"); - } - let name_str_idx = usize::try_from(nodes[target_node_index + name_offset]) .ok() .context("Corrupt snapshot: string index overflow on 32-bit architecture")?; - let name = val.strings.get(name_str_idx).cloned() - .ok_or_else(|| anyhow!("Corrupt snapshot: string index {} out of bounds (strings len {})", name_str_idx, val.strings.len()))?; + let name = val.strings.get(name_str_idx).cloned().ok_or_else(|| { + anyhow!( + "Corrupt snapshot: string index {} out of bounds (strings len {})", + name_str_idx, + val.strings.len() + ) + })?; let self_size = nodes[target_node_index + self_size_offset]; Ok((name, self_size)) @@ -229,15 +286,8 @@ pub fn format_node_details( if format.is_text() { let mut out = String::new(); out.push_str("nodeId,nodeName,selfSize\n"); - let escaped_name = if name.contains(',') || name.contains('"') || name.contains('\n') || name.contains('\r') { - format!("\"{}\"", name.replace('"', "\"\"")) - } else { - name.to_string() - }; - out.push_str(&format!( - "{},{},{}\n", - node_id, escaped_name, self_size - )); + let escaped_name = csv_escape(name); + out.push_str(&format!("{},{},{}\n", node_id, escaped_name, self_size)); Ok(out) } else { let details = json!({ @@ -258,16 +308,514 @@ pub async fn inspect_heapsnapshot_node_offline( format: crate::format::OutputFormat, ) -> Result { let file_path_owned = file_path.to_string(); - let (name, self_size) = tokio::task::spawn_blocking(move || { - parse_node_from_snapshot(&file_path_owned, node_id) - }) - .await - .map_err(|e| anyhow!("Failed to execute blocking snapshot parser: {e}"))??; + let (name, self_size) = + tokio::task::spawn_blocking(move || parse_node_from_snapshot(&file_path_owned, node_id)) + .await + .map_err(|e| anyhow!("Failed to execute blocking snapshot parser: {e}"))??; let out = format_node_details(node_id, &name, self_size, format)?; Ok(CommandResult::output(out)) } +/// Per-class aggregate. Tracks id → self_size so the diff can recover exact +/// per-id sizes for added/deleted nodes without re-parsing the file. +struct ClassAggregate { + nodes: std::collections::HashMap, +} + +impl ClassAggregate { + fn new() -> Self { + Self { + nodes: std::collections::HashMap::new(), + } + } +} + +/// Walk every node in the snapshot and group by class name. For nodes whose +/// `name` field is a constructor/class label (object, native, closure types), +/// the name is used as the group key — matching Chrome DevTools' `className`. +/// For other types (string, array, code, regexp, …) the `name` field holds +/// per-instance content (e.g. the actual string value), so the type name +/// itself is used as the group key instead, preventing one diff row per +/// string value. Pure (no I/O) so it can be unit-tested alongside +/// `find_node_in_snapshot`. +fn build_class_aggregates( + val: &HeapSnapshot, +) -> Result> { + use anyhow::Context; + let nodes = &val.nodes; + let offs = resolve_node_field_offsets(&val.snapshot.meta.node_fields, nodes.len())?; + let NodeFieldOffsets { + id_offset, + name_offset, + self_size_offset, + node_size, + type_offset, + } = offs; + + // Resolve the type-names enum from node_types meta so non-object nodes + // can be grouped by their type name. node_types[type_offset] is itself an + // array of strings (the type enum); other entries are plain field-type + // descriptors ("string", "number", …). + let type_names: Option> = type_offset.and_then(|to| { + val.snapshot + .meta + .node_types + .get(to) + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .map(|t| t.as_str().unwrap_or("unknown").to_string()) + .collect() + }) + }); + + let mut aggregates: std::collections::HashMap = + std::collections::HashMap::new(); + let mut current_idx = 0; + while current_idx + node_size <= nodes.len() { + let id = nodes[current_idx + id_offset]; + let name_str_idx = usize::try_from(nodes[current_idx + name_offset]) + .ok() + .context("Corrupt snapshot: string index overflow on 32-bit architecture")?; + let name_ref = val.strings.get(name_str_idx).ok_or_else(|| { + anyhow!( + "Corrupt snapshot: string index {} out of bounds (strings len {})", + name_str_idx, + val.strings.len() + ) + })?; + let self_size = nodes[current_idx + self_size_offset]; + + // Determine the class key: for object/native/closure types the name + // field holds the constructor/class name. For other types (string, + // array, code, …) the name field holds per-instance content, so use + // the type name instead — matching Chrome DevTools' className logic. + let class_key: &str = match (&type_names, type_offset) { + (Some(names), Some(to)) => { + let type_idx = nodes[current_idx + to] as usize; + let type_name = names.get(type_idx).map(|s| s.as_str()).unwrap_or("unknown"); + match type_name { + "object" | "native" | "closure" => name_ref, + _ => type_name, + } + } + _ => name_ref, + }; + + // Only clone the name on the cold insert path. `entry()` would force a + // clone per node (millions of allocations on large snapshots); the + // get_mut/insert split keeps the hot lookup path allocation-free. + if let Some(agg) = aggregates.get_mut(class_key) { + agg.nodes.insert(id, self_size); + } else { + let mut agg = ClassAggregate::new(); + agg.nodes.insert(id, self_size); + aggregates.insert(class_key.to_string(), agg); + } + + current_idx += node_size; + } + Ok(aggregates) +} + +/// One row of the summary diff. Mirrors the MCP `HeapSnapshotClassDiff` shape +/// so output stays familiar to anyone moving between the two tools. +/// +/// Deliberately holds only counters/sums — per-node-ID detail is computed on +/// demand by `compute_class_node_detail` for the single class the user asks +/// about, so summary-only runs never allocate per-id vectors. +#[derive(Debug, Clone)] +pub struct HeapSnapshotClassDiff { + pub class_name: String, + pub added_count: usize, + pub removed_count: usize, + pub count_delta: i64, + pub added_size: u64, + pub removed_size: u64, + pub size_delta: i64, +} + +/// Compute the per-class summary diff between two snapshots. Returns rows +/// filtered to classes with any change (addedCount > 0 OR removedCount > 0) +/// and sorted by sizeDelta descending — matching DevTools' +/// `#getSortedRawClassDiffs` so the summary list and the `--class-index` +/// detail view share stable indices. +/// +/// Borrows the aggregates rather than consuming them so the caller can keep +/// them alive for a follow-up `compute_class_node_detail` call. +fn diff_snapshots( + base: &std::collections::HashMap, + current: &std::collections::HashMap, +) -> Vec { + let mut diffs: Vec = Vec::new(); + + // 1. Process all classes in `current` (covers retained and added classes). + for (name, cur_agg) in current { + let base_agg = base.get(name); + + let mut added_count: usize = 0; + let mut removed_count: usize = 0; + let mut added_size: u64 = 0; + let mut removed_size: u64 = 0; + + if let Some(b) = base_agg { + for (id, size) in &cur_agg.nodes { + if !b.nodes.contains_key(id) { + added_count += 1; + added_size += size; + } + } + for (id, size) in &b.nodes { + if !cur_agg.nodes.contains_key(id) { + removed_count += 1; + removed_size += size; + } + } + } else { + added_count = cur_agg.nodes.len(); + added_size = cur_agg.nodes.values().sum(); + } + + if added_count > 0 || removed_count > 0 { + diffs.push(HeapSnapshotClassDiff { + class_name: name.clone(), + added_count, + removed_count, + count_delta: added_count as i64 - removed_count as i64, + added_size, + removed_size, + size_delta: added_size as i64 - removed_size as i64, + }); + } + } + + // 2. Classes present in `base` but absent from `current` — deleted + // entirely. + for (name, base_agg) in base { + if current.contains_key(name) || base_agg.nodes.is_empty() { + continue; + } + let removed_count = base_agg.nodes.len(); + let removed_size: u64 = base_agg.nodes.values().sum(); + + diffs.push(HeapSnapshotClassDiff { + class_name: name.clone(), + added_count: 0, + removed_count, + count_delta: -(removed_count as i64), + added_size: 0, + removed_size, + size_delta: -(removed_size as i64), + }); + } + + diffs.sort_by(|a, b| { + b.size_delta + .cmp(&a.size_delta) + .then_with(|| a.class_name.cmp(&b.class_name)) + }); + diffs +} + +/// Per-node-ID detail for a single class: (added, deleted) as +/// (node_id, self_size) tuples. +type ClassNodeDetail = (Vec<(u64, u64)>, Vec<(u64, u64)>); + +/// Compute the per-node-ID detail for a single class, each list sorted by +/// node id for deterministic output. Membership logic mirrors +/// `diff_snapshots` exactly so the detail always agrees with the summary row +/// it was selected from. Only ever run for the one class the user requested, +/// so vectors are allocated lazily instead of pre-sized. +fn compute_class_node_detail( + base_agg: Option<&ClassAggregate>, + cur_agg: Option<&ClassAggregate>, +) -> ClassNodeDetail { + let mut added_nodes: Vec<(u64, u64)> = Vec::new(); + let mut deleted_nodes: Vec<(u64, u64)> = Vec::new(); + + // Unwrap the Options once so the inner loops do a direct HashMap lookup + // per node instead of re-evaluating the Option on every iteration. The + // all-added / all-deleted branches are the class-new and class-gone cases. + match (cur_agg, base_agg) { + (Some(c), Some(b)) => { + for (id, size) in &c.nodes { + if !b.nodes.contains_key(id) { + added_nodes.push((*id, *size)); + } + } + for (id, size) in &b.nodes { + if !c.nodes.contains_key(id) { + deleted_nodes.push((*id, *size)); + } + } + } + (Some(c), None) => { + // Class is brand-new in current: every node is added. + for (id, size) in &c.nodes { + added_nodes.push((*id, *size)); + } + } + (None, Some(b)) => { + // Class is gone from current: every base node is deleted. + for (id, size) in &b.nodes { + deleted_nodes.push((*id, *size)); + } + } + (None, None) => {} + } + + added_nodes.sort_unstable_by_key(|(id, _)| *id); + deleted_nodes.sort_unstable_by_key(|(id, _)| *id); + (added_nodes, deleted_nodes) +} + +/// CSV-escape a class name the same way `format_node_details` escapes node +/// names — names like `(closure)` are safe, but `(string, joined)` would break +/// naive CSV parsing. +fn csv_escape(s: &str) -> std::borrow::Cow<'_, str> { + if s.contains(',') || s.contains('"') || s.contains('\n') || s.contains('\r') { + std::borrow::Cow::Owned(format!("\"{}\"", s.replace('"', "\"\""))) + } else { + std::borrow::Cow::Borrowed(s) + } +} + +/// Format the summary diff table (one row per changed class). +pub fn format_class_diff_summary( + diffs: &[HeapSnapshotClassDiff], + format: crate::format::OutputFormat, +) -> Result { + if format.is_text() { + use std::fmt::Write; + let mut out = String::new(); + out.push_str( + "idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n", + ); + for (i, d) in diffs.iter().enumerate() { + let _ = writeln!( + out, + "{},{},{},{},{},{},{},{}", + i, + csv_escape(&d.class_name), + d.added_count, + d.removed_count, + d.count_delta, + d.added_size, + d.removed_size, + d.size_delta, + ); + } + Ok(out) + } else { + let rows: Vec = diffs + .iter() + .enumerate() + .map(|(i, d)| { + json!({ + "idx": i, + "className": d.class_name, + "addedCount": d.added_count, + "removedCount": d.removed_count, + "countDelta": d.count_delta, + "addedSize": d.added_size, + "removedSize": d.removed_size, + "sizeDelta": d.size_delta, + }) + }) + .collect(); + Ok(crate::format::format_structured( + &json!({ "diffs": rows }), + format, + )?) + } +} + +/// Format the per-class detail (added/deleted node IDs + sizes). Mirrors the +/// summary's `idx` so a user can copy the index straight from summary → detail. +/// The per-id vectors come from `compute_class_node_detail` — they are not +/// stored on the summary row. +pub fn format_class_diff_detail( + idx: usize, + diff: &HeapSnapshotClassDiff, + added_nodes: &[(u64, u64)], + deleted_nodes: &[(u64, u64)], + format: crate::format::OutputFormat, +) -> Result { + if format.is_text() { + use std::fmt::Write; + let mut out = String::new(); + let _ = writeln!( + out, + "idx:{},className:{},addedCount:{},removedCount:{},countDelta:{},addedSize:{},removedSize:{},sizeDelta:{}", + idx, + csv_escape(&diff.class_name), + diff.added_count, + diff.removed_count, + diff.count_delta, + diff.added_size, + diff.removed_size, + diff.size_delta, + ); + out.push_str("\nop,nodeId,selfSize\n"); + for (id, size) in added_nodes { + let _ = writeln!(out, "+,{},{}", id, size); + } + for (id, size) in deleted_nodes { + let _ = writeln!(out, "-,{},{}", id, size); + } + Ok(out) + } else { + let added: Vec = added_nodes + .iter() + .map(|(id, size)| json!({ "op": "+", "nodeId": id, "selfSize": size })) + .collect(); + let deleted: Vec = deleted_nodes + .iter() + .map(|(id, size)| json!({ "op": "-", "nodeId": id, "selfSize": size })) + .collect(); + let mut nodes: Vec = added; + nodes.extend(deleted); + let detail = json!({ + "idx": idx, + "className": diff.class_name, + "addedCount": diff.added_count, + "removedCount": diff.removed_count, + "countDelta": diff.count_delta, + "addedSize": diff.added_size, + "removedSize": diff.removed_size, + "sizeDelta": diff.size_delta, + "nodes": nodes, + }); + Ok(crate::format::format_structured(&detail, format)?) + } +} + +/// Heuristic for snapshots that cannot meaningfully be diffed: V8 heap object +/// IDs are only stable within a single Chrome session, so two non-empty +/// snapshots sharing *zero* node IDs almost certainly come from different +/// sessions (a genuine same-session diff always retains at least the +/// synthetic root/system nodes). Pure so it can be unit-tested. +fn snapshots_share_no_node_ids( + base: &std::collections::HashMap, + current: &std::collections::HashMap, +) -> bool { + let base_has_nodes = base.values().any(|a| !a.nodes.is_empty()); + let current_has_nodes = current.values().any(|a| !a.nodes.is_empty()); + if !base_has_nodes || !current_has_nodes { + return false; + } + // Overlap check is intentionally class-agnostic: an object can change + // class name across snapshots, but its ID cannot. Build the HashSet from + // whichever side has fewer IDs, then scan the other — this bounds the + // peak allocation to the smaller set for large snapshots. + let base_count: usize = base.values().map(|a| a.nodes.len()).sum(); + let current_count: usize = current.values().map(|a| a.nodes.len()).sum(); + if base_count <= current_count { + let mut ids: std::collections::HashSet = + std::collections::HashSet::with_capacity(base_count); + ids.extend(base.values().flat_map(|a| a.nodes.keys().copied())); + !current + .values() + .flat_map(|a| a.nodes.keys()) + .any(|id| ids.contains(id)) + } else { + let mut ids: std::collections::HashSet = + std::collections::HashSet::with_capacity(current_count); + ids.extend(current.values().flat_map(|a| a.nodes.keys().copied())); + !base + .values() + .flat_map(|a| a.nodes.keys()) + .any(|id| ids.contains(id)) + } +} + +/// Offline implementation of `compare-heapsnapshots`. Parses both files, +/// diffs, and renders summary or per-class detail depending on `class_index`. +pub async fn compare_heapsnapshots_offline( + base_path: &str, + current_path: &str, + class_index: Option, + format: crate::format::OutputFormat, +) -> Result { + let base_owned = base_path.to_string(); + let current_owned = current_path.to_string(); + let (diffs, detail, no_overlap) = tokio::task::spawn_blocking( + move || -> Result<(Vec, Option, bool)> { + // Each raw HeapSnapshot (nodes + strings) can be very large; scope the + // parse so it's dropped as soon as its aggregate is built instead of + // holding both raw snapshots in memory for the duration of the diff. + let base_agg = { + let base_val = parse_snapshot_file(&base_owned)?; + build_class_aggregates(&base_val)? + }; + let current_agg = { + let current_val = parse_snapshot_file(¤t_owned)?; + build_class_aggregates(¤t_val)? + }; + let no_overlap = snapshots_share_no_node_ids(&base_agg, ¤t_agg); + let diffs = diff_snapshots(&base_agg, ¤t_agg); + + // Resolve --class-index here, while the aggregates are still + // alive: summary rows carry no per-id vectors, so detail is + // computed on demand for just the requested class. + let detail = match class_index { + None => None, + Some(idx) => { + let diff = diffs.get(idx).ok_or_else(|| { + CliError::new( + ErrorCode::InvalidInput, + format!( + "Invalid classIndex: {}. Total classes with changes: {}", + idx, + diffs.len() + ), + ) + })?; + Some(compute_class_node_detail( + base_agg.get(&diff.class_name), + current_agg.get(&diff.class_name), + )) + } + }; + Ok((diffs, detail, no_overlap)) + }, + ) + .await + .map_err(|e| anyhow!("Failed to execute blocking snapshot diff: {e}"))??; + + if no_overlap { + // Warning, not an error: the zero-overlap heuristic could in theory + // misfire on exotic snapshots, and the raw diff may still be wanted. + eprintln!( + "[warning] The two snapshots share no node IDs — they very likely come from \ + different Chrome sessions. Node IDs are only stable within a single session, \ + so this diff reports everything as added+removed and is not meaningful. \ + Re-take both snapshots within the same session." + ); + } + + let out = match (class_index, detail) { + (Some(idx), Some((added_nodes, deleted_nodes))) => { + // Index validity was established inside the blocking closure. + format_class_diff_detail(idx, &diffs[idx], &added_nodes, &deleted_nodes, format)? + } + _ => format_class_diff_summary(&diffs, format)?, + }; + Ok(CommandResult::output(out)) +} + +/// Read + deserialize a .heapsnapshot file. Shared by the diff path so both +/// base and current snapshots parse identically. +fn parse_snapshot_file(file_path: &str) -> Result { + use anyhow::Context; + let file = File::open(file_path) + .with_context(|| format!("Failed to open heap snapshot file at: {}", file_path))?; + let reader = BufReader::new(file); + serde_json::from_reader(reader) + .context("Failed to deserialize heap snapshot file. Ensure it is valid JSON.") +} + #[cfg(test)] mod tests { use super::*; @@ -301,6 +849,7 @@ mod tests { snapshot: SnapshotMeta { meta: MetaDetails { node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() }, }, nodes: vec![10, 0, 100, 20, 1, 200], @@ -318,6 +867,7 @@ mod tests { snapshot: SnapshotMeta { meta: MetaDetails { node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() }, }, nodes: vec![10, 0, 100], @@ -334,6 +884,7 @@ mod tests { snapshot: SnapshotMeta { meta: MetaDetails { node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() }, }, nodes: vec![10, 5, 100], @@ -354,11 +905,17 @@ mod tests { // Name with comma let out_comma = format_node_details(123, "My,Class", 100, OutputFormat::Text).unwrap(); - assert_eq!(out_comma, "nodeId,nodeName,selfSize\n123,\"My,Class\",100\n"); + assert_eq!( + out_comma, + "nodeId,nodeName,selfSize\n123,\"My,Class\",100\n" + ); // Name with quotes let out_quotes = format_node_details(123, "My\"Class", 100, OutputFormat::Text).unwrap(); - assert_eq!(out_quotes, "nodeId,nodeName,selfSize\n123,\"My\"\"Class\",100\n"); + assert_eq!( + out_quotes, + "nodeId,nodeName,selfSize\n123,\"My\"\"Class\",100\n" + ); // Name with newline let out_nl = format_node_details(123, "My\nClass", 100, OutputFormat::Text).unwrap(); @@ -381,4 +938,417 @@ mod tests { assert!(out_toon.contains("nodeId")); assert!(out_toon.contains("ClassA")); } + + /// Build a synthetic HeapSnapshot from a list of (id, name, self_size) + /// triples. Keeps diff tests readable — node_fields order is fixed at the + /// schema the production parser actually sees. + fn make_snapshot(nodes: &[(u64, &str, u64)]) -> HeapSnapshot { + let mut flat: Vec = Vec::with_capacity(nodes.len() * 3); + let mut strings: Vec = Vec::new(); + let mut string_idx: std::collections::HashMap<&str, u64> = std::collections::HashMap::new(); + for (id, name, size) in nodes { + let &mut idx = string_idx.entry(name).or_insert_with(|| { + let i = strings.len() as u64; + strings.push(name.to_string()); + i + }); + flat.extend_from_slice(&[*id, idx, *size]); + } + HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() + }, + }, + nodes: flat, + strings, + } + } + + #[test] + fn test_build_class_aggregates_groups_by_name() { + let snap = make_snapshot(&[(1, "Map", 100), (2, "Map", 200), (3, "String", 50)]); + let aggs = build_class_aggregates(&snap).unwrap(); + let map = aggs.get("Map").unwrap(); + assert_eq!(map.nodes.len(), 2); + assert_eq!(map.nodes.get(&1), Some(&100)); + assert_eq!(map.nodes.get(&2), Some(&200)); + let s = aggs.get("String").unwrap(); + assert_eq!(s.nodes.get(&3), Some(&50)); + } + + /// Like `make_snapshot` but includes a `type` field (first column) and + /// `node_types` meta so the type-based class-grouping logic can be tested. + /// Each tuple is (type_idx, name, id, self_size). + fn make_typed_snapshot(nodes: &[(u64, &str, u64, u64)]) -> HeapSnapshot { + let type_names = [ + "hidden", + "array", + "string", + "object", + "code", + "closure", + "regexp", + "number", + "native", + "synthetic", + "concatenated string", + "sliced string", + "bigint", + ]; + let type_enum: Vec = type_names + .iter() + .map(|t| serde_json::Value::String(t.to_string())) + .collect(); + + let mut flat: Vec = Vec::with_capacity(nodes.len() * 4); + let mut strings: Vec = Vec::new(); + let mut string_idx: std::collections::HashMap<&str, u64> = std::collections::HashMap::new(); + for (type_idx, name, id, size) in nodes { + let &mut idx = string_idx.entry(name).or_insert_with(|| { + let i = strings.len() as u64; + strings.push(name.to_string()); + i + }); + flat.extend_from_slice(&[*type_idx, idx, *id, *size]); + } + HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec![ + "type".into(), + "name".into(), + "id".into(), + "self_size".into(), + ], + node_types: vec![ + serde_json::Value::Array(type_enum), + serde_json::Value::String("string".into()), + serde_json::Value::String("number".into()), + serde_json::Value::String("number".into()), + ], + }, + }, + nodes: flat, + strings, + } + } + + #[test] + fn test_build_class_aggregates_groups_by_type() { + // Two strings with different values should collapse into one "string" + // class. Object nodes use their name as the class key. An array node + // groups under "array" regardless of its name field. + let snap = make_typed_snapshot(&[ + (2, "hello", 1, 50), // string + (2, "world", 2, 60), // string — different value, same type + (3, "Map", 3, 100), // object + (3, "Window", 4, 200), // object + (1, "(array)", 5, 80), // array + ]); + let aggs = build_class_aggregates(&snap).unwrap(); + + // Both string nodes are in one "string" bucket + let s = aggs.get("string").unwrap(); + assert_eq!(s.nodes.len(), 2); + assert_eq!(s.nodes.get(&1), Some(&50)); + assert_eq!(s.nodes.get(&2), Some(&60)); + + // Object nodes use their name as the class key + let m = aggs.get("Map").unwrap(); + assert_eq!(m.nodes.get(&3), Some(&100)); + let w = aggs.get("Window").unwrap(); + assert_eq!(w.nodes.get(&4), Some(&200)); + + // Array node groups under "array", not its name field value + let a = aggs.get("array").unwrap(); + assert_eq!(a.nodes.get(&5), Some(&80)); + assert!(!aggs.contains_key("(array)")); + } + + #[test] + fn test_diff_groups_strings_by_type() { + // Without type-based grouping, removing "world" and adding "foo" would + // produce two separate diff rows. With the fix they collapse into a + // single "string" row with one add and one remove. + let base = build_class_aggregates(&make_typed_snapshot(&[ + (2, "hello", 1, 50), + (2, "world", 2, 60), + ])) + .unwrap(); + let current = build_class_aggregates(&make_typed_snapshot(&[ + (2, "hello", 1, 50), + (2, "foo", 3, 70), + ])) + .unwrap(); + + let diffs = diff_snapshots(&base, ¤t); + + assert_eq!(diffs.len(), 1); + assert_eq!(diffs[0].class_name, "string"); + assert_eq!(diffs[0].added_count, 1); + assert_eq!(diffs[0].removed_count, 1); + let (added, deleted) = compute_class_node_detail(base.get("string"), current.get("string")); + assert_eq!(added, vec![(3, 70)]); + assert_eq!(deleted, vec![(2, 60)]); + } + + #[test] + fn test_build_class_aggregates_rejects_truncated_nodes() { + // node_fields describes 3-field records, but nodes has 4 entries — a + // truncated/malformed flat array that isn't a multiple of node_size. + // Must error instead of silently dropping the trailing partial record. + let snap = HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() + }, + }, + nodes: vec![1, 0, 100, 2], + strings: vec!["Map".into()], + }; + let err = match build_class_aggregates(&snap) { + Ok(_) => panic!("expected error for truncated nodes array"), + Err(e) => e, + }; + assert!( + err.to_string().contains("not a multiple of node_size"), + "got: {err}" + ); + } + + #[test] + fn test_find_node_rejects_truncated_nodes() { + // Both walkers must reject the same corrupt file identically: before + // the shared check, find_node_in_snapshot silently skipped the + // trailing partial record and reported "not found" instead. + let snap = HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec!["id".into(), "name".into(), "self_size".into()], + ..Default::default() + }, + }, + nodes: vec![1, 0, 100, 2], + strings: vec!["Map".into()], + }; + let err = match find_node_in_snapshot(&snap, 2) { + Ok(_) => panic!("expected error for truncated nodes array"), + Err(e) => e, + }; + assert!( + err.to_string().contains("not a multiple of node_size"), + "got: {err}" + ); + } + + #[test] + fn test_snapshots_share_no_node_ids_detects_disjoint_sessions() { + // Zero ID overlap between two non-empty snapshots → different-session + // heuristic fires. + let base = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[(2, "Map", 100)])).unwrap(); + assert!(snapshots_share_no_node_ids(&base, ¤t)); + + // Any shared ID — even under a different class name — means same + // session; no warning. + let base = + build_class_aggregates(&make_snapshot(&[(1, "Map", 100), (2, "Set", 50)])).unwrap(); + let current = + build_class_aggregates(&make_snapshot(&[(2, "WeakSet", 50), (3, "Map", 10)])).unwrap(); + assert!(!snapshots_share_no_node_ids(&base, ¤t)); + } + + #[test] + fn test_snapshots_share_no_node_ids_ignores_empty_snapshots() { + // An empty snapshot can't indicate a session mismatch. + let base = build_class_aggregates(&make_snapshot(&[])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + assert!(!snapshots_share_no_node_ids(&base, ¤t)); + assert!(!snapshots_share_no_node_ids(¤t, &base)); + assert!(!snapshots_share_no_node_ids(&base, &base)); + } + + #[test] + fn test_diff_added_removed_and_retained() { + // Base: Map{1@100, 2@100}, String{3@50} + // Current: Map{1@100 (retained), 4@150 (new)}, Window{5@500 (new class)} + let base = build_class_aggregates(&make_snapshot(&[ + (1, "Map", 100), + (2, "Map", 100), + (3, "String", 50), + ])) + .unwrap(); + let current = build_class_aggregates(&make_snapshot(&[ + (1, "Map", 100), + (4, "Map", 150), + (5, "Window", 500), + ])) + .unwrap(); + + let diffs = diff_snapshots(&base, ¤t); + + // Sorted by sizeDelta desc: Window(500) > Map(50) > String(-50) + assert_eq!(diffs.len(), 3); + assert_eq!(diffs[0].class_name, "Window"); + assert_eq!(diffs[0].added_count, 1); + assert_eq!(diffs[0].removed_count, 0); + assert_eq!(diffs[0].added_size, 500); + assert_eq!(diffs[0].removed_size, 0); + assert_eq!(diffs[0].size_delta, 500); + let (added, deleted) = compute_class_node_detail(base.get("Window"), current.get("Window")); + assert_eq!(added, vec![(5, 500)]); + assert!(deleted.is_empty()); + + assert_eq!(diffs[1].class_name, "Map"); + assert_eq!(diffs[1].added_count, 1); + assert_eq!(diffs[1].removed_count, 1); + assert_eq!(diffs[1].count_delta, 0); + assert_eq!(diffs[1].added_size, 150); + assert_eq!(diffs[1].removed_size, 100); + assert_eq!(diffs[1].size_delta, 50); + let (added, deleted) = compute_class_node_detail(base.get("Map"), current.get("Map")); + assert_eq!(added, vec![(4, 150)]); + assert_eq!(deleted, vec![(2, 100)]); + + assert_eq!(diffs[2].class_name, "String"); + assert_eq!(diffs[2].added_count, 0); + assert_eq!(diffs[2].removed_count, 1); + assert_eq!(diffs[2].count_delta, -1); + assert_eq!(diffs[2].size_delta, -50); + let (added, deleted) = compute_class_node_detail(base.get("String"), current.get("String")); + assert!(added.is_empty()); + assert_eq!(deleted, vec![(3, 50)]); + } + + #[test] + fn test_diff_filters_unchanged_classes() { + // Map appears in both with identical nodes → no diff row at all. + let base = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + assert!(diffs.is_empty()); + } + + #[test] + fn test_diff_class_gone_entirely_from_current() { + let base = + build_class_aggregates(&make_snapshot(&[(1, "Old", 80), (2, "Old", 40)])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + assert_eq!(diffs.len(), 1); + assert_eq!(diffs[0].class_name, "Old"); + assert_eq!(diffs[0].removed_count, 2); + assert_eq!(diffs[0].removed_size, 120); + assert_eq!(diffs[0].size_delta, -120); + // Detail for a class deleted entirely: every base node is deleted. + let (added, deleted) = compute_class_node_detail(base.get("Old"), current.get("Old")); + assert!(added.is_empty()); + assert_eq!(deleted, vec![(1, 80), (2, 40)]); + } + + #[test] + fn test_format_summary_and_detail_share_indices() { + let base = + build_class_aggregates(&make_snapshot(&[(1, "Map", 100), (2, "String", 50)])).unwrap(); + let current = + build_class_aggregates(&make_snapshot(&[(3, "Window", 500), (4, "Map", 150)])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + + let summary = format_class_diff_summary(&diffs, crate::format::OutputFormat::Text).unwrap(); + // First data row (idx 0) should be the largest sizeDelta — Window. + assert!(summary.starts_with( + "idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n" + )); + assert!(summary.contains("0,Window,1,0,1,500,0,500\n")); + + // Detail for idx 0 must reference the same class. + let (added, deleted) = compute_class_node_detail( + base.get(&diffs[0].class_name), + current.get(&diffs[0].class_name), + ); + let detail = format_class_diff_detail( + 0, + &diffs[0], + &added, + &deleted, + crate::format::OutputFormat::Text, + ) + .unwrap(); + assert!(detail.contains("className:Window")); + assert!(detail.contains("+,3,500")); + // Map's id 4 should NOT appear here — it's a different class. + assert!(!detail.contains("4,150")); + } + + #[test] + fn test_compare_offline_end_to_end_via_files() { + use crate::format::OutputFormat; + use std::io::Write; + use tempfile::NamedTempFile; + + let base_json = json!({ + "snapshot": { "meta": { "node_fields": ["id","name","self_size"] } }, + "nodes": [1, 0, 100, 2, 1, 50], + "strings": ["Map", "String"], + }); + let cur_json = json!({ + "snapshot": { "meta": { "node_fields": ["id","name","self_size"] } }, + "nodes": [3, 0, 500, 4, 1, 150], + "strings": ["Window", "Map"], + }); + + let mut base_file = NamedTempFile::new().unwrap(); + write!(base_file, "{}", base_json).unwrap(); + let mut cur_file = NamedTempFile::new().unwrap(); + write!(cur_file, "{}", cur_json).unwrap(); + + let rt = tokio::runtime::Runtime::new().unwrap(); + let summary = rt + .block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + None, + OutputFormat::Text, + )) + .unwrap(); + let summary_out = summary.output; + assert!(summary_out.contains("0,Window")); + assert!(summary_out.contains("1,Map")); + + // Detail for idx 0 should print Window's node 3. + let detail = rt + .block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + Some(0), + OutputFormat::Text, + )) + .unwrap(); + assert!(detail.output.contains("className:Window")); + assert!(detail.output.contains("+,3,500")); + + // Out-of-range index should error with a clear message. + let err = rt.block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + Some(99), + OutputFormat::Text, + )); + let err = match err { + Ok(_) => panic!("expected error for out-of-range class_index"), + Err(e) => e, + }; + let msg = err.to_string(); + assert!(msg.contains("Invalid classIndex"), "got: {msg}"); + assert!(msg.contains("99")); + // Must surface as a typed InvalidInput error so callers (e.g. main's + // exit-code mapping) get a stable, non-Unspecified error code. + let cli_err = err + .downcast_ref::() + .expect("expected a CliError"); + assert_eq!(cli_err.code(), crate::error::ErrorCode::InvalidInput); + } } diff --git a/src/commands/read_page.rs b/src/commands/read_page.rs index 063de2b..994ec8a 100644 --- a/src/commands/read_page.rs +++ b/src/commands/read_page.rs @@ -329,7 +329,10 @@ mod tests { #[test] fn extract_title_missing() { - assert_eq!(extract_title_from_html("Hello"), None); + assert_eq!( + extract_title_from_html("Hello"), + None + ); } #[test] @@ -352,10 +355,7 @@ mod tests { #[test] fn extract_title_decodes_html_entities() { let html = "Foo & Bar"; - assert_eq!( - extract_title_from_html(html), - Some("Foo & Bar".to_string()) - ); + assert_eq!(extract_title_from_html(html), Some("Foo & Bar".to_string())); } #[test] @@ -456,10 +456,7 @@ mod tests { #[test] fn unwrap_iframes_empty() { - assert_eq!( - unwrap_iframes(""), - "" - ); + assert_eq!(unwrap_iframes(""), ""); } #[test] @@ -506,7 +503,8 @@ mod tests { #[test] fn extract_content_passes_url_for_relative_link_resolution() { - let html = "Testlink"; + let html = + "Testlink"; let url = "https://example.com/article"; let (_, _) = extract_content(html, Some(url)); // Smoke test: URL is accepted without panic. Readability may or may not @@ -613,13 +611,8 @@ mod tests { excerpt: None, site_name: None, }; - let result = format_output( - html, - &meta, - Some("https://example.com"), - OutputFormat::Json, - ) - .unwrap(); + let result = + format_output(html, &meta, Some("https://example.com"), OutputFormat::Json).unwrap(); let parsed: Value = serde_json::from_str(&result).unwrap(); assert_eq!(parsed["url"], "https://example.com"); } diff --git a/src/commands/screenshot.rs b/src/commands/screenshot.rs index a95f19f..a788c57 100644 --- a/src/commands/screenshot.rs +++ b/src/commands/screenshot.rs @@ -78,15 +78,27 @@ pub async fn take_screenshot( if let Some(size) = metrics.get("cssContentSize") { // Filter non-positive values (empty/unrendered pages, certain // document types) — they'd produce an invalid CDP clip. - src_w = size["width"].as_f64().filter(|&v| v > 0.0).unwrap_or(1920.0); - src_h = size["height"].as_f64().filter(|&v| v > 0.0).unwrap_or(1080.0); + src_w = size["width"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1920.0); + src_h = size["height"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1080.0); } } else { // cssLayoutViewport.clientWidth/Height is the visible viewport in // CSS pixels; pageX/pageY are its document-origin scroll offsets. if let Some(viewport) = metrics.get("cssLayoutViewport") { - src_w = viewport["clientWidth"].as_f64().filter(|&v| v > 0.0).unwrap_or(1920.0); - src_h = viewport["clientHeight"].as_f64().filter(|&v| v > 0.0).unwrap_or(1080.0); + src_w = viewport["clientWidth"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1920.0); + src_h = viewport["clientHeight"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1080.0); scroll_x = viewport["pageX"].as_f64().unwrap_or(0.0); scroll_y = viewport["pageY"].as_f64().unwrap_or(0.0); } @@ -138,7 +150,12 @@ pub async fn take_screenshot( /// Returns the smaller of the width and height scale ratios, clamped to <= 1.0 /// (never upscales). A `None` dimension, or a non-positive max/src value, yields /// 1.0 for that axis (no scaling). Returns 1.0 when neither dimension is set. -fn clip_scale_factor(src_w: f64, src_h: f64, max_width: Option, max_height: Option) -> f64 { +fn clip_scale_factor( + src_w: f64, + src_h: f64, + max_width: Option, + max_height: Option, +) -> f64 { let width_scale = match max_width { Some(max_w) if max_w > 0.0 && src_w > 0.0 => (max_w / src_w).min(1.0), _ => 1.0, @@ -166,7 +183,10 @@ mod tests { #[test] fn negative_max_is_treated_as_no_scaling() { - assert_eq!(clip_scale_factor(1920.0, 1080.0, Some(-100.0), Some(-50.0)), 1.0); + assert_eq!( + clip_scale_factor(1920.0, 1080.0, Some(-100.0), Some(-50.0)), + 1.0 + ); } #[test] @@ -189,12 +209,18 @@ mod tests { #[test] fn both_dimensions_uses_the_smaller_ratio() { // src 2000x1000, max 1000x250 → width_scale 0.5, height_scale 0.25 → 0.25 - assert_eq!(clip_scale_factor(2000.0, 1000.0, Some(1000.0), Some(250.0)), 0.25); + assert_eq!( + clip_scale_factor(2000.0, 1000.0, Some(1000.0), Some(250.0)), + 0.25 + ); } #[test] fn never_upscales_when_max_exceeds_source() { // src 800x600, max 1600x1200 → both ratios > 1.0, clamped to 1.0 - assert_eq!(clip_scale_factor(800.0, 600.0, Some(1600.0), Some(1200.0)), 1.0); + assert_eq!( + clip_scale_factor(800.0, 600.0, Some(1600.0), Some(1200.0)), + 1.0 + ); } } diff --git a/src/commands/third_party.rs b/src/commands/third_party.rs index 41c84b2..c18e0ab 100644 --- a/src/commands/third_party.rs +++ b/src/commands/third_party.rs @@ -143,9 +143,7 @@ pub async fn list_3p_tools( let parts: Vec = ann .as_object() .map(|obj| { - obj.iter() - .map(|(k, v)| format!("{}={}", k, v)) - .collect() + obj.iter().map(|(k, v)| format!("{}={}", k, v)).collect() }) .unwrap_or_default(); if !parts.is_empty() { diff --git a/src/lib.rs b/src/lib.rs index 69eb08c..cef203b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -240,6 +240,32 @@ pub enum Commands { node_id: u64, }, + /// Compare two .heapsnapshot files and report per-class added/deleted nodes. + /// Offline: parses local files, no Chrome connection required. Without + /// --class-index, prints a summary table sorted by size delta. With + /// --class-index N, prints the added/deleted node IDs for row N (use the + /// idx column from the summary). Node IDs in the detail output are + /// directly usable with `inspect-heapsnapshot-node --node-id`. + /// + /// IMPORTANT: both snapshots must come from the same Chrome session — + /// nodes are matched by V8 heap object ID, which is only stable within a + /// single session. Snapshots taken across a browser restart diff as + /// "everything added + everything removed", which is meaningless. + #[command(name = "compare-heapsnapshots")] + CompareHeapsnapshots { + /// Path to the base (earlier) .heapsnapshot file + #[arg(long, short)] + base: String, + /// Path to the current (later) .heapsnapshot file + #[arg(long, short)] + current: String, + /// Optional 0-based row index from the summary output. When omitted, + /// prints the summary table; when present, prints per-id detail for + /// that class. + #[arg(long)] + class_index: Option, + }, + /// Manage page emulation (viewport, geolocation, etc.) Emulate { /// Set viewport size as WxH (e.g. 1280x720) @@ -384,7 +410,16 @@ pub enum Commands { /// Stop the background daemon process #[command(name = "kill-daemon")] - KillDaemon, + KillDaemon { + /// Skip the confirmation/refusal guard and kill unconditionally. + /// + /// Killing the daemon drops any already-approved Chrome remote-debugging + /// connection; reconnecting requires the human to re-approve Chrome's + /// consent dialog. This does NOT fix "Failed to connect to Chrome" + /// errors — do not use it as a retry step. + #[arg(long)] + force: bool, + }, } impl Cli { @@ -420,6 +455,7 @@ impl Cli { Commands::ReadPage { .. } => "read-page", Commands::TakeHeapSnapshot { .. } => "take-heapsnapshot", Commands::InspectHeapSnapshotNode { .. } => "inspect-heapsnapshot-node", + Commands::CompareHeapsnapshots { .. } => "compare-heapsnapshots", Commands::Emulate { .. } => "emulate", Commands::WaitFor { .. } => "wait-for", Commands::List3pTools => "list-3p-tools", @@ -429,11 +465,36 @@ impl Cli { Commands::SwLogs { .. } => "sw-logs", Commands::RunScript { .. } => "run-script", Commands::Adapter { .. } => "adapter", - Commands::KillDaemon => "kill-daemon", + Commands::KillDaemon { .. } => "kill-daemon", } } } +/// What to do when `kill-daemon` is invoked, given `--force` and whether +/// stdin is a TTY. Kept pure so the policy is unit-testable without +/// mocking stdin I/O. +#[derive(Debug, PartialEq, Eq)] +enum KillDaemonDecision { + /// Kill immediately, no confirmation needed. + Proceed, + /// Interactive session — ask the human to confirm. + PromptUser, + /// Non-interactive (e.g. an agent) without `--force` — refuse outright. + /// Killing the daemon drops the approved Chrome connection and forces + /// the human to re-approve it, so an agent must never do this silently. + RefuseNonInteractive, +} + +fn kill_daemon_decision(force: bool, can_prompt: bool) -> KillDaemonDecision { + if force { + KillDaemonDecision::Proceed + } else if can_prompt { + KillDaemonDecision::PromptUser + } else { + KillDaemonDecision::RefuseNonInteractive + } +} + /// Build a DaemonRequest from parsed CLI args. /// Resolve a relative path to an absolute one using the CLI process's CWD. /// The daemon retains its own startup CWD, so relative paths sent to it would @@ -446,8 +507,11 @@ fn absolutize_path(path: &str) -> Result { // Fail loudly if the CWD can't be resolved: silently falling back to an // empty path would send a bogus relative path to the daemon, which would // resolve it against the daemon's (different) startup CWD. - let cwd = std::env::current_dir() - .map_err(|e| anyhow::anyhow!("Failed to resolve CLI working directory to absolutize path '{path}': {e}"))?; + let cwd = std::env::current_dir().map_err(|e| { + anyhow::anyhow!( + "Failed to resolve CLI working directory to absolutize path '{path}': {e}" + ) + })?; Ok(cwd.join(p).to_string_lossy().to_string()) } } @@ -517,7 +581,9 @@ fn parse_args(named_args: &[String], raw_args: &[String]) -> Result { + use std::io::Write; + eprint!( + "Kill the daemon? This drops the approved Chrome connection and will \ + require re-approval in Chrome. [y/N] " + ); + std::io::stderr().flush().ok(); + let mut answer = String::new(); + std::io::stdin().read_line(&mut answer).ok(); + if !matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { + println!("Aborted."); + return Ok(()); + } + } + KillDaemonDecision::Proceed => {} + } + let pid_path = protocol::pid_path(); #[cfg(unix)] let sock_path = protocol::socket_path(); @@ -899,6 +1005,25 @@ pub async fn run() -> Result<()> { return Ok(()); } + // Handle compare-heapsnapshots offline — parses two local .heapsnapshot + // files and diffs them. No Chrome connection or daemon required. + if let Commands::CompareHeapsnapshots { + base, + current, + class_index, + } = &cli.command + { + let result = commands::memory::compare_heapsnapshots_offline( + base, + current, + *class_index, + cli.output_format(), + ) + .await?; + print_result(&result); + return Ok(()); + } + let ws_url = browser::resolve_ws_url( cli.ws_endpoint.as_deref(), cli.user_data_dir.as_deref(), @@ -1326,6 +1451,7 @@ mod tests { use super::*; #[test] + #[allow(clippy::approx_constant)] fn test_parse_args() { let args = vec![ "str_val=hello".to_string(), @@ -1340,9 +1466,40 @@ mod tests { assert_eq!(obj.get("str_val").unwrap().as_str().unwrap(), "hello"); assert_eq!(obj.get("int_val").unwrap().as_i64().unwrap(), 42); assert_eq!(obj.get("float_val").unwrap().as_f64().unwrap(), 3.14); - assert_eq!(obj.get("bool_true").unwrap().as_bool().unwrap(), true); + assert!(obj.get("bool_true").unwrap().as_bool().unwrap()); assert!(obj.get("null_val").unwrap().is_null()); - assert_eq!(obj.get("bool_false").unwrap().as_bool().unwrap(), false); + assert!(!obj.get("bool_false").unwrap().as_bool().unwrap()); + } + + #[test] + fn test_kill_daemon_decision_force_always_proceeds() { + assert_eq!( + kill_daemon_decision(true, true), + KillDaemonDecision::Proceed + ); + assert_eq!( + kill_daemon_decision(true, false), + KillDaemonDecision::Proceed + ); + } + + #[test] + fn test_kill_daemon_decision_can_prompt_prompts() { + // Both stdin and stderr are terminals: ask the human. + assert_eq!( + kill_daemon_decision(false, true), + KillDaemonDecision::PromptUser + ); + } + + #[test] + fn test_kill_daemon_decision_non_tty_refuses() { + // This is the agent/non-interactive case: refuse rather than silently + // kill a daemon holding an already-approved Chrome connection. + assert_eq!( + kill_daemon_decision(false, false), + KillDaemonDecision::RefuseNonInteractive + ); } #[test] @@ -1398,17 +1555,28 @@ mod tests { // Standard trailing argument gets mapped to "query" and "_0" let parsed = parse_args(&[], &["what is a witch".to_string()]).unwrap(); let obj = parsed.as_object().unwrap(); - assert_eq!(obj.get("query").unwrap().as_str().unwrap(), "what is a witch"); + assert_eq!( + obj.get("query").unwrap().as_str().unwrap(), + "what is a witch" + ); assert_eq!(obj.get("_0").unwrap().as_str().unwrap(), "what is a witch"); // Mixture of key=value trailing args and raw positional args let parsed = parse_args( &["user=admin".to_string()], - &["what is a witch".to_string(), "limit=10".to_string(), "second_arg".to_string()], - ).unwrap(); + &[ + "what is a witch".to_string(), + "limit=10".to_string(), + "second_arg".to_string(), + ], + ) + .unwrap(); let obj = parsed.as_object().unwrap(); assert_eq!(obj.get("user").unwrap().as_str().unwrap(), "admin"); - assert_eq!(obj.get("query").unwrap().as_str().unwrap(), "what is a witch"); + assert_eq!( + obj.get("query").unwrap().as_str().unwrap(), + "what is a witch" + ); assert_eq!(obj.get("_0").unwrap().as_str().unwrap(), "what is a witch"); assert_eq!(obj.get("limit").unwrap().as_i64().unwrap(), 10); assert_eq!(obj.get("_1").unwrap().as_str().unwrap(), "second_arg"); @@ -1420,12 +1588,21 @@ mod tests { // misparsed as a key=value pair. let parsed = parse_args( &[], - &["https://x.com/search?q=rust".to_string(), "limit=10".to_string()], + &[ + "https://x.com/search?q=rust".to_string(), + "limit=10".to_string(), + ], ) .unwrap(); let obj = parsed.as_object().unwrap(); - assert_eq!(obj.get("query").unwrap().as_str().unwrap(), "https://x.com/search?q=rust"); - assert_eq!(obj.get("_0").unwrap().as_str().unwrap(), "https://x.com/search?q=rust"); + assert_eq!( + obj.get("query").unwrap().as_str().unwrap(), + "https://x.com/search?q=rust" + ); + assert_eq!( + obj.get("_0").unwrap().as_str().unwrap(), + "https://x.com/search?q=rust" + ); assert_eq!(obj.get("limit").unwrap().as_i64().unwrap(), 10); } }