diff --git a/CHANGELOG.md b/CHANGELOG.md index 98811b5b3..61543b7c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ Changed `wf10_batch.robot` to be less likely to create files, and ## [Unreleased] +- **SubplanExecutionService lazy wiring in CLI** (#10268): Wired `subplan_service` + from the DI container into `_get_plan_executor()` so `PlanExecutor` can spawn child + plans during the Execute phase. When `SubplanService` is available but + `SubplanExecutionService` is not explicitly injected, `_execute_subplans()` now + lazily creates a `SubplanExecutionService` using the parent plan's `subplan_config` + and the `_execute_child_plan` callback. Added recursion guard and strategize result + validation to `_execute_child_plan` to prevent re-entrant or orphaned child plan + execution. + Data integrity fix: ValidationAttachmentRepository argument swap (#7492): Fixed a critical data integrity issue in `ValidationAttachmentRepository.attach` where `validation_name` and `resource_id` arguments were being silently swapped based on a diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 5e0105453..88cd53d5e 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -25,6 +25,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **SubplanExecutionService lazy wiring in CLI** (#10268): Wired `subplan_service` + from the DI container into `_get_plan_executor()` so `PlanExecutor` can spawn child + plans during the Execute phase. When `SubplanService` is available but + `SubplanExecutionService` is not explicitly injected, `_execute_subplans()` now + lazily creates a `SubplanExecutionService` using the parent plan's `subplan_config` + and the `_execute_child_plan` callback. Added recursion guard and strategize result + validation to `_execute_child_plan` to prevent re-entrant or orphaned child plan + execution. + - **TDD: MCPToolAdapter.infer_resource_slots() TypeError with null properties** (#10470): Added a TDD issue-capture Behave scenario that reproduces the bug where `MCPToolAdapter.infer_resource_slots()` raises `TypeError` when the input schema diff --git a/features/plan_execution_context_coverage_boost.feature b/features/plan_execution_context_coverage_boost.feature new file mode 100644 index 000000000..ec112f10f --- /dev/null +++ b/features/plan_execution_context_coverage_boost.feature @@ -0,0 +1,68 @@ +@coverage @context +Feature: PlanExecutionContext coverage boost + As a developer + I want complete coverage of PlanExecutionContext and RuntimeExecuteActor + So that all uncovered lines in plan_execution_context.py are exercised + + Scenario: PlanExecutionContext rejects empty plan_id + Given a pec fresh context + When I pec construct PlanExecutionContext with empty plan_id + Then a pec ValidationError should be raised with "plan_id must not be empty" + + Scenario: PlanExecutionContext properties return configured values + Given a pec PlanExecutionContext with all optional fields set + Then pec the automation_profile property should return the configured value + And pec the plan_env property should return the configured value + And pec the project_env property should return the configured value + And pec the project_resources property should return the configured dict + And pec the resource_bindings property should return the configured dict + And pec the changeset_store property should return the configured store + And pec the active_changeset_ids property should return a list + + Scenario: record_change raises PlanError when no changeset is active + Given a pec PlanExecutionContext with a valid plan_id + When I pec call record_change without starting a changeset + Then a pec PlanError should be raised about no active changeset + + Scenario: get_changeset returns None for unknown changeset + Given a pec PlanExecutionContext with a valid plan_id + When I pec call get_changeset with a nonexistent changeset_id + Then pec the result should be None + + Scenario: summarize returns metadata for execution context + Given a pec PlanExecutionContext with a valid plan_id + When I pec call summarize on the execution context + Then pec the summary should contain plan_id and decision_root_id + And pec the summary should contain counts for resources and bindings + + Scenario: RuntimeExecuteActor rejects None tool_runner + Given a pec valid PlanExecutionContext + When I pec construct RuntimeExecuteActor with None tool_runner + Then a pec ValidationError should be raised with "tool_runner must not be None" + + Scenario: RuntimeExecuteActor rejects None execution_context + Given a pec mock ToolRunner + When I pec construct RuntimeExecuteActor with None execution_context + Then a pec ValidationError should be raised with "execution_context must not be None" + + Scenario: RuntimeExecuteActor properties return configured values + Given a pec RuntimeExecuteActor with valid tool_runner and execution_context + Then pec the tool_runner property should return the configured ToolRunner + And pec the execution_context property should return the configured PlanExecutionContext + + Scenario: RuntimeExecuteActor execute with stream callback produces events + Given a pec RuntimeExecuteActor with valid tool_runner and execution_context with sandbox_root + And a pec stream event collector + When I pec execute with decisions and stream callback + Then pec the stream events should include "runtime_execute_started" + And pec the stream events should include "runtime_execute_complete" + + Scenario: RuntimeExecuteActor execute without stream callback still works + Given a pec RuntimeExecuteActor with valid tool_runner and execution_context + When I pec execute with decisions and no stream callback + Then pec the result should be a RuntimeExecuteResult with a valid changeset_id + + Scenario: RuntimeExecuteActor execute with sandbox_root populates sandbox_refs + Given a pec RuntimeExecuteActor with valid tool_runner and execution_context with sandbox_root + When I pec execute with decisions and no stream callback + Then pec the result sandbox_refs should include the sandbox_root diff --git a/features/plan_execution_hierarchical.feature b/features/plan_execution_hierarchical.feature new file mode 100644 index 000000000..1bd1e7390 --- /dev/null +++ b/features/plan_execution_hierarchical.feature @@ -0,0 +1,28 @@ +@mock_only @subplan @plan_executor @tdd_issue @tdd_issue_10268 +Feature: PlanExecutor hierarchical execution wiring + As a system executing hierarchical plans via the production CLI + I want SubplanExecutionService to be available when SubplanService is wired + So that child plan execution works without a pre-configured execution service + + # ------------------------------------------------------------------ + # SubplanExecutionService lazy creation (Forgejo #10268) + # ------------------------------------------------------------------ + + Scenario: PlanExecutor lazily creates SubplanExecutionService when SubplanService is present + Given a PlanExecutor with SubplanService but no SubplanExecutionService + And a parent plan in Execute phase with a subplan_spawn decision + When I call run_execute on the parent plan + Then SubplanService.get_spawn_decisions should have been called + And SubplanService.spawn should have been called with the spawn entries + And SubplanExecutionService.execute_all should have been called via lazy creation + + # ------------------------------------------------------------------ + # _execute_child_plan callback (Forgejo #10268) + # ------------------------------------------------------------------ + + Scenario: _execute_child_plan callback invokes strategize for a child plan + Given a PlanExecutor with SubplanService but no SubplanExecutionService + And a parent plan in Execute phase with a subplan_spawn decision + And a child plan ready for execution in the lifecycle service + When I call run_execute on the parent plan + Then SubplanExecutionService.execute_all should have been called via lazy creation diff --git a/features/plan_executor_child_plan_execution.feature b/features/plan_executor_child_plan_execution.feature new file mode 100644 index 000000000..40394d590 --- /dev/null +++ b/features/plan_executor_child_plan_execution.feature @@ -0,0 +1,124 @@ +@plan @executor @subplan @coverage +Feature: PlanExecutor child plan execution coverage + As a developer + I want comprehensive tests for _execute_child_plan, _execute_subplans fallback, + _apply_subplan_results_to_plan exec_result=None, and checkpoint PlanError handling + So that uncovered lines in plan_executor.py are fully exercised + + # ------------------------------------------------------------------ + # _execute_child_plan — success path (lines 606, 616-649, 662) + # ------------------------------------------------------------------ + + Scenario: Execute child plan succeeds in stub mode + Given a child3 PlanExecutor for child plan execution + And child3 a SubplanStatus for a valid child plan + When I child3 call _execute_child_plan on the executor + Then child3 the SubplanExecutionOutput should indicate success + And child3 the SubplanExecutionOutput should have a changeset_summary + And child3 the running_plan_ids should be empty + + # ------------------------------------------------------------------ + # _execute_child_plan — circular detection (lines 607-615) + # ------------------------------------------------------------------ + + Scenario: Execute child plan detects circular re-entrant execution + Given a child3 PlanExecutor for child plan execution + And child3 a SubplanStatus for a valid child plan + And I child3 pre-add the subplan_id to running_plan_ids + When I child3 call _execute_child_plan on the executor + Then child3 the SubplanExecutionOutput should indicate failure + And child3 the SubplanExecutionOutput error should mention circular + + # ------------------------------------------------------------------ + # _execute_child_plan — plan not found (lines 620-624) + # ------------------------------------------------------------------ + + Scenario: Execute child plan returns failure when child plan not found + Given a child3 PlanExecutor with lifecycle returning None for child plan + And child3 a SubplanStatus for a valid child plan + When I child3 call _execute_child_plan on the executor + Then child3 the SubplanExecutionOutput should indicate failure + And child3 the SubplanExecutionOutput error should mention "not found" + + # ------------------------------------------------------------------ + # _execute_child_plan — strategize produces no decisions (lines 626-634) + # ------------------------------------------------------------------ + + Scenario: Execute child plan returns failure when strategize produces no decisions + Given a child3 PlanExecutor for child plan with empty definition + And child3 a SubplanStatus for a valid child plan + When I child3 call _execute_child_plan on the executor + Then child3 the SubplanExecutionOutput should indicate failure + And child3 the SubplanExecutionOutput error should mention "no decisions" + + # ------------------------------------------------------------------ + # _execute_child_plan — exception during execution (lines 650-660) + # ------------------------------------------------------------------ + + Scenario: Execute child plan catches exception and returns failure output + Given a child3 PlanExecutor whose lifecycle start_strategize raises RuntimeError + And child3 a SubplanStatus for a valid child plan + When I child3 call _execute_child_plan on the executor + Then child3 the SubplanExecutionOutput should indicate failure + And child3 the SubplanExecutionOutput error should contain the exception message + + # ------------------------------------------------------------------ + # _execute_subplans — fallback when subplan_execution_service is None + # but subplan_service is available (lines 521-527) + # ------------------------------------------------------------------ + + Scenario: _execute_subplans creates SubplanExecutionService when not injected + Given a child3 PlanExecutor with subplan_service but no subplan_execution_service + And child3 a SpawnResult with one spawned status + When I child3 call _execute_subplans on the executor + Then child3 the SubplanExecutionResult should not be None + + # ------------------------------------------------------------------ + # _execute_subplans — returns None when no subplan service (line 521) + # ------------------------------------------------------------------ + + Scenario: _execute_subplans returns None when both services are missing + Given a child3 PlanExecutor with no subplan services at all + And child3 a SpawnResult with one spawned status + When I child3 call _execute_subplans on the executor + Then child3 the SubplanExecutionResult should be None + + # ------------------------------------------------------------------ + # _execute_subplans — returns None when statuses is empty (line 532) + # ------------------------------------------------------------------ + + Scenario: _execute_subplans returns None when spawned_statuses is empty + Given a child3 PlanExecutor with subplan_service and success executor + And child3 a SpawnResult with no spawned statuses + When I child3 call _execute_subplans on the executor + Then child3 the SubplanExecutionResult should be None + + # ------------------------------------------------------------------ + # _apply_subplan_results_to_plan — exec_result is None (lines 569-570) + # ------------------------------------------------------------------ + + Scenario: _apply_subplan_results keeps spawned statuses when exec_result is None + Given a child3 PlanExecutor for apply subplan results + And child3 a SpawnResult with one spawned status + When I child3 call _apply_subplan_results_to_plan with exec_result None + Then child3 the plan subplan_statuses should match the spawn result statuses + + # ------------------------------------------------------------------ + # Property accessors for optional services (lines 433, 438, 443) + # ------------------------------------------------------------------ + + Scenario: Optional service properties return configured values + Given a child3 PlanExecutor with all optional services configured + Then child3 the fix_revalidate_orchestrator property should return the configured instance + And child3 the subplan_service property should return the configured instance + And child3 the subplan_execution_service property should return the configured instance + + # ------------------------------------------------------------------ + # _try_create_checkpoint — PlanError re-raise (lines 731-741, 745) + # ------------------------------------------------------------------ + + Scenario: _try_create_checkpoint re-raises PlanError from plan update failure + Given a child3 PlanExecutor with checkpoint manager and lifecycle that fails on commit_plan + And child3 a sandbox that returns a valid checkpoint + When I child3 attempt _try_create_checkpoint expecting PlanError + Then a child3 PlanError should be raised about persisting checkpoint metadata diff --git a/features/steps/plan_execution_context_coverage_boost_steps.py b/features/steps/plan_execution_context_coverage_boost_steps.py new file mode 100644 index 000000000..8e976f53f --- /dev/null +++ b/features/steps/plan_execution_context_coverage_boost_steps.py @@ -0,0 +1,340 @@ +"""Step definitions for plan_execution_context_coverage_boost.feature.""" + +from unittest.mock import MagicMock + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.services.plan_execution_context import ( + PlanExecutionContext, + RuntimeExecuteActor, + RuntimeExecuteResult, +) +from cleveragents.core.exceptions import PlanError, ValidationError +from cleveragents.domain.models.core.change import ChangeSetStore +from cleveragents.tool.runner import ToolRunner + + +@given("a pec fresh context") +def step_pec_fresh_context(context: Context) -> None: + pass + + +@when("I pec construct PlanExecutionContext with empty plan_id") +def step_pec_construct_empty_plan_id(context: Context) -> None: + context.pec_error = None + try: + PlanExecutionContext(plan_id="") + except Exception as exc: + context.pec_error = exc + + +@then('a pec ValidationError should be raised with "plan_id must not be empty"') +def step_pec_validation_error_empty_plan_id(context: Context) -> None: + assert isinstance(context.pec_error, ValidationError), ( + f"Expected ValidationError, got {type(context.pec_error)}: {context.pec_error}" + ) + assert "plan_id must not be empty" in str(context.pec_error) + + +@given("a pec PlanExecutionContext with all optional fields set") +def step_pec_context_with_all_fields(context: Context) -> None: + context.pec_store = MagicMock(spec=ChangeSetStore) + context.pec_ctx = PlanExecutionContext( + plan_id="01PECPLAN01TESTPECPLAN01", + decision_root_id="01PECDECI01TESTPECDECI01", + sandbox_root="/tmp/pec-sandbox", + automation_profile="trusted", + project_resources={"repo": "my-repo"}, + resource_bindings={"db": MagicMock()}, + changeset_store=context.pec_store, + plan_env="docker", + project_env="kubernetes", + ) + + +@then("pec the automation_profile property should return the configured value") +def step_pec_automation_profile(context: Context) -> None: + assert context.pec_ctx.automation_profile == "trusted" + + +@then("pec the plan_env property should return the configured value") +def step_pec_plan_env(context: Context) -> None: + assert context.pec_ctx.plan_env == "docker" + + +@then("pec the project_env property should return the configured value") +def step_pec_project_env(context: Context) -> None: + assert context.pec_ctx.project_env == "kubernetes" + + +@then("pec the project_resources property should return the configured dict") +def step_pec_project_resources(context: Context) -> None: + assert context.pec_ctx.project_resources == {"repo": "my-repo"} + + +@then("pec the resource_bindings property should return the configured dict") +def step_pec_resource_bindings(context: Context) -> None: + assert len(context.pec_ctx.resource_bindings) == 1 + assert "db" in context.pec_ctx.resource_bindings + + +@then("pec the changeset_store property should return the configured store") +def step_pec_changeset_store(context: Context) -> None: + assert context.pec_ctx.changeset_store is context.pec_store + + +@then("pec the active_changeset_ids property should return a list") +def step_pec_active_changeset_ids(context: Context) -> None: + assert isinstance(context.pec_ctx.active_changeset_ids, list) + + +@given("a pec PlanExecutionContext with a valid plan_id") +def step_pec_context_valid_plan_id(context: Context) -> None: + store = MagicMock(spec=ChangeSetStore) + store.get.return_value = None # unknown changeset → None + context.pec_ctx = PlanExecutionContext( + plan_id="01PECPLAN02TESTPECPLAN02", + changeset_store=store, + ) + + +@when("I pec call record_change without starting a changeset") +def step_pec_record_change_no_changeset(context: Context) -> None: + from cleveragents.domain.models.core.change import ChangeEntry, ChangeOperation + + entry = ChangeEntry( + plan_id="01PECPLAN02TESTPECPLAN02", + resource_id="res-1", + tool_name="test-tool", + operation=ChangeOperation.MODIFY, + path="/tmp/test", + ) + context.pec_error = None + try: + context.pec_ctx.record_change(entry) + except Exception as exc: + context.pec_error = exc + + +@then("a pec PlanError should be raised about no active changeset") +def step_pec_no_active_changeset(context: Context) -> None: + assert isinstance(context.pec_error, PlanError), ( + f"Expected PlanError, got {type(context.pec_error)}: {context.pec_error}" + ) + assert "No active changeset" in str(context.pec_error) + + +@when("I pec call get_changeset with a nonexistent changeset_id") +def step_pec_get_changeset_nonexistent(context: Context) -> None: + context.pec_get_result = context.pec_ctx.get_changeset("nonexistent-id") + + +@then("pec the result should be None") +def step_pec_result_none(context: Context) -> None: + assert context.pec_get_result is None, ( + f"Expected None, got {context.pec_get_result}" + ) + + +@when("I pec call summarize on the execution context") +def step_pec_summarize(context: Context) -> None: + context.pec_summary = context.pec_ctx.summarize() + + +@then("pec the summary should contain plan_id and decision_root_id") +def step_pec_summary_has_core_fields(context: Context) -> None: + s = context.pec_summary + assert s["plan_id"] == "01PECPLAN02TESTPECPLAN02" + assert "decision_root_id" in s + + +@then("pec the summary should contain counts for resources and bindings") +def step_pec_summary_has_counts(context: Context) -> None: + s = context.pec_summary + assert "project_resource_count" in s + assert "resource_binding_count" in s + assert "active_changeset_count" in s + assert "active_changeset_ids" in s + assert "changeset_summaries" in s + + +@given("a pec valid PlanExecutionContext") +def step_pec_valid_context(context: Context) -> None: + store = MagicMock(spec=ChangeSetStore) + context.pec_ctx = PlanExecutionContext( + plan_id="01PECPLAN03TESTPECPLAN03", + changeset_store=store, + ) + + +@when("I pec construct RuntimeExecuteActor with None tool_runner") +def step_pec_runtime_actor_no_tool_runner(context: Context) -> None: + context.pec_error = None + try: + RuntimeExecuteActor( + tool_runner=None, + execution_context=context.pec_ctx, + ) + except Exception as exc: + context.pec_error = exc + + +@then('a pec ValidationError should be raised with "tool_runner must not be None"') +def step_pec_tool_runner_validation_error(context: Context) -> None: + assert isinstance(context.pec_error, ValidationError), ( + f"Expected ValidationError, got {type(context.pec_error)}: {context.pec_error}" + ) + assert "tool_runner must not be None" in str(context.pec_error) + + +@given("a pec mock ToolRunner") +def step_pec_mock_tool_runner(context: Context) -> None: + context.pec_tool_runner = MagicMock(spec=ToolRunner) + + +@when("I pec construct RuntimeExecuteActor with None execution_context") +def step_pec_runtime_actor_no_context(context: Context) -> None: + context.pec_error = None + try: + RuntimeExecuteActor( + tool_runner=context.pec_tool_runner, + execution_context=None, + ) + except Exception as exc: + context.pec_error = exc + + +@then( + 'a pec ValidationError should be raised with "execution_context must not be None"' +) +def step_pec_execution_context_validation_error(context: Context) -> None: + assert isinstance(context.pec_error, ValidationError), ( + f"Expected ValidationError, got {type(context.pec_error)}: {context.pec_error}" + ) + assert "execution_context must not be None" in str(context.pec_error) + + +@given("a pec RuntimeExecuteActor with valid tool_runner and execution_context") +def step_pec_runtime_actor_valid(context: Context) -> None: + store = MagicMock(spec=ChangeSetStore) + store.start.return_value = "01PECCHG01TESTPECCHG01" + tool_runner = MagicMock(spec=ToolRunner) + tool_runner.discover.return_value = [] + + ctx = PlanExecutionContext( + plan_id="01PECPLAN04TESTPECPLAN04", + changeset_store=store, + ) + context.pec_store = store + context.pec_tool_runner = tool_runner + context.pec_actor = RuntimeExecuteActor( + tool_runner=tool_runner, + execution_context=ctx, + ) + + +@given( + "a pec RuntimeExecuteActor with valid tool_runner and execution_context " + "with sandbox_root" +) +def step_pec_runtime_actor_with_sandbox(context: Context) -> None: + store = MagicMock(spec=ChangeSetStore) + store.start.return_value = "01PECCHG02TESTPECCHG02" + tool_runner = MagicMock(spec=ToolRunner) + tool_runner.discover.return_value = [] + + ctx = PlanExecutionContext( + plan_id="01PECPLAN05TESTPECPLAN05", + sandbox_root="/tmp/pec-sandbox", + changeset_store=store, + ) + context.pec_store = store + context.pec_tool_runner = tool_runner + context.pec_actor = RuntimeExecuteActor( + tool_runner=tool_runner, + execution_context=ctx, + ) + + +@given("a pec stream event collector") +def step_pec_stream_collector(context: Context) -> None: + events: list[tuple[str, dict]] = [] + context.pec_events = events + context.pec_stream_callback = lambda evt, data: events.append((evt, data)) + + +@then("pec the tool_runner property should return the configured ToolRunner") +def step_pec_tool_runner_property(context: Context) -> None: + assert context.pec_actor.tool_runner is context.pec_tool_runner + + +@then( + "pec the execution_context property should return the configured " + "PlanExecutionContext" +) +def step_pec_execution_context_property(context: Context) -> None: + assert context.pec_actor.execution_context is not None + assert context.pec_actor.execution_context.plan_id == "01PECPLAN04TESTPECPLAN04" + + +class _FakeDecision: + def __init__(self, decision_id: str, step_text: str, sequence: int): + self.decision_id = decision_id + self.step_text = step_text + self.sequence = sequence + + +@when("I pec execute with decisions and stream callback") +def step_pec_execute_with_callback(context: Context) -> None: + decisions = [ + _FakeDecision("dec-1", "Step 1", 0), + _FakeDecision("dec-2", "Step 2", 1), + ] + context.pec_result = context.pec_actor.execute( + decisions=decisions, + stream_callback=context.pec_stream_callback, + ) + + +@when("I pec execute with decisions and no stream callback") +def step_pec_execute_no_callback(context: Context) -> None: + decisions = [ + _FakeDecision("dec-1", "Step 1", 0), + ] + context.pec_result = context.pec_actor.execute( + decisions=decisions, + stream_callback=None, + ) + + +@then('pec the stream events should include "runtime_execute_started"') +def step_pec_events_started(context: Context) -> None: + event_names = [e[0] for e in context.pec_events] + assert "runtime_execute_started" in event_names, ( + f"Expected runtime_execute_started in {event_names}" + ) + + +@then('pec the stream events should include "runtime_execute_complete"') +def step_pec_events_complete(context: Context) -> None: + event_names = [e[0] for e in context.pec_events] + assert "runtime_execute_complete" in event_names, ( + f"Expected runtime_execute_complete in {event_names}" + ) + + +@then("pec the result should be a RuntimeExecuteResult with a valid changeset_id") +def step_pec_result_valid_runtime_result(context: Context) -> None: + assert isinstance(context.pec_result, RuntimeExecuteResult), ( + f"Expected RuntimeExecuteResult, got {type(context.pec_result)}" + ) + assert context.pec_result.changeset_id is not None + assert len(context.pec_result.changeset_id) > 0 + + +@then("pec the result sandbox_refs should include the sandbox_root") +def step_pec_sandbox_refs_include_root(context: Context) -> None: + assert "/tmp/pec-sandbox" in context.pec_result.sandbox_refs, ( + f"Expected /tmp/pec-sandbox in sandbox_refs: {context.pec_result.sandbox_refs}" + ) diff --git a/features/steps/plan_execution_hierarchical_steps.py b/features/steps/plan_execution_hierarchical_steps.py new file mode 100644 index 000000000..451b026d6 --- /dev/null +++ b/features/steps/plan_execution_hierarchical_steps.py @@ -0,0 +1,207 @@ +"""Step definitions for plan_execution_hierarchical.feature. + +Tests that PlanExecutor lazily creates SubplanExecutionService when +SubplanService is wired but SubplanExecutionService is not pre-configured +(Forgejo #10268). +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from behave import given, then +from behave.runner import Context + +from cleveragents.application.services.plan_executor import PlanExecutor +from cleveragents.application.services.subplan_execution_service import ( + SubplanExecutionResult, + SubplanExecutionService, +) +from cleveragents.application.services.subplan_service import ( + SpawnEntry, + SpawnResult, + SubplanService, +) +from cleveragents.domain.models.core.decision import ( + ContextSnapshot, + Decision, + DecisionType, +) +from cleveragents.domain.models.core.plan import ( + ExecutionMode, + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + ProcessingState, + SubplanStatus, +) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +_PLAN_ID = "01KNFMGJSG67S6RG9TVXV205TQ" +_ROOT_ID = "01KNFMGJSH67S6RG9TVXV205TR" +_DEC_ID = "01KNFMGJSH67S6RG9TVXV205TS" +_SUBPLAN_ID = "01KNFMGJSH67S6RG9TVXV205TT" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_plan(*, phase=PlanPhase.EXECUTE, state=ProcessingState.QUEUED): + return Plan( + identity=PlanIdentity(plan_id=_PLAN_ID, root_plan_id=_ROOT_ID), + namespaced_name=NamespacedName(namespace="local", name="test-plan"), + description="Test plan for hierarchical execution", + action_name="local/test-action", + phase=phase, + processing_state=state, + ) + + +def _make_spawn_decision( + decision_type: DecisionType = DecisionType.SUBPLAN_SPAWN, +) -> Decision: + return Decision( + decision_id=_DEC_ID, + plan_id=_PLAN_ID, + decision_type=decision_type, + sequence_number=0, + question="Spawn a child plan?", + chosen_option="local/sub-action", + context_snapshot=ContextSnapshot(relevant_resources=[]), + ) + + +def _make_spawn_result( + execution_mode: str = ExecutionMode.SEQUENTIAL, +) -> SpawnResult: + sub_status = SubplanStatus( + subplan_id=_SUBPLAN_ID, + action_name="local/sub-action", + ) + return SpawnResult( + spawned_statuses=[sub_status], + metadata={}, + total_spawned=1, + execution_mode=execution_mode, + child_plans=[], + ) + + +def _make_subplan_service( + decisions: list[Decision] | None = None, + spawn_result: SpawnResult | None = None, +) -> MagicMock: + svc = MagicMock(spec=SubplanService) + svc.get_spawn_decisions.return_value = decisions or [] + svc.build_spawn_entries.return_value = [ + SpawnEntry(decision=d, action_name="local/sub-action") + for d in (decisions or []) + ] + svc.spawn.return_value = spawn_result or _make_spawn_result() + return svc + + +def _make_lifecycle(plan: Plan) -> MagicMock: + lcs = MagicMock() + lcs.get_plan.return_value = plan + lcs.start_execute = MagicMock() + lcs.complete_execute = MagicMock() + lcs.fail_execute = MagicMock() + lcs._commit_plan = MagicMock() + return lcs + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("a PlanExecutor with SubplanService but no SubplanExecutionService") +def step_given_executor_with_subplan_service_only(context: Context) -> None: + decision = _make_spawn_decision(DecisionType.SUBPLAN_SPAWN) + spawn_result = _make_spawn_result() + context.spawn_decision = decision + context.spawn_result = spawn_result + + mock_subplan_svc = _make_subplan_service( + decisions=[decision], spawn_result=spawn_result + ) + plan = _make_plan() + plan.decision_root_id = _ROOT_ID + context.plan = plan + + lcs = _make_lifecycle(plan) + execute_actor = MagicMock() + execute_result = MagicMock() + execute_result.changeset_id = "01JSPAWN0000000000000CS0001" + execute_result.sandbox_refs = [] + execute_result.tool_calls_count = 0 + execute_actor.execute.return_value = execute_result + + executor = PlanExecutor( + lifecycle_service=lcs, + execute_actor=execute_actor, + subplan_service=mock_subplan_svc, + subplan_execution_service=None, + ) + + sub_status = SubplanStatus( + subplan_id=_SUBPLAN_ID, + action_name="local/sub-action", + status=ProcessingState.COMPLETE, + ) + patcher = patch.object(SubplanExecutionService, "execute_all") + mock_exec_all = patcher.start() + mock_exec_all.return_value = SubplanExecutionResult( + all_succeeded=True, + statuses=[sub_status], + merge_result=None, + total_duration_ms=5, + failed_subplan_ids=[], + ) + context._superb_patcher = patcher + context._mock_execute_all = mock_exec_all + + context.executor = executor + context.lcs = lcs + context.mock_subplan_svc = mock_subplan_svc + + +@given("a child plan ready for execution in the lifecycle service") +def step_given_child_plan_ready(context: Context) -> None: + child_plan = Plan( + identity=PlanIdentity( + plan_id=_SUBPLAN_ID, + parent_plan_id=_PLAN_ID, + root_plan_id=_ROOT_ID, + ), + namespaced_name=NamespacedName(namespace="local", name="child-plan"), + description="Child plan for testing", + action_name="local/sub-action", + phase=PlanPhase.STRATEGIZE, + processing_state=ProcessingState.QUEUED, + definition_of_done="- [ ] Step one", + ) + context.lcs.get_plan.side_effect = lambda pid: ( + child_plan if pid == _SUBPLAN_ID else context.plan + ) + context.child_plan = child_plan + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("SubplanExecutionService.execute_all should have been called via lazy creation") +def step_then_execute_all_called_via_lazy(context: Context) -> None: + try: + context._mock_execute_all.assert_called() + finally: + context._superb_patcher.stop() diff --git a/features/steps/plan_executor_child_plan_execution_steps.py b/features/steps/plan_executor_child_plan_execution_steps.py new file mode 100644 index 000000000..ff51a6629 --- /dev/null +++ b/features/steps/plan_executor_child_plan_execution_steps.py @@ -0,0 +1,601 @@ +"""Step definitions for plan_executor_child_plan_execution.feature. + +Tests that cover previously uncovered code paths: +- _execute_child_plan (success, circular, not-found, no-decisions, exception) +- _execute_subplans fallback (lines 521-527, 532) +- _apply_subplan_results_to_plan exec_result=None (lines 569-570) +- Property accessors (lines 433, 438, 443) +- _try_create_checkpoint PlanError re-raise (lines 731-741, 745) +""" + +from unittest.mock import MagicMock +from typing import Any + +from behave import given, then, when +from behave.runner import Context + +from ulid import ULID + +from cleveragents.application.services.plan_executor import ( + PlanExecutor, +) +from cleveragents.application.services.subplan_service import ( + SpawnResult, +) +from cleveragents.core.exceptions import PlanError +from cleveragents.domain.models.core.plan import ( + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + ProcessingState, + SubplanStatus, +) +from cleveragents.infrastructure.sandbox.checkpoint import ( + CheckpointManager, + SandboxCheckpoint, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_PLAN_ID = str(ULID()) +_SUBPLAN_ID = str(ULID()) +_ROOT_PLAN_ID = str(ULID()) + + +def _make_subplan_status( + subplan_id: str | None = None, + action_name: str = "local/child-action", +) -> SubplanStatus: + return SubplanStatus( + subplan_id=subplan_id or _SUBPLAN_ID, + action_name=action_name, + ) + + +def _make_plan( + plan_id: str | None = None, + phase: PlanPhase = PlanPhase.STRATEGIZE, + state: ProcessingState = ProcessingState.QUEUED, + definition_of_done: str | None = "Do the thing", +) -> Plan: + return Plan( + identity=PlanIdentity( + plan_id=plan_id or _PLAN_ID, + root_plan_id=_ROOT_PLAN_ID, + ), + namespaced_name=NamespacedName(namespace="local", name="test-child-plan"), + description="Test child plan", + action_name="local/child-action", + definition_of_done=definition_of_done, + phase=phase, + processing_state=state, + ) + + +def _make_spawn_result( + num_statuses: int = 1, +) -> SpawnResult: + statuses = [ + SubplanStatus( + subplan_id=str(ULID()), + action_name="local/spawned-action", + ) + for i in range(num_statuses) + ] + return SpawnResult( + spawned_statuses=statuses, + metadata={}, + total_spawned=num_statuses, + execution_mode="sequential", + child_plans=[], + ) + + +# --------------------------------------------------------------------------- +# Given: _execute_child_plan success +# --------------------------------------------------------------------------- + + +def _make_child_plan_execution_lifecycle( + definition_of_done: str | None = "Do the thing", +) -> MagicMock: + """Build a mock lifecycle that returns plans in the correct phase order. + + ``_execute_child_plan`` calls ``run_strategize`` (needs STRATEGIZE plan) + and then ``run_execute`` (needs EXECUTE plan). Each of those calls + ``get_plan`` multiple times. We use a counter-based side_effect to + return STRATEGIZE plans for the first 3 calls, then EXECUTE plans with + decision_root_id for the remaining calls. + """ + lcs = MagicMock() + call_counter = {"count": 0} + + def _get_plan_side_effect(plan_id: str) -> Any: + count = call_counter["count"] + call_counter["count"] = count + 1 + if count < 3: + return _make_plan( + phase=PlanPhase.STRATEGIZE, + state=ProcessingState.QUEUED, + definition_of_done=definition_of_done, + ) + p = _make_plan( + phase=PlanPhase.EXECUTE, + state=ProcessingState.QUEUED, + definition_of_done=definition_of_done, + ) + p.decision_root_id = str(ULID()) + return p + + lcs.get_plan.side_effect = _get_plan_side_effect + lcs.start_strategize = MagicMock() + lcs.complete_strategize = MagicMock() + lcs.start_execute = MagicMock() + lcs.complete_execute = MagicMock() + lcs.fail_execute = MagicMock() + lcs._commit_plan = MagicMock() + return lcs + + +@given("a child3 PlanExecutor for child plan execution") +def step_given_child3_executor_for_child_plan(context: Context) -> None: + lcs = _make_child_plan_execution_lifecycle() + executor = PlanExecutor(lifecycle_service=lcs) + context.child3_executor = executor + context.child3_lcs = lcs + context.child3_plan = _make_plan(phase=PlanPhase.STRATEGIZE) + + +@given("child3 a SubplanStatus for a valid child plan") +def step_given_child3_subplan_status(context: Context) -> None: + context.child3_status = _make_subplan_status() + + +@given("I child3 pre-add the subplan_id to running_plan_ids") +def step_given_child3_pre_add_running(context: Context) -> None: + context.child3_executor._running_plan_ids.add(_SUBPLAN_ID) + + +# --------------------------------------------------------------------------- +# Given: plan not found +# --------------------------------------------------------------------------- + + +@given("a child3 PlanExecutor with lifecycle returning None for child plan") +def step_given_child3_executor_lifecycle_returns_none(context: Context) -> None: + lcs = MagicMock() + lcs.get_plan.return_value = None + executor = PlanExecutor(lifecycle_service=lcs) + context.child3_executor = executor + context.child3_lcs = lcs + + +# --------------------------------------------------------------------------- +# Given: strategize produces no decisions +# --------------------------------------------------------------------------- + + +@given("a child3 PlanExecutor for child plan with empty definition") +def step_given_child3_executor_empty_definition(context: Context) -> None: + lcs = _make_child_plan_execution_lifecycle(definition_of_done="") + executor = PlanExecutor(lifecycle_service=lcs) + # Force run_strategize to return a result with no decisions so that + # the "no decisions" error path in _execute_child_plan is exercised. + from cleveragents.application.services.plan_executor import StrategizeResult + + empty_result = StrategizeResult( + decision_root_id="", + decisions=[], + ) + executor.run_strategize = MagicMock(return_value=empty_result) + + context.child3_executor = executor + context.child3_lcs = lcs + context.child3_plan = _make_plan(phase=PlanPhase.STRATEGIZE, definition_of_done="") + + +# --------------------------------------------------------------------------- +# Given: exception during execution +# --------------------------------------------------------------------------- + + +@given("a child3 PlanExecutor whose lifecycle start_strategize raises RuntimeError") +def step_given_child3_executor_start_strategize_raises(context: Context) -> None: + lcs = MagicMock() + plan = _make_plan(phase=PlanPhase.STRATEGIZE) + lcs.get_plan.return_value = plan + lcs.start_strategize.side_effect = RuntimeError("boom from start_strategize") + lcs.fail_strategize = MagicMock() + lcs._commit_plan = MagicMock() + + executor = PlanExecutor(lifecycle_service=lcs) + context.child3_executor = executor + context.child3_lcs = lcs + + +# --------------------------------------------------------------------------- +# Given: _execute_subplans fallback +# --------------------------------------------------------------------------- + + +@given("a child3 PlanExecutor with subplan_service but no subplan_execution_service") +def step_given_child3_executor_subplan_svc_only(context: Context) -> None: + lcs = _make_child_plan_execution_lifecycle() + mock_subplan_svc = MagicMock() + mock_subplan_svc.get_spawn_decisions.return_value = [] + + executor = PlanExecutor( + lifecycle_service=lcs, + subplan_service=mock_subplan_svc, + ) + context.child3_executor = executor + context.child3_lcs = lcs + context.child3_mock_subplan_svc = mock_subplan_svc + + +@given("a child3 PlanExecutor with no subplan services at all") +def step_given_child3_executor_no_subplan_services(context: Context) -> None: + lcs = MagicMock() + executor = PlanExecutor(lifecycle_service=lcs) + context.child3_executor = executor + context.child3_lcs = lcs + + +@given("a child3 PlanExecutor with subplan_service and success executor") +def step_given_child3_executor_subplan_svc_and_exec(context: Context) -> None: + lcs = MagicMock() + plan = _make_plan() + lcs.get_plan.return_value = plan + lcs._commit_plan = MagicMock() + + mock_subplan_svc = MagicMock() + mock_subplan_svc.get_spawn_decisions.return_value = [] + + executor = PlanExecutor( + lifecycle_service=lcs, + subplan_service=mock_subplan_svc, + ) + context.child3_executor = executor + context.child3_lcs = lcs + context.child3_mock_subplan_svc = mock_subplan_svc + + +@given("child3 a SpawnResult with one spawned status") +def step_given_child3_spawn_result_one(context: Context) -> None: + context.child3_spawn_result = _make_spawn_result(num_statuses=1) + + +@given("child3 a SpawnResult with no spawned statuses") +def step_given_child3_spawn_result_empty(context: Context) -> None: + context.child3_spawn_result = _make_spawn_result(num_statuses=0) + + +@given("a child3 PlanExecutor for apply subplan results") +def step_given_child3_executor_for_apply(context: Context) -> None: + lcs = MagicMock() + lcs._commit_plan = MagicMock() + executor = PlanExecutor(lifecycle_service=lcs) + context.child3_executor = executor + context.child3_lcs = lcs + + +# --------------------------------------------------------------------------- +# Given: property accessors +# --------------------------------------------------------------------------- + + +@given("a child3 PlanExecutor with all optional services configured") +def step_given_child3_executor_with_all_optional(context: Context) -> None: + lcs = MagicMock() + mock_fix_orch = MagicMock() + mock_subplan_svc = MagicMock() + mock_exec_svc = MagicMock() + + executor = PlanExecutor( + lifecycle_service=lcs, + fix_revalidate_orchestrator=mock_fix_orch, + subplan_service=mock_subplan_svc, + subplan_execution_service=mock_exec_svc, + ) + context.child3_executor = executor + context.child3_mock_fix_orch = mock_fix_orch + context.child3_mock_subplan_svc = mock_subplan_svc + context.child3_mock_exec_svc = mock_exec_svc + + +# --------------------------------------------------------------------------- +# Given: checkpoint PlanError +# --------------------------------------------------------------------------- + + +@given( + "a child3 PlanExecutor with checkpoint manager and lifecycle that fails " + "on commit_plan" +) +def step_given_child3_checkpoint_planerror(context: Context) -> None: + lcs = MagicMock() + plan = _make_plan() + lcs.get_plan.return_value = plan + lcs.commit_plan.side_effect = RuntimeError("commit failed") + + mock_checkpoint_mgr = MagicMock(spec=CheckpointManager) + mock_checkpoint = MagicMock(spec=SandboxCheckpoint) + mock_checkpoint.checkpoint_id = "CKPT01" + mock_checkpoint_mgr.create_checkpoint.return_value = mock_checkpoint + + # We need the sandbox to be resolvable so we enter the try block + # Use execution_context to provide a sandbox manager + exec_ctx = MagicMock() + exec_ctx.changeset_store = MagicMock() + mock_sandbox = MagicMock() + mock_sandbox.context = MagicMock() + mock_sandbox.context.sandbox_root = "/tmp/sandbox_root" + mock_sandbox_mgr = MagicMock() + mock_sandbox_mgr.get_sandboxes.return_value = [mock_sandbox] + exec_ctx.sandbox_manager = mock_sandbox_mgr + + executor = PlanExecutor( + lifecycle_service=lcs, + checkpoint_manager=mock_checkpoint_mgr, + execution_context=exec_ctx, + ) + context.child3_executor = executor + context.child3_lcs = lcs + context.child3_checkpoint_mgr = mock_checkpoint_mgr + + +@given("child3 a sandbox that returns a valid checkpoint") +def step_given_child3_sandbox_valid_checkpoint(context: Context) -> None: + pass # Already configured in the previous step + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when("I child3 call _execute_child_plan on the executor") +def step_when_child3_call_execute_child_plan(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + status: SubplanStatus = context.child3_status + try: + context.child3_output = executor._execute_child_plan(status) + context.child3_child_error = None + except Exception as exc: + context.child3_output = None + context.child3_child_error = exc + + +@when("I child3 call _execute_subplans on the executor") +def step_when_child3_call_execute_subplans(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + spawn_result: SpawnResult = context.child3_spawn_result + plan = getattr(context, "child3_plan", None) or _make_plan() + try: + context.child3_exec_result = executor._execute_subplans(plan, spawn_result) + context.child3_child_error = None + except Exception as exc: + context.child3_exec_result = None + context.child3_child_error = exc + + +@when("I child3 call _apply_subplan_results_to_plan with exec_result None") +def step_when_child3_call_apply_subplan_results(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + spawn_result: SpawnResult = context.child3_spawn_result + plan = _make_plan() + try: + executor._apply_subplan_results_to_plan(plan, spawn_result, None) + context.child3_apply_error = None + except Exception as exc: + context.child3_apply_error = exc + context.child3_plan = plan + + +@when("I child3 attempt _try_create_checkpoint expecting PlanError") +def step_when_child3_try_create_checkpoint_planerror(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + context.child3_checkpoint_error = None + try: + executor._try_create_checkpoint( + plan_id=_PLAN_ID, + phase="pre_execute", + ) + except PlanError as exc: + context.child3_checkpoint_error = exc + except Exception as exc: + context.child3_checkpoint_error = exc + + +# --------------------------------------------------------------------------- +# Then: _execute_child_plan success +# --------------------------------------------------------------------------- + + +@then("child3 the SubplanExecutionOutput should indicate success") +def step_then_child3_output_success(context: Context) -> None: + assert context.child3_child_error is None, ( + f"Unexpected error: {context.child3_child_error}" + ) + assert context.child3_output is not None, "Expected output but got None" + assert context.child3_output.success is True, ( + f"Expected success, got: {context.child3_output}" + ) + + +@then("child3 the SubplanExecutionOutput should have a changeset_summary") +def step_then_child3_output_has_summary(context: Context) -> None: + assert context.child3_output.changeset_summary is not None, ( + "Expected changeset_summary to be set" + ) + + +@then("child3 the running_plan_ids should be empty") +def step_then_child3_running_plan_ids_empty(context: Context) -> None: + assert len(context.child3_executor._running_plan_ids) == 0, ( + f"Expected empty running_plan_ids, got: {context.child3_executor._running_plan_ids}" + ) + + +# --------------------------------------------------------------------------- +# Then: circular detection +# --------------------------------------------------------------------------- + + +@then("child3 the SubplanExecutionOutput should indicate failure") +def step_then_child3_output_failure(context: Context) -> None: + assert context.child3_output is not None, "Expected output but got None" + assert context.child3_output.success is False, ( + f"Expected failure, got: {context.child3_output}" + ) + + +@then("child3 the SubplanExecutionOutput error should mention circular") +def step_then_child3_output_error_circular(context: Context) -> None: + assert context.child3_output.error is not None, "Expected error but got None" + assert "circular" in context.child3_output.error.lower(), ( + f"Expected error to mention 'circular', got: {context.child3_output.error}" + ) + + +# --------------------------------------------------------------------------- +# Then: plan not found +# --------------------------------------------------------------------------- + + +@then('child3 the SubplanExecutionOutput error should mention "not found"') +def step_then_child3_output_error_not_found(context: Context) -> None: + assert context.child3_output.error is not None, "Expected error but got None" + assert "not found" in context.child3_output.error.lower(), ( + f"Expected error to mention 'not found', got: {context.child3_output.error}" + ) + + +# --------------------------------------------------------------------------- +# Then: strategize produces no decisions +# --------------------------------------------------------------------------- + + +@then('child3 the SubplanExecutionOutput error should mention "no decisions"') +def step_then_child3_output_error_no_decisions(context: Context) -> None: + assert context.child3_output.error is not None, "Expected error but got None" + assert "no decisions" in context.child3_output.error.lower(), ( + f"Expected error to mention 'no decisions', got: {context.child3_output.error}" + ) + + +# --------------------------------------------------------------------------- +# Then: exception during execution +# --------------------------------------------------------------------------- + + +@then("child3 the SubplanExecutionOutput error should contain the exception message") +def step_then_child3_output_error_contains_exception(context: Context) -> None: + assert context.child3_output.error is not None, "Expected error but got None" + assert "boom from start_strategize" in context.child3_output.error, ( + f"Expected error to contain exception message, got: {context.child3_output.error}" + ) + + +# --------------------------------------------------------------------------- +# Then: _execute_subplans +# --------------------------------------------------------------------------- + + +@then("child3 the SubplanExecutionResult should not be None") +def step_then_child3_exec_result_not_none(context: Context) -> None: + assert context.child3_child_error is None, ( + f"Unexpected error: {context.child3_child_error}" + ) + assert context.child3_exec_result is not None, ( + "Expected SubplanExecutionResult but got None" + ) + + +@then("child3 the SubplanExecutionResult should be None") +def step_then_child3_exec_result_none(context: Context) -> None: + assert context.child3_child_error is None, ( + f"Unexpected error: {context.child3_child_error}" + ) + assert context.child3_exec_result is None, ( + f"Expected None but got: {context.child3_exec_result}" + ) + + +# --------------------------------------------------------------------------- +# Then: _apply_subplan_results +# --------------------------------------------------------------------------- + + +@then("child3 the plan subplan_statuses should match the spawn result statuses") +def step_then_child3_plan_statuses_match_spawn(context: Context) -> None: + assert context.child3_apply_error is None, ( + f"Unexpected error: {context.child3_apply_error}" + ) + plan = context.child3_plan + spawn_result: SpawnResult = context.child3_spawn_result + assert plan.subplan_statuses is not None, "Expected subplan_statuses to be set" + assert len(plan.subplan_statuses) == len(spawn_result.spawned_statuses), ( + f"Expected {len(spawn_result.spawned_statuses)} statuses, " + f"got {len(plan.subplan_statuses)}" + ) + + +# --------------------------------------------------------------------------- +# Then: property accessors +# --------------------------------------------------------------------------- + + +@then( + "child3 the fix_revalidate_orchestrator property should return the " + "configured instance" +) +def step_then_child3_fix_orch_property(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + result = executor.fix_revalidate_orchestrator + assert result is context.child3_mock_fix_orch, ( + f"Expected {context.child3_mock_fix_orch}, got {result}" + ) + + +@then("child3 the subplan_service property should return the configured instance") +def step_then_child3_subplan_svc_property(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + result = executor.subplan_service + assert result is context.child3_mock_subplan_svc, ( + f"Expected {context.child3_mock_subplan_svc}, got {result}" + ) + + +@then( + "child3 the subplan_execution_service property should return the " + "configured instance" +) +def step_then_child3_exec_svc_property(context: Context) -> None: + executor: PlanExecutor = context.child3_executor + result = executor.subplan_execution_service + assert result is context.child3_mock_exec_svc, ( + f"Expected {context.child3_mock_exec_svc}, got {result}" + ) + + +# --------------------------------------------------------------------------- +# Then: checkpoint PlanError +# --------------------------------------------------------------------------- + + +@then("a child3 PlanError should be raised about persisting checkpoint metadata") +def step_then_child3_checkpoint_planerror(context: Context) -> None: + assert context.child3_checkpoint_error is not None, ( + "Expected PlanError but none raised" + ) + assert isinstance(context.child3_checkpoint_error, PlanError), ( + f"Expected PlanError, got: {type(context.child3_checkpoint_error)}" + ) + assert "checkpoint" in str(context.child3_checkpoint_error).lower(), ( + f"Expected error about checkpoints, got: {context.child3_checkpoint_error}" + ) diff --git a/src/cleveragents/application/services/plan_executor.py b/src/cleveragents/application/services/plan_executor.py index 3acfff682..335dc3d69 100644 --- a/src/cleveragents/application/services/plan_executor.py +++ b/src/cleveragents/application/services/plan_executor.py @@ -48,6 +48,11 @@ from cleveragents.application.services.resource_registry_service import ( ResourceRegistryService, ) from cleveragents.application.services.strategy_models import StrategyTree +from cleveragents.application.services.subplan_execution_service import ( + SubplanExecutionOutput, + SubplanExecutionResult, + SubplanExecutionService, +) from cleveragents.core.exceptions import PlanError, ValidationError from cleveragents.domain.models.core.change import ChangeSetStore from cleveragents.domain.models.core.estimation import EstimationResult @@ -75,14 +80,11 @@ if TYPE_CHECKING: from cleveragents.application.services.error_recovery_service import ( ErrorRecoveryService, ) - from cleveragents.application.services.subplan_execution_service import ( - SubplanExecutionResult, - SubplanExecutionService, - ) from cleveragents.application.services.subplan_service import ( SpawnResult, SubplanService, ) + from cleveragents.domain.models.core.plan import SubplanStatus from cleveragents.infrastructure.observability.metrics_emitter import ( MetricsEmitter, ) @@ -408,6 +410,7 @@ class PlanExecutor: self._resource_registry = resource_registry self._strategize_actor = strategize_actor or StrategizeStubActor() self._execute_actor = execute_actor or ExecuteStubActor() + self._running_plan_ids: set[str] = set() self._logger = logger.bind(service="plan_executor") # M1 fix: track which plan IDs have already been hydrated by this # executor instance. Using a per-plan-id set prevents cross-plan @@ -537,6 +540,12 @@ class PlanExecutor: sequential and parallel execution groups as configured on the parent plan's :class:`SubplanConfig`. + When ``_subplan_execution_service`` is ``None`` but + ``_subplan_service`` is available, a ``SubplanExecutionService`` + is created for this invocation from the parent plan's config and + the ``_execute_child_plan`` callback so that child plan execution + works in the production CLI path (Forgejo #10268). + Args: plan: The parent plan domain object. spawn_result: The result returned by :meth:`_spawn_subplans`. @@ -545,15 +554,23 @@ class PlanExecutor: A :class:`SubplanExecutionResult` if execution was attempted, or ``None`` when no execution service is configured. """ - if self._subplan_execution_service is None: - return None + exec_svc = self._subplan_execution_service + if exec_svc is None: + if self._subplan_service is None: + return None + + config = getattr(plan, "subplan_config", None) or SubplanConfig() + exec_svc = SubplanExecutionService( + config=config, + executor_fn=self._execute_child_plan, + ) plan_id: str = plan.identity.plan_id statuses = spawn_result.spawned_statuses if not statuses: return None - exec_result = self._subplan_execution_service.execute_all( + exec_result = exec_svc.execute_all( subplan_statuses=statuses, base_files={}, ) @@ -606,6 +623,83 @@ class PlanExecutor: plan_id=plan.identity.plan_id, ) + def _execute_child_plan( + self, + status: SubplanStatus, + ) -> SubplanExecutionOutput: + """Execute a single child subplan through strategize + execute phases. + + Registered as the ``executor_fn`` callback on + :class:`SubplanExecutionService` so that child plans spawned during + the Execute phase of the parent plan are actually run rather than + being silently skipped (Forgejo #10268). + + Args: + status: The ``SubplanStatus`` for the child plan, carrying + its ``subplan_id`` and ``action_name``. + + Returns: + A ``SubplanExecutionOutput`` indicating success or failure + along with any produced files and a summary. + """ + subplan_id: str = status.subplan_id + if subplan_id in self._running_plan_ids: + return SubplanExecutionOutput( + subplan_id=subplan_id, + success=False, + error=( + "Circular or re-entrant subplan execution" + f" detected for {subplan_id}" + ), + ) + self._running_plan_ids.add(subplan_id) + try: + plan = self._lifecycle.get_plan(subplan_id) + if plan is None: + return SubplanExecutionOutput( + subplan_id=subplan_id, + success=False, + error=f"Child plan {subplan_id} not found in lifecycle service", + ) + + strategize_result = self.run_strategize(subplan_id) + if ( + not strategize_result.decision_root_id + or not strategize_result.decisions + ): + return SubplanExecutionOutput( + subplan_id=subplan_id, + success=False, + error=f"Child plan {subplan_id} strategize produced no decisions", + ) + + execute_result = self.run_execute(subplan_id) + if isinstance(execute_result, RuntimeExecuteResult): + files_changed_count = execute_result.tool_call_count + else: + files_changed_count = execute_result.tool_calls_count + + return SubplanExecutionOutput( + subplan_id=subplan_id, + success=True, + files={}, + files_changed=files_changed_count, + changeset_summary=f"Child plan {subplan_id[:8]} executed", + ) + except Exception as exc: + self._logger.warning( + "Child plan execution failed", + subplan_id=subplan_id, + error=str(exc), + ) + return SubplanExecutionOutput( + subplan_id=subplan_id, + success=False, + error=str(exc), + ) + finally: + self._running_plan_ids.discard(subplan_id) + def _is_auto_trigger_active(self, trigger: str) -> bool: """Return True if the given automatic checkpoint trigger is active. diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 7e20c37d0..44fe96707 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -1400,6 +1400,7 @@ def _get_plan_executor( resource_registry=container.resource_registry_service(), ) + subplan_service = container.subplan_service() checkpoint_manager = container.checkpoint_manager() return PlanExecutor( @@ -1411,6 +1412,7 @@ def _get_plan_executor( tier_service=container.context_tier_service(), project_repository=container.namespaced_project_repo(), resource_registry=container.resource_registry_service(), + subplan_service=subplan_service, )