Skip to content

fix(build_merge): re-extracted node must not lose to its own stale copy - #2387

Open
kellinkquinn-commits wants to merge 1 commit into
Graphify-Labs:v8from
kellinkquinn-commits:fix/reextract-loses-to-stale-lookalike
Open

fix(build_merge): re-extracted node must not lose to its own stale copy#2387
kellinkquinn-commits wants to merge 1 commit into
Graphify-Labs:v8from
kellinkquinn-commits:fix/reextract-loses-to-stale-lookalike

Conversation

@kellinkquinn-commits

Copy link
Copy Markdown

The bug

A re-extracted node can be silently replaced by the stale version it was meant to correct.

Minimal repro (added as a regression test): a legacy unstamped node for notes.md whose source_location is "L38-41", then a semantic re-extract of the same id with a corrected, longer label. Before this change the graph keeps the old label.

[graphify] note: node 'notes_rank' was extracted twice from 'notes.md' under
different labels — keeping 'Unreconciled: book-to-rank labels off by one between
C-23 and C-27', dropping 'Resolved: the C-23/C-27 book-to-rank off-by-one, closed
by C-45 mapping to Technical Ranks 2/3/4'.

Two things combine

1. The tier guess. _is_ast_tier's shape fallback reads any unstamped item with an L<line> source_location as AST-tier. Semantic extractors do emit that shape for markdown — build.py's own dedup path already guards against it, in a comment that says so explicitly:

Strict _origin check on purpose — NOT _is_ast_tier (#2334) ... Those may carry drifted 'L<line>' source_locations

So a legacy semantic node can be read as AST-tier. The tier-scoped replace from #2333/#2336 then does not drop it on a semantic re-extract, and it survives alongside its own fresh version.

2. The tie-break. With both present, _collision_rank picks the survivor. Definer-ness and source_file tie, so it decides on len(label) and keeps the shorter one. Freshness is not a factor, so a re-extract that expands a claim loses to the text it was correcting.

The failure is silent — node counts unchanged, no dangling edges, and the only signal is a note that reads like routine dedup.

Why not fix the shape test

.md is in the AST extractor registry (extract.py: ".md": extract_markdown), so a doc legitimately carries both tiers, and _write_two_tier_graph in the existing tests encodes exactly that. A semantic node with a line-range location is therefore indistinguishable from an AST one by shape alone. Tightening the heuristic would just move the misclassification.

The fix

Key on identity instead of shape: a base node whose id is re-emitted by a new chunk for the same source_file is that node's own freshly-extracted version, never a coexisting other-tier node. Drop it whatever tier each side is read as.

Tier scoping still protects the other tier's distinct nodes, which is what #2333 was about — an AST heading layer has different ids from the semantic concepts, so it survives untouched. The COEXIST tests are unmodified and still pass.

Testing

Full suite on Windows / py3.11:

failed passed
pristine 00efd6e 49 3681
with this patch 49 3682

Identical failure sets — no regressions; the extra pass is the new test. The 49 are pre-existing environment failures on this platform (skillgen, terraform, ollama_retry_cap, uninstall_scope, watch), unrelated to this path.

Also verified end-to-end against the real 1451-node graph where I hit this: with the patch the re-extract produces the corrected label and zero collision notes, because the replace now fires and the collision never happens.

How I ran into it

A knowledge-base corpus where wiki/ pages carry adjudicated claims. A re-extract of one page was supposed to update "Unreconciled: ..." to "Resolved: ... closed by C-45"; the graph kept the stale text. Nothing downstream flagged it — the node count was right and no edge dangled. In a corpus whose whole point is that adjudicated answers supersede earlier ones, a silent revert to superseded text is about the worst available failure mode, which is why I chased it to the tie-break rather than just working around it locally.

A re-extracted node can be silently replaced by the STALE version it was
meant to correct.

Repro (tests/test_build.py::test_build_merge_reextract_wins_over_legacy_lookalike_node):
a legacy unstamped node for notes.md whose source_location is "L38-41", then a
semantic re-extract of the same id with a corrected, longer label. Before this
change the graph keeps the old label.

Two things combine:

1. _is_ast_tier's shape fallback reads any unstamped item with an 'L<line>'
   source_location as AST-tier. Semantic extractors do emit that shape for
   markdown -- build.py's own dedup path already guards against it ("may carry
   drifted 'L<line>' source_locations") -- so a legacy SEMANTIC node can be read
   as AST-tier. The tier-scoped replace (Graphify-Labs#2333/Graphify-Labs#2336) then does not drop it on a
   semantic re-extract, and it survives alongside its own fresh version.

2. With both present, _collision_rank picks the survivor. Definer-ness and
   source_file tie, so it decides on len(label) and keeps the SHORTER one.
   Freshness is not a factor, so a re-extract that expands a claim loses.

The failure is silent: node counts are unchanged, no edge dangles, and the only
signal is a "extracted twice ... under different labels" note that reads like
routine dedup.

Shape cannot be made reliable here -- '.md' is in the AST extractor registry, so
a doc legitimately carries both tiers, and a semantic node with a line-range
location is indistinguishable from an AST one by shape alone. So key on identity
instead: a base node whose id is re-emitted by a new chunk for the SAME
source_file is that node's own fresh version, never a coexisting other-tier
node. Drop it whatever tier each side is read as.

Tier scoping still protects the other tier's DISTINCT nodes, which is what
Graphify-Labs#2333 was about: the COEXIST tests are unchanged and still pass (an AST heading
layer has different ids from the semantic concepts, so it survives untouched).

Full suite on Windows/py3.11: 49 failed / 3682 passed with the patch versus
49 failed / 3681 passed on a pristine checkout of the same commit -- identical
failure sets, the extra pass being the new test. The 49 are pre-existing
environment failures (skillgen, terraform, ollama, uninstall_scope, watch).
@kellinkquinn-commits
kellinkquinn-commits changed the base branch from main to v8 August 2, 2026 04:47
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