[SarveshaKumarKS]

Pull Request

Linked issue

Closes #585

What & why

Two distinct definitions in the same file could produce the same base qualified name. Because nodes.qualified_name is unique and upsert_node() updated on that conflict, the later definition silently replaced the earlier node. Its line range disappeared, and distinct graph edges could collapse onto the surviving node.

Minimal reproduction:

nodes = [
    NodeInfo(kind="Function", name="render", file_path="/repo/app.py", line_start=10, line_end=12),
    NodeInfo(kind="Function", name="render", file_path="/repo/app.py", line_start=20, line_end=24),
]
store.store_file_nodes_edges("/repo/app.py", nodes, [])

Before:

[("/repo/app.py::render", 20, 24)]

After:

[("/repo/app.py::render", 10, 12),
 ("/repo/app.py::render:S<semantic-fingerprint>", 20, 24)]

Identity strategy

The first definition keeps the existing bare qualified name. Only genuine collisions are disambiguated, so all unambiguous identities remain unchanged.

A collided definition receives a deterministic suffix derived from semantic node fields rather than its source line. If multiple colliding definitions have identical semantic fields, a deterministic occurrence suffix is added. This preserves both definitions while keeping their identities stable when harmless lines are inserted above them during an incremental rebuild.

That stability matters because qualified names are also edge keys. A line-based suffix changed after a line shift, leaving cross-file incoming edges pointed at a deleted identity. The semantic suffix keeps those identities—and therefore their incoming edges—valid across the real store_file_nodes_edges() remove-and-replace lifecycle.

Edge-resolution trade-off

Batch storage aligns an edge endpoint with a collided identity only when the edge provides enough evidence:

• outgoing edges use the source definition whose line range contains the edge;
• CONTAINS edges use the target definition's start line; and
• recursive calls use the same disambiguated identity as their source definition.

An incoming call with no evidence about which duplicate it targets deliberately remains on the canonical bare identity. It is not assigned arbitrarily to a disambiguated definition.

How it was tested

The regression coverage now verifies:

• two same-file definitions with the same base qualified name both survive with distinct line ranges;
• direct re-indexing of the same definition updates in place without creating a duplicate;
• a harmless line insertion above duplicate definitions does not change their identities;
• distinct cross-file callers to every duplicate still resolve after re-indexing;
• recursive caller/callee queries resolve a disambiguated node; and
• ambiguous incoming targets remain explicitly attached to the canonical identity.

Validation on current main (805c4c6):

uv run pytest
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional

Results: 2,070 passed, 5 skipped, 2 xpassed; Ruff clean; mypy clean across 66 source files.

Checklist

• Tests added for new functionality
• All tests pass: uv run pytest
• Linting passes: uv run ruff check code_review_graph/
• Type checking passes: uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
• Lines are at most 100 characters
• Implementation docstrings describe the behavior

[tirth8205]

I independently applied this head to current main and ran the submitted collision tests; all 3 pass. It is not mergeable yet because the missing incremental lifecycle case loses graph meaning. Exact failing regression: store two same-file render definitions, create an incoming CALLS edge to the second identity (target.py::render:L20), insert one harmless line above both definitions, and re-index the target file. The second identity becomes :L21; the :L20 node is gone, while the cross-file incoming edge still targets :L20, so it is dangling. Root cause: the identity encodes line_start, and store_file_nodes_edges() removes the file rows before identities are recomputed. The direct upsert_node() test does not exercise that real replace/re-index path. Please replace the line-based identity with a semantic stable identity, or implement explicit old-to-new identity migration for incoming edges, and add regressions for harmless line shifts, callers to every duplicate, and explicit ambiguous-target behavior. I am keeping both the PR and #585 open until those cases pass.

[SarveshaKumarKS]

Thanks for the detailed regression. I reproduced the dangling-edge failure and pushed a revised implementation.
The line-based :L<line_start> identity has been replaced with a deterministic semantic fingerprint, so harmless line shifts no longer change collided node identities or leave incoming edges dangling.
I added regressions covering the real store_file_nodes_edges() lifecycle, callers to every duplicate, line-shift re-indexing, direct upsert idempotency, recursive resolution, and explicit ambiguous-target behavior.
The PR is rebased onto current main, and the updated validation is:
2,070 passed, 5 skipped, 2 xpassed
Ruff clean
mypy clean across 66 source files
Updated head: 1abf73b
The PR description has also been updated with the identity and edge-resolution trade-offs. When you have a chance, could you please re-review the revised implementation?

[tirth8205]

I revalidated the updated head after the lifecycle reconciliation work. The original same-file collision is improved, but one identity transition still leaves a dangling edge: index two same-file render definitions, point a caller at the second semantic-suffix identity, then remove the first definition and reindex the surviving second with a harmless line shift. The survivor changes from target.py::render:S9354c815bcc1 to bare target.py::render, while the caller still targets the removed suffixed identity. The new lifecycle regression fails exactly on that mismatch. This remains blocked until survivor identity is stable when a collision set shrinks; please also cover growth and reordering of the collision set.

[tirth8205]

Revalidated head 1abf73b after applying it to exact current main 329dbb91. The submitted and relevant storage/lifecycle suites pass (94 tests), but collision-set transitions still corrupt graph identity.

• Shrink: [A = bare, B = suffix] becoming only B changes B to the bare key and leaves an existing caller's suffix target dangling.
• Growth: starting with unique B at the bare key and then inserting A before it gives A the old bare key and moves B to a suffix. The existing caller silently changes meaning from B to A.

This is not mergeable until identity is independent of collision-set size/order, or incoming edges are migrated atomically. Please add shrink, growth, and reorder lifecycle regressions plus explicit ambiguity behavior.

[SarveshaKumarKS]

Thanks for the precise shrink and growth cases. I reproduced all three collision-set transitions and pushed a new revision.
The replace path now reconciles definitions with their previous identities before deleting the old file rows:

• semantic matches retain their identities when definitions reorder;
• semantically identical definitions use nearest previous source location across harmless line shifts;
• growth preserves the existing definition’s bare identity and gives the new definition a suffix; and
• shrink preserves the surviving definition’s suffix identity, so its existing callers remain valid.

A newly indexed bare edge endpoint resolves to that historical suffix only when it is the sole candidate. If multiple definitions remain possible, the target stays canonical and ambiguous rather than being routed arbitrarily.
Added regressions cover shrink, growth, reorder, callers to every surviving identity, bare resolution after shrink, and explicit ambiguous-target behavior.
Validation on current main (cbb74ed):

• 2,313 passed, 5 skipped, 2 xpassed
• all 75 graph storage tests passed
• Ruff clean
• mypy clean across 70 source files

Updated head: 392651a
Could you please re-run the lifecycle cases against this revision?

[tirth8205]

Thanks for the continued iteration; the shrink, growth, and reorder cases from my last review now pass, and the 8 new regressions genuinely fail without the fix. Two new blockers found at head 392651a. First, a correctness regression on the common no-collision path: index a single unique function render(a) at line 10, then re-index the file with render(a, b) at line 12 (an ordinary edit that changes the signature while lines shift). The node is permanently re-keyed from /repo/app.py::render to /repo/app.py::render:Sda4664594a6e, get_node on the bare name returns None, an existing cross-file caller edge to the bare name dangles, and repeated re-indexes never restore the bare key. Root cause: in _plan_file_identities the fallback denies the bare key because it is in reserved, after the semantic match fails (signature changed) and the exact-location rescue fails (line moved). When the collision set for a base has exactly one old row and one new definition, they should be paired regardless of signature or line changes. Please add a regression for a lone definition whose params and line_start both change, asserting the bare identity is retained. Second, a performance regression: _resolve_stored_identity uses "substr(qualified_name, 1, ?) = ?", which EXPLAIN QUERY PLAN shows as a full index SCAN, and it runs for every edge endpoint that does not match an existing node (unresolved library-call targets are common). Measured 0.86 ms per unresolved edge at 20k nodes; a full build of this repo goes from about 3.8s on main to about 6.4s on this branch. Please replace it with an index-seekable range or LIKE query. One smaller note: symbol names containing colons (Objective-C selectors, which the parser supports via .m files) can false-match the ":S" prefix heuristic; worth a guard or at least a comment.

[SarveshaKumarKS]

Thanks for the precise reproductions. I confirmed all three issues and pushed a revised implementation.
For the no-collision lifecycle, a base with exactly one old row and one new definition is now paired unconditionally. This preserves the bare identity even when both the signature and source location change, so existing incoming callers remain valid across repeated re-indexes.
_resolve_stored_identity() now searches the qualified-name index using the bounded range [base:S, base:T) instead of applying substr() to every row. EXPLAIN QUERY PLAN reports:
SEARCH nodes USING COVERING INDEX idx_nodes_qualified (qualified_name>? AND qualified_name<?)
Collision candidates are also validated against the complete generated suffix format rather than only checking the :S prefix. This prevents colon-bearing symbols such as Objective-C selectors from being mistaken for disambiguated identities.
Added regressions cover:

• a lone definition whose parameters and line both change across repeated re-indexes;
• preservation of its bare identity and existing cross-file caller;
• use of the index-bounded lookup; and
• Objective-C-style colon symbols not being treated as collision suffixes.

Validation on current main (1a010de):

• 2,409 passed, 5 skipped, 2 xpassed
• all 81 graph tests passed
• Ruff clean
• mypy clean across 70 source files

Updated head: 398adda
Could you please revalidate this revision?

[tirth8205]

The lifecycle cases from my last review all pass now, and pytest, ruff, and mypy are clean on current main. Two blockers remain.

First, _plan_file_identities (graph.py:374) queries WHERE file_path = ? with its raw argument while remove_file_data normalizes. incremental.py passes str(full_path), backslash-spelled on Windows, so the plan finds no old rows and reconciliation silently no-ops there. Repro on any OS: store two same-file render defs via the spelling C:\t\target.py, point a caller at the suffixed survivor, shrink to one def, reindex with the same spelling. The survivor re-keys to bare and the caller dangles. Fix is normalize_file_path at the top of the function, plus a regression using a backslash spelling.

Second, the plan is quadratic. The old_by_base loop at graph.py:384 rescans every old row per base: reindexing 1500/3000/6000 unique defs takes 0.30/1.16/4.74s versus about 0.02s on main, and 1000 identical same-name defs cost 1.44s in the pairs loop. Bucket old rows by base in one pass.

Minor: direct upsert_node of a unique def whose line shifted now creates a suffixed duplicate instead of updating in place.

[tirth8205]

Changes required: same-file collision identities still change when the file path uses backslashes. Using GraphStore.store_file_nodes_edges, store two render nodes for C:\t\target.py at lines 10 and 20 with params (a) and (b), then replace that file with only (b) at line 21; the surviving node's qualified name changes from its semantic suffix to the bare name, leaving incoming references behind. Normalise the planning path and preserve the surviving identity before addressing the remaining repeated scans.

[tirth8205]

This no longer merges into staging. Conflicts in code_review_graph/graph.py, tests/test_graph.py.

git fetch origin && git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git push

When resolving:

• Rebase PR fix(graph): preserve same-file nodes on qualified_name collision (#585) #715 onto current origin/staging and resolve conflicts in code_review_graph/graph.py (replace_file_data / replace_file_data_batch and the new _replace_file_data bulk-insert helper from the [Bug]: Hang at "INFO: Spring event resolver: indexed 0 events and emitted 0 CALLS edges" #721 performance work vs.
• After rebase, re-run pytest / ruff / mypy and re-assess edge-case coverage for qualified_name collisions.

A large integration branch landed on staging today, which is why this drifted.

PRs now target staging, not main. Yours was retargeted already, so nothing to do there.

[tirth8205]

This no longer merges into staging. 2 files conflict and the branch is 313 commits behind:

• code_review_graph/graph.py
• tests/test_graph.py

Worth knowing before you resolve:

• code_review_graph/graph.py: staging added the corrupt-database classifiers and a crg_parent_dir SQLite function registered outside the open guard. Keep that guard wrapped around exactly the statements it covers.

git fetch origin
git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git push

I have not reviewed the change itself yet. That comes once it merges and the checks run against the merged state, since staging has moved a long way and the result is what matters.

[tirth8205]

Thank you for this PR. I ran small scripts, the graph tests, and a trial merge.

Before I can merge it, please fix these:

1. _plan_file_identities looks up old rows with the raw file_path (code_review_graph/graph.py:374-379). Windows paths like C:\t\target.py break this. Example: store render(a) and render(b). Add a caller of render(b). Then store only render(b). The caller edge now points to a deleted node. Please call normalize_file_path first.
2. Each new name scans all old rows again (code_review_graph/graph.py:384-390). Updating one file with 6000 functions takes about 4 seconds. On staging it takes about 0.2 seconds. Please group old rows by name in one pass.
3. upsert_node for render at line 10, then at line 11, leaves two rows. Staging keeps one row.

Please update your branch with the latest staging. There are conflicts in code_review_graph/graph.py and tests/test_graph.py. Staging now writes files through _replace_file_data. Please move the fix there.

After you push the fix, I will test it again.

[Copilot]

🟡 Changes recommended

Unresolved critical defects can swap or lose definition identities, misdirect existing callers, and associate test coverage with the wrong duplicate definition.

5 open issues

• Normalize paths when looking up previous identities · New
• Preserve duplicate-definition identities across line shifts · New
• Reuse unmatched identities before allocating collision suffixes · New
• Align TESTED_BY edges with disambiguated CALLS edges · New
• Index previous identities to avoid quadratic matching · New

What changed in this PR

This PR addresses #585 by preserving same-file definitions that share a qualified name instead of silently overwriting nodes in the graph.

Changes:

• Adds semantic collision suffixes and reconciles identities during file replacement.
• Aligns supported edge endpoints with disambiguated nodes while retaining canonical targets for ambiguous calls.
• Adds regression tests for collision preservation, incremental identity stability, and caller/callee resolution.

File	Description
tests/​test_graph.py	Adds collision, reindexing, and edge-resolution regression tests.
code_review_graph/​graph.py	Implements collision-aware node identities and edge alignment.

_{Review effort: Balanced}