Fix: save_manifest can erase a deleted file's row before it is ever reported (#3426) - #3619
ayushcodes10 wants to merge 6 commits into
Conversation
Fixes issue 3426. save_manifest pruned a row for a file no longer on disk on its own, unconditionally, on the theory that a genuinely deleted file's row is dead weight. But detect_incremental is what reports a deletion to callers through deleted_files, and it runs before this function is asked to save again. Pruning the row here unconditionally could erase it before a caller that saves without also pruning the graph, a scan only run, an interrupted pipeline, ever got to act on that report, making a genuine deletion permanently unreportable from the very next run onward. When the caller supplies the full scan corpus, the existing excluded but alive check already prunes an in root row the scan no longer covers, which a deleted file always satisfies on path alone, regardless of whether it still exists. A full scan caller still cleans the row up, just sequenced after detect_incremental has had the chance to report it on that same scan, matching the ordinary update path exactly. Only a partial or subset caller with no scan corpus now leaves a dead row in place; the next full scan reconciles it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A save with no scan corpus must leave a deleted file's row in place so the deletion stays reportable on the next incremental pass, instead of silently disappearing after only one report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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 2 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Removes the on-disk existence check from save_manifest so a subset/partial save no longer prunes a deleted file's manifest row, keeping the deletion reportable through detect_incremental across runs. Full-scan callers (those passing scan_corpus) still drop the dead row via the existing in-root/not-in-scan check, now sequenced after the deletion gets a chance to be reported on that same scan; only partial saves leave the row for the next full scan to reconcile.
Worth a look
- Deleted-file row now persists indefinitely for out-of-root paths on subset saves —
graphify/detect.py:2347· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Full-scan save no longer prunes a deleted out-of-root or non-in-root file's row —
graphify/detect.py:2347· Escalate · medium · 2 independent checks- 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 — 2718 functions depend on the 740 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 645 callers, 45 callees - new:
_rebuild_code()— 129 callers, 54 callees - new:
detect()— 112 callers, 15 callees - new:
_extract_generic()— 18 callers, 29 callees - new:
save_manifest()— 41 callers, 11 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - …and 45 more — each is listed as a finding
Verification — 2718 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: 1189 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
286 of 286 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/test_affected_cli.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— impact, full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.py— impact, full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_carried_hyperedge_remap.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— impact, full-run-safetytests/test_chunking.py— impact, full-run-safetytests/test_cjs_module_extension.py— impact, full-run-safetytests/test_claude_cli_backend.py— impact, full-run-safetytests/test_claude_md.py— full-run-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cluster.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— impact, full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_external_call_guards.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safety- … and 236 more
non-code file(s) changed (
CHANGELOG.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
CHANGELOG.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests
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
No difference found (not proven): No behavior difference found in save\_manifest (not a proof).
The verifier ran both versions of save\_manifest 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.
· 53 more finding(s) on lines outside this diff (see the check run).
A reviewer found that removing the old unconditional exists() check for issue 3426 preserved reconciliation only for in root rows via the scan exclusion check, leaving a genuinely deleted out of root row unprunable forever, even across full scans. Out of root rows must still never be pruned merely for being outside the current scan (they were never walked by detect, so absence from the corpus alone is not exclusion evidence), but that is a different condition from the file actually being gone from disk. A full scan already gives detect_incremental its chance to report the deletion first, the same safe reconciliation point the in root case relies on, so a full scan now also prunes an out of root row whose file no longer exists, leaving a partial save untouched exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers the fixed full scan case (genuinely deleted out of root row is now pruned) and the boundary that must stay unchanged (a partial save without scan_corpus still preserves a deleted out of root row, same as an in root one). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Good catch — fixed. |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Stops save_manifest from unconditionally pruning a manifest row whose file is gone from disk, which previously erased the row before detect_incremental could report the deletion via deleted_files on a save that doesn't also prune the graph (scan-only runs, interrupted pipelines) — making the deletion permanently unreportable. A partial/subset save (no scan_corpus) now keeps a dead row in place; a full-scan caller still reconciles it, sequenced after the deletion has had its chance to be reported. Extends that full-scan reconciliation to out-of-root rows whose file is genuinely gone, which the in-root-only exclusion check left unprunable forever.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2722 functions depend on the 744 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 645 callers, 45 callees - new:
_rebuild_code()— 129 callers, 54 callees - new:
detect()— 112 callers, 15 callees - new:
_extract_generic()— 18 callers, 29 callees - new:
save_manifest()— 43 callers, 11 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_js()— 85 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - …and 45 more — each is listed as a finding
Verification — 2722 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: 1193 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
286 of 286 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/test_affected_cli.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— impact, full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.py— impact, full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_carried_hyperedge_remap.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— impact, full-run-safetytests/test_chunking.py— impact, full-run-safetytests/test_cjs_module_extension.py— impact, full-run-safetytests/test_claude_cli_backend.py— impact, full-run-safetytests/test_claude_md.py— full-run-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cluster.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— impact, full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_external_call_guards.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safetytests/test_cross_repo_shared_types.py— full-run-safetytests/test_csharp_call_site_generic_args.py— full-run-safety- … and 236 more
non-code file(s) changed (
CHANGELOG.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
CHANGELOG.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests
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
No difference found (not proven): No behavior difference found in save\_manifest (not a proof).
The verifier ran both versions of save\_manifest 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.
· 53 more finding(s) on lines outside this diff (see the check run).
1 participant
Summary
Fixes #3426.
save_manifestcould prune a manifest row for a file that no longer exists on disk before any caller had a chance to observe that deletion viadetect_incremental'sdeleted_filesreport — making the deletion permanently unreportable from the very next run onward, whenever a save happened without also pruning the graph (a scan only run, an interrupted pipeline).Root cause
The reported issue described two cases:
detect_incrementalread side via a directPath(f).exists()check.save_manifestpruned any row whose file no longer existed on disk unconditionally and immediately, regardless of whetherdetect_incrementalhad run yet on that same state. A scan only run or an interrupted pipeline that callssave_manifestwithout also acting on the deletion would silently erase the evidence before anything downstream ever saw it.Fix
Removed the unconditional
Path(f).exists()prune fromsave_manifest. The existing scan corpus based prune (already used to drop "excluded but alive" rows, per #1908) still cleans up a genuinely deleted row when the caller passes the full scan corpus — a deleted file always falls outside that scan, on path alone — but now this happens naturally sequenced afterdetect_incrementalhas had the chance to report the deletion on that same scan. Only a partial/subset save (no scan corpus) now leaves a dead row in place; the next full scan reconciles it.Test plan
test_save_manifest_subset_save_keeps_a_deleted_files_rowintests/test_detect.py, reproducing the exact sequence from the issue: save, delete a file, confirm it is reported once, save again without the full scan corpus, confirm the row survives and the deletion is still reportable on the next pass.test_save_manifest_full_scan_still_prunes_missing_file(scan corpus based prune) still passes unmodified.python3 -m pytest -q— 5622 passed, 68 skipped, 2 deselected, no regressions.python3 -m tools.skillgen --check— OK.🤖 Generated with Claude Code