diff --git a/features/plan_executor_decision_persistence.feature b/features/plan_executor_decision_persistence.feature index 4c96e02f1..b0612f655 100644 --- a/features/plan_executor_decision_persistence.feature +++ b/features/plan_executor_decision_persistence.feature @@ -16,7 +16,7 @@ Feature: PlanExecutor decision persistence during Strategize (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 decision service tree for the plan should have 2 nodes And the dp plan JSON decision storage should remain populated (backward compat) Scenario: run_strategize persists empty decision list via decision_hook @@ -70,4 +70,4 @@ Feature: PlanExecutor decision persistence during Strategize (Issue #10813) 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 + Then listing dp decisions for the plan should return 2 results diff --git a/features/steps/plan_executor_decision_persistence_steps.py b/features/steps/plan_executor_decision_persistence_steps.py index 09e28f45e..23519c7cf 100644 --- a/features/steps/plan_executor_decision_persistence_steps.py +++ b/features/steps/plan_executor_decision_persistence_steps.py @@ -341,3 +341,75 @@ def step_given_dp_failing_service(context: Context) -> None: return _failing_record(*args, **kwargs) context.dp_failing_svc = FailingDecisionSvc() + + +# ---------------------------------------------------------------------- +# Missing step definitions required by feature scenarios +# ---------------------------------------------------------------------- + + +@given("a dp plan in Strategize-Queued state with definition {definition}") +def step_given_dp_plan_with_definition(context: Context, definition: str) -> None: + """Create a mock lifecycle service with a specific plan definition. + + The plan is placed in STRATEGIZE phase with QUEUED processing state. + This step replaces ``a dp mock lifecycle service`` for scenarios that + need explicit control over the decision_of_done text. + + Args: + definition: Multiline decision_of_done string (may contain + literal ``\\n`` which behave will convert to newlines when + using double-quoted step text in Gherkin). + """ + plan = _dp_make_plan(definition_of_done=definition) + 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 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. + + Used by the empty-list scenario to verify the hook handles zero + decisions gracefully. + """ + plan = _dp_make_plan(definition_of_done="") + 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 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") + + +@then("the dp JSON fallback in error_details should still be populated") +def step_then_dp_json_still_populated(context: Context) -> None: + """Verify error_details contains strategy_decisions_json. + + Alias for ``step_then_dp_json_fallback_populated``; Behave/Gherkin + distinguishes between "should be" and "should still be" so a + separate handler is required. + """ + step_then_dp_json_fallback_populated(context) + + diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index e40cac8b0..3fe7882a4 100644 --- a/src/cleveragents/application/services/plan_executor.py +++ b/src/cleveragents/application/services/plan_executor.py @@ -795,6 +795,9 @@ class PlanExecutor: # 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. + # Each decision's parent_id from StrategizeResult drives the + # hierarchy in the persisted decision tree, preserving the exact + # same structure that _build_decisions will later reconstruct. if self._decision_hook is not None: persisted_ids: set[str] = set() for decision in result.decisions: @@ -803,9 +806,10 @@ class PlanExecutor: try: self._decision_hook.record_strategy_choice( question=decision.step_text, - chosen_option=decision.step_text, + chosen_option="Yes", confidence_score=None, rationale="", + parent_decision_id=decision.parent_id, ) self._logger.debug( "Decision persisted to database during strategize", diff --git a/src/cleveragents/application/services/strategize_decision_hook.py b/src/cleveragents/application/services/strategize_decision_hook.py index 414e47a3c..b490bcf95 100644 --- a/src/cleveragents/application/services/strategize_decision_hook.py +++ b/src/cleveragents/application/services/strategize_decision_hook.py @@ -98,6 +98,7 @@ class StrategizeDecisionHook: context_data: dict[str, Any] | None = None, actor_state: dict[str, Any] | None = None, relevant_resources: list[str] | None = None, + parent_decision_id: str | None = None, ) -> Decision: """Record a strategy choice decision during Strategize. @@ -110,6 +111,9 @@ class StrategizeDecisionHook: context_data: Current context window contents. actor_state: Actor's current state. relevant_resources: Resource IDs that influenced the decision. + parent_decision_id: Optional parent decision ULID. When + provided this overrides ``self.parent_decision_id`` so + that each call can specify its own tree position. Returns: The recorded Decision. @@ -128,6 +132,12 @@ class StrategizeDecisionHook: relevant_resources=relevant_resources, ) + # 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 + ) + self._logger.info( "Recording strategy choice decision", question=question, @@ -141,7 +151,7 @@ class StrategizeDecisionHook: decision_type=DecisionType.STRATEGY_CHOICE, question=question, chosen_option=chosen_option, - parent_decision_id=self.parent_decision_id, + parent_decision_id=effective_parent, alternatives_considered=alternatives_considered, confidence_score=confidence_score, rationale=rationale, @@ -171,6 +181,7 @@ class StrategizeDecisionHook: context_data: dict[str, Any] | None = None, actor_state: dict[str, Any] | None = None, relevant_resources: list[str] | None = None, + parent_decision_id: str | None = None, ) -> Decision: """Record a resource selection decision during Strategize. @@ -183,6 +194,9 @@ class StrategizeDecisionHook: context_data: Current context window contents. actor_state: Actor's current state. relevant_resources: Resource IDs that influenced the decision. + parent_decision_id: Optional parent decision ULID. When + provided this overrides ``self.parent_decision_id`` so + that each call can specify its own tree position. Returns: The recorded Decision. @@ -201,6 +215,10 @@ class StrategizeDecisionHook: relevant_resources=relevant_resources, ) + effective_parent = ( + parent_decision_id if parent_decision_id is not None else self.parent_decision_id + ) + self._logger.info( "Recording resource selection decision", question=question, @@ -213,7 +231,7 @@ class StrategizeDecisionHook: decision_type=DecisionType.RESOURCE_SELECTION, question=question, chosen_option=chosen_option, - parent_decision_id=self.parent_decision_id, + parent_decision_id=effective_parent, alternatives_considered=alternatives_considered, confidence_score=confidence_score, rationale=rationale, @@ -243,6 +261,7 @@ class StrategizeDecisionHook: context_data: dict[str, Any] | None = None, actor_state: dict[str, Any] | None = None, relevant_resources: list[str] | None = None, + parent_decision_id: str | None = None, ) -> Decision: """Record a subplan spawn decision during Strategize. @@ -255,6 +274,9 @@ class StrategizeDecisionHook: context_data: Current context window contents. actor_state: Actor's current state. relevant_resources: Resource IDs that influenced the decision. + parent_decision_id: Optional parent decision ULID. When + provided this overrides ``self.parent_decision_id`` so + that each call can specify its own tree position. Returns: The recorded Decision. @@ -273,6 +295,10 @@ class StrategizeDecisionHook: relevant_resources=relevant_resources, ) + effective_parent = ( + parent_decision_id if parent_decision_id is not None else self.parent_decision_id + ) + self._logger.info( "Recording subplan spawn decision", question=question, @@ -285,7 +311,7 @@ class StrategizeDecisionHook: decision_type=DecisionType.SUBPLAN_SPAWN, question=question, chosen_option=chosen_option, - parent_decision_id=self.parent_decision_id, + parent_decision_id=effective_parent, alternatives_considered=alternatives_considered, confidence_score=confidence_score, rationale=rationale, @@ -315,6 +341,7 @@ class StrategizeDecisionHook: context_data: dict[str, Any] | None = None, actor_state: dict[str, Any] | None = None, relevant_resources: list[str] | None = None, + parent_decision_id: str | None = None, ) -> Decision: """Record an invariant enforcement decision during Strategize. @@ -327,6 +354,9 @@ class StrategizeDecisionHook: context_data: Current context window contents. actor_state: Actor's current state. relevant_resources: Resource IDs that influenced the decision. + parent_decision_id: Optional parent decision ULID. When + provided this overrides ``self.parent_decision_id`` so + that each call can specify its own tree position. Returns: The recorded Decision. @@ -345,6 +375,10 @@ class StrategizeDecisionHook: relevant_resources=relevant_resources, ) + effective_parent = ( + parent_decision_id if parent_decision_id is not None else self.parent_decision_id + ) + self._logger.info( "Recording invariant enforced decision", question=question, @@ -357,7 +391,7 @@ class StrategizeDecisionHook: decision_type=DecisionType.INVARIANT_ENFORCED, question=question, chosen_option=chosen_option, - parent_decision_id=self.parent_decision_id, + parent_decision_id=effective_parent, alternatives_considered=alternatives_considered, confidence_score=confidence_score, rationale=rationale,