mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 00:36:39 +00:00
fix(watch): refuse to overwrite an unreadable existing graph in the hook rebuild (#2251)
_reconcile_existing_graph loaded graph.json inside a swallowing try, so a graph that was merely unreadable (over the size cap or unparseable) was silently replaced by the code-only extraction, in both the clustered and --no-cluster hook paths (force made it worse). It now loads through the fail-closed build._load_existing_graph and _rebuild_code refuses the write (prints and returns False) on a load failure, matching the CLI path; the --no-cluster write is now atomic with a protected-graph backup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
f67113361b
commit
2099873ae6
+85
-19
@@ -463,14 +463,27 @@ def _reconcile_existing_graph(
|
||||
if not existing_graph.exists():
|
||||
return result, existing_graph_data
|
||||
|
||||
# Fail-closed load (#2251): reuse build._load_existing_graph, which raises
|
||||
# ValueError when the file exceeds the size cap and RuntimeError when it
|
||||
# exists but cannot be parsed. Those failures must PROPAGATE to the caller
|
||||
# — swallowing them here left existing_graph_data == {}, which _check_shrink
|
||||
# reads as "no baseline, write allowed", so the hook overwrote a graph it
|
||||
# merely failed to READ. A missing file (None) keeps first-build behavior:
|
||||
# reconcile proceeds with an empty baseline and the write is allowed.
|
||||
from graphify.build import _load_existing_graph
|
||||
if _load_existing_graph(existing_graph) is None:
|
||||
return result, existing_graph_data
|
||||
# Cap + parse validated above. Reload as the full dict — reconcile needs
|
||||
# top-level keys (hyperedges, per-node community for _node_community_map,
|
||||
# topology compare) the (nodes, edges, hyperedges) tuple does not carry.
|
||||
# A failure here (e.g. a race rewriting the file) still propagates,
|
||||
# staying fail-closed.
|
||||
existing = json.loads(existing_graph.read_text(encoding="utf-8"))
|
||||
existing_graph_data = existing
|
||||
|
||||
try:
|
||||
from graphify.build import _norm_source_file as _nsf
|
||||
from graphify.extract import _get_extractor
|
||||
from graphify.security import check_graph_file_size_cap
|
||||
|
||||
check_graph_file_size_cap(existing_graph)
|
||||
existing = json.loads(existing_graph.read_text(encoding="utf-8"))
|
||||
existing_graph_data = existing
|
||||
source_paths = _StoredSourcePaths(
|
||||
existing,
|
||||
out=out,
|
||||
@@ -627,7 +640,16 @@ def _reconcile_existing_graph(
|
||||
"input_tokens": 0,
|
||||
"output_tokens": 0,
|
||||
}, existing_graph_data
|
||||
except Exception:
|
||||
except Exception as exc:
|
||||
# Post-load reconciliation failure: fall back to the fresh extraction
|
||||
# while keeping the loaded baseline, so _check_shrink still guards the
|
||||
# write against a collapse. Say so — this used to be silent (#2251).
|
||||
print(
|
||||
"[graphify watch] reconcile of existing graph failed "
|
||||
f"({exc.__class__.__name__}: {exc}); proceeding with fresh "
|
||||
"extraction only.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return result, existing_graph_data
|
||||
|
||||
|
||||
@@ -1107,18 +1129,29 @@ def _rebuild_code(
|
||||
# When the caller supplied changed_paths, also evict preserved nodes whose
|
||||
# source_file matches a path that was changed (re-extracted) or deleted —
|
||||
# otherwise the old nodes for those files would survive forever.
|
||||
result, existing_graph_data = _reconcile_existing_graph(
|
||||
existing_graph,
|
||||
result,
|
||||
out=out,
|
||||
project_root=project_root,
|
||||
watch_root=watch_root,
|
||||
code_files=code_files,
|
||||
extract_targets=extract_targets,
|
||||
full_rebuild=changed_paths is None,
|
||||
deleted_paths=deleted_paths,
|
||||
deleted_source_identities=deleted_source_identities,
|
||||
)
|
||||
try:
|
||||
result, existing_graph_data = _reconcile_existing_graph(
|
||||
existing_graph,
|
||||
result,
|
||||
out=out,
|
||||
project_root=project_root,
|
||||
watch_root=watch_root,
|
||||
code_files=code_files,
|
||||
extract_targets=extract_targets,
|
||||
full_rebuild=changed_paths is None,
|
||||
deleted_paths=deleted_paths,
|
||||
deleted_source_identities=deleted_source_identities,
|
||||
)
|
||||
except (RuntimeError, ValueError) as exc:
|
||||
# Existing graph present but unreadable — over the size cap
|
||||
# (ValueError) or unparseable JSON (RuntimeError, both via
|
||||
# build._load_existing_graph). Refuse to overwrite a graph we
|
||||
# merely failed to READ (#2251), mirroring the CLI's fail-closed
|
||||
# contract (#2169). --force deliberately does NOT bypass this:
|
||||
# force means "accept a shrink", not "clobber an unreadable
|
||||
# graph".
|
||||
print(f"error: {exc}", file=sys.stderr)
|
||||
return False
|
||||
|
||||
_relativize_source_files(result, project_root, scope=watch_root)
|
||||
# Source files re-extracted this run — their symbol sets may legitimately
|
||||
@@ -1152,6 +1185,19 @@ def _rebuild_code(
|
||||
try:
|
||||
check_graph_file_size_cap(existing_graph)
|
||||
existing_payload = json.loads(existing_graph.read_text(encoding="utf-8"))
|
||||
except Exception as exc:
|
||||
# A load failure is NOT "graph changed" (#2251): refuse to
|
||||
# overwrite a graph we merely failed to read. Normally
|
||||
# unreachable — the reconcile load above already failed
|
||||
# closed — but a race rewriting the file can land here.
|
||||
print(
|
||||
f"error: Cannot read {existing_graph}: {exc}. "
|
||||
"Refusing to overwrite; delete the file and run a "
|
||||
"full rebuild.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return False
|
||||
try:
|
||||
same_graph = (
|
||||
json.dumps(_canonical_graph_for_compare(existing_payload), sort_keys=True, ensure_ascii=False)
|
||||
== json.dumps(_canonical_graph_for_compare(candidate_graph_data), sort_keys=True, ensure_ascii=False)
|
||||
@@ -1165,7 +1211,13 @@ def _rebuild_code(
|
||||
rebuilt_sources=rebuilt_sources,
|
||||
):
|
||||
return False
|
||||
existing_graph.write_text(candidate_graph_text, encoding="utf-8")
|
||||
from graphify.export import backup_if_protected as _backup
|
||||
_backup(out)
|
||||
# Atomic replace via tmp file, matching the clustered path: a
|
||||
# crash mid-write must not leave a truncated graph.json.
|
||||
graph_tmp = out / ".graph.tmp.json"
|
||||
graph_tmp.write_text(candidate_graph_text, encoding="utf-8")
|
||||
graph_tmp.replace(existing_graph)
|
||||
|
||||
# Write the user-supplied path only after the candidate graph is
|
||||
# accepted, so a refused shrink cannot mismatch graph and marker.
|
||||
@@ -1314,6 +1366,20 @@ def _rebuild_code(
|
||||
try:
|
||||
check_graph_file_size_cap(existing_graph)
|
||||
existing_payload = json.loads(existing_graph.read_text(encoding="utf-8"))
|
||||
except Exception as exc:
|
||||
# A load failure is NOT "graph changed" (#2251): refuse to
|
||||
# overwrite a graph we merely failed to read. Normally
|
||||
# unreachable — the reconcile load above already failed
|
||||
# closed — but a race rewriting the file can land here.
|
||||
graph_tmp.unlink(missing_ok=True)
|
||||
print(
|
||||
f"error: Cannot read {existing_graph}: {exc}. "
|
||||
"Refusing to overwrite; delete the file and run a "
|
||||
"full rebuild.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return False
|
||||
try:
|
||||
same_graph = (
|
||||
json.dumps(_canonical_graph_for_compare(existing_payload), sort_keys=True, ensure_ascii=False)
|
||||
== json.dumps(_canonical_graph_for_compare(candidate_graph_data), sort_keys=True, ensure_ascii=False)
|
||||
|
||||
@@ -2281,3 +2281,124 @@ def test_rebuild_code_incremental_preserves_present_non_ast_source(tmp_path):
|
||||
assert "spec_concept" in after_ids, (
|
||||
"present-but-unextractable file in change set wrongly evicted as deleted (#2056)"
|
||||
)
|
||||
|
||||
|
||||
# --- #2251: fail-closed load of the existing graph ---
|
||||
|
||||
|
||||
def _seed_graph_with_semantic_layer(corpus: Path) -> Path:
|
||||
"""Build a real graph for one code file, then inject two semantic nodes
|
||||
(no _origin marker) sourced from a non-AST notes.txt, plus a semantic link.
|
||||
Returns the graph.json path."""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
corpus.mkdir()
|
||||
(corpus / "a.py").write_text("def alpha():\n return 1\n", encoding="utf-8")
|
||||
(corpus / "notes.txt").write_text("Design notes with a semantic layer.\n",
|
||||
encoding="utf-8")
|
||||
assert _rebuild_code(corpus, no_cluster=True, acquire_lock=False) is True
|
||||
graph_path = corpus / "graphify-out" / "graph.json"
|
||||
data = json.loads(graph_path.read_text(encoding="utf-8"))
|
||||
assert data["nodes"], "seed rebuild must produce AST code nodes"
|
||||
data["nodes"].extend([
|
||||
{"id": "notes_doc", "label": "Design Notes", "file_type": "document",
|
||||
"source_file": "notes.txt"},
|
||||
{"id": "notes_concept", "label": "Design Concept", "file_type": "concept",
|
||||
"source_file": "notes.txt"},
|
||||
])
|
||||
data["links"].append({
|
||||
"source": "notes_concept", "target": "notes_doc",
|
||||
"relation": "described_in", "confidence": "INFERRED",
|
||||
"source_file": "notes.txt",
|
||||
})
|
||||
graph_path.write_text(json.dumps(data), encoding="utf-8")
|
||||
return graph_path
|
||||
|
||||
|
||||
def test_rebuild_refuses_overwrite_when_existing_graph_over_size_cap(
|
||||
tmp_path, monkeypatch, capsys
|
||||
):
|
||||
"""#2251: an existing graph.json over GRAPHIFY_MAX_GRAPH_BYTES could not be
|
||||
READ, which is not license to overwrite it. The old code swallowed the cap
|
||||
ValueError, treated the baseline as empty, and collapsed the graph to
|
||||
code-only output."""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
corpus = tmp_path / "corpus"
|
||||
graph_path = _seed_graph_with_semantic_layer(corpus)
|
||||
before = graph_path.read_bytes()
|
||||
assert len(before) > 100
|
||||
|
||||
monkeypatch.setenv("GRAPHIFY_MAX_GRAPH_BYTES", "100")
|
||||
assert _rebuild_code(
|
||||
corpus, changed_paths=[Path("a.py")], no_cluster=True, acquire_lock=False,
|
||||
) is False
|
||||
assert graph_path.read_bytes() == before, (
|
||||
"graph.json must be byte-identical after a refused over-cap rebuild"
|
||||
)
|
||||
assert "error:" in capsys.readouterr().err
|
||||
|
||||
|
||||
@pytest.mark.parametrize("no_cluster", [True, False],
|
||||
ids=["no-cluster", "clustered"])
|
||||
def test_rebuild_refuses_overwrite_when_existing_graph_corrupt(
|
||||
tmp_path, capsys, no_cluster
|
||||
):
|
||||
"""#2251: unparseable graph.json (e.g. truncated by a crash) must fail the
|
||||
rebuild closed on both write paths, not be overwritten as if absent."""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
corpus = tmp_path / "corpus"
|
||||
graph_path = _seed_graph_with_semantic_layer(corpus)
|
||||
truncated = graph_path.read_text(encoding="utf-8")[:40]
|
||||
graph_path.write_text(truncated, encoding="utf-8")
|
||||
|
||||
assert _rebuild_code(
|
||||
corpus, changed_paths=[Path("a.py")],
|
||||
no_cluster=no_cluster, acquire_lock=False,
|
||||
) is False
|
||||
assert graph_path.read_text(encoding="utf-8") == truncated, (
|
||||
"corrupt graph.json must be left untouched for manual recovery"
|
||||
)
|
||||
assert "error:" in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_rebuild_force_does_not_clobber_unreadable_graph(tmp_path, capsys):
|
||||
"""#2251: --force means \"accept a shrink\", not \"overwrite a graph that
|
||||
could not be read\"."""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
corpus = tmp_path / "corpus"
|
||||
graph_path = _seed_graph_with_semantic_layer(corpus)
|
||||
truncated = graph_path.read_text(encoding="utf-8")[:40]
|
||||
graph_path.write_text(truncated, encoding="utf-8")
|
||||
|
||||
assert _rebuild_code(
|
||||
corpus, changed_paths=[Path("a.py")], force=True,
|
||||
no_cluster=True, acquire_lock=False,
|
||||
) is False
|
||||
assert graph_path.read_text(encoding="utf-8") == truncated
|
||||
assert "error:" in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_rebuild_readable_graph_still_preserves_semantic_nodes(tmp_path):
|
||||
"""Happy-path regression for the #2251 fix: a valid existing graph under the
|
||||
default cap still reconciles — the rebuild succeeds and the semantic layer
|
||||
survives an incremental code-only update."""
|
||||
from graphify.watch import _rebuild_code
|
||||
|
||||
corpus = tmp_path / "corpus"
|
||||
graph_path = _seed_graph_with_semantic_layer(corpus)
|
||||
|
||||
assert _rebuild_code(
|
||||
corpus, changed_paths=[Path("a.py")], no_cluster=True, acquire_lock=False,
|
||||
) is True
|
||||
after = json.loads(graph_path.read_text(encoding="utf-8"))
|
||||
after_ids = {n["id"] for n in after["nodes"]}
|
||||
assert {"notes_doc", "notes_concept"} <= after_ids, (
|
||||
"semantic nodes must survive an incremental rebuild with a readable graph"
|
||||
)
|
||||
assert any(
|
||||
e.get("source") == "notes_concept" and e.get("target") == "notes_doc"
|
||||
for e in after["links"]
|
||||
), "semantic link must survive an incremental rebuild"
|
||||
|
||||
Reference in New Issue
Block a user