From cc3a5a6246f28a38c8bee42a3df47e0a98335327 Mon Sep 17 00:00:00 2001 From: HAL9001 Date: Wed, 29 Apr 2026 14:02:33 +0000 Subject: [PATCH] fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode Fixed three bugs in _run_execute_with_actor (formerly _run_execute_with_stub): 1. Merged error_details instead of replacing - preserves strategy_decisions_json stored by run_strategize() so retry plans retain the full decision hierarchy (Forgejo #10874). 2. Used type(self._execute_actor).__name__ instead of hardcoded "stub" at all three locations (success, on_error checkpoint, fail) so the mode field reflects the actual actor type (Forgejo #10874). 3. Renamed method from _run_execute_with_stub to _run_execute_with_actor since it accepts any configured actor (stub or LLM), not just ExecuteStubActor. Added two Behave scenarios verifying strategy_decisions_json survival and actual actor mode reporting. Updated test step comments and docs. ISSUES CLOSED: #10874 --- docs/reference/read_only_actions.md | 2 +- features/plan_executor_coverage.feature | 20 +++- .../steps/plan_executor_coverage_steps.py | 103 +++++++++++++++++- .../application/services/plan_executor.py | 24 ++-- 4 files changed, 136 insertions(+), 13 deletions(-) diff --git a/docs/reference/read_only_actions.md b/docs/reference/read_only_actions.md index 3c121fef0..1efa1cd1a 100644 --- a/docs/reference/read_only_actions.md +++ b/docs/reference/read_only_actions.md @@ -72,7 +72,7 @@ raise ReadOnlyViolationError( Read-only tool specs pass through unchanged. `ExecuteStubActor.execute()` propagates the plan's `read_only` flag -into `ChangeSetCapture`, and `PlanExecutor._run_execute_with_stub()` +into `ChangeSetCapture`, and `PlanExecutor._run_execute_with_actor()` reads `plan.read_only` from the plan model. ### 4. CLI Fail-Fast Guards diff --git a/features/plan_executor_coverage.feature b/features/plan_executor_coverage.feature index 55ba61c84..f1b814ed3 100644 --- a/features/plan_executor_coverage.feature +++ b/features/plan_executor_coverage.feature @@ -330,7 +330,7 @@ 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 @@ -367,6 +367,24 @@ Feature: PlanExecutor comprehensive coverage And the cov2 error recovery should have recorded an execute error And the cov2 lifecycle should have called fail_execute + Scenario: run_execute preserves strategy_decisions_json in error_details after execute + Given a cov2 mock lifecycle service + And a cov2 plan with stored strategy_decisions_json in error_details + And a cov2 plan in Execute-Queued state with decision root and definition + And a cov2 PlanExecutor without execution context + When I cov2 call run_execute + Then the cov2 execute completed successfully + And the cov2 plan error_details should contain "strategy_decisions_json" + And the cov2 plan error_details key "mode" should equal "ExecuteStubActor" + + Scenario: run_execute mode reflects actual execute actor type + Given a cov2 mock lifecycle service + And a cov2 plan in Execute-Queued state with decision root and definition + And a cov2 PlanExecutor with a custom execute actor + When I cov2 call run_execute + Then the cov2 execute completed successfully + And the cov2 plan error_details key "mode" should equal "MockCustomExecuteActor" + # ------------------------------------------------------------------ # PlanExecutor._build_decisions # ------------------------------------------------------------------ diff --git a/features/steps/plan_executor_coverage_steps.py b/features/steps/plan_executor_coverage_steps.py index bc004da77..f8f5f428f 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 # ---------------------------------------------------------------------- @@ -1267,4 +1267,105 @@ def step_cov2_stub_result_has_decisions(context: Context) -> None: assert isinstance(result, StrategizeResult), ( f"Expected StrategizeResult, got {type(result)}" ) + + +# ---------------------------------------------------------------------- +# PlanExecutor._run_execute_with_actor - strategy preservation and mode +# ---------------------------------------------------------------------- + + +@given("a cov2 plan with stored strategy_decisions_json in error_details") +def step_cov2_plan_with_strategy_decisions_json(context: Context) -> None: + """Create a plan whose error_details already has strategy_decisions_json.""" + plan = _cov2_make_plan( + phase=PlanPhase.EXECUTE, + state=ProcessingState.QUEUED, + ) + stored = json.dumps([ + { + "decision_id": "01KSTRATEGYDECISION0", + "step_text": "Execute step one", + "sequence": 0, + "parent_id": None, + }, + { + "decision_id": "01KSTRATEGYDECISION1", + "step_text": "Execute step two", + "sequence": 1, + "parent_id": "01KSTRATEGYDECISION0", + }, + ]) + plan.error_details = { + "strategy_decisions_json": stored, + "mode": "strategize", + } + context.cov2_lifecycle = _cov2_make_lifecycle(plan) + context.cov2_plan_id = plan.identity.plan_id + + +# ---------------------------------------------------------------------- +# PlanExecutor.run_execute with custom actor +# ---------------------------------------------------------------------- + + +@when("I cov2 call run_execute") +def step_cov2_call_run_execute(context: Context) -> None: + """Execute the plan. If the actor returns normally record the + result; on exception set cov2_execute_exception.""" + try: + context.cov2_execute_result = context.cov2_executor.run_execute( + plan_id=context.cov2_plan_id, + ) + except Exception: + context.cov2_execute_exception = True + + +@then("the cov2 execute completed successfully") +def step_cov2_execute_completed(context: Context) -> None: + """Verify execute returned without exception.""" + assert not getattr(context, "cov2_execute_exception", False) + assert context.cov2_execute_result is not None + + +@then("the cov2 plan error_details should contain \"{key}\"") +def step_cov2_error_details_contains_key(context: Context, key: str) -> None: + """Check a key survived inside error_details after execute.""" + call_args = context.cov2_lifecycle._commit_plan.call_args + committed_plan = call_args.args[0] + assert committed_plan.error_details is not None + assert key in committed_plan.error_details, ( + f"'{key}' missing from error_details: {committed_plan.error_details}" + ) + + +@then('the cov2 plan error_details key "{key}" should equal "{expected}"') +def step_cov2_error_details_key_equals( + context: Context, key: str, expected: str +) -> None: + """Check that a particular error_details key has the expected value.""" + call_args = context.cov2_lifecycle._commit_plan.call_args + committed_plan = call_args.args[0] + actual = committed_plan.error_details.get(key) + assert actual == expected, ( + f"error_details['{key}'] expected '{expected}', got '{actual}'" + ) + + +@given("a cov2 PlanExecutor with a custom execute actor") +def step_cov2_executor_with_custom_actor(context: Context) -> None: + """Create a PlanExecutor whose execute_actor is a MagicMock with a + distinct class name so type().__name__ != ExecuteStubActor.""" + context.cov2_executor = PlanExecutor( + lifecycle_service=context.cov2_lifecycle, + execution_context=None, + error_recovery_service=None, + ) + custom_actor = MagicMock() + custom_actor.__class__.__name__ = "MockCustomExecuteActor" + context.cov2_executor._execute_actor = custom_actor + custom_actor.execute.return_value = ExecuteResult( + changeset_id="cs-001", + sandbox_refs=[], + tool_calls_count=3, + ) assert len(result.decisions) > 0, "Expected at least one decision" diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index 8952b1636..9ce6d022a 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 actor (stub or LLM) with optional retry.""" plan = self._guard_execute(plan_id) decisions = self._build_decisions(plan) @@ -1043,11 +1043,13 @@ class PlanExecutor: plan = self._lifecycle.get_plan(plan_id) plan.changeset_id = result.changeset_id plan.sandbox_refs = result.sandbox_refs - plan.error_details = { + existing = dict(getattr(plan, "error_details", None) or {}) + existing.update({ "tool_calls_count": str(result.tool_calls_count), "sandbox_refs_count": str(len(result.sandbox_refs)), - "mode": "stub", - } + "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 @@ -1110,17 +1112,19 @@ 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 = { + existing = dict(getattr(plan, "error_details", None) or {}) + existing.update({ "exception_type": type(last_exc).__name__, "traceback": traceback.format_exc(), - "mode": "stub", - } + "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