fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode #10932
@@ -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
|
||||
|
||||
@@ -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
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -821,7 +821,7 @@ class PlanExecutor:
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
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)
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
return self._run_execute_with_actor(plan_id, stream_callback)
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
|
||||
def _build_decisions(self, plan: Any) -> list[StrategyDecision]:
|
||||
"""Build decisions for the Execute phase.
|
||||
@@ -1005,12 +1005,12 @@ class PlanExecutor:
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
self._lifecycle.fail_execute(plan_id, error_msg)
|
||||
raise
|
||||
|
||||
def _run_execute_with_stub(
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
def _run_execute_with_actor(
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
self,
|
||||
plan_id: str,
|
||||
stream_callback: StreamCallback | None = None,
|
||||
) -> ExecuteResult:
|
||||
"""Execute using the legacy ExecuteStubActor with optional retry."""
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"""Execute using the configured actor (stub or LLM) with optional retry."""
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
plan = self._guard_execute(plan_id)
|
||||
decisions = self._build_decisions(plan)
|
||||
|
||||
@@ -1043,11 +1043,13 @@ class PlanExecutor:
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
plan = self._lifecycle.get_plan(plan_id)
|
||||
plan.changeset_id = result.changeset_id
|
||||
plan.sandbox_refs = result.sandbox_refs
|
||||
plan.error_details = {
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
existing = dict(getattr(plan, "error_details", None) or {})
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
existing.update({
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"tool_calls_count": str(result.tool_calls_count),
|
||||
"sandbox_refs_count": str(len(result.sandbox_refs)),
|
||||
"mode": "stub",
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
}
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"mode": type(self._execute_actor).__name__,
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
})
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
plan.error_details = existing
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
plan.timestamps.updated_at = datetime.now(tz=UTC)
|
||||
|
||||
# Spawn and execute child subplans from spawn decisions
|
||||
@@ -1110,17 +1112,19 @@ class PlanExecutor:
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"on_error",
|
||||
{
|
||||
"exception_type": type(last_exc).__name__,
|
||||
"mode": "stub",
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"mode": type(self._execute_actor).__name__,
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
},
|
||||
)
|
||||
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 = {
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
existing = dict(getattr(plan, "error_details", None) or {})
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
existing.update({
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"exception_type": type(last_exc).__name__,
|
||||
"traceback": traceback.format_exc(),
|
||||
"mode": "stub",
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
}
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
"mode": type(self._execute_actor).__name__,
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
})
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
plan.error_details = existing
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
self._lifecycle._commit_plan(plan)
|
||||
self._lifecycle.fail_execute(plan_id, error_msg)
|
||||
raise last_exc
|
||||
|
||||
|
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
HAL9001
commented
Suggestion: the Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.
|
||||
Orphaned assert statement referencing undefined
resultat the end ofstep_cov2_executor_with_custom_actor. This appears to be leftover from a copied@thenscenario that was accidentally left behind. The lineassert len(result.decisions) > 0, "Expected at least one decision"will never execute and references a variable that does not exist in this scope. It must be removed.