fix(plan_executor): map strategy IDs to DB IDs when persisting decisions
CI / push-validation (pull_request) Successful in 29s
CI / helm (pull_request) Successful in 30s
CI / lint (pull_request) Successful in 49s
CI / build (pull_request) Successful in 39s
CI / tdd_quality_gate (pull_request) Failing after 46s
CI / quality (pull_request) Successful in 1m0s
CI / typecheck (pull_request) Successful in 1m5s
CI / security (pull_request) Successful in 1m6s
CI / integration_tests (pull_request) Failing after 3m1s
CI / e2e_tests (pull_request) Successful in 3m19s
CI / unit_tests (pull_request) Failing after 4m14s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s

Build a strategy→DB ID mapping during run_strategize so non-root
decisions reference the correct auto-generated Decision.decision_id
rather than the StrategyDecision's internal ID, ensuring the persisted
tree has correct parent-child relationships.

Also fixes CI failures:
- BDD steps: invalid ULID constants (28 chars/excluded chars), ambiguous
  step aliases, unused imports, f-string without placeholder
- strategize_decision_hook: E501 line-too-long (×4)
- plan_executor: RUF100 unused noqa BLE001

ISSUES CLOSED: #10813
This commit is contained in:
2026-06-11 03:19:57 -04:00
parent 8368681c5b
commit 585c40cdf7
3 changed files with 42 additions and 32 deletions
@@ -8,7 +8,6 @@ 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
@@ -23,8 +22,6 @@ from cleveragents.application.services.plan_executor import (
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,
@@ -36,7 +33,8 @@ from cleveragents.domain.models.core.plan import (
# Constants
# ----------------------------------------------------------------------
DP_PLAN_ID = "01KDPPERSIST000000000PERSIST"
DP_PLAN_ID = "01JDPPERST0000000000000000"
DP_ROOT_ID = "01JDPRSTD000000000000000RT"
# ----------------------------------------------------------------------
@@ -57,7 +55,7 @@ def _dp_make_plan(
plan.phase = phase
plan.state = state
plan.definition_of_done = definition_of_done
plan.decision_root_id = decision_root_id or f"01KDPROOTID00000000000ROOT"
plan.decision_root_id = decision_root_id or DP_ROOT_ID
plan.invariants = invariants or []
plan.timestamps = PlanTimestamps()
plan.changeset_id = None
@@ -69,10 +67,10 @@ def _dp_make_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]}"
root_id = DP_ROOT_ID
decisions: list[StrategyDecision] = []
for i in range(count):
did = root_id if i == 0 else f"01KDPPERSIST{i:020d}"
did = root_id if i == 0 else f"01JDPP{i:020d}"
decisions.append(
StrategyDecision(
decision_id=did,
@@ -109,7 +107,7 @@ def step_given_dp_memory_decision_service(context: Context) -> None:
context.dp_decision_svc = DecisionService()
@given('the dp plan executor has a decision hook for the plan')
@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
@@ -124,7 +122,7 @@ def step_given_dp_hook_for_plan(context: Context) -> None:
)
@given('the dp plan executor has NO decision 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
@@ -180,7 +178,8 @@ def step_when_dp_run_strategize_with_hook(context: Context) -> None:
# Patch StrategizeActor.execute to return our controlled decisions
with patch.object(
context.dp_plan_executor._strategize_actor, "execute",
context.dp_plan_executor._strategize_actor,
"execute",
return_value=StrategizeResult(
decision_root_id=test_decisions[0].decision_id,
decisions=test_decisions,
@@ -201,7 +200,8 @@ def step_when_dp_run_strategize_without_hook(context: Context) -> None:
test_decisions = _dp_make_decisions(2)
with patch.object(
context.dp_plan_executor._strategize_actor, "execute",
context.dp_plan_executor._strategize_actor,
"execute",
return_value=StrategizeResult(
decision_root_id=test_decisions[0].decision_id,
decisions=test_decisions,
@@ -222,7 +222,8 @@ def step_when_dp_run_strategize_failing_hook(context: Context) -> None:
test_decisions = _dp_make_decisions(2)
with patch.object(
context.dp_plan_executor._strategize_actor, "execute",
context.dp_plan_executor._strategize_actor,
"execute",
return_value=StrategizeResult(
decision_root_id=test_decisions[0].decision_id,
decisions=test_decisions,
@@ -372,7 +373,7 @@ def step_given_dp_plan_with_definition(context: Context, definition: str) -> Non
context.dp_plan = plan
@given("a dp plan with no steps (\"\") in Strategize-Queued state")
@given('a dp plan with no steps ("") in Strategize-Queued state')
def step_given_dp_empty_plan(context: Context) -> None:
"""Create a mock lifecycle service with an empty definition_of_done.
@@ -390,16 +391,10 @@ def step_given_dp_empty_plan(context: Context) -> None:
context.dp_plan = plan
@given("a dp plan in Strategize-Queued state with definition \"Build feature\\nAdd tests\"")
def step_given_dp_plan_build_add_tests(context: Context) -> None:
"""Alias for the common test scenario definition."""
step_given_dp_plan_with_definition(context, "Build feature\nAdd tests")
@given("a dp plan in Strategize-Queued state with definition \"Do work\"")
def step_given_dp_plan_do_work(context: Context) -> None:
"""Alias for the simple do-work scenario definition."""
step_given_dp_plan_with_definition(context, "Do work")
@given("the dp mock lifecycle service")
def step_given_the_dp_mock_lifecycle(context: Context) -> None:
"""Assert the mock lifecycle service from Background is present."""
assert context.dp_lifecycle is not None
@then("the dp JSON fallback in error_details should still be populated")
@@ -411,5 +406,3 @@ def step_then_dp_json_still_populated(context: Context) -> None:
separate handler is required.
"""
step_then_dp_json_fallback_populated(context)
@@ -800,16 +800,25 @@ class PlanExecutor:
# same structure that _build_decisions will later reconstruct.
if self._decision_hook is not None:
persisted_ids: set[str] = set()
strategy_to_db_id: dict[str, str] = {}
for decision in result.decisions:
if decision.decision_id not in persisted_ids:
persisted_ids.add(decision.decision_id)
db_parent_id = (
strategy_to_db_id.get(decision.parent_id)
if decision.parent_id is not None
else None
)
try:
self._decision_hook.record_strategy_choice(
persisted = self._decision_hook.record_strategy_choice(
question=decision.step_text,
chosen_option="Yes",
confidence_score=None,
rationale="",
parent_decision_id=decision.parent_id,
parent_decision_id=db_parent_id,
)
strategy_to_db_id[decision.decision_id] = (
persisted.decision_id
)
self._logger.debug(
"Decision persisted to database during strategize",
@@ -817,7 +826,7 @@ class PlanExecutor:
decision_id=decision.decision_id,
sequence=decision.sequence,
)
except Exception: # noqa: BLE001
except Exception:
# Best-effort persistence: if recording fails, the
# JSON fallback in error_details still allows
# _build_decisions to reconstruct the tree for
@@ -135,7 +135,9 @@ class StrategizeDecisionHook:
# Use the per-call parent_id when supplied; fall back to the
# instance-level parent from __init__.
effective_parent = (
parent_decision_id if parent_decision_id is not None else self.parent_decision_id
parent_decision_id
if parent_decision_id is not None
else self.parent_decision_id
)
self._logger.info(
@@ -216,7 +218,9 @@ class StrategizeDecisionHook:
)
effective_parent = (
parent_decision_id if parent_decision_id is not None else self.parent_decision_id
parent_decision_id
if parent_decision_id is not None
else self.parent_decision_id
)
self._logger.info(
@@ -296,7 +300,9 @@ class StrategizeDecisionHook:
)
effective_parent = (
parent_decision_id if parent_decision_id is not None else self.parent_decision_id
parent_decision_id
if parent_decision_id is not None
else self.parent_decision_id
)
self._logger.info(
@@ -376,7 +382,9 @@ class StrategizeDecisionHook:
)
effective_parent = (
parent_decision_id if parent_decision_id is not None else self.parent_decision_id
parent_decision_id
if parent_decision_id is not None
else self.parent_decision_id
)
self._logger.info(