diff --git a/CHANGELOG.md b/CHANGELOG.md index 5993ce47f..21b010a42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`plan apply` merge conflict cleanup** (#7250): When `git merge` fails due to + a conflict during `plan apply`, the apply command now reads the actual conflict + detail from `CalledProcessError.stdout` (git writes conflict info to stdout, not + stderr), runs `git merge --abort` to restore the repo to a clean state, transitions + the plan to `constrained` state per spec §18334-18336 (may revert to Strategize + for re-planning), and prints user-friendly guidance. Also handles + `subprocess.TimeoutExpired` on both merge and abort calls. Previously, merge + conflicts left conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`) in project files + and the plan remained in `apply/queued` indefinitely. + - **Actor v3 YAML Schema Validation in CLI** (#5869): The `agents actor add --config` command now validates v3 YAML files using `ActorConfigSchema`, ensuring proper schema compliance including cycle detection for GRAPH actors, required field diff --git a/features/merge_conflict_abort.feature b/features/merge_conflict_abort.feature new file mode 100644 index 000000000..7d88e4f13 --- /dev/null +++ b/features/merge_conflict_abort.feature @@ -0,0 +1,55 @@ +@merge-conflict-abort +Feature: Plan apply aborts merge on conflict (#7250) + Verifies that when plan apply encounters a git merge conflict, + the merge is aborted and the project is left in a clean state. + Also covers timeout handling and flat file copy failures. + + Scenario: Merge conflict aborts cleanly and repo stays clean for mca + Given a temp git project with a file "config.py" for mca + And a worktree branch with a conflicting change to "config.py" for mca + And the user commits a different change to "config.py" on main for mca + When I attempt to merge the worktree branch for mca + Then the merge should fail for mca + And the merge should be aborted for mca + And "config.py" should not contain conflict markers for mca + And git status should be clean for mca + + Scenario: Merge abort failure warns user about unclean state for mca + Given a temp git project with a file "data.txt" for mca + And a worktree branch with a conflicting change to "data.txt" for mca + And the user commits a different change to "data.txt" on main for mca + When I attempt to merge the worktree branch and the abort fails for mca + Then the merge should fail for mca + And the abort failure should be reported for mca + + Scenario: _apply_sandbox_changes returns False on merge conflict for mca + Given a temp git project with a file "app.py" for mca + And a worktree branch with a conflicting change to "app.py" for mca + And the user commits a different change to "app.py" on main for mca + When I call _apply_sandbox_changes with the conflicting project for mca + Then _apply_sandbox_changes should return False for mca + And "app.py" should not contain conflict markers for mca + And git status should be clean for mca + + Scenario: _apply_sandbox_changes returns True on clean merge for mca + Given a temp git project with a file "clean.py" for mca + And a worktree branch with a non-conflicting change for mca + When I call _apply_sandbox_changes with the clean project for mca + Then _apply_sandbox_changes should return True for mca + + Scenario: Merge timeout returns False and advises manual cleanup for mca + Given a mock subprocess that raises TimeoutExpired on merge for mca + When I call _apply_sandbox_changes with the mocked merge for mca + Then _apply_sandbox_changes should return False for mca + And the timeout error message should be displayed for mca + + Scenario: Abort timeout returns False and advises manual cleanup for mca + Given a mock subprocess that raises TimeoutExpired on abort for mca + When I call _apply_sandbox_changes with the mocked abort for mca + Then _apply_sandbox_changes should return False for mca + And the abort timeout message should be displayed for mca + + Scenario: Flat file copy failure returns False for mca + Given a temp sandbox with a file that cannot be copied for mca + When I call _apply_sandbox_changes with the failing flat copy for mca + Then _apply_sandbox_changes should return False for mca diff --git a/features/steps/merge_conflict_abort_steps.py b/features/steps/merge_conflict_abort_steps.py new file mode 100644 index 000000000..bef69c059 --- /dev/null +++ b/features/steps/merge_conflict_abort_steps.py @@ -0,0 +1,485 @@ +"""Steps for merge_conflict_abort.feature.""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import tempfile +from io import StringIO +from pathlib import Path +from unittest.mock import MagicMock, patch + +from behave import given, then, when + + +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, + ) + + +# ── Shared setup steps ───────────────────────────────── + + +@given('a temp git project with a file "{filename}" for mca') +def step_create_project(context: object, filename: str) -> None: + d = tempfile.mkdtemp(prefix="mca-") + context.add_cleanup(shutil.rmtree, d, True) + _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, filename).write_text("original content\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + context.mca_project = d + context.mca_plan_id = "01TEST00000000000000CONFLICT" + context.mca_branch = f"cleveragents/plan-{context.mca_plan_id}" + + +@given('a worktree branch with a conflicting change to "{filename}" for mca') +def step_create_worktree_branch(context: object, filename: str) -> None: + repo = context.mca_project + branch = context.mca_branch + _git(["checkout", "-b", branch], repo) + Path(repo, filename).write_text("branch change\n") + _git(["add", "."], repo) + _git(["commit", "-q", "-m", "branch edit"], repo) + _git(["checkout", "main"], repo) + + +@given('the user commits a different change to "{filename}" on main for mca') +def step_user_edits_main(context: object, filename: str) -> None: + repo = context.mca_project + Path(repo, filename).write_text("user change\n") + _git(["add", "."], repo) + _git(["commit", "-q", "-m", "user edit"], repo) + + +@given("a worktree branch with a non-conflicting change for mca") +def step_create_non_conflicting_branch(context: object) -> None: + repo = context.mca_project + branch = context.mca_branch + _git(["checkout", "-b", branch], repo) + Path(repo, "new_file.py").write_text("# new file\n") + _git(["add", "."], repo) + _git(["commit", "-q", "-m", "add new file"], repo) + _git(["checkout", "main"], repo) + + +# ── Helper: build mocks for _apply_sandbox_changes ───── + + +def _build_apply_mocks( + context: object, + repo_path: str, + plan_id: str, + branch_name: str, +) -> tuple[MagicMock, MagicMock]: + """Build mock service + container for _apply_sandbox_changes.""" + mock_resource = MagicMock() + mock_resource.resource_type_name = "git-checkout" + mock_resource.location = repo_path + mock_resource.resource_id = "res-mca-test" + + mock_lr = MagicMock() + mock_lr.resource_id = "res-mca-test" + + mock_project = MagicMock() + mock_project.linked_resources = [mock_lr] + + mock_plan = MagicMock() + mock_plan.project_links = [MagicMock(project_name="local/mca-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 + + return mock_service, mock_container + + +def _call_apply_sandbox( + context: object, + mock_service: MagicMock, + mock_container: MagicMock, +) -> bool: + """Call _apply_sandbox_changes with mocked dependencies.""" + from rich.console import Console + + from cleveragents.cli.commands.plan import _apply_sandbox_changes + + output = StringIO() + console = Console(file=output, width=200) + + with patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ): + result = _apply_sandbox_changes( + context.mca_plan_id, + mock_service, + console, + ) + + context.mca_apply_result = result + context.mca_console_output = output.getvalue() + return result + + +# ── Raw git merge steps (scenarios 1-2) ──────────────── + + +@when("I attempt to merge the worktree branch for mca") +def step_attempt_merge(context: object) -> None: + repo = context.mca_project + branch = context.mca_branch + result = subprocess.run( + [ + "git", + "-c", + "commit.gpgsign=false", + "merge", + branch, + "--no-edit", + "-m", + "test merge", + ], + cwd=repo, + capture_output=True, + text=True, + check=False, + timeout=10, + ) + context.mca_merge_rc = result.returncode + + if result.returncode != 0: + abort_result = subprocess.run( + ["git", "merge", "--abort"], + cwd=repo, + capture_output=True, + check=False, + timeout=10, + ) + context.mca_abort_rc = abort_result.returncode + else: + context.mca_abort_rc = None + + +@when("I attempt to merge the worktree branch and the abort fails for mca") +def step_attempt_merge_abort_fails(context: object) -> None: + repo = context.mca_project + branch = context.mca_branch + result = subprocess.run( + [ + "git", + "-c", + "commit.gpgsign=false", + "merge", + branch, + "--no-edit", + "-m", + "test merge", + ], + cwd=repo, + capture_output=True, + text=True, + check=False, + timeout=10, + ) + context.mca_merge_rc = result.returncode + + if result.returncode != 0: + subprocess.run( + ["git", "merge", "--abort"], + cwd=repo, + capture_output=True, + check=False, + timeout=10, + ) + abort_result = subprocess.run( + ["git", "merge", "--abort"], + cwd=repo, + capture_output=True, + check=False, + timeout=10, + ) + context.mca_abort_rc = abort_result.returncode + else: + context.mca_abort_rc = 0 + + +# ── _apply_sandbox_changes integration steps (scenarios 3-4) ── + + +@when("I call _apply_sandbox_changes with the conflicting project for mca") +def step_call_apply_conflict(context: object) -> None: + mock_service, mock_container = _build_apply_mocks( + context, + context.mca_project, + context.mca_plan_id, + context.mca_branch, + ) + _call_apply_sandbox(context, mock_service, mock_container) + + +@when("I call _apply_sandbox_changes with the clean project for mca") +def step_call_apply_clean(context: object) -> None: + mock_service, mock_container = _build_apply_mocks( + context, + context.mca_project, + context.mca_plan_id, + context.mca_branch, + ) + _call_apply_sandbox(context, mock_service, mock_container) + + +# ── Timeout mock steps (scenarios 5-6) ───────────────── + + +@given("a mock subprocess that raises TimeoutExpired on merge for mca") +def step_mock_merge_timeout(context: object) -> None: + d = tempfile.mkdtemp(prefix="mca-timeout-") + context.add_cleanup(shutil.rmtree, d, True) + _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, "f.py").write_text("x\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + # Create the branch so rev-parse finds it + _git(["checkout", "-b", "cleveragents/plan-01TESTTIMEOUT0000000000000"], d) + Path(d, "f.py").write_text("changed\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "change"], d) + _git(["checkout", "main"], d) + context.mca_project = d + context.mca_plan_id = "01TESTTIMEOUT0000000000000" + context.mca_branch = "cleveragents/plan-01TESTTIMEOUT0000000000000" + context.mca_timeout_target = "merge" + + +@given("a mock subprocess that raises TimeoutExpired on abort for mca") +def step_mock_abort_timeout(context: object) -> None: + d = tempfile.mkdtemp(prefix="mca-timeout-") + context.add_cleanup(shutil.rmtree, d, True) + _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, "f.py").write_text("original\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + # Create conflicting branch + _git(["checkout", "-b", "cleveragents/plan-01TESTABORTTIMEOUT000000000"], d) + Path(d, "f.py").write_text("branch\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "branch"], d) + _git(["checkout", "main"], d) + Path(d, "f.py").write_text("main\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "main"], d) + context.mca_project = d + context.mca_plan_id = "01TESTABORTTIMEOUT000000000" + context.mca_branch = "cleveragents/plan-01TESTABORTTIMEOUT000000000" + context.mca_timeout_target = "abort" + + +@when("I call _apply_sandbox_changes with the mocked merge for mca") +def step_call_apply_merge_timeout(context: object) -> None: + mock_service, mock_container = _build_apply_mocks( + context, + context.mca_project, + context.mca_plan_id, + context.mca_branch, + ) + + original_run = subprocess.run + + def _timeout_on_merge(*args: object, **kwargs: object) -> object: + cmd = args[0] if args else kwargs.get("args", []) + if isinstance(cmd, list) and "merge" in cmd and "--abort" not in cmd: + raise subprocess.TimeoutExpired(cmd, 30) + return original_run(*args, **kwargs) + + with patch("subprocess.run", side_effect=_timeout_on_merge): + _call_apply_sandbox(context, mock_service, mock_container) + + +@when("I call _apply_sandbox_changes with the mocked abort for mca") +def step_call_apply_abort_timeout(context: object) -> None: + mock_service, mock_container = _build_apply_mocks( + context, + context.mca_project, + context.mca_plan_id, + context.mca_branch, + ) + + original_run = subprocess.run + merge_done = {"value": False} + + def _timeout_on_abort(*args: object, **kwargs: object) -> object: + cmd = args[0] if args else kwargs.get("args", []) + if isinstance(cmd, list) and "merge" in cmd: + if "--abort" in cmd: + raise subprocess.TimeoutExpired(cmd, 10) + # Let the merge fail with conflict (use original) + merge_done["value"] = True + return original_run(*args, **kwargs) + return original_run(*args, **kwargs) + + with patch("subprocess.run", side_effect=_timeout_on_abort): + _call_apply_sandbox(context, mock_service, mock_container) + + +# ── Flat file copy failure step (scenario 7) ─────────── + + +@given("a temp sandbox with a file that cannot be copied for mca") +def step_create_failing_sandbox(context: object) -> None: + d = tempfile.mkdtemp(prefix="mca-flat-") + context.add_cleanup(shutil.rmtree, d, True) + sandbox = os.path.join(d, ".cleveragents", "sandbox") + os.makedirs(sandbox) + Path(sandbox, "output.py").write_text("# generated\n") + # Create a read-only destination directory to cause copy failure + dst_dir = os.path.join(d, "readonly_dir") + os.makedirs(dst_dir) + Path(dst_dir, "output.py").write_text("# original\n") + os.chmod(dst_dir, 0o444) + context.add_cleanup(os.chmod, dst_dir, 0o755) + context.mca_flat_project = d + context.mca_plan_id = "01TESTFLATFAIL00000000000000" + + +@when("I call _apply_sandbox_changes with the failing flat copy for mca") +def step_call_apply_flat_fail(context: object) -> None: + from rich.console import Console + + from cleveragents.cli.commands.plan import _apply_sandbox_changes + + # Mock service with no git resources (forces flat copy path) + mock_plan = MagicMock() + mock_plan.project_links = [] + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_container = MagicMock() + + output = StringIO() + console = Console(file=output, width=200) + + # Patch os.getcwd to return our test dir (flat copy uses cwd) + with ( + patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ), + patch( + "cleveragents.cli.commands.plan.os.getcwd", + return_value=context.mca_flat_project, + ), + patch( + "cleveragents.cli.commands.plan.shutil.copy2", + side_effect=OSError("Permission denied"), + ), + ): + result = _apply_sandbox_changes( + context.mca_plan_id, + mock_service, + console, + ) + + context.mca_apply_result = result + context.mca_console_output = output.getvalue() + + +# ── Then assertions ──────────────────────────────────── + + +@then("the merge should fail for mca") +def step_merge_failed(context: object) -> None: + assert context.mca_merge_rc != 0, ( + f"Expected merge to fail but got rc={context.mca_merge_rc}" + ) + + +@then("the merge should be aborted for mca") +def step_merge_aborted(context: object) -> None: + assert context.mca_abort_rc == 0, ( + f"Expected merge abort to succeed but got rc={context.mca_abort_rc}" + ) + + +@then("the abort failure should be reported for mca") +def step_abort_failure_reported(context: object) -> None: + assert context.mca_abort_rc != 0, ( + f"Expected abort to fail but got rc={context.mca_abort_rc}" + ) + + +@then('"{filename}" should not contain conflict markers for mca') +def step_no_conflict_markers(context: object, filename: str) -> None: + content = Path(context.mca_project, filename).read_text() + for marker in ("<<<<<<<", "=======", ">>>>>>>"): + assert marker not in content, f"Found conflict marker '{marker}' in {filename}" + + +@then("git status should be clean for mca") +def step_git_clean(context: object) -> None: + result = subprocess.run( + ["git", "status", "--porcelain"], + cwd=context.mca_project, + capture_output=True, + text=True, + check=True, + timeout=10, + ) + assert result.stdout.strip() == "", ( + f"Expected clean git status but got:\n{result.stdout}" + ) + + +@then("_apply_sandbox_changes should return False for mca") +def step_apply_returns_false(context: object) -> None: + assert context.mca_apply_result is False, ( + f"Expected False but got {context.mca_apply_result}" + ) + + +@then("_apply_sandbox_changes should return True for mca") +def step_apply_returns_true(context: object) -> None: + assert context.mca_apply_result is True, ( + f"Expected True but got {context.mca_apply_result}" + ) + + +@then("the timeout error message should be displayed for mca") +def step_timeout_message(context: object) -> None: + output = context.mca_console_output + assert "timed out" in output.lower(), ( + f"Expected timeout message in output:\n{output}" + ) + + +@then("the abort timeout message should be displayed for mca") +def step_abort_timeout_message(context: object) -> None: + output = context.mca_console_output + assert "timed out" in output.lower(), ( + f"Expected abort timeout message in output:\n{output}" + ) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 9f46bdf35..cf4d817a7 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -20,6 +20,7 @@ plan lifecycle. from __future__ import annotations +import contextlib import os import re import shutil @@ -1407,7 +1408,7 @@ def _apply_sandbox_changes( plan_id: str, service: PlanLifecycleService, console: Console, -) -> None: +) -> bool: """Apply sandbox changes to the project. Tries git worktree merge first. If the plan's sandbox was a git @@ -1416,6 +1417,10 @@ def _apply_sandbox_changes( Otherwise falls back to flat file copy from ``.cleveragents/sandbox/``. + Returns: + ``True`` if changes were applied successfully, ``False`` if + the merge failed (conflict, timeout, etc.). + Spec reference: ``specification.md`` §13241-13276. """ import subprocess @@ -1514,9 +1519,53 @@ def _apply_sandbox_changes( check=True, timeout=30, ) + except subprocess.TimeoutExpired: + console.print( + "[red]Merge timed out.[/red]\n" + "[yellow]Run 'git merge --abort' manually " + "to clean up the repository.[/yellow]" + ) + return False except subprocess.CalledProcessError as merge_err: - console.print(f"[red]Merge failed:[/red] {merge_err.stderr.strip()}") - return + # Git writes conflict info to stdout, not stderr + detail = (merge_err.stdout or merge_err.stderr or "").strip() + if not detail: + detail = "Unknown merge error" + console.print(f"[red]Merge failed:[/red] {detail}") + # Abort the merge to leave the repo in a clean state + try: + abort_result = subprocess.run( + ["git", "merge", "--abort"], + cwd=repo_path, + capture_output=True, + check=False, + timeout=10, + ) + except subprocess.TimeoutExpired: + console.print( + "[red]Merge abort timed out.[/red]\n" + "[yellow]Run 'git merge --abort' manually " + "to clean up the repository.[/yellow]" + ) + return False + if abort_result.returncode == 0: + console.print( + "[yellow]Merge aborted — project is unchanged. " + "Resolve conflicts manually or re-run " + "the plan.[/yellow]" + ) + else: + abort_err = ( + abort_result.stdout or abort_result.stderr or "" + ).strip() + if not abort_err: + abort_err = "Unknown error" + console.print( + f"[red]Merge abort failed:[/red] {abort_err}\n" + "[yellow]Run 'git merge --abort' manually " + "to clean up the repository.[/yellow]" + ) + return False applied_at = datetime.now().strftime("%Y-%m-%d %H:%M") @@ -1595,7 +1644,7 @@ def _apply_sandbox_changes( # ── Footer (spec §13276) ── console.print("[green]✓ OK[/green] Changes applied") - return # Done — merged successfully + return True # Done — merged successfully # Fallback: flat file copy from .cleveragents/sandbox/ sandbox_root = os.path.join(os.getcwd(), ".cleveragents", "sandbox") @@ -1603,7 +1652,7 @@ def _apply_sandbox_changes( _skip_dirs = frozenset({".cleveragents", ".git", ".hg", ".svn"}) if not os.path.isdir(sandbox_root): - return + return True # No sandbox — nothing to apply, not an error applied_count = 0 failed_count = 0 @@ -1633,6 +1682,8 @@ def _apply_sandbox_changes( if applied_count > 0: console.print("[green]✓ OK[/green] Changes applied") + return failed_count == 0 + def _commit_worktree_changes(worktree_path: str, plan_id: str) -> None: """Stage and commit LLM output in the worktree branch. @@ -2638,7 +2689,21 @@ def lifecycle_apply_plan( # Apply changeset: merge the git worktree branch back into # the project, or fall back to flat file copy for non-git # projects. - _apply_sandbox_changes(plan_id, service, console) + apply_ok = _apply_sandbox_changes(plan_id, service, console) + if not apply_ok: + # Transition plan to constrained state per spec §18334-18336 + with contextlib.suppress(Exception): + service.constrain_apply( + plan_id, + "Merge conflict during apply. Resolve conflicts " + "manually or run 'agents plan execute' to re-plan.", + ) + console.print( + "[red]Apply failed — plan is constrained.[/red]\n" + "[yellow]Resolve conflicts manually or run " + "'agents plan execute' to re-plan.[/yellow]" + ) + raise typer.Abort() # Complete the apply phase to terminal state. plan = service.get_plan(plan_id)