fix(build_merge): re-extracted node must not lose to its own stale copy - #2387
Open
kellinkquinn-commits wants to merge 1 commit into
Open
Conversation
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).
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
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.mdwhosesource_locationis"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. The tier guess.
_is_ast_tier's shape fallback reads any unstamped item with anL<line>source_locationas 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: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_rankpicks the survivor. Definer-ness andsource_filetie, so it decides onlen(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
.mdis in the AST extractor registry (extract.py:".md": extract_markdown), so a doc legitimately carries both tiers, and_write_two_tier_graphin 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_fileis 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:
00efd6eIdentical 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.