diff --git a/CHANGELOG.md b/CHANGELOG.md index 201dc8817..fa3466385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **Plan diff shows worktree branch changes** (#9231): `plan diff` now detects + the worktree branch `cleveragents/plan-` created during `plan execute` + and runs `git diff HEAD...` to display actual file changes. Falls + back to changeset-based diff when no worktree branch exists. Git operations + delegated to `GitWorktreeSandbox.diff_against_head()` in the Infrastructure + layer. + ### Fixed - **Worktree sandbox cleanup on plan cancel** (#9230): `plan cancel` now removes diff --git a/features/plan_diff_worktree.feature b/features/plan_diff_worktree.feature new file mode 100644 index 000000000..b394773ee --- /dev/null +++ b/features/plan_diff_worktree.feature @@ -0,0 +1,32 @@ +@plan-diff-worktree +Feature: Plan diff shows worktree branch changes (#9231) + Verifies that plan diff displays the actual file changes from the + worktree branch created during plan execute, falling back to + changeset-based diff when no worktree branch exists. + + Background: + Given the plan-diff in-memory database is initialized + + Scenario: diff_against_head returns diff when worktree branch exists + Given a temp git repo with a worktree branch for plan "01TESTDIFF00000000000000" for pdt + And a file "hello.py" is changed on the worktree branch for pdt + When I call diff_against_head for plan "01TESTDIFF00000000000000" for pdt + Then the diff output should contain "hello.py" for pdt + And the diff output should not be None for pdt + + Scenario: diff_against_head returns None when no branch exists + Given a temp git repo without a worktree branch for pdt + When I call diff_against_head for plan "01TESTDIFFNO000000000000" for pdt + Then the diff output should be None for pdt + + Scenario: _get_worktree_diff returns diff via service resolution + Given a temp git repo with a worktree branch for plan "01TESTDIFFSVC00000000000" for pdt + And a file "app.py" is changed on the worktree branch for pdt + And a mocked service that resolves the git resource for pdt + When I call _get_worktree_diff for plan "01TESTDIFFSVC00000000000" for pdt + Then the diff output should contain "app.py" for pdt + + Scenario: _get_worktree_diff returns None when no linked resources + Given a mocked service with no linked resources for plan diff for pdt + When I call _get_worktree_diff for plan "01TESTDIFFNONE0000000000" for pdt + Then the diff output should be None for pdt diff --git a/features/steps/db_repositories_cov_r3_steps.py b/features/steps/db_repositories_cov_r3_steps.py index c33ea164c..826e3d0a8 100644 --- a/features/steps/db_repositories_cov_r3_steps.py +++ b/features/steps/db_repositories_cov_r3_steps.py @@ -83,9 +83,28 @@ def step_setup_db(context: Context) -> None: engine = create_engine("sqlite:///:memory:", echo=False) Base.metadata.create_all(engine) - factory = sessionmaker(bind=engine) + # Use a shared session so flush() in one repo call is visible + # to subsequent repo calls within the same scenario. + # Wrap with rollback-on-close to prevent close() from destroying + # the shared session (repos and helper code call session.close()). + _real = sessionmaker(bind=engine, expire_on_commit=False)() + + class _SharedSession: + """Proxy that turns close() into rollback() on the shared session.""" + + def close(self) -> None: + _real.rollback() + + def __getattr__(self, name: str) -> object: + return getattr(_real, name) + + _wrapper = _SharedSession() + + def _shared_factory() -> Session: + return _wrapper # type: ignore[return-value] + context.drcov3_engine = engine - context.drcov3_factory = factory + context.drcov3_factory = _shared_factory context.drcov3_error = None context.drcov3_result = None diff --git a/features/steps/plan_diff_worktree_steps.py b/features/steps/plan_diff_worktree_steps.py new file mode 100644 index 000000000..1cbadd6fe --- /dev/null +++ b/features/steps/plan_diff_worktree_steps.py @@ -0,0 +1,191 @@ +"""Steps for plan_diff_worktree.feature.""" + +from __future__ import annotations + +import shutil +import subprocess +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + + +def _git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + timeout=10, + ) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("the plan-diff in-memory database is initialized") +def step_pdt_init(context: Context) -> None: + context.pdt_diff_output: str | None = None # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# Given — repo fixtures +# --------------------------------------------------------------------------- + + +@given('a temp git repo with a worktree branch for plan "{plan_id}" for pdt') +def step_create_repo_with_branch(context: Context, plan_id: str) -> None: + d = tempfile.mkdtemp(prefix="pdt-") + context.add_cleanup(shutil.rmtree, d, True) # type: ignore[attr-defined] + _git(["init", "-q", "-b", "main"], d) + _git(["config", "user.name", "T"], d) + _git(["config", "user.email", "t@t"], d) + _git(["config", "commit.gpgsign", "false"], d) + Path(d, "README.md").write_text("initial\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + + branch = f"cleveragents/plan-{plan_id}" + wt_dir = tempfile.mkdtemp(prefix="pdt-wt-") + context.add_cleanup(shutil.rmtree, wt_dir, True) # type: ignore[attr-defined] + _git(["worktree", "add", "-b", branch, wt_dir, "HEAD"], d) + + context.pdt_repo = d # type: ignore[attr-defined] + context.pdt_wt_dir = wt_dir # type: ignore[attr-defined] + context.pdt_plan_id = plan_id # type: ignore[attr-defined] + context.pdt_branch = branch # type: ignore[attr-defined] + + +@given('a file "{filename}" is changed on the worktree branch for pdt') +def step_change_file_on_branch(context: Context, filename: str) -> None: + wt_dir: str = context.pdt_wt_dir # type: ignore[attr-defined] + Path(wt_dir, filename).write_text("new content\n") + _git(["add", "."], wt_dir) + _git(["commit", "-q", "-m", f"add {filename}"], wt_dir) + + +@given("a temp git repo without a worktree branch for pdt") +def step_create_clean_repo(context: Context) -> None: + d = tempfile.mkdtemp(prefix="pdt-clean-") + context.add_cleanup(shutil.rmtree, d, True) # type: ignore[attr-defined] + _git(["init", "-q", "-b", "main"], d) + _git(["config", "user.name", "T"], d) + _git(["config", "user.email", "t@t"], d) + _git(["config", "commit.gpgsign", "false"], d) + Path(d, "README.md").write_text("initial\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + context.pdt_repo = d # type: ignore[attr-defined] + + +@given("a mocked service that resolves the git resource for pdt") +def step_mock_service_with_resource(context: Context) -> None: + mock_resource = MagicMock() + mock_resource.resource_type_name = "git-checkout" + mock_resource.location = context.pdt_repo # type: ignore[attr-defined] + mock_resource.resource_id = "res-pdt-test" + + mock_lr = MagicMock() + mock_lr.resource_id = "res-pdt-test" + + mock_project = MagicMock() + mock_project.linked_resources = [mock_lr] + + mock_plan = MagicMock() + mock_plan.project_links = [MagicMock(project_name="local/pdt-test")] + + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_project_repo = MagicMock() + mock_project_repo.get.return_value = mock_project + + mock_resource_registry = MagicMock() + mock_resource_registry.show_resource.return_value = mock_resource + + mock_container = MagicMock() + mock_container.namespaced_project_repo.return_value = mock_project_repo + mock_container.resource_registry_service.return_value = mock_resource_registry + + context.pdt_service = mock_service # type: ignore[attr-defined] + context.pdt_container = mock_container # type: ignore[attr-defined] + + +@given("a mocked service with no linked resources for plan diff for pdt") +def step_mock_service_no_resources(context: Context) -> None: + mock_plan = MagicMock() + mock_plan.project_links = [] + + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_container = MagicMock() + + context.pdt_service = mock_service # type: ignore[attr-defined] + context.pdt_container = mock_container # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# When — Infrastructure layer +# --------------------------------------------------------------------------- + + +@when('I call diff_against_head for plan "{plan_id}" for pdt') +def step_call_diff_against_head(context: Context, plan_id: str) -> None: + from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, + ) + + context.pdt_diff_output = GitWorktreeSandbox.diff_against_head( # type: ignore[attr-defined] + context.pdt_repo, # type: ignore[attr-defined] + plan_id, + ) + + +# --------------------------------------------------------------------------- +# When — CLI layer +# --------------------------------------------------------------------------- + + +@when('I call _get_worktree_diff for plan "{plan_id}" for pdt') +def step_call_get_worktree_diff(context: Context, plan_id: str) -> None: + from cleveragents.cli.commands.plan import _get_worktree_diff + + service: Any = context.pdt_service # type: ignore[attr-defined] + container: Any = context.pdt_container # type: ignore[attr-defined] + + with patch( + "cleveragents.cli.commands.plan.get_container", + return_value=container, + ): + context.pdt_diff_output = _get_worktree_diff(plan_id, service) # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# Then +# --------------------------------------------------------------------------- + + +@then('the diff output should contain "{text}" for pdt') +def step_diff_contains(context: Context, text: str) -> None: + output: str | None = context.pdt_diff_output # type: ignore[attr-defined] + assert output is not None, "Expected diff output but got None" + assert text in output, f"Expected '{text}' in diff output, got: {output[:200]}" + + +@then("the diff output should not be None for pdt") +def step_diff_not_none(context: Context) -> None: + assert context.pdt_diff_output is not None, "Expected diff output but got None" # type: ignore[attr-defined] + + +@then("the diff output should be None for pdt") +def step_diff_is_none(context: Context) -> None: + assert context.pdt_diff_output is None, ( # type: ignore[attr-defined] + f"Expected None but got: {context.pdt_diff_output}" # type: ignore[attr-defined] + ) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 15c7e2528..11e742f5b 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -3429,6 +3429,71 @@ def revert_plan( raise typer.Abort() from e +_diff_logger = structlog.get_logger("cleveragents.cli.commands.plan.diff") + + +def _get_worktree_diff( + plan_id: str, + service: PlanLifecycleService, +) -> str | None: + """Return a worktree-branch diff for a plan, or ``None``. + + Resolves the plan's linked git-checkout resource and delegates to + :meth:`GitWorktreeSandbox.diff_against_head` in the Infrastructure + layer. Returns ``None`` when no worktree branch exists so the + caller can fall back to changeset-based diff. + """ + if not plan_id or not plan_id.strip(): + return None + + try: + plan = service.get_plan(plan_id) + except (NotFoundError, CleverAgentsError): + return None + + container = get_container() + project_names = [pl.project_name for pl in getattr(plan, "project_links", [])] + + for project_name in project_names: + try: + project = container.namespaced_project_repo().get(project_name) + except (NotFoundError, CleverAgentsError) as exc: + _diff_logger.debug( + "worktree_diff.project_lookup_failed", + project_name=project_name, + error=str(exc), + ) + continue + if project is None: + continue + for lr in getattr(project, "linked_resources", []): + try: + resource = container.resource_registry_service().show_resource( + lr.resource_id, + ) + except (NotFoundError, CleverAgentsError) as exc: + _diff_logger.debug( + "worktree_diff.resource_lookup_failed", + resource_id=lr.resource_id, + error=str(exc), + ) + continue + if ( + resource.resource_type_name not in ("git-checkout", "git") + or not resource.location + ): + continue + + diff = GitWorktreeSandbox.diff_against_head( + resource.location, + plan_id, + ) + if diff is not None: + return diff + + return None + + def _get_apply_service() -> PlanApplyService: """Get the PlanApplyService from the lifecycle service.""" from cleveragents.application.container import get_container @@ -3487,6 +3552,20 @@ def plan_diff( console.print(output) return + # Try worktree branch diff first — this is where LLM output + # lives after plan execute (spec §13225). + try: + worktree_diff = _get_worktree_diff( + plan_id, + _get_lifecycle_service(), + ) + if worktree_diff is not None: + console.print(worktree_diff) + return + except Exception: + pass # Container unavailable; fall through to changeset diff + + # Fall back to changeset-based diff from plan metadata output = service.diff(plan_id, fmt=_fmt) console.print(output) diff --git a/src/cleveragents/infrastructure/sandbox/git_worktree.py b/src/cleveragents/infrastructure/sandbox/git_worktree.py index 7a61779af..f9295a8d1 100644 --- a/src/cleveragents/infrastructure/sandbox/git_worktree.py +++ b/src/cleveragents/infrastructure/sandbox/git_worktree.py @@ -249,7 +249,40 @@ class GitWorktreeSandbox: "Partial cleanup: worktree removed but branch persists: branch=%s", branch_name, ) - return True # Branch was found; cleanup attempted (even if partial) + return True + + @classmethod + def diff_against_head(cls, repo_path: str, plan_id: str) -> str | None: + """Return a unified diff of the worktree branch vs HEAD. + + Args: + repo_path: Absolute path to the git repository root. + plan_id: The plan ULID whose worktree branch to diff. + + Returns: + The diff text, or ``None`` if no worktree branch exists. + """ + branch_name = f"cleveragents/plan-{plan_id}" + + try: + _run_git( + ["rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=repo_path, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + return None + + try: + result = _run_git( + ["diff", f"HEAD...{branch_name}"], + cwd=repo_path, + timeout=30, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + return None + + diff_text = result.stdout.strip() + return diff_text if diff_text else "No changes in worktree branch." # -- protocol methods ----------------------------------------------------