diff --git a/tests/skill/understand/test_merge_batch_graphs.py b/tests/skill/understand/test_merge_batch_graphs.py index 7239bfe6..4b5ecd4c 100644 --- a/tests/skill/understand/test_merge_batch_graphs.py +++ b/tests/skill/understand/test_merge_batch_graphs.py @@ -1072,9 +1072,11 @@ def test_stderr_report_format(self) -> None: class TestUnrecognizedBatchFilename(unittest.TestCase): """File-analyzer fuses multiple batches into one output (e.g., `batch-fused-8-13.json`, `batch-8-13.json`) — the merge script's regex - requires `batch-.json` or `batch--part-.json` and would - otherwise silently drop the contents. The script must warn loudly and - surface the drop in its report so the downstream review step catches it. + requires `batch-.json`, `batch--part-.json`, or + `batch-existing.json`. Anything else would historically have been + silently dropped (see #292), losing every node/edge in the misnamed + file. The script now hard-exits with a clear error so this kind of + silent data loss is impossible. """ def setUp(self) -> None: @@ -1105,9 +1107,9 @@ def _run_merge(self) -> tuple[int, str, dict]: assembled = _j.loads(out_path.read_text()) if out_path.exists() else {} return result.returncode, result.stderr, assembled - def test_fused_filename_emits_stderr_warning(self) -> None: + def test_fused_filename_hard_exits_with_clear_error(self) -> None: # `batch-fused-3-5.json` does not match the merge regex — - # script must warn on stderr (not silently drop). + # the script must abort, not silently drop the contents. self._write_batch("batch-1.json", [_file_node("src/a.ts")], []) self._write_batch("batch-2.json", [_file_node("src/b.ts")], []) self._write_batch( @@ -1115,36 +1117,24 @@ def test_fused_filename_emits_stderr_warning(self) -> None: [_file_node("src/c.ts"), _file_node("src/d.ts"), _file_node("src/e.ts")], [], ) - rc, stderr, _assembled = self._run_merge() - self.assertEqual(rc, 0) - self.assertIn("Warning: merge-batch-graphs:", stderr) - self.assertIn("unrecognized filenames", stderr) + rc, stderr, assembled = self._run_merge() + self.assertNotEqual(rc, 0) + self.assertIn("Error: merge-batch-graphs:", stderr) self.assertIn("batch-fused-3-5.json", stderr) # Remediation hint must be present so users know what to fix. self.assertIn("file-analyzer", stderr) self.assertIn("batch-.json", stderr) - - def test_fused_filename_surfaces_in_report(self) -> None: - # The merge report (printed after the per-file load lines) must - # also flag the drop so Phase 3 review picks it up. - self._write_batch("batch-1.json", [_file_node("src/a.ts")], []) - self._write_batch( - "batch-fused-2-4.json", [_file_node("src/x.ts")], [], - ) - rc, stderr, _assembled = self._run_merge() - self.assertEqual(rc, 0) - # "dropped N batch file(s) with unrecognized filenames" appears in the - # report section (printed after "Output: ..." line). - self.assertIn("dropped 1 batch file(s) with unrecognized filenames", stderr) - self.assertIn("batch-fused-2-4.json", stderr) - self.assertIn( - "every node/edge in these files was excluded from the final graph", - stderr, - ) - - def test_recognized_batches_still_loaded(self) -> None: - # With both recognized and unrecognized files present, recognized - # ones must still produce a valid assembled graph. + # New `batch-existing.json` filename must be in the remediation hint. + self.assertIn("batch-existing.json", stderr) + # No assembled-graph.json should be produced when the script aborts — + # callers should never see a partial merged graph for this failure. + self.assertEqual(assembled, {}) + + def test_no_partial_graph_written_on_hard_exit(self) -> None: + # Even when most batches are valid, a single unrecognized filename + # blocks the merge entirely. Producing a partial graph here is the + # exact bug #292 was about — silent data loss is worse than a loud + # failure that forces the user to investigate. self._write_batch("batch-1.json", [_file_node("src/a.ts")], []) self._write_batch("batch-2.json", [_file_node("src/b.ts")], []) self._write_batch( @@ -1153,20 +1143,15 @@ def test_recognized_batches_still_loaded(self) -> None: [], ) rc, _stderr, assembled = self._run_merge() - self.assertEqual(rc, 0) - node_ids = {n["id"] for n in assembled["nodes"]} - # batch-1 + batch-2 survive - self.assertIn("file:src/a.ts", node_ids) - self.assertIn("file:src/b.ts", node_ids) - # batch-fused-3-5.json content is excluded - self.assertNotIn("file:src/dropped-c.ts", node_ids) - self.assertEqual(node_ids, {"file:src/a.ts", "file:src/b.ts"}) + self.assertNotEqual(rc, 0) + # No assembled-graph.json written. + self.assertEqual(assembled, {}) def test_range_filename_also_unrecognized(self) -> None: # A bare range like `batch-8-13.json` is just as broken as # `batch-fused-8-13.json` — both must be flagged. The regex - # `batch-(\d+)(?:-part-(\d+))?\.json` requires the literal - # `-part-` separator before a second number. + # `batch-(\d+|existing)(?:-part-(\d+))?\.json` requires the + # literal `-part-` separator before a second number. self._write_batch("batch-1.json", [_file_node("src/a.ts")], []) self._write_batch( "batch-8-13.json", @@ -1174,13 +1159,144 @@ def test_range_filename_also_unrecognized(self) -> None: [], ) rc, stderr, assembled = self._run_merge() - self.assertEqual(rc, 0) - self.assertIn("Warning: merge-batch-graphs:", stderr) + self.assertNotEqual(rc, 0) + self.assertIn("Error: merge-batch-graphs:", stderr) self.assertIn("batch-8-13.json", stderr) - # Content is dropped + # No graph produced. + self.assertEqual(assembled, {}) + + +# ── batch-existing.json incremental-update handling ──────────────────────── + + +class TestBatchExistingFilename(unittest.TestCase): + """`batch-existing.json` is the filename SKILL.md's incremental update + path writes to carry the pruned existing graph (nodes/edges that + survived the prune for unchanged files). Historically the merge regex + didn't recognize it and silently dropped it — losing up to ~70% of the + graph (see #292). It must now be accepted alongside numeric batches. + """ + + def setUp(self) -> None: + import tempfile + self.tmp = Path(tempfile.mkdtemp(prefix="ua-mbg-existing-")) + self.intermediate = self.tmp / ".understand-anything" / "intermediate" + self.intermediate.mkdir(parents=True, exist_ok=True) + + def tearDown(self) -> None: + import shutil + shutil.rmtree(self.tmp, ignore_errors=True) + + def _write_batch(self, name: str, nodes: list, edges: list) -> None: + import json as _j + (self.intermediate / name).write_text( + _j.dumps({"nodes": nodes, "edges": edges}), + encoding="utf-8", + ) + + def _run_merge(self) -> tuple[int, str, dict]: + import subprocess + import json as _j + result = subprocess.run( + ["python3", str(_MODULE_PATH), str(self.tmp)], + capture_output=True, text=True, + ) + out_path = self.intermediate / "assembled-graph.json" + assembled = _j.loads(out_path.read_text()) if out_path.exists() else {} + return result.returncode, result.stderr, assembled + + def test_batch_existing_alongside_numeric_batch_is_merged(self) -> None: + # Incremental update: pruned existing nodes/edges go into + # batch-existing.json; fresh re-analysis of changed files goes into + # batch-0.json. Both must show up in the assembled graph. + self._write_batch( + "batch-existing.json", + [ + _file_node("src/unchanged-a.ts"), + _file_node("src/unchanged-b.ts"), + ], + [ + { + "source": "file:src/unchanged-a.ts", + "target": "file:src/unchanged-b.ts", + "type": "imports", + "direction": "forward", + "weight": 0.7, + } + ], + ) + self._write_batch( + "batch-0.json", + [_file_node("src/changed.ts")], + [], + ) + rc, stderr, assembled = self._run_merge() + self.assertEqual(rc, 0, f"merge should succeed, stderr was:\n{stderr}") node_ids = {n["id"] for n in assembled["nodes"]} - self.assertNotIn("file:src/x.ts", node_ids) - self.assertNotIn("file:src/y.ts", node_ids) + # All three nodes from both files survived the merge. + self.assertEqual( + node_ids, + { + "file:src/unchanged-a.ts", + "file:src/unchanged-b.ts", + "file:src/changed.ts", + }, + ) + # The cross-node edge from batch-existing.json is preserved. + edge_keys = { + (e["source"], e["target"], e["type"]) for e in assembled["edges"] + } + self.assertIn( + ( + "file:src/unchanged-a.ts", + "file:src/unchanged-b.ts", + "imports", + ), + edge_keys, + ) + + def test_batch_existing_alone_is_accepted(self) -> None: + # Edge case: a re-analysis where every changed file ended up + # producing zero new nodes still has `batch-existing.json` to merge. + self._write_batch( + "batch-existing.json", + [_file_node("src/keep.ts")], + [], + ) + rc, stderr, assembled = self._run_merge() + self.assertEqual(rc, 0, f"merge should succeed, stderr was:\n{stderr}") + node_ids = {n["id"] for n in assembled["nodes"]} + self.assertEqual(node_ids, {"file:src/keep.ts"}) + + def test_batch_existing_is_loaded_before_numeric_batches(self) -> None: + # Sort order matters for the "keep last" dedup at Step 5: when a + # node id appears in both `batch-existing.json` (the baseline) and + # in a freshly-analyzed numeric batch, the fresh version must win. + # batch-existing.json sorts first (synthetic index -1). + self._write_batch( + "batch-existing.json", + [_file_node("src/foo.ts", summary="stale baseline")], + [], + ) + self._write_batch( + "batch-0.json", + [_file_node("src/foo.ts", summary="fresh analysis")], + [], + ) + rc, stderr, assembled = self._run_merge() + self.assertEqual(rc, 0, f"merge should succeed, stderr was:\n{stderr}") + nodes = [n for n in assembled["nodes"] if n["id"] == "file:src/foo.ts"] + self.assertEqual(len(nodes), 1) + # Fresh batch wins (it was loaded after batch-existing.json so the + # "keep last" dedup keeps its copy). + self.assertEqual(nodes[0]["summary"], "fresh analysis") + # The per-file load order line must list batch-existing.json before + # batch-0.json so the precedence is observable from logs. + existing_idx = stderr.find("batch-existing.json") + zero_idx = stderr.find("batch-0.json") + self.assertGreaterEqual(existing_idx, 0) + self.assertGreaterEqual(zero_idx, 0) + self.assertLess(existing_idx, zero_idx) if __name__ == "__main__": diff --git a/understand-anything-plugin/skills/understand/merge-batch-graphs.py b/understand-anything-plugin/skills/understand/merge-batch-graphs.py index 2021f9ae..b44c065d 100644 --- a/understand-anything-plugin/skills/understand/merge-batch-graphs.py +++ b/understand-anything-plugin/skills/understand/merge-batch-graphs.py @@ -1012,30 +1012,49 @@ def main() -> None: print(f"Error: {intermediate_dir} does not exist", file=sys.stderr) sys.exit(1) - # Discover batch files, sorted by numeric index (not lexicographic) - batch_files = sorted( - intermediate_dir.glob("batch-*.json"), - key=lambda p: int(re.search(r"batch-(\d+)", p.stem).group(1)) - if re.search(r"batch-(\d+)", p.stem) - else 0, - ) + # Discover batch files, sorted deterministically: + # 1. `batch-existing.json` (incremental update payload) sorts first + # with a synthetic index of -1, so pruned baseline nodes/edges are + # seen before any freshly-analyzed batch. Combined with Step 5's + # "keep last" dedup, this means a fresh batch always wins over the + # existing-graph copy of the same node ID (incremental re-analysis + # semantics). + # 2. Numeric batches sort by their integer index. + # 3. Per-batch parts (`batch--part-.json`) are tied with their + # sibling parts at the same index and fall back to lexicographic + # order on the filename — stable enough for the dedup step that + # runs downstream. + def _batch_sort_key(p: Path) -> tuple[int, str]: + m = re.match(r"batch-(\d+|existing)(?:-part-(\d+))?\.json", p.name) + if m is None: + return (sys.maxsize, p.name) # unrecognized — sort last (will be rejected below) + idx_str = m.group(1) + idx = -1 if idx_str == "existing" else int(idx_str) + return (idx, p.name) + + batch_files = sorted(intermediate_dir.glob("batch-*.json"), key=_batch_sort_key) if not batch_files: print("Error: no batch-*.json files found in intermediate/", file=sys.stderr) sys.exit(1) # Group by logical batch index so the report distinguishes single-batch # files from multi-part file-analyzer outputs. Files that don't match the - # `batch-.json` / `batch--part-.json` pattern (e.g. fused - # `batch-fused-8-13.json`, range `batch-8-13.json`) would otherwise be - # silently dropped during load — flag them loudly instead so the user - # can fix the file-analyzer agent. + # `batch-.json` / `batch--part-.json` / `batch-existing.json` + # pattern (e.g. fused `batch-fused-8-13.json`, range `batch-8-13.json`) + # would otherwise be silently dropped during load. We now fail loudly + # and exit non-zero so this kind of silent data loss is impossible + # (see #292). from collections import defaultdict as _dd by_batch = _dd(list) unrecognized_batch_files: list[str] = [] for f in batch_files: - m = re.match(r"batch-(\d+)(?:-part-(\d+))?\.json", f.name) + m = re.match(r"batch-(\d+|existing)(?:-part-(\d+))?\.json", f.name) if m: - by_batch[int(m.group(1))].append((f.name, int(m.group(2)) if m.group(2) else None)) + idx_str = m.group(1) + # Use -1 as the logical index for `batch-existing.json` so it + # groups separately from numeric batches in `by_batch`. + idx = -1 if idx_str == "existing" else int(idx_str) + by_batch[idx].append((f.name, int(m.group(2)) if m.group(2) else None)) else: unrecognized_batch_files.append(f.name) @@ -1046,13 +1065,21 @@ def main() -> None: if len(unrecognized_batch_files) > 5 else "" ) + # Hard error (was: silent drop with stderr warning that callers + # often missed, see #292). Failing loud means a misnamed file — + # including the previously-broken `batch-existing.json` path that + # SKILL.md instructs incremental updates to write — can never + # silently strip ~70% of the graph again. print( - f"Warning: merge-batch-graphs: {len(unrecognized_batch_files)} " - f"batch file(s) with unrecognized filenames will be DROPPED — " + f"Error: merge-batch-graphs: {len(unrecognized_batch_files)} " + f"batch file(s) with unrecognized filenames would be DROPPED — " f"files: {preview}{suffix} — fix the file-analyzer agent to use " - f"only batch-.json or batch--part-.json patterns", + f"only batch-.json, batch--part-.json, or " + f"batch-existing.json patterns. Refusing to produce a " + f"partial merged graph.", file=sys.stderr, ) + sys.exit(1) logical_count = len(by_batch) multi_part = sum(1 for entries in by_batch.values() if len(entries) > 1) @@ -1084,13 +1111,10 @@ def main() -> None: print(f"Warning: merge: {msg}", file=sys.stderr) missing_part_warnings.append(msg) - # Load batches — skip unrecognized filenames so they don't pollute the - # merged graph with content the agent labeled incorrectly. - unrecognized_set = set(unrecognized_batch_files) + # Load batches. We're past the unrecognized-filename check (which now + # hard-exits), so every file here matches the recognized pattern. batches: list[dict[str, Any]] = [] for f in batch_files: - if f.name in unrecognized_set: - continue batch = load_batch(f) if batch is not None: batches.append(batch) @@ -1105,11 +1129,14 @@ def main() -> None: # Merge and normalize assembled, report = merge_and_normalize(batches) - # Surface missing multi-part files to the phase report (parallel to - # unrecognized-filename handling below). Stderr lines emitted during - # batch discovery get buried under per-batch load output — re-emitting - # via the report list ensures the Phase 4 review and final summary see - # the data-loss signal. + # Surface missing multi-part files to the phase report. Stderr lines + # emitted during batch discovery get buried under per-batch load output — + # re-emitting via the report list ensures the Phase 4 review and final + # summary see the data-loss signal. + # + # (Unrecognized-filename drops used to be surfaced here too. They now + # hard-exit before this point — see #292 — so there's nothing left to + # surface.) if missing_part_warnings: report.append("") report.append( @@ -1119,24 +1146,6 @@ def main() -> None: for w in missing_part_warnings: report.append(f" - {w}") - # Surface unrecognized-filename drops to the phase report so the - # downstream review step sees them, not just stderr. - if unrecognized_batch_files: - preview = ", ".join(unrecognized_batch_files[:5]) - suffix = ( - f" (+{len(unrecognized_batch_files) - 5} more)" - if len(unrecognized_batch_files) > 5 - else "" - ) - report.append("") - report.append( - f"Warning: dropped {len(unrecognized_batch_files)} batch file(s) " - f"with unrecognized filenames — files: {preview}{suffix} — " - f"fix the file-analyzer agent to use only batch-.json or " - f"batch--part-.json patterns (every node/edge in these " - f"files was excluded from the final graph)" - ) - # Recover any imports edges file-analyzer batches dropped despite # `batchImportData` containing them. The project-scanner's importMap # is the deterministic source of truth.