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_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, 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 = {