Repository navigation
fix(build): excuse an exact duplicate dedup fold from the shrink guard - #4953
ayushcodes10 wants to merge 2 commits into
Conversation
build merges a saved semantic node under a different id than it was saved under whenever this run's extraction re emits a same label, same file near duplicate that the exact duplicate dedup pass then collapses onto a shorter or richer survivor id. The saved id still names a real concept, just one that now lives under a different id, but the shrink guard's lost saved node check had no way to tell that apart from a node genuinely dropped, since dedup never exposed which id it folded into which. A healthy merge was refused outright, and retrying only re extracted the same unchanged documents and refused again. Added an optional node remap parameter to the entity deduplication pass, filled in place the same way the existing hyperedge parameter already is so every two tuple caller stays unaffected. build stashes the filled map on the graph next to the existing dedup collapsed count, and the shrink guard's lost saved node check now accepts it and excuses a saved id whose remap target survived in the new graph, recording the fold the same way an AST twin fold already is so a caller's own node count math stays consistent either way. The CLI's own early read of this accounting puts the map back before its own call into the guard, since that path pops the accounting before the guard would otherwise see it.
Covers the lost saved node check directly, both with a remap whose target survived and one whose target did not, plus a control confirming the existing no remap call shape is unaffected. Also covers the full build and write path end to end with the exact duplicate dedup scenario reported: a saved concept re emitted under a second, shorter id this run, collapsed onto that shorter id by the exact duplicate pass, writing successfully instead of being refused.
github-actions
Bot
added
the
contributor
|
Thanks for the pull request, @ayushcodes10. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
![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 1 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 6 changed function(s) (2 sampled, not proven, 3 not verified).
Formal verification. 1 change(s) alter behavior, breaking input(s) attached. PR-changed functions: 3/6 verified (0 proven, 2 may-equivalent, 1 distinguished) · 3 not verified (3 vacuous).
Behavior changes: take\_shrink\_accounting changes behavior, here is the input that shows it.
The verifier found a concrete input on which take\_shrink\_accounting behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"''"\}, the old code produced \(0, 0\) but the new code produces \(0, 0, \{\}\) (outputs first differ at character 6). Paste that input straight into a regression test.
Not verified on this run: build (vacuous: never exercised), dispatch\_command (vacuous: never exercised), deduplicate\_entities (vacuous: never exercised).
Graphify review — findings
Fixes the false "lost saved nodes" refusal in to_json when exact-duplicate dedup folds a saved node into a same-label, same-file survivor under a different id. deduplicate_entities now fills a caller-supplied node_remap, build stashes it on the graph, and _lost_saved_semantic_nodes excuses a saved id only when its mapped survivor is actually present in the new graph, so a stale remap still reports a genuine loss. take_shrink_accounting now returns a third value, the remap, which changes the contract for every caller that unpacked two. The lookup is a single hop, so a survivor that was itself refolded later still counts as lost.
Worth a look
- take_shrink_accounting return arity changed from 2 to 3 —
graphify/build.py:2209· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2343 functions depend on the 374 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
build_from_json()— 300 callers, 23 callees - worse:
to_json()— 90 callers, 11 callees - worse:
build()— 87 callers, 6 callees - worse:
_lost_saved_semantic_nodes()— 20 callers, 6 callees
Verification — 2343 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: 2046 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
166 of 489 test file(s) selected (34%) via static blast radius.
tests/test_4874_jvm_resolution.py— impacttests/test_affected_cli.py— impacttests/test_affected_path_file_node.py— impacttests/test_affected_seed_reference_forms.py— impacttests/test_agents_platform.py— impacttests/test_alias_folding.py— impacttests/test_analyze.py— impacttests/test_atomic_canvas_export.py— impacttests/test_atomic_writes.py— impacttests/test_backend_opt_in.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_benchmark_real_query.py— impacttests/test_build.py— impacttests/test_build_located_semantic_identity.py— impacttests/test_build_merge_dedup_scope.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_c_include_resolution.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_cloud_cta.py— impacttests/test_cluster.py— impacttests/test_codebuddy.py— impacttests/test_community_labels_skill.py— impacttests/test_compact_graph_json.py— impacttests/test_confidence.py— impacttests/test_confidence_score_output.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_cross_repo_external_call_guards.py— impacttests/test_csharp_method_owner_labels.py— impacttests/test_csharp_object_creation.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_shrink_refuses_force_write.py— impacttests/test_dedup_survivor_richness.py— impacttests/test_definition_file_portability.py— impacttests/test_devin.py— impacttests/test_duplicate_annotation_edges.py— impacttests/test_edge_also_seen.py— impacttests/test_evidence_binding.py— impacttests/test_explain_cli.py— impacttests/test_explain_same_file_tie.py— impacttests/test_export.py— impacttests/test_export_control_characters.py— impact- … and 116 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
Behavior changes: take\_shrink\_accounting changes behavior, here is the input that shows it.
The verifier found a concrete input on which take\_shrink\_accounting behaves differently before and after the change. If that change is intended, ship it; if not, this is your bug.
Guarantee: This difference was REPRODUCED, the verifier actually ran both versions on that input and saw them disagree. It is real, not an artifact.
Evidence: On input \{"G":"''"\}, the old code produced \(0, 0\) but the new code produces \(0, 0, \{\}\) (outputs first differ at character 6). Paste that input straight into a regression test.
No difference found (not proven): No behavior difference found in \_lost\_saved\_semantic\_nodes (not a proof).
The verifier ran both versions of \_lost\_saved\_semantic\_nodes on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
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: no capturable inputs from the test suite; property tier: 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 dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: no capturable inputs from the test suite; property tier: not verifiable: all 40 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
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: no capturable inputs from the test suite; property tier: not verifiable: all 11 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)
No difference found (not proven): No behavior difference found in to\_json (not a proof).
The verifier ran both versions of to\_json on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 4 more finding(s) on lines outside this diff (see the check run).
|
Closing in favour of #4952, which shipped in 0.9.101. Same core idea (thread the dedup merge map into the write guard). #4952 also fixes the net-count check: when N saved ids fold into one new (non-saved) survivor, the shrink is the expected N-1, which this PR still refused. Thanks for the fix on #4940. |
Fixes #4940.
Bug
During an incremental semantic
--update,build_merge(viabuild) can replace a re-extracted file's nodes and then rundeduplicate_entities's exact-duplicate pass, which collapses a same-label, same-file near-duplicate pair onto one survivor id. When the saved node's id is the one folded away (not the survivor),to_json's#479shrink guard — specifically_lost_saved_semantic_nodes— has no way to know the concept actually survived under a different id. It reports the saved id as missing and refuses the write outright, even though nothing was lost. Retrying re-extracts the same unchanged documents and refuses identically.The accounting for this already half-exists:
build()computes_dedup_collapsed(the total node-count drop from dedup) and stashes it onG.graph, andto_jsoncallstake_shrink_accounting(G)to pop it — but the call site inexport.pydiscards the return value entirely (confirmed:take_shrink_accountinghas exactly one other caller in the whole codebase,cli.py, and neither ever consumes the dict needed to exclude a specific id). The count was never wired into the_lost_saved_semantic_nodescheck that actually produces the refusal.No existing issue or PR covers this; found via the issue tracker (#4940, well-documented with an exact repro shape). Checked PR #3623 (open, "excuse dedup-explained node drops from the #479 shrink guard") — it addresses a different mechanism (
build_from_json's #1145 ghost-merge count,_ghost_dedup_count), notdeduplicate_entities's exact-duplicateremap. No overlap.Fix
dedup.py:deduplicate_entitiesgains an optionalnode_remap: dict[str, str] | Noneout-param, filled in place with every merged id's survivor id — mirroring the existinghyperedgesout-param precedent exactly ("Optional and mutating rather than returned so existing two-tuple callers are unaffected").build.py:build()passes a fresh dict asnode_remap=and stashes it onG.graph["_dedup_node_remap"]next to the existing_dedup_collapsed.take_shrink_accountingnow pops and returns it too (3-tuple; its one other caller,cli.py, is updated to match)._lost_saved_semantic_nodesgains adedup_remapparameter: a saved id whose remap target is present in the new graph is excused (and recorded intofolded, so downstream node-count math stays consistent), checked before the existing AST-twin/file-twin fold checks.export.py:to_jsonnow captures the remap fromtake_shrink_accountingand passes it through to_lost_saved_semantic_nodes.cli.py: the extract CLI's own earlytake_shrink_accounting(G)call pops this map beforeto_jsonruns; whento_json's own internal guard will still execute (force=False, the partial-extraction path), the map is put back ontoG.graphfirst soto_jsoncan retrieve it again.Checked
watch.py's_rebuild_code/_check_shrink(the--updateincremental path) separately — that path writes the raw merged extraction directly, never throughbuild_from_json/deduplicate_entities, so this specific gap doesn't apply there.Tests
Added to
tests/test_lost_saved_semantic_nodes.py:test_dedup_remap_target_present_is_not_lost/test_dedup_remap_target_absent_is_still_lost— the_lost_saved_semantic_nodesunit-level behavior.test_no_dedup_remap_given_is_unaffected— control confirming the existing no-remap call shape (every other caller) is unchanged.test_to_json_writes_after_exact_duplicate_dedup— end-to-end withbuild()+to_json(), reproducing the issue's exact shape (a saved concept re-extracted alongside a same-label, same-file near-duplicate under a shorter id, collapsed by the exact-duplicate pass).Verification
Deduplicated 1 node(s) (1 exact)./Refusing to overwrite — saved semantic node(s) are missing.../graph.json written: False), then confirmed the fix resolves it (graph.json written: True).TypeErrorfrom the not-yet-existingdedup_remapparameter) and pass against the fix; restored byte-identical afterward.tests/test_dedup.py(153),tests/test_merge_sweeps_reextract_orphans.py(the only othertake_shrink_accountingcaller, uses index-based unpacking so the 3-tuple doesn't break it), and the CLI-level shrink-guard tests (test_incomplete_build_guard.py,test_hollow_chunks_arm_shrink_guard.py,test_dedup_shrink_refuses_force_write.py) — all pass.3 failed, 9314 passed, 83 skipped, 1 xfailed— the 3 failures are the pre-existing flakes intest_erlang_extractor.py,test_r_extractor.py, and the skill backtick test, unrelated to this change. (An earlier run under host memory pressure showed spurious unrelated failures — a clean rerun after the pressure cleared confirmed this baseline.)python3 -m tools.skillgen --check:check OK: 134 artifact(s) match committed output and expected/.