fix(plan-executor): persist strategy decisions during Strategize phase

PlanExecutor.run_strategize() produces StrategyDecision objects but was only
storing them as JSON in plan.error_details["strategy_decisions_json"]. Downstream
CLI commands like "plan tree" and "plan correct" query via DecisionService which
finds an empty tree because decisions are never inserted.

Wire StrategizeDecisionHook into the production execution path so every strategy
decision is persistently recorded during the Strategize phase, while maintaining
the JSON fallback for backward compatibility with _build_decisions.

Co-authored-by: <auto-generated>

ISSUES CLOSED: #10813

CHANGELOG.md

@@ -4,6 +4,10 @@
 
 ## Unreleased
 
+### Fixed
+
+- **Strategize phase now persists decisions to database fixing ``plan tree`` returning empty** (#10813): The root cause was that ``PlanExecutor.run_strategize()`` produced ``StrategyDecision`` objects but never persisted them as proper domain ``Decision`` records in the database — they were only serialised as JSON into ``plan.error_details["strategy_decisions_json"]``. This meant CLI commands like ``plan tree`` and ``plan correct`` which query via ``DecisionService.list_decisions()`` would find an empty tree. Fixed by wiring ``StrategizeDecisionHook`` into the production execution path so every strategy decision is persistently recorded during the Strategize phase, while maintaining the JSON fallback for backward compatibility with any code that reads ``error_details["strategy_decisions_json"]``.
+
 - Hardened the TDD bug-fix quality gate for issue #629: PR parsing now
   requires whole-word closing keywords (avoids false positives like
   "prefixes #12"), TDD bug tag discovery now uses exact token matching

CONTRIBUTORS.md

@@ -40,3 +40,4 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed database resource types (PostgreSQL, SQLite) with transaction-based sandbox strategy: implemented ``DatabaseResourceHandler`` providing full CRUD operations (`read`, `write`, `delete`, `list_children`) and connection validation with automatic credential masking for PostgreSQL and SQLite backends. Includes ``TransactionSandbox`` infrastructure wired into ``SandboxFactory``, BDD test coverage in ``features/database_resources.feature``, and Robot Framework integration tests in ``robot/database_resources.robot`` (PR #10591 / issue #8608, Epic #8568).
 * HAL 9000 has contributed the agents plan rollback command (PR #8674 / issue #8557): implemented checkpoint-based plan state restoration with the `agents plan rollback <plan-id> [<checkpoint-id>]` CLI command as part of Epic #8493, enabling plans to be restored to previous checkpoints, discarding post-checkpoint decisions, and resuming execution from the rolled-back state. Supported by `--yes/-y`, `--to-checkpoint`, and `--format/-f` flags. Includes comprehensive BDD test coverage (>= 97%) for rollback, decision discarding, and plan resume functionality.
 * HAL 9000 has contributed the PyYAML security upgrade (PR #11012 / issue #9055): added `pyyaml>=6.0.3` dependency constraint to address known YAML parsing vulnerabilities.
+* HAL 9000 has contributed the plan tree decision persistence fix (issue #10813): wired `StrategizeDecisionHook` into `PlanExecutor.run_strategize()` so strategy decisions are persisted to the database as proper domain `Decision` records during the Strategize phase instead of only being stored as JSON in `plan.error_details`. Fixed `plan tree` and `plan correct` CLI commands which previously found an empty decision tree because no decisions were ever written via `DecisionService.record_decision()`. Includes BDD test coverage for positive path, graceful degradation on hook failure, and backward compatibility of the JSON fallback.

features/plan_executor_decision_persistence.feature

@@ -0,0 +1,73 @@
+Feature: PlanExecutor decision persistence during Strategize (Issue #10813)
+  As an automated implementation worker
+  I want strategy decisions to be persisted to the database during the Strategize phase
+  So that downstream CLI commands like ``plan tree`` and ``plan correct`` can query them
+
+  Background:
+    Given a dp mock lifecycle service
+    And a dp in-memory decision service
+    And a dp plan in Strategize-Queued state with definition "Build feature\nAdd tests"
+
+  # ------------------------------------------------------------------
+  # Decision hook wiring - positive path
+  # ------------------------------------------------------------------
+
+  Scenario: run_strategize persists decisions via decision_hook (Issue #10813)
+    Given the dp plan executor has a decision hook for the plan
+    When I dp call run_strategize with decision hook
+    Then all dp strategize decisions should be persisted to the decision service
+    And the dp decision service tree for the plan should have {decision_count} nodes
+    And the dp plan JSON decision storage should remain populated (backward compat)
+
+  Scenario: run_strategize persists empty decision list via decision_hook
+    Given a dp mock lifecycle service
+    And a dp in-memory decision service
+    And a dp plan with no steps ("") in Strategize-Queued state
+    And the dp plan executor has a decision hook for the plan
+    When I dp call run_strategize with decision hook
+    Then all dp strategize decisions should be persisted to the decision service
+
+  # ------------------------------------------------------------------
+  # Decision hook wiring - graceful degradation
+  # ------------------------------------------------------------------
+
+  Scenario: run_strategize without decision_hook still succeeds (backward compat)
+    Given a dp mock lifecycle service
+    And a dp plan in Strategize-Queued state with definition "Do work"
+    And the dp plan executor has NO decision hook
+    When I dp call run_strategize WITHOUT decision hook
+    Then the dp strategize run result should not raise an error
+    And the dp JSON fallback in error_details should be populated
+
+  Scenario: run_strategize with decision_hook that fails gracefully
+    Given a dp mock lifecycle service
+    And a dp failing decision service
+    And a dp plan in Strategize-Queued state with definition "Do work"
+    And the dp plan executor has a failing decision hook for the plan
+    When I dp call run_strategize with failing decision hook
+    Then the dp strategize should not raise an error (graceful degradation)
+    And the dp JSON fallback in error_details should still be populated
+
+  # ------------------------------------------------------------------
+  # Decision tree structure verification
+  # ------------------------------------------------------------------
+
+  Scenario: persisted decisions form a valid tree with correct parent-child relationships
+    Given the dp mock lifecycle service
+    And a dp in-memory decision service
+    And the dp plan executor has a decision hook for the plan
+    When I dp call run_strategize with decision hook
+    Then the dp first persisted decision should be a root (no parent)
+    And the dp non-root persisted decisions should reference the root as parent
+
+  # ------------------------------------------------------------------
+  # Database queryability post-strategize
+  # ------------------------------------------------------------------
+
+  Scenario: DecisionService.list_decisions returns decisions after run_strategize with hooked executor
+    Given a dp mock lifecycle service
+    And a dp in-memory decision service
+    And a dp plan in Strategize-Queued state with definition "Build feature\nAdd tests"
+    And the dp plan executor has a decision hook for the plan
+    When I dp call run_strategize with decision hook
+    Then listing dp decisions for the plan should return {num_steps} results

features/steps/plan_executor_decision_persistence_steps.py

@@ -0,0 +1,343 @@
+"""Step definitions for plan_executor_decision_persistence.feature.
+
+Tests that StrategizeDecisionHook is wired into PlanExecutor.run_strategize()
+so decisions are persisted to the database, fixing Issue #10813.
+
+All step texts use the ``dp`` prefix (decision persistence) to avoid collisions.
+"""
+
+from __future__ import annotations
+
+import json
+from unittest.mock import MagicMock, patch
+
+from behave import given, then, when
+from behave.runner import Context
+
+from cleveragents.application.services.decision_service import DecisionService
+from cleveragents.application.services.plan_executor import (
+    PlanExecutor,
+    StrategizeResult,
+    StrategyDecision,
+)
+from cleveragents.application.services.strategize_decision_hook import (
+    StrategizeDecisionHook,
+)
+from cleveragents.core.exceptions import PlanError, ValidationError
+from cleveragents.domain.models.core.decision import DecisionType
+from cleveragents.domain.models.core.plan import (
+    PlanInvariant,
+    PlanPhase,
+    PlanTimestamps,
+    ProcessingState,
+)
+
+# ----------------------------------------------------------------------
+# Constants
+# ----------------------------------------------------------------------
+
+DP_PLAN_ID = "01KDPPERSIST000000000PERSIST"
+
+
+# ----------------------------------------------------------------------
+# Helpers
+# ----------------------------------------------------------------------
+
+
+def _dp_make_plan(
+    *,
+    phase: PlanPhase = PlanPhase.STRATEGIZE,
+    state: ProcessingState = ProcessingState.QUEUED,
+    definition_of_done: str | None = "Build feature\nAdd tests",
+    decision_root_id: str | None = None,
+    invariants: list[PlanInvariant] | None = None,
+) -> MagicMock:
+    """Build a mock plan object."""
+    plan = MagicMock()
+    plan.phase = phase
+    plan.state = state
+    plan.definition_of_done = definition_of_done
+    plan.decision_root_id = decision_root_id or f"01KDPROOTID00000000000ROOT"
+    plan.invariants = invariants or []
+    plan.timestamps = PlanTimestamps()
+    plan.changeset_id = None
+    plan.sandbox_refs = []
+    plan.error_details = None
+    plan.project_links = None
+    return plan
+
+
+def _dp_make_decisions(count: int = 2) -> list[StrategyDecision]:
+    """Build a list of StrategyDecision instances for the test."""
+    root_id = f"01KDPROOTID00000000000{DP_PLAN_ID[:4]}"
+    decisions: list[StrategyDecision] = []
+    for i in range(count):
+        did = root_id if i == 0 else f"01KDPPERSIST{i:020d}"
+        decisions.append(
+            StrategyDecision(
+                decision_id=did,
+                step_text=f"Step {i + 1}",
+                sequence=i,
+                parent_id=root_id if i > 0 else None,
+            )
+        )
+    return decisions
+
+
+# ----------------------------------------------------------------------
+# Background steps
+# ----------------------------------------------------------------------
+
+
+@given("a dp mock lifecycle service")
+def step_given_dp_mock_lifecycle(context: Context) -> None:
+    """Create a mock lifecycle service and attach to context."""
+    plan = _dp_make_plan()
+    lcs = MagicMock()
+    lcs.get_plan.return_value = plan
+    lcs.start_strategize = MagicMock()
+    lcs.complete_strategize = MagicMock()
+    lcs.fail_strategize = MagicMock()
+    lcs._commit_plan = MagicMock()
+    context.dp_lifecycle = lcs
+    context.dp_plan = plan
+
+
+@given("a dp in-memory decision service")
+def step_given_dp_memory_decision_service(context: Context) -> None:
+    """Create an in-memory DecisionService."""
+    context.dp_decision_svc = DecisionService()
+
+
+@given('the dp plan executor has a decision hook for the plan')
+def step_given_dp_hook_for_plan(context: Context) -> None:
+    """Wire a StrategizeDecisionHook into the PlanExecutor."""
+    assert context.dp_lifecycle is not None
+    assert context.dp_decision_svc is not None
+    context.dp_hook = StrategizeDecisionHook(
+        decision_service=context.dp_decision_svc,
+        plan_id=DP_PLAN_ID,
+    )
+    context.dp_plan_executor = PlanExecutor(
+        lifecycle_service=context.dp_lifecycle,
+        decision_hook=context.dp_hook,
+    )
+
+
+@given('the dp plan executor has NO decision hook')
+def step_given_dp_no_hook(context: Context) -> None:
+    """Construct a PlanExecutor without a decision hook."""
+    assert context.dp_lifecycle is not None
+    context.dp_plan_executor = PlanExecutor(
+        lifecycle_service=context.dp_lifecycle,
+    )
+
+
+@given("the dp plan executor has a failing decision hook for the plan")
+def step_given_dp_failing_hook(context: Context) -> None:
+    """Create a decision service that fails on record_decision."""
+    assert context.dp_lifecycle is not None
+
+    def _failing_record(*args, **kwargs):
+        raise RuntimeError("Simulated persistence failure")
+
+    class FailingDecisionSvc(DecisionService):
+        def record_decision(self, *args, **kwargs):
+            return _failing_record(*args, **kwargs)
+
+    failing_svc = FailingDecisionSvc()
+    context.dp_hook = StrategizeDecisionHook(
+        decision_service=failing_svc,
+        plan_id=DP_PLAN_ID,
+    )
+    # Monkey-patch so the failing service raises during record_decision
+    context.dp_hook.decision_service.record_decision = _failing_record
+
+    context.dp_plan_executor = PlanExecutor(
+        lifecycle_service=context.dp_lifecycle,
+        decision_hook=context.dp_hook,
+    )
+
+
+# ----------------------------------------------------------------------
+# When steps
+# ----------------------------------------------------------------------
+
+
+@when("I dp call run_strategize with decision hook")
+def step_when_dp_run_strategize_with_hook(context: Context) -> None:
+    """Run strategize with the hooked executor.
+
+    Patch get_plan to return a plan already in Strategize-Queued phase,
+    and inject decisions so we can verify persistence.
+    """
+    # Make sure the plan is in the right phase/state
+    context.dp_plan.phase = PlanPhase.STRATEGIZE
+    context.dp_plan.state = ProcessingState.QUEUED
+
+    # Build test decisions matching the definition_of_done (2 steps)
+    test_decisions = _dp_make_decisions(2)
+
+    # Patch StrategizeActor.execute to return our controlled decisions
+    with patch.object(
+        context.dp_plan_executor._strategize_actor, "execute",
+        return_value=StrategizeResult(
+            decision_root_id=test_decisions[0].decision_id,
+            decisions=test_decisions,
+        ),
+    ):
+        result = context.dp_plan_executor.run_strategize(DP_PLAN_ID)
+
+    context.dp_strategize_result = result
+
+
+@when("I dp call run_strategize WITHOUT decision hook")
+def step_when_dp_run_strategize_without_hook(context: Context) -> None:
+    """Run strategize without a decision hook (backward compat path)."""
+    # Make sure the plan is in the right phase/state
+    context.dp_plan.phase = PlanPhase.STRATEGIZE
+    context.dp_plan.state = ProcessingState.QUEUED
+
+    test_decisions = _dp_make_decisions(2)
+
+    with patch.object(
+        context.dp_plan_executor._strategize_actor, "execute",
+        return_value=StrategizeResult(
+            decision_root_id=test_decisions[0].decision_id,
+            decisions=test_decisions,
+        ),
+    ):
+        result = context.dp_plan_executor.run_strategize(DP_PLAN_ID)
+
+    context.dp_strategize_result = result
+
+
+@when("I dp call run_strategize with failing decision hook")
+def step_when_dp_run_strategize_failing_hook(context: Context) -> None:
+    """Run strategize when the decision hook's record_decision fails."""
+    # Make sure the plan is in the right phase/state
+    context.dp_plan.phase = PlanPhase.STRATEGIZE
+    context.dp_plan.state = ProcessingState.QUEUED
+
+    test_decisions = _dp_make_decisions(2)
+
+    with patch.object(
+        context.dp_plan_executor._strategize_actor, "execute",
+        return_value=StrategizeResult(
+            decision_root_id=test_decisions[0].decision_id,
+            decisions=test_decisions,
+        ),
+    ):
+        result = context.dp_plan_executor.run_strategize(DP_PLAN_ID)
+
+    context.dp_strategize_result = result
+
+
+# ----------------------------------------------------------------------
+# Assertion steps
+# ----------------------------------------------------------------------
+
+
+@then("all dp strategize decisions should be persisted to the decision service")
+def step_then_dp_decisions_persisted(context: Context) -> None:
+    """Verify all decisions from the StrategizeResult were persisted."""
+    assert context.dp_strategize_result is not None
+    decisions = context.dp_decision_svc.list_decisions(DP_PLAN_ID)
+    assert len(decisions) == len(context.dp_strategize_result.decisions), (
+        f"Expected {len(context.dp_strategize_result.decisions)} persisted decisions, "
+        f"got {len(decisions)}"
+    )
+
+
+@then("the dp decision service tree for the plan should have {decision_count} nodes")
+def step_then_dp_tree_has_nodes(context: Context, decision_count: str) -> None:
+    """Verify the BFS tree has the expected number of nodes."""
+    tree = context.dp_decision_svc.get_tree(DP_PLAN_ID)
+    assert len(tree) == int(decision_count), (
+        f"Expected {decision_count} tree nodes, got {len(tree)}"
+    )
+
+
+@then("the dp plan JSON decision storage should remain populated (backward compat)")
+def step_then_dp_json_still_populated(context: Context) -> None:
+    """Verify the backward-compatible JSON storage was also written."""
+    plan = context.dp_lifecycle.get_plan(DP_PLAN_ID)
+    assert plan.error_details is not None, "error_details should be set"
+    sd = plan.error_details.get("strategy_decisions")
+    assert sd == str(len(context.dp_strategize_result.decisions)), (
+        "Strategy decisions count should match in JSON storage"
+    )
+
+
+@then("the dp strategize run result should not raise an error")
+def step_then_dp_no_error(context: Context) -> None:
+    """Verify the operation succeeded."""
+    assert context.dp_strategize_result is not None
+
+
+@then("the dp JSON fallback in error_details should be populated")
+def step_then_dp_json_fallback_populated(context: Context) -> None:
+    """Verify error_details contains strategy_decisions_json even when hook fails."""
+    plan = context.dp_lifecycle.get_plan(DP_PLAN_ID)
+    assert plan.error_details is not None, "error_details should be set"
+    json_data = plan.error_details.get("strategy_decisions_json")
+    assert json_data is not None and len(json_data) > 0, (
+        "JSON fallback should be populated for backward compatibility"
+    )
+
+
+@then("the dp strategize should not raise an error (graceful degradation)")
+def step_then_dp_graceful_degradation(context: Context) -> None:
+    """Verify the operation succeeded despite failing decision hook."""
+    assert context.dp_strategize_result is not None
+
+
+@then("the dp first persisted decision should be a root (no parent)")
+def step_then_dp_first_is_root(context: Context) -> None:
+    """Verify the first persisted decision has no parent (is the tree root)."""
+    decisions = sorted(
+        context.dp_decision_svc.list_decisions(DP_PLAN_ID),
+        key=lambda d: d.sequence_number,
+    )
+    assert len(decisions) > 0, "Should have at least one decision"
+    first = decisions[0]
+    assert first.parent_decision_id is None, (
+        f"First decision should be root, but has parent={first.parent_decision_id!r}"
+    )
+
+
+@then("the dp non-root persisted decisions should reference the root as parent")
+def step_then_dp_non_root_has_parent(context: Context) -> None:
+    """Verify non-root decisions have the first decision as their parent."""
+    decisions = sorted(
+        context.dp_decision_svc.list_decisions(DP_PLAN_ID),
+        key=lambda d: d.sequence_number,
+    )
+    root_id = decisions[0].decision_id if decisions else None
+    for d in decisions[1:]:
+        assert d.parent_decision_id == root_id, (
+            f"Decision {d.decision_id} parent={d.parent_decision_id!r} != root={root_id!r}"
+        )
+
+
+@then("listing dp decisions for the plan should return {num_steps} results")
+def step_then_dp_list_returns_count(context: Context, num_steps: str) -> None:
+    """Verify list_decisions returns the expected number of results."""
+    all_decisions = context.dp_decision_svc.list_decisions(DP_PLAN_ID)
+    assert len(all_decisions) == int(num_steps), (
+        f"Expected {num_steps} decisions, got {len(all_decisions)}"
+    )
+
+
+@given("a dp failing decision service")
+def step_given_dp_failing_service(context: Context) -> None:
+    """Create a decision service that fails on record_decision."""
+
+    def _failing_record(plan_id, decision_type, question, chosen_option, **kwargs):
+        raise RuntimeError("Simulated persistence failure")
+
+    class FailingDecisionSvc(DecisionService):
+        def record_decision(self, *args, **kwargs):
+            return _failing_record(*args, **kwargs)
+
+    context.dp_failing_svc = FailingDecisionSvc()

src/cleveragents/application/services/plan_executor.py

@@ -11,6 +11,11 @@ into the Execute phase so that ``subplan_spawn`` and
 ``subplan_parallel_spawn`` decisions are realised as actual child plan
 executions.
 Updated in M6 to wire StrategyActor decisions through to Execute phase.
+Updated in M7 to wire ``StrategizeDecisionHook`` into the Strategize phase
+so that strategy decisions are persisted to the database (via
+``DecisionService``) during ``run_strategize()``.  This enables downstream
+CLI commands like ``plan tree`` and ``plan correct`` to query decisions from
+the database instead of finding an empty tree. (Issue #10813)
 """
 
 from __future__ import annotations
@@ -62,6 +67,9 @@ if TYPE_CHECKING:
     from cleveragents.application.services.error_recovery_service import (
         ErrorRecoveryService,
     )
+    from cleveragents.application.services.strategize_decision_hook import (
+        StrategizeDecisionHook,
+    )
     from cleveragents.application.services.subplan_execution_service import (
         SubplanExecutionResult,
         SubplanExecutionService,
@@ -326,6 +334,7 @@ class PlanExecutor:
         fix_revalidate_orchestrator: FixThenRevalidateOrchestrator | None = None,
         subplan_service: SubplanService | None = None,
         subplan_execution_service: SubplanExecutionService | None = None,
+        decision_hook: StrategizeDecisionHook | None = None,
     ) -> None:
         """Initialize the plan executor.
 
@@ -359,6 +368,11 @@ class PlanExecutor:
             subplan_execution_service: Optional service for executing
                 spawned child plans.  When ``None``, child plan
                 execution is skipped even if subplans were spawned.
+            decision_hook: Optional ``StrategizeDecisionHook`` for
+                persisting strategy decisions to the database during
+                the Strategize phase.  When ``None``, decisions are only
+                stored as JSON in ``plan.error_details`` and will not be
+                available via ``DecisionService.list_decisions()``.
         """
         if lifecycle_service is None:
             raise ValidationError("lifecycle_service must not be None")
@@ -375,6 +389,7 @@ class PlanExecutor:
         self._subplan_execution_service = subplan_execution_service
         self._strategize_actor = strategize_actor or StrategizeStubActor()
         self._execute_actor = execute_actor or ExecuteStubActor()
+        self._decision_hook = decision_hook
         self._logger = logger.bind(service="plan_executor")
 
     def _try_emit_metric(
@@ -774,6 +789,43 @@ class PlanExecutor:
                 "strategy_decisions_json": decisions_json,
                 "invariant_records": str(len(result.invariant_records)),
             }
+
+            # Persist decisions via the StrategizeDecisionHook so they are
+            # available in the database (DecisionService.list_decisions) for
+            # downstream CLI commands such as ``plan tree`` and ``plan correct``.
+            # This wires the hook into the actual production execution path
+            # instead of leaving it as an unused spike class.
+            if self._decision_hook is not None:
+                persisted_ids: set[str] = set()
+                for decision in result.decisions:
+                    if decision.decision_id not in persisted_ids:
+                        persisted_ids.add(decision.decision_id)
+                        try:
+                            self._decision_hook.record_strategy_choice(
+                                question=decision.step_text,
+                                chosen_option=decision.step_text,
+                                confidence_score=None,
+                                rationale="",
+                            )
+                            self._logger.debug(
+                                "Decision persisted to database during strategize",
+                                plan_id=plan_id,
+                                decision_id=decision.decision_id,
+                                sequence=decision.sequence,
+                            )
+                        except Exception:  # noqa: BLE001
+                            # Best-effort persistence: if recording fails, the
+                            # JSON fallback in error_details still allows
+                            # _build_decisions to reconstruct the tree for
+                            # the Execute phase.  Log and continue.
+                            self._logger.warning(
+                                "Failed to persist decision during strategize "
+                                "(JSON fallback available)",
+                                plan_id=plan_id,
+                                decision_id=decision.decision_id,
+                                exc_info=True,
+                            )
+
             self._lifecycle._commit_plan(plan)
             if self._execution_context is not None:
                 self._execution_context.decision_root_id = result.decision_root_id