diff --git a/features/sandbox_cleanup_conditional.feature b/features/sandbox_cleanup_conditional.feature new file mode 100644 index 000000000..d35e6e30a --- /dev/null +++ b/features/sandbox_cleanup_conditional.feature @@ -0,0 +1,37 @@ +@sandbox-cleanup-conditional +Feature: Sandbox cleanup only on execute failure (#10872) + Verifies that the execute_plan command's finally block only calls + sandbox_obj.cleanup() when execution fails. On success the worktree + branch survives until ``plan apply`` merges it (spec §13256-13260). + + Scenario: Execute success prevents sandbox cleanup for scco + Given a mocked lifecycle service with a strat-complete plan for scco + And the executor completes successfully for scco + And sandbox_infos contains two active sandboxes for scco + When I invoke plan execute for plan "01TESTSCCPASS0000000000" for scco + Then the CLI exit code should be zero for scco + And sandbox cleanup should NOT have been called for scco + + Scenario: Execute exception triggers sandbox cleanup for scef + Given a mocked lifecycle service with a strat-complete plan for scef + And the executor raises RuntimeError during run_execute for scef + And sandbox_infos contains two active sandboxes for scef + When I invoke plan execute for plan "01TESTSCCFAIL0000000000" for scef + Then the CLI exit code should be non-zero for scef + And sandbox cleanup should have been called for scef + + Scenario: Error recovery reversion triggers sandbox cleanup for sceef + Given a mocked lifecycle service with an execute-errored plan for scef-er + And the executor raises ValueError during error recovery for scef-er + And sandbox_infos contains two active sandboxes for scef-er + When I invoke plan execute for plan "01TESTSCEFERR000000000" for scef-er + Then the CLI exit code should be non-zero for scef-er + And sandbox cleanup should have been called for scef-er + + Scenario: Execute with empty sandbox_infos skips cleanup for sccs + Given a mocked lifecycle service with a strat-complete plan for sccs + And the executor completes successfully for sccs + And sandbox_infos is empty for sccs + When I invoke plan execute for plan "01TESTSCCSKIP000000000000" for sccs + Then the CLI exit code should be zero for sccs + And sandbox cleanup should NOT have been called for sccs diff --git a/features/steps/sandbox_cleanup_conditional_steps.py b/features/steps/sandbox_cleanup_conditional_steps.py new file mode 100644 index 000000000..53f7cefe8 --- /dev/null +++ b/features/steps/sandbox_cleanup_conditional_steps.py @@ -0,0 +1,453 @@ +"""Steps for sandbox_cleanup_conditional.feature.""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from typer.testing import CliRunner + +from cleveragents.cli.commands.plan import app as plan_app +from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState +from cleveragents.infrastructure.sandbox.git_worktree import ( + GitWorktreeSandbox, +) + + +# --------------------------------------------------------------------------- +# Fixture helper: mock sandbox objects +# --------------------------------------------------------------------------- + + +def _make_mock_sandbox_infos(count: int) -> list[object]: + """Return a list of ``_SandboxInfo``-like objects with mocked cleanup. + + Since ``_SandboxInfo`` uses ``__slots__``, we cannot monkey-patch extra + attributes. Instead we pass a MagicMock as ``sandbox_obj`` whose + ``cleanup`` method we can later assert on via ``info.sandbox_obj.cleanup``. + """ + from cleveragents.cli.commands.plan import _SandboxInfo + + infos: list[object] = [] + for i in range(count): + sandbox_obj = MagicMock(spec=GitWorktreeSandbox) + sandbox_obj.cleanup = MagicMock() + sandbox_obj.sandbox_path = f"/tmp/sandbox-{i}" + + info = _SandboxInfo( + sandbox_path=sandbox_obj.sandbox_path, + sandbox_obj=sandbox_obj, + resource_location=f"/tmp/resource-{i}", + project_name=f"project-{i}", + ) + infos.append(info) + return infos + + +# --------------------------------------------------------------------------- +# Context label dispatch helper +# --------------------------------------------------------------------------- + +_LABEL_MAP = { + "scco": "scco", + "scef": "scef", + "scef-er": "scef_er", + "sccs": "sccs", +} + + +def _attr(context: object, label: str, suffix: str) -> object: + """Get a context attribute by label and suffix, e.g. scco_runner.""" + key = _LABEL_MAP.get(label) + if key is None: + raise ValueError(f"Unknown context_label: {label}") + return getattr(context, f"{key}_{suffix}") + + +# --------------------------------------------------------------------------- +# scco - sandbox cleanup conditional on execute success +# --------------------------------------------------------------------------- + + +@given("a mocked lifecycle service with a strat-complete plan for scco") +def step_mock_lifecycle_service_scco(context: object) -> None: + plan = MagicMock() + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + plan.identity.plan_id = "01TESTSCCPASS0000000000" + plan.read_only = False + plan.project_links = [] + plan.automation_profile = None + plan.timestamps = MagicMock() + plan.timestamps.execute_started_at = None + plan.estimation_result = None + plan.decisions = [] + plan.invariants = [] + + mock_service = MagicMock() + + # execute_plan transitions to EXECUTE/QUEUED so run_execute is called + def fake_execute(pid: str) -> MagicMock: + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.QUEUED + plan.processing_state = ProcessingState.QUEUED + return plan + + mock_service.execute_plan.side_effect = fake_execute + mock_service.get_plan.return_value = plan + + context.scco_runner = CliRunner() + context.scco_service = mock_service + context.scco_executor = MagicMock() + context.scco_plan = plan + + +@given("the executor completes successfully for scco") +def step_executor_succeeds_scco(context: object) -> None: + """No side-effect — ``run_execute`` does nothing → success.""" + context.scco_executor.run_execute.side_effect = None + + +@given("sandbox_infos contains two active sandboxes for scco") +def step_two_sandboxes_scco(context: object) -> None: + context.scco_sandbox_infos = _make_mock_sandbox_infos(2) + + context.scco_sandbox_patcher = patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=("/tmp/sandbox-parent", context.scco_sandbox_infos), + ) + context.scco_sandbox_patcher.start() + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(context.scco_sandbox_patcher.stop) + + context.scco_executor_patcher = patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=context.scco_executor, + ) + context.scco_executor_patcher.start() + context._cleanup_handlers.append(context.scco_executor_patcher.stop) + + +@given("a mocked lifecycle service with a strat-complete plan for sccs") +def step_mock_lifecycle_service_sccs(context: object) -> None: + plan = MagicMock() + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + plan.identity.plan_id = "01TESTSCCSKIP000000000000" + plan.read_only = False + plan.project_links = [] + plan.automation_profile = None + plan.timestamps = MagicMock() + plan.timestamps.execute_started_at = None + plan.estimation_result = None + plan.decisions = [] + plan.invariants = [] + + mock_service = MagicMock() + + # execute_plan transitions to EXECUTE/QUEUED so run_execute is called + def fake_execute(pid: str) -> MagicMock: + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.QUEUED + plan.processing_state = ProcessingState.QUEUED + return plan + + mock_service.execute_plan.side_effect = fake_execute + mock_service.get_plan.return_value = plan + + context.sccs_runner = CliRunner() + context.sccs_service = mock_service + context.sccs_executor = MagicMock() + context.sccs_plan = plan + + +@given("the executor completes successfully for sccs") +def step_executor_succeeds_sccs(context: object) -> None: + context.sccs_executor.run_execute.side_effect = None + + +@given("sandbox_infos is empty for sccs") +def step_empty_sandboxes_sccs(context: object) -> None: + context.sccs_sandbox_infos = [] + + context.sccs_sandbox_patcher = patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=(None, []), + ) + context.sccs_sandbox_patcher.start() + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(context.sccs_sandbox_patcher.stop) + + context.sccs_executor_patcher = patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=context.sccs_executor, + ) + context.sccs_executor_patcher.start() + context._cleanup_handlers.append(context.sccs_executor_patcher.stop) + + +# --------------------------------------------------------------------------- +# scef - sandbox cleanup on execute failure +# --------------------------------------------------------------------------- + + +@given("a mocked lifecycle service with a strat-complete plan for scef") +def step_mock_lifecycle_service_scef(context: object) -> None: + plan = MagicMock() + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + plan.identity.plan_id = "01TESTSCCFAIL0000000000" + plan.read_only = False + plan.project_links = [] + plan.automation_profile = None + plan.timestamps = MagicMock() + plan.timestamps.execute_started_at = None + plan.estimation_result = None + plan.decisions = [] + plan.invariants = [] + + mock_service = MagicMock() + + # execute_plan transitions to EXECUTE/QUEUED so run_execute is called + def fake_execute(pid: str) -> MagicMock: + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.QUEUED + plan.processing_state = ProcessingState.QUEUED + return plan + + mock_service.execute_plan.side_effect = fake_execute + mock_service.get_plan.return_value = plan + + context.scef_runner = CliRunner() + context.scef_service = mock_service + context.scef_executor = MagicMock() + context.scef_plan = plan + + +@given("the executor raises RuntimeError during run_execute for scef") +def step_executor_raises_scef(context: object) -> None: + context.scef_executor.run_execute.side_effect = RuntimeError( + "Simulated execute failure" + ) + + +@given("sandbox_infos contains two active sandboxes for scef") +def step_two_sandboxes_scef(context: object) -> None: + context.scef_sandbox_infos = _make_mock_sandbox_infos(2) + + context.scef_sandbox_patcher = patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=("/tmp/sandbox-parent", context.scef_sandbox_infos), + ) + context.scef_sandbox_patcher.start() + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(context.scef_sandbox_patcher.stop) + + context.scef_executor_patcher = patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=context.scef_executor, + ) + context.scef_executor_patcher.start() + context._cleanup_handlers.append(context.scef_executor_patcher.stop) + + +# --------------------------------------------------------------------------- +# scef-er - sandbox cleanup on error-recovery failure +# --------------------------------------------------------------------------- + + +@given("a mocked lifecycle service with an execute-errored plan for scef-er") +def step_mock_execute_errored_scefer(context: object) -> None: + plan = MagicMock() + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.ERRORED + plan.processing_state = ProcessingState.ERRORED + plan.identity.plan_id = "01TESTSCEFERR0000000000" + plan.error_details = {"exception_type": "ValueError"} + plan.read_only = False + plan.project_links = [] + plan.automation_profile = None + plan.timestamps = MagicMock() + plan.timestamps.execute_started_at = None + + mock_service = MagicMock() + mock_service.get_plan.return_value = plan + + # revert_plan raises ValueError → _recover_errored_execute_plan + # converts it to typer.Abort → caught by the outer except → cleanup + mock_service.revert_plan.side_effect = ValueError("Recovery failed") + + context.scef_er_runner = CliRunner() + context.scef_er_service = mock_service + context.scef_er_executor = MagicMock() + context.scef_er_plan = plan + + +@given("the executor raises ValueError during error recovery for scef-er") +def step_executor_raises_value_error(context: object) -> None: + """The ValueError is already wired on service.revert_plan — nothing to do.""" + + +@given("sandbox_infos contains two active sandboxes for scef-er") +def step_two_sandboxes_scefer(context: object) -> None: + context.scef_er_sandbox_infos = _make_mock_sandbox_infos(2) + + context.scef_er_sandbox_patcher = patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=( + "/tmp/sandbox-parent", + context.scef_er_sandbox_infos, + ), + ) + context.scef_er_sandbox_patcher.start() + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(context.scef_er_sandbox_patcher.stop) + + context.scef_er_executor_patcher = patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=context.scef_er_executor, + ) + context.scef_er_executor_patcher.start() + context._cleanup_handlers.append(context.scef_er_executor_patcher.stop) + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when('I invoke plan execute for plan "{plan_id}" for {context_label}') +def step_invoke_execute(context: object, plan_id: str, context_label: str) -> None: + """Invoke ``plan execute`` with the appropriate mocked fixtures.""" + from contextlib import ExitStack + + # Map context_label to the correct attribute prefix + prefix_map = { + "scco": "scco", + "scef": "scef", + "sccs": "sccs", + "scef-er": "scef_er", + } + prefix = prefix_map.get(context_label) + if prefix is None: + raise ValueError(f"Unknown context_label: {context_label}") + + runner = getattr(context, f"{prefix}_runner") + sandboxes = getattr(context, f"{prefix}_sandbox_infos") + executor = getattr(context, f"{prefix}_executor") + svc = getattr(context, f"{prefix}_service") + + with ExitStack() as stack: + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._get_lifecycle_service", + return_value=svc, + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=("/tmp/sandbox-parent", sandboxes), + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=executor, + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._validate_plan_ulid", + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._route_sandbox_files_to_worktrees", + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._commit_worktree_changes", + ) + ) + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._notify_facade", + ) + ) + # For error-recovery scenarios the real _recover_errored_execute_plan + # must run so the error propagates. For all others we stub it out. + if context_label != "scef-er": + stack.enter_context( + patch( + "cleveragents.cli.commands.plan._recover_errored_execute_plan", + ) + ) + + context.sandbox_result = runner.invoke(plan_app, ["execute", plan_id]) + + context.sandbox_output = context.sandbox_result.output + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +def _get_sandbox_infos(context: object, context_label: str) -> list[object]: + """Return the sandbox_infos list for a given context label.""" + prefix_map = { + "scco": "scco", + "scef": "scef", + "sccs": "sccs", + "scef-er": "scef_er", + } + prefix = prefix_map.get(context_label) + if prefix is None: + raise AssertionError(f"Unknown context_label: {context_label}") + return getattr(context, f"{prefix}_sandbox_infos") + + +@then("sandbox cleanup should have been called for {context_label}") +def step_cleanup_called(context: object, context_label: str) -> None: + """Verify every sandbox_obj in the infos called cleanup().""" + sandboxes = _get_sandbox_infos(context, context_label) + for info in sandboxes: + assert info.sandbox_obj.cleanup.called, ( + f"Expected sandbox_obj.cleanup() to be called, " + f"but it was not for sandbox {info.sandbox_path}" + ) + + +@then("sandbox cleanup should NOT have been called for {context_label}") +def step_cleanup_not_called(context: object, context_label: str) -> None: + """Verify no sandbox_obj cleanup() was called.""" + sandboxes = _get_sandbox_infos(context, context_label) + for info in sandboxes: + assert not info.sandbox_obj.cleanup.called, ( + f"Expected sandbox_obj.cleanup() NOT to be called, " + f"but it was called for sandbox {info.sandbox_path}" + ) + + +@then("the CLI exit code should be zero for {context_label}") +def step_check_success(context: object, context_label: str) -> None: + assert context.sandbox_result.exit_code == 0, ( + f"Expected zero but got {context.sandbox_result.exit_code}\n" + f"Output: {context.sandbox_output[:500]}" + ) + + +@then("the CLI exit code should be non-zero for {context_label}") +def step_check_failure(context: object, context_label: str) -> None: + assert context.sandbox_result.exit_code != 0, ( + f"Expected non-zero but got {context.sandbox_result.exit_code}" + ) diff --git a/src/cleveragents/application/services/llm_actors.py b/src/cleveragents/application/services/llm_actors.py index ee5f15db9..398e58ef2 100644 --- a/src/cleveragents/application/services/llm_actors.py +++ b/src/cleveragents/application/services/llm_actors.py @@ -486,6 +486,7 @@ class LLMExecuteActor: r"FILE:\s*(.+?)\s*\n```[^\n]*\n(.*?)```", re.DOTALL, ) + for match in pattern.finditer(llm_output): path = match.group(1).strip() content = match.group(2) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 8d051f300..296f37077 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -2757,6 +2757,7 @@ def execute_plan( ) sandbox_infos: list[_SandboxInfo] = [] + execute_succeeded = False try: from cleveragents.domain.models.core.plan import ( PlanPhase, @@ -2941,6 +2942,10 @@ def execute_plan( "Run 'agents plan execute ' to continue.[/dim]" ) + # Mark execute as successful before any teardown — the worktree + # branch survives until ``plan apply`` merges it into the project. + execute_succeeded = True + except PreflightRejection as e: console.print(f"[red]Pre-flight check failed:[/red] {e}") raise typer.Abort() from e @@ -2963,17 +2968,24 @@ def execute_plan( console.print(f"[red]Unexpected error:[/red] {e}") raise typer.Abort() from e finally: - # M4: cleanup sandboxes on any failure path. - # GitWorktreeSandbox.cleanup() is idempotent — safe to call - # even after a successful apply (which already cleaned up). - for _sinfo in sandbox_infos: - try: - _sinfo.sandbox_obj.cleanup() - except Exception: - structlog.get_logger(__name__).warning( - "sandbox_cleanup_failed", - sandbox_path=getattr(_sinfo, "sandbox_path", "unknown"), - ) + # Cleanup sandboxes only on failure — on success the worktree + # branch must survive until ``plan apply`` merges it into the + # project. We use an explicit flag instead of re-reading the + # plan from storage in-flight (which would be racy and fragile + # if an earlier exception handler already mutated the plan). + if not execute_succeeded: + for _sinfo in sandbox_infos: + try: + _sinfo.sandbox_obj.cleanup() + except Exception: + structlog.get_logger(__name__).warning( + "sandbox_cleanup_failed", + sandbox_path=getattr( + _sinfo, + "sandbox_path", + "unknown", + ), + ) @app.command("apply")