diff --git a/graphify/cli.py b/graphify/cli.py index 5d13a47e..d38fda28 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -3158,12 +3158,15 @@ def dispatch_command(cmd: str) -> None: chunk_files.extend(sorted(expanded) if expanded else [arg]) merged: dict = {"nodes": [], "edges": [], "hyperedges": [], "input_tokens": 0, "output_tokens": 0} seen_ids: set[str] = set() + valid_chunks = 0 # These chunk files are untrusted subagent output. load_validated_... # stats the file size BEFORE reading it (so a multi-GB chunk can't blow up # memory), parses the JSON, and validates the security caps + the node/ # edge id charset that blocks path traversal (#825) — the same enforcement # the skill merge path applies. A bad chunk is skipped with a warning - # (filter semantics; never abort). Deliberately NOT wired into + # while valid siblings still merge; if every chunk is invalid, fail + # closed instead of reporting success and replacing --out with an empty + # semantic layer. Deliberately NOT wired into # build_from_json/load_graph_json, which must keep loading valid # pre-existing graphs. file_type is left to build's coercion (#840). from graphify.semantic_cleanup import load_validated_semantic_fragment @@ -3176,6 +3179,7 @@ def dispatch_command(cmd: str) -> None: file=sys.stderr, ) continue + valid_chunks += 1 for n in chunk.get("nodes", []): if n.get("id") not in seen_ids: seen_ids.add(n["id"]) @@ -3188,11 +3192,23 @@ def dispatch_command(cmd: str) -> None: for _tok in ("input_tokens", "output_tokens"): _v = chunk.get(_tok, 0) merged[_tok] += _v if isinstance(_v, (int, float)) else 0 + if not valid_chunks: + print( + f"[graphify merge-chunks] error: no valid chunks to merge; " + f"refusing to write {out_path}", + file=sys.stderr, + ) + sys.exit(1) out_path.parent.mkdir(parents=True, exist_ok=True) from graphify.paths import write_json_atomic as _wja _wja(out_path, merged, ensure_ascii=False) + chunk_summary = ( + f"{valid_chunks} chunks" + if valid_chunks == len(chunk_files) + else f"{valid_chunks} of {len(chunk_files)} chunks" + ) print( - f"Merged {len(chunk_files)} chunks: {len(merged['nodes'])} nodes, {len(merged['edges'])} edges, " + f"Merged {chunk_summary}: {len(merged['nodes'])} nodes, {len(merged['edges'])} edges, " f"{merged['input_tokens']:,} in / {merged['output_tokens']:,} out tokens" ) diff --git a/tests/test_merge_chunks_validation.py b/tests/test_merge_chunks_validation.py index ca9f5eab..b2d53259 100644 --- a/tests/test_merge_chunks_validation.py +++ b/tests/test_merge_chunks_validation.py @@ -3,11 +3,12 @@ merge-chunks concatenates agent-written `.graphify_chunk_*.json` files. Those are untrusted output, so each is run through `validate_semantic_fragment` (caps + the node/edge ID charset that blocks path-escape). An invalid chunk is skipped with a -warning; valid chunks still merge. +warning; valid chunks still merge, but an all-invalid input set fails closed. """ import json import graphify.__main__ as mainmod +import pytest def _write(path, obj): @@ -32,19 +33,64 @@ def test_merge_chunks_skips_chunk_with_path_escape_id(tmp_path, monkeypatch, cap merged = json.loads(out.read_text()) assert {n["id"] for n in merged["nodes"]} == {"pkg.mod.good"} - assert "skipping invalid chunk" in capsys.readouterr().err + captured = capsys.readouterr() + assert "skipping invalid chunk" in captured.err + assert "Merged 1 of 2 chunks" in captured.out -def test_merge_chunks_skips_malformed_shape(tmp_path, monkeypatch, capsys): +def test_merge_chunks_fails_closed_when_every_chunk_is_invalid(tmp_path, monkeypatch, capsys): bad = tmp_path / ".graphify_chunk_0.json" _write(bad, {"nodes": "not-a-list", "edges": []}) out = tmp_path / "merged.json" + out.write_text('{"previous": "semantic result"}', encoding="utf-8") - _run_merge(monkeypatch, ["graphify", "merge-chunks", str(bad), "--out", str(out)]) + with pytest.raises(SystemExit) as exc: + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(bad), "--out", str(out)]) + + assert exc.value.code == 1 + assert json.loads(out.read_text()) == {"previous": "semantic result"} + err = capsys.readouterr().err + assert "skipping invalid chunk" in err + assert "no valid chunks to merge" in err + + +def test_merge_chunks_accepts_valid_empty_chunk(tmp_path, monkeypatch): + """A valid fragment may legitimately contain no entities; it still counts.""" + empty = tmp_path / ".graphify_chunk_0.json" + _write(empty, {"nodes": [], "edges": [], "hyperedges": []}) + out = tmp_path / "merged.json" + + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(empty), "--out", str(out)]) merged = json.loads(out.read_text()) assert merged["nodes"] == [] - assert "skipping invalid chunk" in capsys.readouterr().err + assert merged["edges"] == [] + + +def test_merge_chunks_fails_closed_without_chunk_arguments(tmp_path, monkeypatch, capsys): + out = tmp_path / "merged.json" + + with pytest.raises(SystemExit) as exc: + _run_merge(monkeypatch, ["graphify", "merge-chunks", "--out", str(out)]) + + assert exc.value.code == 1 + assert not out.exists() + assert "no valid chunks to merge" in capsys.readouterr().err + + +def test_merge_chunks_fails_closed_on_unmatched_glob(tmp_path, monkeypatch, capsys): + out = tmp_path / "merged.json" + out.write_text('{"previous": true}', encoding="utf-8") + unmatched = str(tmp_path / ".graphify_chunk_*.json") + + with pytest.raises(SystemExit) as exc: + _run_merge(monkeypatch, ["graphify", "merge-chunks", unmatched, "--out", str(out)]) + + assert exc.value.code == 1 + assert json.loads(out.read_text()) == {"previous": True} + err = capsys.readouterr().err + assert "skipping invalid chunk" in err + assert "no valid chunks to merge" in err def test_merge_chunks_accepts_synonym_file_type(tmp_path, monkeypatch):