diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index 760c0be7..b8f09408 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -175,32 +175,50 @@ fn early_exit_result( } } +/// `(saved_reverse_dep_edges, saved_sibling_groups, removal_reverse_deps, +/// removed_file_neighbors)` — see `save_and_purge_changed`. +type SaveAndPurgeResult = ( + Vec, + HashMap>, + Vec, + Vec, +); + /// Save reverse-dep edges (and reverse-deps of removed files) before purging /// changed files. Mirrors the JS save-then-purge sequence in `build-edges.ts` -/// (#1012). Returns `(saved_reverse_dep_edges, removal_reverse_deps, -/// removed_file_neighbors)` so the pipeline can reconnect edges after Stage 5, -/// reclassify roles in Stage 8, and (#1839) fold a removed file's -/// cross-directory neighbors into Stage 8's directory-metrics refresh. +/// (#1012). Returns `(saved_reverse_dep_edges, saved_sibling_groups, +/// removal_reverse_deps, removed_file_neighbors)` so the pipeline can +/// reconnect edges after Stage 5, reclassify roles in Stage 8, and (#1839) +/// fold a removed file's cross-directory neighbors into Stage 8's +/// directory-metrics refresh. fn save_and_purge_changed( conn: &Connection, parse_changes: &[&detect_changes::ChangedFile], change_result: &detect_changes::ChangeResult, opts: &BuildOpts, root_dir: &str, -) -> (Vec, Vec, Vec) { +) -> SaveAndPurgeResult { let mut saved_reverse_dep_edges: Vec = Vec::new(); + let mut saved_sibling_groups: HashMap> = + HashMap::new(); let mut removal_reverse_deps: Vec = Vec::new(); if change_result.is_full_build { let has_embeddings = detect_changes::has_embeddings(conn); detect_changes::clear_all_graph_data(conn, has_embeddings); - return (saved_reverse_dep_edges, removal_reverse_deps, Vec::new()); + return ( + saved_reverse_dep_edges, + saved_sibling_groups, + removal_reverse_deps, + Vec::new(), + ); } let changed_paths: Vec = parse_changes.iter().map(|c| c.rel_path.clone()).collect(); if !opts.no_reverse_deps.unwrap_or(false) { - saved_reverse_dep_edges = detect_changes::save_reverse_dep_edges(conn, &changed_paths); + (saved_reverse_dep_edges, saved_sibling_groups) = + detect_changes::save_reverse_dep_edges(conn, &changed_paths); if !change_result.removed.is_empty() { let removed_set: HashSet = change_result.removed.iter().cloned().collect(); @@ -225,7 +243,12 @@ fn save_and_purge_changed( .collect(); detect_changes::purge_changed_files(conn, &files_to_purge, &[]); - (saved_reverse_dep_edges, removal_reverse_deps, removed_file_neighbors) + ( + saved_reverse_dep_edges, + saved_sibling_groups, + removal_reverse_deps, + removed_file_neighbors, + ) } /// Parse a changed-file slice in parallel and key the results by relative path. @@ -286,11 +309,18 @@ fn resolve_pipeline_imports( fn reconnect_saved_reverse_dep_edges( conn: &Connection, saved: &[detect_changes::SavedReverseDepEdge], + saved_sibling_groups: &HashMap>, + max_align_group_size: usize, ) { if saved.is_empty() { return; } - let (reconnected, dropped) = detect_changes::reconnect_reverse_dep_edges(conn, saved); + let (reconnected, dropped) = detect_changes::reconnect_reverse_dep_edges( + conn, + saved, + saved_sibling_groups, + max_align_group_size, + ); if dropped > 0 { eprintln!( "[codegraph] reconnect_reverse_dep_edges: {reconnected} reconnected, {dropped} dropped (target nodes not found)" @@ -528,7 +558,7 @@ pub fn run_pipeline( // Stage 3b: save reverse-dep edges (incremental) or clear all (full), // then purge changed files. Returns the saved edges for Stage 7 // reconnect and the removal reverse-dep set for Stage 8 reclassification. - let (saved_reverse_dep_edges, removal_reverse_deps, removed_file_neighbors) = + let (saved_reverse_dep_edges, saved_sibling_groups, removal_reverse_deps, removed_file_neighbors) = save_and_purge_changed(conn, &parse_changes, &change_result, &opts, root_dir); // ── Stage 4: Parse files ─────────────────────────────────────────── @@ -639,7 +669,12 @@ pub fn run_pipeline( ) .map_err(|e| format!("call edge insertion failed: {e}"))?; - reconnect_saved_reverse_dep_edges(conn, &saved_reverse_dep_edges); + reconnect_saved_reverse_dep_edges( + conn, + &saved_reverse_dep_edges, + &saved_sibling_groups, + config.build.reverse_dep_alignment_max_group_size, + ); // Now that edges reflect this revision, commit file_hashes for the // changed files (#1731). Deferred from Stage 5 — see the comment there. diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs b/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs index 98400973..324a39c1 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs @@ -369,32 +369,31 @@ pub struct SavedReverseDepEdge { pub edge_kind: String, pub confidence: f64, pub dynamic: i64, - /// 1-based rank of the target (by ascending line) among nodes sharing its - /// (name, kind) within `tgt_file`, computed at save time — see #1752. - pub tgt_ordinal: i64, - /// Size of that (name, kind) sibling group at save time. - pub tgt_sibling_count: i64, } -/// Computes each node's 1-based ordinal rank (by ascending line) among nodes -/// sharing its (name, kind) within `file`, plus the sibling-group size, -/// keyed by `(name, kind, line)`. +/// Key identifying a same-(name, kind) sibling group within one file. +pub type SiblingGroupKey = (String, String, String); + +/// Computes the sorted line list for every (name, kind) sibling group within +/// `file`, keyed by `(name, kind)`. /// /// A file can contain multiple distinct symbols with the identical name and /// kind — e.g. several object-literal `close() {}` methods returned from /// different functions in the same file. `(name, kind, file)` alone is not a /// unique identity for such symbols, so `reconnect_reverse_dep_edges` cannot /// safely tell them apart by nearest-line matching once unrelated code -/// shifts the candidates unevenly (#1752). The ordinal recorded here — the -/// target's rank among same-named siblings at save time — lets reconnection -/// map an old target to its new node correctly as long as the sibling count -/// is unchanged, regardless of how far the whole group has shifted. -fn compute_ordinals(conn: &Connection, file: &str) -> HashMap<(String, String, i64), (i64, i64)> { - let mut by_group: HashMap<(String, String), Vec> = HashMap::new(); - let mut result = HashMap::new(); +/// shifts the candidates unevenly, or a same-named sibling is added/removed +/// in the same edit (#1752, #1865). The sorted line list captured here — the +/// sibling group's layout at save time — lets reconnection align old targets +/// to their correct new nodes by rank when the sibling count is unchanged, +/// or by the dominant line-shift that best explains the surviving siblings +/// when it changed (see `align_sibling_lines`), which tolerates both a +/// uniform shift of the whole group AND a change in the group's size. +fn compute_sibling_groups(conn: &Connection, file: &str) -> HashMap<(String, String), Vec> { + let mut groups: HashMap<(String, String), Vec> = HashMap::new(); let mut stmt = match conn.prepare("SELECT name, kind, line FROM nodes WHERE file = ?1") { Ok(s) => s, - Err(_) => return result, + Err(_) => return groups, }; let rows = match stmt.query_map([file], |row| { Ok(( @@ -404,19 +403,15 @@ fn compute_ordinals(conn: &Connection, file: &str) -> HashMap<(String, String, i )) }) { Ok(r) => r, - Err(_) => return result, + Err(_) => return groups, }; for row in rows.flatten() { - by_group.entry((row.0, row.1)).or_default().push(row.2); + groups.entry((row.0, row.1)).or_default().push(row.2); } - for ((name, kind), mut lines) in by_group { + for lines in groups.values_mut() { lines.sort_unstable(); - let sibling_count = lines.len() as i64; - for (idx, line) in lines.iter().enumerate() { - result.insert((name.clone(), kind.clone(), *line), (idx as i64 + 1, sibling_count)); - } } - result + groups } /// Save edges from reverse-dep files → changed files BEFORE purge so they @@ -431,10 +426,11 @@ fn compute_ordinals(conn: &Connection, file: &str) -> HashMap<(String, String, i pub fn save_reverse_dep_edges( conn: &Connection, changed_paths: &[String], -) -> Vec { +) -> (Vec, HashMap>) { let mut saved = Vec::new(); + let mut sibling_groups: HashMap> = HashMap::new(); if changed_paths.is_empty() { - return saved; + return (saved, sibling_groups); } let changed_set: HashSet<&str> = changed_paths.iter().map(|s| s.as_str()).collect(); @@ -447,14 +443,14 @@ pub fn save_reverse_dep_edges( WHERE n_tgt.file = ?1 AND n_src.file != n_tgt.file", ) { Ok(s) => s, - Err(_) => return saved, + Err(_) => return (saved, sibling_groups), }; for changed in changed_paths { // Must be computed BEFORE this file's nodes are purged — captures the // pre-purge sibling layout so reconnection can map old→new correctly // even when several same-named/same-kind symbols exist in the file. - let ordinals = compute_ordinals(conn, changed); + let groups = compute_sibling_groups(conn, changed); let rows = match stmt.query_map([changed], |row| { Ok(( row.get::<_, i64>(0)?, @@ -477,10 +473,13 @@ pub fn save_reverse_dep_edges( if changed_set.contains(row.8.as_str()) { continue; } - let (tgt_ordinal, tgt_sibling_count) = ordinals - .get(&(row.1.clone(), row.2.clone(), row.4)) - .copied() - .unwrap_or((1, 1)); + let group_key: SiblingGroupKey = (row.1.clone(), row.2.clone(), row.3.clone()); + sibling_groups.entry(group_key).or_insert_with(|| { + groups + .get(&(row.1.clone(), row.2.clone())) + .cloned() + .unwrap_or_else(|| vec![row.4]) + }); saved.push(SavedReverseDepEdge { source_id: row.0, tgt_name: row.1, @@ -490,30 +489,103 @@ pub fn save_reverse_dep_edges( edge_kind: row.5, confidence: row.6, dynamic: row.7, - tgt_ordinal, - tgt_sibling_count, }); } } - saved + (saved, sibling_groups) } -/// Picks the correct reconnect target among same-(name,kind,file) candidates -/// (sorted by ascending line). +/// Aligns two ascending line arrays representing the same-(name, kind) +/// sibling group before (`old_lines`) and after (`new_lines`) a +/// purge+reinsert. Mirrors `alignSiblingLines` in `build-edges.ts`. /// -/// When only one candidate exists, it's an unambiguous match. When several -/// exist (e.g. multiple object-literal `close() {}` methods in one file) and -/// the sibling-group size is unchanged since save, the saved ordinal — the -/// target's rank by line among its siblings at save time — reliably -/// identifies the original target even though the whole group may have -/// shifted by an arbitrary number of lines. Falls back to nearest-line only -/// when the sibling count itself changed (a same-named sibling was added or -/// removed), since the ordinal mapping can no longer be trusted — see #1752. +/// When the sibling count is unchanged, declarations of the same name and +/// kind within one file keep their relative textual order across an edit — +/// even when the whole group shifts by an arbitrary, non-uniform amount per +/// sibling (e.g. one sibling's own body grew independently) — so mapping by +/// rank (1st old -> 1st new, 2nd -> 2nd, ...) is always correct (#1752). +/// +/// When the count changed (a same-named sibling was added or removed in the +/// same edit), rank order alone can't tell which element is the new/missing +/// one. But the untouched siblings' OWN source text wasn't edited, so they +/// all shift by the exact SAME line delta — whatever unrelated insertion or +/// deletion elsewhere in the file caused the shift applies uniformly to +/// everything below/above it. This finds the single shift value `S` that +/// makes `old + S` land on a real new line for the most siblings — the +/// dominant shift — and matches every old line whose shifted position +/// exists in `new_lines`; the rest were removed. This is far more reliable +/// than picking whichever old/new pairing merely minimizes total line +/// distance, which a uniform shift can fool once an old line coincidentally +/// ends up numerically closer to a different sibling's new position than to +/// its own (confirmed against this repo's real #1752 fixture numbers) — +/// see #1865. +/// +/// Returns a map from each old line to its aligned new line. An old line +/// missing from the result means its sibling was removed (no new line +/// matches it) — callers must drop, not guess, in that case. +fn align_sibling_lines(old_lines: &[i64], new_lines: &[i64]) -> HashMap { + let mut result = HashMap::new(); + if old_lines.is_empty() || new_lines.is_empty() { + return result; + } + + if old_lines.len() == new_lines.len() { + for (old_line, new_line) in old_lines.iter().zip(new_lines.iter()) { + result.insert(*old_line, *new_line); + } + return result; + } + + let new_line_set: HashSet = new_lines.iter().copied().collect(); + let mut shift_counts: HashMap = HashMap::new(); + for &old_line in old_lines { + for &new_line in new_lines { + *shift_counts.entry(new_line - old_line).or_insert(0) += 1; + } + } + // Tie-break fully by value (not HashMap iteration order, which Rust + // leaves unspecified): prefer the higher match count, then the smaller + // magnitude shift, then the smaller signed shift. Mirrors the JS + // implementation exactly so both engines pick the same shift on a tie. + let mut best_shift = 0i64; + let mut best_count = -1i64; + for (&shift, &count) in &shift_counts { + let better = count > best_count + || (count == best_count + && (shift.abs() < best_shift.abs() + || (shift.abs() == best_shift.abs() && shift < best_shift))); + if better { + best_shift = shift; + best_count = count; + } + } + for &old_line in old_lines { + let candidate = old_line + best_shift; + if new_line_set.contains(&candidate) { + result.insert(old_line, candidate); + } + } + result +} + +/// Picks the correct reconnect target among same-(name, kind, file) +/// candidates. +/// +/// A single candidate is an unambiguous match. With several candidates (e.g. +/// multiple object-literal `close() {}` methods in one file), the saved +/// sibling-group snapshot from before purge is aligned against the current +/// candidate lines (see `align_sibling_lines`) to find which new line the +/// saved target's old line maps to — correct even when the whole group +/// shifted and its size changed in the same edit. Falls back to +/// nearest-line only when no sibling-group snapshot is available, or the +/// group is too large to align cheaply. fn pick_reconnect_target( candidates: &[(i64, i64)], - tgt_ordinal: i64, - tgt_sibling_count: i64, tgt_line: i64, + group_key: &SiblingGroupKey, + saved_sibling_groups: &HashMap>, + alignment_cache: &mut HashMap>, + max_align_group_size: usize, ) -> Option { if candidates.is_empty() { return None; @@ -521,12 +593,37 @@ fn pick_reconnect_target( if candidates.len() == 1 { return Some(candidates[0].0); } - if candidates.len() as i64 == tgt_sibling_count - && tgt_ordinal >= 1 - && (tgt_ordinal as usize) <= candidates.len() - { - return Some(candidates[(tgt_ordinal - 1) as usize].0); + + if let Some(old_lines) = saved_sibling_groups.get(group_key) { + if !old_lines.is_empty() + && old_lines.len() <= max_align_group_size + && candidates.len() <= max_align_group_size + { + let alignment = match alignment_cache.entry(group_key.clone()) { + std::collections::hash_map::Entry::Occupied(e) => e.into_mut(), + std::collections::hash_map::Entry::Vacant(e) => { + let new_lines: Vec = candidates.iter().map(|(_, line)| *line).collect(); + e.insert(align_sibling_lines(old_lines, &new_lines)) + } + }; + return match alignment.get(&tgt_line) { + Some(new_line) => candidates + .iter() + .find(|(_, line)| line == new_line) + .map(|(id, _)| *id), + // tgt_line's sibling was legitimately removed in this edit — + // the alignment already accounted for the size change, so + // falling through to nearest-line here would silently + // reattach to an unrelated sibling instead of correctly + // dropping this edge. + None => None, + }; + } } + + // No sibling-group snapshot (shouldn't normally happen — every saved + // edge has one recorded at save time) or the group exceeds the + // alignment size cap — fall back to nearest-line. candidates .iter() .min_by_key(|(_, line)| (line - tgt_line).abs()) @@ -537,14 +634,17 @@ fn pick_reconnect_target( /// /// The source node ID is still valid (reverse-dep nodes were never purged). /// The target was deleted and re-inserted with a new ID — look up all -/// (name, kind, file) candidates and pick the one matching the saved ordinal -/// (see `pick_reconnect_target`), then recreate the edge. Mirrors -/// `reconnectReverseDepEdges` in `build-edges.ts`. +/// (name, kind, file) candidates and pick the one the saved sibling-group +/// snapshot aligns to the saved line (see `pick_reconnect_target`), then +/// recreate the edge. Mirrors `reconnectReverseDepEdges` in +/// `build-edges.ts`. /// /// Returns (reconnected, dropped) counts. pub fn reconnect_reverse_dep_edges( conn: &Connection, saved: &[SavedReverseDepEdge], + saved_sibling_groups: &HashMap>, + max_align_group_size: usize, ) -> (usize, usize) { if saved.is_empty() { return (0, 0); @@ -574,12 +674,14 @@ pub fn reconnect_reverse_dep_edges( // Cache candidate lists per (name, kind, file) group — many saved // edges often share the same target (e.g. several callers of the // same function), so this avoids re-querying per edge. - let mut candidates_cache: HashMap<(String, String, String), Vec<(i64, i64)>> = - HashMap::new(); + let mut candidates_cache: HashMap> = HashMap::new(); + // Cache the (potentially expensive) alignment result per group too — + // shared across every saved edge targeting the same sibling group. + let mut alignment_cache: HashMap> = HashMap::new(); for s in saved { let key = (s.tgt_name.clone(), s.tgt_kind.clone(), s.tgt_file.clone()); - let candidates = match candidates_cache.entry(key) { + let candidates = match candidates_cache.entry(key.clone()) { std::collections::hash_map::Entry::Occupied(e) => e.into_mut(), std::collections::hash_map::Entry::Vacant(e) => { let mut rows: Vec<(i64, i64)> = Vec::new(); @@ -600,9 +702,11 @@ pub fn reconnect_reverse_dep_edges( match pick_reconnect_target( candidates, - s.tgt_ordinal, - s.tgt_sibling_count, s.tgt_line, + &key, + saved_sibling_groups, + &mut alignment_cache, + max_align_group_size, ) { Some(new_id) => { // INSERT OR IGNORE silently swallows duplicate-row constraint @@ -994,48 +1098,95 @@ mod tests { assert_eq!(removed, vec!["src/deleted.ts"]); } - // ── Reverse-dep edge reconnection (#1752) ─────────────────────────── + // ── Reverse-dep edge reconnection (#1752, #1865) ──────────────────── + + /// Convenience wrapper mirroring the pre-#1865 `pick_reconnect_target` + /// call shape: builds the single-group map + a fresh alignment cache + /// internally so each test case reads as a plain (candidates, old_lines, + /// tgt_line) -> Option call. + fn pick( + candidates: &[(i64, i64)], + old_lines: &[i64], + tgt_line: i64, + ) -> Option { + let key: SiblingGroupKey = ("x".to_string(), "method".to_string(), "f.ts".to_string()); + let mut groups = HashMap::new(); + groups.insert(key.clone(), old_lines.to_vec()); + let mut cache = HashMap::new(); + pick_reconnect_target(candidates, tgt_line, &key, &groups, &mut cache, 200) + } #[test] fn pick_reconnect_target_single_candidate_is_unambiguous() { let candidates = vec![(42, 100)]; - assert_eq!(pick_reconnect_target(&candidates, 1, 1, 999), Some(42)); + assert_eq!(pick(&candidates, &[999], 999), Some(42)); } #[test] fn pick_reconnect_target_no_candidates_returns_none() { let candidates: Vec<(i64, i64)> = vec![]; - assert_eq!(pick_reconnect_target(&candidates, 1, 1, 100), None); + assert_eq!(pick(&candidates, &[100], 100), None); } #[test] - fn pick_reconnect_target_uses_ordinal_when_sibling_count_matches() { + fn pick_reconnect_target_aligns_unchanged_group_by_rank() { // Four same-named/same-kind siblings (e.g. four `close() {}` methods), // shifted down by an insertion elsewhere in the file. The 3rd-ranked - // sibling (originally closest to line 433 in the pre-shift layout) - // must still resolve to the 3rd-ranked sibling post-shift (id 30, - // line 580), NOT to whichever candidate is nearest to the stale old - // line 433 (which would wrongly pick id 10 / line 433... except that - // id no longer exists post-purge; the point is nearest-*new*-line - // to the OLD reference can pick the wrong post-shift sibling once the - // group shifts unevenly). - let candidates = vec![(10, 178), (20, 461), (30, 500), (40, 580)]; - // Saved ordinal=3 out of 4 siblings (matches count) → must pick the - // 3rd by line (id 30), regardless of how far tgt_line (the stale old - // reference, 433) now sits from any candidate. - let picked = pick_reconnect_target(&candidates, 3, 4, 433); + // sibling (old line 500) must still resolve to the 3rd-ranked sibling + // post-shift (id 30, line 580), NOT to whichever candidate is nearest + // to the stale old line 500 (which would wrongly pick a different id + // once the group shifts unevenly). + let old_lines = vec![433, 461, 500, 580]; + let candidates = vec![(10, 178), (20, 461), (30, 500 + 80), (40, 580 + 80)]; + let picked = pick(&candidates, &old_lines, 500); assert_eq!(picked, Some(30)); } #[test] - fn pick_reconnect_target_falls_back_to_nearest_line_when_sibling_count_changed() { - // A sibling was added/removed since save — the ordinal mapping can no - // longer be trusted, so fall back to nearest-line (best effort, same - // as pre-#1752 behavior). - let candidates = vec![(10, 100), (20, 200), (30, 300)]; - // Saved sibling_count=2 but now there are 3 candidates → mismatch. - let picked = pick_reconnect_target(&candidates, 2, 2, 195); - assert_eq!(picked, Some(20)); // nearest to 195 is line 200 + fn pick_reconnect_target_drops_edge_when_its_own_sibling_was_removed() { + // Sibling count changed (4 -> 3): old line 200's sibling was removed, + // the other three shifted down by 50. The edge targeting line 200 + // must be dropped (no matching new line) — not silently reattached + // to whichever candidate happens to be nearest. + let old_lines = vec![100, 200, 300, 400]; + let candidates = vec![(10, 150), (30, 350), (40, 450)]; // id 20 (line 200) removed + assert_eq!(pick(&candidates, &old_lines, 200), None); + // The untouched siblings must still resolve correctly despite the + // group's size having changed in the same edit (#1865). + assert_eq!(pick(&candidates, &old_lines, 100), Some(10)); + assert_eq!(pick(&candidates, &old_lines, 300), Some(30)); + assert_eq!(pick(&candidates, &old_lines, 400), Some(40)); + } + + #[test] + fn pick_reconnect_target_survives_added_sibling_plus_shift() { + // Sibling count changed (3 -> 4): a new sibling was inserted between + // the 1st and 2nd old siblings, and the whole group shifted down by + // 50. The two original siblings must still resolve to their own new + // lines, not to the newly inserted one. + let old_lines = vec![100, 300, 400]; + let candidates = vec![(10, 150), (99, 250), (30, 350), (40, 450)]; // id 99 is new + assert_eq!(pick(&candidates, &old_lines, 100), Some(10)); + assert_eq!(pick(&candidates, &old_lines, 300), Some(30)); + assert_eq!(pick(&candidates, &old_lines, 400), Some(40)); + } + + #[test] + fn align_sibling_lines_forces_exact_pairing_when_counts_match() { + let old_lines = vec![433, 461, 500, 580]; + let new_lines = vec![513, 541, 580, 660]; // uniform +80 shift + let alignment = align_sibling_lines(&old_lines, &new_lines); + assert_eq!(alignment.get(&433), Some(&513)); + assert_eq!(alignment.get(&461), Some(&541)); + assert_eq!(alignment.get(&500), Some(&580)); + assert_eq!(alignment.get(&580), Some(&660)); + } + + #[test] + fn align_sibling_lines_empty_inputs_yield_empty_map() { + assert!(align_sibling_lines(&[], &[1, 2, 3]).is_empty()); + assert!(align_sibling_lines(&[1, 2, 3], &[]).is_empty()); + assert!(align_sibling_lines(&[], &[]).is_empty()); } /// Minimal in-memory schema covering only the columns `save_reverse_dep_edges` @@ -1074,7 +1225,7 @@ mod tests { } #[test] - fn compute_ordinals_ranks_same_name_kind_siblings_by_line() { + fn compute_sibling_groups_ranks_same_name_kind_siblings_by_line() { let conn = test_conn(); insert_node(&conn, "close", "method", "src/db/connection.ts", 433); insert_node(&conn, "close", "method", "src/db/connection.ts", 461); @@ -1083,15 +1234,14 @@ mod tests { // An unrelated, uniquely-named sibling must not pollute the group. insert_node(&conn, "openDb", "function", "src/db/connection.ts", 161); - let ordinals = compute_ordinals(&conn, "src/db/connection.ts"); - let key = |line: i64| ("close".to_string(), "method".to_string(), line); - assert_eq!(ordinals.get(&key(433)), Some(&(1, 4))); - assert_eq!(ordinals.get(&key(461)), Some(&(2, 4))); - assert_eq!(ordinals.get(&key(500)), Some(&(3, 4))); - assert_eq!(ordinals.get(&key(580)), Some(&(4, 4))); + let groups = compute_sibling_groups(&conn, "src/db/connection.ts"); + assert_eq!( + groups.get(&("close".to_string(), "method".to_string())), + Some(&vec![433, 461, 500, 580]) + ); assert_eq!( - ordinals.get(&("openDb".to_string(), "function".to_string(), 161)), - Some(&(1, 1)) + groups.get(&("openDb".to_string(), "function".to_string())), + Some(&vec![161]) ); } @@ -1123,10 +1273,12 @@ mod tests { ) .unwrap(); - let saved = save_reverse_dep_edges(&conn, &[file.to_string()]); + let (saved, sibling_groups) = save_reverse_dep_edges(&conn, &[file.to_string()]); assert_eq!(saved.len(), 1); - assert_eq!(saved[0].tgt_ordinal, 3); - assert_eq!(saved[0].tgt_sibling_count, 4); + assert_eq!( + sibling_groups.get(&("close".to_string(), "method".to_string(), file.to_string())), + Some(&vec![433, 461, 500, 580]) + ); // Simulate purge_changed_files: delete the changed file's nodes/edges. conn.execute("DELETE FROM edges WHERE target_id IN (SELECT id FROM nodes WHERE file = ?1)", [file]).unwrap(); @@ -1140,7 +1292,8 @@ mod tests { let target_new_id = insert_node(&conn, "close", "method", file, 500 + 147); insert_node(&conn, "close", "method", file, 580 + 147); - let (reconnected, dropped) = reconnect_reverse_dep_edges(&conn, &saved); + let (reconnected, dropped) = + reconnect_reverse_dep_edges(&conn, &saved, &sibling_groups, 200); assert_eq!((reconnected, dropped), (1, 0)); let new_target: i64 = conn @@ -1153,6 +1306,77 @@ mod tests { assert_eq!(new_target, target_new_id); } + /// End-to-end reproduction of #1865: same shift as #1752's repro, but a + /// same-named/same-kind sibling is ALSO renamed away (removed from the + /// group) in the same edit — the exact compound scenario the pre-#1865 + /// ordinal+nearest-line fallback could not resolve correctly. The three + /// untouched siblings' reverse-dep edges must all reconnect to their own + /// correct new node despite the group shrinking from 4 to 3 in the same + /// build that also shifted it. + #[test] + fn reconnect_survives_shift_plus_sibling_removed_in_same_edit() { + let conn = test_conn(); + let file = "src/db/connection.ts"; + + // Pre-edit layout: four `close` siblings (A, B, C, D by old line). + let a_old = insert_node(&conn, "close", "method", file, 433); + let b_old = insert_node(&conn, "close", "method", file, 461); + let c_old = insert_node(&conn, "close", "method", file, 500); + let d_old = insert_node(&conn, "close", "method", file, 580); + + // One external caller per sibling, in untouched files. + let caller_a = insert_node(&conn, "useA", "function", "src/features/a.ts", 10); + let caller_b = insert_node(&conn, "useB", "function", "src/features/b.ts", 10); + let caller_c = insert_node(&conn, "useC", "function", "src/features/c.ts", 10); + let caller_d = insert_node(&conn, "useD", "function", "src/features/d.ts", 10); + for (caller, target) in [ + (caller_a, a_old), + (caller_b, b_old), + (caller_c, c_old), + (caller_d, d_old), + ] { + conn.execute( + "INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?1, ?2, 'calls', 0.9, 0)", + rusqlite::params![caller, target], + ) + .unwrap(); + } + + let (saved, sibling_groups) = save_reverse_dep_edges(&conn, &[file.to_string()]); + assert_eq!(saved.len(), 4); + + // Simulate purge_changed_files. + conn.execute("DELETE FROM edges WHERE target_id IN (SELECT id FROM nodes WHERE file = ?1)", [file]).unwrap(); + conn.execute("DELETE FROM nodes WHERE file = ?1", [file]).unwrap(); + + // Re-insert: whole group shifted +147 (unrelated helper inserted + // above), AND B's `close` was renamed to `shutdown` in the same + // edit — sibling count for (close, method) drops from 4 to 3. + let a_new = insert_node(&conn, "close", "method", file, 433 + 147); + insert_node(&conn, "shutdown", "method", file, 461 + 147); // was B's close + let c_new = insert_node(&conn, "close", "method", file, 500 + 147); + let d_new = insert_node(&conn, "close", "method", file, 580 + 147); + + let (reconnected, dropped) = + reconnect_reverse_dep_edges(&conn, &saved, &sibling_groups, 200); + // A, C, D reconnect correctly; B's edge is dropped (its `close` no + // longer exists — renamed to `shutdown`). + assert_eq!((reconnected, dropped), (3, 1)); + + let target_of = |caller: i64| -> Option { + conn.query_row( + "SELECT target_id FROM edges WHERE source_id = ?1", + [caller], + |row| row.get(0), + ) + .ok() + }; + assert_eq!(target_of(caller_a), Some(a_new)); + assert_eq!(target_of(caller_b), None); + assert_eq!(target_of(caller_c), Some(c_new)); + assert_eq!(target_of(caller_d), Some(d_new)); + } + // ── capture_removed_file_neighbors (#1839) ────────────────────────── #[test] diff --git a/crates/codegraph-core/src/infrastructure/config.rs b/crates/codegraph-core/src/infrastructure/config.rs index 4392e9c7..f2a37a3d 100644 --- a/crates/codegraph-core/src/infrastructure/config.rs +++ b/crates/codegraph-core/src/infrastructure/config.rs @@ -48,6 +48,16 @@ pub struct BuildSettings { /// Drift detection threshold for incremental builds. #[serde(default = "default_drift_threshold")] pub drift_threshold: f64, + + /// Max size of a same-(name, kind) sibling group + /// `reconnect_reverse_dep_edges` will run its line-alignment against + /// (#1865). Building the shift histogram is O(n*m) in the old/new group + /// sizes; groups above this size fall back to nearest-line matching to + /// bound worst-case incremental-build cost. Mirrors + /// `DEFAULTS.build.reverseDepAlignmentMaxGroupSize` in + /// `src/infrastructure/config.ts`. + #[serde(default = "default_reverse_dep_alignment_max_group_size")] + pub reverse_dep_alignment_max_group_size: usize, } // Manual impl so `BuildSettings::default()` matches the serde field defaults. @@ -58,6 +68,7 @@ impl Default for BuildSettings { Self { incremental: default_true(), drift_threshold: default_drift_threshold(), + reverse_dep_alignment_max_group_size: default_reverse_dep_alignment_max_group_size(), } } } @@ -70,6 +81,10 @@ fn default_drift_threshold() -> f64 { 0.1 } +fn default_reverse_dep_alignment_max_group_size() -> usize { + 200 +} + /// Subset of `CodegraphConfig.analysis` relevant to the build pipeline. #[derive(Debug, Clone, Deserialize)] #[serde(rename_all = "camelCase")] @@ -174,6 +189,15 @@ mod tests { assert!(config.build.incremental); // Default mirrors DEFAULTS.analysis.pointsToMaxIterations in config.ts. assert_eq!(config.analysis.points_to_max_iterations, 50); + // Default mirrors DEFAULTS.build.reverseDepAlignmentMaxGroupSize (#1865). + assert_eq!(config.build.reverse_dep_alignment_max_group_size, 200); + } + + #[test] + fn deserialize_reverse_dep_alignment_max_group_size_override() { + let json = r#"{"build": {"reverseDepAlignmentMaxGroupSize": 32}}"#; + let config: BuildConfig = serde_json::from_str(json).unwrap(); + assert_eq!(config.build.reverse_dep_alignment_max_group_size, 32); } #[test] diff --git a/docs/guides/configuration.md b/docs/guides/configuration.md index a6793c30..a16196ac 100644 --- a/docs/guides/configuration.md +++ b/docs/guides/configuration.md @@ -157,6 +157,7 @@ Controls graph construction. | `driftThreshold` | `number` | `0.2` | Fraction (0–1). If incremental rebuild changes node or edge counts by more than this, codegraph warns and suggests `--no-incremental`. | | `smallFilesThreshold` | `number` | `5` | When ≤ this many files change in an incremental build, codegraph takes faster code paths (skips full rebuilds of structure metrics, scoped barrel re-parsing, JS fallback for inserts). | | `execMaxBufferBytes` | `number` | `104857600` | Max stdout buffer size (bytes) for the `git check-ignore` subprocess spawned while detecting gitignored files during native-orchestrator drop detection. | +| `reverseDepAlignmentMaxGroupSize` | `number` | `200` | Max size of a same-(name, kind) sibling group (e.g. several `close() {}` methods in one file) that incremental rebuilds will line-align when reconnecting reverse-dependency call edges. Larger groups fall back to nearest-line matching. | --- diff --git a/src/domain/graph/builder/context.ts b/src/domain/graph/builder/context.ts index 7f7a085a..2cf8584c 100644 --- a/src/domain/graph/builder/context.ts +++ b/src/domain/graph/builder/context.ts @@ -96,16 +96,6 @@ export class PipelineContext { tgtKind: string; tgtFile: string; tgtLine: number; - /** - * 1-based rank of the target (by ascending line) among nodes sharing its - * (name, kind) within `tgtFile`, computed at save time. Lets - * `reconnectReverseDepEdges` map an old target to its correct new node - * even when multiple distinct symbols in the file share the same name - * and kind (e.g. several object-literal `close() {}` methods) — see #1752. - */ - tgtOrdinal: number; - /** Size of that (name, kind) sibling group at save time. */ - tgtSiblingCount: number; edgeKind: string; confidence: number; dynamic: number; @@ -113,6 +103,21 @@ export class PipelineContext { dynamicKind: string | null; }> = []; + /** + * Pre-purge snapshot of the sorted line list for every (name, kind) + * sibling group referenced by `savedReverseDepEdges`, keyed by + * `name|kind|file`. A file can contain multiple distinct symbols sharing + * the identical name and kind — e.g. several object-literal `close() {}` + * methods — so `(name, kind, file)` alone is not a unique identity. + * `reconnectReverseDepEdges` aligns this old line list against the + * post-purge candidate lines (order-preserving, minimum line-shift) to + * map each saved edge to its correct new target even when the sibling + * group itself was shifted, and even when the group's size changed + * because a same-named sibling was added or removed in the same edit + * (#1752, #1865). + */ + savedSiblingGroups: Map = new Map(); + // ── Misc state ───────────────────────────────────────────────────── hasEmbeddings: boolean = false; lineCountMap!: Map; diff --git a/src/domain/graph/builder/stages/build-edges.ts b/src/domain/graph/builder/stages/build-edges.ts index 8253401a..62924736 100644 --- a/src/domain/graph/builder/stages/build-edges.ts +++ b/src/domain/graph/builder/stages/build-edges.ts @@ -2032,29 +2032,128 @@ function applyEdgeTechniquesAfterNativeInsert( // ── Reverse-dep edge reconnection (#932, #933) ───────────────────────── /** - * Picks the correct reconnect target among same-(name,kind,file) candidates - * (sorted by ascending line). + * Aligns two ascending line arrays representing the same-(name,kind) sibling + * group before (`oldLines`) and after (`newLines`) a purge+reinsert. * - * When only one candidate exists, it's an unambiguous match. When several - * exist (e.g. multiple object-literal `close() {}` methods in one file) and - * the sibling-group size is unchanged since save, the saved ordinal — the - * target's rank by line among its siblings at save time — reliably - * identifies the original target even though the whole group may have - * shifted by an arbitrary number of lines. Falls back to nearest-line only - * when the sibling count itself changed (a same-named sibling was added or - * removed), since the ordinal mapping can no longer be trusted — see #1752. + * When the sibling count is unchanged, declarations of the same name and + * kind within one file keep their relative textual order across an edit — + * even when the whole group shifts by an arbitrary, non-uniform amount per + * sibling (e.g. one sibling's own body grew independently) — so mapping by + * rank (1st old -> 1st new, 2nd -> 2nd, ...) is always correct (#1752). + * + * When the count changed (a same-named sibling was added or removed in the + * same edit), rank order alone can't tell which element is the new/missing + * one. But the untouched siblings' OWN source text wasn't edited, so they + * all shift by the exact SAME line delta — whatever unrelated insertion or + * deletion elsewhere in the file caused the shift applies uniformly to + * everything below/above it. This finds the single shift value `S` that + * makes `old + S` land on a real new line for the most siblings — the + * dominant shift — and matches every old line whose shifted position + * exists in `newLines`; the rest were removed. This is far more reliable + * than picking whichever old/new pairing merely minimizes total line + * distance, which a uniform shift can fool once an old line coincidentally + * ends up numerically closer to a different sibling's new position than to + * its own (confirmed against this repo's real #1752 fixture numbers) — see + * #1865. + * + * Returns a map from each old line to its aligned new line. An old line + * missing from the result means its sibling was removed (no new line + * matches it) — callers must drop, not guess, in that case. + */ +function alignSiblingLines(oldLines: number[], newLines: number[]): Map { + const result = new Map(); + if (oldLines.length === 0 || newLines.length === 0) return result; + + if (oldLines.length === newLines.length) { + for (let i = 0; i < oldLines.length; i++) result.set(oldLines[i]!, newLines[i]!); + return result; + } + + const newLineSet = new Set(newLines); + const shiftCounts = new Map(); + for (const oldLine of oldLines) { + for (const newLine of newLines) { + const shift = newLine - oldLine; + shiftCounts.set(shift, (shiftCounts.get(shift) ?? 0) + 1); + } + } + // Tie-break fully by value (not iteration/insertion order) so this stays + // identical to the Rust mirror, whose HashMap iterates in unspecified + // order: prefer the higher match count, then the smaller magnitude + // shift, then the smaller signed shift. + let bestShift = 0; + let bestCount = -1; + for (const [shift, count] of shiftCounts) { + const better = + count > bestCount || + (count === bestCount && + (Math.abs(shift) < Math.abs(bestShift) || + (Math.abs(shift) === Math.abs(bestShift) && shift < bestShift))); + if (better) { + bestShift = shift; + bestCount = count; + } + } + for (const oldLine of oldLines) { + const candidate = oldLine + bestShift; + if (newLineSet.has(candidate)) result.set(oldLine, candidate); + } + return result; +} + +/** + * Picks the correct reconnect target among same-(name,kind,file) candidates. + * + * A single candidate is an unambiguous match. With several candidates (e.g. + * multiple object-literal `close() {}` methods in one file), the saved + * sibling-group snapshot from before purge is aligned against the current + * candidate lines (see `alignSiblingLines`) to find which new line the saved + * target's old line maps to — correct even when the whole group shifted and + * its size changed in the same edit. Falls back to nearest-line only when no + * sibling-group snapshot is available, or the group is too large to align + * cheaply. */ function pickReconnectTarget( candidates: Array<{ id: number; line: number }>, - tgtOrdinal: number, - tgtSiblingCount: number, tgtLine: number, + groupKey: string, + savedSiblingGroups: ReadonlyMap, + alignmentCache: Map>, + maxAlignGroupSize: number, ): number | null { if (candidates.length === 0) return null; if (candidates.length === 1) return candidates[0]!.id; - if (candidates.length === tgtSiblingCount && tgtOrdinal >= 1 && tgtOrdinal <= candidates.length) { - return candidates[tgtOrdinal - 1]!.id; + + const oldLines = savedSiblingGroups.get(groupKey); + if ( + oldLines && + oldLines.length > 0 && + oldLines.length <= maxAlignGroupSize && + candidates.length <= maxAlignGroupSize + ) { + let alignment = alignmentCache.get(groupKey); + if (!alignment) { + alignment = alignSiblingLines( + oldLines, + candidates.map((c) => c.line), + ); + alignmentCache.set(groupKey, alignment); + } + const newLine = alignment.get(tgtLine); + if (newLine === undefined) { + // tgtLine's sibling was legitimately removed in this edit — the + // alignment already accounted for the size change, so falling + // through to nearest-line here would silently reattach to an + // unrelated sibling instead of correctly dropping this edge. + return null; + } + const match = candidates.find((c) => c.line === newLine); + if (match) return match.id; } + + // No sibling-group snapshot (shouldn't normally happen — every saved edge + // has one recorded at save time) or the group exceeds the alignment size + // cap — fall back to nearest-line. let best = candidates[0]!; let bestDist = Math.abs(best.line - tgtLine); for (const c of candidates) { @@ -2071,11 +2170,11 @@ function pickReconnectTarget( * Reconnect edges that were saved before changed-file purge. * * Each saved edge records: sourceId (still valid — reverse-dep nodes were not - * purged) and target attributes (name, kind, file, line, ordinal, sibling - * count). The target node was deleted and re-inserted with a new ID by - * insertNodes. We look up all (name, kind, file) candidates and pick the one - * matching the saved ordinal (see `pickReconnectTarget`), then re-create the - * edge. + * purged) and target attributes (name, kind, file, line). The target node + * was deleted and re-inserted with a new ID by insertNodes. We look up all + * (name, kind, file) candidates and pick the one the saved sibling-group + * snapshot aligns to the saved line (see `pickReconnectTarget`), then + * re-create the edge. */ function reconnectReverseDepEdges(ctx: PipelineContext): void { const { db } = ctx; @@ -2089,6 +2188,10 @@ function reconnectReverseDepEdges(ctx: PipelineContext): void { // often share the same target (e.g. several callers of the same // function), so this avoids re-querying per edge. const candidatesCache = new Map>(); + // Cache the (potentially expensive) alignment result per group too — + // shared across every saved edge targeting the same sibling group. + const alignmentCache = new Map>(); + const maxAlignGroupSize = ctx.config.build.reverseDepAlignmentMaxGroupSize; for (const saved of ctx.savedReverseDepEdges) { const cacheKey = `${saved.tgtName}|${saved.tgtKind}|${saved.tgtFile}`; @@ -2103,9 +2206,11 @@ function reconnectReverseDepEdges(ctx: PipelineContext): void { const newId = pickReconnectTarget( candidates, - saved.tgtOrdinal, - saved.tgtSiblingCount, saved.tgtLine, + cacheKey, + ctx.savedSiblingGroups, + alignmentCache, + maxAlignGroupSize, ); if (newId != null) { reconnectedRows.push([ diff --git a/src/domain/graph/builder/stages/detect-changes.ts b/src/domain/graph/builder/stages/detect-changes.ts index 94d59a5f..1bb477b5 100644 --- a/src/domain/graph/builder/stages/detect-changes.ts +++ b/src/domain/graph/builder/stages/detect-changes.ts @@ -413,44 +413,37 @@ function captureRemovedFileNeighbors(db: BetterSqlite3Database, removedFiles: st } /** - * Computes each node's 1-based ordinal rank (by ascending line) among nodes - * sharing its (name, kind) within `file`, plus the sibling-group size, keyed - * by `name|kind|line`. + * Computes the sorted line list for every (name, kind) sibling group within + * `file`, keyed by `name|kind`. * * A file can contain multiple distinct symbols with the identical name and * kind — e.g. several object-literal `close() {}` methods returned from * different functions in the same file. `(name, kind, file)` alone is not a * unique identity for such symbols, so `reconnectReverseDepEdges` cannot * safely tell them apart by nearest-line matching once unrelated code shifts - * the candidates unevenly (#1752). The ordinal recorded here — the target's - * rank among same-named siblings at save time — lets reconnection map an old - * target to its new node correctly as long as the sibling count is - * unchanged, regardless of how far the whole group has shifted. + * the candidates unevenly, or a same-named sibling is added/removed in the + * same edit (#1752, #1865). The sorted line list captured here — the + * sibling group's layout at save time — lets reconnection align old targets + * to their correct new nodes by rank when the sibling count is unchanged, + * or by the dominant line-shift that best explains the surviving siblings + * when it changed (see `alignSiblingLines` in `build-edges.ts`), which + * tolerates both a uniform shift of the whole group AND a change in the + * group's size. */ -function computeNodeOrdinals( - db: BetterSqlite3Database, - file: string, -): Map { - const result = new Map(); +function computeSiblingGroups(db: BetterSqlite3Database, file: string): Map { + const groups = new Map(); const rows = db.prepare('SELECT name, kind, line FROM nodes WHERE file = ?').all(file) as Array<{ name: string; kind: string; line: number; }>; - const groups = new Map(); for (const row of rows) { const groupKey = `${row.name}|${row.kind}`; if (!groups.has(groupKey)) groups.set(groupKey, []); groups.get(groupKey)!.push(row.line); } - for (const [groupKey, lines] of groups) { - lines.sort((a, b) => a - b); - const siblingCount = lines.length; - lines.forEach((line, idx) => { - result.set(`${groupKey}|${line}`, { ordinal: idx + 1, siblingCount }); - }); - } - return result; + for (const lines of groups.values()) lines.sort((a, b) => a - b); + return groups; } /** @@ -501,7 +494,7 @@ function addReverseDeps( // Must be computed BEFORE this file's nodes are purged — captures the // pre-purge sibling layout so reconnection can map old→new correctly // even when several same-named/same-kind symbols exist in the file. - const ordinals = computeNodeOrdinals(db, changedPath); + const groups = computeSiblingGroups(db, changedPath); for (const row of saveEdgesStmt.all(changedPath) as Array<{ source_id: number; tgt_name: string; @@ -518,17 +511,19 @@ function addReverseDeps( // Skip edges whose source is also being purged — buildEdges will // re-create them with correct new IDs. if (changePathSet.has(row.src_file)) continue; - const { ordinal, siblingCount } = ordinals.get( - `${row.tgt_name}|${row.tgt_kind}|${row.tgt_line}`, - ) ?? { ordinal: 1, siblingCount: 1 }; + const groupKey = `${row.tgt_name}|${row.tgt_kind}|${row.tgt_file}`; + if (!ctx.savedSiblingGroups.has(groupKey)) { + ctx.savedSiblingGroups.set( + groupKey, + groups.get(`${row.tgt_name}|${row.tgt_kind}`) ?? [row.tgt_line], + ); + } ctx.savedReverseDepEdges.push({ sourceId: row.source_id, tgtName: row.tgt_name, tgtKind: row.tgt_kind, tgtFile: row.tgt_file, tgtLine: row.tgt_line, - tgtOrdinal: ordinal, - tgtSiblingCount: siblingCount, edgeKind: row.edge_kind, confidence: row.confidence, dynamic: row.dynamic, diff --git a/src/infrastructure/config.ts b/src/infrastructure/config.ts index 9fccf1d1..b1ffc395 100644 --- a/src/infrastructure/config.ts +++ b/src/infrastructure/config.ts @@ -64,6 +64,15 @@ export const DEFAULTS = deepFreeze({ * `execFileSync` in `queryGitIgnoredFiles()` (src/domain/graph/builder/stages/native-orchestrator.ts). */ execMaxBufferBytes: 100 * 1024 * 1024, + /** + * Max size of a same-(name, kind) sibling group `reconnectReverseDepEdges` + * will run its line-alignment against (#1865). Building the shift + * histogram is O(n*m) in the old/new group sizes; groups above this size + * fall back to nearest-line matching to bound worst-case incremental-build + * cost. Mirrors `reverse_dep_alignment_max_group_size` in + * `crates/codegraph-core/src/infrastructure/config.rs`. + */ + reverseDepAlignmentMaxGroupSize: 200, }, db: { /** diff --git a/src/types.ts b/src/types.ts index 255f66be..3946f779 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1394,6 +1394,14 @@ export interface CodegraphConfig { * files out of the dropped-language-gap detection). Default: 104857600 (100 MB). */ execMaxBufferBytes: number; + /** + * Max size of a same-(name, kind) sibling group `reconnectReverseDepEdges` + * will run its line-alignment against (#1865). Groups above this size fall + * back to nearest-line matching to bound worst-case incremental-build cost. + * Mirrors `reverse_dep_alignment_max_group_size` in + * `crates/codegraph-core/src/infrastructure/config.rs`. + */ + reverseDepAlignmentMaxGroupSize: number; }; db: { diff --git a/tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts b/tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts new file mode 100644 index 00000000..f9d0b4ed --- /dev/null +++ b/tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts @@ -0,0 +1,286 @@ +/** + * Regression test for #1865: `reconnectReverseDepEdges` (`build-edges.ts`, + * WASM/JS engine) correctly reconnects reverse-dep call edges across an + * incremental rebuild that shifts a group of same-(name, kind) sibling + * declarations (#1752) — but ONLY as long as the sibling count itself stays + * the same. When a same-named/same-kind sibling is ALSO added or removed in + * the SAME edit that shifts the group, the #1752 fix fell back to its + * pre-#1752 nearest-line heuristic — exactly the heuristic #1752 proved + * unreliable once a same-named group shifts far enough. + * + * Fixed by replacing the ordinal/nearest-line two-tier heuristic with a + * single alignment: match old-to-new siblings by rank when the sibling + * count is unchanged (subsumes #1752's fix), or by the single dominant + * line-shift that best explains the surviving (untouched) siblings when the + * count changed — since unrelated code shifts them all by the exact same + * delta, regardless of how far the group has moved (see `alignSiblingLines` + * in `build-edges.ts`; mirrored in Rust as `align_sibling_lines` in + * `detect_changes.rs`, covered by dedicated Rust unit tests there — + * `pick_reconnect_target_drops_edge_when_its_own_sibling_was_removed`, + * `pick_reconnect_target_survives_added_sibling_plus_shift`, + * `reconnect_survives_shift_plus_sibling_removed_in_same_edit`). + * + * As of #1863, name-only resolution of an ambiguous same-file/same-name/ + * same-kind call (the mechanism the original #1752 fixture used to produce + * its saved reverse-dep edges) now resolves to NO edge instead of fanning + * out — so a real source fixture can no longer *naturally* produce a saved + * edge targeting one specific sibling among several sharing a raw (name, + * kind, file) triple (typed/receiver-qualified dispatch avoids the + * ambiguity by qualifying the node's own name, e.g. `OpenA.close`, which no + * longer collides with its siblings at all). This test instead seeds the + * exact DB topology `addReverseDeps`/`reconnectReverseDepEdges` operate on + * directly — four real `close()` method nodes (produced by a real parse) + * plus four synthetic reverse-dep `calls` edges, one per method — then + * exercises the REAL incremental `buildGraph()` pipeline end-to-end, + * mirroring the direct-DB-fixture strategy the original #1752 Rust unit + * tests use for the same reason. + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; + +const REL_CONN_FILE = 'src/db/conn.ts'; + +/** + * Four distinct functions, each returning an object with its OWN `close()` + * method — same name ("close") and kind ("method") repeated four times in + * one file, exactly the ambiguous shape #1752 fixed reconnection for + * (mirrors `src/db/connection.ts`'s four real `close` methods). + */ +function connSource(): string { + return `export function openA() { + return { + close() { + return 'A'; + }, + }; +} + +export function openB() { + return { + close() { + return 'B'; + }, + }; +} + +export function openC() { + return { + close() { + return 'C'; + }, + }; +} + +export function openD() { + return { + close() { + return 'D'; + }, + }; +} +`; +} + +/** + * Same fixture, but: (1) an unrelated helper function is inserted above ALL + * FOUR functions, shifting every `close()` sibling down by a fixed amount + * without touching any of their own source text, AND (2) `openB`'s `close` + * is renamed to `shutdown` — changing the sibling count for + * `(name="close", kind="method")` in this file from 4 to 3 in the SAME edit. + */ +function editedConnSource(): string { + return `function helperPadding(x: number): number { + // Several lines added here shift every function below down by a fixed + // amount, without touching their own source text at all. + console.log('padding line 1'); + console.log('padding line 2'); + console.log('padding line 3'); + console.log('padding line 4'); + console.log('padding line 5'); + return x + 1; +} + +export function openA() { + return { + close() { + return 'A'; + }, + }; +} + +export function openB() { + return { + shutdown() { + return 'B'; + }, + }; +} + +export function openC() { + return { + close() { + return 'C'; + }, + }; +} + +export function openD() { + return { + close() { + return 'D'; + }, + }; +} +`; +} + +/** + * One trivial caller function per sibling, in its own untouched file. Each + * imports its corresponding `open*` function so `findReverseDependencies` + * genuinely classifies this file as a reverse-dep of `conn.ts` — the actual + * `close()` reverse-dep call edge is seeded separately (see module doc + * comment), independent of whatever this import is used for. + */ +function callerSource(name: string, openFn: string): string { + return `import { ${openFn} } from '../db/conn.js'; + +export function ${name}(): void { + ${openFn}(); +} +`; +} + +function writeFixture(root: string, conn: string): void { + fs.mkdirSync(path.join(root, 'src', 'db'), { recursive: true }); + fs.mkdirSync(path.join(root, 'src', 'features'), { recursive: true }); + fs.writeFileSync(path.join(root, REL_CONN_FILE), conn); + fs.writeFileSync(path.join(root, 'src/features/useA.ts'), callerSource('useA', 'openA')); + fs.writeFileSync(path.join(root, 'src/features/useB.ts'), callerSource('useB', 'openB')); + fs.writeFileSync(path.join(root, 'src/features/useC.ts'), callerSource('useC', 'openC')); + fs.writeFileSync(path.join(root, 'src/features/useD.ts'), callerSource('useD', 'openD')); +} + +/** + * Seeds one synthetic `calls` edge from each `use*` caller to its + * corresponding `close()` method node — simulating what a high-confidence + * resolution technique would have produced, without depending on any + * specific name-resolution behavior (see module doc comment for why a real + * source pattern can no longer naturally produce this topology post-#1863). + */ +function seedReverseDepEdges(dbPath: string): void { + const db = new Database(dbPath); + try { + const closeNodes = db + .prepare( + "SELECT id, line FROM nodes WHERE name = 'close' AND kind = 'method' AND file = ? ORDER BY line", + ) + .all(REL_CONN_FILE) as Array<{ id: number; line: number }>; + expect(closeNodes).toHaveLength(4); + const callerIds: Record = {}; + for (const name of ['useA', 'useB', 'useC', 'useD']) { + const row = db + .prepare("SELECT id FROM nodes WHERE name = ? AND kind = 'function'") + .get(name) as { id: number } | undefined; + expect(row, `caller node ${name} must exist`).toBeDefined(); + callerIds[name] = row!.id; + } + const insert = db.prepare( + "INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, 'calls', 0.9, 0)", + ); + insert.run(callerIds.useA, closeNodes[0]!.id); + insert.run(callerIds.useB, closeNodes[1]!.id); + insert.run(callerIds.useC, closeNodes[2]!.id); + insert.run(callerIds.useD, closeNodes[3]!.id); + } finally { + db.close(); + } +} + +/** Sorted list of target lines every `close()` reverse-dep edge points to, keyed by caller. */ +function findCloseTargetLines(dbPath: string): Record { + const db = new Database(dbPath, { readonly: true }); + try { + const result: Record = {}; + for (const caller of ['useA', 'useB', 'useC', 'useD']) { + const row = db + .prepare( + `SELECT t.line AS line + FROM edges e + JOIN nodes s ON e.source_id = s.id + JOIN nodes t ON e.target_id = t.id + WHERE s.name = ? AND e.kind = 'calls' AND t.file = ? AND t.kind = 'method'`, + ) + .get(caller, REL_CONN_FILE) as { line: number } | undefined; + result[caller] = row?.line ?? null; + } + return result; + } finally { + db.close(); + } +} + +describe('Issue #1865: reverse-dep edges survive a same-named-sibling line shift AND count change in the same edit (wasm)', () => { + let projDir: string; + const tmpDirs: string[] = []; + const dbPath = () => path.join(projDir, '.codegraph', 'graph.db'); + + function mkTmp(prefix: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + tmpDirs.push(dir); + return dir; + } + + beforeAll(async () => { + projDir = mkTmp('cg-1865-wasm-'); + writeFixture(projDir, connSource()); + await buildGraph(projDir, { engine: 'wasm', incremental: false, skipRegistry: true }); + seedReverseDepEdges(dbPath()); + }, 60_000); + + afterAll(() => { + for (const dir of tmpDirs) fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('baseline: each seeded caller points to its own close() line', () => { + expect(findCloseTargetLines(dbPath())).toEqual({ useA: 3, useB: 11, useC: 19, useD: 27 }); + }); + + it('incremental rebuild that shifts the whole close() sibling group AND renames one ' + + 'sibling away (4 -> 3) in the same edit reconnects the untouched siblings correctly ' + + 'and drops the edge to the removed one', async () => { + // Edit conn.ts only: shift + rename openB's close -> shutdown. None of + // the use*.ts caller files are touched. + fs.writeFileSync(path.join(projDir, REL_CONN_FILE), editedConnSource()); + await buildGraph(projDir, { engine: 'wasm', skipRegistry: true }); // incremental (default) + + const afterIncremental = findCloseTargetLines(dbPath()); + + // Ground truth: shift the ORIGINAL (pre-edit) close() lines by hand — + // this is exactly what a correct reconnection must produce. A's/C's/ + // D's own bodies never changed, only the padding above them, so each + // survivor's new line is fully determined by the file's own new + // content, independent of the reconnection algorithm under test. + const newConnLines = fs + .readFileSync(path.join(projDir, REL_CONN_FILE), 'utf8') + .split('\n') + .reduce((acc, line, idx) => { + if (line.trim() === 'close() {') acc.push(idx + 1); + return acc; + }, []); + expect(newConnLines).toHaveLength(3); // A, C, D — B was renamed away + + expect( + afterIncremental, + `incremental reconnection result: ${JSON.stringify(afterIncremental)}`, + ).toEqual({ + useA: newConnLines[0], + useB: null, // openB's close() no longer exists — edge must be dropped, not mis-wired + useC: newConnLines[1], + useD: newConnLines[2], + }); + }, 60_000); +});