From c16d90058de424be2faf13f017a394eddc8b2a71 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 9 May 2026 00:56:51 +0000 Subject: [PATCH 1/3] fix(cli): only accept decision_id in plan explain (Closes #6325) --- .../steps/tdd_plan_explain_plan_id_steps.py | 181 ------------------ features/tdd_plan_explain_plan_id.feature | 35 ---- robot/tdd_plan_explain_plan_id.robot | 34 ---- src/cleveragents/cli/commands/plan.py | 22 +-- 4 files changed, 6 insertions(+), 266 deletions(-) delete mode 100644 features/steps/tdd_plan_explain_plan_id_steps.py delete mode 100644 features/tdd_plan_explain_plan_id.feature delete mode 100644 robot/tdd_plan_explain_plan_id.robot diff --git a/features/steps/tdd_plan_explain_plan_id_steps.py b/features/steps/tdd_plan_explain_plan_id_steps.py deleted file mode 100644 index badf21d67..000000000 --- a/features/steps/tdd_plan_explain_plan_id_steps.py +++ /dev/null @@ -1,181 +0,0 @@ -"""Step definitions for tdd_plan_explain_plan_id.feature. - -TDD issue-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 deleted file mode 100644 index e32790b2e..000000000 --- a/features/tdd_plan_explain_plan_id.feature +++ /dev/null @@ -1,35 +0,0 @@ -@tdd_issue @tdd_issue_968 @mock_only -Feature: TDD Issue #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 @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/tdd_plan_explain_plan_id.robot b/robot/tdd_plan_explain_plan_id.robot deleted file mode 100644 index d2eefbafa..000000000 --- a/robot/tdd_plan_explain_plan_id.robot +++ /dev/null @@ -1,34 +0,0 @@ -*** Settings *** -Documentation 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. Bug #968 has been fixed: -... the command now correctly resolves a plan_id to its decisions. -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_issue tdd_issue_968 tdd_issue_4178 - ${result}= Run Process ${PYTHON} ${HELPER} explain-with-plan-id cwd=${WORKSPACE} timeout=180s 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_issue tdd_issue_968 tdd_issue_4178 - ${result}= Run Process ${PYTHON} ${HELPER} explain-plan-id-shows-question cwd=${WORKSPACE} timeout=180s 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 diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 44fe96707..0739606b8 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -4025,7 +4025,7 @@ def _build_explain_dict( def explain_decision_cmd( identifier: Annotated[ str, - typer.Argument(help="Decision or Plan ULID to explain"), + typer.Argument(help="Decision ULID to explain"), ], fmt: Annotated[ str, @@ -4040,7 +4040,7 @@ def explain_decision_cmd( typer.Option("--show-reasoning", help="Include rationale and actor reasoning"), ] = False, ) -> None: - """Explain a single decision or the root decision of a plan.""" + """Explain a single decision in a plan.""" from cleveragents.application.container import get_container from cleveragents.application.services.decision_service import ( DecisionNotFoundError, @@ -4049,22 +4049,12 @@ def explain_decision_cmd( container = get_container() svc = container.decision_service() - # First, try treating the identifier as a decision_id (backward compat). - decision = None - with suppress(DecisionNotFoundError): + # Look up the decision by its ULID. + try: decision = svc.get_decision(identifier) - - # If not found as a decision, try as a plan_id. - if decision is None: - decisions = svc.list_decisions(identifier) - if decisions: - # Find root decision (parent_decision_id is None) - root_decisions = [d for d in decisions if d.parent_decision_id is None] - decision = root_decisions[0] if root_decisions else decisions[0] - - if decision is None: + except DecisionNotFoundError: console.print( - f"[red]Error:[/red] '{identifier}' not found as a decision or plan." + f"[red]Error:[/red] '{identifier}' is not a valid decision. Use `agents plan explain` with a Decision ULID." ) raise typer.Exit(1) -- 2.52.0 From a117445f6d66c82c61cca8d3eb4c4a61d3a00043 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 31 May 2026 11:26:09 -0400 Subject: [PATCH 2/3] fix(cli): fix lint and unit test failures in plan explain error path - Shorten error message in explain_decision_cmd to fit 88-char limit (E501) - Add `from None` to raise typer.Exit(1) inside except block (B904) - Fix step definition mock to raise DecisionNotFoundError instead of returning None, so the exception handler fires and output contains "not found" as the scenario asserts ISSUES CLOSED: #6325 --- features/steps/plan_explain_cli_coverage_steps.py | 3 ++- src/cleveragents/cli/commands/plan.py | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/features/steps/plan_explain_cli_coverage_steps.py b/features/steps/plan_explain_cli_coverage_steps.py index 2b70b95bd..b70b28e8d 100644 --- a/features/steps/plan_explain_cli_coverage_steps.py +++ b/features/steps/plan_explain_cli_coverage_steps.py @@ -20,6 +20,7 @@ from cleveragents.application.services.plan_lifecycle_service import ( InvalidPhaseTransitionError, ) from cleveragents.cli.commands.plan import app as plan_app +from cleveragents.application.services.decision_service import DecisionNotFoundError from cleveragents.core.exceptions import ( CleverAgentsError, PlanError, @@ -169,7 +170,7 @@ def step_pec_mock_decision_svc(context: Context) -> None: def step_pec_mock_decision_none(context: Context) -> None: context.pec_decision_id = str(ULID()) svc = MagicMock() - svc.get_decision.return_value = None + svc.get_decision.side_effect = DecisionNotFoundError(context.pec_decision_id) svc.list_decisions.return_value = [] context.pec_container = _mock_container_with_decision_svc(svc) diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 0739606b8..31528c61a 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -4053,10 +4053,8 @@ def explain_decision_cmd( try: decision = svc.get_decision(identifier) except DecisionNotFoundError: - console.print( - f"[red]Error:[/red] '{identifier}' is not a valid decision. Use `agents plan explain` with a Decision ULID." - ) - raise typer.Exit(1) + console.print(f"[red]Error:[/red] '{identifier}' not found as a decision.") + raise typer.Exit(1) from None data = _build_explain_dict( decision, -- 2.52.0 From 4d04b2e4adc1344bea0b4344fdaa7c9c6925971a Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 31 May 2026 12:32:10 -0400 Subject: [PATCH 3/3] test(tdd): restore tdd_plan_explain_plan_id regression tests for fixed behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TDD regression files for bug #968 were incorrectly deleted instead of updated to assert the spec-compliant fixed behaviour from issue #6325. Per CONTRIBUTING.md the regression guard must be kept and updated — not removed — once the bug is fixed. Restore and update all three files to assert the new expected behaviour: - plan explain rejects a non-decision-id argument with rc=1 - error output contains "not found" - decision data fields are absent from output Also pair the orphaned robot/helper_tdd_plan_explain_plan_id.py with a restored robot test file and update it to verify rc=1 rejection behaviour. ISSUES CLOSED: #6325 --- .../steps/tdd_plan_explain_plan_id_steps.py | 98 ++++++++++++++ features/tdd_plan_explain_plan_id.feature | 23 ++++ robot/helper_tdd_plan_explain_plan_id.py | 125 +++++------------- robot/tdd_plan_explain_plan_id.robot | 32 +++++ 4 files changed, 186 insertions(+), 92 deletions(-) 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/tdd_plan_explain_plan_id.robot 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..2810cc4f5 --- /dev/null +++ b/features/steps/tdd_plan_explain_plan_id_steps.py @@ -0,0 +1,98 @@ +"""Step definitions for tdd_plan_explain_plan_id.feature. + +TDD regression guard for bug #968 / issue #6325: +``plan explain`` must accept only a decision_id as its positional argument. +When passed a plan_id (not a decision ULID), the command should exit with +rc=1 and a clear error message. The old fallback that called +``list_decisions(plan_id)`` has been removed per issue #6325. +""" + +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 + +runner = CliRunner() + +_PATCH_CONTAINER = "cleveragents.application.container.get_container" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +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_raises(context: Context) -> None: + """Simulate a plan_id passed where a decision_id is expected.""" + context.tdd968_plan_id = str(ULID()) + svc = MagicMock() + svc.get_decision.side_effect = DecisionNotFoundError(context.tdd968_plan_id) + context.tdd968_container = _mock_container_with_decision_svc(svc) + + +# --------------------------------------------------------------------------- +# 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 1") +def step_tdd968_rc_one(context: Context) -> None: + """Assert rc=1 — the command correctly rejects a non-decision-id argument.""" + assert context.tdd968_result.exit_code == 1, ( + f"Expected exit code 1 but got {context.tdd968_result.exit_code}. " + f"Output: {context.tdd968_result.output}" + ) + + +@then('tdd968 the output should contain "{text}"') +def step_tdd968_output_contains(context: Context, text: str) -> None: + """Assert the output contains the expected substring.""" + output = context.tdd968_result.output + assert text.lower() in output.lower(), f"Expected '{text}' in output, got: {output}" + + +@then('tdd968 the output should not contain "{text}"') +def step_tdd968_output_not_contains(context: Context, text: str) -> None: + """Assert the output does not contain the given substring.""" + output = context.tdd968_result.output + assert text not in output, f"Unexpected '{text}' found in output: {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..3e59669c2 --- /dev/null +++ b/features/tdd_plan_explain_plan_id.feature @@ -0,0 +1,23 @@ +@tdd_issue @tdd_issue_968 @mock_only +Feature: TDD Issue #968 — plan explain rejects plan_id, accepts only decision_id + As a developer + I want to verify that `plan explain ` is rejected with rc=1 + So that the spec-compliant fix from issue #6325 is protected by a regression test + + # Originally written to capture bug #968: plan explain silently fell back to + # treating the argument as a plan_id via list_decisions. Issue #6325 fixed + # this by removing the fallback and enforcing strict decision-only lookup. + # These scenarios assert the fixed behaviour: rc=1 with a clear error message + # when a non-decision identifier is passed to `plan explain`. + + Scenario: Plan explain rejects a plan_id with return code 1 + Given tdd968 a mock DecisionService where get_decision raises DecisionNotFoundError for a plan id + When tdd968 I invoke plan explain with the plan id + Then tdd968 the command should exit with return code 1 + And tdd968 the output should contain "not found" + + Scenario: Plan explain with plan_id does not output decision data + Given tdd968 a mock DecisionService where get_decision raises DecisionNotFoundError for a plan id + When tdd968 I invoke plan explain with the plan id + Then tdd968 the command should exit with return code 1 + And tdd968 the output should not contain "decision_id" diff --git a/robot/helper_tdd_plan_explain_plan_id.py b/robot/helper_tdd_plan_explain_plan_id.py index 15e336c87..69d5bbd2a 100644 --- a/robot/helper_tdd_plan_explain_plan_id.py +++ b/robot/helper_tdd_plan_explain_plan_id.py @@ -1,14 +1,10 @@ """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. +TDD regression guard for issue #6325 / bug #968: +``plan explain`` must reject a plan_id with rc=1. The old fallback that +resolved a plan_id to its root decision via ``list_decisions`` has been +removed. Passing a plan_id (which is not a decision ULID) should produce +rc=1 and an error message containing "not found". """ from __future__ import annotations @@ -32,12 +28,8 @@ for _p in (_SRC, _ROBOT): from ulid import ULID # noqa: E402 -from cleveragents.application.container import ( # noqa: E402 - get_container, - reset_container, -) +from cleveragents.application.container import reset_container # noqa: E402 from cleveragents.config.settings import Settings # noqa: E402 -from cleveragents.domain.models.core.decision import DecisionType # noqa: E402 # --------------------------------------------------------------------------- # Helpers @@ -78,48 +70,8 @@ def _restore_isolated_home(tmp_home: str, previous_home: str | None) -> None: shutil.rmtree(tmp_home, ignore_errors=True) -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. - """ + """Run ``plan explain `` via subprocess.""" try: return subprocess.run( [ @@ -139,10 +91,7 @@ def _run_plan_explain(plan_id: str) -> subprocess.CompletedProcess[str]: 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." - ) + _fail(f"plan explain {plan_id} timed out after 45 seconds.") # --------------------------------------------------------------------------- @@ -151,63 +100,55 @@ def _run_plan_explain(plan_id: str) -> subprocess.CompletedProcess[str]: def explain_with_plan_id() -> None: - """Verify that ``plan explain `` succeeds (rc=0). + """Verify that ``plan explain `` is rejected with rc=1. - 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. + Issue #6325: the command no longer falls back to list_decisions when the + argument is not a decision ULID. Passing a plan_id should exit with rc=1. """ tmp_home, previous_home = _setup_isolated_home() try: - plan_id: str = _setup_plan_with_decisions() + plan_id: str = str(ULID()) result: subprocess.CompletedProcess[str] = _run_plan_explain(plan_id) - if result.returncode != 0: + if result.returncode != 1: _fail( - f"plan explain {plan_id} exited with rc={result.returncode}. " + f"plan explain {plan_id} rc={result.returncode}, expected rc=1. " 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." + f"Issue #6325: the command should reject a plan_id with rc=1." ) - # 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") + print("tdd-plan-explain-plan-id-rejected") finally: _restore_isolated_home(tmp_home, previous_home) -def explain_plan_id_shows_question() -> None: - """Verify that ``plan explain `` shows the root decision question. +def explain_plan_id_shows_error() -> None: + """Verify that the error output for a plan_id contains 'not found'. - Bug #968: Since the command fails with rc=1 before any output is - rendered, the root decision question is never displayed. + Issue #6325: the error message should clearly indicate the identifier + was not found as a decision. """ tmp_home, previous_home = _setup_isolated_home() try: - plan_id: str = _setup_plan_with_decisions() + plan_id: str = str(ULID()) result: subprocess.CompletedProcess[str] = _run_plan_explain(plan_id) - if result.returncode != 0: + if result.returncode != 1: _fail( - f"plan explain {plan_id} exited with rc={result.returncode}. " - f"Bug #968: command fails before rendering any output." + f"plan explain {plan_id} rc={result.returncode}, expected rc=1. " + f"stdout: {result.stdout}\n" + f"stderr: {result.stderr}" ) - if "What should we build?" not in result.stdout: - _fail(f"Expected root decision question in output. stdout: {result.stdout}") + combined: str = (result.stdout + result.stderr).lower() + if "not found" not in combined: + _fail( + f"Expected 'not found' in plan explain output. " + f"stdout: {result.stdout}\nstderr: {result.stderr}" + ) - print("tdd-plan-explain-plan-id-question-ok") + print("tdd-plan-explain-plan-id-error-ok") finally: _restore_isolated_home(tmp_home, previous_home) @@ -218,7 +159,7 @@ def explain_plan_id_shows_question() -> None: _COMMANDS: dict[str, Callable[[], None]] = { "explain-with-plan-id": explain_with_plan_id, - "explain-plan-id-shows-question": explain_plan_id_shows_question, + "explain-plan-id-shows-error": explain_plan_id_shows_error, } if __name__ == "__main__": diff --git a/robot/tdd_plan_explain_plan_id.robot b/robot/tdd_plan_explain_plan_id.robot new file mode 100644 index 000000000..c636e01cd --- /dev/null +++ b/robot/tdd_plan_explain_plan_id.robot @@ -0,0 +1,32 @@ +*** Settings *** +Documentation Bug #968 / Issue #6325 — plan explain must reject plan_id, accept only decision_id +... Integration tests verifying that ``plan explain `` is correctly +... rejected with rc=1. The old list_decisions fallback has been removed. +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 Rejects Plan ID With RC 1 + [Documentation] Verify that ``plan explain `` exits with rc=1 + ... when given a plan ID (not a decision ID). + ... Issue #6325: the fallback to list_decisions has been removed. + [Tags] tdd_issue tdd_issue_968 tdd_issue_4178 + ${result}= Run Process ${PYTHON} ${HELPER} explain-with-plan-id cwd=${WORKSPACE} timeout=180s 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-rejected + +TDD Plan Explain With Plan ID Shows Error Message + [Documentation] Verify that the output when given a plan_id contains + ... "not found" per the spec-compliant error message. + [Tags] tdd_issue tdd_issue_968 tdd_issue_4178 + ${result}= Run Process ${PYTHON} ${HELPER} explain-plan-id-shows-error cwd=${WORKSPACE} timeout=180s 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-error-ok -- 2.52.0