mirror of
https://github.com/safishamsi/graphify.git
synced 2026-09-25 06:55:43 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
0f936850ba
commit
7d53a02704
+19
-5
@@ -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,
|
||||
)
|
||||
|
||||
+26
-9
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user