From dd3770f930df28af6de128a60efc8443172d5e57 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Thu, 2 Apr 2026 16:59:40 +0000 Subject: [PATCH] refactor(correction): eliminate redundant fields in CorrectionDryRunReport (#1278) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove three top-level fields from CorrectionDryRunReport that duplicated data already present in the embedded CorrectionImpact object: - excluded_decisions → now accessed via report.impact.excluded_decisions - rollback_tier_depth → now accessed via report.impact.rollback_tier_depth - child_plans_to_rollback → now accessed via report.impact.affected_child_plans The redundant fields created a divergence risk: both models are frozen=True Pydantic models, so post-construction mutation is impossible, but nothing prevented constructing an instance where the top-level copies disagreed with the impact sub-object. Approach: Option B (nest-only) — remove the duplicated top-level fields and update all consumers to use the impact object's fields instead. Closes #1087 Co-authored-by: Jeffrey Phillips Freeman Co-committed-by: Jeffrey Phillips Freeman --- CHANGELOG.md | 8 +++++ .../correction_subtree_isolation_steps.py | 18 +++++------ robot/helper_correction_subtree_isolation.py | 17 ++++++----- robot/helper_m6_e2e_verification.py | 1 - .../services/correction_service.py | 3 -- .../domain/models/core/correction.py | 30 +++++++++++-------- 6 files changed, 44 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8cbb7463..a1a8d82b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## Unreleased (pre-3.7.0) +- Eliminated redundant fields (`excluded_decisions`, `rollback_tier_depth`, + `child_plans_to_rollback`) from `CorrectionDryRunReport` that duplicated data + already present in the embedded `CorrectionImpact` object. Consumers now + access these values via `report.impact.excluded_decisions`, + `report.impact.rollback_tier_depth`, and `report.impact.affected_child_plans` + respectively. Updated `CorrectionService.generate_dry_run_report()`, Behave + step definitions, and Robot Framework helpers to use the canonical `impact` + sub-object. (#1087) - Added a context-sensitive TUI help panel overlay toggled by `F1`, with help content that varies for main-screen, slash-command, reference, and shell prompt modes. Updated Behave and Robot coverage for help-panel diff --git a/features/steps/correction_subtree_isolation_steps.py b/features/steps/correction_subtree_isolation_steps.py index 3e85b2db6..e9fdc5c7a 100644 --- a/features/steps/correction_subtree_isolation_steps.py +++ b/features/steps/correction_subtree_isolation_steps.py @@ -199,19 +199,19 @@ def step_subtree_warnings_contain(context: Context, text: str) -> None: @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.""" + """Check that the dry-run report includes excluded decisions via impact.""" report = context.subtree_report - assert len(report.excluded_decisions) > 0, ( - f"Expected non-empty excluded_decisions, got {report.excluded_decisions}" + assert len(report.impact.excluded_decisions) > 0, ( + f"Expected non-empty excluded_decisions, got {report.impact.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.""" + """Check the rollback tier depth in the dry-run report via impact.""" report = context.subtree_report - assert report.rollback_tier_depth == expected, ( - f"Expected report tier_depth={expected}, got {report.rollback_tier_depth}" + assert report.impact.rollback_tier_depth == expected, ( + f"Expected report tier_depth={expected}, got {report.impact.rollback_tier_depth}" ) @@ -219,10 +219,10 @@ def step_subtree_report_tier(context: Context, expected: int) -> None: '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.""" + """Check the dry-run report's excluded decisions list via impact.""" report = context.subtree_report - assert decision_id in report.excluded_decisions, ( - f"Expected '{decision_id}' in report excluded={report.excluded_decisions}" + assert decision_id in report.impact.excluded_decisions, ( + f"Expected '{decision_id}' in report excluded={report.impact.excluded_decisions}" ) diff --git a/robot/helper_correction_subtree_isolation.py b/robot/helper_correction_subtree_isolation.py index 9064dc231..49cfdbbf2 100644 --- a/robot/helper_correction_subtree_isolation.py +++ b/robot/helper_correction_subtree_isolation.py @@ -117,19 +117,22 @@ def _rollback_tier_computation() -> None: def _dry_run_report_enhanced() -> None: - """Verify dry-run report includes excluded decisions and tier depth.""" + """Verify dry-run report includes excluded decisions and tier depth via impact.""" 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 report.impact.rollback_tier_depth == 1, ( + f"Expected tier_depth=1, got {report.impact.rollback_tier_depth}" + ) + assert len(report.impact.excluded_decisions) > 0, ( + "Expected non-empty excluded_decisions" ) - 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}" + assert excluded in report.impact.excluded_decisions, ( + f"Expected '{excluded}' in report " + f"excluded={report.impact.excluded_decisions}" ) print("dry-run-report-enhanced-ok") @@ -143,7 +146,7 @@ def _dry_run_tier_zero_warning() -> None: 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 + assert report.impact.rollback_tier_depth == 0 print("dry-run-tier-zero-warning-ok") diff --git a/robot/helper_m6_e2e_verification.py b/robot/helper_m6_e2e_verification.py index ef0727a3a..1ed3cb4b4 100644 --- a/robot/helper_m6_e2e_verification.py +++ b/robot/helper_m6_e2e_verification.py @@ -416,7 +416,6 @@ def correction_affected_subtree() -> None: mode=CorrectionMode.REVERT, impact=impact, decisions_to_invalidate=[target_id], - child_plans_to_rollback=[], ) if len(report.decisions_to_invalidate) != 1: diff --git a/src/cleveragents/application/services/correction_service.py b/src/cleveragents/application/services/correction_service.py index 1aa4f848b..c976c4285 100644 --- a/src/cleveragents/application/services/correction_service.py +++ b/src/cleveragents/application/services/correction_service.py @@ -391,9 +391,6 @@ class CorrectionService: decisions_to_invalidate=impact.affected_decisions 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, ) diff --git a/src/cleveragents/domain/models/core/correction.py b/src/cleveragents/domain/models/core/correction.py index d12b7f283..8400625fa 100644 --- a/src/cleveragents/domain/models/core/correction.py +++ b/src/cleveragents/domain/models/core/correction.py @@ -295,6 +295,18 @@ class CorrectionDryRunReport(BaseModel): Generated by ``CorrectionService.generate_dry_run_report`` without actually modifying any state. + + Design rationale (Option B — nest-only): + The embedded ``impact`` object is the single authoritative source for + impact data. Previously, ``excluded_decisions``, ``rollback_tier_depth``, + and ``child_plans_to_rollback`` were duplicated at the top level, creating + a divergence risk: both models are ``frozen=True``, so post-construction + mutation is impossible, but nothing prevented constructing an instance + where the top-level copies disagreed with ``impact``. Removing the + redundant fields eliminates that ambiguity. Consumers should access + these values via ``report.impact.excluded_decisions``, + ``report.impact.rollback_tier_depth``, and + ``report.impact.affected_child_plans`` respectively. """ model_config = ConfigDict(frozen=True) @@ -309,24 +321,16 @@ class CorrectionDryRunReport(BaseModel): ) impact: CorrectionImpact = Field( ..., - description="Full impact analysis.", + description=( + "Full impact analysis. Use ``impact.excluded_decisions``, " + "``impact.rollback_tier_depth``, and ``impact.affected_child_plans`` " + "to access data that was previously duplicated at the top level." + ), ) decisions_to_invalidate: list[str] = Field( default_factory=list, description="Decisions that would be marked invalid.", ) - child_plans_to_rollback: list[str] = Field( - 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.",