diff --git a/CHANGELOG.md b/CHANGELOG.md index 76e7a388e..6aa1585bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +- Added TDD bug-capture tests for #969 — `plan correct` expects `decision_id` + but M3 acceptance test passes `plan_id`. Behave BDD scenarios (revert and + append modes) and Robot Framework integration tests verify that + `request_correction` is called with the root decision ID when a plan_id is + given as the first positional argument. Tests use `@tdd_expected_fail` until + the bug fix is merged. Shared mock fixtures extracted to + `features/mocks/tdd_plan_correct_plan_id_fixtures.py`. (#979) - Added TDD bug-capture tests for bug #968: ``plan explain`` expects a decision_id but the M3 acceptance test passes a plan_id. Two Behave BDD scenarios (``@tdd_bug @tdd_bug_968 @tdd_expected_fail``) verify the fixed diff --git a/features/mocks/tdd_plan_correct_plan_id_fixtures.py b/features/mocks/tdd_plan_correct_plan_id_fixtures.py new file mode 100644 index 000000000..72ef0b037 --- /dev/null +++ b/features/mocks/tdd_plan_correct_plan_id_fixtures.py @@ -0,0 +1,146 @@ +"""Shared mock fixtures for TDD plan-correct plan_id tests. + +Provides constants, mock builders, and CLI argument helpers used by both +the Behave step definitions +(``features/steps/tdd_plan_correct_plan_id_steps.py``) and the Robot +Framework integration test helper +(``robot/helper_tdd_plan_correct_plan_id.py``). + +Centralising the mock builders eliminates duplication and ensures both +test suites exercise the ``plan correct`` CLI path with identically-shaped +mock objects. + +Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/969 +TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/979 +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock + +# --------------------------------------------------------------------------- +# Patch targets +# --------------------------------------------------------------------------- + +PATCH_CONTAINER: str = "cleveragents.application.container.get_container" +PATCH_CORRECTION_SVC: str = ( + "cleveragents.application.services.correction_service.CorrectionService" +) +PATCH_RESOLVE_PLAN: str = "cleveragents.cli.commands.plan._resolve_active_plan_id" + +# --------------------------------------------------------------------------- +# Fixed identifiers for deterministic assertions +# --------------------------------------------------------------------------- + +# The plan_id is what the user passes as the first positional argument. +PLAN_ID: str = "01JBG969PLANID0000000000" +# The root_decision_id is what the code SHOULD resolve to when it detects +# the positional argument is a plan_id (not a decision_id). +ROOT_DECISION_ID: str = "DEC-ROOT-969" +# Used to build a multi-node decision tree for realism; not directly asserted. +CHILD_DECISION_ID: str = "DEC-CHILD-969" + + +# --------------------------------------------------------------------------- +# Mock builders +# --------------------------------------------------------------------------- + + +def make_decision_ns( + decision_id: str, + parent_decision_id: str | None, +) -> SimpleNamespace: + """Create a minimal decision-like namespace for list_decisions.""" + return SimpleNamespace( + decision_id=decision_id, + parent_decision_id=parent_decision_id, + ) + + +def make_mock_container( + decisions: list[SimpleNamespace], + influence_edges: dict[str, list[str]], +) -> MagicMock: + """Build a mock DI container returning a DecisionService.""" + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = decisions + mock_decision_svc.get_influence_edges.return_value = influence_edges + mock_container = MagicMock() + mock_container.decision_service.return_value = mock_decision_svc + return mock_container + + +def make_correction_svc( + target_decision_id: str, + mode: str = "revert", +) -> MagicMock: + """Build a mock CorrectionService that succeeds for a given target. + + Args: + target_decision_id: The decision ID expected in the correction. + mode: The correction mode (``"revert"`` or ``"append"``). + """ + svc = MagicMock() + svc.request_correction.return_value = SimpleNamespace( + correction_id="CORR-TDD-969", + mode=SimpleNamespace(value=mode), + target_decision_id=target_decision_id, + guidance="Use SQLAlchemy ORM instead of raw SQL", + ) + svc.execute_correction.return_value = SimpleNamespace( + correction_id="CORR-TDD-969", + status=SimpleNamespace(value="applied"), + reverted_decisions=[target_decision_id], + new_decisions=[], + ) + return svc + + +def make_default_decisions() -> list[SimpleNamespace]: + """Build the standard two-node decision tree (root + child).""" + return [ + make_decision_ns(ROOT_DECISION_ID, None), + make_decision_ns(CHILD_DECISION_ID, ROOT_DECISION_ID), + ] + + +def make_default_container() -> MagicMock: + """Build a mock container with the standard decision tree.""" + return make_mock_container(make_default_decisions(), {}) + + +def build_cli_args(plan_id: str, mode: str = "revert") -> list[str]: + """Build the CLI argument list for ``plan correct ``. + + Args: + plan_id: The plan ID to pass as the first positional argument. + mode: The correction mode (``"revert"`` or ``"append"``). + """ + return [ + "correct", + plan_id, + "--mode", + mode, + "--guidance", + "Use SQLAlchemy ORM instead of raw SQL", + "--yes", + "--format", + "plain", + ] + + +__all__: list[str] = [ + "CHILD_DECISION_ID", + "PATCH_CONTAINER", + "PATCH_CORRECTION_SVC", + "PATCH_RESOLVE_PLAN", + "PLAN_ID", + "ROOT_DECISION_ID", + "build_cli_args", + "make_correction_svc", + "make_decision_ns", + "make_default_container", + "make_default_decisions", + "make_mock_container", +] diff --git a/features/steps/tdd_plan_correct_plan_id_steps.py b/features/steps/tdd_plan_correct_plan_id_steps.py new file mode 100644 index 000000000..ab6dc7c67 --- /dev/null +++ b/features/steps/tdd_plan_correct_plan_id_steps.py @@ -0,0 +1,190 @@ +"""Step definitions for tdd_plan_correct_plan_id.feature. + +Captures bug #969: the ``plan correct`` CLI command declares its first +positional argument as ``decision_id``. When the M3 acceptance test +passes a plan_id, the plan_id is used as ``target_decision_id``, which +fails because the plan ID is not a valid decision ID. The correction +service raises an error because the targeted decision cannot be found. + +This test uses the ``@tdd_expected_fail`` tag until the fix in #969 is +merged. Once fixed, the tag will be removed and the test will run +normally as a regression guard. + +All step text uses the ``tpcpid`` prefix to avoid collisions with +other step files. + +Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/969 +TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/979 +""" + +from __future__ import annotations + +from unittest.mock import patch + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner + +from cleveragents.cli.commands.plan import app as plan_app +from features.mocks.tdd_plan_correct_plan_id_fixtures import ( + PATCH_CONTAINER, + PATCH_CORRECTION_SVC, + PATCH_RESOLVE_PLAN, + PLAN_ID, + ROOT_DECISION_ID, + build_cli_args, + make_correction_svc, + make_default_decisions, + make_mock_container, +) + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# GIVEN steps +# --------------------------------------------------------------------------- + + +@given("tpcpid a plan with a known plan_id and a root decision") +def step_tpcpid_plan_with_root(context: Context) -> None: + """Set up mock plan with a root decision and one child.""" + decisions = make_default_decisions() + influence_edges: dict[str, list[str]] = {} + context.tpcpid_mock_container = make_mock_container( + decisions, + influence_edges, + ) + context.tpcpid_plan_id = PLAN_ID + context.tpcpid_root_decision_id = ROOT_DECISION_ID + + +@given("tpcpid a CorrectionService that succeeds when targeting the root decision") +def step_tpcpid_correction_svc(context: Context) -> None: + """Set up mock CorrectionService for revert mode.""" + context.tpcpid_correction_svc = make_correction_svc(ROOT_DECISION_ID) + + +@given( + "tpcpid a CorrectionService for append mode that succeeds when" + " targeting the root decision" +) +def step_tpcpid_correction_svc_append(context: Context) -> None: + """Set up mock CorrectionService for append mode.""" + context.tpcpid_correction_svc = make_correction_svc( + ROOT_DECISION_ID, + mode="append", + ) + + +# --------------------------------------------------------------------------- +# WHEN steps +# --------------------------------------------------------------------------- + + +@when("tpcpid I invoke plan correct with the plan_id as the positional argument") +def step_tpcpid_invoke_with_plan_id(context: Context) -> None: + """Invoke ``plan correct `` with ``--mode revert``. + + This is the exact pattern used by the M3 acceptance test: + ``plan correct ${plan_id} --mode revert --guidance "..." --yes --format plain`` + + The plan_id is passed as the first positional argument (the + ``decision_id`` parameter in the current code). The bug is that + the code treats it as a decision_id directly instead of detecting + it is a plan_id and resolving the root decision. + """ + args = build_cli_args(context.tpcpid_plan_id, mode="revert") + + with ( + patch( + PATCH_CORRECTION_SVC, + return_value=context.tpcpid_correction_svc, + ), + patch( + PATCH_CONTAINER, + return_value=context.tpcpid_mock_container, + ), + patch( + PATCH_RESOLVE_PLAN, + return_value=context.tpcpid_plan_id, + ), + ): + context.tpcpid_result = runner.invoke(plan_app, args) + + +@when( + "tpcpid I invoke plan correct with the plan_id as the positional" + " argument in append mode" +) +def step_tpcpid_invoke_with_plan_id_append(context: Context) -> None: + """Invoke ``plan correct `` with ``--mode append``. + + Same bug path as revert mode — ``target_decision_id`` resolution + at ``plan.py`` ``correct_decision`` happens **before** the + mode-specific branching, so both modes are affected by bug #969. + """ + args = build_cli_args(context.tpcpid_plan_id, mode="append") + + with ( + patch( + PATCH_CORRECTION_SVC, + return_value=context.tpcpid_correction_svc, + ), + patch( + PATCH_CONTAINER, + return_value=context.tpcpid_mock_container, + ), + patch( + PATCH_RESOLVE_PLAN, + return_value=context.tpcpid_plan_id, + ), + ): + context.tpcpid_result = runner.invoke(plan_app, args) + + +# --------------------------------------------------------------------------- +# THEN steps +# --------------------------------------------------------------------------- + + +@then("tpcpid the command should exit successfully") +def step_tpcpid_exit_ok(context: Context) -> None: + """Assert plan correct exits with code 0. + + NOTE: This assertion is expected to **pass** even while bug #969 + is present — MagicMock accepts any arguments and the CLI command + runs to completion. The ``@tdd_expected_fail`` inversion only + triggers on the *second* assertion (target_decision_id check) + which captures the actual bug. + """ + result = context.tpcpid_result + assert result.exit_code == 0, ( + f"Expected exit code 0, got {result.exit_code}. Output: {result.output}" + ) + + +@then( + "tpcpid request_correction should have targeted the root decision not the plan_id" +) +def step_tpcpid_target_is_root(context: Context) -> None: + """Assert request_correction was called with the root decision ID. + + This is the core assertion that captures bug #969: + - Currently the code passes the plan_id as target_decision_id. + - The fix should resolve the plan_id to its root decision and pass + the root decision_id as target_decision_id. + """ + svc = context.tpcpid_correction_svc + svc.request_correction.assert_called_once() + call_record = svc.request_correction.call_args + kw = call_record.kwargs + + actual_target = kw.get("target_decision_id") + assert actual_target == ROOT_DECISION_ID, ( + f"Bug #969: request_correction was called with " + f"target_decision_id={actual_target!r} but expected " + f"{ROOT_DECISION_ID!r}. The plan_id ({PLAN_ID!r}) was " + f"passed as the first positional argument and the code should " + f"have resolved it to the root decision." + ) diff --git a/features/tdd_plan_correct_plan_id.feature b/features/tdd_plan_correct_plan_id.feature new file mode 100644 index 000000000..78ed1fe6e --- /dev/null +++ b/features/tdd_plan_correct_plan_id.feature @@ -0,0 +1,35 @@ +@tdd_expected_fail @tdd_bug @tdd_bug_969 +Feature: TDD Bug #969 — plan correct should accept plan_id as first positional argument + As a developer + I want plan correct to accept a plan_id as its first positional argument + So that the natural CLI usage pattern works correctly + + This test was written to capture bug #969. The plan correct CLI command + declares its first positional argument as decision_id. When the M3 + acceptance test passes a plan_id, the plan_id is used as + target_decision_id, which fails because the plan ID is not a valid + decision ID. The correction service raises an error because the + targeted decision cannot be found in the plan's decision tree. + + The expected behavior is that when a plan_id is passed as the first + positional argument, the command should recognize it as a plan_id, + auto-select the root decision for that plan as the correction target, + and complete the correction successfully. + + This test uses the @tdd_expected_fail tag until the fix in #969 is + merged. Once fixed, the tag will be removed and the test will run + normally as a regression guard. + + Scenario: plan correct resolves root decision when given a plan_id as positional (revert mode) + Given tpcpid a plan with a known plan_id and a root decision + And tpcpid a CorrectionService that succeeds when targeting the root decision + When tpcpid I invoke plan correct with the plan_id as the positional argument + Then tpcpid the command should exit successfully + And tpcpid request_correction should have targeted the root decision not the plan_id + + Scenario: plan correct resolves root decision when given a plan_id as positional (append mode) + Given tpcpid a plan with a known plan_id and a root decision + And tpcpid a CorrectionService for append mode that succeeds when targeting the root decision + When tpcpid I invoke plan correct with the plan_id as the positional argument in append mode + Then tpcpid the command should exit successfully + And tpcpid request_correction should have targeted the root decision not the plan_id diff --git a/robot/helper_tdd_plan_correct_plan_id.py b/robot/helper_tdd_plan_correct_plan_id.py new file mode 100644 index 000000000..12bb686da --- /dev/null +++ b/robot/helper_tdd_plan_correct_plan_id.py @@ -0,0 +1,129 @@ +"""Helper script for tdd_plan_correct_plan_id.robot integration tests. + +Each subcommand exercises the ``plan correct`` CLI command with a plan_id +as the first positional argument (where ``decision_id`` is expected). +The helper reports the **real** outcome: it exits 0 and prints the sentinel +when the operation succeeds (bug is fixed), and exits 1 when the bug is +still present. The ``tdd_expected_fail_listener`` on the Robot side handles +pass/fail inversion while the bug remains open. + +Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/969 +TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/979 +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from pathlib import Path +from unittest.mock import patch + +# Ensure local source tree is importable +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +# Ensure features package is importable for shared fixtures +_FEATURES = str(Path(__file__).resolve().parents[1]) +if _FEATURES not in sys.path: + sys.path.insert(0, _FEATURES) + +from features.mocks.tdd_plan_correct_plan_id_fixtures import ( # noqa: E402 + PATCH_CONTAINER, + PATCH_CORRECTION_SVC, + PATCH_RESOLVE_PLAN, + PLAN_ID, + ROOT_DECISION_ID, + build_cli_args, + make_correction_svc, + make_default_container, +) +from typer.testing import CliRunner # noqa: E402 + +from cleveragents.cli.commands.plan import app as plan_app # noqa: E402 + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def _run_plan_correct(mode: str, sentinel: str) -> None: + """Invoke ``plan correct `` and verify target_decision_id. + + Exits 0 with *sentinel* when the command correctly resolves the + plan_id to the root decision (bug fixed). + Exits 1 when the bug is still present. + + Args: + mode: The correction mode (``"revert"`` or ``"append"``). + sentinel: The sentinel string to print on success. + """ + mock_container = make_default_container() + correction_svc = make_correction_svc(ROOT_DECISION_ID, mode=mode) + + args = build_cli_args(PLAN_ID, mode=mode) + + with ( + patch(PATCH_CORRECTION_SVC, return_value=correction_svc), + patch(PATCH_CONTAINER, return_value=mock_container), + patch(PATCH_RESOLVE_PLAN, return_value=PLAN_ID), + ): + result = runner.invoke(plan_app, args) + + if result.exit_code != 0: + print( + f"plan correct failed with exit code {result.exit_code}: {result.output}", + file=sys.stderr, + ) + sys.exit(1) + + # Check that request_correction was called with root decision, not plan_id + correction_svc.request_correction.assert_called_once() + call_record = correction_svc.request_correction.call_args + kw = call_record.kwargs + + actual_target = kw.get("target_decision_id") + if actual_target != ROOT_DECISION_ID: + print( + f"Bug #969: request_correction called with " + f"target_decision_id={actual_target!r}, expected " + f"{ROOT_DECISION_ID!r}. Plan_id ({PLAN_ID!r}) was used " + f"as decision_id instead of being resolved to root decision.", + file=sys.stderr, + ) + sys.exit(1) + + print(sentinel) + + +def plan_correct_with_plan_id() -> None: + """Invoke ``plan correct `` with --mode revert.""" + _run_plan_correct("revert", "tdd-plan-correct-plan-id-ok") + + +def plan_correct_append_with_plan_id() -> None: + """Invoke ``plan correct `` with --mode append.""" + _run_plan_correct("append", "tdd-plan-correct-plan-id-append-ok") + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "plan-correct-with-plan-id": plan_correct_with_plan_id, + "plan-correct-append-with-plan-id": plan_correct_append_with_plan_id, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + cmd = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/tdd_plan_correct_plan_id.robot b/robot/tdd_plan_correct_plan_id.robot new file mode 100644 index 000000000..d3440dc59 --- /dev/null +++ b/robot/tdd_plan_correct_plan_id.robot @@ -0,0 +1,43 @@ +*** Settings *** +Documentation TDD Bug #969 — plan correct should accept plan_id as first positional +... argument. Integration test verifying that ``plan correct `` +... resolves the plan_id to the root decision and applies the correction +... successfully. Currently the command treats the first positional argument +... strictly as a decision_id, causing the correction service to fail when +... a plan_id is passed. The test is tagged tdd_expected_fail so CI passes +... via result inversion. +... +... Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/969 +... TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/979 +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_plan_correct_plan_id.py + +*** Test Cases *** +TDD Plan Correct Accepts Plan ID As Positional Argument Revert Mode + [Documentation] Verify that plan correct resolves a plan_id to the root + ... decision when the plan_id is passed as the first positional + ... argument with --mode revert. Bug #969: the code currently + ... uses the plan_id as target_decision_id directly. + [Tags] tdd_expected_fail tdd_bug tdd_bug_969 + ${result}= Run Process ${PYTHON} ${HELPER} plan-correct-with-plan-id cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-plan-correct-plan-id-ok + +TDD Plan Correct Accepts Plan ID As Positional Argument Append Mode + [Documentation] Verify that plan correct resolves a plan_id to the root + ... decision when the plan_id is passed as the first positional + ... argument with --mode append. Bug #969 affects + ... target_decision_id resolution before mode branching, so both + ... revert and append modes are affected. + [Tags] tdd_expected_fail tdd_bug tdd_bug_969 + ${result}= Run Process ${PYTHON} ${HELPER} plan-correct-append-with-plan-id cwd=${WORKSPACE} timeout=30s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-plan-correct-plan-id-append-ok