mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-28 17:26:48 +00:00
fix(watch): bypass shrink-guard when caller declared explicit deletions (#1000)
The post-commit hook passes `git diff --name-only HEAD~1 HEAD` as changed_paths to `_rebuild_code`. That list includes deletions, and `_rebuild_code` correctly identifies them (lines 352-367 in watch.py) and evicts the stale nodes from the preserved set. The rebuilt graph is intentionally smaller. `_check_shrink` then refuses to overwrite the existing graph.json because it only sees the node-count delta, not the cause. The guard fires with "Refusing to overwrite — you may be missing chunk files from a previous session. Pass --force to override." Result: every commit that deletes a tracked file silently leaves stale nodes in graph.json. The user must either pass --force (which also disables the guard for legitimate failure modes) or manually re-run `graphify update . --force` after delete-heavy commits. Fix: thread a `had_explicit_deletions` flag from `_rebuild_code` into `_check_shrink`. When the caller has declared the deletions, the smaller graph is the expected outcome and the guard is skipped. The guard remains intact for SILENT shrinkage — its actual purpose — from failed semantic chunks or corrupted runs. The fix is opt-in by design: callers that don't pass `changed_paths` (e.g. the post-checkout full rebuild path) keep the old conservative behavior. Only paths that explicitly track deletions get the bypass. Tests added (tests/test_watch.py): - `test_check_shrink_blocks_silent_shrink` — pre-existing behavior intact - `test_check_shrink_allows_force_override` — pre-existing behavior intact - `test_check_shrink_allows_explicit_deletions` — new: deletion bypass - `test_check_shrink_allows_no_existing_data` — first-run case - `test_check_shrink_allows_growth` — sanity - `test_check_shrink_unlinks_tmp_on_refuse` — cleanup on refusal - `test_check_shrink_keeps_tmp_when_deletions_declared` — no spurious unlink - `test_rebuild_code_prunes_deleted_file_nodes` — end-to-end probe of the exact scenario the post-commit hook triggers (git init, build, delete one file, re-run with the deleted path in changed_paths, verify the graph shrinks and the surviving file's nodes are preserved) All 8 new tests pass; full test_watch.py + test_build.py + test_export.py (74 tests) pass with no regressions.
This commit is contained in:
+25
-4
@@ -240,12 +240,26 @@ def _topology_from_graph(G) -> dict:
|
||||
return data
|
||||
|
||||
|
||||
def _check_shrink(force: bool, existing_data: dict, new_data: dict, tmp: "Path | None" = None) -> bool:
|
||||
def _check_shrink(
|
||||
force: bool,
|
||||
existing_data: dict,
|
||||
new_data: dict,
|
||||
tmp: "Path | None" = None,
|
||||
*,
|
||||
had_explicit_deletions: bool = False,
|
||||
) -> bool:
|
||||
"""Return True (ok to proceed) or False (shrink refused).
|
||||
|
||||
When False, cleans up *tmp* if provided and prints a warning to stderr.
|
||||
|
||||
The shrink-guard exists to catch SILENT shrinkage from failed extraction
|
||||
chunks (a half-written semantic pass leaving thousands of nodes
|
||||
unaccounted for). When ``had_explicit_deletions`` is True, the caller
|
||||
has declared which files were removed (e.g. the post-commit hook saw
|
||||
a ``D`` in ``git diff --name-only``) and a smaller graph is the expected
|
||||
outcome — skip the guard so legitimate refactors don't require ``--force``.
|
||||
"""
|
||||
if force or not existing_data:
|
||||
if force or not existing_data or had_explicit_deletions:
|
||||
return True
|
||||
existing_n = len(existing_data.get("nodes", []))
|
||||
new_n = len(new_data.get("nodes", []))
|
||||
@@ -444,7 +458,10 @@ def _rebuild_code(
|
||||
except Exception:
|
||||
same_graph = False
|
||||
if not same_graph:
|
||||
if not _check_shrink(force, existing_graph_data, candidate_graph_data):
|
||||
if not _check_shrink(
|
||||
force, existing_graph_data, candidate_graph_data,
|
||||
had_explicit_deletions=bool(deleted_paths),
|
||||
):
|
||||
return False
|
||||
existing_graph.write_text(candidate_graph_text, encoding="utf-8")
|
||||
|
||||
@@ -545,7 +562,11 @@ def _rebuild_code(
|
||||
graph_tmp.unlink(missing_ok=True)
|
||||
print("[graphify watch] No code-graph changes detected; graph.json/GRAPH_REPORT.md left untouched.")
|
||||
else:
|
||||
if not _check_shrink(force, existing_graph_data, candidate_graph_data, tmp=graph_tmp):
|
||||
if not _check_shrink(
|
||||
force, existing_graph_data, candidate_graph_data,
|
||||
tmp=graph_tmp,
|
||||
had_explicit_deletions=bool(deleted_paths),
|
||||
):
|
||||
return False
|
||||
from graphify.export import backup_if_protected as _backup
|
||||
_backup(out)
|
||||
|
||||
+160
-1
@@ -1,11 +1,13 @@
|
||||
"""Tests for watch.py - file watcher helpers (no watchdog required)."""
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import time
|
||||
from pathlib import Path
|
||||
import pytest
|
||||
|
||||
from graphify.watch import _notify_only, _WATCHED_EXTENSIONS, _rebuild_lock
|
||||
from graphify.watch import _notify_only, _WATCHED_EXTENSIONS, _rebuild_lock, _check_shrink
|
||||
|
||||
|
||||
# --- _notify_only ---
|
||||
@@ -294,3 +296,160 @@ def test_watch_loads_graphifyignore_once(tmp_path, monkeypatch):
|
||||
(tmp_path / "ignored" / f"f{i}.py").write_text("x\n", encoding="utf-8")
|
||||
time.sleep(0.7)
|
||||
assert calls["n"] == 1, f"_load_graphifyignore called {calls['n']} times; expected 1"
|
||||
|
||||
|
||||
# --- _check_shrink: silent-corruption guard with explicit-deletion bypass ---
|
||||
|
||||
def _shrink_payload(n: int) -> dict:
|
||||
"""Build a minimal graph-data dict with *n* placeholder nodes."""
|
||||
return {"nodes": [{"id": f"n{i}"} for i in range(n)], "links": []}
|
||||
|
||||
|
||||
def test_check_shrink_blocks_silent_shrink(capsys):
|
||||
"""Default case: smaller new graph + no force + no declared deletions = refuse."""
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data=_shrink_payload(100),
|
||||
new_data=_shrink_payload(80),
|
||||
)
|
||||
assert ok is False
|
||||
captured = capsys.readouterr()
|
||||
assert "Refusing to overwrite" in captured.err
|
||||
assert "80 nodes" in captured.err and "100" in captured.err
|
||||
|
||||
|
||||
def test_check_shrink_allows_force_override():
|
||||
"""force=True bypasses the guard regardless of node delta."""
|
||||
ok = _check_shrink(
|
||||
force=True,
|
||||
existing_data=_shrink_payload(100),
|
||||
new_data=_shrink_payload(1),
|
||||
)
|
||||
assert ok is True
|
||||
|
||||
|
||||
def test_check_shrink_allows_explicit_deletions(capsys):
|
||||
"""Caller declared deletions → shrink is expected → guard skipped silently."""
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data=_shrink_payload(100),
|
||||
new_data=_shrink_payload(80),
|
||||
had_explicit_deletions=True,
|
||||
)
|
||||
assert ok is True
|
||||
# And critically, no scary warning is printed when the shrink is intentional.
|
||||
assert "Refusing to overwrite" not in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_check_shrink_allows_no_existing_data():
|
||||
"""First-run case: no existing graph → guard inert."""
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data={},
|
||||
new_data=_shrink_payload(50),
|
||||
)
|
||||
assert ok is True
|
||||
|
||||
|
||||
def test_check_shrink_allows_growth():
|
||||
"""new > existing is always fine."""
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data=_shrink_payload(50),
|
||||
new_data=_shrink_payload(60),
|
||||
)
|
||||
assert ok is True
|
||||
|
||||
|
||||
def test_check_shrink_unlinks_tmp_on_refuse(tmp_path):
|
||||
"""When refusing, the temp graph file gets cleaned up so it can't leak across runs."""
|
||||
tmp = tmp_path / "graph.tmp.json"
|
||||
tmp.write_text("{}", encoding="utf-8")
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data=_shrink_payload(100),
|
||||
new_data=_shrink_payload(80),
|
||||
tmp=tmp,
|
||||
)
|
||||
assert ok is False
|
||||
assert not tmp.exists()
|
||||
|
||||
|
||||
def test_check_shrink_keeps_tmp_when_deletions_declared(tmp_path):
|
||||
"""Mirror of the above: if the caller declared deletions, the tmp file is NOT unlinked
|
||||
because the caller is going to swap it into place. Regression guard against a future
|
||||
bug where the tmp cleanup leaks out of the refuse branch.
|
||||
"""
|
||||
tmp = tmp_path / "graph.tmp.json"
|
||||
tmp.write_text("{}", encoding="utf-8")
|
||||
ok = _check_shrink(
|
||||
force=False,
|
||||
existing_data=_shrink_payload(100),
|
||||
new_data=_shrink_payload(80),
|
||||
tmp=tmp,
|
||||
had_explicit_deletions=True,
|
||||
)
|
||||
assert ok is True
|
||||
assert tmp.exists()
|
||||
|
||||
|
||||
# --- _rebuild_code integration: post-commit delete scenario ---
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="git CLI behaviour varies on Windows runners")
|
||||
def test_rebuild_code_prunes_deleted_file_nodes(tmp_path):
|
||||
"""End-to-end probe of the post-commit-delete bug fix.
|
||||
|
||||
Build a tiny graph, delete one of its source files, then call _rebuild_code
|
||||
with the deleted path in changed_paths. Without the fix this raises the
|
||||
shrink guard and refuses to write; with the fix the deleted file's nodes
|
||||
are pruned and graph.json is rewritten.
|
||||
"""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
# Set up a minimal "project" with two Python files in a git repo so detect
|
||||
# treats it as a real corpus.
|
||||
subprocess.run(["git", "init", "-q", str(tmp_path)], check=True)
|
||||
subprocess.run(
|
||||
["git", "-C", str(tmp_path), "config", "user.email", "test@example.com"],
|
||||
check=True,
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "-C", str(tmp_path), "config", "user.name", "Test"],
|
||||
check=True,
|
||||
)
|
||||
|
||||
keep = tmp_path / "keep.py"
|
||||
drop = tmp_path / "drop.py"
|
||||
keep.write_text("def keep_fn():\n return 1\n", encoding="utf-8")
|
||||
drop.write_text("def drop_fn():\n return 2\n", encoding="utf-8")
|
||||
|
||||
# Initial build covers both files.
|
||||
cwd = os.getcwd()
|
||||
try:
|
||||
os.chdir(tmp_path)
|
||||
ok = _rebuild_code(tmp_path, no_cluster=True)
|
||||
assert ok is True
|
||||
graph_path = tmp_path / "graphify-out" / "graph.json"
|
||||
assert graph_path.exists()
|
||||
before = json.loads(graph_path.read_text(encoding="utf-8"))
|
||||
before_sources = {n.get("source_file") for n in before.get("nodes", [])}
|
||||
assert "drop.py" in before_sources
|
||||
|
||||
# Now delete drop.py and re-run with it in the change list. This is what
|
||||
# the post-commit hook does when git diff --name-only HEAD~1 HEAD includes
|
||||
# a deletion: the path is passed to _rebuild_code even though it no
|
||||
# longer exists on disk.
|
||||
drop.unlink()
|
||||
ok = _rebuild_code(
|
||||
tmp_path,
|
||||
changed_paths=[Path("drop.py")],
|
||||
no_cluster=True,
|
||||
)
|
||||
assert ok is True, "rebuild should succeed even though the graph shrinks"
|
||||
|
||||
after = json.loads(graph_path.read_text(encoding="utf-8"))
|
||||
after_sources = {n.get("source_file") for n in after.get("nodes", [])}
|
||||
assert "drop.py" not in after_sources, "deleted file's nodes should be pruned"
|
||||
assert "keep.py" in after_sources, "untouched file's nodes should survive"
|
||||
finally:
|
||||
os.chdir(cwd)
|
||||
|
||||
Reference in New Issue
Block a user