From 058d486593813870d4bd0bdcc97239a1249f36d9 Mon Sep 17 00:00:00 2001 From: Rui Hu Date: Wed, 18 Mar 2026 09:20:27 +0000 Subject: [PATCH] =?UTF-8?q?test:=20add=20TDD=20bug-capture=20test=20for=20?= =?UTF-8?q?#968=20=E2=80=94=20plan=20explain=20plan=5Fid=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TDD bug-capture tests for bug #968, which reports that the `plan explain` CLI command fails with rc=1 when given a plan_id because `explain_decision_cmd` treats its argument as a decision_id and `svc.get_decision(plan_id)` returns None. Behave tests (features/tdd_plan_explain_plan_id.feature): - Two scenarios tagged @tdd_expected_fail @tdd_bug @tdd_bug_968 - Mock DecisionService with get_decision returning None for plan_id and list_decisions returning decisions for the plan - Assert plan explain succeeds (rc=0) with decision details - Assertions intentionally fail (proving the bug exists); tag inversion makes CI pass Robot tests (robot/tdd_plan_explain_plan_id.robot): - Two integration test cases with tdd_expected_fail tags - Helper script records decisions via DecisionService, invokes CLI `plan explain `, and asserts rc=0 with decision output The tests will run normally (without @tdd_expected_fail) once bug #968 is fixed, serving as permanent regression tests. ISSUES CLOSED: #978 --- CHANGELOG.md | 7 + .../steps/tdd_plan_explain_plan_id_steps.py | 181 ++++++++++++++++ features/tdd_plan_explain_plan_id.feature | 35 ++++ robot/helper_tdd_plan_explain_plan_id.py | 197 ++++++++++++++++++ robot/tdd_plan_explain_plan_id.robot | 36 ++++ 5 files changed, 456 insertions(+) create mode 100644 features/steps/tdd_plan_explain_plan_id_steps.py create mode 100644 features/tdd_plan_explain_plan_id.feature create mode 100644 robot/helper_tdd_plan_explain_plan_id.py create mode 100644 robot/tdd_plan_explain_plan_id.robot diff --git a/CHANGELOG.md b/CHANGELOG.md index 24a71e1e4..76e7a388e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +- 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 + behaviour — ``plan explain `` succeeds (rc=0) and displays + decision details. Includes Robot Framework integration tests with a + helper script exercising the same CLI path via subprocess, and step + definitions following established patterns. (#978) - Added TDD bug-capture tests for bug #967 — `plan execute` phase processing. Tests exercise the CLI orchestration layer via CliRunner (Behave) and replicated CLI logic (Robot) to verify that `plan execute` correctly diff --git a/features/steps/tdd_plan_explain_plan_id_steps.py b/features/steps/tdd_plan_explain_plan_id_steps.py new file mode 100644 index 000000000..d7aff64a1 --- /dev/null +++ b/features/steps/tdd_plan_explain_plan_id_steps.py @@ -0,0 +1,181 @@ +"""Step definitions for tdd_plan_explain_plan_id.feature. + +TDD bug-capture test for bug #968: ``plan explain`` expects a decision_id +as its first positional argument, but the M3 acceptance test passes a plan_id. +Because ``svc.get_decision(plan_id)`` raises ``DecisionNotFoundError`` (the +plan ID is not a decision ID), the command exits with rc=1 and "Decision not +found" error. + +These steps use the ``@tdd_expected_fail`` tag so that the assertions — which +expect the *fixed* behaviour (rc=0 with decision details) — do not fail CI +while the bug is still unfixed. Once bug #968 is fixed the tag will be +removed and the tests will run normally. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner +from ulid import ULID + +from cleveragents.application.services.decision_service import ( + DecisionNotFoundError, +) +from cleveragents.cli.commands.plan import app as plan_app +from cleveragents.domain.models.core.decision import ( + Decision, + DecisionType, +) + +runner = CliRunner() + +_PATCH_CONTAINER = "cleveragents.application.container.get_container" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_decision( + decision_id: str | None = None, + plan_id: str | None = None, + sequence: int = 0, + parent_id: str | None = None, + dtype: DecisionType = DecisionType.PROMPT_DEFINITION, + question: str = "What should we build?", + chosen: str = "A REST API", +) -> Decision: + """Build a minimal Decision with sensible defaults.""" + did = decision_id or str(ULID()) + pid = plan_id or str(ULID()) + kwargs: dict = { + "decision_id": did, + "plan_id": pid, + "sequence_number": sequence, + "decision_type": dtype, + "question": question, + "chosen_option": chosen, + } + if parent_id is not None: + kwargs["parent_decision_id"] = parent_id + elif dtype != DecisionType.PROMPT_DEFINITION: + kwargs["parent_decision_id"] = str(ULID()) + return Decision(**kwargs) + + +def _mock_container_with_decision_svc(svc_mock: MagicMock) -> MagicMock: + """Create a mock container whose ``decision_service()`` returns *svc_mock*.""" + container = MagicMock() + container.decision_service.return_value = svc_mock + return container + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given( + "tdd968 a mock DecisionService where get_decision raises DecisionNotFoundError for a plan id" +) +def step_tdd968_mock_get_decision_none(context: Context) -> None: + """Simulate the current buggy behaviour: plan ID is not a decision ID.""" + context.tdd968_plan_id = str(ULID()) + svc = MagicMock() + # get_decision raises DecisionNotFoundError when called with a plan_id (the bug) + svc.get_decision.side_effect = DecisionNotFoundError(context.tdd968_plan_id) + context.tdd968_svc = svc + context.tdd968_container = _mock_container_with_decision_svc(svc) + + +@given( + "tdd968 the same mock DecisionService returns decisions " + "via list_decisions for the plan id" +) +def step_tdd968_mock_list_decisions(context: Context) -> None: + """Set up list_decisions to return real decisions for the plan. + + After the fix, the command should fall back to this lookup when + get_decision raises DecisionNotFoundError. + """ + root_id = str(ULID()) + child_id = str(ULID()) + pid = context.tdd968_plan_id + context.tdd968_root_question = "What should we build?" + decisions = [ + _make_decision( + decision_id=root_id, + plan_id=pid, + sequence=0, + question=context.tdd968_root_question, + chosen="A REST API", + ), + _make_decision( + decision_id=child_id, + plan_id=pid, + parent_id=root_id, + sequence=1, + dtype=DecisionType.STRATEGY_CHOICE, + question="Which framework?", + chosen="FastAPI", + ), + ] + context.tdd968_svc.list_decisions.return_value = decisions + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("tdd968 I invoke plan explain with the plan id") +def step_tdd968_invoke_explain(context: Context) -> None: + """Invoke ``plan explain `` via CliRunner with mocked container.""" + with patch(_PATCH_CONTAINER, return_value=context.tdd968_container): + result = runner.invoke( + plan_app, + ["explain", context.tdd968_plan_id, "--format", "json"], + ) + context.tdd968_result = result + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("tdd968 the command should exit with return code 0") +def step_tdd968_rc_zero(context: Context) -> None: + """Assert rc=0 — this will FAIL while bug #968 is unfixed (rc=1).""" + assert context.tdd968_result.exit_code == 0, ( + f"Expected exit code 0 but got {context.tdd968_result.exit_code}. " + f"Output: {context.tdd968_result.output}" + ) + + +@then("tdd968 the output should contain decision details") +def step_tdd968_output_has_details(context: Context) -> None: + """Assert the output contains decision fields — fails while bug exists.""" + output = context.tdd968_result.output + # When the bug is fixed, the output should contain decision fields + assert "decision_id" in output and "question" in output, ( + f"Expected decision details in output, got: {output}" + ) + # Verify that the fix called list_decisions with the correct plan_id + context.tdd968_svc.list_decisions.assert_called_once_with( + context.tdd968_plan_id, + ) + + +@then("tdd968 the output should contain the root decision question") +def step_tdd968_output_has_question(context: Context) -> None: + """Assert the root decision question appears — fails while bug exists.""" + output = context.tdd968_result.output + assert context.tdd968_root_question in output, ( + f"Expected root question '{context.tdd968_root_question}' " + f"in output, got: {output}" + ) diff --git a/features/tdd_plan_explain_plan_id.feature b/features/tdd_plan_explain_plan_id.feature new file mode 100644 index 000000000..77bb8c4f3 --- /dev/null +++ b/features/tdd_plan_explain_plan_id.feature @@ -0,0 +1,35 @@ +@tdd_expected_fail @tdd_bug @tdd_bug_968 @mock_only +Feature: TDD Bug #968 — plan explain expects decision_id but test passes plan_id + As a developer + I want to verify that `plan explain ` succeeds when given a plan ID + So that the bug is captured and will be caught by a regression test + + # This test was written to capture bug #968: + # The `plan explain` CLI command declares its first positional argument as + # `decision_id` (a Decision ULID). When the M3 acceptance test passes a + # plan ID, `svc.get_decision(plan_id)` raises DecisionNotFoundError because + # the plan ID is not a decision ID. The command exits with rc=1 and + # "Decision not found". + # + # The expected fix (#968) will make `explain_decision_cmd` fall back to + # treating the argument as a plan_id when decision lookup fails — looking + # up decisions for the plan via `decision_service.list_decisions(plan_id)` + # and explaining the root decision. + # + # These tests assert the *fixed* behaviour (rc=0 with decision details) and + # will FAIL until the bug is fixed. The @tdd_expected_fail tag inverts the + # result so CI passes. + + Scenario: Plan explain succeeds when given a plan_id with decisions + Given tdd968 a mock DecisionService where get_decision raises DecisionNotFoundError for a plan id + And tdd968 the same mock DecisionService returns decisions via list_decisions for the plan id + When tdd968 I invoke plan explain with the plan id + Then tdd968 the command should exit with return code 0 + And tdd968 the output should contain decision details + + Scenario: Plan explain with plan_id shows root decision question + Given tdd968 a mock DecisionService where get_decision raises DecisionNotFoundError for a plan id + And tdd968 the same mock DecisionService returns decisions via list_decisions for the plan id + When tdd968 I invoke plan explain with the plan id + Then tdd968 the command should exit with return code 0 + And tdd968 the output should contain the root decision question diff --git a/robot/helper_tdd_plan_explain_plan_id.py b/robot/helper_tdd_plan_explain_plan_id.py new file mode 100644 index 000000000..eb811dd6d --- /dev/null +++ b/robot/helper_tdd_plan_explain_plan_id.py @@ -0,0 +1,197 @@ +"""Helper script for tdd_plan_explain_plan_id.robot integration tests. + +Each subcommand exercises the ``plan explain `` CLI path to reproduce +bug #968. The ``plan explain`` command currently declares its first positional +argument as ``decision_id``. When passed a plan ID, ``svc.get_decision(plan_id)`` +raises ``DecisionNotFoundError`` (the plan ID is not a decision ID) and the +command exits with rc=1 and "Decision not found" error. + +The helper exits 0 with a sentinel when the command succeeds (bug 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. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from collections.abc import Callable +from pathlib import Path +from typing import NoReturn + +# Ensure local source tree AND robot/ directory are importable. +_ROOT: Path = Path(__file__).resolve().parents[1] +_SRC: str = str(_ROOT / "src") +_ROBOT: str = str(_ROOT / "robot") +for _p in (_SRC, _ROBOT): + if _p not in sys.path: + sys.path.insert(0, _p) + +from ulid import ULID # noqa: E402 + +from cleveragents.application.container import get_container # noqa: E402 +from cleveragents.domain.models.core.decision import DecisionType # noqa: E402 + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _fail(msg: str) -> NoReturn: + """Print failure message to stderr and exit with code 1.""" + print(msg, file=sys.stderr) + sys.exit(1) + + +def _make_subprocess_env() -> dict[str, str]: + """Build environment for subprocess calls with ``NO_COLOR=1``.""" + env: dict[str, str] = os.environ.copy() + env["NO_COLOR"] = "1" + return env + + +def _setup_plan_with_decisions() -> str: + """Create a plan_id and record decisions against it. + + Uses the DecisionService directly with a synthetic plan ID. + The decision service does not require an actual Plan object to exist — + it records decisions keyed by plan_id string. + + Returns the plan_id with recorded decisions. + """ + container = get_container() + decision_svc = container.decision_service() + + plan_id: str = str(ULID()) + + # Record a root decision against this plan + decision_svc.record_decision( + plan_id=plan_id, + decision_type=DecisionType.PROMPT_DEFINITION, + question="What should we build?", + chosen_option="A REST API", + rationale="REST API is the most common pattern", + ) + + # Defensive check: verify that the decision was persisted before the + # subprocess reads it (distinguishes setup failures from the actual bug). + decisions = decision_svc.list_decisions(plan_id) + if not decisions: + _fail( + f"Setup failure: record_decision succeeded but list_decisions " + f"returned no decisions for plan_id={plan_id}. " + f"This is a test setup problem, not bug #968." + ) + + return plan_id + + +def _run_plan_explain(plan_id: str) -> subprocess.CompletedProcess[str]: + """Run ``plan explain `` via subprocess. + + Handles timeout with a descriptive error and sets ``NO_COLOR=1`` to + prevent ANSI escape codes in captured output. + """ + try: + return subprocess.run( + [ + sys.executable, + "-m", + "cleveragents", + "plan", + "explain", + plan_id, + "--format", + "plain", + ], + capture_output=True, + text=True, + timeout=45, + cwd=str(_ROOT), + env=_make_subprocess_env(), + ) + except subprocess.TimeoutExpired: + _fail( + f"plan explain {plan_id} timed out after 45 seconds. " + f"Bug #968: subprocess exceeded inner timeout." + ) + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def explain_with_plan_id() -> None: + """Verify that ``plan explain `` succeeds (rc=0). + + Bug #968: The command currently treats the argument as a decision_id, + calls ``svc.get_decision(plan_id)`` which raises DecisionNotFoundError, + and exits with rc=1. When the fix is applied, the command should fall + back to looking up decisions for the plan via ``list_decisions(plan_id)`` + and explain the root decision. + """ + plan_id: str = _setup_plan_with_decisions() + result: subprocess.CompletedProcess[str] = _run_plan_explain(plan_id) + + if result.returncode != 0: + _fail( + f"plan explain {plan_id} exited with rc={result.returncode}. " + f"stdout: {result.stdout}\n" + f"stderr: {result.stderr}\n" + f"Bug #968: explain treats the argument as a decision_id, " + f"get_decision(plan_id) raises DecisionNotFoundError, command fails." + ) + + # Verify the output contains decision-related content — both keywords + # must be present (mirrors the AND-based assertion in the Behave test). + combined: str = result.stdout + result.stderr + if "decision" not in combined.lower() or "question" not in combined.lower(): + _fail( + f"plan explain output does not contain decision details. " + f"stdout: {result.stdout}" + ) + + print("tdd-plan-explain-plan-id-ok") + + +def explain_plan_id_shows_question() -> None: + """Verify that ``plan explain `` shows the root decision question. + + Bug #968: Since the command fails with rc=1 before any output is + rendered, the root decision question is never displayed. + """ + plan_id: str = _setup_plan_with_decisions() + result: subprocess.CompletedProcess[str] = _run_plan_explain(plan_id) + + if result.returncode != 0: + _fail( + f"plan explain {plan_id} exited with rc={result.returncode}. " + f"Bug #968: command fails before rendering any output." + ) + + if "What should we build?" not in result.stdout: + _fail(f"Expected root decision question in output. stdout: {result.stdout}") + + print("tdd-plan-explain-plan-id-question-ok") + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "explain-with-plan-id": explain_with_plan_id, + "explain-plan-id-shows-question": explain_plan_id_shows_question, +} + +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: Callable[[], None] = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/tdd_plan_explain_plan_id.robot b/robot/tdd_plan_explain_plan_id.robot new file mode 100644 index 000000000..ad3e4116c --- /dev/null +++ b/robot/tdd_plan_explain_plan_id.robot @@ -0,0 +1,36 @@ +*** Settings *** +Documentation TDD Bug #968 — plan explain expects decision_id but M3 test passes plan_id +... Integration tests verifying that ``plan explain `` succeeds +... when given a plan ID rather than a decision ID. Currently the command +... treats the argument as a decision_id, calls ``svc.get_decision(plan_id)`` +... which returns None, and exits with rc=1 and "Decision not found" error. +... Tests are tagged tdd_expected_fail so CI passes via result inversion. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment With Database Isolation +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_plan_explain_plan_id.py + +*** Test Cases *** +TDD Plan Explain Succeeds With Plan ID + [Documentation] Verify that ``plan explain `` exits with rc=0 + ... when given a plan ID that has associated decisions. + ... Bug #968: the command currently exits with rc=1. + [Tags] tdd_expected_fail tdd_bug tdd_bug_968 + ${result}= Run Process ${PYTHON} ${HELPER} explain-with-plan-id cwd=${WORKSPACE} timeout=60s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-plan-explain-plan-id-ok + +TDD Plan Explain With Plan ID Shows Root Question + [Documentation] Verify that ``plan explain `` output contains + ... the root decision question when given a plan ID. + ... Bug #968: the command fails before rendering any output. + [Tags] tdd_expected_fail tdd_bug tdd_bug_968 + ${result}= Run Process ${PYTHON} ${HELPER} explain-plan-id-shows-question cwd=${WORKSPACE} timeout=60s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-plan-explain-plan-id-question-ok -- 2.52.0