cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(cli): fix plan explain to accept decision_id argument #6618

Merged
HAL9000 merged 3 commits from fix/issue-6325-plan-explain-decision-id into master 2026-05-31 19:26:47 +00:00
Conversation 33 Commits 3 Files Changed 6
+89 -256
6 changed files with 89 additions and 256 deletions
Download Patch File Download Diff File Expand all files Collapse all files
features/steps/plan_explain_cli_coverage_steps.py
+2 -1
View File
1
 @@ -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)
features/steps/tdd_plan_explain_plan_id_steps.py
+20 -103
View File
@@ -1,15 +1,10 @@
"""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.
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
@@ -25,10 +20,6 @@ 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()
@@ -40,33 +31,6 @@ _PATCH_CONTAINER = "cleveragents.application.container.get_container"
# ---------------------------------------------------------------------------
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()
@@ -82,51 +46,14 @@ def _mock_container_with_decision_svc(svc_mock: MagicMock) -> MagicMock:
@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."""
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()
# 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
# ---------------------------------------------------------------------------
@@ -148,34 +75,24 @@ def step_tdd968_invoke_explain(context: Context) -> None:
# ---------------------------------------------------------------------------
@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}. "
@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 decision details")
def step_tdd968_output_has_details(context: Context) -> None:
"""Assert the output contains decision fields — fails while bug exists."""
@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
# 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,
)
assert text.lower() in output.lower(), f"Expected '{text}' in output, got: {output}"
@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."""
@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 context.tdd968_root_question in output, (
f"Expected root question '{context.tdd968_root_question}' "
f"in output, got: {output}"
)
assert text not in output, f"Unexpected '{text}' found in output: {output}"
features/tdd_plan_explain_plan_id.feature
+14 -26
View File
@@ -1,35 +1,23 @@
@tdd_issue @tdd_issue_968 @mock_only
Feature: TDD Issue #968 — plan explain expects decision_id but test passes plan_id
Feature: TDD Issue #968 — plan explain rejects plan_id, accepts only decision_id
As a developer
I want to verify that `plan explain <plan_id>` succeeds when given a plan ID
So that the bug is captured and will be caught by a regression test
I want to verify that `plan explain <plan_id>` is rejected with rc=1
So that the spec-compliant fix from issue #6325 is protected 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.
# 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 succeeds when given a plan_id with decisions
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
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
Then tdd968 the command should exit with return code 1
And tdd968 the output should contain "not found"
Scenario: Plan explain with plan_id shows root decision question
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
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
Then tdd968 the command should exit with return code 1
And tdd968 the output should not contain "decision_id"
robot/helper_tdd_plan_explain_plan_id.py
+33 -92
View File
@@ -1,14 +1,10 @@
"""Helper script for tdd_plan_explain_plan_id.robot integration tests.
Each subcommand exercises the ``plan explain <plan_id>`` 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 <plan_id>`` via subprocess.
Handles timeout with a descriptive error and sets ``NO_COLOR=1`` to
prevent ANSI escape codes in captured output.
"""
"""Run ``plan explain <plan_id>`` 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 <plan_id>`` succeeds (rc=0).
"""Verify that ``plan explain <plan_id>`` 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 <plan_id>`` 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__":
robot/tdd_plan_explain_plan_id.robot
+13 -15
View File
@@ -1,8 +1,7 @@
*** Settings ***
Documentation Bug #968 — plan explain expects decision_id but M3 test passes plan_id
... Integration tests verifying that ``plan explain <plan_id>`` 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.
Documentation Bug #968 / Issue #6325 — plan explain must reject plan_id, accept only decision_id
... Integration tests verifying that ``plan explain <plan_id>`` 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
@@ -11,24 +10,23 @@ Suite Teardown Cleanup Test Environment
${HELPER} ${CURDIR}/helper_tdd_plan_explain_plan_id.py
*** Test Cases ***
TDD Plan Explain Succeeds With Plan ID
[Documentation] Verify that ``plan explain <plan_id>`` exits with rc=0
... when given a plan ID that has associated decisions.
... Bug #968: the command currently exits with rc=1.
TDD Plan Explain Rejects Plan ID With RC 1
[Documentation] Verify that ``plan explain <plan_id>`` 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-ok
Should Contain ${result.stdout} tdd-plan-explain-plan-id-rejected
TDD Plan Explain With Plan ID Shows Root Question
[Documentation] Verify that ``plan explain <plan_id>`` output contains
... the root decision question when given a plan ID.
... Bug #968: the command fails before rendering any output.
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-question cwd=${WORKSPACE} timeout=180s on_timeout=kill
${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-question-ok
Should Contain ${result.stdout} tdd-plan-explain-plan-id-error-ok
src/cleveragents/cli/commands/plan.py
+7 -19
View File
2
 @@ -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,24 +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:
console.print(
f"[red]Error:[/red] '{identifier}' not found as a decision or plan."
)
raise typer.Exit(1)
except DecisionNotFoundError:
console.print(f"[red]Error:[/red] '{identifier}' not found as a decision.")
raise typer.Exit(1) from None
data = _build_explain_dict(
decision,