Fix node loss during incremental deduplication - #3503
Conversation
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Protects untouched existing nodes from being collapsed with each other during incremental merge (#3477): build_merge now passes the IDs of all existing graph nodes as protected_ids through build into deduplicate_entities. Across every dedup pass — exact file-group, cross-file, fuzzy MinHash/LSH, LLM tiebreak, and final survivor selection — nodes tracked via _get_prot/_union_with_prot will never union two distinct protected components, and mixed protected/incoming groups fold incoming nodes into at most one protected survivor while pure-protected groups are left entirely intact. When protected_ids is unset the original global dedup behaviour is unchanged.
Worth a look
- build_merge now passes raw root instead of derived _eff_root to build —
graphify/build.py:2049· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Falsy existing node IDs are omitted from protected_ids —
graphify/build.py:2034· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- build_merge no longer passes inferred effective root into build —
graphify/build.py:2046· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1379 functions depend on the 161 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_rebuild_code()— 115 callers, 51 callees - new:
build_from_json()— 204 callers, 19 callees - new:
deduplicate_entities()— 76 callers, 24 callees - new:
build_merge()— 76 callers, 14 callees - new:
to_obsidian()— 38 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
build()— 52 callers, 6 callees - new:
_call_claude_cli()— 33 callers, 9 callees - …and 40 more — each is listed as a finding
Verification — 1379 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 973 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
88 of 269 test file(s) selected (33%) via static blast radius.
tests/test_analyze.py— impacttests/test_atomic_canvas_export.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_build.py— impacttests/test_build_merge_dedup_scope.py— impact, changed-testtests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_claude_cli_backend.py— impacttests/test_cli_export.py— impacttests/test_cluster.py— impacttests/test_confidence.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_survivor_richness.py— impacttests/test_definition_file_portability.py— impacttests/test_duplicate_annotation_edges.py— impacttests/test_evidence_binding.py— impacttests/test_export.py— impacttests/test_export_control_characters.py— impacttests/test_export_path_length.py— impacttests/test_extract.py— impacttests/test_falkordb_integration.py— impacttests/test_file_label_disambiguation.py— impacttests/test_global_add_tag_inference.py— impacttests/test_global_graph.py— impacttests/test_go_qualified_resolution.py— impacttests/test_god_nodes_exclude_hubs.py— impacttests/test_hyperedge_member_shapes.py— impacttests/test_hyperedge_roundtrip.py— impacttests/test_hypergraph.py— impacttests/test_image_vision.py— impacttests/test_import_self_loops.py— impacttests/test_issue_3472_source_file_collision.py— impacttests/test_java_type_resolution.py— impacttests/test_languages.py— impacttests/test_llm_backends.py— impacttests/test_llm_parser.py— impacttests/test_llm_parser_reasoning.py— impacttests/test_loose_sibling_import_resolution.py— impacttests/test_manifest_ingest.py— impacttests/test_merge_chunks_validation.py— impacttests/test_merge_graphs_cli.py— impacttests/test_multigraph_diagnostics.py— impact- … and 38 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify build.
The verifier did not have enough to check build, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify build\_merge.
The verifier did not have enough to check build\_merge, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify deduplicate\_entities.
The verifier did not have enough to check deduplicate\_entities, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_llm\_tiebreak.
The verifier did not have enough to check \_llm\_tiebreak, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
· 48 more finding(s) on lines outside this diff (see the check run).
…e-loss' into investigate/3477-build-merge-node-loss
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a protected_ids set that flows from build_merge through build into deduplicate_entities, so during incremental merge untouched existing nodes are never collapsed with one another. Incoming nodes still fold into at most one protected survivor per exact/fuzzy/LLM pass, while protected-only groups are preserved verbatim and cross-protected-component bridging is blocked, tracked via a per-root prot_by_root map alongside union-find. When protected_ids is None, dedup behaves exactly as before with global merging.
Worth a look
- Falsy existing node IDs are not protected during merge dedup —
graphify/build.py:2034· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1384 functions depend on the 173 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_rebuild_code()— 115 callers, 51 callees - new:
build_from_json()— 204 callers, 19 callees - new:
deduplicate_entities()— 76 callers, 24 callees - new:
build_merge()— 76 callers, 14 callees - new:
to_obsidian()— 38 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
build()— 52 callers, 6 callees - new:
_call_claude_cli()— 33 callers, 9 callees - …and 40 more — each is listed as a finding
Verification — 1384 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 978 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
88 of 269 test file(s) selected (33%) via static blast radius.
tests/test_analyze.py— impacttests/test_atomic_canvas_export.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_build.py— impacttests/test_build_merge_dedup_scope.py— impact, changed-testtests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_carried_hyperedge_remap.py— impact, changed-testtests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_claude_cli_backend.py— impacttests/test_cli_export.py— impacttests/test_cluster.py— impacttests/test_confidence.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_survivor_richness.py— impacttests/test_definition_file_portability.py— impacttests/test_duplicate_annotation_edges.py— impacttests/test_evidence_binding.py— impacttests/test_export.py— impacttests/test_export_control_characters.py— impacttests/test_export_path_length.py— impacttests/test_extract.py— impacttests/test_falkordb_integration.py— impacttests/test_file_label_disambiguation.py— impacttests/test_global_add_tag_inference.py— impacttests/test_global_graph.py— impacttests/test_go_qualified_resolution.py— impacttests/test_god_nodes_exclude_hubs.py— impacttests/test_hyperedge_member_shapes.py— impacttests/test_hyperedge_roundtrip.py— impacttests/test_hypergraph.py— impacttests/test_image_vision.py— impacttests/test_import_self_loops.py— impacttests/test_issue_3472_source_file_collision.py— impacttests/test_java_type_resolution.py— impacttests/test_languages.py— impacttests/test_llm_backends.py— impacttests/test_llm_parser.py— impacttests/test_llm_parser_reasoning.py— impacttests/test_loose_sibling_import_resolution.py— impacttests/test_manifest_ingest.py— impacttests/test_merge_chunks_validation.py— impacttests/test_merge_graphs_cli.py— impacttests/test_multigraph_diagnostics.py— impact- … and 38 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify build.
The verifier did not have enough to check build, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify build\_merge.
The verifier did not have enough to check build\_merge, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `graph_path` is annotated `str | Path | None` — outside the synthesizable primitive/collection set
Could not verify: Could not verify deduplicate\_entities.
The verifier did not have enough to check deduplicate\_entities, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_llm\_tiebreak.
The verifier did not have enough to check \_llm\_tiebreak, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
· 48 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.59 — landed on |
2 participants
Summary
Fixes #3477, where
build_merge()could silently drop nodes from files outside the scope of an incremental update.The root cause was global entity deduplication collapsing duplicate nodes belonging exclusively to untouched files.
Changes
build_merge()deduplication.The existing shrink-guard and manifest behavior are intentionally unchanged and remain out of scope.
Tests
237 passed, 1 skippedgit diff --checkclean