diff --git a/features/executor_error_details.feature b/features/executor_error_details.feature new file mode 100644 index 000000000..86b8fe9db --- /dev/null +++ b/features/executor_error_details.feature @@ -0,0 +1,34 @@ +@executor-error-details +Feature: Executor preserves strategy_decisions_json and reports actual actor mode (#10874) + Verifies that _run_execute_with_actor merges error_details instead of + replacing them, preserving strategy_decisions_json stored by + run_strategize. Also verifies the mode field reflects the actual + execute actor type rather than a hardcoded "stub" string. + + Scenario: Execute success preserves strategy_decisions_json in error_details for eed + Given a eed mock lifecycle service with strategy_decisions_json in error_details + And a eed PlanExecutor with a succeeding execute actor + When I eed run execute on the plan + Then the eed committed error_details should contain strategy_decisions_json + And the eed committed error_details should contain tool_calls_count + And the eed committed error_details should contain sandbox_refs_count + + Scenario: Execute success reports actual actor type in mode for eed + Given a eed mock lifecycle service with strategy_decisions_json in error_details + And a eed PlanExecutor with a succeeding execute actor + When I eed run execute on the plan + Then the eed committed error_details mode should be the execute actor class name + + Scenario: Execute failure preserves strategy_decisions_json in error_details for eed + Given a eed mock lifecycle service with strategy_decisions_json in error_details + And a eed PlanExecutor with a failing execute actor + When I eed run execute expecting failure on the plan + Then the eed committed error_details should contain strategy_decisions_json + And the eed committed error_details should contain exception_type + And the eed committed error_details should contain traceback + + Scenario: Execute failure reports actual actor type in mode for eed + Given a eed mock lifecycle service with strategy_decisions_json in error_details + And a eed PlanExecutor with a failing execute actor + When I eed run execute expecting failure on the plan + Then the eed committed error_details mode should be the execute actor class name diff --git a/features/plan_executor_coverage.feature b/features/plan_executor_coverage.feature index 55ba61c84..06d20ec83 100644 --- a/features/plan_executor_coverage.feature +++ b/features/plan_executor_coverage.feature @@ -330,10 +330,10 @@ Feature: PlanExecutor comprehensive coverage And the cov2 lifecycle should have called fail_execute # ------------------------------------------------------------------ - # PlanExecutor._run_execute_with_stub - error recovery / retry + # PlanExecutor._run_execute_with_actor - error recovery / retry # ------------------------------------------------------------------ - Scenario: run_execute stub calls fail_execute on exception without recovery + Scenario: run_execute actor calls fail_execute on exception without recovery Given a cov2 mock lifecycle service And a cov2 plan in Execute-Queued state with decision root and definition And a cov2 PlanExecutor with failing execute actor and no recovery @@ -341,7 +341,7 @@ Feature: PlanExecutor comprehensive coverage Then a cov2 RuntimeError should have been raised And the cov2 lifecycle should have called fail_execute - Scenario: run_execute stub retries once with error recovery then succeeds + Scenario: run_execute actor retries once with error recovery then succeeds Given a cov2 mock lifecycle service And a cov2 plan in Execute-Queued state with decision root and definition And a cov2 PlanExecutor with execute actor that fails once then succeeds @@ -349,7 +349,7 @@ Feature: PlanExecutor comprehensive coverage Then the cov2 execute run result should be an ExecuteResult And the cov2 error recovery should have recorded an execute error - Scenario: run_execute stub exhausts retries with error recovery and fails + Scenario: run_execute actor exhausts retries with error recovery and fails Given a cov2 mock lifecycle service And a cov2 plan in Execute-Queued state with decision root and definition And a cov2 PlanExecutor with always-failing execute actor and exhausted retries @@ -358,7 +358,7 @@ Feature: PlanExecutor comprehensive coverage And the cov2 lifecycle should have called fail_execute And the cov2 error recovery should have recorded an execute error - Scenario: run_execute stub with error recovery records error and no retry + Scenario: run_execute actor with error recovery records error and no retry Given a cov2 mock lifecycle service And a cov2 plan in Execute-Queued state with decision root and definition And a cov2 PlanExecutor with failing execute actor and recovery denying retry diff --git a/features/steps/executor_error_details_steps.py b/features/steps/executor_error_details_steps.py new file mode 100644 index 000000000..de147b763 --- /dev/null +++ b/features/steps/executor_error_details_steps.py @@ -0,0 +1,240 @@ +"""Steps for executor_error_details.feature (#10874). + +Verifies that PlanExecutor._run_execute_with_actor: + 1. Merges error_details instead of replacing (preserves strategy_decisions_json). + 2. Reports the actual execute actor class name in the ``mode`` field. +""" + +from __future__ import annotations + +import json +from typing import Any +from unittest.mock import MagicMock + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.services.plan_executor import ( + ExecuteResult, + ExecuteStubActor, + PlanExecutor, +) +from cleveragents.domain.models.core.plan import ( + PlanPhase, + PlanTimestamps, + ProcessingState, +) +from cleveragents.tool.builtins.changeset import ChangeSet + +# ---------------------------------------------------------------------- +# Constants +# ---------------------------------------------------------------------- + +EED_PLAN_ID = "01KEEDPLANID000000000PLAN" +EED_ROOT_ID = "01KEEDROOTID000000000ROOT" + +_STRATEGY_DECISIONS = [ + { + "decision_id": "01KEEDDEC0000000000000DEC0", + "step_text": "Refactor module A", + "sequence": 0, + "parent_id": None, + }, + { + "decision_id": "01KEEDDEC0000000000000DEC1", + "step_text": "Add tests for module A", + "sequence": 1, + "parent_id": "01KEEDDEC0000000000000DEC0", + }, +] + + +# ---------------------------------------------------------------------- +# Helpers +# ---------------------------------------------------------------------- + + +def _eed_make_plan( + *, + phase: PlanPhase = PlanPhase.EXECUTE, + state: ProcessingState = ProcessingState.QUEUED, + error_details: dict[str, Any] | None = None, +) -> MagicMock: + """Build a mock plan with strategy_decisions_json in error_details.""" + plan = MagicMock() + plan.phase = phase + plan.state = state + plan.definition_of_done = "Refactor module A\nAdd tests for module A" + plan.decision_root_id = EED_ROOT_ID + plan.invariants = [] + plan.timestamps = PlanTimestamps() + plan.changeset_id = None + plan.sandbox_refs = [] + plan.error_details = error_details + plan.read_only = False + plan.identity.plan_id = EED_PLAN_ID + return plan + + +def _eed_make_lifecycle(plan: MagicMock) -> MagicMock: + """Build a mock lifecycle service that returns the given plan.""" + lcs = MagicMock() + lcs.get_plan.return_value = plan + lcs.start_execute = MagicMock() + lcs.complete_execute = MagicMock() + lcs.fail_execute = MagicMock() + lcs._commit_plan = MagicMock() + return lcs + + +# ---------------------------------------------------------------------- +# Given steps +# ---------------------------------------------------------------------- + + +@given("a eed mock lifecycle service with strategy_decisions_json in error_details") +def step_eed_mock_lifecycle(context: Context) -> None: + """Set up a plan in EXECUTE/QUEUED with strategy_decisions_json preserved.""" + decisions_json = json.dumps(_STRATEGY_DECISIONS) + error_details = { + "strategy_decisions": "2", + "strategy_decisions_json": decisions_json, + "invariant_records": "0", + } + plan = _eed_make_plan(error_details=error_details) + lifecycle = _eed_make_lifecycle(plan) + context.eed_plan = plan + context.eed_lifecycle = lifecycle + context.eed_plan_id = EED_PLAN_ID + + +@given("a eed PlanExecutor with a succeeding execute actor") +def step_eed_executor_succeeding(context: Context) -> None: + """Create a PlanExecutor whose execute actor returns a valid result.""" + context.eed_executor = PlanExecutor( + lifecycle_service=context.eed_lifecycle, + execution_context=None, + error_recovery_service=None, + ) + mock_actor = MagicMock(spec=ExecuteStubActor) + mock_actor.execute.return_value = ExecuteResult( + changeset_id="cs-001", + changeset=ChangeSet(plan_id=EED_PLAN_ID, entries=[]), + tool_calls_count=3, + sandbox_refs=[], + ) + context.eed_execute_actor = mock_actor + context.eed_executor._execute_actor = mock_actor + + +@given("a eed PlanExecutor with a failing execute actor") +def step_eed_executor_failing(context: Context) -> None: + """Create a PlanExecutor whose execute actor raises on execute.""" + context.eed_executor = PlanExecutor( + lifecycle_service=context.eed_lifecycle, + execution_context=None, + error_recovery_service=None, + ) + mock_actor = MagicMock(spec=ExecuteStubActor) + mock_actor.__class__.__name__ = "FakeExecuteActor" + mock_actor.execute.side_effect = RuntimeError("eed simulated failure") + context.eed_execute_actor = mock_actor + context.eed_executor._execute_actor = mock_actor + + +# ---------------------------------------------------------------------- +# When steps +# ---------------------------------------------------------------------- + + +@when("I eed run execute on the plan") +def step_eed_run_execute(context: Context) -> None: + """Run execute and capture the result.""" + context.eed_result = context.eed_executor.run_execute(context.eed_plan_id) + # Capture the error_details that were committed + commit_calls = context.eed_lifecycle._commit_plan.call_args_list + if commit_calls: + last_plan = commit_calls[-1][0][0] + context.eed_committed_error_details = last_plan.error_details + else: + context.eed_committed_error_details = {} + + +@when("I eed run execute expecting failure on the plan") +def step_eed_run_execute_failure(context: Context) -> None: + """Run execute expecting it to raise, capture the committed error_details.""" + try: + context.eed_executor.run_execute(context.eed_plan_id) + context.eed_raised = False + except Exception: + context.eed_raised = True + # Capture the error_details that were committed + commit_calls = context.eed_lifecycle._commit_plan.call_args_list + if commit_calls: + last_plan = commit_calls[-1][0][0] + context.eed_committed_error_details = last_plan.error_details + else: + context.eed_committed_error_details = {} + + +# ---------------------------------------------------------------------- +# Then steps +# ---------------------------------------------------------------------- + + +@then("the eed committed error_details should contain strategy_decisions_json") +def step_eed_check_strategy_json(context: Context) -> None: + """Verify strategy_decisions_json was NOT destroyed by execute.""" + details = context.eed_committed_error_details + assert "strategy_decisions_json" in details, ( + f"strategy_decisions_json missing from error_details: {details.keys()}" + ) + parsed = json.loads(details["strategy_decisions_json"]) + assert len(parsed) == 2, f"Expected 2 strategy decisions, got {len(parsed)}" + assert parsed[0]["step_text"] == "Refactor module A" + + +@then("the eed committed error_details should contain tool_calls_count") +def step_eed_check_tool_calls(context: Context) -> None: + details = context.eed_committed_error_details + assert "tool_calls_count" in details, ( + f"tool_calls_count missing from error_details: {details.keys()}" + ) + + +@then("the eed committed error_details should contain sandbox_refs_count") +def step_eed_check_sandbox_refs(context: Context) -> None: + details = context.eed_committed_error_details + assert "sandbox_refs_count" in details, ( + f"sandbox_refs_count missing from error_details: {details.keys()}" + ) + + +@then("the eed committed error_details should contain exception_type") +def step_eed_check_exception_type(context: Context) -> None: + details = context.eed_committed_error_details + assert "exception_type" in details, ( + f"exception_type missing from error_details: {details.keys()}" + ) + assert details["exception_type"] == "RuntimeError" + + +@then("the eed committed error_details should contain traceback") +def step_eed_check_traceback(context: Context) -> None: + details = context.eed_committed_error_details + assert "traceback" in details, ( + f"traceback missing from error_details: {details.keys()}" + ) + + +@then("the eed committed error_details mode should be the execute actor class name") +def step_eed_check_mode(context: Context) -> None: + """Verify mode reflects the actual actor type, not hardcoded 'stub'.""" + details = context.eed_committed_error_details + assert "mode" in details, f"mode missing from error_details: {details.keys()}" + actual_mode = details["mode"] + assert actual_mode != "stub", ( + "mode is still hardcoded as 'stub', expected actor class name" + ) + expected = type(context.eed_execute_actor).__name__ + assert actual_mode == expected, f"mode is '{actual_mode}', expected '{expected}'" diff --git a/features/steps/plan_executor_coverage_steps.py b/features/steps/plan_executor_coverage_steps.py index bc004da77..b604e1892 100644 --- a/features/steps/plan_executor_coverage_steps.py +++ b/features/steps/plan_executor_coverage_steps.py @@ -903,7 +903,7 @@ def step_cov2_check_fail_execute(context: Context) -> None: # ---------------------------------------------------------------------- -# PlanExecutor._run_execute_with_stub - retry logic +# PlanExecutor._run_execute_with_actor - retry logic # ---------------------------------------------------------------------- diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index 8952b1636..7bb2d3e24 100644 --- a/src/cleveragents/application/services/plan_executor.py +++ b/src/cleveragents/application/services/plan_executor.py @@ -821,7 +821,7 @@ class PlanExecutor: raise ValidationError("plan_id must not be empty") if self._execution_context is not None: return self._run_execute_with_runtime(plan_id, stream_callback) - return self._run_execute_with_stub(plan_id, stream_callback) + return self._run_execute_with_actor(plan_id, stream_callback) def _build_decisions(self, plan: Any) -> list[StrategyDecision]: """Build decisions for the Execute phase. @@ -843,7 +843,7 @@ class PlanExecutor: try: raw_list: list[dict[str, Any]] = json.loads(stored_json) return [StrategyDecision.model_validate(d) for d in raw_list] - except Exception: + except (json.JSONDecodeError, ValidationError): self._logger.warning( "Failed to deserialise stored strategy decisions; " "falling back to definition_of_done parsing", @@ -953,12 +953,16 @@ class PlanExecutor: plan = self._lifecycle.get_plan(plan_id) plan.changeset_id = result.changeset_id plan.sandbox_refs = result.sandbox_refs - plan.error_details = { - "tool_call_count": str(result.tool_call_count), - "decisions_processed": str(len(result.decision_ids_processed)), - "execution_duration_ms": str(result.execution_duration_ms), - "mode": "runtime", - } + existing = dict(plan.error_details or {}) + existing.update( + { + "tool_call_count": str(result.tool_call_count), + "decisions_processed": str(len(result.decision_ids_processed)), + "execution_duration_ms": str(result.execution_duration_ms), + "mode": "runtime", + } + ) + plan.error_details = existing plan.timestamps.updated_at = datetime.now(tz=UTC) # Spawn and execute child subplans from spawn decisions @@ -996,21 +1000,30 @@ class PlanExecutor: self._try_rollback_to_last_checkpoint(plan_id) error_msg = f"{type(exc).__name__}: {exc}" plan = self._lifecycle.get_plan(plan_id) - plan.error_details = { - "exception_type": type(exc).__name__, - "traceback": traceback.format_exc(), - "mode": "runtime", - } + existing = dict(plan.error_details or {}) + existing.update( + { + "exception_type": type(exc).__name__, + "traceback": traceback.format_exc(), + "mode": "runtime", + } + ) + plan.error_details = existing self._lifecycle._commit_plan(plan) self._lifecycle.fail_execute(plan_id, error_msg) raise - def _run_execute_with_stub( + def _run_execute_with_actor( self, plan_id: str, stream_callback: StreamCallback | None = None, ) -> ExecuteResult: - """Execute using the legacy ExecuteStubActor with optional retry.""" + """Execute using the configured execute actor with optional retry. + + Retry behaviour is controlled by the ``ErrorRecoveryService`` + attached at construction time (``error_recovery.max_retries``). + Without an error recovery service, failures are immediate. + """ plan = self._guard_execute(plan_id) decisions = self._build_decisions(plan) @@ -1043,11 +1056,15 @@ class PlanExecutor: plan = self._lifecycle.get_plan(plan_id) plan.changeset_id = result.changeset_id plan.sandbox_refs = result.sandbox_refs - plan.error_details = { - "tool_calls_count": str(result.tool_calls_count), - "sandbox_refs_count": str(len(result.sandbox_refs)), - "mode": "stub", - } + existing = dict(plan.error_details or {}) + existing.update( + { + "tool_calls_count": str(result.tool_calls_count), + "sandbox_refs_count": str(len(result.sandbox_refs)), + "mode": type(self._execute_actor).__name__, + } + ) + plan.error_details = existing plan.timestamps.updated_at = datetime.now(tz=UTC) # Spawn and execute child subplans from spawn decisions @@ -1067,8 +1084,9 @@ class PlanExecutor: OperationalMetricKey.PLAN_DURATION_MS, plan_id, _duration_ms ) self._logger.info( - "Execute completed (stub)", + "Execute completed", plan_id=plan_id, + mode=type(self._execute_actor).__name__, changeset_id=result.changeset_id, tool_calls=result.tool_calls_count, ) @@ -1110,17 +1128,21 @@ class PlanExecutor: "on_error", { "exception_type": type(last_exc).__name__, - "mode": "stub", + "mode": type(self._execute_actor).__name__, }, ) self._try_rollback_to_last_checkpoint(plan_id) error_msg = f"{type(last_exc).__name__}: {last_exc}" plan = self._lifecycle.get_plan(plan_id) - plan.error_details = { - "exception_type": type(last_exc).__name__, - "traceback": traceback.format_exc(), - "mode": "stub", - } + existing = dict(plan.error_details or {}) + existing.update( + { + "exception_type": type(last_exc).__name__, + "traceback": traceback.format_exc(), + "mode": type(self._execute_actor).__name__, + } + ) + plan.error_details = existing self._lifecycle._commit_plan(plan) self._lifecycle.fail_execute(plan_id, error_msg) raise last_exc