fix(skills): --update must prune files a new ignore rule excluded (#2773) - #2806
fix(skills): --update must prune files a new ignore rule excluded (#2773)#2806abhay-codes07 wants to merge 1 commit into
Conversation
…aphify-Labs#2773) detect_incremental deliberately splits a manifest row whose file is GONE from disk (deleted_files) from one that still exists but has left the scan because an ignore rule or --exclude changed (excluded_files, Graphify-Labs#1908), so that an exclusion is never mis-reported as a deletion. graphify's library path consumes both -- cli.py prunes `list(excluded_files) + graph_stale_sources`. The --update runbook read only deleted_files, so adding a .graphifyignore rule never removed what it excluded from graph.json. Two separate failures, both fixed: 1. The early exit. With nothing else changed, `new_total == 0 and not deleted` held, so the run printed "No files changed since last run. Nothing to update." and exited 0 without ever reaching the merge. Adding an ignore rule on its own -- the exact thing a user does when they want content out of the graph -- was therefore a no-op that reported success. 2. The prune set. Even when the run did continue because some other file changed, `prune = list(deleted) or None` left the excluded file's nodes and edges in place. The leak is permanent, not merely deferred: save_manifest(scan_corpus=...) drops the excluded row, so on the next run the file is neither deleted nor excluded and nothing can prune it again. One --update after adding the rule and the stale content is unreachable without a full rebuild. That is what Graphify-Labs#2773 reports as surviving a forced rebuild with the cache purged. Reproduced on a two-file corpus: with `archive/` newly ignored, prune=deleted leaves arch_free_port in the graph and prune=deleted+excluded removes it, and a second run reports empty deleted_files AND empty excluded_files.
![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.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Propagates newly excluded files through the incremental update path in every references/update.md skill variant (agents, amp, claude, codex, claw, droid, kilo, opencode, pi, vscode, windows, etc.) and their skillgen expected outputs, so a new .graphifyignore/--exclude rule triggers a prune instead of exiting early with "Nothing to update" (#2773). Reads excluded_files from the incremental result to gate the no-op exit and folds it into prune = (deleted + excluded) alongside deletions. Adds test_update_prunes_excluded covering the exclude-then-prune flow.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 131 functions depend on the 131 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_seed_graph()— 4 callers, 3 callees - new:
test_the_leak_is_permanent_once_the_manifest_is_restamped()— 0 callers, 6 callees
Verification — 131 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: 131 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below.
| return tmp_path | ||
|
|
||
|
|
||
| def _seed_graph(root: Path) -> Path: |
There was a problem hiding this comment.
_seed_graph()
high coupling complexity (Ca·Ce = 12).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| for _, _, d in G.edges(data=True)) | ||
|
|
||
|
|
||
| def test_the_leak_is_permanent_once_the_manifest_is_restamped(tmp_path): |
There was a problem hiding this comment.
test_the_leak_is_permanent_once_the_manifest_is_restamped()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Fixes #2773.
Thanks @oilsinwater — the report was right that this survives
--forceand a purged cache, and the reason is worse than a stale cache: the pruning step never asked for those files at all.The bug
detect_incrementaldeliberately splits a manifest row whose file is gone from disk (deleted_files) from one that still exists but has left the scan because an ignore rule or--excludechanged (excluded_files, #1908) — precisely so an exclusion is never mis-reported as a deletion.graphify's own library path consumes both.
graphify/cli.py:The
--updaterunbook — which is how the skill drives incremental rebuilds — read only the first list:So
.graphifyignorewas honoured by the scan and ignored by the graph.Two failures, not one
The early exit. With nothing else changed,
new_total == 0 and not deletedheld:Adding an ignore rule on its own — the exact thing you do when you want content out of the graph — was a no-op that reported success and never reached the merge. This is the case worth fixing first; the prune set below only matters once the run gets that far.
The prune set. When the run did continue because some other file changed,
prune = list(deleted) or Nonestill left the excluded file's nodes and edges in place.The leak is permanent, not deferred
This is why "the next run will catch it" does not hold.
save_manifest(scan_corpus=...)drops the excluded row, so on the following run the file is neither deleted nor excluded:One
--updateafter adding the rule and the stale content is unreachable short of a full rebuild — which matches the report's "same count and same 4 edges, byte-for-byte, before and after the forced/purged rebuild".Reproduced
Two-file corpus,
archive/newly ignored, driving the runbook's own merge call:The leaked node is an archived helper joined by a
callsedge to live production code — the same shape as the report'stests_integration_free_port -> src_loro_irohlorotransport_bind, which is what makes it more than clutter: a false edge from dead code into live code feedsGRAPH_REPORT.md's architectural claims.The change
One fragment,
tools/skillgen/fragments/references/shared/update.md; everything else in the diff ispython -m tools.skillgenoutput plus a re---blessofexpected/. All five validators pass.I kept the fix to
excluded_filesand did not port_stale_graph_sources(#1909), which is the library's more complete answer — it derives prune candidates from the graph's ownsource_files and so also catches files whose manifest row is already gone. That would fix the permanence for graphs already in the leaked state, but it means calling a privateclihelper from the runbook, which felt like your call rather than mine. Happy to add it if you want the stronger version.Tests
tests/test_update_prunes_excluded.py(7 tests). Being straight about what each one does:excluded_files, that the early exit accounts for it, and that every host's renderedreferences/update.mdagrees rather than one lagging. These three fail without the fix.excluded_filesand notdeleted_files, thatprune=deletedleaves the node whileprune=deleted+excludedremoves it, and that the manifest re-stamp makes the row disappear from both lists. Those pass either way by construction — they are the evidence for the fix, not a gate on it, and I would rather say so than let the count imply more than it does.Validation
Windows 11, Python 3.12, branched off
4fca621(0.9.44).20 failed, 4469 passed->20 failed, 4476 passed. Identical failure set — no regressions; the +7 are the new tests.Note on #2801
My other open PR also edits this fragment (removing the
rm -fcleanup a few lines below), so expect a small textual conflict and none semantically. Happy to rebase whichever lands second.