diff --git a/benchmarks/diff_review_bench.py b/benchmarks/diff_review_bench.py new file mode 100644 index 000000000..944c8f229 --- /dev/null +++ b/benchmarks/diff_review_bench.py @@ -0,0 +1,198 @@ +"""ASV benchmarks for diff review artifact building and serialization. + +Measures the performance of: +- DiffBuilder.build() with various changeset sizes +- DiffSerializer.to_plain() / to_json() / to_rich() +- Rename detection with content hash matching +- ReviewArtifact and DiffEntry construction +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +try: + from cleveragents.domain.models.core.change import ( + ChangeEntry, + ChangeOperation, + SpecChangeSet, + ) + from cleveragents.domain.models.core.diff_review import ( + DiffBuilder, + DiffEntry, + DiffSerializer, + ResourceDiffGroup, + ReviewArtifact, + ) +except ModuleNotFoundError: + sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) + from cleveragents.domain.models.core.change import ( + ChangeEntry, + ChangeOperation, + SpecChangeSet, + ) + from cleveragents.domain.models.core.diff_review import ( + DiffBuilder, + DiffEntry, + DiffSerializer, + ResourceDiffGroup, + ReviewArtifact, + ) + + +def _make_changeset( + n_entries: int = 10, +) -> tuple[SpecChangeSet, dict[str, str], dict[str, str]]: + """Create a changeset with n_entries MODIFY entries and content maps.""" + cs = SpecChangeSet(plan_id="BENCH-PLAN") + before_map: dict[str, str] = {} + after_map: dict[str, str] = {} + for i in range(n_entries): + path = f"src/module_{i:04d}.py" + cs.add_change( + ChangeEntry( + plan_id="BENCH-PLAN", + resource_id=f"RES{i % 5:03d}", + tool_name="builtin/bench-tool", + operation=ChangeOperation.MODIFY, + path=path, + before_hash=f"before-{i}", + after_hash=f"after-{i}", + ) + ) + before_map[path] = f"# module {i}\nx = {i}\n" + after_map[path] = f"# module {i}\nx = {i + 1}\ny = {i}\n" + return cs, before_map, after_map + + +class DiffBuildSuite: + """Benchmark DiffBuilder.build() with various changeset sizes.""" + + def setup(self) -> None: + """Prepare changesets of different sizes.""" + self.builder = DiffBuilder() + self.cs_small, self.bm_small, self.am_small = _make_changeset(5) + self.cs_medium, self.bm_medium, self.am_medium = _make_changeset(50) + self.cs_large, self.bm_large, self.am_large = _make_changeset(200) + + def time_build_small(self) -> None: + """Benchmark building artifact from 5-entry changeset.""" + self.builder.build( + self.cs_small, + before_contents=self.bm_small, + after_contents=self.am_small, + ) + + def time_build_medium(self) -> None: + """Benchmark building artifact from 50-entry changeset.""" + self.builder.build( + self.cs_medium, + before_contents=self.bm_medium, + after_contents=self.am_medium, + ) + + def time_build_large(self) -> None: + """Benchmark building artifact from 200-entry changeset.""" + self.builder.build( + self.cs_large, + before_contents=self.bm_large, + after_contents=self.am_large, + ) + + +class DiffSerializationSuite: + """Benchmark DiffSerializer output formats.""" + + def setup(self) -> None: + """Build an artifact to serialize.""" + cs, bm, am = _make_changeset(20) + builder = DiffBuilder() + self.artifact = builder.build(cs, before_contents=bm, after_contents=am) + + def time_to_plain(self) -> None: + """Benchmark plain text serialization.""" + DiffSerializer.to_plain(self.artifact) + + def time_to_json(self) -> None: + """Benchmark JSON serialization.""" + DiffSerializer.to_json(self.artifact) + + def time_to_rich(self) -> None: + """Benchmark rich text serialization.""" + DiffSerializer.to_rich(self.artifact) + + +class DiffRenameSuite: + """Benchmark rename detection with content hash matching.""" + + def setup(self) -> None: + """Prepare a changeset with rename pairs.""" + self.builder = DiffBuilder() + self.cs = SpecChangeSet(plan_id="BENCH-PLAN") + for i in range(20): + self.cs.add_change( + ChangeEntry( + plan_id="BENCH-PLAN", + resource_id="RES001", + tool_name="builtin/bench-tool", + operation=ChangeOperation.DELETE, + path=f"old/module_{i:04d}.py", + before_hash=f"hash-{i}", + ) + ) + self.cs.add_change( + ChangeEntry( + plan_id="BENCH-PLAN", + resource_id="RES001", + tool_name="builtin/bench-tool", + operation=ChangeOperation.CREATE, + path=f"new/module_{i:04d}.py", + after_hash=f"hash-{i}", + ) + ) + + def time_rename_detection(self) -> None: + """Benchmark rename detection for 20 pairs.""" + self.builder.build(self.cs) + + +class DiffModelSuite: + """Benchmark DiffEntry and ReviewArtifact construction.""" + + def time_diff_entry_construction(self) -> None: + """Benchmark creating a DiffEntry.""" + DiffEntry( + resource_id="RES001", + resource_name="test-resource", + path="src/main.py", + change_type="modify", + diff_text="--- a/src/main.py\n+++ b/src/main.py\n", + ) + + def time_resource_diff_group_construction(self) -> None: + """Benchmark creating a ResourceDiffGroup.""" + ResourceDiffGroup( + resource_id="RES001", + resource_name="test-resource", + entries=[ + DiffEntry( + resource_id="RES001", + path="a.py", + change_type="create", + ), + DiffEntry( + resource_id="RES001", + path="b.py", + change_type="modify", + ), + ], + ) + + def time_review_artifact_construction(self) -> None: + """Benchmark creating a ReviewArtifact.""" + ReviewArtifact( + plan_id="PLAN001", + changeset_id="CS001", + total_changes=5, + ) diff --git a/docs/reference/diff_review.md b/docs/reference/diff_review.md new file mode 100644 index 000000000..dfeb2fc27 --- /dev/null +++ b/docs/reference/diff_review.md @@ -0,0 +1,87 @@ +# Diff Review + +The diff review module builds reviewable diff artifacts from `SpecChangeSet` +records, grouping changes by resource and producing unified diff text for +CLI display. + +## Overview + +When a plan accumulates changes through tool executions, the diff review +system transforms the raw `ChangeEntry` records into a structured +`ReviewArtifact` suitable for human review before applying changes. + +## Domain Models + +### DiffEntry + +A single diff entry for CLI review. Contains: + +| Field | Type | Description | +|---|---|---| +| `resource_id` | `str` | Resource ULID | +| `resource_name` | `str` | Human-readable resource name | +| `path` | `str` | File path | +| `change_type` | `str` | Operation type (create/modify/delete/rename) | +| `before_content` | `str \| None` | Content before the change | +| `after_content` | `str \| None` | Content after the change | +| `before_hash` | `str \| None` | SHA-256 before | +| `after_hash` | `str \| None` | SHA-256 after | +| `file_mode` | `int \| None` | File permission mode | +| `is_binary` | `bool` | Whether the file is binary | +| `is_rename` | `bool` | Whether this is a rename/move | +| `rename_from` | `str \| None` | Original path for renames | +| `rename_to` | `str \| None` | Destination path for renames | +| `diff_text` | `str` | Unified diff text | +| `truncated` | `bool` | Whether the diff text was truncated | + +### ResourceDiffGroup + +Groups diff entries for a single resource. Provides summary counts +via properties: `added`, `modified`, `deleted`, `renamed`. + +### ReviewArtifact + +Complete diff review artifact for a plan. Contains grouped diff entries, +total change count, and truncation status. + +## DiffBuilder + +`DiffBuilder` takes a `SpecChangeSet` and optional content maps to produce +a `ReviewArtifact`. + +```python +from cleveragents.domain.models.core.diff_review import DiffBuilder + +builder = DiffBuilder(max_diff_size=50_000, context_lines=3) +artifact = builder.build( + changeset, + before_contents={"src/main.py": "old content"}, + after_contents={"src/main.py": "new content"}, + resource_names={"RES001": "my-resource"}, +) +``` + +### Features + +- **Unified diff generation** using `difflib.unified_diff` +- **Binary detection** via null byte checking +- **Rename detection** using `SequenceMatcher` path similarity (>0.6) + combined with matching content hashes +- **Truncation** when total diff text exceeds `max_diff_size` (applies + uniformly to all entry types including renames) +- **Stable ordering** by resource ID, then path within each group + +## DiffSerializer + +Renders a `ReviewArtifact` to various output formats: + +- `to_plain(artifact)` — plain text for terminal output +- `to_json(artifact)` — structured JSON for programmatic use +- `to_rich(artifact)` — rich markup with `[bold]`, `[green]`, `[red]` tags + +## Constants + +| Name | Default | Description | +|---|---|---| +| `DEFAULT_MAX_DIFF_SIZE` | 50,000 | Maximum total diff text characters | +| `RENAME_SIMILARITY_THRESHOLD` | 0.6 | Minimum path similarity for rename detection | diff --git a/features/diff_review.feature b/features/diff_review.feature new file mode 100644 index 000000000..22f9b4893 --- /dev/null +++ b/features/diff_review.feature @@ -0,0 +1,263 @@ +Feature: Diff review artifacts + As a developer reviewing plan changes + I want to build diff review artifacts from changesets + So that I can inspect changes before applying them + + Background: + Given a plan ID "01PLAN000000000000000001" + And an empty changeset for the plan + + Scenario: Empty changeset produces empty review artifact + When I build a review artifact from the changeset + Then the review artifact should have 0 total changes + And the review artifact should have 0 groups + And the review artifact should not be truncated + + Scenario: Single CREATE change produces a diff entry + Given a change entry with operation "create" for path "src/main.py" in resource "RES001" + And after content for "src/main.py" is "print('hello')\n" + When I build a review artifact from the changeset + Then the review artifact should have 1 total changes + And the review artifact should have 1 groups + And group 0 should have 1 entries + And entry 0 in group 0 should have change type "create" + And entry 0 in group 0 should have path "src/main.py" + + Scenario: MODIFY change generates unified diff + Given a change entry with operation "modify" for path "src/app.py" in resource "RES001" + And before content for "src/app.py" is "x = 1\ny = 2\n" + And after content for "src/app.py" is "x = 1\ny = 3\n" + When I build a review artifact from the changeset + Then entry 0 in group 0 should have change type "modify" + And the diff text in entry 0 of group 0 should contain "---" + And the diff text in entry 0 of group 0 should contain "+++" + + Scenario: DELETE change entry + Given a change entry with operation "delete" for path "old.txt" in resource "RES001" + And before content for "old.txt" is "old content\n" + When I build a review artifact from the changeset + Then entry 0 in group 0 should have change type "delete" + And the diff text in entry 0 of group 0 should contain "---" + + Scenario: Multiple changes across different resources + Given a change entry with operation "create" for path "a.py" in resource "RES001" + And a change entry with operation "modify" for path "b.py" in resource "RES002" + And after content for "a.py" is "new\n" + And before content for "b.py" is "old\n" + And after content for "b.py" is "new\n" + When I build a review artifact from the changeset + Then the review artifact should have 2 total changes + And the review artifact should have 2 groups + + Scenario: Changes grouped by resource + Given a change entry with operation "create" for path "a.py" in resource "RES001" + And a change entry with operation "modify" for path "b.py" in resource "RES001" + And after content for "a.py" is "new\n" + And before content for "b.py" is "old\n" + And after content for "b.py" is "new\n" + When I build a review artifact from the changeset + Then the review artifact should have 1 groups + And group 0 should have 2 entries + + Scenario: Binary file detection + Given a change entry with operation "create" for path "image.bin" in resource "RES001" + And after content for "image.bin" is "binary\x00data" + When I build a review artifact from the changeset + Then entry 0 in group 0 should be binary + And the diff text in entry 0 of group 0 should contain "[binary file]" + + Scenario: Rename detection with matching content hash + Given a delete entry for path "old/file.py" in resource "RES001" with before_hash "abc123" + And a create entry for path "new/file.py" in resource "RES001" with after_hash "abc123" + When I build a review artifact from the changeset + Then the review artifact should have 1 groups + And group 0 should have 1 entries + And entry 0 in group 0 should be a rename + And entry 0 in group 0 rename should be from "old/file.py" to "new/file.py" + And group 0 should have 1 renamed + + Scenario: Rename not detected when hashes differ + Given a delete entry for path "old/file.py" in resource "RES001" with before_hash "abc123" + And a create entry for path "new/file.py" in resource "RES001" with after_hash "xyz789" + When I build a review artifact from the changeset + Then group 0 should have 2 entries + And entry 0 in group 0 should not be a rename + + Scenario: Max diff size truncation + Given a change entry with operation "create" for path "big.py" in resource "RES001" + And after content for "big.py" is a string of 100000 characters + When I build a review artifact with max diff size 100 + Then the review artifact should be truncated + And entry 0 in group 0 should be truncated + + Scenario: Stable ordering by resource then path + Given a change entry with operation "create" for path "z.py" in resource "RES002" + And a change entry with operation "create" for path "a.py" in resource "RES001" + And after content for "z.py" is "z\n" + And after content for "a.py" is "a\n" + When I build a review artifact from the changeset + Then group 0 should have resource id "RES001" + And group 1 should have resource id "RES002" + + Scenario: Before and after content hashes in metadata + Given a hashed change entry "modify" path "f.py" resource "RES001" before_hash "hash1" after_hash "hash2" + And before content for "f.py" is "old\n" + And after content for "f.py" is "new\n" + When I build a review artifact from the changeset + Then entry 0 in group 0 should have before hash "hash1" + And entry 0 in group 0 should have after hash "hash2" + + Scenario: File mode in metadata + Given a moded change entry "modify" path "script.sh" resource "RES001" after_mode 0o755 + And before content for "script.sh" is "#!/bin/sh\n" + And after content for "script.sh" is "#!/bin/bash\n" + When I build a review artifact from the changeset + Then entry 0 in group 0 should have file mode 493 + + Scenario: Serialize to plain text + Given a change entry with operation "create" for path "new.py" in resource "RES001" + And after content for "new.py" is "hello\n" + And resource name "RES001" is "my-resource" + When I build a review artifact from the changeset + And I serialize the artifact to plain text + Then the plain text should contain "my-resource" + And the plain text should contain "new.py" + And the plain text should contain "Total changes: 1" + + Scenario: Serialize to JSON + Given a change entry with operation "create" for path "new.py" in resource "RES001" + And after content for "new.py" is "hello\n" + When I build a review artifact from the changeset + And I serialize the artifact to JSON + Then the JSON output should be valid JSON + And the JSON output should contain key "total_changes" + And the JSON output should contain key "groups" + + Scenario: Serialize to rich format + Given a change entry with operation "create" for path "new.py" in resource "RES001" + And after content for "new.py" is "hello\n" + When I build a review artifact from the changeset + And I serialize the artifact to rich format + Then the rich text should contain "[bold]" + And the rich text should contain "new.py" + + Scenario: ResourceDiffGroup summary counts + Given a change entry with operation "create" for path "a.py" in resource "RES001" + And a change entry with operation "modify" for path "b.py" in resource "RES001" + And a change entry with operation "delete" for path "c.py" in resource "RES001" + And after content for "a.py" is "new\n" + And before content for "b.py" is "old\n" + And after content for "b.py" is "new\n" + And before content for "c.py" is "old\n" + When I build a review artifact from the changeset + Then group 0 should have 1 added + And group 0 should have 1 modified + And group 0 should have 1 deleted + + Scenario: ReviewArtifact has plan and changeset IDs + When I build a review artifact from the changeset + Then the review artifact plan ID should match the changeset plan ID + And the review artifact changeset ID should match the changeset ID + + Scenario: Empty content produces empty diff + Given a change entry with operation "modify" for path "empty.py" in resource "RES001" + When I build a review artifact from the changeset + Then the diff text in entry 0 of group 0 should be empty + + Scenario: Multiple entries in same resource sorted by path + Given a change entry with operation "create" for path "z.py" in resource "RES001" + And a change entry with operation "create" for path "a.py" in resource "RES001" + And after content for "z.py" is "z\n" + And after content for "a.py" is "a\n" + When I build a review artifact from the changeset + Then entry 0 in group 0 should have path "a.py" + And entry 1 in group 0 should have path "z.py" + + Scenario: Second entry fully truncated when budget exhausted + Given a change entry with operation "create" for path "a.py" in resource "RES001" + And a change entry with operation "create" for path "b.py" in resource "RES001" + And after content for "a.py" is a string of 1000 characters + And after content for "b.py" is a string of 1000 characters + When I build a review artifact with max diff size 50 + Then the review artifact should be truncated + And entry 1 in group 0 should be truncated + And the diff text in entry 1 of group 0 should contain "[truncated]" + + Scenario: JSON serialization includes hashes and file mode + Given a hashed change entry "modify" path "f.py" resource "RES001" before_hash "aaa" after_hash "bbb" + And a moded change entry "modify" path "g.py" resource "RES001" after_mode 0o644 + And before content for "f.py" is "old\n" + And after content for "f.py" is "new\n" + And before content for "g.py" is "a\n" + And after content for "g.py" is "b\n" + When I build a review artifact from the changeset + And I serialize the artifact to JSON + Then the JSON output should be valid JSON + And the JSON output should have entry with before hash "aaa" + And the JSON output should have entry with after hash "bbb" + And the JSON output should have entry with file mode + + Scenario: JSON serialization includes rename fields + Given a delete entry for path "old/r.py" in resource "RES001" with before_hash "h1" + And a create entry for path "new/r.py" in resource "RES001" with after_hash "h1" + When I build a review artifact from the changeset + And I serialize the artifact to JSON + Then the JSON output should be valid JSON + And the JSON output should have entry with rename from "old/r.py" + + Scenario: Rich format shows truncation warning + Given a change entry with operation "create" for path "big.py" in resource "RES001" + And after content for "big.py" is a string of 100000 characters + When I build a review artifact with max diff size 100 + And I serialize the artifact to rich format + Then the rich text should contain "[yellow]" + And the rich text should contain "truncated" + + Scenario: Rich format shows binary marker + Given a change entry with operation "create" for path "bin.dat" in resource "RES001" + And after content for "bin.dat" is "binary\x00data" + When I build a review artifact from the changeset + And I serialize the artifact to rich format + Then the rich text should contain "[binary]" + + Scenario: Rich format colorizes diff lines + Given a change entry with operation "modify" for path "c.py" in resource "RES001" + And before content for "c.py" is "old\n" + And after content for "c.py" is "new\n" + When I build a review artifact from the changeset + And I serialize the artifact to rich format + Then the rich text should contain "[green]" + And the rich text should contain "[red]" + + Scenario: Cross-resource delete and create are not renamed + Given a delete entry for path "old/x.py" in resource "RES001" with before_hash "same" + And a create entry for path "new/x.py" in resource "RES002" with after_hash "same" + When I build a review artifact from the changeset + Then the review artifact should have 2 groups + And entry 0 in group 0 should not be a rename + And entry 0 in group 1 should not be a rename + + Scenario: Multiple renames consume creates only once + Given a delete entry for path "old/a.py" in resource "RES001" with before_hash "h1" + And a delete entry for path "old/b.py" in resource "RES001" with before_hash "h1" + And a create entry for path "new/a.py" in resource "RES001" with after_hash "h1" + When I build a review artifact from the changeset + Then group 0 should have 2 entries + + Scenario: Rename entry truncated when budget exhausted + Given a change entry with operation "create" for path "a.py" in resource "RES001" + And after content for "a.py" is a string of 1000 characters + And a delete entry for path "old/r.py" in resource "RES001" with before_hash "rh1" + And a create entry for path "new/r.py" in resource "RES001" with after_hash "rh1" + When I build a review artifact with max diff size 50 + Then the review artifact should be truncated + And entry 1 in group 0 should be a rename + And entry 1 in group 0 should be truncated + And the diff text in entry 1 of group 0 should contain "[truncated]" + + Scenario: Plain serialization shows binary marker + Given a change entry with operation "create" for path "img.png" in resource "RES001" + And after content for "img.png" is "png\x00header" + When I build a review artifact from the changeset + And I serialize the artifact to plain text + Then the plain text should contain "[binary file]" diff --git a/features/steps/diff_review_steps.py b/features/steps/diff_review_steps.py new file mode 100644 index 000000000..970f48c9c --- /dev/null +++ b/features/steps/diff_review_steps.py @@ -0,0 +1,456 @@ +"""Step definitions for diff review artifact BDD tests.""" + +from __future__ import annotations + +import json +from typing import Any + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.change import ( + ChangeEntry, + ChangeOperation, + SpecChangeSet, +) +from cleveragents.domain.models.core.diff_review import ( + DiffBuilder, + DiffSerializer, + ReviewArtifact, +) + +__all__: list[str] = [] + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_entry( + operation: str, + path: str, + resource_id: str, + plan_id: str, + *, + before_hash: str | None = None, + after_hash: str | None = None, + after_mode: int | None = None, +) -> ChangeEntry: + """Build a ChangeEntry with sensible defaults.""" + kwargs: dict[str, Any] = { + "plan_id": plan_id, + "resource_id": resource_id, + "tool_name": "builtin/test-tool", + "operation": ChangeOperation(operation), + "path": path, + } + if before_hash is not None: + kwargs["before_hash"] = before_hash + if after_hash is not None: + kwargs["after_hash"] = after_hash + if after_mode is not None: + kwargs["after_mode"] = after_mode + return ChangeEntry(**kwargs) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given('a plan ID "{plan_id}"') +def step_given_plan_id(context: Context, plan_id: str) -> None: + context.plan_id = plan_id + context.before_contents = {} # dict[str, str] + context.after_contents = {} # dict[str, str] + context.resource_names = {} # dict[str, str] + + +@given("an empty changeset for the plan") +def step_given_empty_changeset(context: Context) -> None: + context.changeset = SpecChangeSet(plan_id=context.plan_id) + + +# --------------------------------------------------------------------------- +# Change entry creation +# --------------------------------------------------------------------------- + + +@given('a change entry with operation "{op}" for path "{path}" in resource "{res}"') +def step_add_change_entry(context: Context, op: str, path: str, res: str) -> None: + entry = _make_entry(op, path, res, context.plan_id) + context.changeset.add_change(entry) + + +@given( + 'a delete entry for path "{path}" in resource "{res}" with before_hash "{bhash}"' +) +def step_add_delete_with_hash( + context: Context, path: str, res: str, bhash: str +) -> None: + entry = _make_entry("delete", path, res, context.plan_id, before_hash=bhash) + context.changeset.add_change(entry) + + +@given('a create entry for path "{path}" in resource "{res}" with after_hash "{ahash}"') +def step_add_create_with_hash( + context: Context, path: str, res: str, ahash: str +) -> None: + entry = _make_entry("create", path, res, context.plan_id, after_hash=ahash) + context.changeset.add_change(entry) + + +@given( + 'a hashed change entry "{op}" path "{path}" ' + 'resource "{res}" before_hash "{bhash}" after_hash "{ahash}"' +) +def step_add_entry_with_hashes( + context: Context, + op: str, + path: str, + res: str, + bhash: str, + ahash: str, +) -> None: + entry = _make_entry( + op, path, res, context.plan_id, before_hash=bhash, after_hash=ahash + ) + context.changeset.add_change(entry) + + +@given('a moded change entry "{op}" path "{path}" resource "{res}" after_mode {mode}') +def step_add_entry_with_mode( + context: Context, op: str, path: str, res: str, mode: str +) -> None: + # mode comes as e.g. "0o755" — parse it + mode_int = int(mode, 0) + entry = _make_entry(op, path, res, context.plan_id, after_mode=mode_int) + context.changeset.add_change(entry) + + +# --------------------------------------------------------------------------- +# Content setup +# --------------------------------------------------------------------------- + + +@given('before content for "{path}" is "{content}"') +def step_before_content(context: Context, path: str, content: str) -> None: + # Interpret escape sequences + context.before_contents[path] = content.replace("\\n", "\n").replace( + "\\x00", "\x00" + ) + + +@given('after content for "{path}" is "{content}"') +def step_after_content(context: Context, path: str, content: str) -> None: + context.after_contents[path] = content.replace("\\n", "\n").replace("\\x00", "\x00") + + +@given('after content for "{path}" is a string of {count} characters') +def step_after_content_large(context: Context, path: str, count: str) -> None: + context.after_contents[path] = "x" * int(count) + + +@given('resource name "{res_id}" is "{name}"') +def step_resource_name(context: Context, res_id: str, name: str) -> None: + context.resource_names[res_id] = name + + +# --------------------------------------------------------------------------- +# Build actions +# --------------------------------------------------------------------------- + + +@when("I build a review artifact from the changeset") +def step_build_artifact(context: Context) -> None: + builder = DiffBuilder() + context.artifact = builder.build( + context.changeset, + before_contents=context.before_contents, + after_contents=context.after_contents, + resource_names=context.resource_names, + ) + + +@when("I build a review artifact with max diff size {size}") +def step_build_artifact_limited(context: Context, size: str) -> None: + builder = DiffBuilder(max_diff_size=int(size)) + context.artifact = builder.build( + context.changeset, + before_contents=context.before_contents, + after_contents=context.after_contents, + resource_names=context.resource_names, + ) + + +# --------------------------------------------------------------------------- +# Serialization actions +# --------------------------------------------------------------------------- + + +@when("I serialize the artifact to plain text") +def step_serialize_plain(context: Context) -> None: + context.plain_text = DiffSerializer.to_plain(context.artifact) + + +@when("I serialize the artifact to JSON") +def step_serialize_json(context: Context) -> None: + context.json_text = DiffSerializer.to_json(context.artifact) + + +@when("I serialize the artifact to rich format") +def step_serialize_rich(context: Context) -> None: + context.rich_text = DiffSerializer.to_rich(context.artifact) + + +# --------------------------------------------------------------------------- +# Review artifact assertions +# --------------------------------------------------------------------------- + + +@then("the review artifact should have {n} total changes") +def step_assert_total_changes(context: Context, n: str) -> None: + artifact: ReviewArtifact = context.artifact + assert artifact.total_changes == int(n), ( + f"Expected {n} total changes, got {artifact.total_changes}" + ) + + +@then("the review artifact should have {n} groups") +def step_assert_groups(context: Context, n: str) -> None: + artifact: ReviewArtifact = context.artifact + assert len(artifact.groups) == int(n), ( + f"Expected {n} groups, got {len(artifact.groups)}" + ) + + +@then("the review artifact should not be truncated") +def step_assert_not_truncated(context: Context) -> None: + assert not context.artifact.truncated, "Artifact should not be truncated" + + +@then("the review artifact should be truncated") +def step_assert_truncated(context: Context) -> None: + assert context.artifact.truncated, "Artifact should be truncated" + + +@then("the review artifact plan ID should match the changeset plan ID") +def step_plan_id_matches(context: Context) -> None: + assert context.artifact.plan_id == context.changeset.plan_id + + +@then("the review artifact changeset ID should match the changeset ID") +def step_changeset_id_matches(context: Context) -> None: + assert context.artifact.changeset_id == context.changeset.changeset_id + + +# --------------------------------------------------------------------------- +# Group assertions +# --------------------------------------------------------------------------- + + +@then("group {g} should have {n} entries") +def step_group_entries(context: Context, g: str, n: str) -> None: + group = context.artifact.groups[int(g)] + assert len(group.entries) == int(n), ( + f"Expected {n} entries in group {g}, got {len(group.entries)}" + ) + + +@then('group {g} should have resource id "{rid}"') +def step_group_resource_id(context: Context, g: str, rid: str) -> None: + group = context.artifact.groups[int(g)] + assert group.resource_id == rid, ( + f"Expected resource id '{rid}', got '{group.resource_id}'" + ) + + +@then("group {g} should have {n} added") +def step_group_added(context: Context, g: str, n: str) -> None: + group = context.artifact.groups[int(g)] + assert group.added == int(n), f"Expected {n} added, got {group.added}" + + +@then("group {g} should have {n} modified") +def step_group_modified(context: Context, g: str, n: str) -> None: + group = context.artifact.groups[int(g)] + assert group.modified == int(n), f"Expected {n} modified, got {group.modified}" + + +@then("group {g} should have {n} deleted") +def step_group_deleted(context: Context, g: str, n: str) -> None: + group = context.artifact.groups[int(g)] + assert group.deleted == int(n), f"Expected {n} deleted, got {group.deleted}" + + +@then("group {g} should have {n} renamed") +def step_group_renamed(context: Context, g: str, n: str) -> None: + group = context.artifact.groups[int(g)] + assert group.renamed == int(n), f"Expected {n} renamed, got {group.renamed}" + + +# --------------------------------------------------------------------------- +# Entry assertions +# --------------------------------------------------------------------------- + + +@then('entry {e} in group {g} should have change type "{ct}"') +def step_entry_change_type(context: Context, e: str, g: str, ct: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.change_type == ct, ( + f"Expected change_type '{ct}', got '{entry.change_type}'" + ) + + +@then('entry {e} in group {g} should have path "{path}"') +def step_entry_path(context: Context, e: str, g: str, path: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.path == path, f"Expected path '{path}', got '{entry.path}'" + + +@then("entry {e} in group {g} should be binary") +def step_entry_binary(context: Context, e: str, g: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.is_binary, "Entry should be binary" + + +@then("entry {e} in group {g} should be a rename") +def step_entry_is_rename(context: Context, e: str, g: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.is_rename, "Entry should be a rename" + + +@then("entry {e} in group {g} should not be a rename") +def step_entry_not_rename(context: Context, e: str, g: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert not entry.is_rename, "Entry should not be a rename" + + +@then('entry {e} in group {g} rename should be from "{src}" to "{dst}"') +def step_entry_rename_paths( + context: Context, e: str, g: str, src: str, dst: str +) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.rename_from == src, ( + f"Expected rename_from '{src}', got '{entry.rename_from}'" + ) + assert entry.rename_to == dst, ( + f"Expected rename_to '{dst}', got '{entry.rename_to}'" + ) + + +@then("entry {e} in group {g} should be truncated") +def step_entry_truncated(context: Context, e: str, g: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.truncated, "Entry should be truncated" + + +@then('entry {e} in group {g} should have before hash "{h}"') +def step_entry_before_hash(context: Context, e: str, g: str, h: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.before_hash == h, ( + f"Expected before_hash '{h}', got '{entry.before_hash}'" + ) + + +@then('entry {e} in group {g} should have after hash "{h}"') +def step_entry_after_hash(context: Context, e: str, g: str, h: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.after_hash == h, f"Expected after_hash '{h}', got '{entry.after_hash}'" + + +@then("entry {e} in group {g} should have file mode {mode}") +def step_entry_file_mode(context: Context, e: str, g: str, mode: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.file_mode == int(mode), ( + f"Expected file_mode {mode}, got {entry.file_mode}" + ) + + +# --------------------------------------------------------------------------- +# Diff text assertions +# --------------------------------------------------------------------------- + + +@then('the diff text in entry {e} of group {g} should contain "{text}"') +def step_diff_contains(context: Context, e: str, g: str, text: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert text in entry.diff_text, ( + f"Expected diff_text to contain '{text}', got: {entry.diff_text!r}" + ) + + +@then("the diff text in entry {e} of group {g} should be empty") +def step_diff_empty(context: Context, e: str, g: str) -> None: + entry = context.artifact.groups[int(g)].entries[int(e)] + assert entry.diff_text == "", f"Expected empty diff_text, got: {entry.diff_text!r}" + + +# --------------------------------------------------------------------------- +# Serialization assertions +# --------------------------------------------------------------------------- + + +@then('the plain text should contain "{text}"') +def step_plain_contains(context: Context, text: str) -> None: + assert text in context.plain_text, f"Expected plain text to contain '{text}'" + + +@then("the JSON output should be valid JSON") +def step_json_valid(context: Context) -> None: + try: + json.loads(context.json_text) + except json.JSONDecodeError as exc: + raise AssertionError(f"Invalid JSON: {exc}") from exc + + +@then('the JSON output should contain key "{key}"') +def step_json_has_key(context: Context, key: str) -> None: + data = json.loads(context.json_text) + assert key in data, f"Expected key '{key}' in JSON output" + + +@then('the rich text should contain "{text}"') +def step_rich_contains(context: Context, text: str) -> None: + assert text in context.rich_text, f"Expected rich text to contain '{text}'" + + +@then('the JSON output should have entry with before hash "{h}"') +def step_json_entry_before_hash(context: Context, h: str) -> None: + data = json.loads(context.json_text) + for group in data["groups"]: + for entry in group["entries"]: + if entry.get("before_hash") == h: + return + raise AssertionError(f"No entry with before_hash '{h}' in JSON output") + + +@then('the JSON output should have entry with after hash "{h}"') +def step_json_entry_after_hash(context: Context, h: str) -> None: + data = json.loads(context.json_text) + for group in data["groups"]: + for entry in group["entries"]: + if entry.get("after_hash") == h: + return + raise AssertionError(f"No entry with after_hash '{h}' in JSON output") + + +@then("the JSON output should have entry with file mode") +def step_json_entry_file_mode(context: Context) -> None: + data = json.loads(context.json_text) + for group in data["groups"]: + for entry in group["entries"]: + if "file_mode" in entry: + return + raise AssertionError("No entry with file_mode in JSON output") + + +@then('the JSON output should have entry with rename from "{path}"') +def step_json_entry_rename_from(context: Context, path: str) -> None: + data = json.loads(context.json_text) + for group in data["groups"]: + for entry in group["entries"]: + if entry.get("rename_from") == path: + return + raise AssertionError(f"No entry with rename_from '{path}' in JSON output") diff --git a/robot/diff_review.robot b/robot/diff_review.robot new file mode 100644 index 000000000..ff6f8e707 --- /dev/null +++ b/robot/diff_review.robot @@ -0,0 +1,50 @@ +*** Settings *** +Documentation Smoke tests for diff review artifact building, +... rename detection, serialization, and truncation. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_diff_review.py + +*** Test Cases *** +Build Empty Changeset Artifact + [Documentation] Build a ReviewArtifact from an empty SpecChangeSet + ${result}= Run Process ${PYTHON} ${HELPER} build-empty cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} build-empty-ok + +Build Create Change Artifact + [Documentation] Build a ReviewArtifact with a single CREATE change entry + ${result}= Run Process ${PYTHON} ${HELPER} build-create cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} build-create-ok + +Detect Rename From Delete And Create + [Documentation] Detect renames by matching content hashes across DELETE/CREATE pairs + ${result}= Run Process ${PYTHON} ${HELPER} build-rename cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} build-rename-ok + +Serialize Artifact To All Formats + [Documentation] Serialize a ReviewArtifact to plain text, JSON, and rich format + ${result}= Run Process ${PYTHON} ${HELPER} serialize-all cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} serialize-all-ok + +Truncate Large Diff + [Documentation] Verify diff truncation when content exceeds max_diff_size + ${result}= Run Process ${PYTHON} ${HELPER} truncation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} truncation-ok diff --git a/robot/helper_diff_review.py b/robot/helper_diff_review.py new file mode 100644 index 000000000..aae1b48b1 --- /dev/null +++ b/robot/helper_diff_review.py @@ -0,0 +1,200 @@ +"""Robot Framework helper for diff review artifact smoke tests. + +Provides a CLI-style interface for Robot to invoke diff review +model creation, serialization, and builder operations. +Exit code 0 = success, 1 = failure. + +Usage: + python robot/helper_diff_review.py build-empty + python robot/helper_diff_review.py build-create + python robot/helper_diff_review.py build-rename + python robot/helper_diff_review.py serialize-all + python robot/helper_diff_review.py truncation +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path +from typing import Any + +# Ensure the src directory is on the import path. +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from cleveragents.domain.models.core.change import ( # noqa: E402 + ChangeEntry, + ChangeOperation, + SpecChangeSet, +) +from cleveragents.domain.models.core.diff_review import ( # noqa: E402 + DiffBuilder, + DiffSerializer, +) + + +def _make_entry( + plan_id: str, + resource_id: str, + operation: str, + path: str, + *, + before_hash: str | None = None, + after_hash: str | None = None, +) -> ChangeEntry: + """Build a ChangeEntry with sensible defaults.""" + kwargs: dict[str, Any] = { + "plan_id": plan_id, + "resource_id": resource_id, + "tool_name": "builtin/test-tool", + "operation": ChangeOperation(operation), + "path": path, + } + if before_hash is not None: + kwargs["before_hash"] = before_hash + if after_hash is not None: + kwargs["after_hash"] = after_hash + return ChangeEntry(**kwargs) + + +def _cmd_build_empty() -> int: + """Build an artifact from an empty changeset.""" + cs = SpecChangeSet(plan_id="PLAN001") + builder = DiffBuilder() + artifact = builder.build(cs) + assert artifact.total_changes == 0 + assert len(artifact.groups) == 0 + assert not artifact.truncated + assert artifact.plan_id == "PLAN001" + print("build-empty-ok") + return 0 + + +def _cmd_build_create() -> int: + """Build an artifact with a CREATE change.""" + cs = SpecChangeSet(plan_id="PLAN001") + entry = _make_entry("PLAN001", "RES001", "create", "src/main.py") + cs.add_change(entry) + + builder = DiffBuilder() + artifact = builder.build( + cs, + after_contents={"src/main.py": "print('hello')\n"}, + ) + assert artifact.total_changes == 1 + assert len(artifact.groups) == 1 + group = artifact.groups[0] + assert group.resource_id == "RES001" + assert len(group.entries) == 1 + assert group.entries[0].change_type == "create" + assert group.entries[0].path == "src/main.py" + assert "+++" in group.entries[0].diff_text + print("build-create-ok") + return 0 + + +def _cmd_build_rename() -> int: + """Build an artifact with rename detection.""" + cs = SpecChangeSet(plan_id="PLAN001") + del_entry = _make_entry( + "PLAN001", "RES001", "delete", "old/file.py", before_hash="abc123" + ) + cre_entry = _make_entry( + "PLAN001", "RES001", "create", "new/file.py", after_hash="abc123" + ) + cs.add_change(del_entry) + cs.add_change(cre_entry) + + builder = DiffBuilder() + artifact = builder.build(cs) + assert len(artifact.groups) == 1 + group = artifact.groups[0] + assert len(group.entries) == 1 + assert group.entries[0].is_rename + assert group.entries[0].rename_from == "old/file.py" + assert group.entries[0].rename_to == "new/file.py" + print("build-rename-ok") + return 0 + + +def _cmd_serialize_all() -> int: + """Build an artifact and serialize to all formats.""" + cs = SpecChangeSet(plan_id="PLAN001") + entry = _make_entry("PLAN001", "RES001", "create", "new.py") + cs.add_change(entry) + + builder = DiffBuilder() + artifact = builder.build( + cs, + after_contents={"new.py": "hello\n"}, + resource_names={"RES001": "my-resource"}, + ) + + # Plain text + plain = DiffSerializer.to_plain(artifact) + assert "my-resource" in plain + assert "Total changes: 1" in plain + + # JSON + json_text = DiffSerializer.to_json(artifact) + data = json.loads(json_text) + assert "total_changes" in data + assert "groups" in data + assert data["total_changes"] == 1 + + # Rich + rich = DiffSerializer.to_rich(artifact) + assert "[bold]" in rich + assert "new.py" in rich + + print("serialize-all-ok") + return 0 + + +def _cmd_truncation() -> int: + """Verify truncation with a small max_diff_size.""" + cs = SpecChangeSet(plan_id="PLAN001") + entry = _make_entry("PLAN001", "RES001", "create", "big.py") + cs.add_change(entry) + + builder = DiffBuilder(max_diff_size=50) + artifact = builder.build( + cs, + after_contents={"big.py": "x" * 10000}, + ) + assert artifact.truncated + assert artifact.groups[0].entries[0].truncated + print("truncation-ok") + return 0 + + +def main() -> int: + """Entry point called by Robot Framework ``Run Process``.""" + if len(sys.argv) < 2: + print( + "Usage: helper_diff_review.py " + "" + ) + return 1 + + command = sys.argv[1] + + if command == "build-empty": + return _cmd_build_empty() + if command == "build-create": + return _cmd_build_create() + if command == "build-rename": + return _cmd_build_rename() + if command == "serialize-all": + return _cmd_serialize_all() + if command == "truncation": + return _cmd_truncation() + + print(f"Unknown command: {command}") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/src/cleveragents/domain/models/core/__init__.py b/src/cleveragents/domain/models/core/__init__.py index a13a8bc7d..64a879911 100644 --- a/src/cleveragents/domain/models/core/__init__.py +++ b/src/cleveragents/domain/models/core/__init__.py @@ -49,6 +49,13 @@ from cleveragents.domain.models.core.correction import ( CorrectionStatus, ) from cleveragents.domain.models.core.debug_attempt import DebugAttempt +from cleveragents.domain.models.core.diff_review import ( + DiffBuilder, + DiffEntry, + DiffSerializer, + ResourceDiffGroup, + ReviewArtifact, +) # Invariant domain models from cleveragents.domain.models.core.invariant import ( @@ -199,6 +206,9 @@ __all__ = [ "CreditsTransaction", "CreditsTransactionType", "DebugAttempt", + "DiffBuilder", + "DiffEntry", + "DiffSerializer", "GuardResult", "InMemoryChangeSetStore", "InMemoryInvocationTracker", @@ -242,11 +252,13 @@ __all__ = [ "Resource", "ResourceAccessMode", "ResourceCapabilities", + "ResourceDiffGroup", "ResourceKind", "ResourceSlot", "ResourceTypeArgument", "ResourceTypeSandboxStrategy", "ResourceTypeSpec", + "ReviewArtifact", "SandboxStrategy", "Session", "SessionExportError", diff --git a/src/cleveragents/domain/models/core/diff_review.py b/src/cleveragents/domain/models/core/diff_review.py new file mode 100644 index 000000000..6ccb8607d --- /dev/null +++ b/src/cleveragents/domain/models/core/diff_review.py @@ -0,0 +1,537 @@ +"""Diff review domain models for CleverAgents. + +Builds reviewable diff artifacts from SpecChangeSet records, +grouping changes by resource and producing unified diff text +for CLI display in rich, plain, and JSON formats. +""" + +from __future__ import annotations + +import difflib +import json +from datetime import UTC, datetime +from difflib import SequenceMatcher +from typing import Any + +from pydantic import BaseModel, ConfigDict, Field +from ulid import ULID + +from cleveragents.domain.models.core.change import ( + ChangeEntry, + ChangeOperation, + SpecChangeSet, +) + +DEFAULT_MAX_DIFF_SIZE: int = 50_000 +"""Maximum total diff text characters before truncation.""" + +RENAME_SIMILARITY_THRESHOLD: float = 0.6 +"""Minimum path similarity ratio for rename detection.""" + + +def _new_ulid() -> str: + return str(ULID()) + + +def _is_binary(content: str | None) -> bool: + """Detect binary content by checking for null bytes.""" + if content is None: + return False + return "\x00" in content + + +def _unified_diff( + before: str | None, + after: str | None, + path: str, + *, + context_lines: int = 3, +) -> str: + """Generate a unified diff string between before and after content.""" + before_lines = (before or "").splitlines(keepends=True) + after_lines = (after or "").splitlines(keepends=True) + diff_lines = difflib.unified_diff( + before_lines, + after_lines, + fromfile=f"a/{path}", + tofile=f"b/{path}", + n=context_lines, + ) + return "".join(diff_lines) + + +class DiffEntry(BaseModel): + """A single diff entry for CLI review.""" + + resource_id: str = Field(..., description="Resource ULID") + resource_name: str = Field( + default="", + description="Human-readable resource name", + ) + path: str = Field(..., min_length=1, description="File path") + change_type: str = Field( + ..., + description="Operation type (create/modify/delete/rename)", + ) + before_content: str | None = Field( + default=None, + description="Content before the change", + ) + after_content: str | None = Field( + default=None, + description="Content after the change", + ) + before_hash: str | None = Field( + default=None, + description="SHA-256 before", + ) + after_hash: str | None = Field( + default=None, + description="SHA-256 after", + ) + file_mode: int | None = Field( + default=None, + description="File permission mode", + ) + is_binary: bool = Field( + default=False, + description="Whether the file is binary", + ) + is_rename: bool = Field( + default=False, + description="Whether this is a rename/move", + ) + rename_from: str | None = Field( + default=None, + description="Original path for renames", + ) + rename_to: str | None = Field( + default=None, + description="Destination path for renames", + ) + diff_text: str = Field( + default="", + description="Unified diff text", + ) + truncated: bool = Field( + default=False, + description="Whether the diff text was truncated", + ) + + model_config = ConfigDict( + str_strip_whitespace=True, + validate_assignment=True, + ) + + +class ResourceDiffGroup(BaseModel): + """Group of diff entries for a single resource.""" + + resource_id: str = Field(..., description="Resource ULID") + resource_name: str = Field( + default="", + description="Human-readable resource name", + ) + entries: list[DiffEntry] = Field( + default_factory=list, + description="Diff entries for this resource", + ) + + model_config = ConfigDict( + str_strip_whitespace=True, + validate_assignment=True, + ) + + @property + def added(self) -> int: + """Count of CREATE entries.""" + return sum(1 for e in self.entries if e.change_type == "create") + + @property + def modified(self) -> int: + """Count of MODIFY entries.""" + return sum(1 for e in self.entries if e.change_type == "modify") + + @property + def deleted(self) -> int: + """Count of DELETE entries.""" + return sum(1 for e in self.entries if e.change_type == "delete") + + @property + def renamed(self) -> int: + """Count of RENAME entries.""" + return sum(1 for e in self.entries if e.change_type == "rename") + + +class ReviewArtifact(BaseModel): + """Complete diff review artifact for a plan.""" + + artifact_id: str = Field( + default_factory=_new_ulid, + description="ULID for this review artifact", + ) + plan_id: str = Field(..., description="Plan ULID") + changeset_id: str = Field(..., description="ChangeSet ULID") + created_at: datetime = Field( + default_factory=lambda: datetime.now(UTC), + description="When this artifact was created", + ) + groups: list[ResourceDiffGroup] = Field( + default_factory=list, + description="Diff groups by resource", + ) + total_changes: int = Field( + default=0, + description="Total number of change entries", + ) + truncated: bool = Field( + default=False, + description="Whether any diff was truncated", + ) + max_diff_size: int = Field( + default=DEFAULT_MAX_DIFF_SIZE, + description="Max diff text chars before truncation", + ) + + model_config = ConfigDict( + str_strip_whitespace=True, + validate_assignment=True, + ) + + +class DiffBuilder: + """Builds a ReviewArtifact from a SpecChangeSet. + + Takes optional content maps (``before_contents`` and + ``after_contents``) keyed by file path to generate unified + diffs. A ``resource_names`` map provides human-readable names + for resource IDs. + """ + + def __init__( + self, + *, + max_diff_size: int = DEFAULT_MAX_DIFF_SIZE, + context_lines: int = 3, + ) -> None: + self._max_diff_size = max_diff_size + self._context_lines = context_lines + + def build( + self, + changeset: SpecChangeSet, + *, + before_contents: dict[str, str] | None = None, + after_contents: dict[str, str] | None = None, + resource_names: dict[str, str] | None = None, + ) -> ReviewArtifact: + """Build a ReviewArtifact from a changeset.""" + before_map = before_contents or {} + after_map = after_contents or {} + name_map = resource_names or {} + + grouped = changeset.grouped_by_resource() + total_diff_size = 0 + artifact_truncated = False + groups: list[ResourceDiffGroup] = [] + + # Detect renames across all entries + rename_pairs = self._detect_renames( + changeset.entries, + before_map, + after_map, + ) + consumed_entries: set[str] = set() + for del_id, cre_id in rename_pairs: + consumed_entries.add(del_id) + consumed_entries.add(cre_id) + + for resource_id in sorted(grouped.keys()): + entries_for_resource = grouped[resource_id] + res_name = name_map.get(resource_id, resource_id) + diff_entries: list[DiffEntry] = [] + + for entry in entries_for_resource: + if entry.entry_id in consumed_entries: + # Skip individually consumed rename entries + continue + + before = before_map.get(entry.path) + after = after_map.get(entry.path) + binary = _is_binary(before) or _is_binary(after) + + diff_text = "" + if binary: + diff_text = "[binary file]" + else: + diff_text = _unified_diff( + before, + after, + entry.path, + context_lines=self._context_lines, + ) + + entry_truncated = False + remaining = self._max_diff_size - total_diff_size + if remaining <= 0: + diff_text = "[truncated]" + entry_truncated = True + artifact_truncated = True + elif len(diff_text) > remaining: + diff_text = diff_text[:remaining] + "\n[truncated]" + entry_truncated = True + artifact_truncated = True + + total_diff_size += len(diff_text) + + diff_entries.append( + DiffEntry( + resource_id=resource_id, + resource_name=res_name, + path=entry.path, + change_type=entry.operation, + before_content=before, + after_content=after, + before_hash=entry.before_hash, + after_hash=entry.after_hash, + file_mode=entry.after_mode or entry.before_mode, + is_binary=binary, + is_rename=False, + diff_text=diff_text, + truncated=entry_truncated, + ) + ) + + # Add rename entries + for del_id, cre_id in rename_pairs: + del_entry = self._find_entry(entries_for_resource, del_id) + cre_entry = self._find_entry(entries_for_resource, cre_id) + if del_entry is None or cre_entry is None: + continue + + after = after_map.get(cre_entry.path) + binary = _is_binary(after) + diff_text = f"renamed: {del_entry.path} -> {cre_entry.path}" + + entry_truncated = False + remaining = self._max_diff_size - total_diff_size + if remaining <= 0: + diff_text = "[truncated]" + entry_truncated = True + artifact_truncated = True + elif len(diff_text) > remaining: + diff_text = diff_text[:remaining] + "\n[truncated]" + entry_truncated = True + artifact_truncated = True + + total_diff_size += len(diff_text) + + diff_entries.append( + DiffEntry( + resource_id=resource_id, + resource_name=res_name, + path=cre_entry.path, + change_type="rename", + before_content=None, + after_content=after, + before_hash=del_entry.before_hash, + after_hash=cre_entry.after_hash, + file_mode=cre_entry.after_mode, + is_binary=binary, + is_rename=True, + rename_from=del_entry.path, + rename_to=cre_entry.path, + diff_text=diff_text, + truncated=entry_truncated, + ) + ) + + # Sort entries by (path, change_type) + diff_entries.sort(key=lambda e: (e.path, e.change_type)) + + groups.append( + ResourceDiffGroup( + resource_id=resource_id, + resource_name=res_name, + entries=diff_entries, + ) + ) + + return ReviewArtifact( + plan_id=changeset.plan_id, + changeset_id=changeset.changeset_id, + groups=groups, + total_changes=len(changeset.entries), + truncated=artifact_truncated, + max_diff_size=self._max_diff_size, + ) + + def _detect_renames( + self, + entries: list[ChangeEntry], + before_map: dict[str, str], + after_map: dict[str, str], + ) -> list[tuple[str, str]]: + """Detect rename pairs using path similarity + content hash.""" + deletes = [e for e in entries if e.operation == ChangeOperation.DELETE] + creates = [e for e in entries if e.operation == ChangeOperation.CREATE] + + pairs: list[tuple[str, str]] = [] + used_creates: set[str] = set() + + for del_entry in deletes: + best_ratio = 0.0 + best_create: ChangeEntry | None = None + + for cre_entry in creates: + if cre_entry.entry_id in used_creates: + continue + if cre_entry.resource_id != del_entry.resource_id: + continue + + # Check content hash match + del_hash = del_entry.before_hash + cre_hash = cre_entry.after_hash + if del_hash is None or cre_hash is None: + continue + if del_hash != cre_hash: + continue + + ratio = SequenceMatcher( + None, + del_entry.path, + cre_entry.path, + ).ratio() + if ratio > best_ratio: + best_ratio = ratio + best_create = cre_entry + + if best_create is not None and best_ratio >= RENAME_SIMILARITY_THRESHOLD: + pairs.append((del_entry.entry_id, best_create.entry_id)) + used_creates.add(best_create.entry_id) + + return pairs + + @staticmethod + def _find_entry( + entries: list[ChangeEntry], + entry_id: str, + ) -> ChangeEntry | None: + """Find a ChangeEntry by ID in a list.""" + for e in entries: + if e.entry_id == entry_id: + return e + return None + + +class DiffSerializer: + """Serialize a ReviewArtifact to various output formats.""" + + @staticmethod + def to_plain(artifact: ReviewArtifact) -> str: + """Render as plain text.""" + lines: list[str] = [] + lines.append( + f"Review: plan={artifact.plan_id} changeset={artifact.changeset_id}" + ) + lines.append(f"Total changes: {artifact.total_changes}") + if artifact.truncated: + lines.append("[some diffs truncated]") + lines.append("") + + for group in artifact.groups: + lines.append(f"Resource: {group.resource_name} ({group.resource_id})") + lines.append( + f" +{group.added} ~{group.modified} -{group.deleted} r{group.renamed}" + ) + for entry in group.entries: + lines.append(f" --- {entry.path} [{entry.change_type}]") + if entry.is_binary: + lines.append(" [binary file]") + elif entry.diff_text: + for diff_line in entry.diff_text.splitlines(): + lines.append(f" {diff_line}") + lines.append("") + + return "\n".join(lines) + + @staticmethod + def to_json(artifact: ReviewArtifact) -> str: + """Render as JSON string.""" + data: dict[str, Any] = { + "artifact_id": artifact.artifact_id, + "plan_id": artifact.plan_id, + "changeset_id": artifact.changeset_id, + "created_at": artifact.created_at.isoformat(), + "total_changes": artifact.total_changes, + "truncated": artifact.truncated, + "groups": [], + } + for group in artifact.groups: + group_data: dict[str, Any] = { + "resource_id": group.resource_id, + "resource_name": group.resource_name, + "added": group.added, + "modified": group.modified, + "deleted": group.deleted, + "renamed": group.renamed, + "entries": [], + } + for entry in group.entries: + entry_data: dict[str, Any] = { + "path": entry.path, + "change_type": entry.change_type, + "is_binary": entry.is_binary, + "is_rename": entry.is_rename, + "truncated": entry.truncated, + "diff_text": entry.diff_text, + } + if entry.before_hash: + entry_data["before_hash"] = entry.before_hash + if entry.after_hash: + entry_data["after_hash"] = entry.after_hash + if entry.file_mode is not None: + entry_data["file_mode"] = entry.file_mode + if entry.rename_from: + entry_data["rename_from"] = entry.rename_from + if entry.rename_to: + entry_data["rename_to"] = entry.rename_to + group_data["entries"].append(entry_data) + data["groups"].append(group_data) + + return json.dumps(data, indent=2) + + @staticmethod + def to_rich(artifact: ReviewArtifact) -> str: + """Render as rich-formatted text with color markers.""" + lines: list[str] = [] + lines.append(f"[bold]Review[/bold]: plan={artifact.plan_id}") + lines.append(f"Changes: {artifact.total_changes}") + if artifact.truncated: + lines.append("[yellow]Some diffs truncated[/yellow]") + lines.append("") + + for group in artifact.groups: + lines.append(f"[cyan]{group.resource_name}[/cyan] ({group.resource_id})") + lines.append( + f" [green]+{group.added}[/green] " + f"[yellow]~{group.modified}[/yellow] " + f"[red]-{group.deleted}[/red] " + f"r{group.renamed}" + ) + for entry in group.entries: + if entry.is_binary: + lines.append(f" {entry.path} [dim][binary][/dim]") + else: + lines.append(f" {entry.path} [{entry.change_type}]") + if entry.diff_text: + for dl in entry.diff_text.splitlines(): + if dl.startswith("+"): + lines.append(f" [green]{dl}[/green]") + elif dl.startswith("-"): + lines.append(f" [red]{dl}[/red]") + else: + lines.append(f" {dl}") + lines.append("") + + return "\n".join(lines)