From 9e3d7c662292835120c1c20bb4ca12a6002363d0 Mon Sep 17 00:00:00 2001 From: HAL9001 Date: Thu, 30 Apr 2026 10:40:03 +0000 Subject: [PATCH 1/3] fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode Merge error_details instead of replacing them in _run_execute_with_actor (formerly _run_execute_with_stub), preserving strategy_decisions_json stored by run_strategize. On execute retry, _build_decisions now finds the full decision hierarchy instead of falling back to definition_of_done parsing. - Merge error_details on both success and failure paths - Report type(self._execute_actor).__name__ instead of hardcoded 'stub' - Rename _run_execute_with_stub to _run_execute_with_actor - Add 4 Behave scenarios verifying preservation and mode reporting ISSUES CLOSED: #10874 --- features/executor_error_details.feature | 34 +++ .../steps/executor_error_details_steps.py | 240 ++++++++++++++++++ .../steps/plan_executor_coverage_steps.py | 2 +- .../application/services/plan_executor.py | 39 +-- 4 files changed, 299 insertions(+), 16 deletions(-) create mode 100644 features/executor_error_details.feature create mode 100644 features/steps/executor_error_details_steps.py 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/steps/executor_error_details_steps.py b/features/steps/executor_error_details_steps.py new file mode 100644 index 000000000..f160244e8 --- /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() + 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..8ba2d65ba 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. @@ -1005,12 +1005,12 @@ class PlanExecutor: 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.""" plan = self._guard_execute(plan_id) decisions = self._build_decisions(plan) @@ -1043,11 +1043,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 +1071,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 +1115,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 -- 2.52.0 From a740d9c15ad72f2022431f50cca18071673c5bef Mon Sep 17 00:00:00 2001 From: Hamza Khyari Date: Thu, 30 Apr 2026 13:38:57 +0000 Subject: [PATCH 2/3] fix: address PR review findings - narrow exception scope and fix test mocks Address CoreRasurae's review comments: 1. Narrow exception scope in _build_decisions (line 846) to catch only json.JSONDecodeError and ValidationError instead of bare Exception. This prevents swallowing unintended errors like memory issues. 2. Update feature file section header and scenario titles from _run_execute_with_stub to _run_execute_with_actor for consistency. 3. Add spec to MagicMock in failing execute scenario (line 138) to match the pattern used in succeeding scenario (line 119). Note: Master feature entry point - Behave auto-discovers all .feature files, no explicit entry point needed. Tests run successfully. ISSUES CLOSED: #10874 --- features/plan_executor_coverage.feature | 10 +++++----- features/steps/executor_error_details_steps.py | 2 +- src/cleveragents/application/services/plan_executor.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) 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 index f160244e8..de147b763 100644 --- a/features/steps/executor_error_details_steps.py +++ b/features/steps/executor_error_details_steps.py @@ -135,7 +135,7 @@ def step_eed_executor_failing(context: Context) -> None: execution_context=None, error_recovery_service=None, ) - mock_actor = MagicMock() + 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 diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index 8ba2d65ba..5e01795d3 100644 --- a/src/cleveragents/application/services/plan_executor.py +++ b/src/cleveragents/application/services/plan_executor.py @@ -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", -- 2.52.0 From 86e1cdf6ae70affdf78fd03942c31c306847ab01 Mon Sep 17 00:00:00 2001 From: HAL9001 Date: Thu, 30 Apr 2026 13:53:13 +0000 Subject: [PATCH 3/3] fix(plan): apply merge pattern to _run_execute_with_runtime and improve docstring - Apply error_details merge pattern to _run_execute_with_runtime for consistency with _run_execute_with_actor (preserves strategy_decisions_json in runtime mode too) - Expand _run_execute_with_actor docstring to clarify retry is controlled by ErrorRecoveryService.max_retries Addresses remaining review findings from PR #10945. --- .../application/services/plan_executor.py | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index 5e01795d3..7bb2d3e24 100644 --- a/src/cleveragents/application/services/plan_executor.py +++ b/src/cleveragents/application/services/plan_executor.py @@ -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,11 +1000,15 @@ 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 @@ -1010,7 +1018,12 @@ class PlanExecutor: plan_id: str, stream_callback: StreamCallback | None = None, ) -> ExecuteResult: - """Execute using the configured execute actor 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) -- 2.52.0