fix(graph): refresh preserved dependent nodes on increment (PR-P4)#343
Merged
Conversation
PR-P2 replaced the per-row `_MERGE_SYMBOL` upsert in `_write_nodes_impl` with a skip-if-exists filter, which dropped the property refresh on preserved dependent type nodes. A dependent's `role`/`capabilities` depend on project-wide inputs (meta-annotation chain, brownfield overrides) and can shift without its own source changing, so the incremental graph diverged from a full rebuild — a preserved dependent whose role lever changed stayed stale until the next full rebuild. The `_delete_file_scope` contract (issue #305) relied on that re-MERGE to refresh preserved dependents in place. Found by fanning out reviewer subagents over the landed init/increment-perf plan and adjudicating a B-vs-D conflict empirically (the "benign" verdict came from a reviewer that couldn't run ladybug; the "bug" verdict reproduced). Changes: - Mark `TypeIndexEntry`/`MemberEntry` `loaded_from_db` when `_load_existing_{types,members}` creates them, so DB-loaded stubs (placeholder decls) are distinguishable from freshly-parsed scoped/dependent decls. - After the bulk COPY of new nodes, re-SET every mutable Symbol field on preserved, non-stub, type-kind nodes (`_SET_SYMBOL_BY_ID`), restoring the upsert the design relied on. Stubs are skipped (their stored values are authoritative); non-type kinds carry no mutable role/capabilities; the full path is unaffected (empty DB → no existing nodes). - `_populate_declares_rows` skips loaded members: their DECLARES edges already persist and re-emitting duplicated them (REL tables carry no PK). This second incremental≠full divergence surfaced once the equivalence test was fixed. - Delete dead `_existing_symbol_ids` (zero callers); correct the Route-MERGE loop comment (it iterates all routes, not phantoms). Tests: - `test_incremental_bulk_write_equivalent_to_full_rebuild` now asserts real incremental≡full equivalence (node count, per-type edge counts, type roles) instead of the prior tautology (`set(keys)==set(keys)` + `count>0`). - New `test_incremental_refreshes_dependent_role_on_meta_chain_change` guards the exact regression. - Rename `test_bulk_write_edges_match_per_row_baseline` → `test_bank_chat_bulk_build_matches_committed_baseline`: the per-row path is gone, so the fixture (bulk-generated) is a drift anchor, not a per-row proof. Known follow-ups (out of scope): converting the remaining per-row Route MERGE to bulk; OVERRIDES may have an analogous duplication for cross-file pairs; annotation-definition edits don't pull annotation-users into incremental scope (a separate, pre-existing scope gap, not the upsert regression fixed here). Co-Authored-By: Claude <noreply@anthropic.com>
HumanBean17
added a commit
that referenced
this pull request
Jun 22, 2026
) All of the init/increment-perf work has landed — the original plan (PR-P1..P3: #340 cached ignore, #341 _write_edges bulk, #342 nodes/routes bulk) and the post-review follow-ups (PR-P4 #343 dependent refresh + DECLARES dedup, PR-P5 #344 annotation-scope fix + route bulk + overrides invariant), plus its proposal (#338). Relocate the plan, agent-prompts, and proposal from active/ to completed/, matching the Ladybug/INDEX-OUTPUT close-out convention (pure rename, no content edits). Co-authored-by: Claude <noreply@anthropic.com>
Merged
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.
Summary
Follow-up to the init/increment-perf plan (PR-P1/P2/P3, #340–#342). A review fan-out over the landed code found that PR-P2 introduced a latent data-correctness regression: converting
_write_nodes_implto bulkCOPY FROMreplaced the per-row_MERGE_SYMBOLupsert with a skip-if-exists filter, which silently dropped the property refresh on preserved dependent type nodes.A preserved dependent's
role/capabilitiesdepend on project-wide inputs (meta-annotation chain, brownfield overrides) and can shift without the dependent's own source changing. The_delete_file_scopecontract (issue #305) deliberately preserves dependent nodes and relied on that re-MERGE to refresh them in place. With the skip filter, the role was recomputed and then discarded — so the incremental graph diverged from a full rebuild until the next full rebuild. Roles drive semantic-search ranking, so this silently degraded results. No crash, no node/edge loss, self-heals on full rebuild. CI green — the two tests meant to guard incremental≡full equivalence were broken.This PR fixes the regression and the tests that let it ship.
How it was found
Fanned out four reviewer subagents over the landed plan. Two analyzed the same code path and disagreed (one said HIGH bug with a repro, one said benign/unreachable). Adjudicated empirically: the "benign" verdict came from a reviewer that couldn't run ladybug in its env (bare
python— the system-shadows-venv trap) and over-reasoned; the "bug" verdict was confirmed with a deterministic repro:Changes
build_ast_graph.pyTypeIndexEntry/MemberEntryget aloaded_from_dbflag set by_load_existing_{types,members}, distinguishing DB-loaded stubs (placeholder decls used only for cross-file resolution) from freshly-parsed scoped/dependent decls._write_nodes_impl: after the bulk COPY of new nodes, re-SET every mutable Symbol field on preserved, non-stub, type-kind nodes (_SET_SYMBOL_BY_ID), restoring the upsert. Stubs are skipped (their stored values are authoritative); non-type kinds carry no mutable role/capabilities; the full path is unaffected (empty DB → no existing nodes → no SETs)._populate_declares_rowsskips loaded members: their DECLARES edges already persist and re-emitting duplicated them (REL tables carry no PK). This second incremental≠full divergence surfaced once the equivalence test was fixed (incremental=7 vs full=5 DECLARES)._existing_symbol_ids(zero callers); correct the misleading Route-MERGE loop comment (it iterates all routes, not phantoms).Tests
test_incremental_bulk_write_equivalent_to_full_rebuildnow asserts real incremental≡full equivalence (node count, per-type edge counts, type roles) instead of the prior tautology (set(edge_type_keys) == set(edge_type_keys)+count > 0, which never failed because every rel type yields a count row even at 0).test_incremental_refreshes_dependent_role_on_meta_chain_changeguards the exact regression.test_bulk_write_edges_match_per_row_baseline→test_bank_chat_bulk_build_matches_committed_baseline: the per-row path is gone, so the bulk-generated fixture is a drift anchor, not a per-row equivalence proof.Validation
.venv/bin/ruff check .— clean.venv/bin/python -m pytest tests -q— 848 passed, 14 skipped (HEAVY-gated)meta: ontology_version 17 (unchanged), parse_errors 0, all counters present/tmp/repro_role_staleness2.py): increment now yields SERVICE = full rebuild (no divergence)No ontology bump, no schema change, no re-index required, no public-surface change.
Known follow-ups (out of scope)
🤖 Generated with Claude Code