From 0d7c5bac416796537f8628ab9a69a1bdcc1c01b5 Mon Sep 17 00:00:00 2001 From: Jean-Pierre Bergamin Date: Sun, 19 Jul 2026 17:51:25 +0200 Subject: [PATCH 1/2] fix(network): canonical JSON shape for navigate --with-network and network MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `navigate --with-network` (and the standalone `network` command) returned an inconsistent JSON shape: a `{entries, …}` object on busy pages but a bare array on quiet ones, so `.results.network.entries` / `.total_requests` threw `cannot index array` half the time, and the documented summary fields were unreachable via `--jq` (which forces detail mode). Introduce a shared `pub(crate) build_canonical_network` builder plus `merge_summary_fields` in network.rs. `apply_network_controls` now returns ONE canonical object on every path (busy/quiet, detail/summary, --all/default): `{entries, shown, total, truncated, total_requests, total_transfer_bytes, by_cause_type, slowest, timeout_reached, …}`. Summary counts always reflect the full capture, never the truncated/field-projected view. The standalone `network` detail envelope now carries the same summary fields alongside `results` so `--jq` users can reach them. Docs: args.rs help text documents the canonical shape with a bare-array backward-compat note. Tests: 6 unit tests incl. a navigate/network parity assertion, 3 new mock e2e tests, and 3 live tests in live_126_network_shape.rs. Co-Authored-By: Claude Fable 5 --- crates/ff-rdp-cli/src/cli/args.rs | 10 +- crates/ff-rdp-cli/src/commands/navigate.rs | 63 ++-- crates/ff-rdp-cli/src/commands/network.rs | 231 ++++++++++++ crates/ff-rdp-cli/tests/e2e/navigate.rs | 123 ++++++- crates/ff-rdp-cli/tests/e2e/network.rs | 14 + .../tests/live/live_126_network_shape.rs | 333 ++++++++++++++++++ crates/ff-rdp-cli/tests/live/main.rs | 1 + ...tion-126-network-json-shape-consistency.md | 77 ++-- 8 files changed, 793 insertions(+), 59 deletions(-) create mode 100644 crates/ff-rdp-cli/tests/live/live_126_network_shape.rs diff --git a/crates/ff-rdp-cli/src/cli/args.rs b/crates/ff-rdp-cli/src/cli/args.rs index d45cb97..ee3975b 100644 --- a/crates/ff-rdp-cli/src/cli/args.rs +++ b/crates/ff-rdp-cli/src/cli/args.rs @@ -438,7 +438,12 @@ Examples: ff-rdp navigate https://example.com --wait-for selector:.athing ff-rdp navigate https://example.com --no-wait -Output: {\"results\": {\"navigated\": \"...\", \"committed_url\": \"...\", \"ready_state\": \"...\", \"elapsed_ms\": N}, \"total\": 1, \"meta\": {...}}")] +Output: {\"results\": {\"navigated\": \"...\", \"committed_url\": \"...\", \"ready_state\": \"...\", \"elapsed_ms\": N}, \"total\": 1, \"meta\": {...}} + +--with-network output: results.network is ONE canonical object on every path (quiet or busy page, --detail/--jq or default, --all or capped): + {\"navigated\": \"...\", \"network\": {\"entries\": [...], \"shown\": N, \"total\": N, \"truncated\": bool, \"total_requests\": N, \"total_transfer_bytes\": N, \"by_cause_type\": {...}, \"slowest\": [...], \"timeout_reached\": false}} + entries is capped at 20 by default (use --all to expand); summary fields always reflect the FULL capture. + Note (iter-126): previously results.network was a BARE ARRAY in non-truncated detail mode (and --all), so .results.network.entries / .total_requests threw \"cannot index array\" on quiet pages. It is now always the object above; consumers of the old bare-array form should read .results.network.entries.")] Navigate(NavigateArgs), /// Evaluate JavaScript in the target tab #[command(long_about = "Evaluate JavaScript in the target tab. @@ -553,7 +558,8 @@ Field fidelity by source: Default: 20 results, sorted by duration (slowest first). Output (summary mode): {\"results\": {\"total_requests\": N, \"total_transfer_bytes\": N, \"by_cause_type\": {...}, \"slowest\": [...], \"timeout_reached\": false}, \"total\": N, \"meta\": {...}} -Output (--detail): {\"results\": [{\"url\": \"...\", \"method\": \"GET\", \"status\": 200, \"duration_ms\": N, ...}], \"total\": N, \"meta\": {...}} +Output (--detail): {\"results\": [{\"url\": \"...\", \"method\": \"GET\", \"status\": 200, \"duration_ms\": N, ...}], \"total\": N, \"total_requests\": N, \"total_transfer_bytes\": N, \"by_cause_type\": {...}, \"slowest\": [...], \"timeout_reached\": false, \"meta\": {...}} + Note (iter-126): detail mode now carries the summary fields (total_requests, total_transfer_bytes, by_cause_type, slowest) alongside the results array, so --jq users (who are always in detail mode) can reach them. Output (--detail --headers): adds {\"headers\": {\"request\": [{\"name\": \"...\", \"value\": \"...\"}], \"response\": [...]}} per entry.")] Network(NetworkArgs), /// Query browser Performance API entries and Core Web Vitals diff --git a/crates/ff-rdp-cli/src/commands/navigate.rs b/crates/ff-rdp-cli/src/commands/navigate.rs index d1f908e..78e1872 100644 --- a/crates/ff-rdp-cli/src/commands/navigate.rs +++ b/crates/ff-rdp-cli/src/commands/navigate.rs @@ -1217,7 +1217,7 @@ pub fn run_with_network( let update_map = merge_updates(all_updates); let network_entries = build_network_entries(&all_resources, &update_map); - let network_entries = apply_network_controls(cli, network_entries, timeout_reached); + let network_entries = apply_network_controls(cli, &network_entries, timeout_reached); let mut result = json!({ "navigated": url, @@ -1336,7 +1336,7 @@ pub fn run_with_network( let wait_result = wait_after_navigate(&mut ctx, wait_opts)?; let wait_for_result = run_wait_for_predicates(&mut ctx, wait_opts)?; - let network_entries = apply_network_controls(cli, network_entries, timeout_reached); + let network_entries = apply_network_controls(cli, &network_entries, timeout_reached); let mut result = json!({ "navigated": url, @@ -1377,14 +1377,26 @@ pub fn run_with_network( /// Apply output controls (sort, limit, fields) to network entries from navigate. /// -/// In detail mode (when the user sets --detail, --jq, --sort, --limit, --fields, -/// or --all), returns the processed array. Otherwise returns a summary object. +/// Iteration 126: this always returns the ONE canonical network object built by +/// [`super::network::build_canonical_network`] — +/// `{entries, shown, total, truncated, total_requests, total_transfer_bytes, +/// by_cause_type, slowest, timeout_reached, ...}` — on every path (busy or +/// quiet page, detail or summary mode, `--all` or default). Previously this +/// flipped between a bare array (quiet/`--all`), a truncation object (busy), and +/// a summary object (non-detail), so `.results.network.entries` and +/// `.results.network.total_requests` threw `cannot index array` half the time. /// -/// `timeout_reached` is forwarded to [`build_network_summary`] so it can include -/// the hint field when the collection deadline fired while events were still arriving. +/// In detail mode (`--detail`/`--jq`/`--sort`/`--limit`/`--fields`/`--all`) the +/// `entries` list is sorted, capped at 20 (unless `--all`), and field-projected; +/// in summary mode `entries` carries the full unsorted capture. Summary fields +/// (`total_requests`, …) always reflect the full capture regardless of the view. +/// +/// `timeout_reached` is forwarded to [`super::network::build_network_summary`] +/// so the object carries the hint field when the collection deadline fired while +/// events were still arriving. fn apply_network_controls( cli: &Cli, - network_entries: Vec, + network_entries: &[serde_json::Value], timeout_reached: bool, ) -> serde_json::Value { let use_detail = cli.detail @@ -1396,7 +1408,7 @@ fn apply_network_controls( if use_detail { let controls = OutputControls::from_cli(cli, SortDir::Desc); - let mut detail = network_entries; + let mut detail = network_entries.to_vec(); if cli.sort.is_none() { let dir = controls.sort_dir; detail.sort_by(|a, b| { @@ -1413,20 +1425,29 @@ fn apply_network_controls( } let (limited, total, truncated) = controls.apply_limit(detail, Some(20)); let limited = controls.apply_fields(limited); - if truncated { - let shown = limited.len(); - json!({ - "entries": limited, - "shown": shown, - "total": total, - "truncated": true, - "hint": format!("showing {shown} of {total}, use --all for complete list"), - }) - } else { - json!(limited) - } + let shown = limited.len(); + // Summary fields are computed from the FULL capture (`network_entries`), + // never the truncated/field-projected `limited` view. + super::network::build_canonical_network( + limited, + shown, + total, + truncated, + network_entries, + timeout_reached, + ) } else { - super::network::build_network_summary(&network_entries, timeout_reached) + // Summary mode: `entries` carries the full unsorted capture so consumers + // can still reach `.entries` without flipping to detail mode. + let total = network_entries.len(); + super::network::build_canonical_network( + network_entries.to_vec(), + total, + total, + false, + network_entries, + timeout_reached, + ) } } diff --git a/crates/ff-rdp-cli/src/commands/network.rs b/crates/ff-rdp-cli/src/commands/network.rs index df4b263..f579470 100644 --- a/crates/ff-rdp-cli/src/commands/network.rs +++ b/crates/ff-rdp-cli/src/commands/network.rs @@ -285,6 +285,14 @@ pub fn run( if use_detail { let controls = OutputControls::from_cli(cli, SortDir::Desc); + // Iteration 126: keep the FULL entry list so the detail envelope can + // carry the same summary fields (total_requests, total_transfer_bytes, + // slowest, …) as summary mode — otherwise `--jq` users, who are forced + // into detail mode by the trigger list above, can never reach them. + // build_network_summary only reads url/status/duration_ms/transfer_size/ + // cause_type, so the internal `_resource_id` marker on these entries is + // harmless here. + let summary_source = results.clone(); let mut detail = results; // Default sort by duration_ms desc when no explicit sort is provided. if cli.sort.is_none() { @@ -384,6 +392,12 @@ pub fn run( let limited = controls.apply_fields(limited); let mut envelope = output::envelope_with_truncation(&json!(limited), shown, total, truncated, &meta); + // Iteration 126: carry the summary fields alongside `results` in the + // detail envelope so `--jq` consumers get total_requests etc. Summary + // counts are computed from the full capture, never the truncated view. + // `timeout_reached` is always false here: the non-timed drain used by + // the standalone `network` command stops on idle (see summary mode). + merge_summary_fields(&mut envelope, &summary_source, false); if let Some(hint) = empty_hint && let Some(obj) = envelope.as_object_mut() { @@ -701,6 +715,87 @@ pub fn build_network_summary( summary } +/// Merge the summary fields from [`build_network_summary`] into a target object. +/// +/// Copies `total_requests`, `total_transfer_bytes`, `by_cause_type`, `slowest` +/// and `timeout_reached` (plus the `hint` field when `timeout_reached` is true) +/// onto `target` without disturbing keys `target` already holds. Existing keys +/// on `target` win, so entry-level fields such as `entries`/`shown`/`total` are +/// never clobbered by the summary. +/// +/// `entries` is the FULL, unlimited entry list — summary counts (e.g. +/// `total_requests`) always reflect the whole capture, not the truncated view. +pub(crate) fn merge_summary_fields( + target: &mut serde_json::Value, + entries: &[serde_json::Value], + timeout_reached: bool, +) { + let summary = build_network_summary(entries, timeout_reached); + if let (Some(dst), Some(src)) = (target.as_object_mut(), summary.as_object()) { + for (k, v) in src { + // Do not overwrite entry-level keys the caller already set. + dst.entry(k.clone()).or_insert_with(|| v.clone()); + } + } +} + +/// Build the ONE canonical network object shape returned on every path. +/// +/// Iteration 126: `navigate --with-network` (and the standalone `network` +/// detail view) previously flipped between a bare array (quiet/`--all` pages), +/// a `{entries, shown, total, truncated, hint}` object (busy pages), and a +/// summary object (non-detail) — so `.results.network.entries` threw +/// `cannot index array` half the time and the documented summary fields were +/// unreachable via `--jq`. This builder returns a single object on every path: +/// +/// ```json +/// { +/// "entries": [ ... ], // limited + field-projected view +/// "shown": N, // entries.len() +/// "total": N, // total BEFORE the default 20-entry limit +/// "truncated": bool, // total > shown +/// "total_requests": N, // summary fields (from the FULL capture) +/// "total_transfer_bytes": N, +/// "by_cause_type": { ... }, +/// "slowest": [ ... ], +/// "timeout_reached": bool, +/// "hint": "..." // only when truncated or timeout_reached +/// } +/// ``` +/// +/// The summary counts are computed from `all_entries` (the full capture) while +/// `entries` carries the possibly-truncated, field-projected view, so +/// `--fields url` can never strip `total_requests` and `--limit`/the default cap +/// never distorts the totals. +pub(crate) fn build_canonical_network( + entries: Vec, + shown: usize, + total: usize, + truncated: bool, + all_entries: &[serde_json::Value], + timeout_reached: bool, +) -> serde_json::Value { + // Move `entries` into the object explicitly via Value::Array so the Vec is + // consumed rather than borrowed (avoids a needless clone). + let mut obj = json!({ + "entries": Value::Array(entries), + "shown": shown, + "total": total, + "truncated": truncated, + }); + merge_summary_fields(&mut obj, all_entries, timeout_reached); + // The summary's own `hint` (timeout) is preserved by merge_summary_fields; + // add the truncation hint only when it is not already present. + if truncated && let Some(map) = obj.as_object_mut() { + map.entry("hint".to_string()).or_insert_with(|| { + json!(format!( + "showing {shown} of {total}, use --all for complete list" + )) + }); + } + obj +} + /// Return buffered network events as a JSON array. /// /// Used by the script runner's `assert_network` step. @@ -1183,4 +1278,140 @@ mod tests { "null cause_type must NOT produce \"other\" key; got: {by_cause:?}" ); } + + // ----------------------------------------------------------------------- + // iter-126: canonical network object shape + // ----------------------------------------------------------------------- + + fn sample_entries(n: usize) -> Vec { + (0..n) + .map(|i| { + // Test indices are tiny; cast losslessly via u16 for a stable + // increasing duration without triggering cast_precision_loss. + let duration_ms = f64::from(u16::try_from(i).unwrap_or(u16::MAX)) * 10.0; + json!({ + "url": format!("https://example.com/{i}"), + "duration_ms": duration_ms, + "status": 200, + "transfer_size": 100.0, + "cause_type": "script", + }) + }) + .collect() + } + + #[test] + fn build_canonical_network_carries_entries_and_summary() { + let entries = sample_entries(3); + let obj = build_canonical_network(entries.clone(), 3, 3, false, &entries, false); + assert!(obj.is_object(), "canonical shape must be an object"); + // Entry-level keys. + assert!(obj["entries"].is_array()); + assert_eq!(obj["entries"].as_array().unwrap().len(), 3); + assert_eq!(obj["shown"], 3); + assert_eq!(obj["total"], 3); + assert_eq!(obj["truncated"], false); + // Summary keys ride alongside. + assert_eq!(obj["total_requests"], 3); + assert_eq!(obj["total_transfer_bytes"], 300.0); + assert!(obj["by_cause_type"].is_object()); + assert!(obj["slowest"].is_array()); + assert_eq!(obj["timeout_reached"], false); + } + + #[test] + fn build_canonical_network_empty_keeps_all_keys() { + // A zero-request page still carries entries:[] and total_requests:0 — + // keys present, not omitted (plan Task A, third bullet). + let obj = build_canonical_network(vec![], 0, 0, false, &[], false); + assert!(obj.is_object()); + assert!(obj["entries"].is_array()); + assert_eq!(obj["entries"].as_array().unwrap().len(), 0); + assert_eq!(obj["shown"], 0); + assert_eq!(obj["total"], 0); + assert_eq!(obj["truncated"], false); + assert_eq!(obj["total_requests"], 0); + assert_eq!(obj["total_transfer_bytes"], 0.0); + } + + #[test] + fn build_canonical_network_truncated_summary_reflects_full_capture() { + // The `entries` view is truncated to 2, but summary counts must reflect + // the FULL 5-entry capture, and a truncation hint is added. + let all = sample_entries(5); + let limited: Vec = all.iter().take(2).cloned().collect(); + let obj = build_canonical_network(limited, 2, 5, true, &all, false); + assert_eq!(obj["shown"], 2); + assert_eq!(obj["total"], 5); + assert_eq!(obj["truncated"], true); + // total_requests reflects the full capture, never the truncated view. + assert_eq!(obj["total_requests"], 5); + assert_eq!(obj["total_transfer_bytes"], 500.0); + let hint = obj["hint"].as_str().expect("truncation hint present"); + assert!(hint.contains("--all"), "hint should mention --all: {hint}"); + } + + #[test] + fn build_canonical_network_timeout_hint_wins_over_truncation() { + // When both timeout and truncation could add a hint, the timeout hint + // (from build_network_summary) is set first and must not be overwritten. + let all = sample_entries(5); + let limited: Vec = all.iter().take(2).cloned().collect(); + let obj = build_canonical_network(limited, 2, 5, true, &all, true); + assert_eq!(obj["timeout_reached"], true); + let hint = obj["hint"].as_str().expect("hint present"); + assert!( + hint.contains("--network-timeout"), + "timeout hint must win over truncation hint: {hint}" + ); + } + + #[test] + fn merge_summary_fields_does_not_clobber_entry_keys() { + let all = sample_entries(2); + let mut target = json!({ + "entries": [{"url": "kept"}], + "shown": 1, + "total": 2, + "truncated": true, + }); + merge_summary_fields(&mut target, &all, false); + // Entry keys survive; summary keys are added. + assert_eq!(target["entries"][0]["url"], "kept"); + assert_eq!(target["shown"], 1); + assert_eq!(target["total"], 2); + assert_eq!(target["truncated"], true); + assert_eq!(target["total_requests"], 2); + assert!(target["slowest"].is_array()); + } + + #[test] + fn network_and_navigate_summary_fields_agree_field_for_field() { + // Parity assertion (iter-125 precedent): the summary fields carried by + // the standalone `network` detail envelope and by the `navigate + // --with-network` canonical object must be byte-identical for the same + // capture. Both go through merge_summary_fields / build_network_summary, + // so extract each side's summary key set and compare. + let entries = sample_entries(4); + + // navigate side: the canonical object embeds summary fields directly. + let nav = build_canonical_network(entries.clone(), 4, 4, false, &entries, false); + + // network side: summary fields are merged onto the envelope. + let mut net_env = json!({ "results": [], "total": 4 }); + merge_summary_fields(&mut net_env, &entries, false); + + for key in [ + "total_requests", + "total_transfer_bytes", + "by_cause_type", + "slowest", + "timeout_reached", + ] { + assert_eq!( + nav[key], net_env[key], + "summary field `{key}` must agree between navigate and network shapes" + ); + } + } } diff --git a/crates/ff-rdp-cli/tests/e2e/navigate.rs b/crates/ff-rdp-cli/tests/e2e/navigate.rs index 7b2c6d2..ef49976 100644 --- a/crates/ff-rdp-cli/tests/e2e/navigate.rs +++ b/crates/ff-rdp-cli/tests/e2e/navigate.rs @@ -194,9 +194,10 @@ fn navigate_with_network_captures_requests() { // The navigated field is present. assert_eq!(json["results"]["navigated"], "https://example.com"); - // Default mode returns a summary object, not a raw array. + // iter-126: the canonical shape is ONE object on every path, never a bare + // array. Default mode carries both the summary fields and an `entries` array. let network = &json["results"]["network"]; - assert!(network.is_object(), "network should be a summary object"); + assert!(network.is_object(), "network should be a canonical object"); assert_eq!(network["total_requests"], 2, "expected 2 network entries"); // total reflects the outer envelope (single navigate result). @@ -206,6 +207,110 @@ fn navigate_with_network_captures_requests() { assert!(network["total_transfer_bytes"].is_number()); assert!(network["by_cause_type"].is_object()); assert!(network["slowest"].is_array()); + + // iter-126: `.entries` is reachable (array) even in default/summary mode — + // no more "cannot index array" when a consumer probes .entries. + assert!( + network["entries"].is_array(), + "network.entries must be an array in default mode, got: {}", + network["entries"] + ); + assert_eq!(network["entries"].as_array().unwrap().len(), 2); + assert_eq!(network["shown"], 2); + assert_eq!(network["total"], 2); + assert_eq!(network["truncated"], false); +} + +#[test] +fn navigate_with_network_detail_mode_is_object_not_array() { + // iter-126 regression: --detail (a detail-mode trigger) previously returned + // a bare array on quiet pages (≤20 entries), so `.results.network.entries` + // and `.results.network.total_requests` threw "cannot index array". Assert + // the canonical object shape with both entries and summary fields present. + let server = navigate_with_network_server(); + let port = server.port(); + let handle = std::thread::spawn(move || server.serve_one()); + + let mut args = base_args(port); + args.extend([ + "navigate".to_owned(), + "https://example.com".to_owned(), + "--with-network".to_owned(), + "--detail".to_owned(), + ]); + + let output = std::process::Command::new(ff_rdp_bin()) + .args(&args) + .output() + .expect("failed to spawn ff-rdp"); + + handle.join().unwrap(); + + assert!( + output.status.success(), + "expected success, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let json: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("stdout must be valid JSON"); + + let network = &json["results"]["network"]; + assert!( + network.is_object(), + "detail-mode network must be a canonical object, not a bare array, got: {network}" + ); + // Both entry-level and summary keys are present in detail mode. + assert!(network["entries"].is_array(), "entries must be an array"); + assert_eq!(network["entries"].as_array().unwrap().len(), 2); + assert_eq!(network["total_requests"], 2); + assert!(network["total_transfer_bytes"].is_number()); + assert!(network["slowest"].is_array()); + assert_eq!(network["truncated"], false); +} + +#[test] +fn navigate_with_network_all_keeps_object_shape() { + // iter-126 AC (live_navigate_with_network_all_keeps_summary equivalent under + // the mock server): --all is a detail-mode trigger that previously produced + // a bare array dump. Assert it now keeps the object shape with summary fields. + let server = navigate_with_network_server(); + let port = server.port(); + let handle = std::thread::spawn(move || server.serve_one()); + + let mut args = base_args(port); + args.extend([ + "navigate".to_owned(), + "https://example.com".to_owned(), + "--with-network".to_owned(), + "--all".to_owned(), + ]); + + let output = std::process::Command::new(ff_rdp_bin()) + .args(&args) + .output() + .expect("failed to spawn ff-rdp"); + + handle.join().unwrap(); + + assert!( + output.status.success(), + "expected success, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let json: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("stdout must be valid JSON"); + + let network = &json["results"]["network"]; + assert!( + network.is_object(), + "--all network must stay an object, not a bare array, got: {network}" + ); + assert!(network["entries"].is_array()); + assert_eq!(network["entries"].as_array().unwrap().len(), 2); + assert_eq!(network["total_requests"], 2); + assert_eq!(network["truncated"], false); } #[test] @@ -296,10 +401,20 @@ fn navigate_with_network_empty_when_no_events() { assert_eq!(json["results"]["navigated"], "https://example.com"); - // Default mode returns a summary object even when there are no entries. + // iter-126: even a zero-request page carries the canonical object with + // `entries: []` and `total_requests: 0` — keys are present, not omitted. let network = &json["results"]["network"]; - assert!(network.is_object(), "network should be a summary object"); + assert!(network.is_object(), "network should be a canonical object"); assert_eq!(network["total_requests"], 0, "expected no network entries"); + assert!( + network["entries"].is_array(), + "entries must be [] not absent on a zero-request page, got: {}", + network["entries"] + ); + assert_eq!(network["entries"].as_array().unwrap().len(), 0); + assert_eq!(network["shown"], 0); + assert_eq!(network["total"], 0); + assert_eq!(network["truncated"], false); assert_eq!(json["total"], 1); } diff --git a/crates/ff-rdp-cli/tests/e2e/network.rs b/crates/ff-rdp-cli/tests/e2e/network.rs index 524f08a..d73ffd5 100644 --- a/crates/ff-rdp-cli/tests/e2e/network.rs +++ b/crates/ff-rdp-cli/tests/e2e/network.rs @@ -175,6 +175,20 @@ fn network_detail_shows_requests() { assert_eq!(results[1]["method"], "GET"); assert_eq!(results[1]["url"], "https://example.com/favicon.ico"); assert_eq!(results[1]["status"], 404); + + // iter-126: detail mode (which --jq forces) now carries the summary fields + // alongside `results`, so consumers are not cut off from total_requests etc. + assert_eq!( + json["total_requests"], 2, + "detail envelope must carry total_requests, got: {json}" + ); + assert!( + json["total_transfer_bytes"].is_number(), + "detail envelope must carry total_transfer_bytes" + ); + assert!(json["by_cause_type"].is_object()); + assert!(json["slowest"].is_array()); + assert_eq!(json["timeout_reached"], false); } // --------------------------------------------------------------------------- diff --git a/crates/ff-rdp-cli/tests/live/live_126_network_shape.rs b/crates/ff-rdp-cli/tests/live/live_126_network_shape.rs new file mode 100644 index 0000000..e0ae4a9 --- /dev/null +++ b/crates/ff-rdp-cli/tests/live/live_126_network_shape.rs @@ -0,0 +1,333 @@ +//! Live tests for iter-126 — canonical network JSON shape. +//! +//! `navigate --with-network` (and the standalone `network` detail view) used to +//! flip between a `{entries, …}` object on busy pages and a BARE ARRAY on quiet +//! ones, so `.results.network.entries` / `.results.network.total_requests` threw +//! `cannot index array` half the time. These tests assert the ONE canonical +//! object shape on every path: quiet page, busy page, `--all`, and the +//! standalone `network` detail envelope carrying summary fields. +//! +//! # Running +//! +//! Requires Firefox, network access (example.com + a busy page), and the +//! ff-rdp binary. Gates on `FF_RDP_LIVE_NETWORK_TESTS=1`. +//! +//! FF_RDP_LIVE_NETWORK_TESTS=1 cargo test -p ff-rdp-cli --test live live_126 -- --nocapture + +use std::process::{Command, Output}; + +use crate::common::{LiveFirefox, ff_rdp_bin}; + +fn parse_json(output: &Output) -> serde_json::Value { + let s = String::from_utf8_lossy(&output.stdout); + serde_json::from_str(s.trim()).unwrap_or_else(|e| { + panic!( + "stdout is not valid JSON: {e}\nstdout={s}\nstderr={}", + String::from_utf8_lossy(&output.stderr) + ) + }) +} + +fn base_args(port: u16) -> Vec { + vec![ + "--host".to_owned(), + "127.0.0.1".to_owned(), + "--port".to_owned(), + port.to_string(), + "--timeout".to_owned(), + "30000".to_owned(), + ] +} + +fn stop_daemon(port: u16) { + let _ = Command::new(ff_rdp_bin()) + .args([ + "--host", + "127.0.0.1", + "--port", + &port.to_string(), + "daemon", + "stop", + ]) + .output(); +} + +/// Assert the value at `.results.network` is the canonical object with all +/// entry-level and summary keys present. Returns the object for further checks. +fn assert_canonical_network(json: &serde_json::Value) { + let network = &json["results"]["network"]; + assert!( + network.is_object(), + "results.network must be an object (never a bare array), got: {network}" + ); + // Entry-level keys. + assert!( + network["entries"].is_array(), + "results.network.entries must be an array — no `cannot index array`, got: {}", + network["entries"] + ); + assert!(network["shown"].is_u64(), "shown must be numeric"); + assert!(network["total"].is_u64(), "total must be numeric"); + assert!(network["truncated"].is_boolean(), "truncated must be bool"); + // Summary keys. + assert!( + network["total_requests"].is_u64(), + "results.network.total_requests must be numeric, got: {}", + network["total_requests"] + ); + assert!( + network["total_transfer_bytes"].is_number(), + "total_transfer_bytes must be numeric" + ); + assert!(network["by_cause_type"].is_object()); + assert!(network["slowest"].is_array()); +} + +/// The canonical key set on `.results.network`, used for key-by-key parity. +fn network_keys(json: &serde_json::Value) -> Vec { + let mut keys: Vec = json["results"]["network"] + .as_object() + .expect("results.network is an object") + .keys() + .cloned() + .collect(); + keys.sort(); + keys +} + +/// `live_navigate_with_network_shape_quiet`: `navigate --with-network --jq` on a +/// quiet page (example.com, ≤20 requests) yields `.results.network.entries` of +/// type array and a numeric `.results.network.total_requests` — no bare array. +/// +/// `live_navigate_with_network_shape_busy`: the same on a busy page (>20 +/// requests) yields the identical key set, with `truncated == true`, +/// `shown == 20`, and `total_requests >= total`. +/// +/// Both branches run inside one Firefox instance and the quiet/busy key sets +/// are asserted equal key-by-key. +#[test] +#[ignore = "requires Firefox, network access, and FF_RDP_LIVE_NETWORK_TESTS=1"] +fn live_navigate_with_network_shape_quiet_and_busy() { + if std::env::var("FF_RDP_LIVE_NETWORK_TESTS").is_err() { + eprintln!( + "live_navigate_with_network_shape_quiet_and_busy: set FF_RDP_LIVE_NETWORK_TESTS=1 to run" + ); + return; + } + + let Some(ff) = LiveFirefox::headless_on_random_port() else { + eprintln!( + "live_navigate_with_network_shape_quiet_and_busy: Firefox not available — skipping" + ); + return; + }; + let port = ff.port(); + + // --- Quiet page: example.com, ≤20 requests. --- + let quiet = Command::new(ff_rdp_bin()) + .args(base_args(port)) + .args([ + "navigate", + "https://example.com", + "--with-network", + "--jq", + ".", + ]) + .output() + .expect("navigate quiet --with-network"); + if !quiet.status.success() { + stop_daemon(port); + eprintln!( + "live_navigate_with_network_shape_quiet_and_busy: quiet navigate failed — {}", + String::from_utf8_lossy(&quiet.stderr) + ); + return; + } + let quiet_json = parse_json(&quiet); + assert_canonical_network(&quiet_json); + let quiet_net = &quiet_json["results"]["network"]; + assert!( + quiet_net["total_requests"].as_u64().unwrap() <= 20, + "example.com should be a quiet (≤20) page, got {}", + quiet_net["total_requests"] + ); + assert_eq!( + quiet_net["truncated"], false, + "quiet page must not truncate" + ); + + // --- Busy page: >20 requests. --- + let busy = Command::new(ff_rdp_bin()) + .args(base_args(port)) + .args([ + "navigate", + "https://en.wikipedia.org/wiki/Firefox", + "--with-network", + "--jq", + ".", + ]) + .output() + .expect("navigate busy --with-network"); + stop_daemon(port); + if !busy.status.success() { + eprintln!( + "live_navigate_with_network_shape_quiet_and_busy: busy navigate failed — {}", + String::from_utf8_lossy(&busy.stderr) + ); + return; + } + let busy_json = parse_json(&busy); + assert_canonical_network(&busy_json); + let busy_net = &busy_json["results"]["network"]; + let busy_total = busy_net["total"].as_u64().unwrap(); + let busy_total_requests = busy_net["total_requests"].as_u64().unwrap(); + assert!( + busy_total_requests > 20, + "wikipedia should be a busy (>20) page, got {busy_total_requests}" + ); + assert_eq!(busy_net["truncated"], true, "busy page must truncate"); + assert_eq!(busy_net["shown"], 20, "busy page shows 20 by default"); + assert!( + busy_total_requests >= busy_total, + "total_requests ({busy_total_requests}) >= total ({busy_total})" + ); + + // Shape equality: the quiet and busy key sets must be identical. + assert_eq!( + network_keys(&quiet_json), + network_keys(&busy_json), + "quiet and busy network shapes must have the same key set" + ); + + eprintln!( + "live_navigate_with_network_shape_quiet_and_busy: PASSED — quiet total_requests={}, busy total_requests={busy_total_requests}", + quiet_net["total_requests"] + ); +} + +/// `live_navigate_with_network_all_keeps_summary`: adding `--all` still returns +/// the object shape (full `entries`, summary fields intact) — never a bare array. +#[test] +#[ignore = "requires Firefox, network access, and FF_RDP_LIVE_NETWORK_TESTS=1"] +fn live_navigate_with_network_all_keeps_summary() { + if std::env::var("FF_RDP_LIVE_NETWORK_TESTS").is_err() { + eprintln!( + "live_navigate_with_network_all_keeps_summary: set FF_RDP_LIVE_NETWORK_TESTS=1 to run" + ); + return; + } + + let Some(ff) = LiveFirefox::headless_on_random_port() else { + eprintln!("live_navigate_with_network_all_keeps_summary: Firefox not available — skipping"); + return; + }; + let port = ff.port(); + + let out = Command::new(ff_rdp_bin()) + .args(base_args(port)) + .args([ + "navigate", + "https://en.wikipedia.org/wiki/Firefox", + "--with-network", + "--all", + "--jq", + ".", + ]) + .output() + .expect("navigate --with-network --all"); + stop_daemon(port); + if !out.status.success() { + eprintln!( + "live_navigate_with_network_all_keeps_summary: navigate failed — {}", + String::from_utf8_lossy(&out.stderr) + ); + return; + } + let json = parse_json(&out); + assert_canonical_network(&json); + let net = &json["results"]["network"]; + // --all expands entries to the full capture (shown == total), summary intact. + assert_eq!( + net["shown"].as_u64().unwrap(), + net["total"].as_u64().unwrap(), + "--all must show every entry (shown == total)" + ); + assert_eq!(net["truncated"], false, "--all must not truncate"); + assert!( + net["total_requests"].as_u64().unwrap() >= 1, + "summary fields must remain present under --all" + ); + eprintln!( + "live_navigate_with_network_all_keeps_summary: PASSED — shown={} total_requests={}", + net["shown"], net["total_requests"] + ); +} + +/// `live_network_detail_carries_summary`: standalone `network --jq` returns the +/// summary fields (`total_requests`, `total_transfer_bytes`) alongside the entry +/// list on a page with captured traffic. +#[test] +#[ignore = "requires Firefox, network access, and FF_RDP_LIVE_NETWORK_TESTS=1"] +fn live_network_detail_carries_summary() { + if std::env::var("FF_RDP_LIVE_NETWORK_TESTS").is_err() { + eprintln!("live_network_detail_carries_summary: set FF_RDP_LIVE_NETWORK_TESTS=1 to run"); + return; + } + + let Some(ff) = LiveFirefox::headless_on_random_port() else { + eprintln!("live_network_detail_carries_summary: Firefox not available — skipping"); + return; + }; + let port = ff.port(); + + // Capture traffic first. + let nav = Command::new(ff_rdp_bin()) + .args(base_args(port)) + .args(["navigate", "https://example.com", "--with-network"]) + .output() + .expect("navigate --with-network"); + if !nav.status.success() { + stop_daemon(port); + eprintln!( + "live_network_detail_carries_summary: navigate failed — {}", + String::from_utf8_lossy(&nav.stderr) + ); + return; + } + + // Standalone `network --jq` forces detail mode; summary fields must ride along. + let net = Command::new(ff_rdp_bin()) + .args(base_args(port)) + .args(["network", "--jq", "."]) + .output() + .expect("network --jq"); + stop_daemon(port); + if !net.status.success() { + eprintln!( + "live_network_detail_carries_summary: network failed — {}", + String::from_utf8_lossy(&net.stderr) + ); + return; + } + let json = parse_json(&net); + // Detail mode: results is the entry array, summary fields at the envelope top. + assert!( + json["results"].is_array(), + "network --jq detail results must be an array, got: {}", + json["results"] + ); + assert!( + json["total_requests"].is_u64(), + "detail envelope must carry total_requests, got: {json}" + ); + assert!( + json["total_transfer_bytes"].is_number(), + "detail envelope must carry total_transfer_bytes" + ); + assert!(json["by_cause_type"].is_object()); + assert!(json["slowest"].is_array()); + eprintln!( + "live_network_detail_carries_summary: PASSED — total_requests={}", + json["total_requests"] + ); +} diff --git a/crates/ff-rdp-cli/tests/live/main.rs b/crates/ff-rdp-cli/tests/live/main.rs index e6ebf49..3751f70 100644 --- a/crates/ff-rdp-cli/tests/live/main.rs +++ b/crates/ff-rdp-cli/tests/live/main.rs @@ -40,6 +40,7 @@ mod live_110_kill_scoping; mod live_111_daemon_follow_cross_process; mod live_113_launch_timeout; mod live_123_daemon_autostart_and_registry; +mod live_126_network_shape; mod live_61l; mod live_61q_resource_bus; mod live_61r_eval; diff --git a/kb/iterations/iteration-126-network-json-shape-consistency.md b/kb/iterations/iteration-126-network-json-shape-consistency.md index b13d3d0..fefa7b1 100644 --- a/kb/iterations/iteration-126-network-json-shape-consistency.md +++ b/kb/iterations/iteration-126-network-json-shape-consistency.md @@ -2,7 +2,7 @@ title: "Iteration 126: network / navigate --with-network JSON shape flips between object and bare array" type: iteration date: 2026-07-19 -status: planned +status: done branch: iter-126/network-json-shape-consistency depends_on: [] firefox_refs: [] @@ -80,46 +80,59 @@ standalone `network` command has the same object/array divergence on `.results`: ### A. Canonical shape in navigate --with-network -- [ ] Rework `apply_network_controls` (`navigate.rs:1291-1337`) to always return one object: - merge `build_network_summary` output with `entries`/`shown`/`total`/`truncated` so the - truncated (`navigate.rs:1322-1330`), non-truncated (`navigate.rs:1331-1333`), and - summary (`navigate.rs:1334-1336`) branches converge on the same key set. -- [ ] Keep the default entry limit (20) on the detail path so the canonical shape does not +- [x] Rework `apply_network_controls` to always return one object: it now delegates to the + shared `build_canonical_network` builder on BOTH the detail and summary branches, so + the truncated, non-truncated (was bare array), and summary paths converge on the same + key set `{entries, shown, total, truncated, total_requests, total_transfer_bytes, + by_cause_type, slowest, timeout_reached, …}`. +- [x] Keep the default entry limit (20) on the detail path so the canonical shape does not reintroduce the ~13 KB dump; `--all` still expands `entries` but keeps the summary - fields and `total_requests` alongside. -- [ ] `build_network_summary` (`network.rs:638-706`) already yields sane zero values for an - empty slice (`network.rs:642-658`); assert that the canonical object on a zero-request - page carries `entries: []` and `total_requests: 0` rather than omitting keys. -- [ ] Re-record e2e fixtures for both the quiet and busy shapes via - `live_record_fixtures.rs` (never hand-crafted) and update the shape assertions in - `crates/ff-rdp-cli/tests/e2e/navigate.rs` and `tests/e2e/network.rs`. + fields and `total_requests` alongside (summary counts come from the full capture, not + the truncated view). +- [x] `build_network_summary` already yields sane zero values for an empty slice; the + canonical object on a zero-request page carries `entries: []` and `total_requests: 0` + rather than omitting keys — asserted by e2e `navigate_with_network_empty_when_no_events` + and unit `build_canonical_network_empty_keeps_all_keys`. +- [x] Reused the existing real-Firefox-recorded fixtures (the canonical shape is derived from + the same recorded entries, so no re-record was needed) and updated the shape assertions + in `crates/ff-rdp-cli/tests/e2e/navigate.rs` (added `..._detail_mode_is_object_not_array`, + `..._all_keeps_object_shape`) and `tests/e2e/network.rs` (summary fields in + `network_detail_shows_requests`). Busy/truncated + quiet-page shapes exercised live in + `live_126_network_shape.rs`. ### B. Standalone network command + contract docs -- [ ] Extend the `network` detail envelope (`network.rs:384-402`) with the same summary - fields (`total_requests`, `total_transfer_bytes`, `slowest`, …) so `--jq` users are not - cut off from them by the detail-mode trigger (`network.rs:262-269`). -- [ ] Update the help text: the summary-shape line (`args.rs:555`) plus the - `navigate --with-network` usage sections (`args.rs:18`, `args.rs:126`, `args.rs:436`) - describe the canonical object and carry a one-line backward-compat note ("previously a - bare array in non-truncated detail mode"). +- [x] Extended the `network` detail envelope with the same summary fields via + `merge_summary_fields` (computed from the full capture `summary_source`) so `--jq` users + are not cut off from `total_requests`/`total_transfer_bytes`/`slowest` by the detail-mode + trigger. +- [x] Updated the help text: the summary-shape line (`args.rs`) plus the + `navigate --with-network` `long_about` section now describe the canonical object and + carry a one-line backward-compat note ("previously a bare array in non-truncated detail + mode"). -## Acceptance Criteria [0/5] +## Acceptance Criteria [5/5] -- [ ] live_navigate_with_network_shape_quiet: `navigate --with-network --jq` on a quiet page - (example.com class, ≤20 requests) yields `.results.network.entries` of type array and a - numeric `.results.network.total_requests` — no bare array, no `cannot index array`. -- [ ] live_navigate_with_network_shape_busy: the same invocation on a busy page (>20 - requests) yields the identical key set, with `truncated == true`, `shown == 20`, and - `total_requests >= total` — shape equality with the quiet case asserted key-by-key. -- [ ] live_navigate_with_network_all_keeps_summary: adding `--all` still returns the object - shape (full `entries`, summary fields intact) — never a bare array. -- [ ] live_network_detail_carries_summary: standalone `network --jq` returns summary fields +- [x] live_navigate_with_network_shape_quiet_and_busy (quiet half): `navigate --with-network + --jq` on a quiet page (example.com, ≤20 requests) yields `.results.network.entries` of + type array and a numeric `.results.network.total_requests` — no bare array, no `cannot + index array`. Also covered by mock e2e `navigate_with_network_captures_requests` and + unit `build_canonical_network_carries_entries_and_summary`. +- [x] live_navigate_with_network_shape_quiet_and_busy (busy half): the same invocation on a + busy page (wikipedia, >20 requests) yields the identical key set, with `truncated == + true`, `shown == 20`, and `total_requests >= total` — quiet/busy key sets asserted equal + via `network_keys()`. Truncation math covered by unit + `build_canonical_network_truncated_summary_reflects_full_capture`. +- [x] live_navigate_with_network_all_keeps_summary: adding `--all` still returns the object + shape (full `entries`, summary fields intact) — never a bare array. Mock e2e: + `navigate_with_network_all_keeps_object_shape`. +- [x] live_network_detail_carries_summary: standalone `network --jq` returns summary fields (`total_requests`, `total_transfer_bytes`) alongside the entry list on a page with - captured traffic. -- [ ] `cargo fmt && cargo clippy --workspace --all-targets -- -D warnings && cargo test --workspace -q` clean. + captured traffic. Mock e2e: `network_detail_shows_requests`; parity unit + `network_and_navigate_summary_fields_agree_field_for_field`. +- [x] `cargo fmt && cargo clippy --workspace --all-targets -- -D warnings && cargo test --workspace -q` clean. ## Design notes From d04ca984b5666956f70d647a6a238b9b7a284bcc Mon Sep 17 00:00:00 2001 From: Jean-Pierre Bergamin Date: Sun, 19 Jul 2026 18:00:31 +0200 Subject: [PATCH 2/2] chore(iter-127): fix args.rs line drift from iter-126, add parity-test note iter-126 inserted 6 lines earlier in args.rs (navigate/network help text), shifting the a11y-contrast `total` help-text line reference from 654 to 660. Also note the iter-126 parity-test / dogfood-compact-jq precedent as a design note so iter-127's fix pins exact field values, not just presence. --- .../iteration-127-a11y-contrast-fail-only-total.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/kb/iterations/iteration-127-a11y-contrast-fail-only-total.md b/kb/iterations/iteration-127-a11y-contrast-fail-only-total.md index cce3cce..60205ca 100644 --- a/kb/iterations/iteration-127-a11y-contrast-fail-only-total.md +++ b/kb/iterations/iteration-127-a11y-contrast-fail-only-total.md @@ -47,7 +47,7 @@ Root cause in `crates/ff-rdp-cli/src/commands/a11y_contrast.rs`: `total` is the post-filter/pre-limit count from `apply_limit`, `a11y_contrast.rs:76`) — so the top-level `total` reports the sampled count whenever it exceeds the failure count, i.e. always under `--fail-only` on a mostly-passing page. - The help text (`crates/ff-rdp-cli/src/cli/args.rs:654`) documents `"total": N` with no + The help text (`crates/ff-rdp-cli/src/cli/args.rs:660`) documents `"total": N` with no hint that it can exceed `results`. The `.max()` was presumably meant to keep `total` honest when the output limit truncates @@ -79,7 +79,7 @@ The `.max()` was presumably meant to keep `total` honest when the output limit t - [ ] Emit `sampled` (from JS `summary.total`, `a11y_contrast.rs:53-58`) at the top level of the envelope next to `total`, and keep the existing `meta.summary` (`a11y_contrast.rs:60-64`) untouched for aa_pass/aa_fail/capped detail. -- [ ] Update the help text (`args.rs:654`, plus the `--fail-only` usage lines at `args.rs:62`, +- [ ] Update the help text (`args.rs:660`, plus the `--fail-only` usage lines at `args.rs:62`, `args.rs:128`, `args.rs:153`) to document `total` = returned results (failures when `--fail-only`) and `sampled` = elements checked, with a one-line backward-compat note that `total` previously reported the sample size. @@ -108,6 +108,12 @@ The `.max()` was presumably meant to keep `total` honest when the output limit t (`crates/ff-rdp-cli/src/output.rs:28`) everywhere else receives the post-filter population count, with `shown` covering the limited slice — a11y contrast becomes consistent instead of special. +- iter-126 precedent (output-contract fixes): keep the parity discipline from + `network_and_navigate_summary_fields_agree_field_for_field` — write one unit test that pins + the exact field values (not just presence/type) for both the zero-failure and + known-failure-count cases, and record the live dogfood transcript in the same + `{t, n}`-style compact `--jq` projection used by `dogfood_path` above so a shape regression + is caught by the pre-PR gate, not just by hand-inspection. - Without `--fail-only`, `total == sampled` by construction; emitting both keeps the shape stable across flag combinations (no key that appears only with the flag).