Skip to content

fix(detect): scope incremental manifest anchoring to prevent false deletions (#3785) - #4118

Closed
harshaygadekar wants to merge 1 commit into
Graphify-Labs:v8from
harshaygadekar:fix/detect-incremental-subfolder-manifest
Closed

harshaygadekar wants to merge 1 commit into
Graphify-Labs:v8from
harshaygadekar:fix/detect-incremental-subfolder-manifest

Conversation

@harshaygadekar

@harshaygadekar harshaygadekar commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #3785.

In multi-directory workspaces or repositories where distinct subdirectories are graphed or updated independently (e.g. /graphify workflows, /graphify experts, or --update\ on subfolders):

  1. When a manifest is stored at <workspace>/graphify-out/manifest.json, stored keys are relative to <workspace>. Running an incremental scan on a subfolder (\workflows/) previously caused \load_manifest(..., root=workflows)\ to re-anchor those keys against \workflows/, creating invalid paths that failed disk checks and were falsely reported as deleted.
  2. \detect_incremental()\ iterated over all rows in the loaded manifest without verifying whether the entries actually belonged to
    oot.

Fix

  1. Manifest Storage Anchor Resolution (_manifest_storage_anchor): When a manifest resides under <workspace>/graphify-out/manifest.json\ and
    oot\ is a subfolder of <workspace>, stored relative keys are anchored to <workspace>\ rather than the subfolder root. _to_absolute_from_storage\ remains completely pure with zero filesystem I/O.
  2. Root Containment Guard: In \detect_incremental(), manifest entries outside
    oot\ are skipped (\if not _in_root(f): continue), ensuring files belonging to other subfolders or parent directories are never falsely reported as \deleted_files\ or \excluded_files.
  3. No Filesystem Traversal or Slug Overrides: Eliminates speculative ancestor traversal and preserves default un-slugged manifest locations so --out\ and CLI workflows continue to operate as expected.

Verification

  • Standalone reproduction script
    eproduce_issue_3785.py\ passes: 0 false deletions, 0 false exclusions across both subfolder update scenarios.
  • Detect test suite: \uv run pytest tests/test_detect.py\ (275 passed, 11 skipped, 0 failures).
  • CLI extract test suite: \uv run pytest tests/test_extract_cli.py\ (34 passed, 0 failures).
  • Pre-commit checks: \skillgen --check\ and
    uff\ passed.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

Thanks for the pull request, @harshaygadekar. 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 graphify-labs Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 1 change(s) alter behavior, breaking input(s) attached. PR-changed functions: 3/4 verified (0 proven, 2 may-equivalent, 1 distinguished) · 1 not verified (1 unsupported).

Behavior changes: \_to\_absolute\_from\_storage changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_to\_absolute\_from\_storage 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 \{"key":"'\.'","root":"\_\_import\_\_\('pathlib'\)\.Path\('missing/deeper\.txt'\)"\}, the old code produced '/tmp/gfy\-fv\-path\-5oop69gc/ws/missing/deeper\.txt' but the new code produces '/tmp/gfy\-fv\-path\-5oop69gc/ws' (outputs first differ at character 32). Paste that input straight into a regression test.

Not verified on this run: save\_manifest (unsupported).


Graphify review — findings

Scopes the default manifest per scanned subfolder: load_manifest, save_manifest, and detect_incremental resolve graphify-out/manifest.json to graphify-out/<slug>/manifest.json through the newly exported manifest_path_for_root, so sibling subfolder runs no longer clobber each other. Reads still fall back to the unscoped manifest when no scoped one exists yet. detect_incremental also skips manifest entries outside the scanned root instead of reporting them as deleted or excluded, and _to_absolute_from_storage resolves relative keys against ancestor directories and the cwd when the root-joined path doesn't exist (#3785).

Worth a look

  • Ancestor lookup can hide deletions for relative manifest keys — graphify/detect.py:2402 · 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 — 2963 functions depend on the 582 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract() — 766 callers, 50 callees
  • new: _rebuild_code() — 149 callers, 56 callees
  • new: detect() — 121 callers, 16 callees
  • new: save_manifest() — 44 callers, 14 callees
  • new: _extract_generic() — 18 callers, 31 callees
  • new: extract_js() — 87 callers, 4 callees
  • new: extract_files_direct() — 17 callers, 20 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • …and 56 more — each is listed as a finding

Verification — 2963 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: 1094 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

329 of 329 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-safety
  • tests/test_affected_member_seed.py — full-run-safety
  • tests/test_agents_platform.py — full-run-safety
  • tests/test_analyze.py — full-run-safety
  • tests/test_anthropic_custom_endpoint.py — full-run-safety
  • tests/test_antigravity_install.py — full-run-safety
  • tests/test_apm_fallback_version.py — full-run-safety
  • tests/test_architecture_doc.py — full-run-safety
  • tests/test_astro_extraction.py — impact, full-run-safety
  • tests/test_astro_import_ids.py — full-run-safety
  • tests/test_atomic_canvas_export.py — full-run-safety
  • tests/test_atomic_version_stamp.py — full-run-safety
  • tests/test_atomic_writes.py — impact, full-run-safety
  • tests/test_backend_env_isolation.py — full-run-safety
  • tests/test_backend_extras.py — full-run-safety
  • tests/test_benchmark.py — full-run-safety
  • tests/test_benchmark_raw_graph.py — full-run-safety
  • tests/test_blade_extractor.py — full-run-safety
  • tests/test_build.py — impact, full-run-safety
  • tests/test_build_merge_dedup_scope.py — full-run-safety
  • tests/test_build_merge_hyperedges_and_prune.py — full-run-safety
  • tests/test_build_merge_shrink_guard.py — full-run-safety
  • tests/test_builtin_global_type_refs.py — full-run-safety
  • tests/test_cache.py — full-run-safety
  • tests/test_callflow_html.py — full-run-safety
  • tests/test_cargo_introspect.py — impact, full-run-safety
  • tests/test_cargo_missing_manifest.py — full-run-safety
  • tests/test_carried_hyperedge_remap.py — full-run-safety
  • tests/test_case_sensitive_resolution.py — full-run-safety
  • tests/test_charmap_encoding.py — impact, full-run-safety
  • tests/test_chunking.py — impact, full-run-safety
  • tests/test_cjs_module_extension.py — impact, full-run-safety
  • tests/test_claude_cli_backend.py — impact, full-run-safety
  • tests/test_claude_md.py — full-run-safety
  • tests/test_cli_broken_pipe.py — full-run-safety
  • tests/test_cli_export.py — full-run-safety
  • tests/test_cli_help.py — full-run-safety
  • tests/test_cluster.py — full-run-safety
  • tests/test_cluster_exclude_hubs.py — full-run-safety
  • tests/test_cobol_extractor.py — full-run-safety
  • tests/test_codebuddy.py — full-run-safety
  • tests/test_community_hub_labels.py — full-run-safety
  • tests/test_community_labels_skill.py — full-run-safety
  • tests/test_confidence.py — full-run-safety
  • tests/test_corrupt_graph_json.py — full-run-safety
  • tests/test_cpp_method_declarations.py — full-run-safety
  • tests/test_cpp_nested_and_cli.py — impact, full-run-safety
  • tests/test_cpp_objc_cross_file_calls.py — full-run-safety
  • tests/test_cpp_preprocess.py — full-run-safety
  • tests/test_cross_extension_reexport_self_cycle.py — full-run-safety
  • … and 279 more

changed code file(s) with no mapped test (graphify/manifest.py) — 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

Behavior changes: \_to\_absolute\_from\_storage changes behavior, here is the input that shows it.

The verifier found a concrete input on which \_to\_absolute\_from\_storage 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 \{"key":"'\.'","root":"\_\_import\_\_\('pathlib'\)\.Path\('missing/deeper\.txt'\)"\}, the old code produced '/tmp/gfy\-fv\-path\-5oop69gc/ws/missing/deeper\.txt' but the new code produces '/tmp/gfy\-fv\-path\-5oop69gc/ws' (outputs first differ at character 32). Paste that input straight into a regression test.

No difference found (not proven): No behavior difference found in detect\_incremental (not a proof).

The verifier ran both versions of detect\_incremental 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.

No difference found (not proven): No behavior difference found in load\_manifest (not a proof).

The verifier ran both versions of load\_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.

Could not verify: Could not verify save\_manifest.

The verifier did not have enough to check save\_manifest, 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: the input domain has 144 values but only 24 distinct were tested — a small finite domain must be EXHAUSTED, not sampled (an untested input could invert the result)

· 3 grounded finding(s) anchored inline below; 61 more finding(s) on lines outside this diff (see the check run).

Comment thread graphify/detect.py
Comment thread graphify/detect.py
Comment thread graphify/detect.py
@harshaygadekar
harshaygadekar force-pushed the fix/detect-incremental-subfolder-manifest branch from c6b8ac2 to 1c16199 Compare October 5, 2026 17:09
@harshaygadekar harshaygadekar changed the title fix(detect): scope incremental manifest per subfolder to prevent false deletions (#3785) fix(detect): scope incremental manifest anchoring to prevent false deletions (#3785) Oct 5, 2026
@harshaygadekar

harshaygadekar commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Update Addressing Bot Review & Verification

  1. Pure Lexical Resolution in _to_absolute_from_storage: Reverted all filesystem calls (.exists()) and ancestor traversal in _to_absolute_from_storage. The function remains strictly a lexical path normalizer that never touches disk, eliminating the formal verification discrepancy and ensuring no deletion can ever be masked by ancestor files.
  2. Fixed CLI --out\ Manifest Locations: Removed slug directory redirects on default manifest paths so explicit --out\ CLI options and default output directories are never moved.
  3. Anchor Resolution via _manifest_storage_anchor: Handled subfolder manifest re-anchoring by inspecting whether
    oot\ is a subfolder of the manifest's containing workspace root (\manifest_path.parent.parent), correctly resolving relative paths without double-nesting.
  4. Out-of-Root Containment Filter: Manifest entries outside
    oot\ continue to be skipped cleanly in \detect_incremental.

@safishamsi

Copy link
Copy Markdown
Member

Landed in v0.9.77 via an authorship-preserving cherry-pick, so your commit is on v8 with you credited as the author. Closing as shipped — thanks @harshaygadekar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants