diff --git a/features/execute_error_recovery.feature b/features/execute_error_recovery.feature new file mode 100644 index 000000000..2d450a29b --- /dev/null +++ b/features/execute_error_recovery.feature @@ -0,0 +1,89 @@ +@execute-error-recovery +Feature: Execute phase error recovery (#10843) + Per spec §28530/§35958 and §18323, errored plans recover via two + paths: transient failures (classified by domain model's + classify_error()) retry with the same strategy, non-transient + failures revert to Strategize via service.try_auto_revert_from_execute(). + + Scenario: Transient error resets plan to queued and re-executes for eer + Given a mocked plan in execute/errored with exception_type "RateLimitError" for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reset to execute/queued for eer + And run_execute should have been called for eer + And the CLI output should contain "transient" for eer + + Scenario: Timeout error is treated as transient for eer + Given a mocked plan in execute/errored with exception_type "TimeoutError" for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reset to execute/queued for eer + And run_execute should have been called for eer + + Scenario: Strategy constraint error reverts via service API then re-executes for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reverted to strategize for eer + And run_strategize should have been called for eer + And run_execute should have been called for eer + And the CLI output should contain "Reverting to Strategize" for eer + + Scenario: Authentication error is non-transient per domain model for eer + Given a mocked plan in execute/errored with exception_type "AuthenticationError" for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reverted to strategize for eer + And run_strategize should have been called for eer + + Scenario: Error findings are preserved and redacted for strategy actor for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + And the mock error_details include sensitive data "bearer_token" for eer + When I invoke plan execute on the errored plan for eer + Then the committed plan should have reversion_reason in error_details for eer + And the committed prior_error_details should not contain "SECRET_VALUE_12345" for eer + + Scenario: Strategy decisions preserved on transient retry for eer + Given a mocked plan in execute/errored with exception_type "RateLimitError" for eer + And the mock error_details include strategy_decisions_json for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reset to execute/queued for eer + And the committed error_details should contain strategy_decisions_json for eer + + Scenario: Strategy revision failure aborts with non-zero exit for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + And the strategy revision will fail for eer + When I invoke plan execute on the errored plan for eer + Then the CLI output should contain "Strategy revision failed" for eer + And the CLI exit code should be non-zero for eer + + Scenario: Plan not found after strategize aborts with non-zero exit for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + And the plan disappears after strategize for eer + When I invoke plan execute on the errored plan for eer + Then the CLI output should contain "not found" for eer + And the CLI exit code should be non-zero for eer + + Scenario: Plan not found after transient recovery aborts for eer + Given a mocked plan in execute/errored with exception_type "RateLimitError" for eer + And the plan disappears after transient recovery for eer + When I invoke plan execute on the errored plan for eer + Then the CLI output should contain "not found" for eer + And the CLI exit code should be non-zero for eer + + Scenario: Empty exception_type triggers strategy reversion for eer + Given a mocked plan in execute/errored with no exception_type for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reverted to strategize for eer + And run_strategize should have been called for eer + And run_execute should have been called for eer + + Scenario: Auto_progress moves plan directly to Execute after strategize for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + And the auto_progress moves the plan to Execute after strategize for eer + When I invoke plan execute on the errored plan for eer + Then the plan should have been reverted to strategize for eer + And run_execute should have been called for eer + + Scenario: MAX_REVERSIONS exceeded aborts with clear message for eer + Given a mocked plan in execute/errored with exception_type "ValueError" for eer + And revert_plan raises PlanError for max reversions for eer + When I invoke plan execute on the errored plan for eer + Then the CLI output should contain "Strategy reversion failed" for eer + And the CLI exit code should be non-zero for eer diff --git a/features/steps/execute_error_recovery_steps.py b/features/steps/execute_error_recovery_steps.py new file mode 100644 index 000000000..8d61d44cd --- /dev/null +++ b/features/steps/execute_error_recovery_steps.py @@ -0,0 +1,279 @@ +"""Steps for execute_error_recovery.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 + +_PLAN_ID = "01HXM8C2ZK4Q7C2B3F2R4VYV6J" + + +@given('a mocked plan in execute/errored with exception_type "{exc_type}" for eer') +def step_mock_errored_plan(context: object, exc_type: str) -> None: + plan = MagicMock() + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.ERRORED + plan.processing_state = ProcessingState.ERRORED + plan.error_details = {"exception_type": exc_type} + plan.identity.plan_id = _PLAN_ID + plan.read_only = False + plan.project_links = [] + plan.automation_profile = None + plan.timestamps = MagicMock() + + mock_service = MagicMock() + mock_service.get_plan.return_value = plan + + def fake_execute_plan(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_plan + + def fake_revert_plan( + pid: str, + to_phase: object = None, + reason: str = "", + ) -> MagicMock: + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.QUEUED + plan.processing_state = ProcessingState.QUEUED + return plan + + mock_service.revert_plan.side_effect = fake_revert_plan + + context.eer_committed_plans = [] + + def fake_commit(p: object) -> None: + p.state = p.processing_state + context.eer_committed_plans.append( + { + "phase": p.phase, + "processing_state": p.processing_state, + "error_details": (dict(p.error_details) if p.error_details else None), + } + ) + + mock_service._commit_plan.side_effect = fake_commit + + mock_executor = MagicMock() + + def fake_run_strategize(pid: str) -> None: + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + + mock_executor.run_strategize.side_effect = fake_run_strategize + + def fake_run_execute(pid: str) -> None: + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + plan.error_details = None + + mock_executor.run_execute.side_effect = fake_run_execute + + context.eer_runner = CliRunner() + context.eer_service = mock_service + context.eer_executor = mock_executor + context.eer_plan = plan + context.eer_exc_type = exc_type + + +@given("a mocked plan in execute/errored with no exception_type for eer") +def step_mock_errored_no_type(context: object) -> None: + # Reuse the main step with empty string + step_mock_errored_plan(context, "") + + +@given("revert_plan raises PlanError for max reversions for eer") +def step_revert_raises_max(context: object) -> None: + from cleveragents.core.exceptions import PlanError + + context.eer_service.revert_plan.side_effect = PlanError( + "Plan has exceeded the maximum number of reversions (3). " + "Manual intervention required." + ) + + +@given("the strategy revision will fail for eer") +def step_strategy_will_fail(context: object) -> None: + plan = context.eer_plan + + def fake_fail(pid: str) -> None: + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.ERRORED + plan.processing_state = ProcessingState.ERRORED + + context.eer_executor.run_strategize.side_effect = fake_fail + + +@given("the plan disappears after strategize for eer") +def step_plan_disappears(context: object) -> None: + plan = context.eer_plan + call_count = {"n": 0} + + def fake_ok(pid: str) -> None: + plan.phase = PlanPhase.STRATEGIZE + plan.state = ProcessingState.COMPLETE + plan.processing_state = ProcessingState.COMPLETE + + context.eer_executor.run_strategize.side_effect = fake_ok + + def get_plan_then_none(pid: str) -> object: + call_count["n"] += 1 + # Calls: 1=errored check, 2=after commit, 3=after revert, + # 4=after strategize → None + if call_count["n"] >= 4: + return None + return plan + + context.eer_service.get_plan.side_effect = get_plan_then_none + + +@given("the plan disappears after transient recovery for eer") +def step_plan_disappears_transient(context: object) -> None: + plan = context.eer_plan + call_count = {"n": 0} + + def get_plan_then_none(pid: str) -> object: + call_count["n"] += 1 + # Calls: 1=caller phase detection, 2=errored check, + # 3=after commit → None + if call_count["n"] >= 3: + return None + return plan + + context.eer_service.get_plan.side_effect = get_plan_then_none + + +@given('the mock error_details include sensitive data "{key}" for eer') +def step_add_sensitive_data(context: object, key: str) -> None: + context.eer_plan.error_details[key] = "SECRET_VALUE_12345" + + +@given("the mock error_details include strategy_decisions_json for eer") +def step_add_strategy_json(context: object) -> None: + context.eer_plan.error_details["strategy_decisions_json"] = '{"root": "decision1"}' + + +@given("the auto_progress moves the plan to Execute after strategize for eer") +def step_auto_progress(context: object) -> None: + plan = context.eer_plan + + def fake_auto(pid: str) -> None: + plan.phase = PlanPhase.EXECUTE + plan.state = ProcessingState.QUEUED + plan.processing_state = ProcessingState.QUEUED + + context.eer_executor.run_strategize.side_effect = fake_auto + + +@when("I invoke plan execute on the errored plan for eer") +def step_invoke_execute(context: object) -> None: + with ( + patch( + "cleveragents.cli.commands.plan._get_lifecycle_service", + return_value=context.eer_service, + ), + patch( + "cleveragents.cli.commands.plan._create_sandbox_for_plan", + return_value=(None, []), + ), + patch( + "cleveragents.cli.commands.plan._get_plan_executor", + return_value=context.eer_executor, + ), + patch("cleveragents.cli.commands.plan._validate_plan_ulid"), + ): + context.eer_result = context.eer_runner.invoke( + plan_app, + ["execute", _PLAN_ID], + ) + context.eer_output = context.eer_result.output + + +@then("the plan should have been reset to execute/queued for eer") +def step_check_reset(context: object) -> None: + committed = context.eer_committed_plans + assert len(committed) >= 1, "Expected at least one _commit_plan call" + first = committed[0] + assert first["phase"] == PlanPhase.EXECUTE + assert first["processing_state"] == ProcessingState.QUEUED + + +@then("run_execute should have been called for eer") +def step_check_run_execute(context: object) -> None: + context.eer_executor.run_execute.assert_called_with(_PLAN_ID) + + +@then("run_strategize should have been called for eer") +def step_check_run_strategize(context: object) -> None: + context.eer_executor.run_strategize.assert_called_with(_PLAN_ID) + + +@then("the plan should have been reverted to strategize for eer") +def step_check_reverted(context: object) -> None: + context.eer_service.revert_plan.assert_called_with( + _PLAN_ID, + PlanPhase.STRATEGIZE, + reason="execute_error_recovery", + ) + + +@then('the CLI output should contain "{text}" for eer') +def step_check_output(context: object, text: str) -> None: + assert text in context.eer_output, ( + f"Expected '{text}' in output:\n{context.eer_output[:500]}" + ) + + +@then("the committed plan should have reversion_reason in error_details for eer") +def step_check_error_details(context: object) -> None: + committed = context.eer_committed_plans + assert len(committed) >= 1 + details = committed[0]["error_details"] + assert details is not None + assert "reversion_reason" in details + assert details["reversion_reason"] == "execute_error_recovery" + + +@then("the CLI exit code should be non-zero for eer") +def step_check_exit_code(context: object) -> None: + assert context.eer_result.exit_code != 0, ( + f"Expected non-zero but got {context.eer_result.exit_code}" + ) + + +@then("the CLI exit code should be zero for eer") +def step_check_exit_code_zero(context: object) -> None: + assert context.eer_result.exit_code == 0, ( + f"Expected zero but got {context.eer_result.exit_code}\n" + f"Output: {context.eer_output[:300]}" + ) + + +@then("the committed error_details should contain strategy_decisions_json for eer") +def step_check_strategy_json(context: object) -> None: + committed = context.eer_committed_plans + assert len(committed) >= 1 + details = committed[0]["error_details"] + assert details is not None + assert "strategy_decisions_json" in details + + +@then('the committed prior_error_details should not contain "{text}" for eer') +def step_check_redacted(context: object, text: str) -> None: + committed = context.eer_committed_plans + assert len(committed) >= 1 + details = committed[0]["error_details"] + assert details is not None + prior = details.get("prior_error_details", "") + assert text not in prior diff --git a/features/steps/tdd_plan_execute_phase_processing_steps.py b/features/steps/tdd_plan_execute_phase_processing_steps.py index bb1fc47ea..e72f26ab1 100644 --- a/features/steps/tdd_plan_execute_phase_processing_steps.py +++ b/features/steps/tdd_plan_execute_phase_processing_steps.py @@ -143,12 +143,14 @@ def step_plan_in_strategize_queued(context: Context) -> None: # get_plan call sequence in the CLI execute_plan handler: # 1. current_plan (phase detection + read-only check — merged) # 2. re-fetch after run_strategize (auto_progress moved to Execute) - # 3. re-fetch for inline execute check - # 4. re-fetch after run_execute + # 3. re-fetch for error recovery check (execute/errored detection) + # 4. re-fetch for inline execute queued check + # 5. re-fetch after run_execute context.mock_service_967.get_plan.side_effect = [ queued_plan, execute_queued_plan, execute_queued_plan, + execute_queued_plan, execute_complete_plan, ] @@ -343,13 +345,15 @@ def step_single_queued_plan_auto_discovery(context: Context) -> None: # get_plan call sequence after auto-discovery selects the plan: # 1. current_plan (phase detection + read-only check — merged) - # 2. re-fetch after run_strategize - # 3. re-fetch for inline execute check - # 4. re-fetch after run_execute + # 2. re-fetch after run_strategize (auto_progress → Execute) + # 3. re-fetch for error recovery check (execute/errored detection) + # 4. re-fetch for inline execute queued check + # 5. re-fetch after run_execute context.mock_service_967.get_plan.side_effect = [ queued_plan, execute_queued_plan, execute_queued_plan, + execute_queued_plan, execute_complete_plan, ] diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 2ad8c188f..8d051f300 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -21,6 +21,7 @@ plan lifecycle. from __future__ import annotations import contextlib +import json import os import re import shutil @@ -34,6 +35,7 @@ from typing import TYPE_CHECKING, Annotated, Any, Literal, cast import structlog import typer from rich.console import Console +from rich.markup import escape as rich_escape from rich.panel import Panel from rich.progress import Progress, SpinnerColumn, TextColumn from rich.table import Table @@ -42,12 +44,17 @@ from sqlalchemy.exc import SQLAlchemyError from cleveragents.a2a.models import A2aRequest from cleveragents.application.container import get_container from cleveragents.cli.formatting import OutputFormat, format_output +from cleveragents.core.error_handling import redact_error_details from cleveragents.core.exceptions import ( CleverAgentsError, NotFoundError, PlanError, ValidationError, ) +from cleveragents.domain.models.core.error_recovery import ( + ErrorCategory, + classify_error, +) from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState from cleveragents.infrastructure.sandbox.git_worktree import ( GitWorktreeSandbox, @@ -229,6 +236,7 @@ app = typer.Typer( ) console = Console() + # Reusable --format option description _FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich)" @@ -1905,6 +1913,148 @@ def _route_sandbox_files_to_worktrees( break +def _recover_errored_execute_plan( + plan_id: str, + service: PlanLifecycleService, + executor: Any, + console: Console, +) -> None: + """Recover a plan stuck in execute/errored. + + Uses the domain model's ``classify_error()`` to determine the error + category, then selects the appropriate recovery path: + + 1. **Transient** (spec: modify_config automation flag): + reset to ``execute/queued`` and + re-execute with the same strategy. Preserves + ``strategy_decisions_json``. + 2. **Non-transient** (spec: Execute → Strategize reversion): delegate to + ``service.try_auto_revert_from_execute()`` which enforces the + ``MAX_REVERSIONS`` loop guard, increments ``reversion_count``, + records a reversion decision, and respects the + ``delete_content`` automation threshold. + + Does nothing if the plan is not in ``execute/errored``. + """ + current_plan = service.get_plan(plan_id) + if ( + current_plan is None + or current_plan.phase != PlanPhase.EXECUTE + or current_plan.state != ProcessingState.ERRORED + ): + return + + error_type = (current_plan.error_details or {}).get( + "exception_type", + "", + ) + error_msg = (current_plan.error_details or {}).get( + "error_message", + "", + ) + category = classify_error(error_msg, error_type) + is_transient = category == ErrorCategory.TRANSIENT + safe_error_type = rich_escape(error_type or "unknown") + + if is_transient: + # Transient retry (spec: modify_config automation flag). + # Reset to queued, re-execute with same strategy. Explicit CLI + # modify_config automation threshold. + console.print( + f"[yellow]Plan is in execute/errored " + f"(transient: {safe_error_type}). " + f"Retrying with same strategy (spec: modify_config).[/yellow]" + ) + # Preserve strategy_decisions_json so _build_decisions() can + # reconstruct the full strategy hierarchy. + strategy_json = (current_plan.error_details or {}).get( + "strategy_decisions_json", + ) + current_plan.processing_state = ProcessingState.QUEUED + current_plan.error_details = None + if strategy_json: + current_plan.error_details = { + "strategy_decisions_json": strategy_json, + } + service._commit_plan(current_plan) + current_plan = service.get_plan(plan_id) + if current_plan is None: + console.print( + f"[red]Plan '{plan_id}' not found after transient recovery.[/red]" + ) + raise typer.Abort() + return + + # Non-transient failure — revert to Strategize + # (spec: Execute → Strategize reversion). + # Use service.revert_plan() (manual reversion entry point) which + # enforces MAX_REVERSIONS loop guard, records a reversion decision, + # and increments reversion_count. Unlike try_auto_revert_from_execute(), + # revert_plan() does NOT check the delete_content automation threshold — + # correct for explicit CLI invocations where the user's intent is clear. + console.print( + f"[yellow]Plan is in execute/errored " + f"(non-transient: {safe_error_type}). " + f"Reverting to Strategize for revised " + f"strategy (spec: Execute → Strategize reversion).[/yellow]" + ) + + # Revert first, then store error findings — prevents data corruption + # if the reversion is blocked (e.g. MAX_REVERSIONS exceeded). + try: + service.revert_plan( + plan_id, + PlanPhase.STRATEGIZE, + reason="execute_error_recovery", + ) + except Exception as revert_err: + console.print( + f"[red]Strategy reversion failed:[/red] {rich_escape(str(revert_err))}" + ) + raise typer.Abort() from revert_err + + # Store redacted error findings for the strategy actor + # (spec: reversion error findings) — only after reversion. + current_plan = service.get_plan(plan_id) + if current_plan is None: + console.print(f"[red]Plan '{plan_id}' not found after reversion.[/red]") + raise typer.Abort() + + prior_errors = redact_error_details( + dict(current_plan.error_details or {}), + ) + current_plan.error_details = { + "reversion_reason": "execute_error_recovery", + "prior_error_type": error_type, + "prior_error_details": json.dumps(prior_errors), + } + service._commit_plan(current_plan) + + # If reversion succeeded, re-run strategize with error findings + if current_plan.phase == PlanPhase.STRATEGIZE: + executor.run_strategize(plan_id) + current_plan = service.get_plan(plan_id) + if current_plan is None: + console.print(f"[red]Plan '{plan_id}' not found after strategize.[/red]") + raise typer.Abort() + + # Transition to execute after revised strategy + if ( + current_plan.phase == PlanPhase.STRATEGIZE + and current_plan.state == ProcessingState.COMPLETE + ): + service.execute_plan(plan_id) + elif current_plan.phase == PlanPhase.EXECUTE: + pass # auto_progress or reversion didn't happen + else: + console.print( + f"[red]Strategy revision failed " + f"({current_plan.phase.value}/" + f"{current_plan.state.value}).[/red]" + ) + raise typer.Abort() + + def _commit_worktree_changes(worktree_path: str, plan_id: str) -> None: """Stage and commit LLM output in the worktree branch. @@ -2727,11 +2877,16 @@ def execute_plan( ) raise typer.Abort() + _recover_errored_execute_plan( + plan_id, + service, + executor, + console, + ) + # Run the execute phase inline so the plan progresses through # execute/queued → execute/processing → execute/complete in a - # single CLI invocation. Without this, `plan execute` would - # leave the plan in execute/queued and `apply` would - # fail because it requires execute/complete. + # single CLI invocation. current_plan = service.get_plan(plan_id) if ( current_plan is not None @@ -2812,8 +2967,13 @@ def execute_plan( # GitWorktreeSandbox.cleanup() is idempotent — safe to call # even after a successful apply (which already cleaned up). for _sinfo in sandbox_infos: - with contextlib.suppress(Exception): + 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")