Repository navigation
fix(build): don't treat intentional ghost folds as drops in build_merge (#4955) - #4957
Deepjyoti-Sarmah wants to merge 4 commits into
Conversation
…raphify-Labs#4955) Clean graphs, semantic-vs-semantic duplicates under dedup=False, ghosts with their own rationale (Graphify-Labs#4896), ambiguous file labels and prune_sources are all left alone by build_merge. Pinned first so the guard change that follows cannot loosen them.
…abs#4955) build_from_json folds a semantic ghost into its AST twin (Graphify-Labs#1145), a document node that names a file into that file (Graphify-Labs#3344), and a bare method into its AST method (Graphify-Labs#3705). Entity dedup already reports its folds as {merged-away id: survivor} on G.graph (Graphify-Labs#4940) so to_json's shrink guard can excuse them; the ghost folds were never reported. to_json re-derived the first two from labels and missed the third, so a saved method ghost made it refuse to write. Record the ghost folds on the same channel, and merge into it in build() instead of overwriting it. to_json already pops the key before it serialises the graph.
…ge (Graphify-Labs#4955) The Graphify-Labs#479 guard in build_merge excused one legitimate collapse, the Graphify-Labs#1799 doc twin. The Graphify-Labs#1145, Graphify-Labs#3344 and Graphify-Labs#3705 folds that build_from_json performs on purpose looked like unexplained losses, so an on-disk graph holding such a twin could not be updated through build_merge(dedup=False): the saved nodes are stamped by shape on load, which makes the AST twin the canonical node, and the fold runs even under dedup=False. Excuse an id the fold mapped onto a survivor that is still in the graph. A survivor that is gone excuses nothing.
…r ghost folds (Graphify-Labs#4955) An edge into a folded ghost lands on the survivor, an ordinary update that brings a new AST node next to a saved semantic twin no longer raises, and build_merge(dedup=False) followed by to_json round-trips each of the three folds.
github-actions
Bot
added
the
contributor
|
Thanks for the pull request, @Deepjyoti-Sarmah. 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.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 3 changed function(s) (3 not verified).
Formal verification. PR-changed functions: 0/3 verified (0 proven, 0 may-equivalent, 0 distinguished) · 3 not verified (3 vacuous).
Not verified on this run: build (vacuous: never exercised), build\_from\_json (vacuous: never exercised), build\_merge (vacuous: never exercised).
Graphify review — findings
Fixes false "lost node" refusals when build_from_json intentionally folds ghost nodes (#1145, #3344, #3705): it now records each fold as a {merged-away id: survivor id} entry in G.graph["_dedup_remap"], the same channel entity dedup uses, and build merges into that map instead of overwriting it. In build_merge with dedup=False, the #479 check treats a saved node as present when its fold survivor is still in the graph, while a fold whose survivor is gone still counts as a loss. Because the map is now written by build_from_json itself, it sits in G.graph for every direct caller, and only to_json strips it.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2062 functions depend on the 147 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
build_merge()— 109 callers, 16 callees - worse:
to_json()— 100 callers, 13 callees
Verification — 2062 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: 1408 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
146 of 502 test file(s) selected (29%) 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_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_ghost_fold_4955.py— impact, changed-testtests/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_cluster.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_duplicate_annotation_edges.py— impacttests/test_edge_also_seen.py— impacttests/test_evidence_binding.py— impacttests/test_export.py— impacttests/test_export_control_characters.py— impacttests/test_export_direction.py— impacttests/test_export_idempotent_writes.py— impacttests/test_export_path_length.py— impacttests/test_ext_tagged_id_clash.py— impacttests/test_external_stub_endpoints.py— impact- … and 96 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: 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 build\_from\_json.
The verifier did not have enough to check build\_from\_json, 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
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: no capturable inputs from the test suite; property tier: not verifiable: all 224 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)
· 2 more finding(s) on lines outside this diff (see the check run).
What does this PR do?
Fixes #4955.
build_from_jsonfolds three kinds of duplicate on purpose: a semantic ghost into its AST twin (#1145), a document node whose label names a file into that file (#3344), and a bare method into its AST method (#3705). The #479 guard inbuild_mergeexcuses only the #1799 doc twin, so it read these folds as nodes lost from sources nobody re-extracted or pruned. A graph that holds such a twin could not be updated throughbuild_merge(dedup=False), even with an empty chunk, and the error pointed atprune_sources, which was not what the caller wanted.dedup=Falsedoes not stop the fold. Nodes loaded from disk are stamped_originby shape, which makes the AST twin the canonical node, andtest_ast_twin_remains_canonical_despite_semantic_location_driftpins that an AST twin wins even withdedup=False. So the fold is intended, and the guard has to know about it.This change reports the ghost folds on the
{merged-away id: survivor id}channel entity dedup already uses (#4940), and lets thebuild_mergeguard read it:build_from_json Pass 2 / Pass 2b / method branch -> _ghost_remap + G.graph["_dedup_remap"].update(_ghost_remap) build - G.graph["_dedup_remap"] = _dedup_remap + G.graph.setdefault("_dedup_remap", {}).update(_dedup_remap) build_merge (#479 guard) _in_new_graph(saved node) + folded into a survivor that is still in G -> not lost to_json take_dedup_remap(G) # already excuses it, and keeps it out of graph.jsonA survivor that is itself gone excuses nothing.
to_jsonhas its own guard. It re-derives the #1145 and #3344 folds from labels but never learned the #3705 method fold, so with only thebuild_mergechange a saved method ghost would pass the merge and then be refused on write. Feeding it the real fold map fixes that, and removes the need for it to guess.Type of change
Evidence
Before: on
upstream/v8(8e6a6d21), the issue's two cases with an empty chunk anddedup=False.An ordinary update raises too: a re-extracted file brings a new AST node while a saved semantic twin of it exists.
After:
The full suite ran serially with
PYTHONPATH=/var/tmp/wt-4955-A, so subprocess CLI tests imported this worktree and not the installed package.Verification & Invariants
The invariant: the guard may refuse a write, but only when content is gone, never because
build_from_jsonfolded a node into a twin that is still there.The tests assert on what
build_mergereturns and what reachesgraph.json, so they hold for any fix of this bug. They pin:build_merge(dedup=False)followed byto_jsonround-trips all three folds.dedup=False, a ghost with its own rationale (watch liveness check resolves relative source identities against process CWD instead of watch root #4896), an ambiguous file label andprune_sourcesbehave as before. These pass before and after.graph.json.I tried a second fix against the same shapes: under
dedup=False, skip Pass 2 and 2b for nodes loaded from disk. It leaves the nodes at 2 → 2, as 0.8.31 did, but it breakstest_ast_twin_remains_canonical_despite_semantic_location_drift, which pins the opposite on purpose. So I kept the fold and fixed the guard.Known limitations:
dedup=Falsestill merges an AST twin with its ghost, as the existing test pins. Nothing here changes node counts, only whether the guard refuses.watch._check_shrinkcalls_lost_saved_semantic_nodeswithout the fold map. Agraphify updatethrough it could still refuse a extraction-spec method IDs omit the class segment the AST extractor emits, so method nodes never dedupe #3705 method fold. I did not reproduce that, and it needs a separate change.How was this tested?
Branch
fix/4955-merge-guard-ghost-foldsis based onupstream/v8(8e6a6d21). It has four commits:Merge Danger
Door: Two-way.
When
build_from_jsonfolds nothing the default path is unchanged. No cache schema bump and no graph format change. Reverting restores the old guard.Blast Radius: Write guard.
Every graph written after a merge passes the
build_mergeguard andto_json, so a mistake here either blocks a good write or lets a bad one through. A fold is excused only when its survivor is in the new graph, and the control tests pin the refusals. After merge, a graph holding these twins can be updated throughbuild_merge(dedup=False)instead of being refused with no way out. Extraction, ignore rules and secret filtering are unchanged.Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. No source fragments changed.