forked from HAL9000/cleveragents-core
774dfedc6b
## Summary This PR adds TDD bug-capture tests for bug #967 — `plan execute` only transitions state without running strategize or execute phase processing. ### Motivation Bug #967 describes that the `plan execute` CLI command originally only called `service.execute_plan(plan_id)`, which is a state transition only (Strategize/COMPLETE → Execute/QUEUED). When a plan was in Strategize/QUEUED state (immediately after `plan use`), the command failed because `execute_plan()` requires Strategize/COMPLETE. The CLI should detect the plan's current phase and run `PlanExecutor.run_strategize()` before transitioning. Per the project's TDD Bug Fix Workflow (`CONTRIBUTING.md`), the first step in fixing any bug is to write a test that captures the buggy behavior. Since the fix for #967 is already present in the codebase (the CLI handler already orchestrates properly), the `@tdd_expected_fail` tags have been removed and these tests serve as **permanent regression guards** ensuring the fix is never reverted. ### Design Approach **All `@tdd_expected_fail` scenarios were rewritten to exercise the CLI orchestration layer** — the actual code path affected by bug #967. This satisfies AC4: "The test is specific enough that it will pass normally (without the tag) only when the bug is genuinely fixed." - **Scenarios 1, 2, 4** (previously `@tdd_expected_fail`): Use Typer's `CliRunner` with mocked services to invoke the `plan execute` CLI command handler directly. This tests the orchestration logic in `plan.py` — the exact code that was buggy. - **Scenario 3** (positive control): Uses real `PlanLifecycleService` (in-memory) and `PlanExecutor` (stub actors) to demonstrate that proper service-level orchestration works. - **Robot tests**: Replicate the CLI orchestration logic using real services to verify at the integration level. ### Changes #### Behave Unit Tests - `features/tdd_plan_execute_phase_processing.feature` — 4 scenarios - `features/steps/tdd_plan_execute_phase_processing_steps.py` — Step definitions using CliRunner and mocked services **Scenarios:** 1. **CLI execute command handles plan in Strategize/QUEUED state** — Invokes `plan execute` via CliRunner on a QUEUED plan. Verifies the CLI succeeds and the plan reaches Execute phase. 2. **CLI execute command orchestrates full lifecycle for QUEUED plan** — Verifies `run_strategize()` and `run_execute()` are both called by the CLI handler. 3. **Positive control — proper orchestration transitions QUEUED plan to Execute** — Demonstrates that `run_strategize()` → `execute_plan()` works correctly at the service level. Always passes. 4. **CLI auto-discovery finds plans in Strategize/QUEUED state** — Invokes `plan execute` with no plan_id. Verifies the auto-discovery filter includes QUEUED plans. #### Robot Integration Tests - `robot/tdd_plan_execute_phase_processing.robot` — 4 test cases matching the Behave scenarios - `robot/helper_tdd_plan_execute_phase_processing.py` — Helper script replicating CLI orchestration logic #### CHANGELOG - `CHANGELOG.md` — Added entry under "## Unreleased" describing the new tests. ### Quality Gates - `nox -e lint`: ✅ passed - `nox -e typecheck`: ✅ passed (0 errors) - `nox -e unit_tests`: ✅ passed (391 features, 11177 scenarios, 0 failures) - `nox -e integration_tests`: ✅ passed (1572 tests, 0 failures) - `nox -e e2e_tests`: ✅ passed (16 tests, 0 failures) - `nox -e coverage_report`: ✅ 97% coverage ### Review Cycle 2 Fixes - **Critical #1**: Rewrote `@tdd_expected_fail` scenarios to exercise the CLI orchestration layer via CliRunner instead of testing service/executor APIs that are correct by design. Removed `@tdd_expected_fail` tags since the bug fix is already in the codebase. - **Major #2**: Added CHANGELOG entry. - **Major #3**: Rebased onto current `master`. - **Minor #4**: Added `ProcessingState.QUEUED` assertion to positive control scenario. - **Minor #5**: Narrowed exception handler from bare `except Exception` to `except (PlanError, PlanNotReadyError)`. - **Minor #7**: Changed Robot suite to use `Setup Test Environment With Database Isolation`. - **Minor #8**: Added `on_timeout=kill` to all Robot `Run Process` calls. - **Minor #12**: Added docstring to `_fail()` helper. - **Minor #13**: Removed redundant `Settings()` instantiation. ### Known Limitations - The `@tdd_expected_fail` tags were removed because the bug fix for #967 is already in the codebase. If this PR is merged before the #967 fix PR, the tags would need to be re-added. However, the CHANGELOG and existing CLI code confirm the fix is already on `master`. Closes #977 Reviewed-on: cleveragents/cleveragents-core#1050 Co-authored-by: Rui Hu <rui.hu@cleverthis.com> Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
392 lines
15 KiB
Python
392 lines
15 KiB
Python
"""Step definitions for TDD Bug #967 — plan execute phase processing.
|
|
|
|
These steps exercise the **CLI orchestration layer** (the ``execute_plan``
|
|
command handler in ``plan.py``) via Typer's ``CliRunner`` to verify that
|
|
the ``plan execute`` command correctly handles plans in Strategize/QUEUED
|
|
state.
|
|
|
|
Bug #967: The ``plan execute`` CLI command originally only called
|
|
``service.execute_plan(plan_id)``, which is a state transition only
|
|
(Strategize/COMPLETE → Execute/QUEUED). It did not construct a
|
|
``PlanExecutor`` or call ``run_strategize()`` / ``run_execute()``. When a
|
|
plan was in Strategize/QUEUED state (immediately after ``plan use``), the
|
|
command failed because ``execute_plan()`` requires Strategize/COMPLETE.
|
|
|
|
The fix added phase-aware orchestration to the CLI handler: when a plan is
|
|
in Strategize/QUEUED, it calls ``PlanExecutor.run_strategize()`` before
|
|
transitioning. The auto-discovery filter was also updated to include
|
|
Strategize/QUEUED plans.
|
|
|
|
Scenarios 1, 2, and 4 exercise the CLI handler (the code path that was
|
|
fixed). Since the fix is already in the codebase, the ``@tdd_expected_fail``
|
|
tag has been removed and these tests serve as permanent regression guards.
|
|
|
|
The "positive control" scenario (Scenario 3) demonstrates that the proper
|
|
service-level orchestration path (``run_strategize`` → ``execute_plan``)
|
|
works correctly.
|
|
|
|
This test was written to capture bug #967 per ticket #977.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from datetime import datetime
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
from behave import given, then, when
|
|
from behave.runner import Context
|
|
from typer.testing import CliRunner
|
|
|
|
from cleveragents.application.services.plan_executor import PlanExecutor
|
|
from cleveragents.application.services.plan_lifecycle_service import (
|
|
PlanLifecycleService,
|
|
PlanNotReadyError,
|
|
)
|
|
from cleveragents.cli.commands.plan import app as plan_app
|
|
from cleveragents.config.settings import Settings
|
|
from cleveragents.core.exceptions import PlanError
|
|
from cleveragents.domain.models.core.plan import (
|
|
NamespacedName,
|
|
Plan,
|
|
PlanIdentity,
|
|
PlanPhase,
|
|
PlanTimestamps,
|
|
ProcessingState,
|
|
ProjectLink,
|
|
)
|
|
|
|
|
|
def _make_cli_plan(
|
|
*,
|
|
plan_id: str = "01ARZ3NDEKTSV4RRFFQ69G5967",
|
|
phase: PlanPhase = PlanPhase.STRATEGIZE,
|
|
state: ProcessingState = ProcessingState.QUEUED,
|
|
) -> Plan:
|
|
"""Build a real ``Plan`` domain object for CLI tests.
|
|
|
|
Uses the Plan domain model directly (not a MagicMock) so that the CLI
|
|
handler's ``isinstance`` checks and attribute accesses work correctly.
|
|
"""
|
|
return Plan(
|
|
identity=PlanIdentity(plan_id=plan_id),
|
|
namespaced_name=NamespacedName(namespace="local", name="tdd-967-plan"),
|
|
action_name="local/tdd-967-action",
|
|
description="TDD plan for bug #967",
|
|
phase=phase,
|
|
processing_state=state,
|
|
project_links=[ProjectLink(project_name="proj-1")],
|
|
strategy_actor="openai/gpt-4",
|
|
execution_actor="openai/gpt-4",
|
|
timestamps=PlanTimestamps(created_at=datetime.now(), updated_at=datetime.now()),
|
|
)
|
|
|
|
|
|
def _create_service_and_queued_plan() -> tuple[PlanLifecycleService, str]:
|
|
"""Create a real in-memory PlanLifecycleService and a plan in Strategize/QUEUED.
|
|
|
|
Returns a tuple of (service, plan_id). The plan is created via the normal
|
|
``create_action`` → ``use_action`` flow, which puts the plan in
|
|
Strategize/QUEUED — the state that triggers bug #967.
|
|
"""
|
|
settings: Settings = Settings()
|
|
service: PlanLifecycleService = PlanLifecycleService(settings=settings)
|
|
action = service.create_action(
|
|
name="local/tdd-967-action",
|
|
description="TDD action for bug #967",
|
|
definition_of_done="Complete the test plan",
|
|
strategy_actor="local/stub-strategize",
|
|
execution_actor="local/stub-execute",
|
|
)
|
|
plan: Plan = service.use_action(action_name=str(action.namespaced_name))
|
|
return service, plan.identity.plan_id
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI-level test setup (used by Scenarios 1, 2, 4)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_PLAN_ID: str = "01ARZ3NDEKTSV4RRFFQ69G5967"
|
|
|
|
|
|
@given("a CLI runner and mocked services for bug 967")
|
|
def step_cli_runner_and_mocked_services(context: Context) -> None:
|
|
"""Set up a CliRunner and mock service/executor for CLI-level testing."""
|
|
context.runner_967 = CliRunner()
|
|
context.mock_service_967 = MagicMock()
|
|
context.mock_executor_967 = MagicMock()
|
|
|
|
|
|
@given("a plan in Strategize/QUEUED state for bug 967")
|
|
def step_plan_in_strategize_queued(context: Context) -> None:
|
|
"""Configure the mocked service to return a plan in Strategize/QUEUED.
|
|
|
|
The mock ``get_plan`` returns the QUEUED plan for the initial checks,
|
|
then returns an Execute/QUEUED plan after strategize runs (simulating
|
|
auto_progress), and finally an Execute/COMPLETE plan after run_execute.
|
|
"""
|
|
queued_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.STRATEGIZE,
|
|
state=ProcessingState.QUEUED,
|
|
)
|
|
execute_queued_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.EXECUTE,
|
|
state=ProcessingState.QUEUED,
|
|
)
|
|
execute_complete_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.EXECUTE,
|
|
state=ProcessingState.COMPLETE,
|
|
)
|
|
|
|
# get_plan call sequence in the CLI execute_plan handler:
|
|
# 1. pre_plan (read-only check)
|
|
# 2. current_plan (phase detection)
|
|
# 3. re-fetch after run_strategize (auto_progress moved to Execute)
|
|
# 4. re-fetch for inline execute check
|
|
# 5. re-fetch after run_execute
|
|
context.mock_service_967.get_plan.side_effect = [
|
|
queued_plan,
|
|
queued_plan,
|
|
execute_queued_plan,
|
|
execute_queued_plan,
|
|
execute_complete_plan,
|
|
]
|
|
|
|
context.queued_plan_967 = queued_plan
|
|
context.execute_complete_plan_967 = execute_complete_plan
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Scenario 1: CLI execute command handles plan in Strategize/QUEUED state
|
|
# Scenario 2: CLI execute command orchestrates full lifecycle for QUEUED plan
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when("I invoke the plan execute CLI command for the QUEUED plan for bug 967")
|
|
def step_invoke_cli_execute(context: Context) -> None:
|
|
"""Invoke the ``plan execute`` CLI command via CliRunner.
|
|
|
|
This exercises the CLI orchestration layer — the actual code path that
|
|
was buggy in #967. The handler should detect Strategize/QUEUED and
|
|
call ``run_strategize`` before transitioning to Execute.
|
|
"""
|
|
with (
|
|
patch(
|
|
"cleveragents.cli.commands.plan._get_lifecycle_service",
|
|
return_value=context.mock_service_967,
|
|
),
|
|
patch(
|
|
"cleveragents.cli.commands.plan._get_plan_executor",
|
|
return_value=context.mock_executor_967,
|
|
),
|
|
):
|
|
context.cli_result_967 = context.runner_967.invoke(
|
|
plan_app, ["execute", _PLAN_ID]
|
|
)
|
|
|
|
|
|
@then("the CLI should succeed and the plan should reach Execute phase for bug 967")
|
|
def step_cli_succeed_execute_phase(context: Context) -> None:
|
|
"""Assert the CLI command completed without error.
|
|
|
|
Bug #967 regression guard: if the CLI orchestration is broken, this
|
|
assertion fails because the command would abort with an error.
|
|
"""
|
|
result = context.cli_result_967
|
|
assert result.exit_code == 0, (
|
|
f"Bug #967: CLI 'plan execute' failed with exit code {result.exit_code}. "
|
|
f"The CLI should handle Strategize/QUEUED plans by running "
|
|
f"run_strategize() before transitioning. Output:\n{result.output}"
|
|
)
|
|
assert "Execute" in result.output or "execute" in result.output, (
|
|
f"Bug #967: CLI output does not mention Execute phase. Output:\n{result.output}"
|
|
)
|
|
|
|
|
|
@then("the executor should have run strategize for the plan for bug 967")
|
|
def step_executor_ran_strategize(context: Context) -> None:
|
|
"""Assert the CLI handler invoked run_strategize on the executor.
|
|
|
|
Bug #967 regression guard: the CLI handler must call
|
|
``executor.run_strategize(plan_id)`` for plans in Strategize/QUEUED.
|
|
"""
|
|
context.mock_executor_967.run_strategize.assert_called_once_with(_PLAN_ID)
|
|
|
|
|
|
@then("the plan should have completed execute phase processing via CLI for bug 967")
|
|
def step_plan_completed_execute_via_cli(context: Context) -> None:
|
|
"""Assert the CLI handler also ran the execute phase.
|
|
|
|
Bug #967 regression guard: after strategize, the CLI should run
|
|
``executor.run_execute(plan_id)`` to complete the Execute phase.
|
|
"""
|
|
context.mock_executor_967.run_execute.assert_called_once_with(_PLAN_ID)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Scenario 3: Positive control — proper orchestration works
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@given("a real plan executor with a plan in Strategize/QUEUED for bug 967")
|
|
def step_real_executor_queued_plan(context: Context) -> None:
|
|
"""Set up a real PlanExecutor and PlanLifecycleService with a QUEUED plan."""
|
|
service, plan_id = _create_service_and_queued_plan()
|
|
context.service_967 = service
|
|
context.plan_id_967 = plan_id
|
|
context.executor_967 = PlanExecutor(lifecycle_service=service)
|
|
|
|
|
|
@when("I run the proper orchestration of strategize then execute for bug 967")
|
|
def step_proper_orchestration(context: Context) -> None:
|
|
"""Run the correct orchestration: run_strategize → execute_plan → verify.
|
|
|
|
This positive control demonstrates that when the caller properly
|
|
orchestrates the lifecycle (as the fixed CLI does), a QUEUED plan
|
|
can be taken through to Execute phase.
|
|
"""
|
|
service: PlanLifecycleService = context.service_967
|
|
executor: PlanExecutor = context.executor_967
|
|
plan_id: str = context.plan_id_967
|
|
context.orchestration_error_967 = None
|
|
context.orchestrated_plan_967 = None
|
|
|
|
try:
|
|
# Step 1: Run strategize to completion (QUEUED → PROCESSING → COMPLETE)
|
|
executor.run_strategize(plan_id)
|
|
|
|
# Step 2: Re-fetch the plan — auto_progress in complete_strategize
|
|
# may have already advanced the plan to Execute phase.
|
|
plan: Plan = service.get_plan(plan_id)
|
|
|
|
if plan.phase == PlanPhase.EXECUTE:
|
|
# auto_progress already moved it
|
|
context.orchestrated_plan_967 = plan
|
|
elif (
|
|
plan.phase == PlanPhase.STRATEGIZE
|
|
and plan.state == ProcessingState.COMPLETE
|
|
):
|
|
# Transition to Execute phase
|
|
context.orchestrated_plan_967 = service.execute_plan(plan_id)
|
|
else:
|
|
context.orchestration_error_967 = PlanError(
|
|
f"Unexpected plan state after strategize: "
|
|
f"{plan.phase.value}/{plan.state.value}"
|
|
)
|
|
except (PlanError, PlanNotReadyError) as exc:
|
|
context.orchestration_error_967 = exc
|
|
|
|
|
|
@then("the plan should be in Execute/QUEUED state via orchestration for bug 967")
|
|
def step_plan_in_execute_via_orchestration(context: Context) -> None:
|
|
"""Assert the properly orchestrated plan reached Execute phase with QUEUED state.
|
|
|
|
This is a positive control: when the orchestration is done correctly,
|
|
the plan should be in Execute phase. This scenario is NOT tagged
|
|
``@tdd_expected_fail`` and must always pass.
|
|
"""
|
|
if context.orchestration_error_967 is not None:
|
|
raise AssertionError(
|
|
f"Proper orchestration failed unexpectedly: "
|
|
f"{context.orchestration_error_967}"
|
|
)
|
|
plan: Plan | None = context.orchestrated_plan_967
|
|
assert plan is not None, "Orchestration returned no plan"
|
|
assert plan.phase == PlanPhase.EXECUTE, (
|
|
f"Expected Execute phase, got {plan.phase.value}"
|
|
)
|
|
assert plan.state == ProcessingState.QUEUED, (
|
|
f"Expected QUEUED state, got {plan.state.value}"
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Scenario 4: CLI auto-discovery finds plans in Strategize/QUEUED state
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@given(
|
|
"a single plan in Strategize/QUEUED state eligible for auto-discovery for bug 967"
|
|
)
|
|
def step_single_queued_plan_auto_discovery(context: Context) -> None:
|
|
"""Configure the mocked service for auto-discovery with a QUEUED plan.
|
|
|
|
The CLI auto-discovery (no plan_id) calls ``service.list_plans()`` and
|
|
filters for eligible plans. Bug #967 had a filter that only accepted
|
|
Strategize/COMPLETE, missing QUEUED plans. The fix includes QUEUED.
|
|
"""
|
|
queued_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.STRATEGIZE,
|
|
state=ProcessingState.QUEUED,
|
|
)
|
|
execute_queued_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.EXECUTE,
|
|
state=ProcessingState.QUEUED,
|
|
)
|
|
execute_complete_plan: Plan = _make_cli_plan(
|
|
plan_id=_PLAN_ID,
|
|
phase=PlanPhase.EXECUTE,
|
|
state=ProcessingState.COMPLETE,
|
|
)
|
|
|
|
# list_plans is called twice: once for STRATEGIZE, once for EXECUTE
|
|
context.mock_service_967.list_plans.side_effect = [
|
|
[queued_plan], # Strategize phase plans
|
|
[], # Execute phase plans (none)
|
|
]
|
|
|
|
# get_plan call sequence after auto-discovery selects the plan:
|
|
# 1. pre_plan (read-only check)
|
|
# 2. current_plan (phase detection)
|
|
# 3. re-fetch after run_strategize
|
|
# 4. re-fetch for inline execute check
|
|
# 5. re-fetch after run_execute
|
|
context.mock_service_967.get_plan.side_effect = [
|
|
queued_plan,
|
|
queued_plan,
|
|
execute_queued_plan,
|
|
execute_queued_plan,
|
|
execute_complete_plan,
|
|
]
|
|
|
|
|
|
@when("I invoke the plan execute CLI command without a plan id for bug 967")
|
|
def step_invoke_cli_execute_no_plan_id(context: Context) -> None:
|
|
"""Invoke ``plan execute`` without a plan ID to trigger auto-discovery.
|
|
|
|
Bug #967 regression guard: the CLI auto-discovery filter must include
|
|
Strategize/QUEUED plans, not just Strategize/COMPLETE.
|
|
"""
|
|
with (
|
|
patch(
|
|
"cleveragents.cli.commands.plan._get_lifecycle_service",
|
|
return_value=context.mock_service_967,
|
|
),
|
|
patch(
|
|
"cleveragents.cli.commands.plan._get_plan_executor",
|
|
return_value=context.mock_executor_967,
|
|
),
|
|
):
|
|
context.cli_result_967 = context.runner_967.invoke(plan_app, ["execute"])
|
|
|
|
|
|
@then("the CLI should succeed and auto-discover the QUEUED plan for bug 967")
|
|
def step_cli_auto_discovery_succeeds(context: Context) -> None:
|
|
"""Assert the CLI auto-discovered and processed the Strategize/QUEUED plan.
|
|
|
|
Bug #967 regression guard: if the auto-discovery filter excludes QUEUED
|
|
plans, the CLI would abort with "No plans ready for execution."
|
|
"""
|
|
result = context.cli_result_967
|
|
assert result.exit_code == 0, (
|
|
"Bug #967: CLI 'plan execute' (no plan_id) failed with exit code "
|
|
f"{result.exit_code}. The auto-discovery filter should include "
|
|
f"Strategize/QUEUED plans. Output:\n{result.output}"
|
|
)
|
|
# Verify the executor was actually invoked (plan was found and processed)
|
|
context.mock_executor_967.run_strategize.assert_called_once_with(_PLAN_ID)
|