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

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

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

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