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

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 <plan_id>``.
+
+    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",
+]

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 <plan_id>`` 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 <plan_id>`` 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."
+    )

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

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 <plan_id>`` 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 <plan_id>`` with --mode revert."""
+    _run_plan_correct("revert", "tdd-plan-correct-plan-id-ok")
+
+
+def plan_correct_append_with_plan_id() -> None:
+    """Invoke ``plan correct <plan_id>`` 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()

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 <plan_id>``
+...              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