fix(plan): implement error recovery for execute phase per spec §35958 and §18323
CI / lint (pull_request) Successful in 1m8s
CI / quality (pull_request) Successful in 1m7s
CI / typecheck (pull_request) Successful in 1m31s
CI / security (pull_request) Successful in 1m39s
CI / helm (pull_request) Successful in 28s
CI / push-validation (pull_request) Successful in 24s
CI / build (pull_request) Successful in 36s
CI / integration_tests (pull_request) Successful in 4m21s
CI / unit_tests (pull_request) Successful in 4m45s
CI / e2e_tests (pull_request) Successful in 4m8s
CI / docker (pull_request) Successful in 1m29s
CI / benchmark-publish (pull_request) Has been skipped
CI / coverage (pull_request) Successful in 11m24s
CI / status-check (pull_request) Successful in 4s
CI / benchmark-regression (pull_request) Successful in 1h10m52s

Two recovery paths for execute/errored plans using the domain model's
classify_error() for canonical error classification:

1. Transient failures (§28530/§35958): rate limit, timeout, connection
   errors — reset to execute/queued and re-execute with the same
   strategy.  Preserves strategy_decisions_json for decision
   hierarchy reconstruction.

2. Non-transient failures (§18323-18329): strategy constraint,
   authentication, unknown — delegate to
   service.try_auto_revert_from_execute() which enforces
   MAX_REVERSIONS loop guard, increments reversion_count, records a
   reversion decision, and respects the delete_content automation
   threshold.  Error findings are redacted via redact_error_details()
   and stored as JSON for the strategy actor (§18329).

Error classification uses the domain model's classify_error() and
ErrorCategory enum — single source of truth, no parallel frozenset.
AuthenticationError correctly classified as non-transient per domain
model.

ISSUES CLOSED: #10843
This commit is contained in:
2026-04-24 11:55:51 +00:00
parent 43fdc31624
commit 12fc9e089f
4 changed files with 541 additions and 9 deletions
+89
View File
@@ -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
@@ -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
@@ -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,
]
+164 -4
View File
@@ -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")