fix(plan): clean up stale worktree branch before re-creating sandbox #10798

Merged

HAL9000 merged 1 commit from bugfix/sandbox-reexecute-cleanup into master 2026-04-21 12:16:30 +00:00

Conversation 4 Commits 1 Files changed 3 +160

hamza.khyari commented 2026-04-20 13:44:20 +00:00

Member

Copy link

Summary

Re-executing a plan after a failed attempt or diff review crashed with fatal: a branch named 'cleveragents/plan-<id>' already exists because the worktree branch from the previous execute was never cleaned up.

Fix

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().

Changed files

• src/cleveragents/infrastructure/sandbox/git_worktree.py: +97 lines — new cleanup_stale() classmethod
• src/cleveragents/cli/commands/plan.py: -55 +12 lines — delegation to infrastructure layer
• features/sandbox_reexecute_cleanup.feature + step file: 3 Behave scenarios
• CHANGELOG.md: updated

Testing

• M1 E2E: m1-plan-lifecycle-ok
• Lint: passes | Typecheck: 0 errors
• 3 Behave scenarios: stale branch removal, idempotency, create-after-cleanup

Review history

Previous review feedback on PR #10000 (closed due to broken fork metadata):
#10000

All blocking findings from that review have been addressed in this PR.

Closes #7271

## Summary Re-executing a plan after a failed attempt or diff review crashed with `fatal: a branch named 'cleveragents/plan-<id>' already exists` because the worktree branch from the previous execute was never cleaned up. ## Fix 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()`. ## Changed files - `src/cleveragents/infrastructure/sandbox/git_worktree.py`: +97 lines — new `cleanup_stale()` classmethod - `src/cleveragents/cli/commands/plan.py`: -55 +12 lines — delegation to infrastructure layer - `features/sandbox_reexecute_cleanup.feature` + step file: 3 Behave scenarios - `CHANGELOG.md`: updated ## Testing - M1 E2E: `m1-plan-lifecycle-ok` - Lint: passes | Typecheck: 0 errors - 3 Behave scenarios: stale branch removal, idempotency, create-after-cleanup ## Review history Previous review feedback on PR #10000 (closed due to broken fork metadata): https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/10000 All blocking findings from that review have been addressed in this PR. Closes #7271

hamza.khyari added this to the v3.5.0 milestone 2026-04-20 13:44:20 +00:00

hamza.khyari added 1 commit 2026-04-20 13:44:20 +00:00

fix(plan): clean up stale worktree branch before re-creating sandbox ... a9c820610a

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-<id> 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

hamza.khyari added the

State

In Review

Type

Bug

Priority

Medium

labels 2026-04-20 13:45:33 +00:00

HAL9000 was assigned by hamza.khyari 2026-04-20 13:45:39 +00:00

hamza.khyari commented 2026-04-20 13:47:33 +00:00

Author

Member

Copy link

@HAL9000 review this PR merge if it is ready to be merged

@HAL9000 review this PR merge if it is ready to be merged

brent.edwards requested changes 2026-04-20 19:54:11 +00:00

Dismissed

brent.edwards left a comment

Copy link

⚠️ CODE REVIEW – PR #10798 – Clean Up Stale Worktree Branch Before Re-creating Sandbox

Branch: bugfix/sandbox-reexecute-cleanup
Issue: #7271
Status: REQUEST CHANGES (2 issues identified: 1×P2, 1×P3)

---

EXECUTIVE SUMMARY

PR #10798 implements cleanup of stale worktree branches before re-executing a plan, fixing the fatal: a branch named 'cleveragents/plan-<id>' already exists crash. The implementation is well-architected and properly tested but has two issues:

Strengths:

• ✅ Comprehensive error handling with fallback (manual rmtree)
• ✅ Excellent Behave test coverage (3 scenarios, 12 steps, all passing)
• ✅ Good infrastructure-layer abstraction (idempotent cleanup_stale method)
• ✅ Proper layering (infrastructure → CLI)

Issues:

• ⚠️ P2: Misleading partial cleanup logging (always logs success even when branch deletion fails)
• ⚠️ P3: Function-level imports instead of module-level (G2 violation)

---

DETAILED FINDINGS

⚠️ P2 ISSUE: Partial Cleanup Logging Bug

File: src/cleveragents/infrastructure/sandbox/git_worktree.py lines 237–259

Problem: Unconditional success logging even on partial cleanup:

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,
    )

# PROBLEM: Always logs success, even if branch deletion failed!
logger.info(
    "Stale sandbox branch cleaned up: branch=%s",
    branch_name,
)
return True

Impact: Misleading operators — when branch deletion fails, function still logs "Stale sandbox branch cleaned up" even though the branch persists. Violates accurate error reporting principle.

Comparison with PR #10003 (correct pattern):

• ✅ PR #10003 tracks branch_deleted flag
• ✅ Logs info only on success (branch_deleted == True)
• ✅ Logs warning on partial cleanup (branch_deleted == False)
• ✅ Clear, accurate operational visibility

Fix Required:

1. Track branch_deleted flag (or use conditional logging)
2. Conditionally log info only on success
3. Log warning on partial cleanup

Severity: P2 (should-fix) — Undermines operational visibility.

---

⚠️ P3 ISSUE: Function-Level Imports (G2 Violation)

File: src/cleveragents/cli/commands/plan.py

Locations:

• Line 1359–1360: _cleanup_stale_sandbox() imports GitWorktreeSandbox
• Line 1380–1381: _create_sandbox_for_plan() imports GitWorktreeSandbox

Problem: Per review playbook (G2), imports should be at module top-level:

def _cleanup_stale_sandbox(repo_path: str, plan_id: str) -> None:
    """..."""
    from cleveragents.infrastructure.sandbox.git_worktree import (
        GitWorktreeSandbox,
    )  # ← Should be at module top-level, not inside function

Comparison with PR #10003 (correct approach):

• ✅ Imports at module top-level (lines 52–54)
• ✅ Clean, maintainable, follows project conventions

Fix Required: Move both imports to module-level (lines 20–54 section).

Severity: P3 (nit) — Style/consistency, not functional.

---

✅ Test Coverage Analysis

Test Results:

1 feature passed, 0 failed
3 scenarios passed, 0 failed  
12 steps passed, 0 failed

Scenarios Verified:

• ✅ cleanup_stale removes existing branch and worktree
• ✅ cleanup_stale is idempotent when no branch exists (returns False)
• ✅ create succeeds after cleanup_stale removes stale branch

Finding: No issues. Tests are comprehensive and all pass.

---

✅ Exception Handling – Infrastructure

Review: Specific exceptions caught (subprocess.CalledProcessError, subprocess.TimeoutExpired); fallback behavior (manual rmtree) is correct.

Finding: No issues.

---

✅ Idempotency

Review: Returns False if no stale branch exists; no exceptions thrown. Idempotent behavior confirmed by tests.

Finding: No issues.

---

COMPARISON WITH PR #10003

Complementary Use Cases (Not Contradictory)

Aspect	PR #10003	PR #10798
Trigger	agents plan cancel	agents plan execute (re-run)
Called From	cancel_plan() (line 3329)	_create_sandbox_for_plan() (line 1408)
When	AFTER service.cancel_plan()	BEFORE sandbox.create()

These are complementary, not contradictory. Both PRs can coexist.

Contradictions Identified

1. Logging Pattern (P2):

   • ✅ PR #10003: Conditional logging (tracks branch_deleted flag)
   • ❌ PR #10798: Unconditional logging (always logs success)

2. Import Organization (P3):

   • ✅ PR #10003: Module-level imports
   • ❌ PR #10798: Function-level imports

Both contradictions should be resolved by fixing PR #10798 to match PR #10003's patterns.

---

ARCHITECTURE & SECURITY

Architecture: ✅ Proper layering (infrastructure → CLI). Clean separation of concerns.

Security: ✅ No data loss risk (removes worktree copy only). No TOCTOU race. No symlink traversal issues.

Finding: No issues.

---

MERGE READINESS

Status: REQUEST CHANGES (P2 + P3 issues)

Pre-Merge Checklist:

• Unit tests pass
• Architecture sound
• Error handling correct (infrastructure)
• FIX: P2 partial cleanup logging bug
• FIX: P3 import organization
• Verify nox -s lint passes
• Verify nox -s typecheck passes
• Verify nox -s coverage_report ≥ 97%

---

RECOMMENDED FIXES

Fix P2: Partial Cleanup Logging

Replace lines 237–259 in git_worktree.py with:

# Delete the branch
branch_deleted = True
try:
    _run_git(["branch", "-D", branch_name], cwd=repo_path)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired):
    branch_deleted = False
    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)

# Log outcome based on actual success
if branch_deleted:
    logger.info("Stale sandbox branch cleaned up: branch=%s", branch_name)
else:
    logger.warning(
        "Partial cleanup: worktree removed but branch persists: branch=%s",
        branch_name,
    )
return True  # Branch was found; cleanup attempted (even if partial)

Fix P3: Import Organization

Add to module-level imports (near line 52–54):

from cleveragents.infrastructure.sandbox.git_worktree import (
    GitWorktreeSandbox,
)

Remove function-level imports from:

• _cleanup_stale_sandbox() (line 1359–1360)
• _create_sandbox_for_plan() (line 1380–1381)

---

SUMMARY

Issues Found: 2

ID	Type	Severity	Description
1	Bug	P2	Partial cleanup logging (unconditional success log)
2	Style	P3	Function-level imports (G2 violation)

All Other Categories: ✅ APPROVED

Category	Status
Architecture	✅
Infrastructure error handling	✅
CLI integration	✅
Test coverage	✅
Idempotency	✅
Integration with PR #10003	✅ Complementary (after fixes)

---

FINAL RECOMMENDATION

REQUEST CHANGES ⚠️

Next Steps:

1. Fix P2 logging issue (adopt PR #10003's branch_deleted flag pattern)
2. Fix P3 imports (move to module-level)
3. Re-request review after changes
4. Both PRs (#10003 + #10798) will merge cleanly with these fixes

---

Reviewer: Build Agent (haiku-4-5)
Review Type: Comprehensive code review per playbook
Date: 2026-04-20
Result: REQUEST CHANGES

## ⚠️ CODE REVIEW – PR #10798 – Clean Up Stale Worktree Branch Before Re-creating Sandbox **Branch**: `bugfix/sandbox-reexecute-cleanup` **Issue**: #7271 **Status**: **REQUEST CHANGES** (2 issues identified: 1×P2, 1×P3) --- ### EXECUTIVE SUMMARY PR #10798 implements cleanup of stale worktree branches before re-executing a plan, fixing the `fatal: a branch named 'cleveragents/plan-<id>' already exists` crash. The implementation is **well-architected** and **properly tested** but has two issues: **Strengths**: - ✅ Comprehensive error handling with fallback (manual rmtree) - ✅ Excellent Behave test coverage (3 scenarios, 12 steps, all passing) - ✅ Good infrastructure-layer abstraction (idempotent cleanup_stale method) - ✅ Proper layering (infrastructure → CLI) **Issues**: - ⚠️ **P2**: Misleading partial cleanup logging (always logs success even when branch deletion fails) - ⚠️ **P3**: Function-level imports instead of module-level (G2 violation) --- ### DETAILED FINDINGS #### ⚠️ **P2 ISSUE: Partial Cleanup Logging Bug** **File**: `src/cleveragents/infrastructure/sandbox/git_worktree.py` lines 237–259 **Problem**: Unconditional success logging even on partial cleanup: ```python 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, ) # PROBLEM: Always logs success, even if branch deletion failed! logger.info( "Stale sandbox branch cleaned up: branch=%s", branch_name, ) return True ``` **Impact**: Misleading operators — when branch deletion fails, function still logs "Stale sandbox branch cleaned up" even though the branch persists. Violates accurate error reporting principle. **Comparison with PR #10003** (correct pattern): - ✅ PR #10003 tracks `branch_deleted` flag - ✅ Logs info only on success (branch_deleted == True) - ✅ Logs warning on partial cleanup (branch_deleted == False) - ✅ Clear, accurate operational visibility **Fix Required**: 1. Track `branch_deleted` flag (or use conditional logging) 2. Conditionally log info only on success 3. Log warning on partial cleanup **Severity**: **P2 (should-fix)** — Undermines operational visibility. --- #### ⚠️ **P3 ISSUE: Function-Level Imports (G2 Violation)** **File**: `src/cleveragents/cli/commands/plan.py` **Locations**: - Line 1359–1360: `_cleanup_stale_sandbox()` imports GitWorktreeSandbox - Line 1380–1381: `_create_sandbox_for_plan()` imports GitWorktreeSandbox **Problem**: Per review playbook (G2), imports should be at module top-level: ```python def _cleanup_stale_sandbox(repo_path: str, plan_id: str) -> None: """...""" from cleveragents.infrastructure.sandbox.git_worktree import ( GitWorktreeSandbox, ) # ← Should be at module top-level, not inside function ``` **Comparison with PR #10003** (correct approach): - ✅ Imports at module top-level (lines 52–54) - ✅ Clean, maintainable, follows project conventions **Fix Required**: Move both imports to module-level (lines 20–54 section). **Severity**: **P3 (nit)** — Style/consistency, not functional. --- #### ✅ Test Coverage Analysis **Test Results**: ``` 1 feature passed, 0 failed 3 scenarios passed, 0 failed 12 steps passed, 0 failed ``` **Scenarios Verified**: - ✅ cleanup_stale removes existing branch and worktree - ✅ cleanup_stale is idempotent when no branch exists (returns False) - ✅ create succeeds after cleanup_stale removes stale branch **Finding**: **No issues.** Tests are comprehensive and all pass. --- #### ✅ Exception Handling – Infrastructure **Review**: Specific exceptions caught (`subprocess.CalledProcessError`, `subprocess.TimeoutExpired`); fallback behavior (manual `rmtree`) is correct. **Finding**: **No issues.** --- #### ✅ Idempotency **Review**: Returns False if no stale branch exists; no exceptions thrown. Idempotent behavior confirmed by tests. **Finding**: **No issues.** --- ### COMPARISON WITH PR #10003 #### Complementary Use Cases (Not Contradictory) | Aspect | PR #10003 | PR #10798 | |--------|-----------|-----------| | **Trigger** | `agents plan cancel` | `agents plan execute` (re-run) | | **Called From** | cancel_plan() (line 3329) | _create_sandbox_for_plan() (line 1408) | | **When** | AFTER service.cancel_plan() | BEFORE sandbox.create() | These are **complementary**, not contradictory. Both PRs can coexist. #### Contradictions Identified 1. **Logging Pattern** (P2): - ✅ PR #10003: Conditional logging (tracks branch_deleted flag) - ❌ PR #10798: Unconditional logging (always logs success) 2. **Import Organization** (P3): - ✅ PR #10003: Module-level imports - ❌ PR #10798: Function-level imports Both contradictions should be resolved by fixing PR #10798 to match PR #10003's patterns. --- ### ARCHITECTURE & SECURITY **Architecture**: ✅ Proper layering (infrastructure → CLI). Clean separation of concerns. **Security**: ✅ No data loss risk (removes worktree copy only). No TOCTOU race. No symlink traversal issues. **Finding**: **No issues.** --- ### MERGE READINESS **Status**: REQUEST CHANGES (P2 + P3 issues) **Pre-Merge Checklist**: - [x] Unit tests pass - [x] Architecture sound - [x] Error handling correct (infrastructure) - [ ] **FIX**: P2 partial cleanup logging bug - [ ] **FIX**: P3 import organization - [ ] Verify `nox -s lint` passes - [ ] Verify `nox -s typecheck` passes - [ ] Verify `nox -s coverage_report` ≥ 97% --- ### RECOMMENDED FIXES #### Fix P2: Partial Cleanup Logging Replace lines 237–259 in `git_worktree.py` with: ```python # Delete the branch branch_deleted = True try: _run_git(["branch", "-D", branch_name], cwd=repo_path) except (subprocess.CalledProcessError, subprocess.TimeoutExpired): branch_deleted = False 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) # Log outcome based on actual success if branch_deleted: logger.info("Stale sandbox branch cleaned up: branch=%s", branch_name) else: logger.warning( "Partial cleanup: worktree removed but branch persists: branch=%s", branch_name, ) return True # Branch was found; cleanup attempted (even if partial) ``` #### Fix P3: Import Organization Add to module-level imports (near line 52–54): ```python from cleveragents.infrastructure.sandbox.git_worktree import ( GitWorktreeSandbox, ) ``` Remove function-level imports from: - `_cleanup_stale_sandbox()` (line 1359–1360) - `_create_sandbox_for_plan()` (line 1380–1381) --- ### SUMMARY **Issues Found**: 2 | ID | Type | Severity | Description | |----|------|----------|-------------| | 1 | Bug | **P2** | Partial cleanup logging (unconditional success log) | | 2 | Style | **P3** | Function-level imports (G2 violation) | **All Other Categories**: ✅ APPROVED | Category | Status | |----------|--------| | Architecture | ✅ | | Infrastructure error handling | ✅ | | CLI integration | ✅ | | Test coverage | ✅ | | Idempotency | ✅ | | Integration with PR #10003 | ✅ Complementary (after fixes) | --- ### FINAL RECOMMENDATION **REQUEST CHANGES** ⚠️ **Next Steps**: 1. Fix P2 logging issue (adopt PR #10003's branch_deleted flag pattern) 2. Fix P3 imports (move to module-level) 3. Re-request review after changes 4. Both PRs (#10003 + #10798) will merge cleanly with these fixes --- **Reviewer**: Build Agent (haiku-4-5) **Review Type**: Comprehensive code review per playbook **Date**: 2026-04-20 **Result**: REQUEST CHANGES

hamza.khyari force-pushed bugfix/sandbox-reexecute-cleanup from a9c820610a to a0f99368b6 2026-04-20 22:25:12 +00:00 Compare

hamza.khyari commented 2026-04-20 22:27:45 +00:00

Author

Member

Copy link

@brent.edwards Both findings addressed:

Finding	Fix
P2: Unconditional success logging	✅ Added branch_deleted flag — logger.info only on full success, logger.warning("Partial cleanup: worktree removed but branch persists") when branch deletion fails. Matches PR #10003 pattern.
P3: Function-level imports	✅ GitWorktreeSandbox moved to module-level import in plan.py (line 49). Removed both function-level imports from _cleanup_stale_sandbox() and _create_sandbox_for_plan().

Lint passes, typecheck 0 errors, M1 E2E passes. Ready for re-review.

@brent.edwards Both findings addressed: | Finding | Fix | |---|---| | **P2**: Unconditional success logging | ✅ Added `branch_deleted` flag — `logger.info` only on full success, `logger.warning("Partial cleanup: worktree removed but branch persists")` when branch deletion fails. Matches PR #10003 pattern. | | **P3**: Function-level imports | ✅ `GitWorktreeSandbox` moved to module-level import in `plan.py` (line 49). Removed both function-level imports from `_cleanup_stale_sandbox()` and `_create_sandbox_for_plan()`. | Lint passes, typecheck 0 errors, M1 E2E passes. Ready for re-review.

hamza.khyari force-pushed bugfix/sandbox-reexecute-cleanup from a0f99368b6 to 8d1ec814d7 2026-04-20 22:46:06 +00:00 Compare

brent.edwards approved these changes 2026-04-20 22:56:52 +00:00

brent.edwards left a comment

Copy link

✅ CODE REVIEW RE-CHECK – PR #10798 – All Issues Resolved and Fixed

Branch: bugfix/sandbox-reexecute-cleanup (commit: 8d1ec814)
Issue: #7271
Status: ✅ APPROVED FOR MERGE – All previously identified issues are now fixed.

---

VERIFICATION OF FIXES

✅ P2 ISSUE FIXED: Partial Cleanup Logging Bug

Issue: Unconditional success logging even when branch deletion failed

Fix Applied ✅:

• Line 237: branch_deleted = True flag added
• Line 244: Set to False on exception
• Lines 257-266: Conditional logging:
  • logger.info() only when branch_deleted == True
  • logger.warning("Partial cleanup...") when branch_deleted == False

Matches PR #10003 pattern: ✅ Yes, identical

---

✅ P3 ISSUE FIXED: Function-Level Imports (G2 Violation)

Issue: GitWorktreeSandbox imported inside functions instead of module-level

Fix Applied ✅:

• Line 49: Module-level import added: from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox
• Function _cleanup_stale_sandbox() (line 1360): Uses module-level import directly
• Function _create_sandbox_for_plan() (line 1406): Uses module-level import directly
• ✅ No function-level imports in either function

---

TEST COVERAGE VERIFICATION ✅

Test Results:

1 feature passed, 0 failed, 0 skipped
3 scenarios passed, 0 failed, 0 skipped
12 steps passed, 0 failed, 0 skipped

All Scenarios Passing:

1. ✅ cleanup_stale removes existing branch and worktree
2. ✅ cleanup_stale is idempotent when no branch exists (returns False)
3. ✅ create succeeds after cleanup_stale removes stale branch

Verified: Conditional logging works correctly — test output shows success message only when branch is deleted.

---

ALIGNMENT WITH PR #10003 ✅

Now Identical:

Aspect	PR #10003	PR #10798
Partial cleanup logging	✅ Conditional flag	✅ Conditional flag
Success message	✅ "Stale sandbox branch cleaned up"	✅ "Stale sandbox branch cleaned up"
Failure message	✅ "Partial cleanup: worktree removed but branch persists"	✅ "Partial cleanup: worktree removed but branch persists"
Module-level imports	✅ Yes	✅ Yes
Function-level imports	✅ None	✅ None

Result: Both PRs follow the identical pattern and can merge cleanly without conflicts.

---

FINAL SUMMARY

Issue	Resolution
P2: Partial cleanup logging	✅ FIXED – branch_deleted flag added; conditional logging
P3: Function-level imports	✅ FIXED – Moved to module-level (line 49)
Test Coverage	✅ 100% – 3 scenarios, 12 steps, all passing
Alignment with PR #10003	✅ ALIGNED – Identical patterns

---

RECOMMENDATION

Status: ✅ APPROVED FOR MERGE

Rationale:

1. ✅ Both P2 and P3 issues completely fixed
2. ✅ Code now matches PR #10003 pattern exactly
3. ✅ All tests pass (3 scenarios, 12 steps)
4. ✅ Both PRs can merge cleanly without conflicts
5. ✅ Implementation is production-ready

No Further Changes Required — Ready to merge.

---

Reviewer: Build Agent (haiku-4-5)
Review Type: Re-check verification of fixes
Date: 2026-04-20
Commit: 8d1ec814
Result: ✅ APPROVED

## ✅ CODE REVIEW RE-CHECK – PR #10798 – All Issues Resolved and Fixed **Branch**: `bugfix/sandbox-reexecute-cleanup` (commit: 8d1ec814) **Issue**: #7271 **Status**: ✅ **APPROVED FOR MERGE** – All previously identified issues are now fixed. --- ### VERIFICATION OF FIXES #### ✅ **P2 ISSUE FIXED: Partial Cleanup Logging Bug** **Issue**: Unconditional success logging even when branch deletion failed **Fix Applied** ✅: - Line 237: `branch_deleted = True` flag added - Line 244: Set to `False` on exception - Lines 257-266: **Conditional logging**: - `logger.info()` only when `branch_deleted == True` - `logger.warning("Partial cleanup...")` when `branch_deleted == False` **Matches PR #10003 pattern**: ✅ Yes, identical --- #### ✅ **P3 ISSUE FIXED: Function-Level Imports (G2 Violation)** **Issue**: `GitWorktreeSandbox` imported inside functions instead of module-level **Fix Applied** ✅: - Line 49: Module-level import added: `from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox` - Function `_cleanup_stale_sandbox()` (line 1360): Uses module-level import directly - Function `_create_sandbox_for_plan()` (line 1406): Uses module-level import directly - ✅ No function-level imports in either function --- ### TEST COVERAGE VERIFICATION ✅ **Test Results**: ``` 1 feature passed, 0 failed, 0 skipped 3 scenarios passed, 0 failed, 0 skipped 12 steps passed, 0 failed, 0 skipped ``` **All Scenarios Passing**: 1. ✅ cleanup_stale removes existing branch and worktree 2. ✅ cleanup_stale is idempotent when no branch exists (returns False) 3. ✅ create succeeds after cleanup_stale removes stale branch **Verified**: Conditional logging works correctly — test output shows success message only when branch is deleted. --- ### ALIGNMENT WITH PR #10003 ✅ **Now Identical**: | Aspect | PR #10003 | PR #10798 | |--------|-----------|-----------| | Partial cleanup logging | ✅ Conditional flag | ✅ Conditional flag | | Success message | ✅ "Stale sandbox branch cleaned up" | ✅ "Stale sandbox branch cleaned up" | | Failure message | ✅ "Partial cleanup: worktree removed but branch persists" | ✅ "Partial cleanup: worktree removed but branch persists" | | Module-level imports | ✅ Yes | ✅ Yes | | Function-level imports | ✅ None | ✅ None | **Result**: Both PRs follow the **identical pattern** and can merge cleanly without conflicts. --- ### FINAL SUMMARY | Issue | Resolution | |-------|-----------| | **P2**: Partial cleanup logging | ✅ FIXED – `branch_deleted` flag added; conditional logging | | **P3**: Function-level imports | ✅ FIXED – Moved to module-level (line 49) | | **Test Coverage** | ✅ 100% – 3 scenarios, 12 steps, all passing | | **Alignment with PR #10003** | ✅ ALIGNED – Identical patterns | --- ### RECOMMENDATION **Status**: ✅ **APPROVED FOR MERGE** **Rationale**: 1. ✅ Both P2 and P3 issues completely fixed 2. ✅ Code now matches PR #10003 pattern exactly 3. ✅ All tests pass (3 scenarios, 12 steps) 4. ✅ Both PRs can merge cleanly without conflicts 5. ✅ Implementation is production-ready **No Further Changes Required** — Ready to merge. --- **Reviewer**: Build Agent (haiku-4-5) **Review Type**: Re-check verification of fixes **Date**: 2026-04-20 **Commit**: 8d1ec814 **Result**: ✅ **APPROVED**

HAL9000 scheduled this pull request to auto merge when all checks succeed 2026-04-21 02:18:44 +00:00

HAL9000 force-pushed bugfix/sandbox-reexecute-cleanup from b14894f6fe to 5edfe3033e 2026-04-21 08:54:09 +00:00 Compare

HAL9000 force-pushed bugfix/sandbox-reexecute-cleanup from 5edfe3033e to eb74795c50 2026-04-21 10:01:37 +00:00 Compare

HAL9000 force-pushed bugfix/sandbox-reexecute-cleanup from eb74795c50 to 4ac5380060 2026-04-21 10:38:59 +00:00 Compare

hamza.khyari force-pushed bugfix/sandbox-reexecute-cleanup from 4ac5380060 to 5d8bbf22b5 2026-04-21 11:56:38 +00:00 Compare

HAL9000 merged commit 5d8bbf22b5 into master 2026-04-21 12:16:30 +00:00

hamza.khyari referenced this pull request 2026-04-22 11:51:47 +00:00

fix(plan): wire cleanup_stale into _create_sandbox_for_plan #10827

hamza.khyari referenced this pull request from a commit 2026-04-22 11:51:47 +00:00

fix(plan): wire cleanup_stale into _create_sandbox_for_plan

Sign in to join this conversation.

Reviewers

No reviewers

brent.edwards

Labels

Clear labels   

auto/needs-reevaluation

Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  

controller-managed

Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  

auto/blocked-by-deps

PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  

auto/ci-timeout

Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  

auto/claimed-implementer

Currently being processed by an implementer worker.

  

auto/claimed-merge

Currently being processed by the merge driver.

  

auto/claimed-reviewer

Currently being processed by a reviewer worker.

  

auto/driver-down

Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  

auto/invariant-violation

Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  

auto/last-attempt-tier-0

In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-1

In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-2

In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  

auto/last-attempt-tier-min

In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  

Automation Tracking

Tracking issues used by the AI Automation system for agents to communicate and report.

  

auto/needs-conflict-resolution

Rebase conflict needs LLM conflict-resolver.

  

auto/needs-implementer

Failing CI needs implementer attention.

  

auto/postmortem

Documenting a driver incident or rollback.

  

auto/ready-to-merge

Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  

auto/restart-throttled

Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  

auto/revert

Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  

auto/sentinel

Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  

auto/stale-inactivity

No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  

auto/unstable

Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  

Blocked

A ticket in a blocked state and unable to complete until some other task is completed first.

  

Bounty

$100

A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$1000

A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$10000

A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$20

A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$2000

A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$250

A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$50

A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$500

A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$5000

A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$750

A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  

MoSCoW

Could have

Could have feature in order to satisfy the epic/legendary.

  

MoSCoW

Must have

Must have feature in order to satisfy the epic/legendary.

  

MoSCoW

Should have

Should have feature in order to satisfy the epic/legendary.

  

Needs Feedback

There are questions in the ticket that can not be completed until the project owner provides clarity.

  

Points

1

1 man-hours worth of work for an expert with no learning curve.

  

Points

13

13 man-hours worth of work for an expert with no learning curve.

  

Points

2

2 man-hours worth of work for an expert with no learning curve.

  

Points

21

21 man-hours worth of work for an expert with no learning curve.

  

Points

3

3 man-hours worth of work for an expert with no learning curve.

  

Points

34

34 man-hours worth of work for an expert with no learning curve.

  

Points

5

5 man-hours worth of work for an expert with no learning curve.

  

Points

55

55 man-hours worth of work for an expert with no learning curve.

  

Points

8

8 man-hours worth of work for an expert with no learning curve.

  

Points

88

88 man-hours worth of work for an expert with no learning curve.

  

Priority

Backlog

This ticket has backlogged priority and is not to be worked on yet

  

Priority

CI Blocker

Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Signed-off: Owner

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Scrum Master

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Tech Lead

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Spike

A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  

State

Completed

The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  

State

Duplicate

A ticket that represents the same content as an existing ticket.

  

State

In Progress

A ticket that is actively being developed.

  

State

In Review

A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  

State

Paused

This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  

State

Unverified

All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  

State

Verified

The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  

State

Wont Do

This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  

Type

Automation

Any edits or discussion about the AI automated coding system.

  

Type

Bug

Something that doesnt work as intended.

  

Type

Discussion

Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  

Type

Documentation

An error or improvement needed in the documentation.

  

Type

Epic

Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  

Type

Feature

Some new functionality not present.

  

Type

Legendary

A type of Epic which will contain other Epics.

  

Type

Refactor

A code change that restructures existing code without changing its external behavior.

  

Type

Support

Someone needs help using the project.

  

Type

Task

A generic task that doesnt fit into the other type categories.

  

Type

Testing

Work exclusively focusing on fixing or expanding testing.

No labels

auto/needs-reevaluation

controller-managed

auto/blocked-by-deps

auto/ci-timeout

auto/claimed-implementer

auto/claimed-merge

auto/claimed-reviewer

auto/driver-down

auto/invariant-violation

auto/last-attempt-tier-0

auto/last-attempt-tier-1

auto/last-attempt-tier-2

auto/last-attempt-tier-min

Automation Tracking

auto/needs-conflict-resolution

auto/needs-implementer

auto/postmortem

auto/ready-to-merge

auto/restart-throttled

auto/revert

auto/sentinel

auto/stale-inactivity

auto/unstable

Blocked

Bounty

$100

Bounty

$1000

Bounty

$10000

Bounty

$20

Bounty

$2000

Bounty

$250

Bounty

$50

Bounty

$500

Bounty

$5000

Bounty

$750

MoSCoW

Could have

MoSCoW

Must have

MoSCoW

Should have

Needs Feedback

Points

1

Points

13

Points

2

Points

21

Points

3

Points

34

Points

5

Points

55

Points

8

Points

88

Priority

Backlog

Priority

CI Blocker

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Signed-off: Owner

Signed-off: Scrum Master

Signed-off: Tech Lead

Spike

State

Completed

State

Duplicate

State

In Progress

State

In Review

State

Paused

State

Unverified

State

Verified

State

Wont Do

Type

Automation

Type

Bug

Type

Discussion

Type

Documentation

Type

Epic

Type

Feature

Type

Legendary

Type

Refactor

Type

Support

Type

Task

Type

Testing

Milestone

Clear milestone

No items

No milestone

v3.5.0

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

HAL9000

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cleveragents/cleveragents-core!10798

No description provided.