Skip to content

fix: reconnect reverse-dep edges correctly when a sibling group's size also changes#2014

Open
carlos-alm wants to merge 1 commit into
fix/issue-1847-native-ffi-hybrid-import-edge-path-dropsfrom
fix/issue-1865-reconnectreversedepedges-still-mis-wires-same
Open

fix: reconnect reverse-dep edges correctly when a sibling group's size also changes#2014
carlos-alm wants to merge 1 commit into
fix/issue-1847-native-ffi-hybrid-import-edge-path-dropsfrom
fix/issue-1865-reconnectreversedepedges-still-mis-wires-same

Conversation

@carlos-alm

@carlos-alm carlos-alm commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1865 — both scenarios named in the issue's own reproduction (a same-named sibling removed, and one added, each combined with a whole-group line shift) are fixed and verified on both engines. Filed #2015 for a genuinely distinct, out-of-scope follow-up: a same-named sibling simultaneously added and removed in one edit (net-zero count change) is undetectable from line data alone and needs per-declaration content hashing, as floated as one possible direction in #1865's own text — a separate, schema-touching change.

Test plan

  • New Rust unit tests in detect_changes.rs: pick_reconnect_target_drops_edge_when_its_own_sibling_was_removed, pick_reconnect_target_survives_added_sibling_plus_shift, align_sibling_lines_forces_exact_pairing_when_counts_match, align_sibling_lines_empty_inputs_yield_empty_map, reconnect_survives_shift_plus_sibling_removed_in_same_edit (end-to-end, real Incremental rebuild leaves stale blast-radius/fn-impact numbers for line-shifted declarations (same root cause as #1738, #1743) #1752 fixture line numbers) — plus updated existing ordinal tests for the new API shape.
  • New TS integration test tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts — seeds the exact DB topology addReverseDeps/reconnectReverseDepEdges operate on (four real close() method nodes + four synthetic reverse-dep edges), then exercises the real incremental buildGraph() pipeline. A/B-verified: reverted to pre-fix source and confirmed this test fails with the exact mis-wiring signature described in the issue (three of four callers collapse onto the same wrong target); restored the fix and confirmed it passes.
  • cargo test --release — 569 passed, 0 failed (full crate suite)
  • cargo clippy --release — no new warnings (fixed one type_complexity warning introduced by an extended tuple return type via a named type alias)
  • npm test — full suite green: 3882 passed, 0 failed, 30 skipped, 2 todo (native addon locally built + codesigned to exercise both engines)
  • npm run lint / npm run typecheck / npm run build — clean
  • codegraph diff-impact --staged -T — confirms the change is scoped exactly to the reverse-dep reconnection subsystem (7 functions, 13 transitive callers, all within build-edges.ts/detect-changes.ts/context.ts/types.ts)

Stacked on #1847 (base branch fix/issue-1847-native-ffi-hybrid-import-edge-path-drops) — only the reverse-dep reconnection diff is this issue's change.

…e also changes

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.
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the two-tier ordinal/nearest-line heuristic for reconnecting reverse-dep call edges with a single alignSiblingLines / align_sibling_lines alignment that handles both unchanged-count (rank-based zip) and changed-count (dominant-shift histogram) scenarios, fixing the compound case where a same-named sibling is added or removed in the same edit that shifts the whole group (#1865).

  • computeSiblingGroups (TS + Rust) now stores the full pre-purge sorted line list for each (name, kind, file) group instead of per-node ordinal/count pairs; this snapshot is threaded through the pipeline via ctx.savedSiblingGroups / saved_sibling_groups and consumed by pickReconnectTarget / pick_reconnect_target.
  • alignSiblingLines / align_sibling_lines zips by rank when counts match, and finds the dominant shift (O(n·m), capped at reverseDepAlignmentMaxGroupSize=200) when counts differ, correctly dropping edges whose target sibling was removed rather than falling through to nearest-line.
  • New Rust unit tests cover the end-to-end compound scenario; a new TS integration test seeds the exact DB topology and exercises the real incremental buildGraph() pipeline end-to-end, A/B-verified against the pre-fix code.

Confidence Score: 4/5

Safe to merge; changes are scoped entirely to the reverse-dep reconnection subsystem and are well-tested by both Rust unit tests and a TS end-to-end integration test.

The dominant-shift alignment is correct and well-verified for the described compound scenario. One small asymmetry exists: after a successful alignment lookup, the TS pickReconnectTarget falls through to nearest-line if the candidate isn't found at the aligned line (dead code), whereas Rust always returns None in that branch. In practice this path is unreachable because the alignment is built from the very same candidates list that's queried afterward, but it leaves the TS behavior subtly inconsistent with Rust and could silently mis-wire an edge if the invariant were ever violated by a future change.

src/domain/graph/builder/stages/build-edges.ts — the pickReconnectTarget fallthrough after if (match) return match.id.

Important Files Changed

Filename Overview
src/domain/graph/builder/stages/build-edges.ts Replaces ordinal/nearest-line heuristic with alignSiblingLines dominant-shift approach; new pickReconnectTarget has a silent nearest-line fallthrough after a successful alignment that diverges from the Rust mirror's drop-edge behavior.
crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs Rust mirror of the alignment fix: align_sibling_lines and updated pick_reconnect_target are logically consistent; always returns via return match ..., so the drop-on-removed-sibling semantics are guaranteed. New end-to-end test verifies the compound scenario.
src/domain/graph/builder/stages/detect-changes.ts Replaces computeNodeOrdinals with computeSiblingGroups and stores whole sibling-group line lists on ctx.savedSiblingGroups; logic is correct and the guard idiom ensures pre-purge state is not overwritten by a later iteration.
crates/codegraph-core/src/domain/graph/builder/pipeline.rs Plumbing-only changes: extends SaveAndPurgeResult to carry saved_sibling_groups and threads it through to reconnect_saved_reverse_dep_edges; the type alias cleanly documents the tuple shape.
tests/integration/issue-1865-reverse-dep-sibling-count-change.test.ts Well-structured integration test that seeds the exact DB topology, exercises the real incremental pipeline, and validates correct reconnection of surviving siblings and edge-drop for the renamed one.
src/domain/graph/builder/context.ts Removes tgtOrdinal/tgtSiblingCount from SavedReverseDepEdge and adds the savedSiblingGroups map; type changes are consistent with the new algorithm.
src/infrastructure/config.ts Adds reverseDepAlignmentMaxGroupSize: 200 to DEFAULTS.build; default matches the Rust counterpart.
src/types.ts Adds reverseDepAlignmentMaxGroupSize: number to the public CodegraphConfig interface with a cross-reference to the Rust mirror.
crates/codegraph-core/src/infrastructure/config.rs Adds reverse_dep_alignment_max_group_size: usize with a serde default function; the Default impl is updated to match and two new config tests verify round-trip deserialization.
docs/guides/configuration.md Documents the new reverseDepAlignmentMaxGroupSize option with a correct default value and a clear description of the fallback behaviour.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["save_and_purge_changed / addReverseDeps"] -->|"computeSiblingGroups before purge"| B["savedSiblingGroups: name,kind,file to sorted lines"]
    A --> C["savedReverseDepEdges: one per caller-to-target edge"]
    B --> D["reconnect_reverse_dep_edges"]
    C --> D
    D --> E{"candidates.length == 1?"}
    E -->|yes| F["return that id"]
    E -->|no| G{"savedSiblingGroups has key AND size <= maxAlignGroupSize?"}
    G -->|no| H["nearest-line fallback"]
    G -->|yes| I["alignSiblingLines: old_lines vs new_lines"]
    I --> J{"counts equal?"}
    J -->|yes| K["rank-based zip: 1st to 1st, 2nd to 2nd"]
    J -->|no| L["shift histogram O(n*m): pick dominant shift S"]
    K --> M["alignment map: old_line to new_line"]
    L --> M
    M --> N{"tgt_line in map?"}
    N -->|yes| O["return matched candidate id"]
    N -->|no| P["return None or null: edge dropped"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["save_and_purge_changed / addReverseDeps"] -->|"computeSiblingGroups before purge"| B["savedSiblingGroups: name,kind,file to sorted lines"]
    A --> C["savedReverseDepEdges: one per caller-to-target edge"]
    B --> D["reconnect_reverse_dep_edges"]
    C --> D
    D --> E{"candidates.length == 1?"}
    E -->|yes| F["return that id"]
    E -->|no| G{"savedSiblingGroups has key AND size <= maxAlignGroupSize?"}
    G -->|no| H["nearest-line fallback"]
    G -->|yes| I["alignSiblingLines: old_lines vs new_lines"]
    I --> J{"counts equal?"}
    J -->|yes| K["rank-based zip: 1st to 1st, 2nd to 2nd"]
    J -->|no| L["shift histogram O(n*m): pick dominant shift S"]
    K --> M["alignment map: old_line to new_line"]
    L --> M
    M --> N{"tgt_line in map?"}
    N -->|yes| O["return matched candidate id"]
    N -->|no| P["return None or null: edge dropped"]
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix: reconnect reverse-dep edges correct..." | Re-trigger Greptile

Comment on lines +2150 to +2156
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent nearest-line fallthrough after a successful alignment

When alignment.get(tgtLine) returns a newLine but candidates.find((c) => c.line === newLine) returns undefined (theoretically impossible since the alignment is built from candidates.map((c) => c.line), but still reachable as dead code), the TS function falls through to the nearest-line block — reconnecting the edge to a likely-wrong target. The Rust mirror (pick_reconnect_target) instead always returns inside the alignment block, so .find().map() returning None correctly drops the edge. Adding an explicit return null after the if (match) check would close this gap and make the misleading "No sibling-group snapshot … fall back to nearest-line" comment accurate for the code paths that actually reach it.

Fix in Claude Code

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

7 functions changed13 callers affected across 5 files

  • PipelineContext in src/domain/graph/builder/context.ts:22 (4 transitive callers)
  • alignSiblingLines in src/domain/graph/builder/stages/build-edges.ts:2063 (3 transitive callers)
  • pickReconnectTarget in src/domain/graph/builder/stages/build-edges.ts:2116 (3 transitive callers)
  • reconnectReverseDepEdges in src/domain/graph/builder/stages/build-edges.ts:2179 (3 transitive callers)
  • computeSiblingGroups in src/domain/graph/builder/stages/detect-changes.ts:433 (4 transitive callers)
  • addReverseDeps in src/domain/graph/builder/stages/detect-changes.ts:466 (4 transitive callers)
  • CodegraphConfig.build in src/types.ts:1352 (0 transitive callers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant