fix(indexer): disambiguate node identity so colliding symbols stop merging - #1630
Open
Falehaqazi wants to merge 1 commit into
Open
fix(indexer): disambiguate node identity so colliding symbols stop merging#1630Falehaqazi wants to merge 1 commit into
Falehaqazi wants to merge 1 commit into
Conversation
|
@Falehaqazi is attempting to deploy a commit to the shashankss1205's projects Team on Vercel. A member of the Team first needs to authorize it. |
Falehaqazi
force-pushed
the
fix/1393-node-identity-collision
branch
from
August 14, 2026 19:43
de0b44a to
e3b8687
Compare
Author
|
Small correction: there were twelve UNIQUE constraints on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
writer.pymerged every code entity on(name, path, line_number). That triple is not unique, so two distinct symbols in one file sharing a name and a line collapsed onto a single node, and the followingSET n += rowoverwrote the first one'sargs,class_context,end_lineandcyclomatic_complexitywith the last one's.Reproduction
I re-ran the shipped CSS parser's tree-sitter query over this repo's CSS files:
Concrete cases, both in files already in the repo:
tests/fixtures/sample_projects/sample_project_misc/tables.csstfootdocs/docs/stylesheets/redwood.csscodedocs/docs/stylesheets/redwood.csstbody,td,trtables.css:44istfoot th, tfoot td { }— one grouped rule emittingtfoottwice on one line.Every collision had distinct start columns, which is worth noting for a future refinement (see "Alternatives" below).
Why not
end_lineorclass_contextThe issue suggests
end_line,class_context, or a per-file ordinal. Only the ordinal covers both reported sources:end_line— a grouped CSS rule's selectors share a start and end line, so this does not separate them.class_context— the CSS parser (tools/languages/css.py:83) emits neitherend_linenorclass_contextat all, so this isnullfor every CSS record.The per-file ordinal works for both CSS and minified JS, and needs no parser changes.
The part the issue does not mention
schema.pycarried Neo4jIS UNIQUEconstraints on the same three properties forFunction,Class,Trait,Interface,Macro,Variable,Struct,Enum,Union,RecordandProperty. The database was preventing the fix — a writer-only patch throws a constraint violation on Neo4j the moment it tries to create the second colliding node.The audit in #1393 ran on FalkorDB, where
CREATE CONSTRAINTis deliberately skipped (per the comment about theEnforceUniqueEntitynull-pointer crash). That is why the bug presented as silent merging rather than an error, and why a fix that looks correct on FalkorDB would break Neo4j.Changes
persistence/writer.py_assign_occurrence_indices()returning a per-item ordinal plus a collision report.occurrence_indexadded to the merge and match keys.warning_logger, which also covers the issue's "at minimum, detect and log" fallback.HAS_PARAMETERmatch, so two colliding functions that share an argument name each keep their own parameters.{"Module", "DbTable", "ExternalClass"}hoisted to_NAME_ONLY_MERGE_LABELS; those are global one-node-per-name labels and keep their existing identity.schema.py<label>_identitywith the four-property key. Renaming keeps theDROPa no-op on later startups — recreating a constraint on every boot would rebuild the index on a large graph.occurrence_index.Annotationis untouched: it is not initem_mappings, so its nodes never receive anoccurrence_index.schema_contract.py—FUNCTION_MERGE_KEYS/CLASS_MERGE_KEYSupdated, with the existing test adjusted.Why this is safe
occurrence_indexis0unless two symbols in the same file actually collide, so node identity is byte-identical for the overwhelming majority of symbols.writer.pyis the only place these nodes are created — every other reference insrc/is a read-pathMATCH.(name, path, line_number)are unaffected for non-colliding symbols. For the genuinely ambiguous keys they now return both symbols instead of one clobbered node, which is the correct answer.update_file_in_graphcallsdelete_file_from_graphbefore re-adding.Verification
After the fix, the same CSS corpus:
Tests:
tests/unit/tools/test_issue_1393_node_identity.py, 16 tests covering the CSS grouped-selector case, the minified-JS case, three-way collisions, determinism, missingname/line_number, and a model ofMERGE+SET n += rowshowing the old key losing a symbol and the new key preserving both with their own properties.Full unit suite: 1241 passed, 19 skipped, 0 failed.
Alternatives considered
Start column as the disambiguator. Every collision observed had a distinct column, so
columnwould be a stable, semantically meaningful key rather than an ordinal, and would let read-path matches address a specific symbol. It would require adding column output to every language extractor, so it is out of scope here — butoccurrence_indexcan be swapped for it later without another identity migration if that is the direction you prefer.Deduplicating at parse time. Not viable: these are genuinely distinct symbols, not duplicate records.
Notes for review
writer.py(F401 sanitize_props,F841 batch_size) are untouched — they are outside this change and not in the CI lint file list.occurrence_indexsimply falls outside the constraint rather than erroring.Fixes #1393