From a4499897309155611571cbf0380ee40fd9563658 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Wed, 18 Mar 2026 19:29:16 +0000 Subject: [PATCH] fix(cli): make plan correct accept plan_id as primary identifier The `plan correct` command previously only accepted a decision_id as its positional argument. When M3 acceptance tests pass a plan_id instead, the command fails because the plan_id is not a valid decision. The positional parameter is now named `identifier` and tries plan_id resolution first via `PlanLifecycleService.get_plan()`. When the identifier is a valid plan, the root decision (parent_decision_id is None) is automatically selected as the correction target. If the identifier is not a plan, the original decision_id behavior is preserved for backward compatibility. ISSUES CLOSED: #969 --- .../tdd_plan_correct_plan_id_fixtures.py | 44 +++++++++++++++- features/tdd_plan_correct_plan_id.feature | 2 +- robot/tdd_plan_correct_plan_id.robot | 4 +- src/cleveragents/cli/commands/plan.py | 51 +++++++++++++++---- 4 files changed, 88 insertions(+), 13 deletions(-) diff --git a/features/mocks/tdd_plan_correct_plan_id_fixtures.py b/features/mocks/tdd_plan_correct_plan_id_fixtures.py index 72ef0b037..131c8e7dd 100644 --- a/features/mocks/tdd_plan_correct_plan_id_fixtures.py +++ b/features/mocks/tdd_plan_correct_plan_id_fixtures.py @@ -16,9 +16,20 @@ TDD: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/979 from __future__ import annotations +from datetime import datetime from types import SimpleNamespace from unittest.mock import MagicMock +from cleveragents.domain.models.core.plan import ( + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + PlanTimestamps, + ProcessingState, + ProjectLink, +) + # --------------------------------------------------------------------------- # Patch targets # --------------------------------------------------------------------------- @@ -58,16 +69,47 @@ def make_decision_ns( ) +def _make_plan(_plan_id: str) -> Plan: + """Build a real ``Plan`` domain object so ``isinstance`` checks pass. + + The *_plan_id* argument is accepted for API symmetry but not used + directly because ``PlanIdentity.plan_id`` must be a valid 26-char + ULID. A fresh ULID is generated instead. + """ + from ulid import ULID + + return Plan( + identity=PlanIdentity(plan_id=str(ULID())), + namespaced_name=NamespacedName(namespace="local", name="tdd-969-plan"), + action_name="local/tdd-969-action", + description="TDD plan for bug #969", + phase=PlanPhase.EXECUTE, + processing_state=ProcessingState.COMPLETE, + project_links=[ProjectLink(project_name="proj-1")], + strategy_actor="openai/gpt-4", + execution_actor="openai/gpt-4", + timestamps=PlanTimestamps(created_at=datetime.now(), updated_at=datetime.now()), + ) + + def make_mock_container( decisions: list[SimpleNamespace], influence_edges: dict[str, list[str]], ) -> MagicMock: - """Build a mock DI container returning a DecisionService.""" + """Build a mock DI container returning DecisionService and PlanLifecycleService.""" mock_decision_svc = MagicMock() mock_decision_svc.list_decisions.return_value = decisions mock_decision_svc.get_influence_edges.return_value = influence_edges + + # The fix for bug #969 calls plan_lifecycle_service().get_plan() + # to detect whether the identifier is a plan_id. We must return + # a real Plan so that ``isinstance(plan_obj, Plan)`` passes. + mock_plan_svc = MagicMock() + mock_plan_svc.get_plan.return_value = _make_plan(PLAN_ID) + mock_container = MagicMock() mock_container.decision_service.return_value = mock_decision_svc + mock_container.plan_lifecycle_service.return_value = mock_plan_svc return mock_container diff --git a/features/tdd_plan_correct_plan_id.feature b/features/tdd_plan_correct_plan_id.feature index 78ed1fe6e..a0f3a1eb4 100644 --- a/features/tdd_plan_correct_plan_id.feature +++ b/features/tdd_plan_correct_plan_id.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_969 +@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 diff --git a/robot/tdd_plan_correct_plan_id.robot b/robot/tdd_plan_correct_plan_id.robot index d3440dc59..f8586faf6 100644 --- a/robot/tdd_plan_correct_plan_id.robot +++ b/robot/tdd_plan_correct_plan_id.robot @@ -22,7 +22,7 @@ TDD Plan Correct Accepts Plan ID As Positional Argument Revert Mode ... 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 + [Tags] 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} @@ -35,7 +35,7 @@ TDD Plan Correct Accepts Plan ID As Positional Argument Append Mode ... 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 + [Tags] 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} diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 64b669a3d..6ff6fc342 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -2642,9 +2642,11 @@ def plan_artifacts( @app.command("correct") def correct_decision( - decision_id: Annotated[ + identifier: Annotated[ str, - typer.Argument(help="Decision ID to correct"), + typer.Argument( + help="Plan ID (auto-selects root decision) or Decision ID to correct" + ), ], mode: Annotated[ str, @@ -2698,6 +2700,10 @@ def correct_decision( ) -> None: """Correct a decision in a plan's decision tree. + The positional *identifier* can be either a **plan ID** or a + **decision ID**. When a plan ID is given the root decision of that + plan is automatically selected as the correction target. + Supports two modes: * **revert** -- undo a decision and recompute affected subtrees @@ -2707,6 +2713,7 @@ def correct_decision( Examples:: + agents plan correct --mode revert -g "Use FastAPI instead" PLAN-001 agents plan correct --mode revert -g "Use FastAPI instead" DEC-001 agents plan correct --mode append -g "Add caching layer" --dry-run DEC-002 """ @@ -2731,16 +2738,42 @@ def correct_decision( console.print("[red]Error:[/red] --guidance / -g must not be blank.") raise typer.Abort() - # Resolve plan_id - resolved_plan_id = plan_id or _resolve_active_plan_id() - - # Resolve DecisionService via DI to build the structural tree - # and influence DAG for affected-subtree computation (issue #606). + # Resolve identifier: try as plan_id first, then fall back to + # decision_id for backward compatibility (issue #969). from cleveragents.application.container import get_container + from cleveragents.domain.models.core.plan import Plan container = get_container() decision_svc = container.decision_service() + target_decision_id: str + resolved_plan_id: str + _is_plan = False + try: + service = container.plan_lifecycle_service() + plan_obj = service.get_plan(identifier) + if isinstance(plan_obj, Plan): + _is_plan = True + except RNF: + # Lookup failed - identifier is not a plan_id + pass + + if _is_plan: + # identifier is a valid plan_id - auto-select root decision + resolved_plan_id = identifier + decisions = decision_svc.list_decisions(resolved_plan_id) + root_decisions = [d for d in decisions if d.parent_decision_id is None] + if not root_decisions: + console.print( + f"[red]Error:[/red] Plan '{identifier}' has no root decision." + ) + raise typer.Abort() + target_decision_id = root_decisions[0].decision_id + else: + # Not a plan_id - treat as decision_id (backward compat) + target_decision_id = identifier + resolved_plan_id = plan_id or _resolve_active_plan_id() + # Build structural tree adjacency list (parent -> children) decisions = decision_svc.list_decisions(resolved_plan_id) decision_tree: dict[str, list[str]] = {} @@ -2756,7 +2789,7 @@ def correct_decision( # Create the correction request request = svc.request_correction( plan_id=resolved_plan_id, - target_decision_id=decision_id, + target_decision_id=target_decision_id, mode=correction_mode, guidance=guidance, dry_run=dry_run, @@ -2805,7 +2838,7 @@ def correct_decision( if not yes: console.print( f"\n[bold]Correction:[/bold] {correction_mode.value} " - f"decision {decision_id}" + f"decision {target_decision_id}" ) console.print(f"[bold]Guidance:[/bold] {guidance}") confirm = typer.confirm("\nProceed with correction?") -- 2.52.0