From a9c820610a2e0fc1536f84645cc9770d7d7bba2a Mon Sep 17 00:00:00 2001 From: Hamza Khyari Date: Mon, 20 Apr 2026 13:22:44 +0000 Subject: [PATCH] fix(plan): clean up stale worktree branch before re-creating sandbox When a user re-executes a plan (e.g. after a failed attempt or after reviewing the diff), _create_sandbox_for_plan crashes with 'fatal: a branch named cleveragents/plan- already exists' because the worktree branch from the previous execute was never cleaned up. Add GitWorktreeSandbox.cleanup_stale() classmethod in the infrastructure layer that idempotently removes stale worktree directories and branches. Each git command has explicit error handling with logging and fallback (manual rmtree if worktree remove fails). _create_sandbox_for_plan in the CLI layer delegates to cleanup_stale before calling sandbox.create(). ISSUES CLOSED: #7271 --- CHANGELOG.md | 6 + features/sandbox_reexecute_cleanup.feature | 22 +++ .../steps/sandbox_reexecute_cleanup_steps.py | 132 ++++++++++++++++++ src/cleveragents/cli/commands/plan.py | 18 +++ .../infrastructure/sandbox/git_worktree.py | 95 +++++++++++++ 5 files changed, 273 insertions(+) create mode 100644 features/sandbox_reexecute_cleanup.feature create mode 100644 features/steps/sandbox_reexecute_cleanup_steps.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6807f1be9..68f7f46fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Stale worktree branch cleanup on re-execute** (#7271): Re-executing a plan + after a failed attempt or diff review crashed with `fatal: a branch named + 'cleveragents/plan-' already exists`. Added `GitWorktreeSandbox.cleanup_stale()` + classmethod in the infrastructure layer that idempotently removes stale worktree + directories and branches before creating a fresh sandbox. + - **`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 diff --git a/features/sandbox_reexecute_cleanup.feature b/features/sandbox_reexecute_cleanup.feature new file mode 100644 index 000000000..7f126bc25 --- /dev/null +++ b/features/sandbox_reexecute_cleanup.feature @@ -0,0 +1,22 @@ +@sandbox-reexecute-cleanup +Feature: Stale worktree branch cleanup before re-execute (#7271) + Verifies that re-executing a plan cleans up the stale worktree + branch from the previous execution before creating a fresh sandbox. + + Scenario: cleanup_stale removes existing branch and worktree for srec + Given a temp git repo with a worktree branch for plan "01TESTREEXEC000000000000" for srec + When I call cleanup_stale for plan "01TESTREEXEC000000000000" for srec + Then the branch "cleveragents/plan-01TESTREEXEC000000000000" should not exist for srec + And the worktree directory should not exist for srec + + Scenario: cleanup_stale is idempotent when no branch exists for srec + Given a temp git repo without any worktree branches for srec + When I call cleanup_stale for plan "01TESTNOEXIST00000000000" for srec + Then cleanup_stale should return False for srec + + Scenario: create succeeds after cleanup_stale removes stale branch for srec + Given a temp git repo with a worktree branch for plan "01TESTRECREATE0000000000" for srec + When I call cleanup_stale for plan "01TESTRECREATE0000000000" for srec + And I create a fresh sandbox for plan "01TESTRECREATE0000000000" for srec + Then the fresh sandbox should be a directory for srec + And the branch "cleveragents/plan-01TESTRECREATE0000000000" should exist for srec diff --git a/features/steps/sandbox_reexecute_cleanup_steps.py b/features/steps/sandbox_reexecute_cleanup_steps.py new file mode 100644 index 000000000..69ce6b75e --- /dev/null +++ b/features/steps/sandbox_reexecute_cleanup_steps.py @@ -0,0 +1,132 @@ +"""Steps for sandbox_reexecute_cleanup.feature.""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import tempfile +from pathlib import Path + +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, + ) + + +@given('a temp git repo with a worktree branch for plan "{plan_id}" for srec') +def step_create_repo_with_branch(context: object, plan_id: str) -> None: + d = tempfile.mkdtemp(prefix="srec-") + 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, "file.py").write_text("content\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + + # Create a worktree branch (simulating a previous execute) + branch = f"cleveragents/plan-{plan_id}" + wt_dir = tempfile.mkdtemp(prefix="srec-wt-") + context.add_cleanup(shutil.rmtree, wt_dir, True) + _git(["worktree", "add", "-b", branch, wt_dir, "HEAD"], d) + + context.srec_repo = d + context.srec_wt_dir = wt_dir + context.srec_branch = branch + + +@given("a temp git repo without any worktree branches for srec") +def step_create_clean_repo(context: object) -> None: + d = tempfile.mkdtemp(prefix="srec-clean-") + 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, "file.py").write_text("content\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init"], d) + context.srec_repo = d + + +@when('I call cleanup_stale for plan "{plan_id}" for srec') +def step_call_cleanup_stale(context: object, plan_id: str) -> None: + from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, + ) + + context.srec_cleanup_result = GitWorktreeSandbox.cleanup_stale( + context.srec_repo, + plan_id, + ) + + +@when('I create a fresh sandbox for plan "{plan_id}" for srec') +def step_create_fresh_sandbox(context: object, plan_id: str) -> None: + from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, + ) + + sandbox = GitWorktreeSandbox( + resource_id="res-srec-test", + original_path=context.srec_repo, + ) + ctx = sandbox.create(plan_id) + context.srec_fresh_sandbox = ctx.sandbox_path + context.srec_fresh_sandbox_obj = sandbox + context.add_cleanup(sandbox.cleanup) + + +@then('the branch "{branch_name}" should not exist for srec') +def step_branch_not_exists(context: object, branch_name: str) -> None: + result = subprocess.run( + ["git", "rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=context.srec_repo, + capture_output=True, + check=False, + timeout=10, + ) + assert result.returncode != 0, f"Branch {branch_name} still exists" + + +@then('the branch "{branch_name}" should exist for srec') +def step_branch_exists(context: object, branch_name: str) -> None: + result = subprocess.run( + ["git", "rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=context.srec_repo, + capture_output=True, + check=False, + timeout=10, + ) + assert result.returncode == 0, f"Branch {branch_name} does not exist" + + +@then("the worktree directory should not exist for srec") +def step_worktree_gone(context: object) -> None: + assert not os.path.exists(context.srec_wt_dir), ( + f"Worktree directory still exists: {context.srec_wt_dir}" + ) + + +@then("cleanup_stale should return False for srec") +def step_cleanup_returned_false(context: object) -> None: + assert context.srec_cleanup_result is False, ( + f"Expected False but got {context.srec_cleanup_result}" + ) + + +@then("the fresh sandbox should be a directory for srec") +def step_fresh_sandbox_is_dir(context: object) -> None: + assert os.path.isdir(context.srec_fresh_sandbox), ( + f"Fresh sandbox is not a directory: {context.srec_fresh_sandbox}" + ) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index cf4d817a7..f30c46801 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -1349,6 +1349,20 @@ def _get_lifecycle_service(): return container.plan_lifecycle_service() +def _cleanup_stale_sandbox(repo_path: str, plan_id: str) -> None: + """Remove a stale worktree branch and directory for a plan. + + Delegates to :meth:`GitWorktreeSandbox.cleanup_stale` in the + infrastructure layer. Silently does nothing if no stale branch + exists. + """ + from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, + ) + + GitWorktreeSandbox.cleanup_stale(repo_path, plan_id) + + def _create_sandbox_for_plan( plan_id: str, service: PlanLifecycleService, @@ -1391,6 +1405,10 @@ def _create_sandbox_for_plan( and resource.location and os.path.isdir(os.path.join(resource.location, ".git")) ): + _cleanup_stale_sandbox( + resource.location, + plan_id, + ) sandbox = GitWorktreeSandbox( resource_id=resource.resource_id, original_path=resource.location, diff --git a/src/cleveragents/infrastructure/sandbox/git_worktree.py b/src/cleveragents/infrastructure/sandbox/git_worktree.py index 790baec0a..f4df35497 100644 --- a/src/cleveragents/infrastructure/sandbox/git_worktree.py +++ b/src/cleveragents/infrastructure/sandbox/git_worktree.py @@ -163,6 +163,101 @@ class GitWorktreeSandbox: """Context after creation, ``None`` before ``create``.""" return self._context + # -- class helpers ------------------------------------------------------- + + @classmethod + def cleanup_stale(cls, repo_path: str, plan_id: str) -> bool: + """Remove a stale worktree branch left by a previous execution. + + If a previous ``create()`` left behind a branch + ``cleveragents/plan-``, this method removes the + worktree directory and deletes the branch so that a fresh + sandbox can be created. + + Idempotent — does nothing if no stale branch exists. + + Args: + repo_path: Absolute path to the git repository root. + plan_id: The plan ULID whose stale branch should be removed. + + Returns: + ``True`` if a stale branch was found and cleaned up, + ``False`` if no stale branch existed. + """ + branch_name = f"cleveragents/plan-{plan_id}" + + # Check if the branch exists + try: + _run_git( + ["rev-parse", "--verify", f"refs/heads/{branch_name}"], + cwd=repo_path, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + return False # No stale branch — nothing to clean + + logger.info( + "Cleaning up stale sandbox branch: branch=%s repo=%s", + branch_name, + repo_path, + ) + + # Find and remove the worktree directory + try: + wt_result = _run_git( + ["worktree", "list", "--porcelain"], + cwd=repo_path, + ) + for wt_block in wt_result.stdout.split("\n\n"): + if f"branch refs/heads/{branch_name}" in wt_block: + for line in wt_block.splitlines(): + if line.startswith("worktree "): + wt_path = line.split("worktree ", 1)[1] + try: + _run_git( + ["worktree", "remove", "--force", wt_path], + cwd=repo_path, + ) + except ( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + ): + logger.warning( + "git worktree remove failed; " + "removing directory manually: %s", + wt_path, + ) + shutil.rmtree(wt_path, ignore_errors=True) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + logger.warning( + "Failed to list worktrees for stale cleanup: %s", + branch_name, + ) + + # Delete the branch + try: + _run_git( + ["branch", "-D", branch_name], + cwd=repo_path, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + logger.warning( + "Failed to delete stale branch %s; it may need manual cleanup", + branch_name, + ) + + # Prune stale worktree references + with contextlib.suppress( + subprocess.CalledProcessError, + subprocess.TimeoutExpired, + ): + _run_git(["worktree", "prune"], cwd=repo_path) + + logger.info( + "Stale sandbox branch cleaned up: branch=%s", + branch_name, + ) + return True + # -- protocol methods ---------------------------------------------------- def create(self, plan_id: str) -> SandboxContext: -- 2.52.0