Skip to content

fix(dedup): let the defining file win an ID collision, and warn only about real loss#1852

Open
bchan84x wants to merge 1 commit into
Graphify-Labs:v8from
bchan84x:fix/1851-id-collision-survivor-and-warning
Open

fix(dedup): let the defining file win an ID collision, and warn only about real loss#1852
bchan84x wants to merge 1 commit into
Graphify-Labs:v8from
bchan84x:fix/1851-id-collision-survivor-and-warning

Conversation

@bchan84x

Copy link
Copy Markdown
Contributor

Fixes the ID-collision handling described in #1851.

The problem

A node ID is <slug(source_path)>_<slug(entity)>, so a doc that merely references an entity mints that entity's own ID and collides with the entity's defining node by construction. The pre-dedup pass in deduplicate_entities handled that badly:

  • Survivor was insertion-order. seen_ids kept whichever node arrived first, so chunk order decided whether an entity kept its own attributes or a passing cross-reference's thinner label.
  • The warning asserted a wrong cause. It hardcoded "two files with the same name in different directories" — in the reported case the two files had different names, and nothing was lost (edges are keyed by ID and rewired to the survivor).
  • The genuinely lossy drop was silent. The warning was gated on existing_sf != new_sf, so two same-ID nodes from the same file — the extractor emitting two different labels — were dropped with no output at all.

The change

graphify/dedup.py only.

  1. Definer wins. When several nodes share an ID, the survivor is the one whose source_file is the file the ID encodes; otherwise first-seen, as before. _id_prefixes() accepts any trailing slice of the path, so absolute paths, repo-relative paths, and pre-Silent data loss: doc files with the same name in different directories produce colliding node IDs across extraction chunks #1504 bare-filename-stem IDs all resolve.
  2. Report in proportion to what the drop costs.

Tests

New tests in tests/test_dedup.py: definer-wins in both insertion orders, reference collisions stay silent and keep their edges, absolute source_file paths still resolve, same-file relabels emit the note, plus a unit test for _defines_id. Both existing collision tests (test_cross_chunk_id_collision_emits_warning, test_same_id_same_source_file_no_warning) are untouched and green.

pytest tests/test_dedup.py → 40 passed. ruff check clean.

Notes

  • CHANGELOG.md deliberately not touched — the entries there look maintainer-written at release time. Happy to add one if you'd prefer.
  • Two local failure clusters are environmental, not from this change: test_falkordb_integration.py (no FalkorDB module in my local Redis) and test_skillgen.py (git show of baseline blobs, which a --depth 1 clone lacks; CI uses fetch-depth: 0).

Refs #1851

…about real loss

Node IDs are <source-path>_<entity>, so a doc that merely references an entity
mints the entity's own ID and collides with the entity's node by construction.
The pre-dedup pass kept whichever arrived first, so chunk order decided whether
an entity kept its own attributes or a passing cross-reference's, and the
warning that fired (Graphify-Labs#1504) told the user a same-name-different-directory clash
had lost their data — while the one drop that really is lossy, two labels for
one ID from the same file, was silent because the warning was gated on
source_file differing.

The survivor is now the node whose source_file is the file its ID encodes (any
trailing slice of the path, so absolute, repo-relative, and pre-Graphify-Labs#1504
bare-stem IDs all resolve), falling back to first-seen. Reporting follows what
the drop actually costs: a cross-reference folding into the node it references
loses nothing (edges are keyed by ID and rewire to the survivor) and is silent;
a same-file relabel notes the discarded label; two files that both encode the
ID are distinct entities and still WARNING, now stating the ID-scheme cause
rather than a filename clash.

Refs Graphify-Labs#1851
@rickgonzalez

Copy link
Copy Markdown

A scan of this PR to explain it - using a tool I'm working on. Hope that it helps: https://www.lenzon.ai/viewer/cmrju75nq0011odzhizb1hp1p?voice=google-chirp3

safishamsi added a commit that referenced this pull request Jul 14, 2026
… followup)

Two tweaks on top of @bchan84x's #1852:

1. The definer heuristic decided the survivor pairwise, so when several
   same-file nodes co-defined one ID the surviving label still depended on
   arrival order (the exact 3-node case #1851 reports). Choose the survivor
   by a total order (definer first, then shorter/canonical label, then
   lexically) via a min over _collision_rank, so it is order-independent.
   Direction preserves #1504 (lexically-first source path wins).

2. _defines_id now also recognizes a bare file-level node whose id is
   exactly the slugified path (nid == prefix), not only <path>_<entity>.

Adds an order-independence test over all 6 permutations of definer +
same-file relabel + cross-file reference, and a bare-file-node test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants