Skip to content

fix(watch): keep placeholder nodes for never-scanned files on incremental rebuilds - #4161

Closed
rohit-jsfreaky wants to merge 2 commits into
Graphify-Labs:v8from
rohit-jsfreaky:fix/incremental-keeps-missing-target-placeholders
Closed

rohit-jsfreaky wants to merge 2 commits into
Graphify-Labs:v8from
rohit-jsfreaky:fix/incremental-keeps-missing-target-placeholders

Conversation

@rohit-jsfreaky

@rohit-jsfreaky rohit-jsfreaky commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #4160.

A changed-files rebuild (post-commit hook, graphify watch) evicted nodes whose source_file is a path that was never in the checkout: placeholders the REFERRING file mints for an unresolved dynamic import (import('./queue.js')) or for a project a .sln / .slnx / <ProjectReference> names. The corpus sweep read the missing path as a deleted source, so the unchanged referrer's preserved edges to the placeholder dangled and were dropped, while a full rebuild of the same tree keeps both.

_referenced_unscanned_identities (watch.py) uses the previous scan as the deletion evidence #1795 asks for. manifest.json is only rewritten after reconcile, so during reconcile it is the last scan:

  • a missing path listed in it was a real file: evicted, as before;
  • a missing path never listed in it cannot have been deleted: its nodes are kept while a referrer that is not re-extracted this run still points at them (a re-extracted referrer re-emits them, or not, itself);
  • a missing path is only kept if it looks like a referrer's placeholder: none of its nodes has a source_location and it is the source_file of no edge. An extracted file always has one or the other (its file node, symbols, contains edges), so a real deleted file is evicted even if the manifest is stale or lost its entry;
  • no readable manifest, or one that lists none of today's files (anchored elsewhere): the old behaviour, unchanged.

The check runs in both sweep branches (a placeholder path without an extension, ./staticHelper, goes through the non-AST branch). It is computed once, lazily, only when a missing source is met.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

Verification & Invariants

Invariant: a changed-files rebuild must give the graph a full rebuild of the same tree gives (CONTRIBUTING: cross-file edges must be preserved during incremental updates), and a source with deletion evidence is still evicted.

Parity on graphify's own repo (975 files): three identical copies, each fully built by v8, then the same change; A = full rebuild, B = changed-files rebuild with this PR, C = changed-files rebuild on v8. Compared node-by-node and edge-by-edge with every attribute (community fields excluded, they depend on the whole graph):

change v8 changed-files vs full this PR vs full
edit graphify/manifest.py 8 nodes, 15 edges missing identical, all attributes
add a new module 8 nodes, 15 edges missing identical, all attributes
delete graphify/file_slice.py (7 importers) 10 nodes / 19 edges differ 2 / 4 differ, every one of them also differs on v8

What this PR does not change, and is already on v8: after a delete, the deleted file's unchanged importers keep their old edges into it. (One earlier edit run also showed 126 external stub nodes, posixpath etc., whose _origin differed between full and changed-files rebuilds on v8 and on this PR alike; the final run above did not reproduce it, and this PR does not touch _origin.)

Two alternatives were measured and rejected: re-extracting the referrers (a partial batch extracts differently from the full corpus: on the delete case it added 2 nodes a full rebuild does not have), and telling placeholders apart by label/location (a <ProjectReference> placeholder is labelled with the file name, like a real empty file).

  • Persisted state: placeholder nodes and their edges now survive incremental runs, as they do in full rebuilds. Nothing else changes.

  • Read the CONTRIBUTING.md guide.

  • Reproduced the issue and identified the invariant.

  • Made the smallest fix necessary.

  • Added a regression test (if bug fix) or isolated boundary test.

  • Kept the PR description synchronized with the final implementation.

  • Documented any limitations / unsupported cases explicitly.

How was this tested?

tests/test_incremental_unscanned_placeholders.py:

  • an unresolved import('./queue.js') placeholder and its edges survive an incremental rebuild of an unrelated file, and the result equals a full rebuild of the same tree (fails on v8: the placeholder is gone);
  • a scanned file that is deleted is still evicted;
  • without manifest.json the old eviction is kept;
  • a manifest that lists none of today's files is no evidence (old eviction);
  • a really deleted, extracted file whose manifest entry was lost is still evicted (the stale-manifest case the review bot raised; the last two fail on the first commit, 69ee389, and pass with 7340762).
pytest tests/test_incremental_unscanned_placeholders.py -q   # 5 passed; the first fails on 5c7b847
pytest tests/test_watch.py tests/test_incremental.py -q       # only the 2 Windows deleted-cwd tests that fail on clean v8
pytest tests -q                                              # 6589 passed, 19 failed: 18 on the clean-v8 Windows failure list + test_validate_url_accepts_http (DNS lookup of example.com; passes 3/3 rerun)
ruff check graphify/watch.py tests/test_incremental_unscanned_placeholders.py   # All checks passed
bandit -q -ll graphify/watch.py                              # no findings (same as v8)
pyright graphify/watch.py tests/test_incremental_unscanned_placeholders.py      # 22 errors, same as v8

Windows 11, Python 3.12, uv sync --all-extras --frozen, PYTHONHASHSEED=0.

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments. (not applicable)
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables).
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

🤖 Generated with Claude Code

…ntal rebuilds

An extractor can mint a node for a file that is not in the checkout: the
target of an unresolved dynamic import (`import('./queue.js')`) or a
project a .sln / <ProjectReference> names. The node carries that missing
path as its source_file, but it is the referring file's output. The
corpus sweep in _reconcile_existing_graph read the missing path as a
deleted source and evicted it, so the unchanged referrer's preserved
edges to it dangled and were dropped. A full rebuild of the same tree
keeps both, so every hook / watch rebuild silently diverged from it.

Use the previous scan as the deletion evidence Graphify-Labs#1795 asks for: a path
listed in manifest.json was a real file and is evicted as before; a path
that was never scanned is kept while a referrer that is not re-extracted
this run still points at it. Without a readable manifest the old
behaviour is unchanged.

On graphify's own repo, a changed-files rebuild after editing, adding or
deleting a file lost 8 nodes and 15 edges against a full rebuild. It now
has the same nodes and edges as the full rebuild after an edit or an add;
every difference left (the _origin tag of external stubs, and the
deleted file's importers after a delete) is already there on v8.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B8gwEpjajiWHVW3c9smuKf
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Thanks for the pull request, @rohit-jsfreaky. 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. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).

Not verified on this run: \_reconcile\_existing\_graph (vacuous: never exercised).


Graphify review — findings

Fixes changed-files rebuilds that evicted placeholder nodes minted for paths that never existed, such as an unresolved import('./queue.js') or a project a .sln names. Those nodes now survive while an unchanged, non-re-extracted referrer still has edges to them, so the incremental graph matches a full rebuild. _referenced_unscanned_identities treats manifest.json as the deletion evidence: paths it listed are still evicted when missing, and without a readable manifest the old eviction applies.

Worth a look

  • Deleted file kept as a placeholder when manifest.json is stale — graphify/watch.py:843 · 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 — 469 functions depend on the 93 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _rebuild_code() — 151 callers, 56 callees
  • new: main() — 102 callers, 3 callees
  • new: dispatch_command() — 2 callers, 128 callees
  • new: _run_cli() — 6 callers, 7 callees
  • new: _reconcile_graph_html() — 8 callers, 5 callees
  • new: watch() — 5 callers, 7 callees
  • new: _reconcile_existing_graph() — 1 callers, 9 callees
  • new: _reconcile_markdown_links() — 1 callers, 7 callees
  • …and 1 more — each is listed as a finding

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

Test selection

Test selection

11 of 335 test file(s) selected (3%) via static blast radius.

  • tests/test_elixir_import_resolution.py — impact
  • tests/test_elixir_unqualified_call_scope.py — impact
  • tests/test_export_direction.py — impact
  • tests/test_external_stub_endpoints.py — impact
  • tests/test_incremental_unscanned_placeholders.py — impact, changed-test
  • tests/test_kotlin_grammar.py — impact
  • tests/test_labeling.py — impact
  • tests/test_markdown_code_spans.py — impact
  • tests/test_terraform_modules.py — impact
  • tests/test_watch.py — impact
  • tests/test_watch_manifest_location.py — impact

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.

Docs that may be stale (advisory)

…and 10 more.

Formal verification

Could not verify: Could not verify \_reconcile\_existing\_graph.

The verifier did not have enough to check \_reconcile\_existing\_graph, 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 24 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)

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

Comment thread graphify/watch.py
return referenced


def _reconcile_existing_graph(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Health regression — _reconcile_existing_graph()

fans out to 9 callees (efferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

The previous commit kept nodes for a missing path the manifest does not
list. If the manifest is stale (a rebuild wrote the graph but not the
manifest) or anchored at another root, a really deleted file that an
unchanged file still imports looked never-scanned and kept its nodes.

Only keep a missing path whose nodes look like a referrer's placeholder:
none has a source_location and the path is the source_file of no edge.
An extracted file always has one or the other (its file node, symbols,
contains edges), so it is evicted whatever the manifest says. A manifest
that lists none of today's files is ignored (old behaviour).

Tests: a deleted real file dropped from the manifest is still evicted,
and a manifest anchored elsewhere keeps the old eviction; both fail on
the previous commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B8gwEpjajiWHVW3c9smuKf
@rohit-jsfreaky

Copy link
Copy Markdown
Contributor Author

The stale-manifest finding was real. On 69ee389, a deleted file that had been extracted, whose manifest.json entry was missing and that an unchanged file still imports, looked never-scanned and kept its nodes; test_a_deleted_real_file_is_evicted_even_if_the_manifest_lost_it reproduces it and fails on that commit.

Fixed in 7340762: a missing path is only kept if it looks like a referrer's placeholder (no node with a source_location, and it is the source_file of no edge). An extracted file always has its file node, symbols or contains edges, so it is evicted whatever the manifest says. A manifest that lists none of today's files is ignored (old behaviour), also tested.

Re-ran the parity runs on graphify's own repo with this: edit, identical to a full rebuild including all attributes; delete, every remaining difference also exists on v8. Full suite: no new failures (one DNS-dependent test flaked and passes on rerun). Description updated.

safishamsi pushed a commit that referenced this pull request Oct 6, 2026
…ntal rebuilds (#4161, #4160)

On an incremental reconcile the corpus sweep read a never-scanned referenced
path (unresolved dynamic import target, .sln/.csproj reference) as a deleted
source and evicted the placeholder node a referrer minted, dropping the
unchanged referrer's preserved cross-file edges. Skip eviction for such
referenced-but-unscanned paths (purely additive; never causes a new eviction),
with deletion-evidence guards so genuinely deleted files are still reaped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@safishamsi

Copy link
Copy Markdown
Member

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: changed-files rebuild (hook/watch) evicts placeholder nodes for files that were never scanned

2 participants