cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(domain): align plan lifecycle model validation with specification #1077

Merged
CoreRasurae merged 1 commits from fix/plan-lifecycle-model-validation into master 2026-03-23 23:56:34 +00:00
Conversation 5 Commits 1 Files Changed 17
+970 -30
CoreRasurae commented 2026-03-19 20:46:53 +00:00
Member
Copy Link
Copy Source

Summary

Aligns the plan lifecycle model with the specification by fixing three discrepancies:

  1. ERRORED is now terminal — Added ProcessingState.ERRORED to the is_terminal property tuple, matching the spec table where errored is marked "Terminal? Yes" for all processing phases.

  2. Per-phase state validation — Added validate_phase_state_constraints model validator enforcing:

    • APPLIED and CONSTRAINED are only valid in the APPLY phase
    • COMPLETE is only valid in STRATEGIZE or EXECUTE phases
    • QUEUED, PROCESSING, ERRORED, CANCELLED are valid in any phase
    • Invalid combinations raise ValueError at construction time

  3. Docstring update — Updated ProcessingState.COMPLETE docstring from "non-terminal" to "terminal within phase" to clarify phase-level terminality semantics.

Files Modified/Created

Core changes

  • src/cleveragents/domain/models/core/plan.py — is_terminal, model validator, docstring
  • src/cleveragents/application/services/plan_lifecycle_service.py — Fixed field assignment order in apply_plan and _perform_reversion to set processing_state before phase (avoids validator rejection during phase transitions)

New test files

  • features/plan_lifecycle_model_validation.feature — 13 Behave scenarios
  • features/steps/plan_lifecycle_model_validation_steps.py — Step definitions
  • robot/plan_lifecycle_model_validation.robot — 5 Robot Framework integration tests
  • robot/helper_plan_lifecycle_model_validation.py — Robot helper

Updated existing tests

  • features/edge_case_plan_scenarios.feature — ERRORED is now terminal
  • features/steps/database_models_lifecycle_coverage_steps.py — APPLY/COMPLETE → APPLY/APPLIED
  • features/steps/m1_sourcecode_smoke_steps.py — APPLY/COMPLETE → APPLY/QUEUED
  • features/steps/plan_cli_legacy_r2_steps.py — Added APPLY phase for APPLIED state
  • features/steps/plan_lifecycle_cli_steps.py — Fixed phase-state combo
  • features/steps/plan_persistence_steps.py — Fixed assignment order
  • robot/helper_m6_e2e_verification.py — ACTION/COMPLETE → ACTION/QUEUED
  • robot/helper_phase_reversion.py — Fixed assignment order

Quality Gates

  • Lint: ✅ All checks passed
  • Typecheck: ✅ 0 errors
  • Unit tests: ✅ 11,507 scenarios passed, 0 failed, 0 errored
  • Integration tests: ✅ 1,609 tests passed, 0 failed
  • Coverage: 97% (≥ 97% threshold met)

ISSUES CLOSED: #918

## Summary Aligns the plan lifecycle model with the specification by fixing three discrepancies: 1. **ERRORED is now terminal** — Added `ProcessingState.ERRORED` to the `is_terminal` property tuple, matching the spec table where errored is marked "Terminal? Yes" for all processing phases. 2. **Per-phase state validation** — Added `validate_phase_state_constraints` model validator enforcing: - `APPLIED` and `CONSTRAINED` are only valid in the `APPLY` phase - `COMPLETE` is only valid in `STRATEGIZE` or `EXECUTE` phases - `QUEUED`, `PROCESSING`, `ERRORED`, `CANCELLED` are valid in any phase - Invalid combinations raise `ValueError` at construction time 3. **Docstring update** — Updated `ProcessingState.COMPLETE` docstring from "non-terminal" to "terminal within phase" to clarify phase-level terminality semantics. ## Files Modified/Created ### Core changes - `src/cleveragents/domain/models/core/plan.py` — is_terminal, model validator, docstring - `src/cleveragents/application/services/plan_lifecycle_service.py` — Fixed field assignment order in `apply_plan` and `_perform_reversion` to set `processing_state` before `phase` (avoids validator rejection during phase transitions) ### New test files - `features/plan_lifecycle_model_validation.feature` — 13 Behave scenarios - `features/steps/plan_lifecycle_model_validation_steps.py` — Step definitions - `robot/plan_lifecycle_model_validation.robot` — 5 Robot Framework integration tests - `robot/helper_plan_lifecycle_model_validation.py` — Robot helper ### Updated existing tests - `features/edge_case_plan_scenarios.feature` — ERRORED is now terminal - `features/steps/database_models_lifecycle_coverage_steps.py` — APPLY/COMPLETE → APPLY/APPLIED - `features/steps/m1_sourcecode_smoke_steps.py` — APPLY/COMPLETE → APPLY/QUEUED - `features/steps/plan_cli_legacy_r2_steps.py` — Added APPLY phase for APPLIED state - `features/steps/plan_lifecycle_cli_steps.py` — Fixed phase-state combo - `features/steps/plan_persistence_steps.py` — Fixed assignment order - `robot/helper_m6_e2e_verification.py` — ACTION/COMPLETE → ACTION/QUEUED - `robot/helper_phase_reversion.py` — Fixed assignment order ## Quality Gates - Lint: ✅ All checks passed - Typecheck: ✅ 0 errors - Unit tests: ✅ 11,507 scenarios passed, 0 failed, 0 errored - Integration tests: ✅ 1,609 tests passed, 0 failed - Coverage: 97% (≥ 97% threshold met) ISSUES CLOSED: #918
CoreRasurae added this to the v3.4.0 milestone 2026-03-19 20:47:42 +00:00
CoreRasurae added the
Type
Task
 label 2026-03-19 20:47:43 +00:00
freemo requested review from freemo 2026-03-22 16:35:29 +00:00
freemo requested review from brent.edwards 2026-03-22 16:35:29 +00:00
freemo approved these changes 2026-03-23 02:46:39 +00:00
Dismissed
freemo left a comment
Owner
Copy Link
Copy Source

Review: APPROVED (with minor comments)

This is a well-crafted, specification-aligned fix. The implementation correctly makes ERRORED terminal, adds per-phase state validators, and carefully handles the Pydantic field-assignment ordering subtlety.

Minor items (non-blocking):

  1. Closing keyword format — The PR body uses ISSUES CLOSED: #918 which is the conventional-commit footer format, not a Forgejo auto-close keyword. Add Closes #918 or Fixes #918 to the PR body so the issue auto-closes on merge.

  2. ERRORED + ACTION phase test gap — The BDD scenario "ERRORED state is terminal" only tests STRATEGIZE phase. The Robot helper tests STRATEGIZE/EXECUTE/APPLY but omits ACTION. Consider adding PlanPhase.ACTION to the Robot helper's _errored_is_terminal() loop for completeness.

  3. Module docstring inconsistency — The Processing States table at the top of plan.py still reads ERRORED | Failed (may retry). Now that ERRORED is terminal, this should be updated to Failed (terminal) or Failed; includes error metadata to match the inline enum comment.

What's done well:

  • The validate_phase_state_constraints validator is clean and well-documented with spec rationale.
  • The field assignment order fix in plan_lifecycle_service.py (setting processing_state before phase) is a subtle but important correctness fix, and the explanatory comments are helpful.
  • The 13 BDD scenarios + 5 Robot test cases provide comprehensive coverage of valid/invalid phase-state combinations.
  • Step file naming (plan_lifecycle_model_validation_steps.py ↔ plan_lifecycle_model_validation.feature) follows guidelines correctly.
  • All existing test fixes correctly update now-invalid phase-state combos.
## Review: APPROVED (with minor comments) This is a well-crafted, specification-aligned fix. The implementation correctly makes ERRORED terminal, adds per-phase state validators, and carefully handles the Pydantic field-assignment ordering subtlety. ### Minor items (non-blocking): 1. **Closing keyword format** — The PR body uses `ISSUES CLOSED: #918` which is the conventional-commit footer format, not a Forgejo auto-close keyword. Add `Closes #918` or `Fixes #918` to the PR body so the issue auto-closes on merge. 2. **ERRORED + ACTION phase test gap** — The BDD scenario "ERRORED state is terminal" only tests STRATEGIZE phase. The Robot helper tests STRATEGIZE/EXECUTE/APPLY but omits ACTION. Consider adding `PlanPhase.ACTION` to the Robot helper's `_errored_is_terminal()` loop for completeness. 3. **Module docstring inconsistency** — The Processing States table at the top of `plan.py` still reads `ERRORED | Failed (may retry)`. Now that ERRORED is terminal, this should be updated to `Failed (terminal)` or `Failed; includes error metadata` to match the inline enum comment. ### What's done well: - The `validate_phase_state_constraints` validator is clean and well-documented with spec rationale. - The field assignment order fix in `plan_lifecycle_service.py` (setting `processing_state` before `phase`) is a subtle but important correctness fix, and the explanatory comments are helpful. - The 13 BDD scenarios + 5 Robot test cases provide comprehensive coverage of valid/invalid phase-state combinations. - Step file naming (`plan_lifecycle_model_validation_steps.py` ↔ `plan_lifecycle_model_validation.feature`) follows guidelines correctly. - All existing test fixes correctly update now-invalid phase-state combos.
freemo added the
Priority
Medium
State
In Review
 labels 2026-03-23 03:33:55 +00:00
freemo reviewed 2026-03-23 03:42:35 +00:00
freemo left a comment
Owner
Copy Link
Copy Source

Day 43 Review — PR #1077 fix(domain): plan lifecycle model validation

Milestone: v3.4.0
Status: Mergeable (no conflicts)

Review Notes

This PR has been reviewed for compliance with CONTRIBUTING.md standards. Key checks:

  • Commit message format: Verified Conventional Changelog format from title
  • Mergeable status: Clean
  • Milestone assignment: v3.4.0

Action Items

  • Ensure the PR body includes a closing keyword (e.g., Closes #NNN)
  • Ensure at least 2 peer reviewers are assigned
  • Verify all CI checks pass before merge

Please ensure all subtasks in the linked issue are complete before merging.

## Day 43 Review — PR #1077 `fix(domain): plan lifecycle model validation` **Milestone**: v3.4.0 **Status**: Mergeable (no conflicts) ### Review Notes This PR has been reviewed for compliance with `CONTRIBUTING.md` standards. Key checks: - **Commit message format**: Verified Conventional Changelog format from title - **Mergeable status**: Clean - **Milestone assignment**: v3.4.0 ### Action Items - Ensure the PR body includes a closing keyword (e.g., `Closes #NNN`) - Ensure at least 2 peer reviewers are assigned - Verify all CI checks pass before merge Please ensure all subtasks in the linked issue are complete before merging.
CoreRasurae commented 2026-03-23 22:21:21 +00:00
Author
Member
Copy Link
Copy Source

Code Review Report — PR #1077

PR: fix(domain): align plan lifecycle model validation with specification
Branch: fix/plan-lifecycle-model-validation
Issue: #918
Reviewer scope: Code changes in this branch + immediate surrounding code interactions.

Summary

The PR correctly addresses the three issues in #918: ERRORED terminality, per-phase state validation via model validator, and COMPLETE docstring clarification. The test coverage for the new validator itself is solid (13 Behave scenarios + 5 Robot tests). However, the review uncovered behavioral regressions in surrounding service-layer code caused by adding ERRORED to is_terminal, a database deserialization crash risk, an inconsistent assignment-ordering pattern, and test coverage gaps in the new test files.

Findings by Severity

MEDIUM Severity

M1 — Behavioral Regression: cancel_plan() now rejects ERRORED plans

  • File: plan_lifecycle_service.py:1401
  • Category: Bug / Behavioral Regression
  • Detail: cancel_plan() guards with if plan.is_terminal: raise PlanError(...). Since ERRORED is now terminal, users can no longer explicitly cancel an errored plan. Previously, a user could cancel an errored plan to signal intent (e.g., "stop retrying, mark as cancelled"). This is a user-facing behavioral change not addressed by the PR or its tests.
  • Suggestion: Either (a) change cancel_plan to check a narrower set (APPLIED only, since CANCELLED is already the target state), or (b) document this as an intentional consequence and add a test proving it.

M2 — Database Deserialization Crash Risk

  • File: infrastructure/database/models.py:749-815 (to_domain())
  • Category: Bug / Data Integrity
  • Detail: The DB schema's CHECK constraints allow phase='apply' + processing_state='complete' (no cross-column constraint exists). However, to_domain() calls Plan(phase=..., processing_state=...) which now triggers validate_phase_state_constraints and raises ValueError for this combination. If any such row exists in the database (from legacy code, manual edits, or pre-validator writes), all code paths loading that plan will crash — including plan list, plan status, etc. No migration or guard was added.
  • Suggestion: Add a defensive try/except in to_domain() that logs a warning and falls back to a safe state (e.g., coerce complete → applied when phase=apply), or add a data migration that fixes any invalid rows. At minimum, a one-time migration script should be provided.

M3 — Inconsistent Resume Paths for ERRORED Plans

  • File: plan_lifecycle_service.py:1632 vs plan_resume_service.py:255
  • Category: Bug / Inconsistency
  • Detail: PlanLifecycleService.resume_plan() uses is_terminal and now blocks ERRORED plans. But PlanResumeService.resume_plan() bypasses is_terminal entirely and directly transitions ERRORED → PROCESSING. Two resume paths with contradictory behavior for the same state.
  • Suggestion: Reconcile the two paths. If ERRORED plans should be resumable (which the spec implies via errored → processing), the lifecycle service's resume_plan should either not check is_terminal or use the same narrower set as can_revert_to (APPLIED + CANCELLED only).

M4 — Spec Tension: ERRORED Terminal vs Recovery Paths

  • File: plan.py:841-858
  • Category: Spec Compliance
  • Detail: The specification marks ERRORED as "Terminal? Yes" in the state tables, but also documents errored → processing transitions (error recovery, plan prompt). Making ERRORED terminal in is_terminal blocks cancel_plan, pause_plan, and resume_plan (lifecycle service), while revert_plan and PlanResumeService.resume_plan still allow recovery. The PR doesn't document which recovery paths remain valid or explain the "terminal but recoverable" semantics. Issue #918 itself notes this tension and says "If retry is desired, the spec should be updated; otherwise, the code should match spec" — but the PR doesn't resolve this ambiguity.
  • Suggestion: Add an explanatory comment or ADR note clarifying the design decision: ERRORED is terminal for is_terminal (preventing auto-progression, applying, etc.) but still revertable and resumable through explicit recovery paths. Alternatively, introduce an is_permanently_terminal property for guards that should truly block all recovery.

LOW Severity

L1 — Inconsistent Assignment Ordering in execute_plan()

  • File: plan_lifecycle_service.py:1095-1096
  • Category: Code Smell / Latent Bug
  • Detail: execute_plan() uses phase-first ordering (plan.phase = PlanPhase.EXECUTE then plan.processing_state = ProcessingState.QUEUED). This is the opposite of the state-first pattern established by the PR's own fixes at lines 1230-1231 and 1842-1843. It currently works by coincidence (COMPLETE is valid in EXECUTE), but the pattern is inconsistent and fragile.
  • Suggestion: Reorder to match the PR's pattern: plan.processing_state = ProcessingState.QUEUED first, then plan.phase = PlanPhase.EXECUTE.

L2 — Stale Module Docstring: ERRORED Description

  • File: plan.py:32
  • Category: Documentation
  • Detail: The Processing States table says ERRORED | Failed (may retry). The "(may retry)" wording contradicts the new terminal semantics. The PR updated line 33 (COMPLETE) but not line 32 (ERRORED).
  • Suggestion: Update to ERRORED | Failed (terminal; recovery via revert) or similar.

L3 — Stale Module Docstring: Terminal Outcomes Location

  • File: plan.py:13-15
  • Category: Documentation
  • Detail: States "Terminal outcomes live in the Apply phase's processing state: applied, constrained, errored, or cancelled." With ERRORED now terminal in all phases (not just Apply), this is misleading. CANCELLED was already terminal in all phases too.
  • Suggestion: Update to clarify that errored and cancelled are terminal in any phase, while applied and constrained are Apply-phase-specific terminals.

L4 — can_revert_to Docstring Incomplete

  • File: plan.py:960-963
  • Category: Documentation
  • Detail: Docstring says "permanently terminal state (APPLIED or CANCELLED)" but doesn't mention that ERRORED (and CONSTRAINED) are also terminal per is_terminal, just not permanently so.
  • Suggestion: Add a note: "ERRORED and CONSTRAINED are terminal (per is_terminal) but still revertable."

L5 — pause_plan() Rejects ERRORED Plans

  • File: plan_lifecycle_service.py:1588
  • Category: Behavioral Change
  • Detail: Pausing an errored plan (to switch automation to manual) is now blocked. Minor impact since pausing an already-failed plan has limited utility.

L6 — PlanResumeService.validate_eligibility() Docstring Omits ERRORED

  • File: plan_resume_service.py:104-108
  • Category: Documentation / Inconsistency
  • Detail: Docstring says "A plan is eligible for resume when it is NOT in a terminal state (applied, cancelled, constrained)" — does not list ERRORED. The method's code also does not check for ERRORED, meaning ERRORED plans remain eligible for resume here. This is now inconsistent with is_terminal including ERRORED.

TEST COVERAGE Gaps

T1 — Missing PROCESSING State Validation Tests

  • Category: Test Coverage
  • Detail: Tests verify QUEUED is valid in all phases but do not test PROCESSING. Per the validator, PROCESSING should also be valid in any phase.

T2 — Missing ERRORED in ACTION Phase Test

  • Category: Test Coverage
  • Detail: Feature tests cover ERRORED in STRATEGIZE (line 11) and APPLY (line 69). Robot tests cover STRATEGIZE/EXECUTE/APPLY. No test verifies ERRORED in ACTION phase.

T3 — Missing Negative Test: APPLIED in ACTION Phase

  • Category: Test Coverage
  • Detail: Tests verify APPLIED is rejected in STRATEGIZE and EXECUTE but not in ACTION.

T4 — Missing Negative Test: CONSTRAINED in ACTION Phase

  • Category: Test Coverage
  • Detail: Tests verify CONSTRAINED is rejected in EXECUTE but not in ACTION or STRATEGIZE.

T5 — Missing Negative Test: CONSTRAINED in STRATEGIZE Phase

  • Category: Test Coverage
  • Detail: Only EXECUTE phase is tested for CONSTRAINED rejection.

T6 — No Assignment-Ordering Regression Test

  • Category: Test Coverage
  • Detail: No test verifies that the two-step phase+state mutation pattern (state-first) doesn't trigger intermediate validation errors. This is critical because validate_assignment=True fires validators on each individual field set.

T7 — No Database Deserialization Edge-Case Test

  • Category: Test Coverage
  • Detail: No test verifies behavior when deserializing a DB record with a previously-valid but now-invalid phase/state combination.

Performance

No performance concerns. The new validator performs simple tuple-membership checks — negligible overhead even with validate_assignment=True.

Security

No security issues identified. Changes are purely domain-model validation.

Recommendation

The Medium-severity items (M1-M4) should be addressed before merge, particularly M2 (DB deserialization crash risk) and M3 (inconsistent resume paths). L1 (assignment ordering) is a quick fix that should be included for consistency with the PR's own pattern. The test coverage gaps (T1-T7) would strengthen the PR significantly.

# Code Review Report — PR #1077 **PR:** `fix(domain): align plan lifecycle model validation with specification` **Branch:** `fix/plan-lifecycle-model-validation` **Issue:** #918 **Reviewer scope:** Code changes in this branch + immediate surrounding code interactions. --- ## Summary The PR correctly addresses the three issues in #918: ERRORED terminality, per-phase state validation via model validator, and COMPLETE docstring clarification. The test coverage for the new validator itself is solid (13 Behave scenarios + 5 Robot tests). However, the review uncovered **behavioral regressions** in surrounding service-layer code caused by adding ERRORED to `is_terminal`, **a database deserialization crash risk**, an **inconsistent assignment-ordering pattern**, and **test coverage gaps** in the new test files. --- ## Findings by Severity ### MEDIUM Severity #### M1 — Behavioral Regression: `cancel_plan()` now rejects ERRORED plans - **File:** `plan_lifecycle_service.py:1401` - **Category:** Bug / Behavioral Regression - **Detail:** `cancel_plan()` guards with `if plan.is_terminal: raise PlanError(...)`. Since ERRORED is now terminal, users can no longer explicitly cancel an errored plan. Previously, a user could cancel an errored plan to signal intent (e.g., "stop retrying, mark as cancelled"). This is a user-facing behavioral change not addressed by the PR or its tests. - **Suggestion:** Either (a) change `cancel_plan` to check a narrower set (`APPLIED` only, since `CANCELLED` is already the target state), or (b) document this as an intentional consequence and add a test proving it. #### M2 — Database Deserialization Crash Risk - **File:** `infrastructure/database/models.py:749-815` (`to_domain()`) - **Category:** Bug / Data Integrity - **Detail:** The DB schema's CHECK constraints allow `phase='apply'` + `processing_state='complete'` (no cross-column constraint exists). However, `to_domain()` calls `Plan(phase=..., processing_state=...)` which now triggers `validate_phase_state_constraints` and raises `ValueError` for this combination. If any such row exists in the database (from legacy code, manual edits, or pre-validator writes), **all code paths loading that plan will crash** — including `plan list`, `plan status`, etc. No migration or guard was added. - **Suggestion:** Add a defensive `try/except` in `to_domain()` that logs a warning and falls back to a safe state (e.g., coerce `complete → applied` when `phase=apply`), or add a data migration that fixes any invalid rows. At minimum, a one-time migration script should be provided. #### M3 — Inconsistent Resume Paths for ERRORED Plans - **File:** `plan_lifecycle_service.py:1632` vs `plan_resume_service.py:255` - **Category:** Bug / Inconsistency - **Detail:** `PlanLifecycleService.resume_plan()` uses `is_terminal` and now blocks ERRORED plans. But `PlanResumeService.resume_plan()` bypasses `is_terminal` entirely and directly transitions ERRORED → PROCESSING. Two resume paths with contradictory behavior for the same state. - **Suggestion:** Reconcile the two paths. If ERRORED plans should be resumable (which the spec implies via `errored → processing`), the lifecycle service's `resume_plan` should either not check `is_terminal` or use the same narrower set as `can_revert_to` (APPLIED + CANCELLED only). #### M4 — Spec Tension: ERRORED Terminal vs Recovery Paths - **File:** `plan.py:841-858` - **Category:** Spec Compliance - **Detail:** The specification marks ERRORED as "Terminal? Yes" in the state tables, but also documents `errored → processing` transitions (error recovery, plan prompt). Making ERRORED terminal in `is_terminal` blocks `cancel_plan`, `pause_plan`, and `resume_plan` (lifecycle service), while `revert_plan` and `PlanResumeService.resume_plan` still allow recovery. The PR doesn't document which recovery paths remain valid or explain the "terminal but recoverable" semantics. Issue #918 itself notes this tension and says "If retry is desired, the spec should be updated; otherwise, the code should match spec" — but the PR doesn't resolve this ambiguity. - **Suggestion:** Add an explanatory comment or ADR note clarifying the design decision: ERRORED is terminal for `is_terminal` (preventing auto-progression, applying, etc.) but still revertable and resumable through explicit recovery paths. Alternatively, introduce an `is_permanently_terminal` property for guards that should truly block all recovery. --- ### LOW Severity #### L1 — Inconsistent Assignment Ordering in `execute_plan()` - **File:** `plan_lifecycle_service.py:1095-1096` - **Category:** Code Smell / Latent Bug - **Detail:** `execute_plan()` uses phase-first ordering (`plan.phase = PlanPhase.EXECUTE` then `plan.processing_state = ProcessingState.QUEUED`). This is the **opposite** of the state-first pattern established by the PR's own fixes at lines 1230-1231 and 1842-1843. It currently works by coincidence (COMPLETE is valid in EXECUTE), but the pattern is inconsistent and fragile. - **Suggestion:** Reorder to match the PR's pattern: `plan.processing_state = ProcessingState.QUEUED` first, then `plan.phase = PlanPhase.EXECUTE`. #### L2 — Stale Module Docstring: ERRORED Description - **File:** `plan.py:32` - **Category:** Documentation - **Detail:** The Processing States table says `ERRORED | Failed (may retry)`. The "(may retry)" wording contradicts the new terminal semantics. The PR updated line 33 (COMPLETE) but not line 32 (ERRORED). - **Suggestion:** Update to `ERRORED | Failed (terminal; recovery via revert)` or similar. #### L3 — Stale Module Docstring: Terminal Outcomes Location - **File:** `plan.py:13-15` - **Category:** Documentation - **Detail:** States "Terminal outcomes live in the Apply phase's processing state: applied, constrained, errored, or cancelled." With ERRORED now terminal in **all** phases (not just Apply), this is misleading. CANCELLED was already terminal in all phases too. - **Suggestion:** Update to clarify that `errored` and `cancelled` are terminal in any phase, while `applied` and `constrained` are Apply-phase-specific terminals. #### L4 — `can_revert_to` Docstring Incomplete - **File:** `plan.py:960-963` - **Category:** Documentation - **Detail:** Docstring says "permanently terminal state (APPLIED or CANCELLED)" but doesn't mention that ERRORED (and CONSTRAINED) are also terminal per `is_terminal`, just not permanently so. - **Suggestion:** Add a note: "ERRORED and CONSTRAINED are terminal (per `is_terminal`) but still revertable." #### L5 — `pause_plan()` Rejects ERRORED Plans - **File:** `plan_lifecycle_service.py:1588` - **Category:** Behavioral Change - **Detail:** Pausing an errored plan (to switch automation to manual) is now blocked. Minor impact since pausing an already-failed plan has limited utility. #### L6 — `PlanResumeService.validate_eligibility()` Docstring Omits ERRORED - **File:** `plan_resume_service.py:104-108` - **Category:** Documentation / Inconsistency - **Detail:** Docstring says "A plan is eligible for resume when it is NOT in a terminal state (applied, cancelled, constrained)" — does not list ERRORED. The method's code also does not check for ERRORED, meaning ERRORED plans remain eligible for resume here. This is now inconsistent with `is_terminal` including ERRORED. --- ### TEST COVERAGE Gaps #### T1 — Missing PROCESSING State Validation Tests - **Category:** Test Coverage - **Detail:** Tests verify QUEUED is valid in all phases but do not test PROCESSING. Per the validator, PROCESSING should also be valid in any phase. #### T2 — Missing ERRORED in ACTION Phase Test - **Category:** Test Coverage - **Detail:** Feature tests cover ERRORED in STRATEGIZE (line 11) and APPLY (line 69). Robot tests cover STRATEGIZE/EXECUTE/APPLY. No test verifies ERRORED in ACTION phase. #### T3 — Missing Negative Test: APPLIED in ACTION Phase - **Category:** Test Coverage - **Detail:** Tests verify APPLIED is rejected in STRATEGIZE and EXECUTE but not in ACTION. #### T4 — Missing Negative Test: CONSTRAINED in ACTION Phase - **Category:** Test Coverage - **Detail:** Tests verify CONSTRAINED is rejected in EXECUTE but not in ACTION or STRATEGIZE. #### T5 — Missing Negative Test: CONSTRAINED in STRATEGIZE Phase - **Category:** Test Coverage - **Detail:** Only EXECUTE phase is tested for CONSTRAINED rejection. #### T6 — No Assignment-Ordering Regression Test - **Category:** Test Coverage - **Detail:** No test verifies that the two-step phase+state mutation pattern (state-first) doesn't trigger intermediate validation errors. This is critical because `validate_assignment=True` fires validators on each individual field set. #### T7 — No Database Deserialization Edge-Case Test - **Category:** Test Coverage - **Detail:** No test verifies behavior when deserializing a DB record with a previously-valid but now-invalid phase/state combination. --- ## Performance No performance concerns. The new validator performs simple tuple-membership checks — negligible overhead even with `validate_assignment=True`. ## Security No security issues identified. Changes are purely domain-model validation. --- ## Recommendation The Medium-severity items (M1-M4) should be addressed before merge, particularly M2 (DB deserialization crash risk) and M3 (inconsistent resume paths). L1 (assignment ordering) is a quick fix that should be included for consistency with the PR's own pattern. The test coverage gaps (T1-T7) would strengthen the PR significantly.
CoreRasurae force-pushed fix/plan-lifecycle-model-validation from 7ebb687c91 to 464e6281a3 2026-03-23 22:47:08 +00:00 Compare
CoreRasurae dismissed freemo's review 2026-03-23 22:47:08 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

CoreRasurae reviewed 2026-03-23 22:58:40 +00:00
CoreRasurae left a comment
Author
Member
Copy Link
Copy Source

Code Review Report — PR #1077 fix(domain): align plan lifecycle model validation with specification

Reviewer: Automated Code Review (2 full cycles across all categories)
Branch: fix/plan-lifecycle-model-validation
Commit: 464e628
Issue: #918

Overall Assessment

The PR correctly addresses the core issue: aligning ERRORED terminality and adding per-phase state validation. The implementation is well-structured, the commit message is thorough, and the new tests are comprehensive. However, the review identified several issues across bug detection, test coverage, data integrity, and spec compliance that warrant attention before merge.

Findings by Severity

HIGH — Behavioral Regression / Bug Risk

H1. cancel_plan() now rejects ERRORED plans

  • File: plan_lifecycle_service.py:1403
  • Category: Bug / Behavioral Regression
  • Detail: The is_terminal guard in cancel_plan() now blocks cancellation of ERRORED plans. Before this change, a user could explicitly cancel an errored plan (e.g., "I don't want to retry, just mark it done"). Now the user gets "Plan {id} is already in terminal state and cannot be cancelled". While the spec says ERRORED is terminal, the UX impact should be considered: a user seeing a plan stuck in ERRORED may attempt to cancel it for cleanup. The error message doesn't guide them toward the alternative (revert_plan).
  • Suggestion: Either (a) allow cancel_plan to accept ERRORED plans as a convenience (making it idempotent for terminal states), or (b) improve the error message to suggest using revert_plan for ERRORED plans, or (c) add a note in the is_terminal docstring acknowledging this trade-off.

H2. Silent defensive coercion without logging

  • File: infrastructure/database/models.py:752-765
  • Category: Data Integrity / Observability
  • Detail: The to_domain() coercion silently rewrites legacy DB values (e.g., APPLY/COMPLETE → APPLY/APPLIED, STRATEGIZE/APPLIED → STRATEGIZE/ERRORED) without any logging or warning. In production, this makes it impossible to detect when legacy data is being coerced and to differentiate between genuine ERRORED plans and coerced ones. This could mask data corruption.
  • Suggestion: Add logger.warning("Coercing legacy phase/state combo ...", phase=..., old_state=..., new_state=...) to each coercion branch.

H3. Incomplete test coverage for coercion branches

  • File: robot/helper_plan_lifecycle_model_validation.py:159-193
  • Category: Test Coverage
  • Detail: The _deserialization_coercion() function only tests one of the three coercion branches (APPLY/COMPLETE → APPLIED). The remaining two branches are untested:
    • APPLIED or CONSTRAINED in non-APPLY phases → ERRORED
    • COMPLETE in ACTION phase → QUEUED
  • Suggestion: Add test cases for all three coercion branches.

H4. No BDD scenario for deserialization coercion

  • File: features/plan_lifecycle_model_validation.feature
  • Category: Test Coverage
  • Detail: The deserialization coercion is only tested at the Robot level. There is no BDD scenario covering this critical data path, creating a gap in the BDD-level specification.
  • Suggestion: Add at least one BDD scenario exercising the coercion through the full persistence round-trip.

MEDIUM — Correctness Concern / Spec Compliance

M1. PlanResumeService.validate_eligibility() docstring is stale

  • File: plan_resume_service.py:107-108
  • Category: Documentation / Correctness
  • Detail: The docstring says "A plan is eligible for resume when it is NOT in a terminal state (applied, cancelled, constrained)". With ERRORED now terminal per is_terminal, this docstring is misleading — ERRORED is terminal but IS eligible for resume (the method checks individual states, not is_terminal). The asymmetry between PlanLifecycleService.resume_plan() (blocks ERRORED via is_terminal) and PlanResumeService.resume_plan() (allows ERRORED) is subtle and could confuse maintainers.
  • Suggestion: Update the docstring to list ERRORED as "terminal but eligible for resume" and clarify the distinction between the two resume paths.

M2. Phase-first assignment ordering in test code not updated

  • File: Multiple files not modified by this PR
  • Category: Fragility / Latent Regression
  • Detail: Several test files use phase-first ordering (plan.phase = X before plan.processing_state = Y) which is fragile with the new validate_phase_state_constraints validator:
    • features/steps/plan_repository_steps.py:251-252
    • features/steps/phase_reversion_steps.py:109 (fallback path)
    • robot/helper_plan_persistence_e2e.py:130,142,153
    • robot/helper_persistence_lifecycle.py:145,156,167
      These currently pass because the intermediate states happen to be universally valid (QUEUED, PROCESSING), but future test changes could unknowingly trigger the validator with incompatible intermediate states.
  • Suggestion: Either fix the ordering in these files for consistency, or add a brief comment in the PR noting this is a known fragility for future cleanup.

M3. Coercion maps APPLIED/CONSTRAINED to ERRORED for non-APPLY phases

  • File: infrastructure/database/models.py:759-763
  • Category: Data Integrity
  • Detail: If something writes STRATEGIZE/APPLIED to the DB, it's a data corruption bug that should be investigated, not silently "fixed" to STRATEGIZE/ERRORED. The coercion makes the corruption invisible. The ERRORED state also has semantic meaning (processing failed), which doesn't accurately describe a data integrity issue.
  • Suggestion: Consider using QUEUED instead of ERRORED as the coercion target for APPLIED/CONSTRAINED in non-APPLY phases, since it's a neutral "reset" state. Or, at minimum, log at WARNING level (see H2).

M4. Inconsistent "Terminal" interpretation between ERRORED and COMPLETE

  • File: domain/models/core/plan.py:841-869
  • Category: Spec Compliance
  • Detail: The spec marks both ERRORED and COMPLETE as "Terminal? Yes" in the Strategize/Execute phase tables (spec lines 18256-18257). The code treats ERRORED as plan-terminal (is_terminal → True) but COMPLETE as phase-terminal only (is_terminal → False). While this is a deliberate and reasonable design choice (COMPLETE means "advance to next phase," not "stop"), the asymmetric interpretation of the same spec column isn't formally documented.
  • Suggestion: Consider adding a brief ADR note or a comment block in the Plan model explaining why "Terminal? Yes" in the spec means different things for ERRORED vs COMPLETE.

M5. No test for cancel_plan() behavioral change with ERRORED

  • File: N/A (missing test)
  • Category: Test Coverage
  • Detail: The behavioral change where ERRORED plans can no longer be cancelled has no test coverage. This should be tested to prevent future regressions.
  • Suggestion: Add a scenario: "Cancelling an ERRORED plan is rejected because it is terminal."

M6. No negative test for phase-first assignment order

  • File: features/plan_lifecycle_model_validation.feature
  • Category: Test Coverage
  • Detail: There is a positive test that state-first assignment works (the "Assignment-order regression" scenario), but no test verifying that the wrong order (phase-first with an invalid intermediate state) actually raises a validation error. This would catch regressions if validate_assignment=True is accidentally removed.
  • Suggestion: Add a scenario that attempts phase-first assignment from e.g., STRATEGIZE/COMPLETE → APPLY, demonstrating the validator rejects the intermediate APPLY/COMPLETE state.

LOW — Minor / Informational

L1. No test for pause_plan() and resume_plan() (lifecycle) on ERRORED

  • File: N/A (missing test)
  • Category: Test Coverage
  • Detail: The behavioral changes where pause_plan() and PlanLifecycleService.resume_plan() now reject ERRORED plans are untested.

L2. CLI active plan resolution excludes ERRORED plans

  • File: cli/commands/plan.py:3067
  • Category: Behavioral Change
  • Detail: active = [p for p in plans if not p.is_terminal] now excludes ERRORED plans. Previously, an ERRORED plan in STRATEGIZE would still be auto-resolved as the "active" plan. This is correct behavior but is an undocumented behavioral change.

L3. Multiple model validators on every field assignment

  • File: domain/models/core/plan.py (Plan model_config)
  • Category: Performance
  • Detail: With validate_assignment=True, every individual field assignment triggers all model validators (including the new validate_phase_state_constraints). In service methods that mutate multiple Plan fields sequentially, this means multiple full validation passes. The overhead is O(1) per validator but cumulative across the ~5 validators on Plan. This is unlikely to be a bottleneck but worth being aware of.

Summary Table

ID Severity Category Summary
H1 High Bug/Regression cancel_plan() rejects ERRORED plans
H2 High Data Integrity Silent coercion without logging
H3 High Test Coverage Only 1 of 3 coercion branches tested
H4 High Test Coverage No BDD scenario for deserialization coercion
M1 Medium Documentation PlanResumeService docstring stale
M2 Medium Fragility Phase-first ordering in unmodified test files
M3 Medium Data Integrity APPLIED → ERRORED coercion semantics
M4 Medium Spec Compliance Asymmetric "Terminal" interpretation undocumented
M5 Medium Test Coverage No test for cancel-on-ERRORED change
M6 Medium Test Coverage No negative test for phase-first ordering
L1 Low Test Coverage No test for pause/resume on ERRORED
L2 Low Behavioral Change CLI active plan excludes ERRORED
L3 Low Performance Validator overhead on assignment

Recommendation: Address H1-H4 and M1 before merge. The remaining medium and low items can be tracked as follow-up tasks if desired.

## Code Review Report — PR #1077 `fix(domain): align plan lifecycle model validation with specification` **Reviewer:** Automated Code Review (2 full cycles across all categories) **Branch:** `fix/plan-lifecycle-model-validation` **Commit:** `464e628` **Issue:** #918 --- ### Overall Assessment The PR correctly addresses the core issue: aligning ERRORED terminality and adding per-phase state validation. The implementation is well-structured, the commit message is thorough, and the new tests are comprehensive. However, the review identified several issues across bug detection, test coverage, data integrity, and spec compliance that warrant attention before merge. --- ## Findings by Severity ### HIGH — Behavioral Regression / Bug Risk **H1. `cancel_plan()` now rejects ERRORED plans** - **File:** `plan_lifecycle_service.py:1403` - **Category:** Bug / Behavioral Regression - **Detail:** The `is_terminal` guard in `cancel_plan()` now blocks cancellation of ERRORED plans. Before this change, a user could explicitly cancel an errored plan (e.g., "I don't want to retry, just mark it done"). Now the user gets `"Plan {id} is already in terminal state and cannot be cancelled"`. While the spec says ERRORED is terminal, the UX impact should be considered: a user seeing a plan stuck in ERRORED may attempt to cancel it for cleanup. The error message doesn't guide them toward the alternative (`revert_plan`). - **Suggestion:** Either (a) allow `cancel_plan` to accept ERRORED plans as a convenience (making it idempotent for terminal states), or (b) improve the error message to suggest using `revert_plan` for ERRORED plans, or (c) add a note in the `is_terminal` docstring acknowledging this trade-off. **H2. Silent defensive coercion without logging** - **File:** `infrastructure/database/models.py:752-765` - **Category:** Data Integrity / Observability - **Detail:** The `to_domain()` coercion silently rewrites legacy DB values (e.g., APPLY/COMPLETE → APPLY/APPLIED, STRATEGIZE/APPLIED → STRATEGIZE/ERRORED) without any logging or warning. In production, this makes it impossible to detect when legacy data is being coerced and to differentiate between genuine ERRORED plans and coerced ones. This could mask data corruption. - **Suggestion:** Add `logger.warning("Coercing legacy phase/state combo ...", phase=..., old_state=..., new_state=...)` to each coercion branch. **H3. Incomplete test coverage for coercion branches** - **File:** `robot/helper_plan_lifecycle_model_validation.py:159-193` - **Category:** Test Coverage - **Detail:** The `_deserialization_coercion()` function only tests one of the three coercion branches (APPLY/COMPLETE → APPLIED). The remaining two branches are untested: - `APPLIED` or `CONSTRAINED` in non-APPLY phases → `ERRORED` - `COMPLETE` in ACTION phase → `QUEUED` - **Suggestion:** Add test cases for all three coercion branches. **H4. No BDD scenario for deserialization coercion** - **File:** `features/plan_lifecycle_model_validation.feature` - **Category:** Test Coverage - **Detail:** The deserialization coercion is only tested at the Robot level. There is no BDD scenario covering this critical data path, creating a gap in the BDD-level specification. - **Suggestion:** Add at least one BDD scenario exercising the coercion through the full persistence round-trip. --- ### MEDIUM — Correctness Concern / Spec Compliance **M1. `PlanResumeService.validate_eligibility()` docstring is stale** - **File:** `plan_resume_service.py:107-108` - **Category:** Documentation / Correctness - **Detail:** The docstring says *"A plan is eligible for resume when it is NOT in a terminal state (applied, cancelled, constrained)"*. With ERRORED now terminal per `is_terminal`, this docstring is misleading — ERRORED is terminal but IS eligible for resume (the method checks individual states, not `is_terminal`). The asymmetry between `PlanLifecycleService.resume_plan()` (blocks ERRORED via `is_terminal`) and `PlanResumeService.resume_plan()` (allows ERRORED) is subtle and could confuse maintainers. - **Suggestion:** Update the docstring to list ERRORED as "terminal but eligible for resume" and clarify the distinction between the two resume paths. **M2. Phase-first assignment ordering in test code not updated** - **File:** Multiple files not modified by this PR - **Category:** Fragility / Latent Regression - **Detail:** Several test files use phase-first ordering (`plan.phase = X` before `plan.processing_state = Y`) which is fragile with the new `validate_phase_state_constraints` validator: - `features/steps/plan_repository_steps.py:251-252` - `features/steps/phase_reversion_steps.py:109` (fallback path) - `robot/helper_plan_persistence_e2e.py:130,142,153` - `robot/helper_persistence_lifecycle.py:145,156,167` These currently pass because the intermediate states happen to be universally valid (QUEUED, PROCESSING), but future test changes could unknowingly trigger the validator with incompatible intermediate states. - **Suggestion:** Either fix the ordering in these files for consistency, or add a brief comment in the PR noting this is a known fragility for future cleanup. **M3. Coercion maps APPLIED/CONSTRAINED to ERRORED for non-APPLY phases** - **File:** `infrastructure/database/models.py:759-763` - **Category:** Data Integrity - **Detail:** If something writes `STRATEGIZE/APPLIED` to the DB, it's a data corruption bug that should be investigated, not silently "fixed" to `STRATEGIZE/ERRORED`. The coercion makes the corruption invisible. The ERRORED state also has semantic meaning (processing failed), which doesn't accurately describe a data integrity issue. - **Suggestion:** Consider using QUEUED instead of ERRORED as the coercion target for APPLIED/CONSTRAINED in non-APPLY phases, since it's a neutral "reset" state. Or, at minimum, log at WARNING level (see H2). **M4. Inconsistent "Terminal" interpretation between ERRORED and COMPLETE** - **File:** `domain/models/core/plan.py:841-869` - **Category:** Spec Compliance - **Detail:** The spec marks both ERRORED and COMPLETE as "Terminal? Yes" in the Strategize/Execute phase tables (spec lines 18256-18257). The code treats ERRORED as plan-terminal (`is_terminal` → True) but COMPLETE as phase-terminal only (`is_terminal` → False). While this is a deliberate and reasonable design choice (COMPLETE means "advance to next phase," not "stop"), the asymmetric interpretation of the same spec column isn't formally documented. - **Suggestion:** Consider adding a brief ADR note or a comment block in the Plan model explaining why "Terminal? Yes" in the spec means different things for ERRORED vs COMPLETE. **M5. No test for `cancel_plan()` behavioral change with ERRORED** - **File:** N/A (missing test) - **Category:** Test Coverage - **Detail:** The behavioral change where ERRORED plans can no longer be cancelled has no test coverage. This should be tested to prevent future regressions. - **Suggestion:** Add a scenario: "Cancelling an ERRORED plan is rejected because it is terminal." **M6. No negative test for phase-first assignment order** - **File:** `features/plan_lifecycle_model_validation.feature` - **Category:** Test Coverage - **Detail:** There is a positive test that state-first assignment works (the "Assignment-order regression" scenario), but no test verifying that the wrong order (phase-first with an invalid intermediate state) actually raises a validation error. This would catch regressions if `validate_assignment=True` is accidentally removed. - **Suggestion:** Add a scenario that attempts phase-first assignment from e.g., STRATEGIZE/COMPLETE → APPLY, demonstrating the validator rejects the intermediate APPLY/COMPLETE state. --- ### LOW — Minor / Informational **L1. No test for `pause_plan()` and `resume_plan()` (lifecycle) on ERRORED** - **File:** N/A (missing test) - **Category:** Test Coverage - **Detail:** The behavioral changes where `pause_plan()` and `PlanLifecycleService.resume_plan()` now reject ERRORED plans are untested. **L2. CLI active plan resolution excludes ERRORED plans** - **File:** `cli/commands/plan.py:3067` - **Category:** Behavioral Change - **Detail:** `active = [p for p in plans if not p.is_terminal]` now excludes ERRORED plans. Previously, an ERRORED plan in STRATEGIZE would still be auto-resolved as the "active" plan. This is correct behavior but is an undocumented behavioral change. **L3. Multiple model validators on every field assignment** - **File:** `domain/models/core/plan.py` (Plan model_config) - **Category:** Performance - **Detail:** With `validate_assignment=True`, every individual field assignment triggers all model validators (including the new `validate_phase_state_constraints`). In service methods that mutate multiple Plan fields sequentially, this means multiple full validation passes. The overhead is O(1) per validator but cumulative across the ~5 validators on Plan. This is unlikely to be a bottleneck but worth being aware of. --- ### Summary Table | ID | Severity | Category | Summary | |----|----------|----------|---------| | H1 | High | Bug/Regression | `cancel_plan()` rejects ERRORED plans | | H2 | High | Data Integrity | Silent coercion without logging | | H3 | High | Test Coverage | Only 1 of 3 coercion branches tested | | H4 | High | Test Coverage | No BDD scenario for deserialization coercion | | M1 | Medium | Documentation | `PlanResumeService` docstring stale | | M2 | Medium | Fragility | Phase-first ordering in unmodified test files | | M3 | Medium | Data Integrity | APPLIED → ERRORED coercion semantics | | M4 | Medium | Spec Compliance | Asymmetric "Terminal" interpretation undocumented | | M5 | Medium | Test Coverage | No test for cancel-on-ERRORED change | | M6 | Medium | Test Coverage | No negative test for phase-first ordering | | L1 | Low | Test Coverage | No test for pause/resume on ERRORED | | L2 | Low | Behavioral Change | CLI active plan excludes ERRORED | | L3 | Low | Performance | Validator overhead on assignment | --- **Recommendation:** Address H1-H4 and M1 before merge. The remaining medium and low items can be tracked as follow-up tasks if desired.
CoreRasurae force-pushed fix/plan-lifecycle-model-validation from 464e6281a3 to a4fd59bd27 2026-03-23 23:28:23 +00:00 Compare
CoreRasurae force-pushed fix/plan-lifecycle-model-validation from a4fd59bd27 to 9e316b1a3e 2026-03-23 23:33:54 +00:00 Compare
CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-03-23 23:34:24 +00:00
CoreRasurae canceled auto merging this pull request when all checks succeed 2026-03-23 23:34:51 +00:00
CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-03-23 23:35:25 +00:00
CoreRasurae reviewed 2026-03-23 23:52:42 +00:00
CoreRasurae left a comment
Author
Member
Copy Link
Copy Source

Code Review Report — PR #1077 (fix/plan-lifecycle-model-validation)

Reviewed commit: 9e316b1 by Luis Mendes
Issue: #918 — Align plan lifecycle model with spec (ERRORED terminality, phase-state validation)
Reviewer method: 3 full review cycles (bugs, security, performance, test coverage, test flaws, code quality) until convergence (no new findings on cycles 2 and 3).

Summary

The commit correctly addresses all three acceptance criteria from #918: ERRORED is now terminal in is_terminal, model-level phase-state validation is enforced, and the COMPLETE docstring clarifies phase-level terminality. The state-first assignment ordering fix and defensive deserialization coercion are well-designed. Test coverage is comprehensive (28 Behave scenarios + 9 Robot tests). The findings below are refinement opportunities, not blockers.

Findings by Severity and Category

MEDIUM — Documentation Bug

B1. is_terminal docstring incorrectly claims CONSTRAINED is resumable via PlanResumeService

  • File: src/cleveragents/domain/models/core/plan.py:870-872
  • Lines: The docstring states:

    "ERRORED and CONSTRAINED plans can still be reverted to an earlier phase via revert_plan or resumed through PlanResumeService."

  • Problem: PlanResumeService.validate_eligibility() explicitly blocks CONSTRAINED plans (returns eligible=False with TERMINAL_CONSTRAINED reason at plan_resume_service.py:148-156). Only ERRORED is eligible for resume via PlanResumeService; CONSTRAINED can only be reverted.
  • Suggested fix:

    "ERRORED and CONSTRAINED plans can still be reverted to an earlier phase via revert_plan. ERRORED plans can additionally be resumed through PlanResumeService; CONSTRAINED plans require reversion."

MEDIUM — Test Coverage Gap

T1. "QUEUED valid in any phase" scenario omits ACTION phase

  • File: features/plan_lifecycle_model_validation.feature:41-44
  • Problem: The scenario tests STRATEGIZE, EXECUTE, and APPLY but not ACTION. The validator docstring explicitly states QUEUED is valid in any phase. Missing ACTION reduces confidence in the ACTION-phase handling path.
  • Suggested fix: Add And a plmv plan in ACTION phase with QUEUED state to the Given steps: 
    Scenario: QUEUED valid in any phase
  Given a plmv plan in ACTION phase with QUEUED state
  And a plmv plan in STRATEGIZE phase with QUEUED state
  And a plmv plan in EXECUTE phase with QUEUED state
  And a plmv plan in APPLY phase with QUEUED state
  Then all plmv plans should be valid

MEDIUM — Test Flaw

T2. "Not cancellable/pausable/resumable" test assertions are indirect proxies

  • File: features/steps/plan_lifecycle_model_validation_steps.py:186-210
  • Problem: The steps step_plmv_not_cancellable, step_plmv_not_pausable, and step_plmv_not_resumable_lifecycle all assert is_terminal only. They don't verify the actual service-layer guards (cancel_plan(), pause_plan(), resume_plan()). The feature file scenario names ("cannot be cancelled", "cannot be paused") imply behavioral testing, but the assertions only test the model property.
  • Impact: If a service method's guard condition diverges from is_terminal in the future, these tests would pass erroneously.
  • Note: For a model-level validation test file this is acceptable, but consider either renaming the scenarios to reflect they test the is_terminal property, or adding service-level integration tests in a separate file.

LOW — Test Coverage Gap

T3. Missing Behave deserialization scenario for EXECUTE/CONSTRAINED coercion

  • Problem: The Robot helper covers EXECUTE/CONSTRAINED -> EXECUTE/ERRORED (branch 2b in _deserialization_coercion_all_branches), but there is no matching Behave scenario. This creates asymmetric coverage between the two test layers.
  • Suggested fix: Add a fourth Behave deserialization scenario: 
    Scenario: Legacy EXECUTE/CONSTRAINED is coerced to EXECUTE/ERRORED on deserialization
  When I plmv deserialize a plan with phase "execute" and state "constrained"
  Then the plmv deserialized plan should have phase EXECUTE and state ERRORED

LOW — Code Robustness

C1. step_update_plan_phase_state state-first pattern is fragile for phase-restricted target states

  • File: features/steps/plan_persistence_steps.py:272-273
  • Problem: The state-first assignment (processing_state = X; phase = Y) can fail if the target state is phase-restricted (APPLIED, CONSTRAINED, COMPLETE) and the current phase doesn't allow it. For example, transitioning from EXECUTE/PROCESSING to APPLY/APPLIED in one step would create the invalid intermediate EXECUTE/APPLIED.
  • Current impact: All existing persistence test scenarios are safe because restricted states are only set when already in the correct phase. This is a latent fragility, not an active bug.
  • Suggested fix (defensive): Use a 3-step safe pattern: 
    plan.processing_state = ProcessingState.QUEUED   # universally safe intermediate
plan.phase = PlanPhase(phase)
plan.processing_state = ProcessingState(state)

LOW — Documentation

C2. Coercion semantic choice lacks justification comment

  • File: src/cleveragents/infrastructure/database/models.py:770-782
  • Problem: Coercing APPLIED (success semantics) to ERRORED (failure semantics) for legacy rows outside APPLY phase is a lossy transformation. The code comment explains what happens but not why ERRORED was chosen over alternatives like QUEUED (neutral reset). A brief rationale would help future maintainers.
  • Suggested addition to comment:

    "ERRORED is chosen over QUEUED because these legacy rows represent a state that should not auto-progress; ERRORED is terminal and safe, preventing unintended lifecycle advancement."

Areas Reviewed with No Issues Found

Category Notes
Security No injection vectors, no PII in logs, no unsafe deserialization
Performance Validator overhead is O(1) tuple-membership checks; negligible
Spec alignment All three acceptance criteria from #918 met; phase/state tables match spec lines 18250-18269
Validator ordering validate_phase_state_consistency (ACTION/None→QUEUED) runs before validate_phase_state_constraints; correct
Service-layer state assignments All 14 processing_state assignments in plan_lifecycle_service.py verified safe (phase-guarded or universally valid states)
can_revert_to / revert_plan Correctly allows ERRORED and CONSTRAINED (only blocks APPLIED/CANCELLED); unaffected by is_terminal change
to_domain() coercion completeness All invalid phase/state combinations are handled by the three coercion branches
Existing test updates All 7 pre-existing test file changes correctly adapt to the new validator

Review performed using 3 global review cycles across all categories (bugs, security, performance, test coverage, test flaws, code quality). Cycles 2 and 3 found no new issues, confirming convergence.

# Code Review Report — PR #1077 (`fix/plan-lifecycle-model-validation`) **Reviewed commit:** `9e316b1` by Luis Mendes **Issue:** #918 — Align plan lifecycle model with spec (ERRORED terminality, phase-state validation) **Reviewer method:** 3 full review cycles (bugs, security, performance, test coverage, test flaws, code quality) until convergence (no new findings on cycles 2 and 3). --- ## Summary The commit correctly addresses all three acceptance criteria from #918: ERRORED is now terminal in `is_terminal`, model-level phase-state validation is enforced, and the COMPLETE docstring clarifies phase-level terminality. The state-first assignment ordering fix and defensive deserialization coercion are well-designed. Test coverage is comprehensive (28 Behave scenarios + 9 Robot tests). The findings below are refinement opportunities, not blockers. --- ## Findings by Severity and Category ### MEDIUM — Documentation Bug **B1. `is_terminal` docstring incorrectly claims CONSTRAINED is resumable via `PlanResumeService`** - **File:** `src/cleveragents/domain/models/core/plan.py:870-872` - **Lines:** The docstring states: > *"ERRORED and CONSTRAINED plans can still be reverted to an earlier phase via `revert_plan` or resumed through `PlanResumeService`."* - **Problem:** `PlanResumeService.validate_eligibility()` explicitly blocks CONSTRAINED plans (returns `eligible=False` with `TERMINAL_CONSTRAINED` reason at `plan_resume_service.py:148-156`). Only ERRORED is eligible for resume via `PlanResumeService`; CONSTRAINED can only be *reverted*. - **Suggested fix:** > *"ERRORED and CONSTRAINED plans can still be reverted to an earlier phase via `revert_plan`. ERRORED plans can additionally be resumed through `PlanResumeService`; CONSTRAINED plans require reversion."* --- ### MEDIUM — Test Coverage Gap **T1. "QUEUED valid in any phase" scenario omits ACTION phase** - **File:** `features/plan_lifecycle_model_validation.feature:41-44` - **Problem:** The scenario tests STRATEGIZE, EXECUTE, and APPLY but not ACTION. The validator docstring explicitly states QUEUED is valid in *any* phase. Missing ACTION reduces confidence in the ACTION-phase handling path. - **Suggested fix:** Add `And a plmv plan in ACTION phase with QUEUED state` to the Given steps: ```gherkin Scenario: QUEUED valid in any phase Given a plmv plan in ACTION phase with QUEUED state And a plmv plan in STRATEGIZE phase with QUEUED state And a plmv plan in EXECUTE phase with QUEUED state And a plmv plan in APPLY phase with QUEUED state Then all plmv plans should be valid ``` --- ### MEDIUM — Test Flaw **T2. "Not cancellable/pausable/resumable" test assertions are indirect proxies** - **File:** `features/steps/plan_lifecycle_model_validation_steps.py:186-210` - **Problem:** The steps `step_plmv_not_cancellable`, `step_plmv_not_pausable`, and `step_plmv_not_resumable_lifecycle` all assert `is_terminal` only. They don't verify the actual service-layer guards (`cancel_plan()`, `pause_plan()`, `resume_plan()`). The feature file scenario names ("cannot be cancelled", "cannot be paused") imply behavioral testing, but the assertions only test the model property. - **Impact:** If a service method's guard condition diverges from `is_terminal` in the future, these tests would pass erroneously. - **Note:** For a model-level validation test file this is acceptable, but consider either renaming the scenarios to reflect they test the `is_terminal` property, or adding service-level integration tests in a separate file. --- ### LOW — Test Coverage Gap **T3. Missing Behave deserialization scenario for EXECUTE/CONSTRAINED coercion** - **Problem:** The Robot helper covers `EXECUTE/CONSTRAINED -> EXECUTE/ERRORED` (branch 2b in `_deserialization_coercion_all_branches`), but there is no matching Behave scenario. This creates asymmetric coverage between the two test layers. - **Suggested fix:** Add a fourth Behave deserialization scenario: ```gherkin Scenario: Legacy EXECUTE/CONSTRAINED is coerced to EXECUTE/ERRORED on deserialization When I plmv deserialize a plan with phase "execute" and state "constrained" Then the plmv deserialized plan should have phase EXECUTE and state ERRORED ``` --- ### LOW — Code Robustness **C1. `step_update_plan_phase_state` state-first pattern is fragile for phase-restricted target states** - **File:** `features/steps/plan_persistence_steps.py:272-273` - **Problem:** The state-first assignment (`processing_state = X; phase = Y`) can fail if the target state is phase-restricted (APPLIED, CONSTRAINED, COMPLETE) and the *current* phase doesn't allow it. For example, transitioning from `EXECUTE/PROCESSING` to `APPLY/APPLIED` in one step would create the invalid intermediate `EXECUTE/APPLIED`. - **Current impact:** All existing persistence test scenarios are safe because restricted states are only set when already in the correct phase. This is a latent fragility, not an active bug. - **Suggested fix (defensive):** Use a 3-step safe pattern: ```python plan.processing_state = ProcessingState.QUEUED # universally safe intermediate plan.phase = PlanPhase(phase) plan.processing_state = ProcessingState(state) ``` --- ### LOW — Documentation **C2. Coercion semantic choice lacks justification comment** - **File:** `src/cleveragents/infrastructure/database/models.py:770-782` - **Problem:** Coercing `APPLIED` (success semantics) to `ERRORED` (failure semantics) for legacy rows outside APPLY phase is a lossy transformation. The code comment explains *what* happens but not *why* ERRORED was chosen over alternatives like QUEUED (neutral reset). A brief rationale would help future maintainers. - **Suggested addition to comment:** > *"ERRORED is chosen over QUEUED because these legacy rows represent a state that should not auto-progress; ERRORED is terminal and safe, preventing unintended lifecycle advancement."* --- ## Areas Reviewed with No Issues Found | Category | Notes | |---|---| | **Security** | No injection vectors, no PII in logs, no unsafe deserialization | | **Performance** | Validator overhead is O(1) tuple-membership checks; negligible | | **Spec alignment** | All three acceptance criteria from #918 met; phase/state tables match spec lines 18250-18269 | | **Validator ordering** | `validate_phase_state_consistency` (ACTION/None→QUEUED) runs before `validate_phase_state_constraints`; correct | | **Service-layer state assignments** | All 14 `processing_state` assignments in `plan_lifecycle_service.py` verified safe (phase-guarded or universally valid states) | | **`can_revert_to` / `revert_plan`** | Correctly allows ERRORED and CONSTRAINED (only blocks APPLIED/CANCELLED); unaffected by `is_terminal` change | | **`to_domain()` coercion completeness** | All invalid phase/state combinations are handled by the three coercion branches | | **Existing test updates** | All 7 pre-existing test file changes correctly adapt to the new validator | --- *Review performed using 3 global review cycles across all categories (bugs, security, performance, test coverage, test flaws, code quality). Cycles 2 and 3 found no new issues, confirming convergence.*
CoreRasurae merged commit 9e316b1a3e into master 2026-03-23 23:56:34 +00:00
CoreRasurae deleted branch fix/plan-lifecycle-model-validation 2026-03-23 23:56:34 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
brent.edwards
freemo
CoreRasurae
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
 controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
 overdue
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
Priority
Medium
State
In Review
Type
Task
Milestone
No items
No Milestone v3.4.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: cleveragents/cleveragents-core#1077
Delete Branch "fix/plan-lifecycle-model-validation"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?