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/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 90b99e1b3..ee6362010 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -2671,9 +2671,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, @@ -2727,6 +2729,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 @@ -2736,6 +2742,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 """ @@ -2760,29 +2767,44 @@ 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() - # Build structural tree adjacency list (parent -> children) - decisions = decision_svc.list_decisions(resolved_plan_id) + 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 - # Bug #969: detect if decision_id is actually a plan_id - known_decision_ids = {d.decision_id for d in decisions} - target_decision_id = decision_id - if target_decision_id not in known_decision_ids: - # Treat it as a plan_id: re-resolve plan and find root decision - resolved_plan_id = target_decision_id + 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 root_decisions: - target_decision_id = root_decisions[0].decision_id + 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]] = {} for d in decisions: @@ -2846,7 +2868,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?")