From 4706420e948ac6d24bbca7c61dc240c8d9bf09f2 Mon Sep 17 00:00:00 2001 From: batistalucasv Date: Wed, 11 Nov 2026 15:11:33 -0300 Subject: [PATCH] tools: chunk LCS alignment past MAX_LCS window to prevent false revisions (#6110) (#6124) --- crates/engine/src/cmd/tools/compare.rs | 116 ++++--- crates/engine/src/cmd/tools/tests.rs | 421 +++++++++++++++++++++++++ 2 files changed, 498 insertions(+), 39 deletions(-) diff --git a/crates/engine/src/cmd/tools/compare.rs b/crates/engine/src/cmd/tools/compare.rs index 3709cb6ca0..a62397d830 100644 --- a/crates/engine/src/cmd/tools/compare.rs +++ b/crates/engine/src/cmd/tools/compare.rs @@ -202,13 +202,14 @@ fn block_text(b: &Block, depth: usize) -> String { } } -/// A copy of the block with every run (in tables, every cell's) changed by `f`. +/// Returns a copy of `b` with all text runs and paragraph marks (or nested table cells) updated by `f`. fn mark_block(b: &Block, f: &dyn Fn(&mut CharProps), depth: usize) -> Block { match b { Block::Para(p) => { let mut p = p.clone(); let len = p.len(); let _ = p.format(0, len, f); + f(&mut p.mark); Block::Para(p) } Block::Table(t) => { @@ -230,59 +231,96 @@ pub(super) enum Op { Ins, } -/// Longest sequence the comparison table is built for; the rest of a longer sequence is -/// compared as deleted and inserted, never dropped. +/// Longest sequence window per LCS comparison table chunk; sequences longer than this are +/// aligned in successive chunked windows rather than truncated, preserving tail alignment. const MAX_LCS: usize = 4000; /// Comparison table cells one compare may fill over all its tables (blocks and every changed /// paragraph pair); once spent, the remaining middles are compared as deleted and inserted. const MAX_COMPARE_CELLS: u64 = 50_000_000; -/// LCS edit script between two sequences: the common start and end are matched first, then the -/// middle (bounded size). The middle's table is charged to `budget`; when it doesn't fit, the -/// middle is deleted and inserted whole. +/// Computes the longest common subsequence (LCS) edit script (`Op::Same`, `Op::Del`, `Op::Ins`) +/// between sequences `a` and `b`. +/// +/// Strips common prefix and suffix first, then iterates over windows capped at [`MAX_LCS`], +/// charging comparison cells against `budget`. Between windows, intermediate common prefixes +/// are trimmed to re-anchor alignment without table allocations. When the remaining cells +/// exceed `budget`, the unaligned tail degrades honestly as whole deletions and insertions. pub(super) fn lcs_ops(a: &[T], b: &[T], budget: &mut u64) -> Vec { let pre = a.iter().zip(b).take_while(|(x, y)| x == y).count(); let (ra, rb) = (a.get(pre..).unwrap_or_default(), b.get(pre..).unwrap_or_default()); let suf = ra.iter().rev().zip(rb.iter().rev()).take_while(|(x, y)| x == y).count(); - let (a, b) = (ra.get(..ra.len() - suf).unwrap_or_default(), rb.get(..rb.len() - suf).unwrap_or_default()); + let mut rem_a = ra.get(..ra.len().saturating_sub(suf)).unwrap_or_default(); + let mut rem_b = rb.get(..rb.len().saturating_sub(suf)).unwrap_or_default(); let mut ops = vec![Op::Same; pre]; - let (mut n, mut m) = (a.len().min(MAX_LCS), b.len().min(MAX_LCS)); - let cells = (n as u64).saturating_mul(m as u64); - if cells > *budget { - (n, m) = (0, 0); - } else { - *budget -= cells; - } - let mut t = vec![vec![0u32; m + 1]; n + 1]; - for i in (0..n).rev() { - for j in (0..m).rev() { - let v = if a.get(i) == b.get(j) { - t.get(i + 1).and_then(|r| r.get(j + 1)).copied().unwrap_or(0) + 1 - } else { - t.get(i + 1).and_then(|r| r.get(j)).copied().unwrap_or(0).max(t.get(i).and_then(|r| r.get(j + 1)).copied().unwrap_or(0)) - }; - if let Some(c) = t.get_mut(i).and_then(|r| r.get_mut(j)) { - *c = v; + + while !rem_a.is_empty() && !rem_b.is_empty() { + let p = rem_a.iter().zip(rem_b).take_while(|(x, y)| x == y).count(); + if p > 0 { + ops.extend(std::iter::repeat_n(Op::Same, p)); + rem_a = rem_a.get(p..).unwrap_or_default(); + rem_b = rem_b.get(p..).unwrap_or_default(); + if rem_a.is_empty() || rem_b.is_empty() { + break; } } - } - let (mut i, mut j) = (0, 0); - while i < n && j < m { - if a.get(i) == b.get(j) { - ops.push(Op::Same); - i += 1; - j += 1; - } else if t.get(i + 1).and_then(|r| r.get(j)) >= t.get(i).and_then(|r| r.get(j + 1)) { - ops.push(Op::Del); - i += 1; - } else { - ops.push(Op::Ins); - j += 1; + + let n = rem_a.len().min(MAX_LCS); + let m = rem_b.len().min(MAX_LCS); + let cells = (n as u64).saturating_mul(m as u64); + if cells > *budget || n == 0 || m == 0 { + break; } + *budget = budget.saturating_sub(cells); + + let stride = m.saturating_add(1); + let table_len = n.saturating_add(1).saturating_mul(stride); + let mut t = vec![0u32; table_len]; + + for i in (0..n).rev() { + let row = i.saturating_mul(stride); + let next_row = i.saturating_add(1).saturating_mul(stride); + for j in (0..m).rev() { + let v = if rem_a.get(i) == rem_b.get(j) { + t.get(next_row.saturating_add(j).saturating_add(1)).copied().unwrap_or(0).saturating_add(1) + } else { + let down = t.get(next_row.saturating_add(j)).copied().unwrap_or(0); + let right = t.get(row.saturating_add(j).saturating_add(1)).copied().unwrap_or(0); + down.max(right) + }; + if let Some(c) = t.get_mut(row.saturating_add(j)) { + *c = v; + } + } + } + + let (mut i, mut j) = (0usize, 0usize); + while i < n && j < m { + let row = i.saturating_mul(stride); + let next_row = i.saturating_add(1).saturating_mul(stride); + if rem_a.get(i) == rem_b.get(j) { + ops.push(Op::Same); + i = i.saturating_add(1); + j = j.saturating_add(1); + } else { + let down = t.get(next_row.saturating_add(j)).copied().unwrap_or(0); + let right = t.get(row.saturating_add(j).saturating_add(1)).copied().unwrap_or(0); + if down >= right { + ops.push(Op::Del); + i = i.saturating_add(1); + } else { + ops.push(Op::Ins); + j = j.saturating_add(1); + } + } + } + + rem_a = rem_a.get(i..).unwrap_or_default(); + rem_b = rem_b.get(j..).unwrap_or_default(); } - ops.extend(std::iter::repeat_n(Op::Del, a.len() - i)); - ops.extend(std::iter::repeat_n(Op::Ins, b.len() - j)); + + ops.extend(std::iter::repeat_n(Op::Del, rem_a.len())); + ops.extend(std::iter::repeat_n(Op::Ins, rem_b.len())); ops.extend(std::iter::repeat_n(Op::Same, suf)); ops } diff --git a/crates/engine/src/cmd/tools/tests.rs b/crates/engine/src/cmd/tools/tests.rs index 74f10197ae..0dcc04e0fc 100644 --- a/crates/engine/src/cmd/tools/tests.rs +++ b/crates/engine/src/cmd/tools/tests.rs @@ -66,6 +66,309 @@ fn collapse_noise_matches_merges_short_interior_runs_but_keeps_prefix_suffix() { assert_eq!(collapse_noise_matches(&ops), ops); } +/// Verifies that `lcs_ops` chunks sequence alignment across multiple `MAX_LCS` windows, +/// replaying operations to prove exact reconstruction and asserting specific edit indices. +#[test] +fn lcs_ops_chunks_across_multiple_windows() { + // Two sequences of 9,000 elements (spanning three 4,000-element MAX_LCS windows). + // Element at 50 moves to 8,500. Substitutions at 0 and 8,999 zero the outer prefix/suffix. + let total = 9000; + let mut a: Vec = (0..total).collect(); + let mut b = a.clone(); + a[0] = 99999; + b[0] = 88888; + a[total - 1] = 77777; + b[total - 1] = 66666; + let moved = a[50]; + b.remove(50); + b.insert(8500, moved); + + let mut budget = u64::MAX; + let ops = lcs_ops(&a, &b, &mut budget); + + // Replay ops against `a` and `b` to verify exact sequence reconstruction and order. + let mut ia = 0; + let mut ib = 0; + let mut dels_seen = Vec::new(); + let mut inss_seen = Vec::new(); + let mut reconstructed = Vec::new(); + + for op in &ops { + match op { + Op::Same => { + assert_eq!(a.get(ia), b.get(ib), "Same op must match elements at ia={ia}, ib={ib}"); + if let Some(&val) = a.get(ia) { + reconstructed.push(val); + } + ia += 1; + ib += 1; + } + Op::Del => { + if let Some(&val) = a.get(ia) { + dels_seen.push((ia, val)); + } + ia += 1; + } + Op::Ins => { + if let Some(&val) = b.get(ib) { + inss_seen.push((ib, val)); + reconstructed.push(val); + } + ib += 1; + } + } + } + + assert_eq!(ia, a.len()); + assert_eq!(ib, b.len()); + assert_eq!(reconstructed, b); + // Truth: 2 substitutions (indices 0 and 8999), 1 deletion at 50, 1 insertion at 8500. + assert_eq!(dels_seen, vec![(0, 99999), (50, 50), (8999, 77777)]); + assert_eq!(inss_seen, vec![(0, 88888), (8500, 50), (8999, 66666)]); +} + +/// Verifies that `lcs_ops` forces successive full 4,000-element windows without +/// intermediate prefix trimming collapsing them, asserting exact budget exhaustion. +#[test] +fn lcs_ops_forces_successive_full_windows_without_intermediate_prefix_trim() { + // Force successive full 4,000-element windows without intermediate prefix trimming + // collapsing the middle into fewer tables. + // Length: 9,000 elements across 3 windows (4,000 + 4,000 + 1,000). + // Boundary substitutions at indices 0, 4000, 8000, and 8999 guarantee rem_a[0] != rem_b[0] + // at the beginning of each window, forcing intermediate prefix trim to find p == 0 so + // all three full DP tables must execute. + let total = 9000; + let mut a: Vec = (0..total).collect(); + let mut b = a.clone(); + a[0] = 11111; + b[0] = 22222; + a[4000] = 33333; + b[4000] = 44444; + a[8000] = 55555; + b[8000] = 66666; + a[total - 1] = 77777; + b[total - 1] = 88888; + + // Budget for 3 full windows: + // Window 1: 4,000 * 4,000 = 16,000,000 cells + // Window 2: 4,000 * 4,000 = 16,000,000 cells + // Window 3: 1,000 * 1,000 = 1,000,000 cells + // Total cells = 33,000,000. + let mut budget = 33_000_000u64; + let ops = lcs_ops(&a, &b, &mut budget); + + // Exact budget is fully exhausted (budget == 0), proving all 3 full DP tables were built. + assert_eq!(budget, 0, "all 3 successive windows must execute their DP tables without collapsing"); + + let mut ia = 0; + let mut ib = 0; + let mut reconstructed = Vec::new(); + let mut dels_seen = Vec::new(); + let mut inss_seen = Vec::new(); + + for op in &ops { + match op { + Op::Same => { + assert_eq!(a.get(ia), b.get(ib)); + if let Some(&val) = a.get(ia) { + reconstructed.push(val); + } + ia += 1; + ib += 1; + } + Op::Del => { + if let Some(&val) = a.get(ia) { + dels_seen.push((ia, val)); + } + ia += 1; + } + Op::Ins => { + if let Some(&val) = b.get(ib) { + inss_seen.push((ib, val)); + reconstructed.push(val); + } + ib += 1; + } + } + } + + assert_eq!(ia, a.len()); + assert_eq!(ib, b.len()); + assert_eq!(reconstructed, b); + assert_eq!(dels_seen, vec![(0, 11111), (4000, 33333), (8000, 55555), (8999, 77777)]); + assert_eq!(inss_seen, vec![(0, 22222), (4000, 44444), (8000, 66666), (8999, 88888)]); + + // Also verify completely disjoint sequences across multiple windows with zero common items. + // Window 1: 4,000 * 4,000 = 16,000,000 cells (deletes 4,000 of a_diff) + // Window 2: 4,000 * 4,000 = 16,000,000 cells (deletes next 4,000 of a_diff) + // Window 3: 1,000 of a_diff * 4,000 of b_diff = 4,000,000 cells + // Total cells = 36,000,000. + let a_diff: Vec = (0..9000).collect(); + let b_diff: Vec = (10_000..19_000).collect(); + let mut budget_diff = 36_000_000u64; + let ops_diff = lcs_ops(&a_diff, &b_diff, &mut budget_diff); + assert_eq!(budget_diff, 0, "all 3 windows in disjoint comparison must exhaust exact 36,000,000 budget"); + assert_eq!(ops_diff.iter().filter(|o| **o == Op::Del).count(), 9000); + assert_eq!(ops_diff.iter().filter(|o| **o == Op::Ins).count(), 9000); + assert_eq!(ops_diff.iter().filter(|o| **o == Op::Same).count(), 0); +} + +/// Tests `lcs_ops` budget behavior across edge cases: zero initial budget, exact-fit allocation, +/// one-short exhaustion, and multi-window exhaustion after the first chunk. +#[test] +fn lcs_ops_exhausted_budget_falls_back_whole() { + // 1. Budget 0 before any table: middle degrades whole, budget remains 0 + let a = vec![1, 2, 3, 4, 5]; + let b = vec![1, 8, 9, 5]; + let mut budget = 0; + let ops = lcs_ops(&a, &b, &mut budget); + assert_eq!(ops, vec![Op::Same, Op::Del, Op::Del, Op::Del, Op::Ins, Op::Ins, Op::Same]); + assert_eq!(budget, 0, "budget must remain 0"); + + // 2. Exact-fit: cells == budget + // Middle has 3 elements in a (2, 3, 4) and 2 in b (8, 9) -> 3 * 2 = 6 cells. + let mut budget_exact = 6; + let ops_exact = lcs_ops(&a, &b, &mut budget_exact); + assert_eq!(budget_exact, 0, "budget exactly exhausted to 0"); + assert_eq!(ops_exact, vec![Op::Same, Op::Del, Op::Del, Op::Del, Op::Ins, Op::Ins, Op::Same]); + + // Exact-fit with shared elements in middle + let a_mid = vec![1, 2, 3, 5]; + let b_mid = vec![1, 2, 9, 5]; + // Middle is (3) vs (9): 1 * 1 = 1 cell + let mut budget_mid = 1; + let ops_mid = lcs_ops(&a_mid, &b_mid, &mut budget_mid); + assert_eq!(budget_mid, 0); + assert_eq!(ops_mid, vec![Op::Same, Op::Same, Op::Del, Op::Ins, Op::Same]); + + // 3. One-short: budget == cells - 1 + let mut budget_short = 5; + let ops_short = lcs_ops(&a, &b, &mut budget_short); + assert_eq!(budget_short, 5, "one-short budget must not be charged"); + assert_eq!(ops_short, vec![Op::Same, Op::Del, Op::Del, Op::Del, Op::Ins, Op::Ins, Op::Same]); + + // 4. Multi-window: Exhaustion after the first window + // Two windows: window 1 has 4,000 elements (16,000,000 cells), window 2 has 500 elements (250,000 cells). + let mut a_multi: Vec = (0..4500).collect(); + let mut b_multi = a_multi.clone(); + a_multi[0] = 99999; + b_multi[0] = 88888; + a_multi[4000] = 77777; + b_multi[4000] = 66666; + a_multi[4499] = 55555; + b_multi[4499] = 44444; + + // Case 4A: budget fits window 1 exactly (16,000,000), exhausts for window 2 + let mut budget_w1_only = 16_000_000u64; + let ops_w1_only = lcs_ops(&a_multi, &b_multi, &mut budget_w1_only); + assert_eq!(budget_w1_only, 0, "budget spent on window 1, 0 remaining for window 2"); + // Window 1 aligned via DP table, window 2 fell back to 500 Dels and 500 Inss + let tail_dels = ops_w1_only.iter().rev().skip(500).take(500).filter(|o| **o == Op::Del).count(); + let tail_inss = ops_w1_only.iter().rev().take(500).filter(|o| **o == Op::Ins).count(); + assert_eq!(tail_dels, 500); + assert_eq!(tail_inss, 500); + + // Case 4B: budget fits both windows exactly: 16,250,000 + let mut budget_both = 16_250_000u64; + let ops_both = lcs_ops(&a_multi, &b_multi, &mut budget_both); + assert_eq!(budget_both, 0, "both windows charged exactly to 0"); + assert_eq!(ops_both.iter().filter(|o| **o == Op::Del).count(), 3); + assert_eq!(ops_both.iter().filter(|o| **o == Op::Ins).count(), 3); + + // Case 4C: one short on window 2: 16_249_999 + let mut budget_both_short = 16_249_999u64; + let _ = lcs_ops(&a_multi, &b_multi, &mut budget_both_short); + assert_eq!(budget_both_short, 249_999, "remaining budget must reflect window 1 spent and window 2 unspent"); +} + +/// Tests boundary conditions for `lcs_ops`: empty slices, one-sided empty, exact `MAX_LCS`, +/// `MAX_LCS + 1`, `2 * MAX_LCS`, and prefix-trim loop break paths. +#[test] +fn lcs_ops_boundary_cases_and_break_paths() { + // 1. Empty sequences + let mut b = u64::MAX; + assert_eq!(lcs_ops::(&[], &[], &mut b), Vec::::new()); + + // 2. One-sided empty + let items = [1, 2, 3]; + assert_eq!(lcs_ops(&[], &items, &mut b), vec![Op::Ins, Op::Ins, Op::Ins]); + assert_eq!(lcs_ops(&items, &[], &mut b), vec![Op::Del, Op::Del, Op::Del]); + + // 3. Exact MAX_LCS (4,000) + let len_max = 4000; + let mut a_max: Vec = (0..len_max).collect(); + let mut b_max = a_max.clone(); + a_max[0] = 99999; + b_max[0] = 88888; + a_max[len_max - 1] = 77777; + b_max[len_max - 1] = 66666; + let mut b_budget = u64::MAX; + let ops_max = lcs_ops(&a_max, &b_max, &mut b_budget); + assert_eq!(ops_max.len(), len_max + 2); + assert_eq!(ops_max.iter().filter(|o| **o == Op::Same).count(), len_max - 2); + + // 4. Exact MAX_LCS + 1 (4,001) + let len_plus1 = 4001; + let mut a_p1: Vec = (0..len_plus1).collect(); + let mut b_p1 = a_p1.clone(); + a_p1[0] = 99999; + b_p1[0] = 88888; + a_p1[len_plus1 - 1] = 77777; + b_p1[len_plus1 - 1] = 66666; + let mut b_budget2 = u64::MAX; + let ops_p1 = lcs_ops(&a_p1, &b_p1, &mut b_budget2); + assert_eq!(ops_p1.len(), len_plus1 + 2); + assert_eq!(ops_p1.iter().filter(|o| **o == Op::Same).count(), len_plus1 - 2); + + // 5. Exact 2 * MAX_LCS (8,000) + let len_2m = 8000; + let mut a_2m: Vec = (0..len_2m).collect(); + let mut b_2m = a_2m.clone(); + a_2m[0] = 11111; + b_2m[0] = 22222; + a_2m[4000] = 33333; + b_2m[4000] = 44444; + a_2m[len_2m - 1] = 55555; + b_2m[len_2m - 1] = 66666; + let mut b_budget3 = u64::MAX; + let ops_2m = lcs_ops(&a_2m, &b_2m, &mut b_budget3); + assert_eq!(ops_2m.iter().filter(|o| **o == Op::Del).count(), 3); + assert_eq!(ops_2m.iter().filter(|o| **o == Op::Ins).count(), 3); + assert_eq!(ops_2m.iter().filter(|o| **o == Op::Same).count(), len_2m - 3); + + // 6. Prefix-trim break paths inside the while loop: + // Case 6A: rem_a becomes empty after matching p elements + let mut a_break_a: Vec = (0..4005).collect(); + let mut b_break_a: Vec = (0..4008).collect(); + a_break_a[0] = 99999; + b_break_a[0] = 88888; + let mut b_break1 = u64::MAX; + let ops_break_a = lcs_ops(&a_break_a, &b_break_a, &mut b_break1); + assert_eq!(ops_break_a.iter().filter(|o| **o == Op::Del).count(), 1); + assert_eq!(ops_break_a.iter().filter(|o| **o == Op::Ins).count(), 4); + + // Case 6B: rem_b becomes empty after matching p elements + let mut a_break_b: Vec = (0..4008).collect(); + let mut b_break_b: Vec = (0..4005).collect(); + a_break_b[0] = 99999; + b_break_b[0] = 88888; + let mut b_break2 = u64::MAX; + let ops_break_b = lcs_ops(&a_break_b, &b_break_b, &mut b_break2); + assert_eq!(ops_break_b.iter().filter(|o| **o == Op::Del).count(), 4); + assert_eq!(ops_break_b.iter().filter(|o| **o == Op::Ins).count(), 1); + + // Case 6C: both become empty simultaneously after matching p elements + let mut a_break_both: Vec = (0..4005).collect(); + let mut b_break_both: Vec = (0..4005).collect(); + a_break_both[0] = 99999; + b_break_both[0] = 88888; + let mut b_break3 = u64::MAX; + let ops_break_both = lcs_ops(&a_break_both, &b_break_both, &mut b_break3); + assert_eq!(ops_break_both.iter().filter(|o| **o == Op::Del).count(), 1); + assert_eq!(ops_break_both.iter().filter(|o| **o == Op::Ins).count(), 1); + assert_eq!(ops_break_both.iter().filter(|o| **o == Op::Same).count(), 4004); +} + #[test] fn compare_at_word_level_still_matches_a_whole_unchanged_word() { // `collapse_noise_matches` only runs for character-level compares (wired up in @@ -332,6 +635,124 @@ fn compare_bounds_total_diff_work() { assert_eq!(s.doc.plain_text(StoryRef::Body), revised); } +/// Verifies that moving a paragraph past `MAX_LCS` in a long document does not cascade into false +/// revisions in the tail, asserting exact changed block indices, text, and edit types. +#[test] +fn compare_lcs_window_move_past_cap_does_not_cascade_false_revisions() { + // Issue #6110: 4,600 blocks where prefix/suffix trim is zeroed (substitutions at blocks 0 + // and 4599) and a record moves from index 100 to index 4400 (past MAX_LCS = 4000). + // Without chunking/continuation past the window, the tail misaligns and positional pairing + // marks ~202 paragraphs instead of the true 4 changes (substitutions at 0 and 4599, + // deletion at 100, insertion at 4400). + let total = 4600; + let mut orig_paras: Vec = (0..total).map(|i| format!("Record {i} has alpha beta gamma payload.")).collect(); + let mut rev_paras = orig_paras.clone(); + // Zero out prefix trim: substitution at 0 + orig_paras[0] = "Record 0 has alpha beta gamma original.".into(); + rev_paras[0] = "Record 0 has alpha beta gamma revised.".into(); + // Zero out suffix trim: substitution at 4599 + orig_paras[total - 1] = format!("Record {} has alpha beta gamma original.", total - 1); + rev_paras[total - 1] = format!("Record {} has alpha beta gamma revised.", total - 1); + // Move record 100 to 4400 + let moved = orig_paras[100].clone(); + rev_paras.remove(100); + rev_paras.insert(4400, moved); + + let original = orig_paras.join("\n"); + let revised = rev_paras.join("\n"); + + let mut s = Session::new(Document::from_text(&original)); + let r = s.run("review.compare", &json!({"text": revised})).unwrap(); + + // The true number of changed paragraphs is 4 (the two substitutions, the move's deletion, + // and the move's insertion). On unpatched code, window truncation misaligned the tail + // and marked ~202 paragraphs. + assert_eq!(r["changes"], 4, "move past MAX_LCS cap must not cascade into false revisions in the tail"); + + // In s.doc.body (total 4601 blocks: 4598 unchanged, 1 del at 100, 1 ins at 4401, 2 substituted at 0 and 4600): + let changed_blocks: Vec<(usize, String)> = s + .doc + .body + .iter() + .enumerate() + .filter_map(|(idx, b)| { + let p = b.as_para()?; + let has_change = p.mark.del.is_some() || p.mark.ins.is_some() || p.runs.iter().any(|r| r.props.del.is_some() || r.props.ins.is_some()); + if has_change { Some((idx, p.plain_text())) } else { None } + }) + .collect(); + + assert_eq!(changed_blocks.len(), 4); + assert_eq!(changed_blocks[0], (0, "Record 0 has alpha beta gamma original.revised.".into())); + assert_eq!(changed_blocks[1], (100, "Record 100 has alpha beta gamma payload.".into())); + assert_eq!(changed_blocks[2], (4401, "Record 100 has alpha beta gamma payload.".into())); + assert_eq!(changed_blocks[3], (4600, "Record 4599 has alpha beta gamma original.revised.".into())); + + // Verify exact change types on those blocks: + let b0 = s.doc.body[0].as_para().unwrap(); + assert!(b0.runs.iter().any(|r| r.props.del.is_some()) && b0.runs.iter().any(|r| r.props.ins.is_some()), "block 0 is text replacement"); + let b100 = s.doc.body[100].as_para().unwrap(); + assert!(b100.runs.iter().all(|r| r.props.del.is_some()) && b100.mark.del.is_some(), "block 100 is deleted whole"); + let b4401 = s.doc.body[4401].as_para().unwrap(); + assert!(b4401.runs.iter().all(|r| r.props.ins.is_some()) && b4401.mark.ins.is_some(), "block 4401 is inserted whole"); + let b4600 = s.doc.body[4600].as_para().unwrap(); + assert!(b4600.runs.iter().any(|r| r.props.del.is_some()) && b4600.runs.iter().any(|r| r.props.ins.is_some()), "block 4600 is text replacement"); + + s.run("review.acceptAll", &json!({})).unwrap(); + assert_eq!(s.doc.plain_text(StoryRef::Body), revised); +} + +/// Verifies that `mark_block` records paragraph mark revisions on whole-block deletions, +/// whole-block insertions, empty paragraphs, and ensures clean resolution via `review.acceptAll`. +#[test] +fn compare_mark_block_records_paragraph_mark_revisions_on_whole_block_changes() { + // Whole block delete/insert: test with differing paragraph count so whole-block paths trigger + let orig_multi = "Keep start.\nDelete me.\nKeep end."; + let rev_multi = "Keep start.\nKeep end."; + let mut s2 = Session::new(Document::from_text(orig_multi)); + if let Some(Block::Para(p)) = s2.doc.body.get_mut(1).map(Arc::make_mut) { + p.mark.bold = Some(true); + } + s2.run("review.compare", &json!({"text": rev_multi})).unwrap(); + + // In s2.doc.body, block 1 was deleted whole. + // Verify that p.mark.del is recorded by mark_block and formatting is preserved. + let del_para = s2.doc.body[1].as_para().expect("block 1 must be paragraph"); + assert!(del_para.mark.del.is_some(), "whole-block delete must record revision on paragraph mark"); + assert_eq!(del_para.mark.bold, Some(true), "paragraph mark formatting must be preserved on deleted block"); + + // Accept all changes: deleted paragraph is removed + s2.run("review.acceptAll", &json!({})).unwrap(); + assert_eq!(s2.doc.plain_text(StoryRef::Body), rev_multi); + + // Test whole-block insertion + let mut s3 = Session::new(Document::from_text(rev_multi)); + s3.run("review.compare", &json!({"text": orig_multi})).unwrap(); + let ins_para = s3.doc.body[1].as_para().expect("block 1 must be paragraph"); + assert!(ins_para.mark.ins.is_some(), "whole-block insert must record revision on paragraph mark"); + s3.run("review.acceptAll", &json!({})).unwrap(); + assert_eq!(s3.doc.plain_text(StoryRef::Body), orig_multi); + + // Test empty paragraph deletion and insertion (len == 0, where only mark carries the revision) + let orig_empty = "Before.\n\nAfter."; + let rev_empty = "Before.\nAfter."; + let mut s4 = Session::new(Document::from_text(orig_empty)); + s4.run("review.compare", &json!({"text": rev_empty})).unwrap(); + let empty_del = s4.doc.body[1].as_para().expect("empty paragraph block"); + assert_eq!(empty_del.len(), 0); + assert!(empty_del.mark.del.is_some(), "empty paragraph deletion must set mark.del"); + s4.run("review.acceptAll", &json!({})).unwrap(); + assert_eq!(s4.doc.plain_text(StoryRef::Body), rev_empty); + + let mut s5 = Session::new(Document::from_text(rev_empty)); + s5.run("review.compare", &json!({"text": orig_empty})).unwrap(); + let empty_ins = s5.doc.body[1].as_para().expect("empty paragraph block"); + assert_eq!(empty_ins.len(), 0); + assert!(empty_ins.mark.ins.is_some(), "empty paragraph insertion must set mark.ins"); + s5.run("review.acceptAll", &json!({})).unwrap(); + assert_eq!(s5.doc.plain_text(StoryRef::Body), orig_empty); +} + #[test] fn macros_record_and_run() { let mut s = Session::new(Document::new());