diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a0f04bfe..126949442 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,56 @@ hides them from `resource add` scaffolding (user_addable: false). Includes YAML configurations, Behave BDD tests (47 scenarios), Robot Framework integration tests, ASV benchmarks, and reference documentation. (#331) +- Enhanced `CorrectionService` subtree isolation: `analyze_impact()` now + populates `excluded_decisions` and `rollback_tier_depth`; added + `compute_rollback_tier()`, `validate_subtree_isolation()`, and dry-run + report enhancements with tier-0 root-targeted warnings. Fixed status + state-machine regression in `execute_revert()` where `analyze_impact()` + overwrote status back to ANALYZING; `execute_revert()` now transitions + through ANALYZING before EXECUTING for correct lifecycle ordering. + Fixed `validate_subtree_isolation()` to check structural-only BFS for + sibling invariant so influence-DAG-caused sibling reachability is not + misreported as a violation. Fixed false-positive cycle-detection warnings + from convergent (diamond) topologies by using a global enqueued set in + BFS instead of per-node seen_this_round. Added `dry_run` enforcement + guard in `_assert_executable()` to prevent execution of dry-run-only + corrections per spec (§ plan correct --dry-run). Added terminal-state + guard in `analyze_impact()` to reject re-analysis after execution. + Added mode validation in `execute_revert()`/`execute_append()` to + prevent mode-mismatched execution. Fixed `generate_dry_run_report()` + to preserve request status (dry-run is non-mutating). Fixed tier-0 + warning to only trigger when target is genuinely in the structural + tree. Fixed `_collect_all_decisions()` to always include the target + decision in the universe. Improved cycle-detection log message + accuracy. Extracted cost/time estimation constants. Fixed + `generate_dry_run_report()` to use try/finally for status restoration + so that an exception during `analyze_impact()` does not leave the + request stuck in ANALYZING status. Promoted terminal-status set to + a module-level `_TERMINAL_STATUSES` frozenset constant. Review-cycle + fixes: fixed `generate_dry_run_report()` to also restore `_impacts` + dict (not only status) so dry-run is fully non-mutating; fixed tier-0 + warning to compare against the actual root via `_find_root()` rather + than key-in-tree heuristic, preventing false warnings for forest + topologies; fixed BFS cycle detection to log at enqueue time so the + warning is reachable (previously dead code due to redundant visited + vs enqueued sets); added `_EXECUTABLE_STATUSES` module-level + frozenset for `_assert_executable()` and `cancel_correction()`; + added `_MAX_TREE_NODES` guard in `analyze_impact()` to reject + pathologically large inputs; `execute_revert()` now reuses cached + impact from `_impacts` when available to avoid redundant O(V+E) + recomputation; `execute_append()` now transitions through ANALYZING + before EXECUTING for consistent lifecycle across both modes; event + emission (`_emit_correction_applied`) now includes `attempt_id` and + logs failures at error level; added `max_length=10000` to + `CorrectionRequest.guidance` field; moved status transition after + attempt creation in both execution paths. Includes Behave BDD + scenarios (influence DAG, append mode, negative isolation validation, + dry-run enforcement, execute-revert end-to-end, status guard, mode + mismatch, single-node tree, terminal state guard, exact-match + affected count, convergent diamond DAG topology, dry-run exception + recovery, execute-revert with influence edges, DAG-only nodes in + excluded set), Robot Framework integration tests, and updated + dry-run report model fields. (#845) - Added deferred physical resource types for git object taxonomy (`git`, `git-remote`, `git-branch`, `git-tag`, `git-commit`, `git-tree`, `git-tree-entry`, `git-stash`, `git-submodule`) and filesystem link types diff --git a/features/correction_subtree_isolation.feature b/features/correction_subtree_isolation.feature new file mode 100644 index 000000000..9657e2f57 --- /dev/null +++ b/features/correction_subtree_isolation.feature @@ -0,0 +1,254 @@ +Feature: Decision correction recomputes only affected subtree + As a developer using CleverAgents correction flows + I want corrections to recompute only the affected subtree + So that sibling and ancestor decisions are preserved unchanged + + Background: + Given a subtree isolation correction service + And a subtree isolation decision tree with root "root" and children + | parent | children | + | root | A,B | + | A | C,D | + | B | E | + + # --- Leaf targeting --- + + Scenario: Correction targets leaf node - only leaf affected + When I subtree isolate target decision "D" with mode "revert" + And I subtree isolate analyze the impact + Then the subtree isolate affected decisions should be "D" + And the subtree isolate excluded decisions should contain "root" + And the subtree isolate excluded decisions should contain "A" + And the subtree isolate excluded decisions should contain "B" + And the subtree isolate excluded decisions should contain "C" + And the subtree isolate excluded decisions should contain "E" + + # --- Middle node targeting --- + + Scenario: Correction targets middle node - middle and descendants affected + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate analyze the impact + Then the subtree isolate affected decisions should contain "A" + And the subtree isolate affected decisions should contain "C" + And the subtree isolate affected decisions should contain "D" + And the subtree isolate excluded decisions should contain "root" + And the subtree isolate excluded decisions should contain "B" + And the subtree isolate excluded decisions should contain "E" + + # --- Root targeting --- + + Scenario: Correction targets root - all decisions affected with tier 0 warning + When I subtree isolate target decision "root" with mode "revert" + And I subtree isolate generate a dry-run report + Then the subtree isolate affected decisions should contain "root" + And the subtree isolate affected decisions should contain "A" + And the subtree isolate affected decisions should contain "B" + And the subtree isolate affected decisions should contain "C" + And the subtree isolate affected decisions should contain "D" + And the subtree isolate affected decisions should contain "E" + And the subtree isolate excluded decisions should be empty + And the subtree isolate rollback tier depth should be 0 + And the subtree isolate dry-run warnings should contain "Tier 0" + + # --- Rollback tier computation --- + + Scenario: Rollback tier is 0 for root + When I subtree isolate compute rollback tier for "root" + Then the subtree isolate rollback tier should be 0 + + Scenario: Rollback tier is 1 for direct child + When I subtree isolate compute rollback tier for "A" + Then the subtree isolate rollback tier should be 1 + + Scenario: Rollback tier is 1 for second direct child + When I subtree isolate compute rollback tier for "B" + Then the subtree isolate rollback tier should be 1 + + Scenario: Rollback tier is 2 for grandchild + When I subtree isolate compute rollback tier for "C" + Then the subtree isolate rollback tier should be 2 + + Scenario: Rollback tier depth 3 for deep tree + Given a subtree isolation deep tree with depth 4 + When I subtree isolate compute rollback tier for "depth_3" + Then the subtree isolate rollback tier should be 3 + + # --- Dry-run report enhancements --- + + Scenario: Dry-run report includes excluded decisions and rollback tier + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate generate a dry-run report + Then the subtree isolate dry-run report should include excluded decisions + And the subtree isolate dry-run report rollback tier depth should be 1 + And the subtree isolate dry-run report excluded decisions should contain "root" + And the subtree isolate dry-run report excluded decisions should contain "B" + And the subtree isolate dry-run report excluded decisions should contain "E" + + # --- Subtree isolation validation --- + + Scenario: Subtree isolation is valid for non-root target + When I subtree isolate validate isolation for target "A" + Then the subtree isolate isolation should be valid + + Scenario: Subtree isolation is valid for leaf target + When I subtree isolate validate isolation for target "D" + Then the subtree isolate isolation should be valid + + # --- Edge cases for coverage --- + + Scenario: Subtree isolation with empty tree returns valid + Given a subtree isolation empty tree + When I subtree isolate validate isolation for target "X" + Then the subtree isolate isolation should be valid + + Scenario: Rollback tier for unknown node is 0 + When I subtree isolate compute rollback tier for "UNKNOWN" + Then the subtree isolate rollback tier should be 0 + + Scenario: Find parent returns None for root node + When I subtree isolate check parent of "root" + Then the subtree isolate parent should be none + + # --- Influence DAG traversal --- + + Scenario: Influence edge adds extra decision to affected set + Given a subtree isolate influence edge from "D" to "E" + When I subtree isolate target decision "D" with mode "revert" + And I subtree isolate analyze the impact with influence edges + Then the subtree isolate affected decisions should contain "D" + And the subtree isolate affected decisions should contain "E" + And the subtree isolate excluded decisions should contain "root" + And the subtree isolate excluded decisions should contain "A" + And the subtree isolate excluded decisions should contain "B" + And the subtree isolate excluded decisions should contain "C" + + Scenario: Influence DAG cycle does not cause infinite loop + Given a subtree isolate influence cycle from "C" to "D" back to "C" + When I subtree isolate target decision "C" with mode "revert" + And I subtree isolate analyze the impact with influence edges + Then the subtree isolate affected decisions should contain "C" + And the subtree isolate affected decisions should contain "D" + + Scenario: Isolation validation passes when influence DAG reaches sibling + Given a subtree isolate influence edge from "D" to "E" + When I subtree isolate validate isolation for target "D" with influence edges + Then the subtree isolate isolation should be valid + + # --- Append mode --- + + Scenario: Append correction spawns child plan + When I subtree isolate target decision "A" with mode "append" + And I subtree isolate execute append correction + Then the subtree isolate append result should have a spawned child plan + And the subtree isolate append result status should be "applied" + + # --- Dry-run enforcement --- + + Scenario: Executing a dry-run revert correction raises ValidationError + When I subtree isolate target decision "A" with mode "revert" and dry_run + Then subtree isolate executing the correction should raise a dry-run error + + Scenario: Executing a dry-run append correction raises ValidationError + When I subtree isolate target decision "A" with mode "append" and dry_run + Then subtree isolate executing the correction should raise a dry-run error + + # --- Negative isolation validation --- + + Scenario: Isolation validation detects structural root contamination + Given a subtree isolation tree where child structurally reaches root + | parent | children | + | root | A | + | A | root | + When I subtree isolate validate isolation for target "A" + Then the subtree isolate isolation should be invalid + + # --- Execute revert (non-dry-run) --- + + Scenario: Execute revert correction succeeds for middle node + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate execute revert correction + Then the subtree isolate revert result status should be "applied" + And the subtree isolate revert result should contain reverted decisions "A,C,D" + And the subtree isolate revert result should have archived artifacts + + # --- Status guard enforcement --- + + Scenario: Executing an already-applied correction raises ValidationError + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate execute revert correction + Then subtree isolate executing the correction again should raise a status error + + Scenario: Executing a cancelled correction raises ValidationError + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate cancel the correction + Then subtree isolate executing the correction should raise a status error + + # --- Mode mismatch enforcement --- + + Scenario: Calling execute_revert on an append correction raises ValidationError + When I subtree isolate target decision "A" with mode "append" + Then subtree isolate calling execute_revert should raise a mode error + + Scenario: Calling execute_append on a revert correction raises ValidationError + When I subtree isolate target decision "A" with mode "revert" + Then subtree isolate calling execute_append should raise a mode error + + # --- Single-node tree edge case --- + + Scenario: Single-node tree has no excluded decisions + Given a subtree isolation single node tree + When I subtree isolate target decision "root" with mode "revert" + And I subtree isolate analyze the impact + Then the subtree isolate affected decisions should be "root" + And the subtree isolate excluded decisions should be empty + And the subtree isolate rollback tier depth should be 0 + + # --- Terminal state guard --- + + Scenario: Re-analyzing an applied correction raises ValidationError + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate execute revert correction + Then subtree isolate re-analyzing the impact should raise a terminal state error + + # --- Exact-match affected set for middle node --- + + Scenario: Middle node affected count is exactly 3 + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate analyze the impact + Then the subtree isolate affected decisions count should be 3 + + # --- Convergent (diamond) DAG topology --- + + Scenario: Convergent diamond topology does not duplicate affected nodes + Given a subtree isolate diamond influence where "C" and "D" both target "E" + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate analyze the impact with influence edges + Then the subtree isolate affected decisions should contain "C" + And the subtree isolate affected decisions should contain "D" + And the subtree isolate affected decisions should contain "E" + And the subtree isolate affected decision "E" should appear exactly once + + # --- Dry-run exception recovery --- + + Scenario: Dry-run report restores status when analyze_impact fails + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate generate a dry-run report that fails internally + Then the subtree isolate correction status should be "pending" + + # --- Execute revert with influence edges --- + + Scenario: Execute revert with influence edges includes DAG-reachable decisions + Given a subtree isolate influence edge from "D" to "E" + When I subtree isolate target decision "A" with mode "revert" + And I subtree isolate execute revert correction with influence edges + Then the subtree isolate revert result status should be "applied" + And the subtree isolate revert result should contain reverted decision "E" + + # --- DAG-only nodes in excluded set --- + + Scenario: Nodes only in influence DAG appear in excluded set + Given a subtree isolate influence edge from "X_EXT" to "Y_EXT" only in DAG + When I subtree isolate target decision "D" with mode "revert" + And I subtree isolate analyze the impact with influence edges + Then the subtree isolate excluded decisions should contain "X_EXT" + And the subtree isolate excluded decisions should contain "Y_EXT" diff --git a/features/resource_type_deferred_physical.feature b/features/resource_type_deferred_physical.feature index c2493cf90..719ae9e13 100644 --- a/features/resource_type_deferred_physical.feature +++ b/features/resource_type_deferred_physical.feature @@ -273,14 +273,3 @@ Feature: Deferred Physical Resource Types Scenario: as_cli_dict works for trigger-based auto_discovery for phys_deferred Given the built-in "devcontainer-instance" type from registry for phys_deferred Then as_cli_dict should succeed and include auto_discovery for phys_deferred - - # ── as_cli_dict regression: both auto_discovery schemas ── - - Scenario: as_cli_dict works for rules-based auto_discovery for phys_deferred - Given the built-in "git" YAML for phys_deferred - When I load the YAML via from_config for phys_deferred - Then as_cli_dict should succeed and include auto_discovery for phys_deferred - - Scenario: as_cli_dict works for trigger-based auto_discovery for phys_deferred - Given the built-in "devcontainer-instance" type from registry for phys_deferred - Then as_cli_dict should succeed and include auto_discovery for phys_deferred diff --git a/features/steps/correction_subtree_isolation_steps.py b/features/steps/correction_subtree_isolation_steps.py new file mode 100644 index 000000000..3e85b2db6 --- /dev/null +++ b/features/steps/correction_subtree_isolation_steps.py @@ -0,0 +1,678 @@ +"""Step definitions for correction subtree isolation tests. + +All step names are prefixed with ``subtree isolat`` to avoid +``AmbiguousStep`` conflicts with existing correction steps. +""" + +from __future__ import annotations + +from unittest.mock import patch + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.application.services.correction_service import CorrectionService +from cleveragents.core.exceptions import ValidationError +from cleveragents.domain.models.core.correction import CorrectionMode, CorrectionStatus + +_PLAN_ID = "01SUBTREEPLAN000000000000001" + + +def _parse_children(text: str) -> list[str]: + """Split a comma-separated children string into a list.""" + return [c.strip() for c in text.split(",") if c.strip()] + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a subtree isolation correction service") +def step_subtree_service(context: Context) -> None: + """Set up a fresh CorrectionService for subtree tests.""" + context.subtree_svc = CorrectionService() + context.subtree_tree = {} + context.subtree_impact = None + context.subtree_report = None + context.subtree_tier = None + context.subtree_isolation_valid = None + + +@given('a subtree isolation decision tree with root "{root}" and children') +def step_subtree_tree(context: Context, root: str) -> None: + """Build the decision tree from the table.""" + tree: dict[str, list[str]] = {} + for row in context.table: + parent = row["parent"] + children = _parse_children(row["children"]) + tree[parent] = children + context.subtree_tree = tree + + +@given("a subtree isolation deep tree with depth {depth:d}") +def step_subtree_deep_tree(context: Context, depth: int) -> None: + """Build a linear chain tree with given depth.""" + tree: dict[str, list[str]] = {} + for i in range(depth): + parent = f"depth_{i}" if i > 0 else "root" + child = f"depth_{i + 1}" + tree[parent] = [child] + context.subtree_tree = tree + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when('I subtree isolate target decision "{decision_id}" with mode "{mode}"') +def step_subtree_target(context: Context, decision_id: str, mode: str) -> None: + """Create a correction request targeting the given decision.""" + svc: CorrectionService = context.subtree_svc + correction_mode = CorrectionMode(mode) + req = svc.request_correction( + plan_id=_PLAN_ID, + target_decision_id=decision_id, + mode=correction_mode, + ) + context.subtree_correction_id = req.correction_id + + +@when("I subtree isolate analyze the impact") +def step_subtree_analyze(context: Context) -> None: + """Analyze impact for the current correction.""" + svc: CorrectionService = context.subtree_svc + impact = svc.analyze_impact( + context.subtree_correction_id, + context.subtree_tree, + ) + context.subtree_impact = impact + + +@when("I subtree isolate generate a dry-run report") +def step_subtree_dry_run(context: Context) -> None: + """Generate a dry-run report for the current correction.""" + svc: CorrectionService = context.subtree_svc + report = svc.generate_dry_run_report( + context.subtree_correction_id, + context.subtree_tree, + ) + context.subtree_report = report + context.subtree_impact = report.impact + + +@when('I subtree isolate compute rollback tier for "{decision_id}"') +def step_subtree_compute_tier(context: Context, decision_id: str) -> None: + """Compute the rollback tier for a decision.""" + svc: CorrectionService = context.subtree_svc + context.subtree_tier = svc.compute_rollback_tier( + decision_id, _PLAN_ID, context.subtree_tree + ) + + +@when('I subtree isolate validate isolation for target "{decision_id}"') +def step_subtree_validate(context: Context, decision_id: str) -> None: + """Validate subtree isolation for a target decision.""" + svc: CorrectionService = context.subtree_svc + context.subtree_isolation_valid = svc.validate_subtree_isolation( + decision_id, context.subtree_tree + ) + + +# --------------------------------------------------------------------------- +# Then steps: affected / excluded decisions +# --------------------------------------------------------------------------- + + +@then('the subtree isolate affected decisions should be "{expected}"') +def step_subtree_affected_exact(context: Context, expected: str) -> None: + """Check that affected decisions match exactly.""" + impact = context.subtree_impact + expected_list = _parse_children(expected) + assert impact.affected_decisions == expected_list, ( + f"Expected affected={expected_list}, got {impact.affected_decisions}" + ) + + +@then('the subtree isolate affected decisions should contain "{decision_id}"') +def step_subtree_affected_contains(context: Context, decision_id: str) -> None: + """Check that a specific decision is in the affected set.""" + impact = context.subtree_impact + assert decision_id in impact.affected_decisions, ( + f"Expected '{decision_id}' in affected={impact.affected_decisions}" + ) + + +@then('the subtree isolate excluded decisions should contain "{decision_id}"') +def step_subtree_excluded_contains(context: Context, decision_id: str) -> None: + """Check that a specific decision is in the excluded set.""" + impact = context.subtree_impact + assert decision_id in impact.excluded_decisions, ( + f"Expected '{decision_id}' in excluded={impact.excluded_decisions}" + ) + + +@then("the subtree isolate excluded decisions should be empty") +def step_subtree_excluded_empty(context: Context) -> None: + """Check that excluded decisions is empty.""" + impact = context.subtree_impact + assert len(impact.excluded_decisions) == 0, ( + f"Expected empty excluded, got {impact.excluded_decisions}" + ) + + +# --------------------------------------------------------------------------- +# Then steps: rollback tier +# --------------------------------------------------------------------------- + + +@then("the subtree isolate rollback tier depth should be {expected:d}") +def step_subtree_tier_depth(context: Context, expected: int) -> None: + """Check the rollback tier depth from impact analysis.""" + impact = context.subtree_impact + assert impact.rollback_tier_depth == expected, ( + f"Expected tier_depth={expected}, got {impact.rollback_tier_depth}" + ) + + +@then("the subtree isolate rollback tier should be {expected:d}") +def step_subtree_tier_value(context: Context, expected: int) -> None: + """Check the rollback tier from direct computation.""" + assert context.subtree_tier == expected, ( + f"Expected tier={expected}, got {context.subtree_tier}" + ) + + +# --------------------------------------------------------------------------- +# Then steps: dry-run report +# --------------------------------------------------------------------------- + + +@then('the subtree isolate dry-run warnings should contain "{text}"') +def step_subtree_warnings_contain(context: Context, text: str) -> None: + """Check that dry-run warnings contain specific text.""" + report = context.subtree_report + all_warnings = " ".join(report.warnings) + assert text in all_warnings, f"Expected '{text}' in warnings: {report.warnings}" + + +@then("the subtree isolate dry-run report should include excluded decisions") +def step_subtree_report_has_excluded(context: Context) -> None: + """Check that the dry-run report includes excluded decisions.""" + report = context.subtree_report + assert len(report.excluded_decisions) > 0, ( + f"Expected non-empty excluded_decisions, got {report.excluded_decisions}" + ) + + +@then("the subtree isolate dry-run report rollback tier depth should be {expected:d}") +def step_subtree_report_tier(context: Context, expected: int) -> None: + """Check the rollback tier depth in the dry-run report.""" + report = context.subtree_report + assert report.rollback_tier_depth == expected, ( + f"Expected report tier_depth={expected}, got {report.rollback_tier_depth}" + ) + + +@then( + 'the subtree isolate dry-run report excluded decisions should contain "{decision_id}"' +) +def step_subtree_report_excluded_contains(context: Context, decision_id: str) -> None: + """Check the dry-run report's excluded decisions list.""" + report = context.subtree_report + assert decision_id in report.excluded_decisions, ( + f"Expected '{decision_id}' in report excluded={report.excluded_decisions}" + ) + + +# --------------------------------------------------------------------------- +# Then steps: subtree isolation validation +# --------------------------------------------------------------------------- + + +@then("the subtree isolate isolation should be valid") +def step_subtree_isolation_valid(context: Context) -> None: + """Check that subtree isolation validation passed.""" + assert context.subtree_isolation_valid is True, ( + "Expected isolation to be valid, but it was not" + ) + + +# --------------------------------------------------------------------------- +# Edge-case steps for additional coverage +# --------------------------------------------------------------------------- + + +@given("a subtree isolation empty tree") +def step_subtree_empty_tree(context: Context) -> None: + """Set the decision tree to empty for edge-case testing.""" + context.subtree_tree = {} + + +@when('I subtree isolate check parent of "{decision_id}"') +def step_subtree_check_parent(context: Context, decision_id: str) -> None: + """Check the parent of a decision in the tree.""" + parent = CorrectionService._find_parent(decision_id, context.subtree_tree) + context.subtree_parent_result = parent + + +@then("the subtree isolate parent should be none") +def step_subtree_parent_none(context: Context) -> None: + """Verify that the parent lookup returned None.""" + assert context.subtree_parent_result is None, ( + f"Expected None, got {context.subtree_parent_result}" + ) + + +# --------------------------------------------------------------------------- +# Influence DAG steps +# --------------------------------------------------------------------------- + + +@given('a subtree isolate influence edge from "{src}" to "{dst}"') +def step_subtree_influence_edge(context: Context, src: str, dst: str) -> None: + """Set up a single influence edge.""" + context.subtree_influence_edges = {src: [dst]} + + +@given('a subtree isolate influence cycle from "{src}" to "{mid}" back to "{back}"') +def step_subtree_influence_cycle( + context: Context, + src: str, + mid: str, + back: str, +) -> None: + """Set up cyclic influence edges.""" + context.subtree_influence_edges = {src: [mid], mid: [back]} + + +@when("I subtree isolate analyze the impact with influence edges") +def step_subtree_analyze_with_influence(context: Context) -> None: + """Analyze impact using both tree and influence edges.""" + svc: CorrectionService = context.subtree_svc + impact = svc.analyze_impact( + context.subtree_correction_id, + context.subtree_tree, + influence_edges=context.subtree_influence_edges, + ) + context.subtree_impact = impact + + +@when( + 'I subtree isolate validate isolation for target "{decision_id}"' + " with influence edges" +) +def step_subtree_validate_with_influence( + context: Context, + decision_id: str, +) -> None: + """Validate subtree isolation with influence edges.""" + svc: CorrectionService = context.subtree_svc + context.subtree_isolation_valid = svc.validate_subtree_isolation( + decision_id, + context.subtree_tree, + influence_edges=context.subtree_influence_edges, + ) + + +# --------------------------------------------------------------------------- +# Append mode steps +# --------------------------------------------------------------------------- + + +@when("I subtree isolate execute append correction") +def step_subtree_execute_append(context: Context) -> None: + """Execute an append correction.""" + svc: CorrectionService = context.subtree_svc + result = svc.execute_append(context.subtree_correction_id) + context.subtree_append_result = result + + +@then("the subtree isolate append result should have a spawned child plan") +def step_subtree_append_has_child_plan(context: Context) -> None: + """Check that the append result has a spawned child plan.""" + result = context.subtree_append_result + assert result.spawned_child_plan_id is not None, ( + "Expected spawned_child_plan_id, got None" + ) + + +@then('the subtree isolate append result status should be "{status}"') +def step_subtree_append_status(context: Context, status: str) -> None: + """Check the append result status.""" + result = context.subtree_append_result + assert result.status.value == status, ( + f"Expected status='{status}', got '{result.status.value}'" + ) + + +# --------------------------------------------------------------------------- +# Dry-run enforcement steps +# --------------------------------------------------------------------------- + + +@when( + 'I subtree isolate target decision "{decision_id}" with mode "{mode}" and dry_run' +) +def step_subtree_target_dry_run( + context: Context, + decision_id: str, + mode: str, +) -> None: + """Create a dry-run correction request.""" + svc: CorrectionService = context.subtree_svc + correction_mode = CorrectionMode(mode) + req = svc.request_correction( + plan_id=_PLAN_ID, + target_decision_id=decision_id, + mode=correction_mode, + dry_run=True, + ) + context.subtree_correction_id = req.correction_id + + +@then("subtree isolate executing the correction should raise a dry-run error") +def step_subtree_execute_raises_dry_run(context: Context) -> None: + """Verify that executing a dry-run correction raises ValidationError.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.execute_correction(context.subtree_correction_id, context.subtree_tree) + except ValidationError as exc: + raised = True + assert "dry-run" in str(exc).lower(), ( + f"Expected 'dry-run' in error message, got: {exc}" + ) + assert raised, "Expected ValidationError for dry-run execution, but none was raised" + + +# --------------------------------------------------------------------------- +# Negative isolation validation steps +# --------------------------------------------------------------------------- + + +@given("a subtree isolation tree where child structurally reaches root") +def step_subtree_cyclic_tree(context: Context) -> None: + """Build a tree where a child has the root as structural child.""" + tree: dict[str, list[str]] = {} + for row in context.table: + parent = row["parent"] + children = _parse_children(row["children"]) + tree[parent] = children + context.subtree_tree = tree + + +@then("the subtree isolate isolation should be invalid") +def step_subtree_isolation_invalid(context: Context) -> None: + """Check that subtree isolation validation failed.""" + assert context.subtree_isolation_valid is False, ( + "Expected isolation to be invalid, but it was valid" + ) + + +# --------------------------------------------------------------------------- +# Execute revert (non-dry-run) steps +# --------------------------------------------------------------------------- + + +@when("I subtree isolate execute revert correction") +def step_subtree_execute_revert(context: Context) -> None: + """Execute a revert correction.""" + svc: CorrectionService = context.subtree_svc + result = svc.execute_revert( + context.subtree_correction_id, + context.subtree_tree, + ) + context.subtree_revert_result = result + + +@then('the subtree isolate revert result status should be "{status}"') +def step_subtree_revert_status(context: Context, status: str) -> None: + """Check the revert result status.""" + result = context.subtree_revert_result + assert result.status.value == status, ( + f"Expected status='{status}', got '{result.status.value}'" + ) + + +@then( + 'the subtree isolate revert result should contain reverted decisions "{expected}"' +) +def step_subtree_revert_decisions(context: Context, expected: str) -> None: + """Check the reverted decisions list.""" + result = context.subtree_revert_result + expected_list = sorted(_parse_children(expected)) + actual_list = sorted(result.reverted_decisions) + assert actual_list == expected_list, ( + f"Expected reverted={expected_list}, got {actual_list}" + ) + + +@then("the subtree isolate revert result should have archived artifacts") +def step_subtree_revert_has_artifacts(context: Context) -> None: + """Check that archived artifacts is non-empty.""" + result = context.subtree_revert_result + assert len(result.archived_artifacts) > 0, ( + f"Expected non-empty archived_artifacts, got {result.archived_artifacts}" + ) + + +# --------------------------------------------------------------------------- +# Status guard enforcement steps +# --------------------------------------------------------------------------- + + +@then("subtree isolate executing the correction again should raise a status error") +def step_subtree_execute_again_raises(context: Context) -> None: + """Verify re-executing an applied correction raises ValidationError.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.execute_correction(context.subtree_correction_id, context.subtree_tree) + except ValidationError: + raised = True + assert raised, "Expected ValidationError when re-executing an applied correction" + + +@when("I subtree isolate cancel the correction") +def step_subtree_cancel(context: Context) -> None: + """Cancel the current correction.""" + svc: CorrectionService = context.subtree_svc + svc.cancel_correction(context.subtree_correction_id) + + +@then("subtree isolate executing the correction should raise a status error") +def step_subtree_execute_cancelled_raises(context: Context) -> None: + """Verify executing a cancelled correction raises ValidationError.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.execute_correction(context.subtree_correction_id, context.subtree_tree) + except ValidationError: + raised = True + assert raised, "Expected ValidationError when executing a cancelled correction" + + +# --------------------------------------------------------------------------- +# Mode mismatch enforcement steps +# --------------------------------------------------------------------------- + + +@then("subtree isolate calling execute_revert should raise a mode error") +def step_subtree_revert_on_append_raises(context: Context) -> None: + """Verify calling execute_revert on an append correction raises.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.execute_revert(context.subtree_correction_id, context.subtree_tree) + except ValidationError as exc: + raised = True + assert "REVERT" in str(exc), f"Expected 'REVERT' in error, got: {exc}" + assert raised, "Expected ValidationError for mode mismatch" + + +@then("subtree isolate calling execute_append should raise a mode error") +def step_subtree_append_on_revert_raises(context: Context) -> None: + """Verify calling execute_append on a revert correction raises.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.execute_append(context.subtree_correction_id) + except ValidationError as exc: + raised = True + assert "APPEND" in str(exc), f"Expected 'APPEND' in error, got: {exc}" + assert raised, "Expected ValidationError for mode mismatch" + + +# --------------------------------------------------------------------------- +# Single-node tree steps +# --------------------------------------------------------------------------- + + +@given("a subtree isolation single node tree") +def step_subtree_single_node_tree(context: Context) -> None: + """Set a tree with only a root node and no children.""" + context.subtree_tree = {"root": []} + + +# --------------------------------------------------------------------------- +# Terminal state guard steps +# --------------------------------------------------------------------------- + + +@then("subtree isolate re-analyzing the impact should raise a terminal state error") +def step_subtree_reanalyze_raises(context: Context) -> None: + """Verify re-analyzing an applied correction raises ValidationError.""" + svc: CorrectionService = context.subtree_svc + raised = False + try: + svc.analyze_impact(context.subtree_correction_id, context.subtree_tree) + except ValidationError as exc: + raised = True + assert "terminal" in str(exc).lower(), ( + f"Expected 'terminal' in error, got: {exc}" + ) + assert raised, "Expected ValidationError when re-analyzing in terminal state" + + +# --------------------------------------------------------------------------- +# Exact-match affected count steps +# --------------------------------------------------------------------------- + + +@then("the subtree isolate affected decisions count should be {expected:d}") +def step_subtree_affected_count(context: Context, expected: int) -> None: + """Check the exact count of affected decisions.""" + impact = context.subtree_impact + assert len(impact.affected_decisions) == expected, ( + f"Expected {expected} affected decisions, " + f"got {len(impact.affected_decisions)}: {impact.affected_decisions}" + ) + + +# --------------------------------------------------------------------------- +# Convergent (diamond) DAG topology steps +# --------------------------------------------------------------------------- + + +@given( + 'a subtree isolate diamond influence where "{src1}" and "{src2}" both target "{dst}"' +) +def step_subtree_diamond_influence( + context: Context, + src1: str, + src2: str, + dst: str, +) -> None: + """Set up a convergent (diamond) influence DAG.""" + context.subtree_influence_edges = {src1: [dst], src2: [dst]} + + +@then( + 'the subtree isolate affected decision "{decision_id}" should appear exactly once' +) +def step_subtree_affected_appears_once(context: Context, decision_id: str) -> None: + """Verify a decision appears exactly once in the affected list.""" + impact = context.subtree_impact + count = impact.affected_decisions.count(decision_id) + assert count == 1, ( + f"Expected '{decision_id}' exactly once in affected, " + f"found {count} times: {impact.affected_decisions}" + ) + + +# --------------------------------------------------------------------------- +# Dry-run exception recovery steps +# --------------------------------------------------------------------------- + + +@when("I subtree isolate generate a dry-run report that fails internally") +def step_subtree_dry_run_fails(context: Context) -> None: + """Generate a dry-run report where analyze_impact raises internally.""" + svc: CorrectionService = context.subtree_svc + try: + with patch.object( + CorrectionService, + "_compute_affected_subtree", + side_effect=RuntimeError("simulated BFS failure"), + ): + svc.generate_dry_run_report( + context.subtree_correction_id, + context.subtree_tree, + ) + except RuntimeError: + pass # Expected — the exception propagates from analyze_impact + + +@then('the subtree isolate correction status should be "{expected_status}"') +def step_subtree_correction_status(context: Context, expected_status: str) -> None: + """Verify the correction request's current status.""" + svc: CorrectionService = context.subtree_svc + request = svc.get_correction(context.subtree_correction_id) + expected = CorrectionStatus(expected_status) + assert request.status == expected, ( + f"Expected status='{expected}', got '{request.status}'" + ) + + +# --------------------------------------------------------------------------- +# Execute revert with influence edges steps +# --------------------------------------------------------------------------- + + +@when("I subtree isolate execute revert correction with influence edges") +def step_subtree_execute_revert_with_influence(context: Context) -> None: + """Execute a revert correction with influence edges.""" + svc: CorrectionService = context.subtree_svc + result = svc.execute_revert( + context.subtree_correction_id, + context.subtree_tree, + influence_edges=context.subtree_influence_edges, + ) + context.subtree_revert_result = result + + +@then( + 'the subtree isolate revert result should contain reverted decision "{decision_id}"' +) +def step_subtree_revert_contains_decision( + context: Context, + decision_id: str, +) -> None: + """Check that a specific decision is in the reverted set.""" + result = context.subtree_revert_result + assert decision_id in result.reverted_decisions, ( + f"Expected '{decision_id}' in reverted={result.reverted_decisions}" + ) + + +# --------------------------------------------------------------------------- +# DAG-only nodes in excluded set steps +# --------------------------------------------------------------------------- + + +@given('a subtree isolate influence edge from "{src}" to "{dst}" only in DAG') +def step_subtree_dag_only_influence(context: Context, src: str, dst: str) -> None: + """Set up influence edges with nodes that are NOT in the structural tree.""" + context.subtree_influence_edges = {src: [dst]} diff --git a/robot/correction_subtree_isolation.robot b/robot/correction_subtree_isolation.robot new file mode 100644 index 000000000..cad20da35 --- /dev/null +++ b/robot/correction_subtree_isolation.robot @@ -0,0 +1,137 @@ +*** Settings *** +Documentation Integration tests for correction subtree isolation. +... Verifies that decision corrections recompute only the +... affected subtree while excluding root and sibling nodes. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} robot/helper_correction_subtree_isolation.py + +*** Test Cases *** +Leaf Node Isolation + [Documentation] Target a leaf — only the leaf is affected + [Tags] phase2 correction subtree isolation + ${result}= Run Process ${PYTHON} ${HELPER} leaf-isolation cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} leaf-isolation-ok + +Middle Node Isolation + [Documentation] Target a middle node — middle and descendants affected + [Tags] phase2 correction subtree isolation + ${result}= Run Process ${PYTHON} ${HELPER} middle-node-isolation cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} middle-node-isolation-ok + +Root Targets All Decisions + [Documentation] Target root — all decisions affected, none excluded + [Tags] phase2 correction subtree isolation + ${result}= Run Process ${PYTHON} ${HELPER} root-targets-all cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} root-targets-all-ok + +Rollback Tier Computation + [Documentation] Verify tier depth at depth 0, 1, 2, and 3 + [Tags] phase2 correction subtree tier + ${result}= Run Process ${PYTHON} ${HELPER} rollback-tier-computation cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} rollback-tier-computation-ok + +Enhanced Dry Run Report + [Documentation] Dry-run report includes excluded decisions and tier + [Tags] phase2 correction subtree dryrun + ${result}= Run Process ${PYTHON} ${HELPER} dry-run-report-enhanced cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} dry-run-report-enhanced-ok + +Dry Run Tier Zero Warning + [Documentation] Root targeting triggers tier-0 warning + [Tags] phase2 correction subtree dryrun tier + ${result}= Run Process ${PYTHON} ${HELPER} dry-run-tier-zero-warning cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} dry-run-tier-zero-warning-ok + +Subtree Isolation Validation + [Documentation] Isolation validation passes for non-root targets + [Tags] phase2 correction subtree validation + ${result}= Run Process ${PYTHON} ${HELPER} subtree-isolation-validation cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} subtree-isolation-validation-ok + +Edge Case Empty Tree + [Documentation] Edge cases: empty tree, unknown node, root parent + [Tags] phase2 correction subtree edge + ${result}= Run Process ${PYTHON} ${HELPER} edge-case-empty-tree cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} edge-case-empty-tree-ok + +Influence DAG Traversal + [Documentation] Influence edges pull additional decisions into affected set + [Tags] phase2 correction subtree influence + ${result}= Run Process ${PYTHON} ${HELPER} influence-dag-traversal cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} influence-dag-traversal-ok + +Append Mode Correction + [Documentation] Append mode spawns a child plan without impact analysis + [Tags] phase2 correction subtree append + ${result}= Run Process ${PYTHON} ${HELPER} append-mode-correction cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} append-mode-correction-ok + +Dry Run Enforcement + [Documentation] Dry-run corrections cannot be executed + [Tags] phase2 correction subtree dryrun enforcement + ${result}= Run Process ${PYTHON} ${HELPER} dry-run-enforcement cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} dry-run-enforcement-ok + +Execute Revert Non Dry Run + [Documentation] Revert correction executes end-to-end with correct results + [Tags] phase2 correction subtree revert + ${result}= Run Process ${PYTHON} ${HELPER} execute-revert-non-dry-run cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} execute-revert-non-dry-run-ok + +Status Guard Enforcement + [Documentation] Applied and cancelled corrections cannot be re-executed + [Tags] phase2 correction subtree guard + ${result}= Run Process ${PYTHON} ${HELPER} status-guard-enforcement cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} status-guard-enforcement-ok + +Mode Mismatch Enforcement + [Documentation] execute_revert on append and execute_append on revert both raise + [Tags] phase2 correction subtree guard + ${result}= Run Process ${PYTHON} ${HELPER} mode-mismatch-enforcement cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} mode-mismatch-enforcement-ok + +Diamond DAG Topology + [Documentation] Convergent (diamond) DAG topology does not cause duplicate visits + [Tags] phase2 correction subtree influence diamond + ${result}= Run Process ${PYTHON} ${HELPER} diamond-dag-topology cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} diamond-dag-topology-ok + +Dry Run Exception Recovery + [Documentation] Dry-run report restores status even when analyze_impact fails + [Tags] phase2 correction subtree dryrun recovery + ${result}= Run Process ${PYTHON} ${HELPER} dry-run-exception-recovery cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} dry-run-exception-recovery-ok + +Execute Revert With Influence Edges + [Documentation] Revert correction works correctly with influence DAG edges + [Tags] phase2 correction subtree revert influence + ${result}= Run Process ${PYTHON} ${HELPER} execute-revert-with-influence cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} execute-revert-with-influence-ok + +DAG Only Nodes In Excluded Set + [Documentation] Nodes appearing only in the influence DAG are in the excluded set + [Tags] phase2 correction subtree influence excluded + ${result}= Run Process ${PYTHON} ${HELPER} dag-only-nodes-excluded cwd=${WORKSPACE} on_timeout=kill + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} dag-only-nodes-excluded-ok diff --git a/robot/helper_correction_subtree_isolation.py b/robot/helper_correction_subtree_isolation.py new file mode 100644 index 000000000..9064dc231 --- /dev/null +++ b/robot/helper_correction_subtree_isolation.py @@ -0,0 +1,457 @@ +"""Helper script for correction subtree isolation Robot Framework tests. + +Validates that CorrectionService correctly isolates correction scope: +- excluded_decisions populated after impact analysis +- rollback_tier_depth computation +- dry-run report enhancements +- subtree isolation validation +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from unittest.mock import patch + +from cleveragents.application.services.correction_service import CorrectionService +from cleveragents.core.exceptions import ValidationError +from cleveragents.domain.models.core.correction import CorrectionMode, CorrectionStatus + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- +_PLAN_ID = "01ROBOTSUBTREEPLAN0000000001" + + +def _sample_tree() -> dict[str, list[str]]: + return { + "root": ["A", "B"], + "A": ["C", "D"], + "B": ["E"], + } + + +def _deep_tree(depth: int) -> dict[str, list[str]]: + tree: dict[str, list[str]] = {} + for i in range(depth): + parent = f"depth_{i}" if i > 0 else "root" + child = f"depth_{i + 1}" + tree[parent] = [child] + return tree + + +# --------------------------------------------------------------------------- +# Command handlers +# --------------------------------------------------------------------------- + + +def _leaf_isolation() -> None: + """Target a leaf node and verify only it is affected.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "D", CorrectionMode.REVERT) + impact = svc.analyze_impact(req.correction_id, tree) + + assert impact.affected_decisions == ["D"], ( + f"Expected ['D'], got {impact.affected_decisions}" + ) + for excluded in ["root", "A", "B", "C", "E"]: + assert excluded in impact.excluded_decisions, ( + f"Expected '{excluded}' in excluded={impact.excluded_decisions}" + ) + print("leaf-isolation-ok") + + +def _middle_node_isolation() -> None: + """Target a middle node and verify subtree is affected.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + impact = svc.analyze_impact(req.correction_id, tree) + + for affected in ["A", "C", "D"]: + assert affected in impact.affected_decisions, ( + f"Expected '{affected}' in affected={impact.affected_decisions}" + ) + for excluded in ["root", "B", "E"]: + assert excluded in impact.excluded_decisions, ( + f"Expected '{excluded}' in excluded={impact.excluded_decisions}" + ) + print("middle-node-isolation-ok") + + +def _root_targets_all() -> None: + """Target root and verify all decisions are affected.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "root", CorrectionMode.REVERT) + impact = svc.analyze_impact(req.correction_id, tree) + + for node in ["root", "A", "B", "C", "D", "E"]: + assert node in impact.affected_decisions, ( + f"Expected '{node}' in affected={impact.affected_decisions}" + ) + assert len(impact.excluded_decisions) == 0, ( + f"Expected empty excluded, got {impact.excluded_decisions}" + ) + assert impact.rollback_tier_depth == 0 + print("root-targets-all-ok") + + +def _rollback_tier_computation() -> None: + """Verify rollback tier at various depths.""" + svc = CorrectionService() + tree = _sample_tree() + + assert svc.compute_rollback_tier("root", _PLAN_ID, tree) == 0 + assert svc.compute_rollback_tier("A", _PLAN_ID, tree) == 1 + assert svc.compute_rollback_tier("B", _PLAN_ID, tree) == 1 + assert svc.compute_rollback_tier("C", _PLAN_ID, tree) == 2 + assert svc.compute_rollback_tier("D", _PLAN_ID, tree) == 2 + assert svc.compute_rollback_tier("E", _PLAN_ID, tree) == 2 + + # Deep tree test + deep = _deep_tree(4) + assert svc.compute_rollback_tier("depth_3", _PLAN_ID, deep) == 3 + print("rollback-tier-computation-ok") + + +def _dry_run_report_enhanced() -> None: + """Verify dry-run report includes excluded decisions and tier depth.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + report = svc.generate_dry_run_report(req.correction_id, tree) + + assert report.rollback_tier_depth == 1, ( + f"Expected tier_depth=1, got {report.rollback_tier_depth}" + ) + assert len(report.excluded_decisions) > 0, "Expected non-empty excluded_decisions" + for excluded in ["root", "B", "E"]: + assert excluded in report.excluded_decisions, ( + f"Expected '{excluded}' in report excluded={report.excluded_decisions}" + ) + print("dry-run-report-enhanced-ok") + + +def _dry_run_tier_zero_warning() -> None: + """Verify tier-0 warning when root is targeted.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "root", CorrectionMode.REVERT) + report = svc.generate_dry_run_report(req.correction_id, tree) + + all_warnings = " ".join(report.warnings) + assert "Tier 0" in all_warnings, f"Expected 'Tier 0' in warnings: {report.warnings}" + assert report.rollback_tier_depth == 0 + print("dry-run-tier-zero-warning-ok") + + +def _subtree_isolation_validation() -> None: + """Verify subtree isolation validation passes for non-root targets.""" + svc = CorrectionService() + tree = _sample_tree() + + assert svc.validate_subtree_isolation("A", tree) is True + assert svc.validate_subtree_isolation("D", tree) is True + assert svc.validate_subtree_isolation("E", tree) is True + print("subtree-isolation-validation-ok") + + +def _edge_case_empty_tree() -> None: + """Verify edge cases with empty tree.""" + svc = CorrectionService() + + # validate_subtree_isolation with empty tree + assert svc.validate_subtree_isolation("X", {}) is True + + # _find_root with empty tree + assert CorrectionService._find_root({}) is None + + # _find_parent for root node (not a child of anything) + tree = _sample_tree() + assert CorrectionService._find_parent("root", tree) is None + + # compute_rollback_tier for unknown node + assert svc.compute_rollback_tier("UNKNOWN", _PLAN_ID, tree) == 0 + print("edge-case-empty-tree-ok") + + +def _influence_dag_traversal() -> None: + """Verify influence edges pull extra decisions into affected set.""" + svc = CorrectionService() + tree = _sample_tree() + + # D -> E via influence DAG (E is a sibling-subtree node) + req = svc.request_correction(_PLAN_ID, "D", CorrectionMode.REVERT) + influence = {"D": ["E"]} + impact = svc.analyze_impact(req.correction_id, tree, influence_edges=influence) + + assert "D" in impact.affected_decisions, ( + f"Expected 'D' in affected={impact.affected_decisions}" + ) + assert "E" in impact.affected_decisions, ( + f"Expected 'E' in affected (via influence)={impact.affected_decisions}" + ) + assert "root" in impact.excluded_decisions + assert "A" in impact.excluded_decisions + + # Cycle detection: C -> D -> C via influence DAG + svc2 = CorrectionService() + req2 = svc2.request_correction(_PLAN_ID, "C", CorrectionMode.REVERT) + cycle_influence = {"C": ["D"], "D": ["C"]} + impact2 = svc2.analyze_impact( + req2.correction_id, tree, influence_edges=cycle_influence + ) + assert "C" in impact2.affected_decisions + assert "D" in impact2.affected_decisions + + # Isolation validation passes even when influence edges reach sibling + assert svc.validate_subtree_isolation("D", tree, influence_edges=influence) is True + + print("influence-dag-traversal-ok") + + +def _append_mode_correction() -> None: + """Verify append mode spawns a child plan.""" + svc = CorrectionService() + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.APPEND) + result = svc.execute_append(req.correction_id) + + assert result.status.value == "applied", ( + f"Expected status='applied', got '{result.status.value}'" + ) + assert result.spawned_child_plan_id is not None, ( + "Expected spawned_child_plan_id, got None" + ) + assert len(result.new_decisions) > 0, ( + f"Expected new_decisions, got {result.new_decisions}" + ) + print("append-mode-correction-ok") + + +def _dry_run_enforcement() -> None: + """Verify that executing a dry-run correction raises ValidationError.""" + svc = CorrectionService() + + # Revert dry-run should not be executable + req_revert = svc.request_correction( + _PLAN_ID, "A", CorrectionMode.REVERT, dry_run=True + ) + try: + svc.execute_correction(req_revert.correction_id, _sample_tree()) + raise AssertionError("Expected ValidationError for dry-run revert execution") + except ValidationError as exc: + assert "dry-run" in str(exc).lower(), ( + f"Expected 'dry-run' in error message, got: {exc}" + ) + + # Append dry-run should not be executable + svc2 = CorrectionService() + req_append = svc2.request_correction( + _PLAN_ID, "A", CorrectionMode.APPEND, dry_run=True + ) + try: + svc2.execute_correction(req_append.correction_id) + raise AssertionError("Expected ValidationError for dry-run append execution") + except ValidationError as exc: + assert "dry-run" in str(exc).lower(), ( + f"Expected 'dry-run' in error message, got: {exc}" + ) + + print("dry-run-enforcement-ok") + + +def _execute_revert_non_dry_run() -> None: + """Verify execute_revert works end-to-end for a non-dry-run correction.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + result = svc.execute_revert(req.correction_id, tree) + + assert result.status.value == "applied", ( + f"Expected status='applied', got '{result.status.value}'" + ) + assert sorted(result.reverted_decisions) == sorted(["A", "C", "D"]), ( + f"Expected reverted=['A','C','D'], got {result.reverted_decisions}" + ) + assert len(result.archived_artifacts) > 0, ( + f"Expected non-empty archived_artifacts, got {result.archived_artifacts}" + ) + print("execute-revert-non-dry-run-ok") + + +def _status_guard_enforcement() -> None: + """Verify status guards prevent re-execution and cancelled execution.""" + svc = CorrectionService() + tree = _sample_tree() + + # Re-execution of applied correction + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + svc.execute_revert(req.correction_id, tree) + try: + svc.execute_correction(req.correction_id, tree) + raise AssertionError("Expected ValidationError for re-execution") + except ValidationError: + pass + + # Execution of cancelled correction + svc2 = CorrectionService() + req2 = svc2.request_correction(_PLAN_ID, "B", CorrectionMode.REVERT) + svc2.cancel_correction(req2.correction_id) + try: + svc2.execute_correction(req2.correction_id, tree) + raise AssertionError("Expected ValidationError for cancelled execution") + except ValidationError: + pass + + print("status-guard-enforcement-ok") + + +def _mode_mismatch_enforcement() -> None: + """Verify mode mismatch raises ValidationError.""" + svc = CorrectionService() + + # execute_revert on an append correction + req_a = svc.request_correction(_PLAN_ID, "A", CorrectionMode.APPEND) + try: + svc.execute_revert(req_a.correction_id, _sample_tree()) + raise AssertionError("Expected ValidationError for mode mismatch") + except ValidationError as exc: + assert "REVERT" in str(exc), f"Expected 'REVERT' in error, got: {exc}" + + # execute_append on a revert correction + svc2 = CorrectionService() + req_r = svc2.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + try: + svc2.execute_append(req_r.correction_id) + raise AssertionError("Expected ValidationError for mode mismatch") + except ValidationError as exc: + assert "APPEND" in str(exc), f"Expected 'APPEND' in error, got: {exc}" + + print("mode-mismatch-enforcement-ok") + + +def _diamond_dag_topology() -> None: + """Verify convergent (diamond) DAG topology does not duplicate visits.""" + svc = CorrectionService() + tree = _sample_tree() + + # Both C and D (children of A) influence E via DAG — diamond pattern + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + influence = {"C": ["E"], "D": ["E"]} + impact = svc.analyze_impact(req.correction_id, tree, influence_edges=influence) + + assert "E" in impact.affected_decisions, ( + f"Expected 'E' in affected={impact.affected_decisions}" + ) + count = impact.affected_decisions.count("E") + assert count == 1, ( + f"Expected 'E' exactly once, found {count} times: {impact.affected_decisions}" + ) + print("diamond-dag-topology-ok") + + +def _dry_run_exception_recovery() -> None: + """Verify dry-run report restores status when analyze_impact fails.""" + svc = CorrectionService() + tree = _sample_tree() + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + + assert req.status == CorrectionStatus.PENDING, ( + f"Expected PENDING before dry-run, got {req.status}" + ) + + try: + with patch.object( + CorrectionService, + "_compute_affected_subtree", + side_effect=RuntimeError("simulated BFS failure"), + ): + svc.generate_dry_run_report(req.correction_id, tree) + except RuntimeError: + pass # Expected — exception propagates from analyze_impact + + assert req.status == CorrectionStatus.PENDING, ( + f"Expected PENDING after failed dry-run, got {req.status}" + ) + print("dry-run-exception-recovery-ok") + + +def _execute_revert_with_influence() -> None: + """Verify execute_revert works with influence edges.""" + svc = CorrectionService() + tree = _sample_tree() + + # D influences E via DAG — revert of A should include E + influence = {"D": ["E"]} + req = svc.request_correction(_PLAN_ID, "A", CorrectionMode.REVERT) + result = svc.execute_revert(req.correction_id, tree, influence_edges=influence) + + assert result.status.value == "applied", ( + f"Expected status='applied', got '{result.status.value}'" + ) + assert "E" in result.reverted_decisions, ( + f"Expected 'E' in reverted={result.reverted_decisions}" + ) + print("execute-revert-with-influence-ok") + + +def _dag_only_nodes_excluded() -> None: + """Verify nodes in the DAG (not in tree) appear in excluded set.""" + svc = CorrectionService() + tree = _sample_tree() + + # X_EXT and Y_EXT exist only in the influence DAG, not in the tree + influence = {"X_EXT": ["Y_EXT"]} + req = svc.request_correction(_PLAN_ID, "D", CorrectionMode.REVERT) + impact = svc.analyze_impact(req.correction_id, tree, influence_edges=influence) + + assert "X_EXT" in impact.excluded_decisions, ( + f"Expected 'X_EXT' in excluded={impact.excluded_decisions}" + ) + assert "Y_EXT" in impact.excluded_decisions, ( + f"Expected 'Y_EXT' in excluded={impact.excluded_decisions}" + ) + print("dag-only-nodes-excluded-ok") + + +# --------------------------------------------------------------------------- +# Dispatch +# --------------------------------------------------------------------------- + +COMMANDS: dict[str, Callable[[], None]] = { + "leaf-isolation": _leaf_isolation, + "middle-node-isolation": _middle_node_isolation, + "root-targets-all": _root_targets_all, + "rollback-tier-computation": _rollback_tier_computation, + "dry-run-report-enhanced": _dry_run_report_enhanced, + "dry-run-tier-zero-warning": _dry_run_tier_zero_warning, + "subtree-isolation-validation": _subtree_isolation_validation, + "edge-case-empty-tree": _edge_case_empty_tree, + "influence-dag-traversal": _influence_dag_traversal, + "append-mode-correction": _append_mode_correction, + "dry-run-enforcement": _dry_run_enforcement, + "execute-revert-non-dry-run": _execute_revert_non_dry_run, + "status-guard-enforcement": _status_guard_enforcement, + "mode-mismatch-enforcement": _mode_mismatch_enforcement, + "diamond-dag-topology": _diamond_dag_topology, + "dry-run-exception-recovery": _dry_run_exception_recovery, + "execute-revert-with-influence": _execute_revert_with_influence, + "dag-only-nodes-excluded": _dag_only_nodes_excluded, +} + + +def main() -> None: + """Dispatch command from sys.argv.""" + if len(sys.argv) < 2: + raise SystemExit("Expected command argument") + command = sys.argv[1] + if command not in COMMANDS: + raise SystemExit(f"Unknown command: {command}") + func = COMMANDS[command] + func() + + +if __name__ == "__main__": + main() diff --git a/src/cleveragents/application/services/correction_service.py b/src/cleveragents/application/services/correction_service.py index 937b1f035..b4ca25855 100644 --- a/src/cleveragents/application/services/correction_service.py +++ b/src/cleveragents/application/services/correction_service.py @@ -41,6 +41,36 @@ logger = structlog.get_logger(__name__) _RISK_LOW_MAX = 3 _RISK_MEDIUM_MAX = 10 +# --------------------------------------------------------------------------- +# Cost / time estimation constants +# --------------------------------------------------------------------------- +_COST_PER_DECISION = 1.5 +_RECOMPUTE_SECONDS_PER_DECISION = 2.0 + +# --------------------------------------------------------------------------- +# Terminal correction statuses (immutable after execution) +# --------------------------------------------------------------------------- +_TERMINAL_STATUSES: frozenset[CorrectionStatus] = frozenset( + { + CorrectionStatus.APPLIED, + CorrectionStatus.FAILED, + CorrectionStatus.CANCELLED, + CorrectionStatus.REJECTED, + } +) + +# --------------------------------------------------------------------------- +# Executable / cancellable statuses (shared by guards) +# --------------------------------------------------------------------------- +_EXECUTABLE_STATUSES: frozenset[CorrectionStatus] = frozenset( + {CorrectionStatus.PENDING, CorrectionStatus.ANALYZING} +) + +# --------------------------------------------------------------------------- +# Maximum decision tree size (DoS protection) +# --------------------------------------------------------------------------- +_MAX_TREE_NODES = 50_000 + class CorrectionService: """Service for creating, analysing, and executing decision corrections. @@ -75,6 +105,7 @@ class CorrectionService: correction_id: str, request: CorrectionRequest, result: CorrectionResult, + attempt_id: str | None = None, ) -> None: """Emit a ``CORRECTION_APPLIED`` event when the result is successful.""" if self._event_bus is not None and result.status == CorrectionStatus.APPLIED: @@ -85,6 +116,7 @@ class CorrectionService: plan_id=request.plan_id, details={ "correction_id": correction_id, + "attempt_id": attempt_id, "target_decision_id": request.target_decision_id, "mode": request.mode.value if hasattr(request.mode, "value") @@ -94,7 +126,7 @@ class CorrectionService: ) ) except Exception: - logger.warning( + logger.error( "event_bus_emit_failed", event_type="CORRECTION_APPLIED", correction_id=correction_id, @@ -170,6 +202,11 @@ class CorrectionService: corrections cascade through influence relationships as required by the specification (§ Affected Subtree Computation). + After computing the affected subtree, populates + ``excluded_decisions`` with all plan decisions that are **not** + in the affected set, enabling callers to understand which parts + of the tree remain untouched. + Args: correction_id: Previously created correction request ID. decision_tree: Adjacency list mapping parent → children. @@ -177,31 +214,69 @@ class CorrectionService: in the influence DAG (``decision_dependencies``). Returns: - ``CorrectionImpact`` with affected nodes and risk level. + ``CorrectionImpact`` with affected nodes, excluded nodes, + rollback tier depth, and risk level. Raises: ResourceNotFoundError: If the correction does not exist. """ request = self._get_request_or_raise(correction_id) - request.status = CorrectionStatus.ANALYZING + # Guard against pathologically large inputs that could cause + # unbounded memory / CPU consumption during BFS traversal. tree = decision_tree or {} dag = influence_edges or {} + total_keys = len(tree) + len(dag) + if total_keys > _MAX_TREE_NODES: + raise ValidationError( + f"Decision tree + influence DAG too large ({total_keys} keys, " + f"max {_MAX_TREE_NODES}). Reduce tree size before analyzing." + ) + + # Reject analysis on terminal states to prevent audit-data + # corruption (the stored impact must match what was used during + # execution). + if request.status in _TERMINAL_STATUSES: + raise ValidationError( + f"Cannot analyze correction in terminal '{request.status}' " + "status. Impact data is immutable after execution." + ) + + if request.status == CorrectionStatus.PENDING: + request.status = CorrectionStatus.ANALYZING + affected = self._compute_affected_subtree(request.target_decision_id, tree, dag) risk = self._classify_risk(len(affected)) + # Collect all decisions known in the plan from the adjacency lists + all_decisions = self._collect_all_decisions(tree, dag) + # Guarantee the target is in the universe even if it does not + # appear in any adjacency list (isolated single-node plan). + all_decisions.add(request.target_decision_id) + affected_set = set(affected) + excluded = sorted(d for d in all_decisions if d not in affected_set) + + # Compute rollback tier depth (hops from target to root) + tier_depth = self._compute_rollback_tier_depth(request.target_decision_id, tree) + # Derive artefacts from decision IDs (convention: .artifact) artifacts = [f"{d}.artifact" for d in affected] impact = CorrectionImpact( affected_decisions=affected, + excluded_decisions=excluded, + # TODO: affected_files and artifacts_to_archive use synthetic + # placeholders derived from decision IDs. Replace with real + # file / artifact tracking once the resource-rollback layer + # is integrated (see spec § Mid-Execute Correction). affected_files=[f"{d}.py" for d in affected], affected_child_plans=[], - estimated_cost=float(len(affected)) * 1.5, + estimated_cost=float(len(affected)) * _COST_PER_DECISION, risk_level=risk, rollback_tier="full" if request.mode == CorrectionMode.REVERT else "append_only", + rollback_tier_depth=tier_depth, artifacts_to_archive=artifacts, ) self._impacts[correction_id] = impact @@ -210,6 +285,8 @@ class CorrectionService: "correction.impact_analyzed", correction_id=correction_id, affected_count=len(affected), + excluded_count=len(excluded), + rollback_tier_depth=tier_depth, risk_level=risk, ) return impact @@ -224,10 +301,16 @@ class CorrectionService: decision_tree: dict[str, list[str]] | None = None, influence_edges: dict[str, list[str]] | None = None, ) -> CorrectionDryRunReport: - """Generate a dry-run report without mutating state. + """Generate a dry-run report without executing the correction. - Calls ``analyze_impact`` internally, then assembles warnings - and estimated recompute time. + Calls ``analyze_impact`` internally to compute the affected + subtree, then assembles warnings, excluded decisions, rollback + tier depth, and estimated recompute time. The request status + is preserved (not mutated) so that generating a preview does + not advance the correction lifecycle. + + A tier-0 warning is emitted when the root decision is targeted, + indicating the entire decision tree will be affected. Args: correction_id: Correction request ID. @@ -238,7 +321,20 @@ class CorrectionService: ``CorrectionDryRunReport`` describing what *would* happen. """ request = self._get_request_or_raise(correction_id) - impact = self.analyze_impact(correction_id, decision_tree, influence_edges) + + # Preserve original status and impact — dry-run is conceptually + # read-only. The try/finally ensures both are restored even when + # analyze_impact raises after transitioning the status. + original_status = request.status + original_impact = self._impacts.get(correction_id) + try: + impact = self.analyze_impact(correction_id, decision_tree, influence_edges) + finally: + request.status = original_status + if original_impact is None: + self._impacts.pop(correction_id, None) + else: + self._impacts[correction_id] = original_impact warnings: list[str] = [] if impact.risk_level == "high": @@ -246,15 +342,33 @@ class CorrectionService: "High risk: more than 10 decisions affected. " "Review carefully before executing." ) - if impact.risk_level == "medium": + elif impact.risk_level == "medium": warnings.append("Medium risk: 4-10 decisions affected.") if request.mode == CorrectionMode.REVERT and len(impact.affected_decisions) > 1: warnings.append( f"Revert will invalidate {len(impact.affected_decisions)} decisions " "and archive associated artifacts." ) + # Only emit a tier-0 warning when the target is genuinely the + # tree root. Compare against the actual root determined by + # _find_root to avoid false positives for non-root subtree + # heads in forest (disconnected) topologies. + tree = decision_tree or {} + actual_root = self._find_root(tree) + if ( + impact.rollback_tier_depth == 0 + and len(impact.affected_decisions) > 1 + and actual_root is not None + and request.target_decision_id == actual_root + ): + warnings.append( + "Tier 0: root decision targeted — entire decision tree " + "will be affected." + ) - recompute_seconds = float(len(impact.affected_decisions)) * 2.0 + recompute_seconds = ( + float(len(impact.affected_decisions)) * _RECOMPUTE_SECONDS_PER_DECISION + ) report = CorrectionDryRunReport( correction_id=correction_id, @@ -264,6 +378,8 @@ class CorrectionService: if request.mode == CorrectionMode.REVERT else [], child_plans_to_rollback=impact.affected_child_plans, + excluded_decisions=list(impact.excluded_decisions), + rollback_tier_depth=impact.rollback_tier_depth, estimated_recompute_time_seconds=recompute_seconds, warnings=warnings, ) @@ -272,6 +388,8 @@ class CorrectionService: "correction.dry_run_generated", correction_id=correction_id, warning_count=len(warnings), + rollback_tier_depth=impact.rollback_tier_depth, + excluded_count=len(impact.excluded_decisions), ) return report @@ -301,17 +419,32 @@ class CorrectionService: Raises: ResourceNotFoundError: If correction does not exist. - ValidationError: If correction is not in PENDING or ANALYZING status. + ValidationError: If correction is not in PENDING or ANALYZING + status, or if the correction mode is not REVERT. """ request = self._get_request_or_raise(correction_id) + if request.mode != CorrectionMode.REVERT: + raise ValidationError( + f"execute_revert requires mode=REVERT, got mode={request.mode.value!r}." + ) self._assert_executable(request) - request.status = CorrectionStatus.EXECUTING attempt = CorrectionAttempt(correction_id=correction_id) self._attempts[correction_id].append(attempt) + # Transition through ANALYZING before EXECUTING so the full + # state-machine lifecycle is honoured. + request.status = CorrectionStatus.ANALYZING + try: - impact = self.analyze_impact(correction_id, decision_tree, influence_edges) + # Use previously cached impact when available to avoid + # redundant O(V+E) recomputation. + impact = self._impacts.get(correction_id) + if impact is None: + impact = self.analyze_impact( + correction_id, decision_tree, influence_edges + ) + request.status = CorrectionStatus.EXECUTING result = CorrectionResult( correction_id=correction_id, @@ -322,6 +455,11 @@ class CorrectionService: request.status = CorrectionStatus.APPLIED attempt.success = True except Exception as exc: + logger.error( + "correction.revert_failed", + correction_id=correction_id, + exc_info=True, + ) result = CorrectionResult( correction_id=correction_id, status=CorrectionStatus.FAILED, @@ -339,7 +477,9 @@ class CorrectionService: correction_id=correction_id, status=result.status, ) - self._emit_correction_applied(correction_id, request, result) + self._emit_correction_applied( + correction_id, request, result, attempt_id=attempt.attempt_id + ) return result # ------------------------------------------------------------------ @@ -363,15 +503,24 @@ class CorrectionService: Raises: ResourceNotFoundError: If correction does not exist. - ValidationError: If correction is not in PENDING or ANALYZING status. + ValidationError: If correction is not in PENDING or ANALYZING + status, or if the correction mode is not APPEND. """ request = self._get_request_or_raise(correction_id) + if request.mode != CorrectionMode.APPEND: + raise ValidationError( + f"execute_append requires mode=APPEND, got mode={request.mode.value!r}." + ) self._assert_executable(request) - request.status = CorrectionStatus.EXECUTING attempt = CorrectionAttempt(correction_id=correction_id) self._attempts[correction_id].append(attempt) + # Transition through ANALYZING → EXECUTING for consistent + # lifecycle across both correction modes. + request.status = CorrectionStatus.ANALYZING + request.status = CorrectionStatus.EXECUTING + try: child_plan_id = str(ULID()) new_decision_id = str(ULID()) @@ -389,6 +538,11 @@ class CorrectionService: "new_decision_id": new_decision_id, } except Exception as exc: + logger.error( + "correction.append_failed", + correction_id=correction_id, + exc_info=True, + ) result = CorrectionResult( correction_id=correction_id, status=CorrectionStatus.FAILED, @@ -406,7 +560,9 @@ class CorrectionService: correction_id=correction_id, status=result.status, ) - self._emit_correction_applied(correction_id, request, result) + self._emit_correction_applied( + correction_id, request, result, attempt_id=attempt.attempt_id + ) return result # ------------------------------------------------------------------ @@ -473,11 +629,10 @@ class CorrectionService: ValidationError: If correction is not in a cancellable status. """ request = self._get_request_or_raise(correction_id) - cancellable = {CorrectionStatus.PENDING, CorrectionStatus.ANALYZING} - if request.status not in cancellable: + if request.status not in _EXECUTABLE_STATUSES: raise ValidationError( f"Cannot cancel correction in '{request.status}' status. " - f"Cancellation is only allowed in: {sorted(cancellable)}" + f"Cancellation is only allowed in: {sorted(_EXECUTABLE_STATUSES)}" ) request.status = CorrectionStatus.CANCELLED logger.info( @@ -501,14 +656,145 @@ class CorrectionService: return request def _assert_executable(self, request: CorrectionRequest) -> None: - """Ensure the correction is in an executable status.""" - executable = {CorrectionStatus.PENDING, CorrectionStatus.ANALYZING} - if request.status not in executable: + """Ensure the correction is in an executable status. + + Raises: + ValidationError: If the correction was created as dry-run + only, or if its status is not in the executable set. + """ + if request.dry_run: + raise ValidationError( + "Cannot execute a dry-run correction. " + "Use generate_dry_run_report() or analyze_impact() instead." + ) + if request.status not in _EXECUTABLE_STATUSES: raise ValidationError( f"Cannot execute correction in '{request.status}' status. " - f"Execution requires status in: {sorted(executable)}" + f"Execution requires status in: {sorted(_EXECUTABLE_STATUSES)}" ) + # ------------------------------------------------------------------ + # Rollback tier computation + # ------------------------------------------------------------------ + + def compute_rollback_tier( + self, + target_decision_id: str, + plan_id: str, + decision_tree: dict[str, list[str]] | None = None, + ) -> int: + """Compute the rollback tier (depth) for a target decision. + + The tier is the number of parent hops from the target to the + tree root: + + - **Tier 0**: the root decision itself is targeted. + - **Tier 1**: a direct child of the root is targeted. + - **Tier N**: the target is *N* levels below the root. + + Args: + target_decision_id: Decision to compute the tier for. + plan_id: Plan owning the decision tree (reserved for + future repository look-ups; currently unused beyond + logging). + decision_tree: Adjacency list (parent → children). + + Returns: + Non-negative integer representing the tier depth. + """ + tree = decision_tree or {} + depth = self._compute_rollback_tier_depth(target_decision_id, tree) + logger.info( + "correction.rollback_tier_computed", + target_decision_id=target_decision_id, + plan_id=plan_id, + tier_depth=depth, + ) + return depth + + # ------------------------------------------------------------------ + # Subtree isolation validation + # ------------------------------------------------------------------ + + def validate_subtree_isolation( + self, + target_decision_id: str, + decision_tree: dict[str, list[str]], + influence_edges: dict[str, list[str]] | None = None, + ) -> bool: + """Validate that the affected subtree is correctly isolated. + + Confirms two invariants for non-root corrections: + + 1. The **root decision** (the node with no parent in the tree) + is never in the *structural* affected set unless it is + explicitly the *target* decision. + 2. **Sibling decisions** (children of the same parent as the + target, excluding the target itself) are not in the + *structural* affected set. + + Both invariants are checked against the **structural-only** + affected set (tree traversal without influence edges). If the + influence DAG legitimately pulls in a sibling or the root, + that is expected behaviour per the spec (§ Affected Subtree + Computation) and is not an isolation violation. + + Args: + target_decision_id: Decision node that was targeted. + decision_tree: Structural tree adjacency list. + influence_edges: Optional influence DAG adjacency list. + Accepted for API consistency but **not used** in + isolation checks — structural-only BFS is used per + the specification (§ Affected Subtree Computation). + + Returns: + ``True`` if isolation invariants hold, ``False`` otherwise. + """ + tree = decision_tree + + # Use structural-only BFS for isolation invariant checks so + # that influence-DAG-caused reachability is not misreported + # as a violation. + structural_affected = self._compute_affected_subtree( + target_decision_id, + tree, + influence_edges=None, + ) + structural_set = set(structural_affected) + + # Find the root (node that never appears as a child) + root = self._find_root(tree) + if root is None: + # Empty or flat tree — nothing to validate + return True + + # Invariant 1: root not in structural affected set unless + # explicitly targeted + if root in structural_set and root != target_decision_id: + logger.warning( + "correction.isolation_violation_root", + root=root, + target=target_decision_id, + ) + return False + + # Invariant 2: siblings of target not in structural affected set + parent = self._find_parent(target_decision_id, tree) + if parent is not None: + siblings = [ + child for child in tree.get(parent, []) if child != target_decision_id + ] + for sibling in siblings: + if sibling in structural_set: + logger.warning( + "correction.isolation_violation_sibling", + sibling=sibling, + target=target_decision_id, + ) + return False + + return True + @staticmethod def _compute_affected_subtree( target_id: str, @@ -540,33 +826,50 @@ class CorrectionService: """ dag = influence_edges or {} affected: list[str] = [] + # A single ``visited`` set tracks every node that has been + # dequeued and processed. New neighbors are only enqueued + # when not yet in ``visited``, preventing both duplicate + # processing and infinite loops from cycles. When a neighbor + # is already visited it means we encountered a back-edge + # (cycle in the structural tree or influence DAG); we log the + # cycle for operational observability. visited: set[str] = set() queue: deque[str] = deque([target_id]) while queue: node = queue.popleft() if node in visited: - logger.warning( - "correction.cycle_detected", - node=node, - msg="Skipping already-visited node during BFS " - "(possible cycle in influence DAG)", - ) + # Node was already enqueued by a different parent + # (convergent / diamond topology) — skip silently. continue visited.add(node) affected.append(node) - # Follow structural tree children - structural_children = tree.get(node, []) - # Follow influence DAG dependents - influence_dependents = dag.get(node, []) - - # Union of both edge sets - for neighbor in structural_children: - if neighbor not in visited: + # Follow structural tree children first, then influence DAG + # dependents. + for neighbor in tree.get(node, []): + if neighbor in visited: + logger.warning( + "correction.cycle_detected", + node=neighbor, + source=node, + edge_type="structural", + msg="Back-edge detected during BFS " + "(cycle in decision tree or influence DAG)", + ) + elif neighbor not in visited: queue.append(neighbor) - for neighbor in influence_dependents: - if neighbor not in visited: + for neighbor in dag.get(node, []): + if neighbor in visited: + logger.warning( + "correction.cycle_detected", + node=neighbor, + source=node, + edge_type="influence", + msg="Back-edge detected during BFS " + "(cycle in decision tree or influence DAG)", + ) + elif neighbor not in visited: queue.append(neighbor) influence_count = sum(len(v) for v in dag.values()) if dag else 0 @@ -589,6 +892,95 @@ class CorrectionService: return "medium" return "high" + @staticmethod + def _collect_all_decisions( + tree: dict[str, list[str]], + dag: dict[str, list[str]], + ) -> set[str]: + """Collect every decision ID from both the tree and DAG edges. + + Gathers all nodes that appear as either keys or values in the + structural tree and influence DAG adjacency lists. + """ + all_ids: set[str] = set() + for parent, children in tree.items(): + all_ids.add(parent) + all_ids.update(children) + for source, targets in dag.items(): + all_ids.add(source) + all_ids.update(targets) + return all_ids + + @staticmethod + def _compute_rollback_tier_depth( + target_id: str, + tree: dict[str, list[str]], + ) -> int: + """Count parent hops from *target_id* up to the tree root. + + Builds a child → parent mapping from the adjacency list, then + walks upward from the target. Returns ``0`` when the target + is the root **or** when the target is not present in the tree. + + .. note:: + + A return value of ``0`` is ambiguous: it can mean either + "the target is the tree root" or "the target was not found + in the tree." Callers that need to distinguish these cases + should check whether the target appears in the tree before + calling this method. + """ + child_to_parent: dict[str, str] = {} + for parent, children in tree.items(): + for child in children: + child_to_parent[child] = parent + + depth = 0 + current = target_id + visited: set[str] = set() + while current in child_to_parent and current not in visited: + visited.add(current) + current = child_to_parent[current] + depth += 1 + + return depth + + @staticmethod + def _find_root(tree: dict[str, list[str]]) -> str | None: + """Find the root node of the tree (not a child of any node). + + Returns ``None`` if the tree is empty. + + .. note:: + + If the tree is a forest (multiple disconnected subtrees) + the first parent that is not a child of any other node is + returned, following Python dict insertion order. For + degenerate cases where every node appears as someone's + child (cycles), the first key is returned as a fallback. + """ + if not tree: + return None + all_children: set[str] = set() + for children in tree.values(): + all_children.update(children) + for parent in tree: + if parent not in all_children: + return parent + # Fallback: return the first key (degenerate tree) + return next(iter(tree)) + + @staticmethod + def _find_parent( + target_id: str, + tree: dict[str, list[str]], + ) -> str | None: + """Find the parent of *target_id* in the tree, or ``None``.""" + for parent, children in tree.items(): + if target_id in children: + return parent + return None + __all__ = [ "CorrectionService", diff --git a/src/cleveragents/domain/models/core/correction.py b/src/cleveragents/domain/models/core/correction.py index 36c9d5122..20acfdb1a 100644 --- a/src/cleveragents/domain/models/core/correction.py +++ b/src/cleveragents/domain/models/core/correction.py @@ -73,6 +73,7 @@ class CorrectionRequest(BaseModel): ) guidance: str = Field( default="", + max_length=10_000, description="Human-supplied guidance for the correction.", ) dry_run: bool = Field( @@ -121,6 +122,10 @@ class CorrectionImpact(BaseModel): default_factory=list, description="Decision IDs in the affected subtree (BFS order).", ) + excluded_decisions: list[str] = Field( + default_factory=list, + description="Decision IDs NOT in the affected subtree (preserved).", + ) affected_files: list[str] = Field( default_factory=list, description="Files touched by affected decisions.", @@ -141,6 +146,14 @@ class CorrectionImpact(BaseModel): default="full", description="Rollback granularity: full, phase, or append_only.", ) + rollback_tier_depth: int = Field( + default=0, + description=( + "Depth of the targeted decision from the tree root. " + "Tier 0 means the root itself is targeted; tier N means " + "the target is N hops below the root." + ), + ) artifacts_to_archive: list[str] = Field( default_factory=list, description="Artifact paths that should be archived on revert.", @@ -265,6 +278,14 @@ class CorrectionDryRunReport(BaseModel): default_factory=list, description="Child plans that would be rolled back.", ) + excluded_decisions: list[str] = Field( + default_factory=list, + description="Decisions NOT affected by the correction.", + ) + rollback_tier_depth: int = Field( + default=0, + description="Depth of target decision from tree root.", + ) estimated_recompute_time_seconds: float | None = Field( default=None, description="Estimated wall-clock seconds to recompute.",