From e584f57f413fa859181f40f1d67a4d37d7328068 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 8 Mar 2026 02:12:04 +0000 Subject: [PATCH 1/2] fix(cli): pass decision tree and influence edges to CorrectionService in plan correct handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve DecisionService via the DI container in correct_decision() to build the structural tree (parent → children adjacency list) and fetch influence DAG edges, then forward both to CorrectionService.analyze_impact() and CorrectionService.execute_correction(). Previously these methods received None for both arguments, causing _compute_affected_subtree() BFS to return only the single target decision instead of the full affected subtree. Changes: - src/cleveragents/cli/commands/plan.py: Added DI container resolution of DecisionService, structural tree construction from list_decisions(), and influence edge retrieval via get_influence_edges(). Both are passed as keyword arguments to analyze_impact() and execute_correction(). - features/plan_correct_tree_wiring.feature: 3 BDD scenarios verifying tree/edge forwarding for dry-run, execution, and leaf-node cases. - features/steps/plan_correct_tree_wiring_steps.py: Step definitions with mock container and argument-capturing CorrectionService. - robot/plan_correct_tree_wiring.robot: Integration smoke tests. - robot/helper_plan_correct_tree_wiring.py: Robot helper exercising CLI via CliRunner with mock patches. - benchmarks/plan_correct_tree_wiring_bench.py: ASV benchmarks for tree building and analyze_impact overhead. - Updated existing test steps (m3, m4, pec, uncov-rgn) to mock the DI container so existing correction scenarios continue to pass. ISSUES CLOSED: #606 --- benchmarks/plan_correct_tree_wiring_bench.py | 108 ++++++ features/plan_correct_tree_wiring.feature | 31 ++ .../m3_decision_validation_smoke_steps.py | 13 + .../m4_correction_subplan_smoke_steps.py | 14 +- ...lan_cli_uncovered_region_coverage_steps.py | 17 +- .../steps/plan_correct_tree_wiring_steps.py | 324 ++++++++++++++++++ .../steps/plan_explain_cli_coverage_steps.py | 17 +- robot/helper_plan_correct_tree_wiring.py | 222 ++++++++++++ robot/plan_correct_tree_wiring.robot | 27 ++ src/cleveragents/cli/commands/plan.py | 32 +- 10 files changed, 796 insertions(+), 9 deletions(-) create mode 100644 benchmarks/plan_correct_tree_wiring_bench.py create mode 100644 features/plan_correct_tree_wiring.feature create mode 100644 features/steps/plan_correct_tree_wiring_steps.py create mode 100644 robot/helper_plan_correct_tree_wiring.py create mode 100644 robot/plan_correct_tree_wiring.robot diff --git a/benchmarks/plan_correct_tree_wiring_bench.py b/benchmarks/plan_correct_tree_wiring_bench.py new file mode 100644 index 000000000..19200e731 --- /dev/null +++ b/benchmarks/plan_correct_tree_wiring_bench.py @@ -0,0 +1,108 @@ +"""ASV benchmarks for plan correct tree wiring (issue #606). + +Measures overhead of building the decision tree adjacency list and +influence DAG edges from DecisionService output, and passing them +through to CorrectionService.analyze_impact() and +CorrectionService.execute_correction(). +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from types import SimpleNamespace + +try: + from cleveragents.application.services.correction_service import CorrectionService + from cleveragents.domain.models.core.correction import CorrectionMode +except ModuleNotFoundError: + sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) + from cleveragents.application.services.correction_service import CorrectionService + from cleveragents.domain.models.core.correction import CorrectionMode + +_PLAN_ID = "01BENCH_PLAN_000000000000001" +_ROOT = "DEC-BENCH-ROOT" + + +def _make_decision(decision_id: str, parent_decision_id: str | None) -> SimpleNamespace: + """Create a minimal decision-like namespace.""" + return SimpleNamespace( + decision_id=decision_id, + parent_decision_id=parent_decision_id, + ) + + +def _build_chain(depth: int) -> list[SimpleNamespace]: + """Build a linear chain of decisions.""" + decisions: list[SimpleNamespace] = [_make_decision(_ROOT, None)] + for i in range(1, depth): + decisions.append(_make_decision(f"{_ROOT}_c{i}", decisions[-1].decision_id)) + return decisions + + +def _build_tree_from_decisions( + decisions: list[SimpleNamespace], +) -> dict[str, list[str]]: + """Replicate the tree-building logic from correct_decision().""" + tree: dict[str, list[str]] = {} + for d in decisions: + if d.parent_decision_id is not None: + tree.setdefault(d.parent_decision_id, []).append(d.decision_id) + return tree + + +# --------------------------------------------------------------------------- +# Benchmarks +# --------------------------------------------------------------------------- + + +class TreeBuildingSuite: + """Benchmark decision tree adjacency-list construction.""" + + params: list[int] = [10, 100, 500] + param_names: list[str] = ["chain_depth"] + + def setup(self, chain_depth: int) -> None: + """Prepare decision chains.""" + self.decisions = _build_chain(chain_depth) + + def time_build_tree(self, chain_depth: int) -> None: + """Time the tree-building loop from correct_decision.""" + _build_tree_from_decisions(self.decisions) + + +class AnalyzeWithTreeSuite: + """Benchmark analyze_impact when decision_tree is pre-built.""" + + params: list[int] = [10, 100, 500] + param_names: list[str] = ["chain_depth"] + + def setup(self, chain_depth: int) -> None: + """Prepare CorrectionService and tree.""" + self.svc = CorrectionService() + decisions = _build_chain(chain_depth) + self.tree = _build_tree_from_decisions(decisions) + self.edges: dict[str, list[str]] = {} + # Create a correction request for the root + self.req = self.svc.request_correction( + plan_id=_PLAN_ID, + target_decision_id=_ROOT, + mode=CorrectionMode.REVERT, + guidance="benchmark", + ) + + def time_analyze_impact_with_tree(self, chain_depth: int) -> None: + """Time analyze_impact with pre-built tree.""" + # Re-create request each iteration to avoid status issues + svc = CorrectionService() + req = svc.request_correction( + plan_id=_PLAN_ID, + target_decision_id=_ROOT, + mode=CorrectionMode.REVERT, + guidance="benchmark", + ) + svc.analyze_impact( + req.correction_id, + decision_tree=self.tree, + influence_edges=self.edges, + ) diff --git a/features/plan_correct_tree_wiring.feature b/features/plan_correct_tree_wiring.feature new file mode 100644 index 000000000..cdc9fadca --- /dev/null +++ b/features/plan_correct_tree_wiring.feature @@ -0,0 +1,31 @@ +@unit +Feature: plan correct passes decision tree and influence edges to CorrectionService + The plan correct CLI handler must resolve DecisionService via the DI + container, build the structural tree and influence DAG edges, and + forward them to CorrectionService.analyze_impact() and + CorrectionService.execute_correction() so that the BFS subtree + traversal reports the full affected subtree—not just the single + target decision. + + Fixes: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/606 + + Scenario: plan correct dry-run reports full affected subtree (not just target) + Given pctw a plan with a three-level decision tree + And pctw a CorrectionService that records analyze_impact arguments + When pctw I invoke plan correct in dry-run mode + Then pctw the dry-run output should list all three affected decisions + And pctw analyze_impact received the decision_tree and influence_edges + + Scenario: plan correct execution reverts full affected subtree + Given pctw a plan with a three-level decision tree + And pctw a CorrectionService that records execute_correction arguments + When pctw I invoke plan correct in execution mode + Then pctw the execution output should show reverted decisions + And pctw execute_correction received the decision_tree and influence_edges + + Scenario: plan correct with no children reports single decision + Given pctw a plan with a single leaf decision + And pctw a CorrectionService that records analyze_impact arguments + When pctw I invoke plan correct in dry-run mode for a leaf + Then pctw the dry-run output should list only the target decision + And pctw analyze_impact received empty tree and edges diff --git a/features/steps/m3_decision_validation_smoke_steps.py b/features/steps/m3_decision_validation_smoke_steps.py index 3b97508ce..0c205a7ce 100644 --- a/features/steps/m3_decision_validation_smoke_steps.py +++ b/features/steps/m3_decision_validation_smoke_steps.py @@ -594,6 +594,19 @@ def step_m3_plan_with_decisions(context: Context) -> None: context.correction_patcher.start() context.add_cleanup(context.correction_patcher.stop) + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + context.m3_container_patcher = patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ) + context.m3_container_patcher.start() + context.add_cleanup(context.m3_container_patcher.stop) + @when("I m3 smoke invoke plan correct in dry-run mode") def step_m3_plan_correct_dry_run(context: Context) -> None: diff --git a/features/steps/m4_correction_subplan_smoke_steps.py b/features/steps/m4_correction_subplan_smoke_steps.py index ce9020866..9a6ff8ed0 100644 --- a/features/steps/m4_correction_subplan_smoke_steps.py +++ b/features/steps/m4_correction_subplan_smoke_steps.py @@ -233,6 +233,18 @@ def step_m4_plan_with_decision_tree(context: Context) -> None: context.m4_correction_patcher.start() context.m4_mock_correction_service = mock_correction_svc + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + context.m4_container_patcher = patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ) + context.m4_container_patcher.start() + @when('I m4 smoke invoke plan correct with mode "{mode}" and guidance "{guidance}"') def step_m4_invoke_correct(context: Context, mode: str, guidance: str) -> None: @@ -572,7 +584,7 @@ def step_m4_correct_empty_decision(context: Context) -> None: def after_scenario(context: Context, scenario: object) -> None: """Clean up patchers after each scenario.""" - for name in ("m4_plan_patcher", "m4_correction_patcher"): + for name in ("m4_plan_patcher", "m4_correction_patcher", "m4_container_patcher"): patcher = getattr(context, name, None) if patcher: with contextlib.suppress(RuntimeError): diff --git a/features/steps/plan_cli_uncovered_region_coverage_steps.py b/features/steps/plan_cli_uncovered_region_coverage_steps.py index 08d38824f..1d52a5d55 100644 --- a/features/steps/plan_cli_uncovered_region_coverage_steps.py +++ b/features/steps/plan_cli_uncovered_region_coverage_steps.py @@ -50,6 +50,7 @@ _PATCH_GET_LIFECYCLE = "cleveragents.cli.commands.plan._get_lifecycle_service" _PATCH_GET_APPLY = "cleveragents.cli.commands.plan._get_apply_service" _PATCH_CORRECTION_SVC = "cleveragents.cli.commands.plan.CorrectionService" _PATCH_RESOLVE_ACTIVE = "cleveragents.cli.commands.plan._resolve_active_plan_id" +_PATCH_CONTAINER = "cleveragents.application.container.get_container" # --------------------------------------------------------------------------- @@ -495,10 +496,20 @@ def _invoke_correct( mock_correction = getattr(context, "uncov_mock_correction", MagicMock()) + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + # Patch CorrectionService to return our mock when instantiated - with patch( - "cleveragents.application.services.correction_service.CorrectionService", - return_value=mock_correction, + with ( + patch( + "cleveragents.application.services.correction_service.CorrectionService", + return_value=mock_correction, + ), + patch(_PATCH_CONTAINER, return_value=mock_container), ): context.uncov_result = context.uncov_runner.invoke(plan_app, args) diff --git a/features/steps/plan_correct_tree_wiring_steps.py b/features/steps/plan_correct_tree_wiring_steps.py new file mode 100644 index 000000000..560d1d9eb --- /dev/null +++ b/features/steps/plan_correct_tree_wiring_steps.py @@ -0,0 +1,324 @@ +"""Step definitions for plan_correct_tree_wiring.feature. + +Verifies that the ``plan correct`` CLI handler resolves the +DecisionService via the DI container, builds the structural tree +and influence DAG, and forwards them to +``CorrectionService.analyze_impact()`` and +``CorrectionService.execute_correction()``. + +All step text uses the ``pctw`` prefix to avoid collisions with +other step files. + +Fixes: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/606 +""" + +from __future__ import annotations + +from types import SimpleNamespace +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.cli.commands.plan import app as plan_app + +runner = CliRunner() + +_PATCH_CONTAINER = "cleveragents.application.container.get_container" +_PATCH_CORRECTION_SVC = ( + "cleveragents.application.services.correction_service.CorrectionService" +) + +# Fixed ULIDs for deterministic assertions +_PLAN_ID = str(ULID()) +_ROOT_ID = "DEC-ROOT-001" +_CHILD_A = "DEC-CHILD-A" +_CHILD_B = "DEC-CHILD-B" +_LEAF_ID = "DEC-LEAF-001" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_decision_ns( + decision_id: str, + parent_decision_id: str | None, +) -> SimpleNamespace: + """Create a minimal decision-like namespace for list_decisions.""" + return SimpleNamespace( + decision_id=decision_id, + parent_decision_id=parent_decision_id, + ) + + +def _make_mock_container( + decisions: list[SimpleNamespace], + influence_edges: dict[str, list[str]], +) -> MagicMock: + """Build a mock DI container whose resolve() returns a DecisionService.""" + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = decisions + mock_decision_svc.get_influence_edges.return_value = influence_edges + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + return mock_container + + +# --------------------------------------------------------------------------- +# GIVEN — three-level tree +# --------------------------------------------------------------------------- + + +@given("pctw a plan with a three-level decision tree") +def step_pctw_three_level_tree(context: Context) -> None: + """Set up a mock DecisionService returning a three-level tree.""" + decisions = [ + _make_decision_ns(_ROOT_ID, None), + _make_decision_ns(_CHILD_A, _ROOT_ID), + _make_decision_ns(_CHILD_B, _ROOT_ID), + ] + influence_edges: dict[str, list[str]] = {_ROOT_ID: [_CHILD_A]} + context.pctw_mock_container = _make_mock_container(decisions, influence_edges) + context.pctw_plan_id = _PLAN_ID + context.pctw_target_id = _ROOT_ID + + +@given("pctw a plan with a single leaf decision") +def step_pctw_single_leaf(context: Context) -> None: + """Set up a mock DecisionService returning a lone leaf decision.""" + decisions = [_make_decision_ns(_LEAF_ID, None)] + context.pctw_mock_container = _make_mock_container(decisions, {}) + context.pctw_plan_id = _PLAN_ID + context.pctw_target_id = _LEAF_ID + + +# --------------------------------------------------------------------------- +# GIVEN — CorrectionService with argument capture +# --------------------------------------------------------------------------- + + +def _make_capturing_correction_svc( + *, + capture_analyze: bool = False, + capture_execute: bool = False, + affected_decisions: list[str] | None = None, + target_decision_id: str = _ROOT_ID, +) -> MagicMock: + """Build a mock CorrectionService that records keyword arguments.""" + svc = MagicMock() + + # request_correction always returns a request-like object + svc.request_correction.return_value = SimpleNamespace( + correction_id="CORR-PCTW-01", + mode=SimpleNamespace(value="revert"), + target_decision_id=target_decision_id, + guidance="Fix this", + ) + + affected = affected_decisions or [_ROOT_ID, _CHILD_A, _CHILD_B] + if capture_analyze: + impact = SimpleNamespace( + affected_decisions=affected, + affected_files=[f"{d}.py" for d in affected], + estimated_cost=float(len(affected)) * 1.5, + risk_level="low", + ) + svc.analyze_impact.return_value = impact + + if capture_execute: + result = SimpleNamespace( + correction_id="CORR-PCTW-01", + status=SimpleNamespace(value="applied"), + reverted_decisions=affected, + new_decisions=[], + ) + svc.execute_correction.return_value = result + + return svc + + +@given("pctw a CorrectionService that records analyze_impact arguments") +def step_pctw_capture_analyze(context: Context) -> None: + """Mock CorrectionService recording analyze_impact kwargs.""" + context.pctw_correction_svc = _make_capturing_correction_svc( + capture_analyze=True, + ) + + +@given("pctw a CorrectionService that records execute_correction arguments") +def step_pctw_capture_execute(context: Context) -> None: + """Mock CorrectionService recording execute_correction kwargs.""" + context.pctw_correction_svc = _make_capturing_correction_svc( + capture_execute=True, + affected_decisions=[_ROOT_ID, _CHILD_A, _CHILD_B], + ) + + +# --------------------------------------------------------------------------- +# WHEN — invoke correct +# --------------------------------------------------------------------------- + + +def _invoke( + context: Context, + *, + dry_run: bool = False, + target_id: str | None = None, +) -> None: + """Invoke plan correct with appropriate patches.""" + tid = target_id or context.pctw_target_id + args = [ + "correct", + tid, + "--mode", + "revert", + "--guidance", + "Fix this", + "--plan", + context.pctw_plan_id, + ] + if dry_run: + args.append("--dry-run") + else: + args.append("--yes") + + with ( + patch(_PATCH_CORRECTION_SVC, return_value=context.pctw_correction_svc), + patch(_PATCH_CONTAINER, return_value=context.pctw_mock_container), + ): + context.pctw_result = runner.invoke(plan_app, args) + + +@when("pctw I invoke plan correct in dry-run mode") +def step_pctw_invoke_dry_run(context: Context) -> None: + """Invoke plan correct with --dry-run.""" + _invoke(context, dry_run=True) + + +@when("pctw I invoke plan correct in execution mode") +def step_pctw_invoke_execute(context: Context) -> None: + """Invoke plan correct with --yes.""" + _invoke(context, dry_run=False) + + +@when("pctw I invoke plan correct in dry-run mode for a leaf") +def step_pctw_invoke_dry_run_leaf(context: Context) -> None: + """Invoke plan correct for a single leaf decision.""" + # Override with single-decision impact and correct target id + context.pctw_correction_svc = _make_capturing_correction_svc( + capture_analyze=True, + affected_decisions=[_LEAF_ID], + target_decision_id=_LEAF_ID, + ) + _invoke(context, dry_run=True, target_id=_LEAF_ID) + + +# --------------------------------------------------------------------------- +# THEN — output assertions +# --------------------------------------------------------------------------- + + +@then("pctw the dry-run output should list all three affected decisions") +def step_pctw_dryrun_all_three(context: Context) -> None: + """Verify the dry-run output mentions all three decisions.""" + output = context.pctw_result.output + assert context.pctw_result.exit_code == 0, ( + f"Expected exit 0, got {context.pctw_result.exit_code}. Output: {output}" + ) + for did in (_ROOT_ID, _CHILD_A, _CHILD_B): + assert did in output, f"Expected '{did}' in output: {output}" + + +@then("pctw the execution output should show reverted decisions") +def step_pctw_exec_shows_reverted(context: Context) -> None: + """Verify the execution output mentions reverted decisions.""" + output = context.pctw_result.output + assert context.pctw_result.exit_code == 0, ( + f"Expected exit 0, got {context.pctw_result.exit_code}. Output: {output}" + ) + assert "Correction applied" in output or "applied" in output.lower(), ( + f"Expected 'applied' in output: {output}" + ) + + +@then("pctw the dry-run output should list only the target decision") +def step_pctw_dryrun_single(context: Context) -> None: + """Verify only the target decision appears in dry-run output.""" + output = context.pctw_result.output + assert context.pctw_result.exit_code == 0, ( + f"Expected exit 0, got {context.pctw_result.exit_code}. Output: {output}" + ) + assert _LEAF_ID in output, f"Expected '{_LEAF_ID}' in output: {output}" + assert _ROOT_ID not in output, f"Did NOT expect '{_ROOT_ID}' in output: {output}" + + +# --------------------------------------------------------------------------- +# THEN — argument forwarding assertions +# --------------------------------------------------------------------------- + + +@then("pctw analyze_impact received the decision_tree and influence_edges") +def step_pctw_analyze_got_tree(context: Context) -> None: + """Verify analyze_impact was called with decision_tree and influence_edges.""" + svc = context.pctw_correction_svc + svc.analyze_impact.assert_called_once() + call_kwargs = svc.analyze_impact.call_args + # keyword arguments + kw = call_kwargs.kwargs if call_kwargs.kwargs else {} + # Also check positional-keyword mix via call_args[1] + if not kw: + kw = call_kwargs[1] if len(call_kwargs) > 1 else {} + assert "decision_tree" in kw, ( + f"Expected 'decision_tree' kwarg in analyze_impact call. Got: {call_kwargs}" + ) + assert "influence_edges" in kw, ( + f"Expected 'influence_edges' kwarg in analyze_impact call. Got: {call_kwargs}" + ) + # Verify the tree contains our parent->children mapping + tree = kw["decision_tree"] + assert _ROOT_ID in tree, f"Expected '{_ROOT_ID}' key in tree: {tree}" + assert _CHILD_A in tree[_ROOT_ID], ( + f"Expected '{_CHILD_A}' in tree['{_ROOT_ID}']: {tree}" + ) + assert _CHILD_B in tree[_ROOT_ID], ( + f"Expected '{_CHILD_B}' in tree['{_ROOT_ID}']: {tree}" + ) + + +@then("pctw execute_correction received the decision_tree and influence_edges") +def step_pctw_execute_got_tree(context: Context) -> None: + """Verify execute_correction was called with decision_tree and influence_edges.""" + svc = context.pctw_correction_svc + svc.execute_correction.assert_called_once() + call_kwargs = svc.execute_correction.call_args + kw = call_kwargs.kwargs if call_kwargs.kwargs else {} + if not kw: + kw = call_kwargs[1] if len(call_kwargs) > 1 else {} + assert "decision_tree" in kw, ( + f"Expected 'decision_tree' kwarg in execute_correction call. Got: {call_kwargs}" + ) + assert "influence_edges" in kw, ( + f"Expected 'influence_edges' kwarg in execute_correction call. " + f"Got: {call_kwargs}" + ) + + +@then("pctw analyze_impact received empty tree and edges") +def step_pctw_analyze_empty_tree(context: Context) -> None: + """Verify analyze_impact was called with empty tree for a leaf.""" + svc = context.pctw_correction_svc + svc.analyze_impact.assert_called_once() + call_kwargs = svc.analyze_impact.call_args + kw = call_kwargs.kwargs if call_kwargs.kwargs else {} + if not kw: + kw = call_kwargs[1] if len(call_kwargs) > 1 else {} + assert kw.get("decision_tree") == {}, ( + f"Expected empty decision_tree, got: {kw.get('decision_tree')}" + ) + assert kw.get("influence_edges") == {}, ( + f"Expected empty influence_edges, got: {kw.get('influence_edges')}" + ) diff --git a/features/steps/plan_explain_cli_coverage_steps.py b/features/steps/plan_explain_cli_coverage_steps.py index e71223ce9..4a36e132f 100644 --- a/features/steps/plan_explain_cli_coverage_steps.py +++ b/features/steps/plan_explain_cli_coverage_steps.py @@ -646,9 +646,20 @@ def _invoke_correct( ] if extra_args: args.extend(extra_args) - with patch( - "cleveragents.application.services.correction_service.CorrectionService", - return_value=context.pec_correction_svc, + + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + + with ( + patch( + "cleveragents.application.services.correction_service.CorrectionService", + return_value=context.pec_correction_svc, + ), + patch(_PATCH_CONTAINER, return_value=mock_container), ): context.pec_result = runner.invoke(plan_app, args, input=input_text) diff --git a/robot/helper_plan_correct_tree_wiring.py b/robot/helper_plan_correct_tree_wiring.py new file mode 100644 index 000000000..d3cc2bfc8 --- /dev/null +++ b/robot/helper_plan_correct_tree_wiring.py @@ -0,0 +1,222 @@ +"""Helper script for Robot Framework plan correct tree wiring tests. + +Verifies that ``correct_decision()`` passes ``decision_tree`` and +``influence_edges`` to ``CorrectionService.analyze_impact()`` and +``CorrectionService.execute_correction()`` when invoked via the CLI. + +Fixes: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/606 +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +# Ensure src is importable when run from workspace root +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) + +from typer.testing import CliRunner + +from cleveragents.cli.commands.plan import app as plan_app + +runner = CliRunner() + +_PATCH_CONTAINER = "cleveragents.application.container.get_container" +_PATCH_CORRECTION_SVC = ( + "cleveragents.application.services.correction_service.CorrectionService" +) + +_PLAN_ID = "01ROBOT_PLAN_ID_000000000001" +_ROOT_ID = "DEC-ROOT-R01" +_CHILD_A = "DEC-CHILD-RA" +_CHILD_B = "DEC-CHILD-RB" +_LEAF_ID = "DEC-LEAF-R01" + + +def _make_decision(decision_id: str, parent_decision_id: str | None) -> SimpleNamespace: + """Create a minimal decision-like namespace.""" + return SimpleNamespace( + decision_id=decision_id, + parent_decision_id=parent_decision_id, + ) + + +def _build_container( + decisions: list[SimpleNamespace], + influence_edges: dict[str, list[str]], +) -> MagicMock: + """Build a mock DI container returning a mock DecisionService.""" + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = decisions + mock_decision_svc.get_influence_edges.return_value = influence_edges + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + return mock_container + + +def _build_correction_svc( + *, + analyze_affected: list[str] | None = None, + execute_affected: list[str] | None = None, +) -> MagicMock: + """Build a mock CorrectionService.""" + svc = MagicMock() + svc.request_correction.return_value = SimpleNamespace( + correction_id="CORR-ROBOT-01", + mode=SimpleNamespace(value="revert"), + target_decision_id=_ROOT_ID, + guidance="Fix this", + ) + if analyze_affected is not None: + svc.analyze_impact.return_value = SimpleNamespace( + affected_decisions=analyze_affected, + affected_files=[f"{d}.py" for d in analyze_affected], + estimated_cost=float(len(analyze_affected)) * 1.5, + risk_level="low", + ) + if execute_affected is not None: + svc.execute_correction.return_value = SimpleNamespace( + correction_id="CORR-ROBOT-01", + status=SimpleNamespace(value="applied"), + reverted_decisions=execute_affected, + new_decisions=[], + ) + return svc + + +# --------------------------------------------------------------------------- +# Test functions +# --------------------------------------------------------------------------- + + +def _test_dry_run_tree() -> None: + """Verify analyze_impact receives decision_tree and influence_edges.""" + decisions = [ + _make_decision(_ROOT_ID, None), + _make_decision(_CHILD_A, _ROOT_ID), + _make_decision(_CHILD_B, _ROOT_ID), + ] + influence_edges: dict[str, list[str]] = {_ROOT_ID: [_CHILD_A]} + container = _build_container(decisions, influence_edges) + correction_svc = _build_correction_svc( + analyze_affected=[_ROOT_ID, _CHILD_A, _CHILD_B], + ) + + args = [ + "correct", + _ROOT_ID, + "--mode", + "revert", + "--guidance", + "Fix this", + "--plan", + _PLAN_ID, + "--dry-run", + ] + with ( + patch(_PATCH_CORRECTION_SVC, return_value=correction_svc), + patch(_PATCH_CONTAINER, return_value=container), + ): + result = runner.invoke(plan_app, args) + + assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}" + correction_svc.analyze_impact.assert_called_once() + kw = correction_svc.analyze_impact.call_args.kwargs + assert "decision_tree" in kw, f"Missing decision_tree: {kw}" + assert "influence_edges" in kw, f"Missing influence_edges: {kw}" + assert _ROOT_ID in kw["decision_tree"] + print("plan-correct-tree-wiring-dry-run-ok") + + +def _test_execute_tree() -> None: + """Verify execute_correction receives decision_tree and influence_edges.""" + decisions = [ + _make_decision(_ROOT_ID, None), + _make_decision(_CHILD_A, _ROOT_ID), + _make_decision(_CHILD_B, _ROOT_ID), + ] + influence_edges: dict[str, list[str]] = {_ROOT_ID: [_CHILD_A]} + container = _build_container(decisions, influence_edges) + correction_svc = _build_correction_svc( + execute_affected=[_ROOT_ID, _CHILD_A, _CHILD_B], + ) + + args = [ + "correct", + _ROOT_ID, + "--mode", + "revert", + "--guidance", + "Fix this", + "--plan", + _PLAN_ID, + "--yes", + ] + with ( + patch(_PATCH_CORRECTION_SVC, return_value=correction_svc), + patch(_PATCH_CONTAINER, return_value=container), + ): + result = runner.invoke(plan_app, args) + + assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}" + correction_svc.execute_correction.assert_called_once() + kw = correction_svc.execute_correction.call_args.kwargs + assert "decision_tree" in kw, f"Missing decision_tree: {kw}" + assert "influence_edges" in kw, f"Missing influence_edges: {kw}" + print("plan-correct-tree-wiring-execute-ok") + + +def _test_leaf_empty() -> None: + """Verify a leaf decision produces empty tree/edges.""" + decisions = [_make_decision(_LEAF_ID, None)] + container = _build_container(decisions, {}) + correction_svc = _build_correction_svc( + analyze_affected=[_LEAF_ID], + ) + + args = [ + "correct", + _LEAF_ID, + "--mode", + "revert", + "--guidance", + "Fix this", + "--plan", + _PLAN_ID, + "--dry-run", + ] + with ( + patch(_PATCH_CORRECTION_SVC, return_value=correction_svc), + patch(_PATCH_CONTAINER, return_value=container), + ): + result = runner.invoke(plan_app, args) + + assert result.exit_code == 0, f"Exit {result.exit_code}: {result.output}" + kw = correction_svc.analyze_impact.call_args.kwargs + assert kw["decision_tree"] == {}, f"Expected empty tree: {kw}" + assert kw["influence_edges"] == {}, f"Expected empty edges: {kw}" + print("plan-correct-tree-wiring-leaf-ok") + + +# --------------------------------------------------------------------------- +# Dispatch +# --------------------------------------------------------------------------- + +_TESTS: dict[str, Callable[[], None]] = { + "dry_run_tree": _test_dry_run_tree, + "execute_tree": _test_execute_tree, + "leaf_empty": _test_leaf_empty, +} + +if __name__ == "__main__": + if len(sys.argv) < 2: + print(f"Usage: {sys.argv[0]} <{'|'.join(_TESTS)}>") + sys.exit(1) + test_name = sys.argv[1] + if test_name not in _TESTS: + print(f"Unknown test: {test_name}. Available: {list(_TESTS)}") + sys.exit(1) + _TESTS[test_name]() diff --git a/robot/plan_correct_tree_wiring.robot b/robot/plan_correct_tree_wiring.robot new file mode 100644 index 000000000..39594114c --- /dev/null +++ b/robot/plan_correct_tree_wiring.robot @@ -0,0 +1,27 @@ +*** Settings *** +Documentation Smoke tests for plan correct decision-tree wiring (issue #606) +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER_SCRIPT} robot/helper_plan_correct_tree_wiring.py + +*** Test Cases *** +Plan Correct Dry Run Passes Tree To CorrectionService + [Documentation] Verify analyze_impact receives decision_tree and influence_edges + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} dry_run_tree cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} plan-correct-tree-wiring-dry-run-ok + +Plan Correct Execution Passes Tree To CorrectionService + [Documentation] Verify execute_correction receives decision_tree and influence_edges + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} execute_tree cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} plan-correct-tree-wiring-execute-ok + +Plan Correct Single Leaf Passes Empty Tree + [Documentation] Verify a leaf decision produces empty tree/edges + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} leaf_empty cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} plan-correct-tree-wiring-leaf-ok diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index a0e28e5e9..f06d87803 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -2406,6 +2406,26 @@ def correct_decision( # 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). + from cleveragents.application.container import get_container + from cleveragents.application.services.decision_service import ( + DecisionService as _DS, + ) + + container = get_container() + decision_svc: DecisionService = container.resolve(_DS) + + # 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: + if d.parent_decision_id is not None: + decision_tree.setdefault(d.parent_decision_id, []).append(d.decision_id) + + # Fetch influence DAG edges + influence_edges = decision_svc.get_influence_edges(resolved_plan_id) + svc = CorrectionService() # Create the correction request @@ -2419,7 +2439,11 @@ def correct_decision( if dry_run: # Analyze and display impact - impact = svc.analyze_impact(request.correction_id) + impact = svc.analyze_impact( + request.correction_id, + decision_tree=decision_tree, + influence_edges=influence_edges, + ) if fmt != OutputFormat.RICH.value: data = { "correction_id": request.correction_id, @@ -2465,7 +2489,11 @@ def correct_decision( raise typer.Exit(0) # Execute the correction - result = svc.execute_correction(request.correction_id) + result = svc.execute_correction( + request.correction_id, + decision_tree=decision_tree, + influence_edges=influence_edges, + ) if fmt != OutputFormat.RICH.value: data = { -- 2.52.0 From 652236450390e9d8f18a08c3753e4bf23f58f9a0 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 8 Mar 2026 22:35:45 +0000 Subject: [PATCH 2/2] test(robot): mock get_container in Robot helpers for plan correct tree wiring The production fix in plan.py now resolves DecisionService via the DI container to build the structural tree and influence edges. The Behave step definitions were updated but three Robot Framework helper scripts were missed, causing 7 test failures (exit code 1) because get_container was called without a mock. - helper_m4_correction_subplan_smoke: add get_container mock to correction_revert, correction_append, correction_dry_run, full_flow - helper_m3_decision_validation_smoke: add get_container mock to plan_correct_dry_run - helper_m3_e2e_verification: add get_container mock and update analyze_impact/execute_correction assertions to include the new decision_tree and influence_edges keyword arguments --- robot/helper_m3_decision_validation_smoke.py | 19 ++++++-- robot/helper_m3_e2e_verification.py | 50 ++++++++++++++++---- robot/helper_m4_correction_subplan_smoke.py | 26 ++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/robot/helper_m3_decision_validation_smoke.py b/robot/helper_m3_decision_validation_smoke.py index b5510887e..7dba8dc66 100644 --- a/robot/helper_m3_decision_validation_smoke.py +++ b/robot/helper_m3_decision_validation_smoke.py @@ -272,9 +272,22 @@ def plan_correct_dry_run() -> None: mock_correction_svc.request_correction.return_value = mock_request mock_correction_svc.analyze_impact.return_value = mock_impact - with patch( - "cleveragents.application.services.correction_service.CorrectionService", - return_value=mock_correction_svc, + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + + with ( + patch( + "cleveragents.application.services.correction_service.CorrectionService", + return_value=mock_correction_svc, + ), + patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ), ): result = runner.invoke( plan_app, diff --git a/robot/helper_m3_e2e_verification.py b/robot/helper_m3_e2e_verification.py index faf6b1d34..fe8996654 100644 --- a/robot/helper_m3_e2e_verification.py +++ b/robot/helper_m3_e2e_verification.py @@ -542,9 +542,22 @@ def correction_dry_run() -> None: mock_service.request_correction.return_value = mock_request mock_service.analyze_impact.return_value = mock_impact - with patch( - "cleveragents.application.services.correction_service.CorrectionService", - return_value=mock_service, + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + + with ( + patch( + "cleveragents.application.services.correction_service.CorrectionService", + return_value=mock_service, + ), + patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ), ): result = cli_runner.invoke( plan_app, @@ -581,7 +594,11 @@ def correction_dry_run() -> None: guidance="Use session cookies instead of JWT", dry_run=True, ) - mock_service.analyze_impact.assert_called_once_with(mock_request.correction_id) + mock_service.analyze_impact.assert_called_once_with( + mock_request.correction_id, + decision_tree={}, + influence_edges={}, + ) mock_service.execute_correction.assert_not_called() print("m3-correction-dry-run-ok") @@ -643,9 +660,22 @@ def correction_live_revert() -> None: mock_service.request_correction.return_value = mock_request mock_service.execute_correction.return_value = mock_result - with patch( - "cleveragents.application.services.correction_service.CorrectionService", - return_value=mock_service, + # Mock DecisionService resolved via DI container (issue #606 fix) + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + mock_container = MagicMock() + mock_container.resolve.return_value = mock_decision_svc + + with ( + patch( + "cleveragents.application.services.correction_service.CorrectionService", + return_value=mock_service, + ), + patch( + "cleveragents.application.container.get_container", + return_value=mock_container, + ), ): cli_result = cli_runner.invoke( plan_app, @@ -683,7 +713,11 @@ def correction_live_revert() -> None: guidance="Switch auth from JWT to session cookies", dry_run=False, ) - mock_service.execute_correction.assert_called_once_with(mock_request.correction_id) + mock_service.execute_correction.assert_called_once_with( + mock_request.correction_id, + decision_tree={}, + influence_edges={}, + ) print("m3-correction-live-revert-ok") diff --git a/robot/helper_m4_correction_subplan_smoke.py b/robot/helper_m4_correction_subplan_smoke.py index 3a209817d..2844efe49 100644 --- a/robot/helper_m4_correction_subplan_smoke.py +++ b/robot/helper_m4_correction_subplan_smoke.py @@ -111,6 +111,16 @@ def _mock_correction_service() -> MagicMock: # --------------------------------------------------------------------------- +def _mock_container() -> MagicMock: + """Create a mock DI container that resolves a stub DecisionService.""" + mock_decision_svc = MagicMock() + mock_decision_svc.list_decisions.return_value = [] + mock_decision_svc.get_influence_edges.return_value = {} + container = MagicMock() + container.resolve.return_value = mock_decision_svc + return container + + def correction_revert() -> None: """Invoke plan correct --mode revert.""" mock_svc = MagicMock() @@ -126,6 +136,10 @@ def correction_revert() -> None: "cleveragents.application.services.correction_service.CorrectionService", return_value=mock_correction, ), + patch( + "cleveragents.application.container.get_container", + return_value=_mock_container(), + ), ): result = runner.invoke( plan_app, @@ -169,6 +183,10 @@ def correction_append() -> None: "cleveragents.application.services.correction_service.CorrectionService", return_value=mock_correction, ), + patch( + "cleveragents.application.container.get_container", + return_value=_mock_container(), + ), ): result = runner.invoke( plan_app, @@ -206,6 +224,10 @@ def correction_dry_run() -> None: "cleveragents.application.services.correction_service.CorrectionService", return_value=mock_correction, ), + patch( + "cleveragents.application.container.get_container", + return_value=_mock_container(), + ), ): result = runner.invoke( plan_app, @@ -373,6 +395,10 @@ def full_flow() -> None: "cleveragents.application.services.correction_service.CorrectionService", return_value=mock_correction, ), + patch( + "cleveragents.application.container.get_container", + return_value=_mock_container(), + ), ): r1 = runner.invoke( plan_app, -- 2.52.0