diff --git a/graphify/watch.py b/graphify/watch.py index 45bd97b6..c87ec8e6 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -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) diff --git a/tests/test_watch.py b/tests/test_watch.py index f2bec415..1b2efce3 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -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"