fix(plan): clean up stale worktree branch before re-creating sandbox #10797
@@ -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-<id>' 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
|
||||
|
||||
@@ -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
|
||||
@@ -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}"
|
||||
)
|
||||
@@ -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,
|
||||
|
||||
@@ -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-<plan_id>``, 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:
|
||||
|
||||
Reference in New Issue
Block a user