Skip to content

fix(indexer): disambiguate node identity so colliding symbols stop merging - #1630

Open
Falehaqazi wants to merge 1 commit into
CodeGraphContext:mainfrom
Falehaqazi:fix/1393-node-identity-collision
Open

fix(indexer): disambiguate node identity so colliding symbols stop merging#1630
Falehaqazi wants to merge 1 commit into
CodeGraphContext:mainfrom
Falehaqazi:fix/1393-node-identity-collision

Conversation

@Falehaqazi

Copy link
Copy Markdown

The bug

writer.py merged 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 following SET n += row overwrote the first one's args, class_context, end_line and cyclomatic_complexity with the last one's.

Reproduction

I re-ran the shipped CSS parser's tree-sitter query over this repo's CSS files:

css files scanned:      5
total parsed selectors: 279
distinct merge keys:    269
lost to collision:      10   (9 colliding keys)

Concrete cases, both in files already in the repo:

file line symbol count
tests/fixtures/sample_projects/sample_project_misc/tables.css 44 tfoot 2
docs/docs/stylesheets/redwood.css 558 code 3
docs/docs/stylesheets/redwood.css 493 tbody, td, tr 2 each

tables.css:44 is tfoot th, tfoot td { } — one grouped rule emitting tfoot twice on one line.

Every collision had distinct start columns, which is worth noting for a future refinement (see "Alternatives" below).

Why not end_line or class_context

The 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 neither end_line nor class_context at all, so this is null for 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.py carried Neo4j IS UNIQUE constraints on the same three properties for Function, Class, Trait, Interface, Macro, Variable, Struct, Enum, Union, Record and Property. 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 CONSTRAINT is deliberately skipped (per the comment about the EnforceUniqueEntity null-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

  • New pure helper _assign_occurrence_indices() returning a per-item ordinal plus a collision report.
  • occurrence_index added to the merge and match keys.
  • Collisions logged via warning_logger, which also covers the issue's "at minimum, detect and log" fallback.
  • The ordinal is threaded into the parameter dedupe key and the HAS_PARAMETER match, 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

  • The eleven legacy constraints are dropped and recreated as <label>_identity with the four-property key. Renaming keeps the DROP a no-op on later startups — recreating a constraint on every boot would rebuild the index on a large graph.
  • FalkorDB's supporting composite indexes include occurrence_index.
  • Annotation is untouched: it is not in item_mappings, so its nodes never receive an occurrence_index.

schema_contract.pyFUNCTION_MERGE_KEYS / CLASS_MERGE_KEYS updated, with the existing test adjusted.

Why this is safe

  • occurrence_index is 0 unless two symbols in the same file actually collide, so node identity is byte-identical for the overwhelming majority of symbols.
  • writer.py is the only place these nodes are created — every other reference in src/ is a read-path MATCH.
  • Read paths that match on (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.
  • Ordinals cannot drift into stale nodes: update_file_in_graph calls delete_file_from_graph before re-adding.
  • Parse order is deterministic, so re-indexing an unchanged file reproduces the same ordinals.

Verification

After the fix, the same CSS corpus:

total parsed selectors: 279
distinct merge keys:    279
lost:                   0
collisions logged:      9

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, missing name/line_number, and a model of MERGE + SET n += row showing 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 column would 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 — but occurrence_index can 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

  • Two pre-existing ruff findings in writer.py (F401 sanitize_props, F841 batch_size) are untouched — they are outside this change and not in the CI lint file list.
  • Neo4j applies a property-uniqueness constraint only to nodes that have all the named properties, so any node created without occurrence_index simply falls outside the constraint rather than erroring.

Fixes #1393

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Author

Small correction: there were twelve UNIQUE constraints on (name, path, line_number), not eleven. I migrated eleven and deliberately left Annotation — it isn't in item_mappings, so its nodes never receive an occurrence_index and its existing three-property constraint remains correct.

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

Labels

None yet

Projects

Status: Backlog tasks

Development

Successfully merging this pull request may close these issues.

bug(indexer): Node identity (name, path, line_number) is not unique; symbols merge and properties are clobbered

1 participant