From 7d53a02704379826d948915fae443a582be6de63 Mon Sep 17 00:00:00 2001 From: safishamsi Date: Thu, 16 Jul 2026 23:42:47 +0100 Subject: [PATCH] fix(extract): fail closed on malformed existing graph + honor walk_errors (follow-up to #1951) Two gaps the review found in the incomplete-build shrink guard: - existing_graph_node_count() returned None ("proceed") on a present-but- unparseable graph.json, so the --no-cluster path could clobber a complete graph whose file was corrupt/mid-write. It now returns a MALFORMED_GRAPH sentinel and the caller fails closed, matching to_json's #479 handling. - A walk that couldn't fully enumerate the corpus (permission-denied subtree, I/O error) is now treated as an incomplete extraction: detect()/ detect_incremental() already record walk_errors; the extract path consumes them so a walk-truncated graph can't force-overwrite a complete one. Co-Authored-By: Claude Opus 4.8 (1M context) --- graphify/cli.py | 24 +++++++++++++++---- graphify/export.py | 35 +++++++++++++++++++++------- tests/test_export.py | 12 ++++++---- tests/test_incomplete_build_guard.py | 19 +++++++++++++++ 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/graphify/cli.py b/graphify/cli.py index d373454a7..e499ba809 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -2527,6 +2527,12 @@ def dispatch_command(cmd: str) -> None: # be force-written over a good complete graph — the final write falls back # to the #479 shrink guard unless --allow-partial is set. _extraction_incomplete = False + # A walk that couldn't fully enumerate the corpus (permission-denied + # subtree, I/O error) yields a legitimately smaller graph that must not + # be force-written over a complete one — same failure class as a crashed + # pass. detect()/detect_incremental() already record these; consume them. + if detection.get("walk_errors"): + _extraction_incomplete = True # AST extraction on code files. Empty code list (docs-only corpus) is # the issue #698 case — skip cleanly instead of crashing inside extract(). @@ -2817,14 +2823,22 @@ def dispatch_command(cmd: str) -> None: # to_json, so replicate the shrink check against the existing file and # exit before the write/manifest unless --allow-partial is set. if _extraction_incomplete and not cli_allow_partial: + from graphify.export import MALFORMED_GRAPH as _MALFORMED_GRAPH _existing_n = _existing_graph_node_count(graph_json_path) - if _existing_n is not None and len(merged["nodes"]) < _existing_n: + _malformed = _existing_n is _MALFORMED_GRAPH + _shrinks = isinstance(_existing_n, int) and len(merged["nodes"]) < _existing_n + if _malformed or _shrinks: + _detail = ( + f"the existing {graph_json_path} is present but unparseable " + "(corrupt or a mid-write), so a shrink cannot be ruled out" + if _malformed + else f"smaller than the existing {graph_json_path} " + f"({len(merged['nodes'])} < {_existing_n} nodes)" + ) print( "[graphify extract] error: extraction was incomplete (an AST/" - "semantic pass failed) and the resulting --no-cluster graph is " - f"smaller than the existing {graph_json_path} " - f"({len(merged['nodes'])} < {_existing_n} nodes). Refusing to " - "overwrite a complete graph with a partial one. Re-run after " + f"semantic pass failed) and the resulting --no-cluster graph is {_detail}. " + "Refusing to overwrite a complete graph with a partial one. Re-run after " "fixing the failures, or pass --allow-partial to overwrite anyway.", file=sys.stderr, ) diff --git a/graphify/export.py b/graphify/export.py index 39ee2ac4f..d6e982565 100644 --- a/graphify/export.py +++ b/graphify/export.py @@ -180,14 +180,27 @@ def _git_head() -> str | None: return None -def existing_graph_node_count(path: "str | Path") -> int | None: - """Node count of an existing graph.json, or None when it can't be compared - (absent, empty, unreadable, malformed, or over the size cap). +# Sentinel: an existing graph.json is present and non-empty but cannot be parsed +# into a node count (corrupt, mid-write, or structurally wrong). The caller must +# fail CLOSED on this — the same way to_json's #479 guard refuses to overwrite +# such a file — because we cannot prove the new graph isn't a silent shrink. +MALFORMED_GRAPH = object() + + +def existing_graph_node_count(path: "str | Path"): + """Node count of an existing graph.json. + + Returns: + - an ``int`` node count when the file parses; + - ``None`` when there is verifiably nothing to protect — absent, empty, or + over the size cap (matching how :func:`to_json` lets the new graph + replace an empty/oversized file); + - :data:`MALFORMED_GRAPH` when the file is present and non-empty but + unparseable — the caller must treat this as fail-closed (refuse to + overwrite), mirroring to_json's #479 handling of a corrupt/mid-write file. The raw ``--no-cluster`` write path uses this to apply the same #479 shrink - guard that :func:`to_json` applies inline for the clustered path. None means - "can't verify — let the write proceed", matching how ``to_json`` treats an - empty/oversized/unreadable existing file. + guard that :func:`to_json` applies inline for the clustered path. """ p = Path(path) if not p.exists(): @@ -201,15 +214,19 @@ def existing_graph_node_count(path: "str | Path") -> int | None: try: raw = p.read_text(encoding="utf-8") except Exception: - return None + # Present but unreadable: fail closed if it has bytes, else nothing to lose. + try: + return MALFORMED_GRAPH if p.stat().st_size > 0 else None + except Exception: + return None if not raw.strip(): return None try: data = json.loads(raw) except Exception: - return None + return MALFORMED_GRAPH nodes = data.get("nodes") if isinstance(data, dict) else None - return len(nodes) if isinstance(nodes, list) else None + return len(nodes) if isinstance(nodes, list) else MALFORMED_GRAPH def to_json(G: nx.Graph, communities: dict[int, list[str]], output_path: str, *, force: bool = False, built_at_commit: str | None = None, community_labels: dict[int, str] | None = None) -> bool: diff --git a/tests/test_export.py b/tests/test_export.py index fa43dd7ee..28a4707a7 100644 --- a/tests/test_export.py +++ b/tests/test_export.py @@ -778,12 +778,16 @@ def test_to_html_handles_null_source_file_and_label(tmp_path): def test_existing_graph_node_count(tmp_path): - from graphify.export import existing_graph_node_count + from graphify.export import existing_graph_node_count, MALFORMED_GRAPH p = tmp_path / "graph.json" - assert existing_graph_node_count(p) is None # absent + assert existing_graph_node_count(p) is None # absent -> nothing to protect p.write_text("", encoding="utf-8") - assert existing_graph_node_count(p) is None # empty + assert existing_graph_node_count(p) is None # empty -> nothing to protect + # Non-empty but unparseable must fail CLOSED (sentinel), matching to_json's + # #479 guard — a corrupt/mid-write file could be hiding a complete graph. p.write_text("{not json", encoding="utf-8") - assert existing_graph_node_count(p) is None # malformed + assert existing_graph_node_count(p) is MALFORMED_GRAPH # malformed -> fail closed + p.write_text('{"nodes": "notalist"}', encoding="utf-8") + assert existing_graph_node_count(p) is MALFORMED_GRAPH # structurally wrong -> fail closed p.write_text('{"nodes": [{"id": "a"}, {"id": "b"}], "links": []}', encoding="utf-8") assert existing_graph_node_count(p) == 2 # valid diff --git a/tests/test_incomplete_build_guard.py b/tests/test_incomplete_build_guard.py index cec5c922d..8c05c39e2 100644 --- a/tests/test_incomplete_build_guard.py +++ b/tests/test_incomplete_build_guard.py @@ -167,3 +167,22 @@ def test_no_cluster_allow_partial_overwrites(tmp_path, monkeypatch): assert exc.value.code == 0 # the raw --no-cluster path exits 0 on success assert len(json.loads(graph.read_text())["nodes"]) == 1 + + +def test_no_cluster_incomplete_build_fails_closed_on_malformed_existing_graph( + tmp_path, monkeypatch, capsys +): + """A present-but-unparseable existing graph.json (corrupt or mid-write) could + be hiding a complete graph, so an incomplete --no-cluster build must refuse + to overwrite it — matching to_json's #479 fail-closed handling, not the + fail-open 'proceed when we can't count' path.""" + graph = _arm_no_cluster(monkeypatch, tmp_path) + graph.write_text("{corrupt json", encoding="utf-8") # non-empty, unparseable + + with pytest.raises(SystemExit) as exc: + mainmod.main() + + assert exc.value.code == 1 + assert "unparseable" in capsys.readouterr().err + # The corrupt file is left untouched rather than clobbered by the partial build. + assert graph.read_text() == "{corrupt json"