From 431c0e5e78780f6fc717aa45728368a5843e4bed Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Wed, 8 Jul 2026 02:01:45 -0600 Subject: [PATCH] fix: reconnect reverse-dep edges correctly when a sibling group's size also changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconnectReverseDepEdges (build-edges.ts) and its native mirror reconnect_reverse_dep_edges (detect_changes.rs) re-attach a reverse-dep caller's edge to its purged-and-reinserted target using a saved ordinal rank among same-(name, kind) siblings, falling back to nearest-line matching whenever the sibling count itself changed since save (a same-named sibling added or removed in the same edit). That fallback is exactly the nearest-line heuristic #1752 already proved unreliable once a same-named group shifts far enough, so the compound scenario (shift + count change in one incremental build) still produced call edges that diverged from a full rebuild. Replaces 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 exactly), or by the single dominant line-shift that best explains the surviving (untouched) siblings when the count changed. Untouched siblings' own source text is never edited, so they all shift by the exact same delta regardless of how far the group has moved — finding that shift correctly identifies which old sibling was removed (or which new one was added) without guessing based on raw line proximity, which a uniform shift can fool (confirmed against this repo's own real #1752 fixture numbers, where a naive minimum-distance alignment picks the wrong element to drop). Adds `build.reverseDepAlignmentMaxGroupSize` (default 200) bounding the alignment's shift-histogram cost for pathologically large sibling groups, mirrored in both engines' DEFAULTS. --- .../src/domain/graph/builder/pipeline.rs | 57 ++- .../graph/builder/stages/detect_changes.rs | 418 ++++++++++++++---- .../src/infrastructure/config.rs | 24 + docs/guides/configuration.md | 1 + src/domain/graph/builder/context.ts | 25 +- .../graph/builder/stages/build-edges.ts | 147 +++++- .../graph/builder/stages/detect-changes.ts | 49 +- src/infrastructure/config.ts | 9 + src/types.ts | 8 + ...5-reverse-dep-sibling-count-change.test.ts | 286 ++++++++++++ 10 files changed, 858 insertions(+), 166 deletions(-) create mode 100644 tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts 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); +});