From 9e316b1a3e3e4ae99380e29fd22014f9130cb0f6 Mon Sep 17 00:00:00 2001 From: Luis Mendes Date: Thu, 19 Mar 2026 20:46:18 +0000 Subject: [PATCH] fix(domain): align plan lifecycle model validation with specification Aligned the plan lifecycle model with the specification: 1. ERRORED is now treated as terminal in is_terminal property, matching the spec table where errored is marked "Terminal? Yes" for all processing phases. 2. Added per-phase state validation via model_validator: APPLIED and CONSTRAINED are only valid in APPLY phase; COMPLETE is only valid in STRATEGIZE or EXECUTE phases. Invalid combinations now raise ValueError at construction time. 3. Updated ProcessingState.COMPLETE docstring to clarify phase-level terminality semantics. 4. Fixed assignment ordering in execute_plan() to set processing_state before phase, consistent with the state-first pattern used in apply_plan() and _perform_reversion(). 5. Added defensive coercion in LifecyclePlanModel.to_domain() to handle legacy DB rows with invalid phase/state combinations (e.g. APPLY/COMPLETE -> APPLY/APPLIED) with warning-level logging for observability. 6. Updated module docstrings: ERRORED description now reflects terminal semantics, terminal outcomes location clarified for all phases, can_revert_to docstring notes ERRORED/CONSTRAINED are terminal but revertable, is_terminal docstring explains the distinction between terminal and permanently irrecoverable and documents why COMPLETE is not plan-terminal despite the spec marking it "Terminal? Yes" (phase-level vs plan-level). 7. Updated PlanResumeService.validate_eligibility() docstring to reflect that ERRORED is now terminal but still eligible for resume. 8. Added CHANGELOG entry. ISSUES CLOSED: #918 --- CHANGELOG.md | 10 + features/edge_case_plan_scenarios.feature | 2 +- .../plan_lifecycle_model_validation.feature | 190 ++++++++++++ ...atabase_models_lifecycle_coverage_steps.py | 2 +- features/steps/m1_sourcecode_smoke_steps.py | 2 +- features/steps/plan_cli_legacy_r2_steps.py | 2 +- features/steps/plan_lifecycle_cli_steps.py | 4 +- .../plan_lifecycle_model_validation_steps.py | 253 ++++++++++++++++ features/steps/plan_persistence_steps.py | 9 +- robot/helper_m6_e2e_verification.py | 6 +- robot/helper_phase_reversion.py | 5 +- .../helper_plan_lifecycle_model_validation.py | 280 ++++++++++++++++++ robot/plan_lifecycle_model_validation.robot | 74 +++++ .../services/plan_lifecycle_service.py | 18 +- .../services/plan_resume_service.py | 7 +- src/cleveragents/domain/models/core/plan.py | 94 +++++- .../infrastructure/database/models.py | 42 +++ 17 files changed, 970 insertions(+), 30 deletions(-) create mode 100644 features/plan_lifecycle_model_validation.feature create mode 100644 features/steps/plan_lifecycle_model_validation_steps.py create mode 100644 robot/helper_plan_lifecycle_model_validation.py create mode 100644 robot/plan_lifecycle_model_validation.robot diff --git a/CHANGELOG.md b/CHANGELOG.md index 94b408313..e6ff542a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ ## Unreleased +- Aligned plan lifecycle model with specification: ERRORED is now + terminal in `is_terminal`, per-phase state validation enforces + APPLIED/CONSTRAINED to APPLY-only and COMPLETE to + STRATEGIZE/EXECUTE-only via model validator, COMPLETE docstring + clarified as phase-level terminal. Added defensive coercion in + database deserialization for legacy invalid phase/state + combinations with warning-level logging. Fixed assignment ordering + in `execute_plan()` for consistency with phase-state validator. + Updated `PlanResumeService` docstring to reflect ERRORED + terminality. (#918) - Fixed `shell=True` subprocess usage in `cli_coverage_steps.py` by replacing with `shlex.split()` and `shell=False` for defense-in-depth command injection prevention, consistent with the existing pattern in diff --git a/features/edge_case_plan_scenarios.feature b/features/edge_case_plan_scenarios.feature index 99d8445e8..b55557d59 100644 --- a/features/edge_case_plan_scenarios.feature +++ b/features/edge_case_plan_scenarios.feature @@ -229,7 +229,7 @@ Feature: Plan edge case scenarios When apply fails with error "Disk full during write" Then the lifecycle plan processing state should be "errored" And the lifecycle plan error message should be "Disk full during write" - And the plan should not be terminal + And the plan should be terminal Scenario: Cancel plan clears processing but does not revert to previous phase Given I have a plan lifecycle service diff --git a/features/plan_lifecycle_model_validation.feature b/features/plan_lifecycle_model_validation.feature new file mode 100644 index 000000000..0da676550 --- /dev/null +++ b/features/plan_lifecycle_model_validation.feature @@ -0,0 +1,190 @@ +Feature: Plan lifecycle model validation + Verify that the plan lifecycle model enforces the specification + constraints: ERRORED terminality, per-phase state validation, + and rejection of invalid phase/state combinations. + + # --------------------------------------------------------------- + # ERRORED is terminal (spec alignment) + # --------------------------------------------------------------- + + Scenario: ERRORED state is terminal + Given a plmv plan in STRATEGIZE phase with ERRORED state + Then the plmv plan should be terminal + + # --------------------------------------------------------------- + # APPLIED only valid in APPLY phase + # --------------------------------------------------------------- + + Scenario: APPLIED only valid in APPLY phase + When I try to create a plmv plan with phase STRATEGIZE and state APPLIED + Then a plmv validation error should be raised containing "APPLIED is only valid in the APPLY phase" + + # --------------------------------------------------------------- + # CONSTRAINED only valid in APPLY phase + # --------------------------------------------------------------- + + Scenario: CONSTRAINED only valid in APPLY phase + When I try to create a plmv plan with phase EXECUTE and state CONSTRAINED + Then a plmv validation error should be raised containing "CONSTRAINED is only valid in the APPLY phase" + + # --------------------------------------------------------------- + # COMPLETE only valid in STRATEGIZE or EXECUTE phases + # --------------------------------------------------------------- + + Scenario: COMPLETE only valid in STRATEGIZE or EXECUTE phase + When I try to create a plmv plan with phase APPLY and state COMPLETE + Then a plmv validation error should be raised containing "COMPLETE is only valid in STRATEGIZE or EXECUTE" + + # --------------------------------------------------------------- + # QUEUED valid in any phase + # --------------------------------------------------------------- + + Scenario: QUEUED valid in any phase + Given 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 + + # --------------------------------------------------------------- + # Additional coverage: valid combos + # --------------------------------------------------------------- + + Scenario: APPLIED is valid in APPLY phase + Given a plmv plan in APPLY phase with APPLIED state + Then the plmv plan should be terminal + + Scenario: CONSTRAINED is valid in APPLY phase + Given a plmv plan in APPLY phase with CONSTRAINED state + Then the plmv plan should be terminal + + Scenario: COMPLETE is valid in STRATEGIZE phase + Given a plmv plan in STRATEGIZE phase with COMPLETE state + Then the plmv plan should not be terminal + + Scenario: COMPLETE is valid in EXECUTE phase + Given a plmv plan in EXECUTE phase with COMPLETE state + Then the plmv plan should not be terminal + + Scenario: ERRORED in APPLY phase is also terminal + Given a plmv plan in APPLY phase with ERRORED state + Then the plmv plan should be terminal + + Scenario: CANCELLED in EXECUTE phase is terminal + Given a plmv plan in EXECUTE phase with CANCELLED state + Then the plmv plan should be terminal + + # --------------------------------------------------------------- + # Invalid combos for ACTION phase + # --------------------------------------------------------------- + + Scenario: COMPLETE is not valid in ACTION phase + When I try to create a plmv plan with phase ACTION and state COMPLETE + Then a plmv validation error should be raised containing "COMPLETE is only valid in STRATEGIZE or EXECUTE" + + Scenario: APPLIED is not valid in EXECUTE phase + When I try to create a plmv plan with phase EXECUTE and state APPLIED + Then a plmv validation error should be raised containing "APPLIED is only valid in the APPLY phase" + + Scenario: APPLIED is not valid in ACTION phase + When I try to create a plmv plan with phase ACTION and state APPLIED + Then a plmv validation error should be raised containing "APPLIED is only valid in the APPLY phase" + + Scenario: CONSTRAINED is not valid in ACTION phase + When I try to create a plmv plan with phase ACTION and state CONSTRAINED + Then a plmv validation error should be raised containing "CONSTRAINED is only valid in the APPLY phase" + + Scenario: CONSTRAINED is not valid in STRATEGIZE phase + When I try to create a plmv plan with phase STRATEGIZE and state CONSTRAINED + Then a plmv validation error should be raised containing "CONSTRAINED is only valid in the APPLY phase" + + # --------------------------------------------------------------- + # PROCESSING valid in any phase (T1) + # --------------------------------------------------------------- + + Scenario: PROCESSING valid in any phase + Given a plmv plan in ACTION phase with PROCESSING state + And a plmv plan in STRATEGIZE phase with PROCESSING state + And a plmv plan in EXECUTE phase with PROCESSING state + And a plmv plan in APPLY phase with PROCESSING state + Then all plmv plans should be valid + + # --------------------------------------------------------------- + # ERRORED valid in all phases including ACTION (T2) + # --------------------------------------------------------------- + + Scenario: ERRORED in ACTION phase is terminal + Given a plmv plan in ACTION phase with ERRORED state + Then the plmv plan should be terminal + + Scenario: ERRORED in EXECUTE phase is terminal + Given a plmv plan in EXECUTE phase with ERRORED state + Then the plmv plan should be terminal + + # --------------------------------------------------------------- + # CANCELLED valid in all phases (additional) + # --------------------------------------------------------------- + + Scenario: CANCELLED in ACTION phase is terminal + Given a plmv plan in ACTION phase with CANCELLED state + Then the plmv plan should be terminal + + Scenario: CANCELLED in STRATEGIZE phase is terminal + Given a plmv plan in STRATEGIZE phase with CANCELLED state + Then the plmv plan should be terminal + + Scenario: CANCELLED in APPLY phase is terminal + Given a plmv plan in APPLY phase with CANCELLED state + Then the plmv plan should be terminal + + # --------------------------------------------------------------- + # Assignment-order regression: state-before-phase (T6) + # --------------------------------------------------------------- + + Scenario: Phase transition with state-first assignment does not raise + Given a plmv plan in STRATEGIZE phase with COMPLETE state + When I plmv transition the plan to EXECUTE phase with QUEUED state using state-first order + Then the plmv plan should have phase EXECUTE and state QUEUED + + # --------------------------------------------------------------- + # Assignment-order negative: phase-first with invalid intermediate + # --------------------------------------------------------------- + + Scenario: Phase-first assignment with invalid intermediate state raises + Given a plmv plan in STRATEGIZE phase with COMPLETE state + When I plmv transition the plan to APPLY phase with APPLIED state using phase-first order + Then a plmv validation error should be raised containing "COMPLETE is only valid in STRATEGIZE or EXECUTE" + + # --------------------------------------------------------------- + # ERRORED terminality blocks cancel, pause, resume (lifecycle) + # --------------------------------------------------------------- + + Scenario: ERRORED plan cannot be cancelled because it is terminal + Given a plmv plan in EXECUTE phase with ERRORED state + Then the plmv plan should be terminal + And the plmv plan should not be cancellable + + Scenario: ERRORED plan cannot be paused because it is terminal + Given a plmv plan in STRATEGIZE phase with ERRORED state + Then the plmv plan should be terminal + And the plmv plan should not be pausable + + Scenario: ERRORED plan cannot be resumed via lifecycle because it is terminal + Given a plmv plan in EXECUTE phase with ERRORED state + Then the plmv plan should be terminal + And the plmv plan should not be resumable via lifecycle + + # --------------------------------------------------------------- + # Deserialization coercion for legacy DB rows + # --------------------------------------------------------------- + + Scenario: Legacy APPLY/COMPLETE is coerced to APPLY/APPLIED on deserialization + When I plmv deserialize a plan with phase "apply" and state "complete" + Then the plmv deserialized plan should have phase APPLY and state APPLIED + + Scenario: Legacy STRATEGIZE/APPLIED is coerced to STRATEGIZE/ERRORED on deserialization + When I plmv deserialize a plan with phase "strategize" and state "applied" + Then the plmv deserialized plan should have phase STRATEGIZE and state ERRORED + + Scenario: Legacy ACTION/COMPLETE is coerced to ACTION/QUEUED on deserialization + When I plmv deserialize a plan with phase "action" and state "complete" + Then the plmv deserialized plan should have phase ACTION and state QUEUED diff --git a/features/steps/database_models_lifecycle_coverage_steps.py b/features/steps/database_models_lifecycle_coverage_steps.py index 9da2b3110..4f63d811c 100644 --- a/features/steps/database_models_lifecycle_coverage_steps.py +++ b/features/steps/database_models_lifecycle_coverage_steps.py @@ -713,7 +713,7 @@ def step_create_plan_model_all_timestamps(context): context.plan_model = _make_plan_model( plan_id=VALID_ULID_3, phase="apply", - state="complete", + state="applied", strategize_started_at="2025-06-01T14:00:00", strategize_completed_at="2025-06-01T14:30:00", execute_started_at="2025-06-01T15:00:00", diff --git a/features/steps/m1_sourcecode_smoke_steps.py b/features/steps/m1_sourcecode_smoke_steps.py index 0e5383486..86840f5ed 100644 --- a/features/steps/m1_sourcecode_smoke_steps.py +++ b/features/steps/m1_sourcecode_smoke_steps.py @@ -551,7 +551,7 @@ def step_m1_plan_in_apply(context: Context) -> None: """Set up a plan in apply phase.""" plan = _make_m1_plan( phase=PlanPhase.APPLY, - state=ProcessingState.COMPLETE, + state=ProcessingState.QUEUED, ) context.mock_service.list_plans.return_value = [plan] context.mock_service.get_plan.return_value = plan diff --git a/features/steps/plan_cli_legacy_r2_steps.py b/features/steps/plan_cli_legacy_r2_steps.py index f7aeddd57..8e9750029 100644 --- a/features/steps/plan_cli_legacy_r2_steps.py +++ b/features/steps/plan_cli_legacy_r2_steps.py @@ -377,7 +377,7 @@ def step_list_returns_none(context: Any) -> None: def step_resolve_no_active(context: Any) -> None: mock_svc = MagicMock() # All plans are terminal - p = _make_plan(processing_state=ProcessingState.APPLIED) + p = _make_plan(phase=PlanPhase.APPLY, processing_state=ProcessingState.APPLIED) mock_svc.list_plans.return_value = [p] with patch( "cleveragents.cli.commands.plan._get_lifecycle_service", diff --git a/features/steps/plan_lifecycle_cli_steps.py b/features/steps/plan_lifecycle_cli_steps.py index faea92931..c319a5e50 100644 --- a/features/steps/plan_lifecycle_cli_steps.py +++ b/features/steps/plan_lifecycle_cli_steps.py @@ -337,7 +337,9 @@ def step_plan_status_without_plan_id(context, count: int) -> None: plan_id=_ULIDS[5 + idx], name=f"local/status-plan-{idx}", phase=PlanPhase.EXECUTE if idx % 2 == 0 else PlanPhase.APPLY, - processing_state=ProcessingState.COMPLETE, + processing_state=( + ProcessingState.COMPLETE if idx % 2 == 0 else ProcessingState.QUEUED + ), ) for idx in range(count) ] diff --git a/features/steps/plan_lifecycle_model_validation_steps.py b/features/steps/plan_lifecycle_model_validation_steps.py new file mode 100644 index 000000000..9ea6daa8e --- /dev/null +++ b/features/steps/plan_lifecycle_model_validation_steps.py @@ -0,0 +1,253 @@ +"""Step definitions for plan lifecycle model validation tests. + +Covers ERRORED terminality, per-phase state constraints, rejection +of invalid phase/state combinations, deserialization coercion for +legacy DB rows, and terminal-state guard behaviour per the +specification. +""" + +from __future__ import annotations + +from typing import Any + +from behave import given, then, when +from behave.runner import Context +from pydantic import ValidationError + +from cleveragents.domain.models.core.plan import ( + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + ProcessingState, +) + +_ULID = "01ARZ3NDEKTSV4RRFFQ69G5FAV" + + +def _make_plan( + phase: PlanPhase, + processing_state: ProcessingState, +) -> Plan: + """Build a Plan with the given phase and processing state.""" + return Plan( + identity=PlanIdentity(plan_id=_ULID), + namespaced_name=NamespacedName( + server=None, namespace="local", name="plmv-test" + ), + description="Plan lifecycle model validation test", + action_name="local/plmv-action", + phase=phase, + processing_state=processing_state, + ) + + +_PHASE_MAP: dict[str, PlanPhase] = { + "ACTION": PlanPhase.ACTION, + "STRATEGIZE": PlanPhase.STRATEGIZE, + "EXECUTE": PlanPhase.EXECUTE, + "APPLY": PlanPhase.APPLY, +} + +_STATE_MAP: dict[str, ProcessingState] = { + "QUEUED": ProcessingState.QUEUED, + "PROCESSING": ProcessingState.PROCESSING, + "ERRORED": ProcessingState.ERRORED, + "COMPLETE": ProcessingState.COMPLETE, + "CANCELLED": ProcessingState.CANCELLED, + "APPLIED": ProcessingState.APPLIED, + "CONSTRAINED": ProcessingState.CONSTRAINED, +} + + +# ---- Given steps ---- + + +@given("a plmv plan in {phase} phase with {state} state") +def step_create_plmv_plan(context: Context, phase: str, state: str) -> None: + """Create a plan with the given phase and processing state.""" + plan = _make_plan(_PHASE_MAP[phase], _STATE_MAP[state]) + if not hasattr(context, "plmv_plans"): + context.plmv_plans = [] + context.plmv_plans.append(plan) + context.plmv_plan = plan + + +# ---- When steps ---- + + +@when("I try to create a plmv plan with phase {phase} and state {state}") +def step_try_create_invalid_plmv_plan(context: Context, phase: str, state: str) -> None: + """Attempt to create a plan that should fail validation.""" + context.plmv_error = None + try: + _make_plan(_PHASE_MAP[phase], _STATE_MAP[state]) + except (ValidationError, ValueError) as exc: + context.plmv_error = exc + + +# ---- Then steps ---- + + +@then("the plmv plan should be terminal") +def step_plmv_plan_terminal(context: Context) -> None: + """Assert the plan is in a terminal state.""" + assert context.plmv_plan.is_terminal, ( + f"Expected plan to be terminal, but is_terminal=False " + f"(phase={context.plmv_plan.phase.value}, " + f"state={context.plmv_plan.processing_state.value})" + ) + + +@then("the plmv plan should not be terminal") +def step_plmv_plan_not_terminal(context: Context) -> None: + """Assert the plan is NOT in a terminal state.""" + assert not context.plmv_plan.is_terminal, ( + f"Expected plan to NOT be terminal, but is_terminal=True " + f"(phase={context.plmv_plan.phase.value}, " + f"state={context.plmv_plan.processing_state.value})" + ) + + +@then('a plmv validation error should be raised containing "{fragment}"') +def step_plmv_validation_error(context: Context, fragment: str) -> None: + """Assert that a validation error was raised with the expected message.""" + assert context.plmv_error is not None, ( + "Expected a validation error, but none was raised" + ) + msg = str(context.plmv_error) + assert fragment in msg, f"Expected error to contain {fragment!r}, got: {msg}" + + +@then("all plmv plans should be valid") +def step_all_plmv_plans_valid(context: Context) -> None: + """Assert that all previously created plans are valid.""" + assert hasattr(context, "plmv_plans"), "No plmv_plans found on context" + assert len(context.plmv_plans) > 0, "plmv_plans list is empty" + for plan in context.plmv_plans: + assert plan is not None, "Plan should not be None" + + +# ---- Assignment-order regression steps (T6) ---- + + +@when( + "I plmv transition the plan to {phase} phase with {state} state" + " using state-first order" +) +def step_plmv_transition_state_first(context: Context, phase: str, state: str) -> None: + """Mutate an existing plan's phase and state using the safe + state-first ordering. Verifies that ``validate_assignment=True`` + does not trigger the phase-state validator on the intermediate + state. + """ + plan = context.plmv_plan + plan.processing_state = _STATE_MAP[state] + plan.phase = _PHASE_MAP[phase] + context.plmv_plan = plan + + +@then("the plmv plan should have phase {phase} and state {state}") +def step_plmv_plan_phase_state(context: Context, phase: str, state: str) -> None: + """Assert the plan's current phase and processing state.""" + plan = context.plmv_plan + assert plan.phase == _PHASE_MAP[phase], ( + f"Expected phase {phase}, got {plan.phase.value}" + ) + assert plan.processing_state == _STATE_MAP[state], ( + f"Expected state {state}, got {plan.processing_state.value}" + ) + + +# ---- Phase-first assignment negative test ---- + + +@when( + "I plmv transition the plan to {phase} phase with {state} state" + " using phase-first order" +) +def step_plmv_transition_phase_first(context: Context, phase: str, state: str) -> None: + """Attempt phase-first mutation which should fail the validator + when the intermediate phase/state combination is invalid. + """ + context.plmv_error = None + try: + plan = context.plmv_plan + plan.phase = _PHASE_MAP[phase] + plan.processing_state = _STATE_MAP[state] + context.plmv_plan = plan + except (ValidationError, ValueError) as exc: + context.plmv_error = exc + + +# ---- Terminal-state guard steps ---- + + +@then("the plmv plan should not be cancellable") +def step_plmv_not_cancellable(context: Context) -> None: + """Assert the plan is terminal and therefore not cancellable.""" + assert context.plmv_plan.is_terminal, ( + "Expected plan to be terminal (not cancellable)" + ) + + +@then("the plmv plan should not be pausable") +def step_plmv_not_pausable(context: Context) -> None: + """Assert the plan is terminal and therefore not pausable.""" + assert context.plmv_plan.is_terminal, "Expected plan to be terminal (not pausable)" + + +@then("the plmv plan should not be resumable via lifecycle") +def step_plmv_not_resumable_lifecycle(context: Context) -> None: + """Assert the plan is terminal and therefore not resumable via + ``PlanLifecycleService.resume_plan()``. + """ + assert context.plmv_plan.is_terminal, ( + "Expected plan to be terminal (not resumable via lifecycle)" + ) + + +# ---- Deserialization coercion steps ---- + + +@when('I plmv deserialize a plan with phase "{phase}" and state "{state}"') +def step_plmv_deserialize(context: Context, phase: str, state: str) -> None: + """Build a ``LifecyclePlanModel`` ORM row with the given phase/state + and call ``to_domain()`` to exercise the defensive coercion logic. + """ + from cleveragents.infrastructure.database.models import ( + LifecyclePlanModel, + ) + + model = LifecyclePlanModel() + model.plan_id = _ULID + model.namespaced_name = "local/plmv-deser" + model.description = "deserialization coercion test" + model.action_name = "local/plmv-action" + model.phase = phase + model.processing_state = state + model.attempt = 1 + model.created_at = "2025-01-01T00:00:00" + model.updated_at = "2025-01-01T00:00:00" + model.reusable = True + model.read_only = False + model.sandbox_refs_json = "[]" + model.tags_json = "[]" + model.reversion_count = 0 + model.last_completed_step = -1 + + context.plmv_deserialized_plan = model.to_domain() + + +@then("the plmv deserialized plan should have phase {phase} and state {state}") +def step_plmv_deserialized_phase_state( + context: Context, phase: str, state: str +) -> None: + """Assert the deserialized plan's phase and state after coercion.""" + plan: Any = context.plmv_deserialized_plan + assert plan.phase == _PHASE_MAP[phase], ( + f"Expected phase {phase}, got {plan.phase.value}" + ) + assert plan.processing_state == _STATE_MAP[state], ( + f"Expected state {state}, got {plan.processing_state.value}" + ) diff --git a/features/steps/plan_persistence_steps.py b/features/steps/plan_persistence_steps.py index ab0aaaa51..7ea5cb1d8 100644 --- a/features/steps/plan_persistence_steps.py +++ b/features/steps/plan_persistence_steps.py @@ -266,11 +266,16 @@ def step_persisted_plan_phase_state(context: Context, phase: str, state: str) -> @when('I update the plan phase to "{phase}" with state "{state}"') def step_update_plan_phase_state(context: Context, phase: str, state: str) -> None: - """Update the plan's phase and processing state.""" + """Update the plan's phase and processing state. + + Sets processing_state before phase so that the per-phase state + validator sees a universally valid state (e.g. QUEUED) when the + phase assignment triggers re-validation. + """ plan = context._pp_plan_repo.get(context._pp_current_plan_id) assert plan is not None - plan.phase = PlanPhase(phase) plan.processing_state = ProcessingState(state) + plan.phase = PlanPhase(phase) plan.timestamps.updated_at = datetime.now(tz=UTC) context._pp_plan_repo.update(plan) context._pp_session.commit() diff --git a/robot/helper_m6_e2e_verification.py b/robot/helper_m6_e2e_verification.py index ca437d656..94d307c37 100644 --- a/robot/helper_m6_e2e_verification.py +++ b/robot/helper_m6_e2e_verification.py @@ -517,13 +517,13 @@ def porting_task_autonomous() -> None: Simulates the full lifecycle: ACTION → STRATEGIZE → EXECUTE → APPLY with subplan decomposition and completion. """ - # Phase 1: ACTION → STRATEGIZE + # Phase 1: ACTION → STRATEGIZE (ACTION can always transition) plan_action = _make_plan( phase=PlanPhase.ACTION, - processing_state=ProcessingState.COMPLETE, + processing_state=ProcessingState.QUEUED, ) if not plan_action.can_transition_to_next_phase: - _fail("ACTION/COMPLETE should allow transition") + _fail("ACTION/QUEUED should allow transition") # Phase 2: STRATEGIZE with subplan config plan_strat = _make_plan( diff --git a/robot/helper_phase_reversion.py b/robot/helper_phase_reversion.py index f95ac2afa..6f27ac0fb 100644 --- a/robot/helper_phase_reversion.py +++ b/robot/helper_phase_reversion.py @@ -253,9 +253,10 @@ def test_can_revert_to() -> None: plan.processing_state = ProcessingState.APPLIED assert not plan.can_revert_to(PlanPhase.STRATEGIZE) - # Max reversions reached blocks reversion - plan.phase = PlanPhase.EXECUTE + # Max reversions reached blocks reversion — set processing_state + # first so the phase-state validator sees QUEUED (valid in any phase). plan.processing_state = ProcessingState.QUEUED + plan.phase = PlanPhase.EXECUTE plan.reversion_count = 3 assert not plan.can_revert_to(PlanPhase.STRATEGIZE) diff --git a/robot/helper_plan_lifecycle_model_validation.py b/robot/helper_plan_lifecycle_model_validation.py new file mode 100644 index 000000000..4c901fdab --- /dev/null +++ b/robot/helper_plan_lifecycle_model_validation.py @@ -0,0 +1,280 @@ +"""Robot Framework helper for plan lifecycle model validation tests. + +Each function is invoked by the corresponding Robot test case via +``Run Process python ``. +""" + +from __future__ import annotations + +import sys + +from pydantic import ValidationError + +from cleveragents.domain.models.core.plan import ( + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + ProcessingState, +) + +_ULID = "01ARZ3NDEKTSV4RRFFQ69G5FAV" + + +def _make_plan( + phase: PlanPhase, + processing_state: ProcessingState, +) -> Plan: + """Build a Plan with the given phase and processing state.""" + return Plan( + identity=PlanIdentity(plan_id=_ULID), + namespaced_name=NamespacedName( + server=None, namespace="local", name="plmv-robot" + ), + description="Plan lifecycle model validation robot test", + action_name="local/plmv-robot-action", + phase=phase, + processing_state=processing_state, + ) + + +def _errored_is_terminal() -> None: + """Verify ERRORED state is terminal in all applicable phases.""" + for phase in ( + PlanPhase.ACTION, + PlanPhase.STRATEGIZE, + PlanPhase.EXECUTE, + PlanPhase.APPLY, + ): + plan = _make_plan(phase, ProcessingState.ERRORED) + assert plan.is_terminal, f"ERRORED should be terminal in {phase.value} phase" + assert plan.is_errored, ( + f"ERRORED plan should report is_errored in {phase.value} phase" + ) + print("errored-is-terminal-ok") + + +def _processing_any_phase() -> None: + """Verify PROCESSING is accepted in all phases.""" + for phase in PlanPhase: + plan = _make_plan(phase, ProcessingState.PROCESSING) + assert plan.processing_state == ProcessingState.PROCESSING, ( + f"PROCESSING not set in {phase.value}" + ) + print("processing-any-phase-ok") + + +def _applied_only_apply() -> None: + """Verify APPLIED is rejected outside APPLY phase.""" + # Valid in APPLY + plan = _make_plan(PlanPhase.APPLY, ProcessingState.APPLIED) + assert plan.is_terminal + + # Invalid in STRATEGIZE, EXECUTE, and ACTION + for phase in (PlanPhase.STRATEGIZE, PlanPhase.EXECUTE, PlanPhase.ACTION): + try: + _make_plan(phase, ProcessingState.APPLIED) + raise AssertionError( + f"Expected ValidationError for APPLIED in {phase.value}" + ) + except ValidationError as exc: + assert "APPLIED is only valid in the APPLY phase" in str(exc) + + print("applied-only-apply-ok") + + +def _constrained_only_apply() -> None: + """Verify CONSTRAINED is rejected outside APPLY phase.""" + # Valid in APPLY + plan = _make_plan(PlanPhase.APPLY, ProcessingState.CONSTRAINED) + assert plan.is_terminal + + # Invalid in STRATEGIZE, EXECUTE, and ACTION + for phase in (PlanPhase.STRATEGIZE, PlanPhase.EXECUTE, PlanPhase.ACTION): + try: + _make_plan(phase, ProcessingState.CONSTRAINED) + raise AssertionError( + f"Expected ValidationError for CONSTRAINED in {phase.value}" + ) + except ValidationError as exc: + assert "CONSTRAINED is only valid in the APPLY phase" in str(exc) + + print("constrained-only-apply-ok") + + +def _complete_strategize_execute() -> None: + """Verify COMPLETE is only valid in STRATEGIZE and EXECUTE phases.""" + # Valid + for phase in (PlanPhase.STRATEGIZE, PlanPhase.EXECUTE): + plan = _make_plan(phase, ProcessingState.COMPLETE) + assert not plan.is_terminal, f"COMPLETE should not be terminal in {phase.value}" + + # Invalid in APPLY + try: + _make_plan(PlanPhase.APPLY, ProcessingState.COMPLETE) + raise AssertionError("Expected ValidationError for COMPLETE in APPLY") + except ValidationError as exc: + assert "COMPLETE is only valid in STRATEGIZE or EXECUTE" in str(exc) + + # Invalid in ACTION + try: + _make_plan(PlanPhase.ACTION, ProcessingState.COMPLETE) + raise AssertionError("Expected ValidationError for COMPLETE in ACTION") + except ValidationError as exc: + assert "COMPLETE is only valid in STRATEGIZE or EXECUTE" in str(exc) + + print("complete-strategize-execute-ok") + + +def _queued_any_phase() -> None: + """Verify QUEUED is accepted in all phases.""" + for phase in PlanPhase: + plan = _make_plan(phase, ProcessingState.QUEUED) + assert plan.processing_state == ProcessingState.QUEUED, ( + f"QUEUED not set in {phase.value}" + ) + print("queued-any-phase-ok") + + +def _assignment_order_safety() -> None: + """Verify that state-first assignment order avoids validator errors. + + With ``validate_assignment=True`` on the Plan model, each field + set triggers the phase-state validator. Setting ``processing_state`` + (to a universally valid value like QUEUED) before ``phase`` avoids + transient invalid intermediate states. + """ + plan = _make_plan(PlanPhase.STRATEGIZE, ProcessingState.COMPLETE) + # State-first transition: COMPLETE → QUEUED then STRATEGIZE → EXECUTE + plan.processing_state = ProcessingState.QUEUED + plan.phase = PlanPhase.EXECUTE + assert plan.phase == PlanPhase.EXECUTE + assert plan.processing_state == ProcessingState.QUEUED + print("assignment-order-safety-ok") + + +def _deserialization_coercion() -> None: + """Verify that to_domain() coerces legacy invalid phase/state combos. + + The defensive coercion in LifecyclePlanModel.to_domain() should + map APPLY/COMPLETE → APPLY/APPLIED without crashing. + """ + from cleveragents.infrastructure.database.models import ( + LifecyclePlanModel, + ) + + model = LifecyclePlanModel() + model.plan_id = _ULID + model.namespaced_name = "local/plmv-deser" + model.description = "deserialization coercion test" + model.action_name = "local/plmv-action" + model.phase = "apply" + model.processing_state = "complete" # legacy invalid combo + model.attempt = 1 + model.created_at = "2025-01-01T00:00:00" + model.updated_at = "2025-01-01T00:00:00" + model.reusable = True + model.read_only = False + model.sandbox_refs_json = "[]" + model.tags_json = "[]" + model.reversion_count = 0 + model.last_completed_step = -1 + + plan = model.to_domain() + # Should have coerced COMPLETE → APPLIED in APPLY phase + assert plan.processing_state.value == "applied", ( + f"Expected 'applied', got '{plan.processing_state.value}'" + ) + assert plan.phase.value == "apply" + print("deserialization-coercion-ok") + + +def _deserialization_coercion_all_branches() -> None: + """Verify all three coercion branches in to_domain(). + + Branch 1: APPLY/COMPLETE → APPLY/APPLIED + Branch 2: non-APPLY/APPLIED → non-APPLY/ERRORED + Branch 3: ACTION/COMPLETE → ACTION/QUEUED + """ + from cleveragents.infrastructure.database.models import ( + LifecyclePlanModel, + ) + + def _make_legacy_model(phase: str, state: str, suffix: str) -> LifecyclePlanModel: + model = LifecyclePlanModel() + model.plan_id = _ULID + model.namespaced_name = f"local/plmv-deser-{suffix}" + model.description = "deserialization coercion all-branches test" + model.action_name = "local/plmv-action" + model.phase = phase + model.processing_state = state + model.attempt = 1 + model.created_at = "2025-01-01T00:00:00" + model.updated_at = "2025-01-01T00:00:00" + model.reusable = True + model.read_only = False + model.sandbox_refs_json = "[]" + model.tags_json = "[]" + model.reversion_count = 0 + model.last_completed_step = -1 + return model + + # Branch 1: APPLY/COMPLETE → APPLY/APPLIED + plan1 = _make_legacy_model("apply", "complete", "b1").to_domain() + assert plan1.processing_state.value == "applied", ( + f"Branch 1: expected 'applied', got '{plan1.processing_state.value}'" + ) + + # Branch 2: STRATEGIZE/APPLIED → STRATEGIZE/ERRORED + plan2 = _make_legacy_model("strategize", "applied", "b2").to_domain() + assert plan2.processing_state.value == "errored", ( + f"Branch 2: expected 'errored', got '{plan2.processing_state.value}'" + ) + + # Branch 2b: EXECUTE/CONSTRAINED → EXECUTE/ERRORED + plan2b = _make_legacy_model("execute", "constrained", "b2b").to_domain() + assert plan2b.processing_state.value == "errored", ( + f"Branch 2b: expected 'errored', got '{plan2b.processing_state.value}'" + ) + + # Branch 3: ACTION/COMPLETE → ACTION/QUEUED + plan3 = _make_legacy_model("action", "complete", "b3").to_domain() + assert plan3.processing_state.value == "queued", ( + f"Branch 3: expected 'queued', got '{plan3.processing_state.value}'" + ) + + print("deserialization-coercion-all-branches-ok") + + +_COMMANDS: dict[str, object] = { + "errored-is-terminal": _errored_is_terminal, + "processing-any-phase": _processing_any_phase, + "applied-only-apply": _applied_only_apply, + "constrained-only-apply": _constrained_only_apply, + "complete-strategize-execute": _complete_strategize_execute, + "queued-any-phase": _queued_any_phase, + "assignment-order-safety": _assignment_order_safety, + "deserialization-coercion": _deserialization_coercion, + "deserialization-coercion-all-branches": _deserialization_coercion_all_branches, +} + + +def main() -> None: + """Dispatch to the requested test function.""" + if len(sys.argv) < 2: + print(f"Usage: {sys.argv[0]} ", file=sys.stderr) + print(f"Commands: {', '.join(_COMMANDS)}", file=sys.stderr) + sys.exit(1) + + cmd = sys.argv[1] + fn = _COMMANDS.get(cmd) + if fn is None: + print(f"Unknown command: {cmd}", file=sys.stderr) + sys.exit(1) + + assert callable(fn) + fn() + + +if __name__ == "__main__": + main() diff --git a/robot/plan_lifecycle_model_validation.robot b/robot/plan_lifecycle_model_validation.robot new file mode 100644 index 000000000..eeee79f42 --- /dev/null +++ b/robot/plan_lifecycle_model_validation.robot @@ -0,0 +1,74 @@ +*** Settings *** +Documentation Integration tests for plan lifecycle model validation: +... ERRORED terminality, per-phase state constraints, and +... rejection of invalid phase/state combinations. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER_SCRIPT} robot/helper_plan_lifecycle_model_validation.py + +*** Test Cases *** +ERRORED State Is Terminal + [Documentation] Verify that a plan with ERRORED processing state is terminal + [Tags] lifecycle plan model terminal + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} errored-is-terminal cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} errored-is-terminal-ok + +APPLIED Only Valid In APPLY Phase + [Documentation] Verify APPLIED state is rejected outside the APPLY phase + [Tags] lifecycle plan model validation + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} applied-only-apply cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} applied-only-apply-ok + +CONSTRAINED Only Valid In APPLY Phase + [Documentation] Verify CONSTRAINED state is rejected outside the APPLY phase + [Tags] lifecycle plan model validation + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} constrained-only-apply cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} constrained-only-apply-ok + +COMPLETE Only Valid In STRATEGIZE Or EXECUTE + [Documentation] Verify COMPLETE state is rejected in APPLY and ACTION phases + [Tags] lifecycle plan model validation + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} complete-strategize-execute cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} complete-strategize-execute-ok + +QUEUED Valid In Any Phase + [Documentation] Verify QUEUED state is accepted in all phases + [Tags] lifecycle plan model validation + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} queued-any-phase cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} queued-any-phase-ok + +PROCESSING Valid In Any Phase + [Documentation] Verify PROCESSING state is accepted in all phases + [Tags] lifecycle plan model validation + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} processing-any-phase cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} processing-any-phase-ok + +Assignment Order Safety + [Documentation] Verify state-first assignment order avoids validator errors + [Tags] lifecycle plan model validation regression + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} assignment-order-safety cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} assignment-order-safety-ok + +Deserialization Coercion + [Documentation] Verify to_domain coerces legacy invalid phase/state combos + [Tags] lifecycle plan model validation deserialization + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} deserialization-coercion cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} deserialization-coercion-ok + +Deserialization Coercion All Branches + [Documentation] Verify all three coercion branches in to_domain + [Tags] lifecycle plan model validation deserialization + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} deserialization-coercion-all-branches cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} deserialization-coercion-all-branches-ok diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index 7e0097d00..159063e6b 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -1091,9 +1091,11 @@ class PlanLifecycleService: # Layer 4: Consult Error Pattern Database for preventive guidance self._consult_error_patterns(plan) - # Transition to Execute phase - plan.phase = PlanPhase.EXECUTE + # Transition to Execute phase — set processing_state first so that + # the phase-state validator sees QUEUED (valid in any phase) when + # the phase assignment triggers re-validation. plan.processing_state = ProcessingState.QUEUED + plan.phase = PlanPhase.EXECUTE plan.timestamps.updated_at = datetime.now() self._commit_plan(plan) @@ -1224,9 +1226,11 @@ class PlanLifecycleService: plan_id, plan.phase, plan.state or ProcessingState.QUEUED ) - # Transition to Apply phase - plan.phase = PlanPhase.APPLY + # Transition to Apply phase — set processing_state first so that + # the phase-state validator sees QUEUED (valid in any phase) when + # the phase assignment triggers re-validation. plan.processing_state = ProcessingState.QUEUED + plan.phase = PlanPhase.APPLY plan.timestamps.apply_started_at = datetime.now() plan.timestamps.updated_at = datetime.now() @@ -1834,9 +1838,11 @@ class PlanLifecycleService: } plan.decisions.append(decision) - # Update plan state - plan.phase = to_phase + # Update plan state — set processing_state first so that the + # phase-state validator sees QUEUED (valid in any phase) when the + # phase assignment triggers re-validation. plan.processing_state = ProcessingState.QUEUED + plan.phase = to_phase plan.reversion_count += 1 plan.error_message = None plan.timestamps.updated_at = datetime.now() diff --git a/src/cleveragents/application/services/plan_resume_service.py b/src/cleveragents/application/services/plan_resume_service.py index 39498f4e7..27a80bbac 100644 --- a/src/cleveragents/application/services/plan_resume_service.py +++ b/src/cleveragents/application/services/plan_resume_service.py @@ -104,8 +104,11 @@ class PlanResumeService: def validate_eligibility(self, plan_id: str) -> ResumeEligibility: """Validate whether a plan can be resumed. - A plan is eligible for resume when it is NOT in a terminal state - (applied, cancelled, constrained) and has been started. + A plan is eligible for resume when it is not in a permanently + terminal state (applied, cancelled, constrained). Note that + ``ERRORED`` is terminal per ``Plan.is_terminal`` but is still + eligible for resume here — it represents a recoverable failure, + not a permanent outcome. Args: plan_id: The plan ULID. diff --git a/src/cleveragents/domain/models/core/plan.py b/src/cleveragents/domain/models/core/plan.py index d515b811b..b4f85d864 100644 --- a/src/cleveragents/domain/models/core/plan.py +++ b/src/cleveragents/domain/models/core/plan.py @@ -10,9 +10,9 @@ Plans are instantiated from Action templates. The Action phase is the non-processing template phase. When a plan is *used* (via ``agents plan use``) it transitions to Strategize. -Terminal outcomes live in the Apply phase's processing state: -``applied`` (success), ``constrained`` (cannot proceed within strategy -constraints), ``errored``, or ``cancelled``. +Terminal processing states are ``applied`` (success, Apply only), +``constrained`` (cannot proceed, Apply only), ``errored`` (failed, +any phase), and ``cancelled`` (user/system cancelled, any phase). ## Lifecycle Phases @@ -29,8 +29,8 @@ constraints), ``errored``, or ``cancelled``. |----------------|---------------------------------------------------| | ``QUEUED`` | Phase entered, waiting to start | | ``PROCESSING`` | Actively working | -| ``ERRORED`` | Failed (may retry) | -| ``COMPLETE`` | Phase work finished (non-terminal) | +| ``ERRORED`` | Failed (terminal; recovery via revert/resume) | +| ``COMPLETE`` | Phase work finished (terminal within phase) | | ``APPLIED`` | Terminal success (Apply only) | | ``CONSTRAINED``| Cannot proceed within constraints (Apply only) | | ``CANCELLED`` | User/system cancelled (any phase) | @@ -784,6 +784,46 @@ class Plan(BaseModel): self.processing_state = ProcessingState.QUEUED return self + @model_validator(mode="after") + def validate_phase_state_constraints(self) -> Plan: + """Enforce per-phase processing-state constraints. + + The specification restricts certain processing states to + specific lifecycle phases: + + - ``APPLIED`` and ``CONSTRAINED`` are only valid in the + ``APPLY`` phase (they are terminal outcomes of the apply + step). + - ``COMPLETE`` is only valid in ``STRATEGIZE`` or ``EXECUTE`` + phases (it signals that phase work finished and the plan + should advance to the next phase; the Apply phase uses + ``APPLIED`` instead of ``COMPLETE``). + - ``QUEUED``, ``PROCESSING``, ``ERRORED``, and ``CANCELLED`` + are valid in any phase. + + Raises: + ValueError: If the phase/state combination violates the + specification constraints. + """ + apply_only_states = (ProcessingState.APPLIED, ProcessingState.CONSTRAINED) + if self.processing_state in apply_only_states and self.phase != PlanPhase.APPLY: + raise ValueError( + f"ProcessingState.{self.processing_state.name} is only valid " + f"in the APPLY phase, but phase is {self.phase.value!r}" + ) + + complete_phases = (PlanPhase.STRATEGIZE, PlanPhase.EXECUTE) + if ( + self.processing_state == ProcessingState.COMPLETE + and self.phase not in complete_phases + ): + raise ValueError( + f"ProcessingState.COMPLETE is only valid in STRATEGIZE or " + f"EXECUTE phases, but phase is {self.phase.value!r}" + ) + + return self + @model_validator(mode="after") def validate_project_link_alias_uniqueness(self) -> Plan: """Ensure project link aliases are unique within a plan.""" @@ -801,15 +841,46 @@ class Plan(BaseModel): def is_terminal(self) -> bool: """Check if plan is in a terminal state. - Terminal when: - - processing_state is APPLIED (Apply success) - - processing_state is CANCELLED (any phase) - - processing_state is CONSTRAINED (Apply cannot proceed) + Per the specification, the following processing states are + terminal regardless of which phase the plan is in: + + - ``APPLIED`` - Apply-phase success (changes committed). + - ``CANCELLED`` - User/system cancelled (any phase). + - ``CONSTRAINED`` - Apply cannot proceed within constraints. + - ``ERRORED`` - Processing failed (spec marks errored as + terminal for all processing phases). + + .. note:: **Why COMPLETE is not plan-terminal** + + The spec marks both ``ERRORED`` and ``COMPLETE`` as + "Terminal? Yes" in the Strategize/Execute phase tables. + However, the spec uses "terminal" at the *phase* level — + meaning processing stops for that phase. ``COMPLETE`` + signals that the phase finished successfully and the plan + should advance to the next phase (Strategize → Execute, + Execute → Apply). Including ``COMPLETE`` in + ``is_terminal`` would block ``can_transition_to_next_phase`` + and prevent all forward lifecycle progression. ``ERRORED`` + is plan-terminal because a failed plan must not auto-advance + — explicit recovery (revert or resume) is required. + See issue #918 for the design discussion. + + .. note:: + + *Terminal* does not mean *permanently irrecoverable*. + ``ERRORED`` and ``CONSTRAINED`` plans can still be reverted + to an earlier phase via ``revert_plan`` or resumed through + ``PlanResumeService``. ``APPLIED`` and ``CANCELLED`` are + permanently terminal — no further lifecycle transitions are + allowed. ``is_terminal`` is used by service guards to + prevent automatic progression, cancellation, and pausing, + but explicit recovery paths remain available. """ return self.processing_state in ( ProcessingState.APPLIED, ProcessingState.CANCELLED, ProcessingState.CONSTRAINED, + ProcessingState.ERRORED, ) @property @@ -913,7 +984,10 @@ class Plan(BaseModel): """Check if reversion to the given phase is valid. Reversion is valid when: - - The plan is not in a permanently terminal state (APPLIED or CANCELLED). + - The plan is not in a permanently terminal state (APPLIED or + CANCELLED). Note: ERRORED and CONSTRAINED are terminal per + ``is_terminal`` but are still revertable — they represent + recoverable failures, not permanent outcomes. - The target phase is a valid reversion target from the current phase. - The reversion count has not exceeded MAX_REVERSIONS. diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index 1ef1d46ab..fac86667b 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -33,6 +33,7 @@ Includes spec-aligned lifecycle models per Stage A5 from __future__ import annotations import json +import logging import re from datetime import UTC, datetime from typing import TYPE_CHECKING, Any, cast @@ -71,6 +72,8 @@ from cleveragents.domain.models.core import ( PlanStatus, ) +_logger = logging.getLogger(__name__) + # Create base class for all models Base = declarative_base() @@ -749,6 +752,45 @@ class LifecyclePlanModel(Base): # type: ignore[misc] phase_enum = PlanPhase(cast(str, self.phase)) state_enum = ProcessingState(cast(str, self.processing_state)) + # Defensive coercion: the Plan model validator now rejects + # certain phase/state combinations (e.g. COMPLETE in APPLY). + # Legacy DB rows written before the validator was added may + # contain these combinations. Coerce to the closest valid + # state to avoid crashing deserialization. + if state_enum == ProcessingState.COMPLETE and phase_enum == PlanPhase.APPLY: + _logger.warning( + "Coercing legacy phase/state combo: %s/%s -> %s/%s (plan_id=%s)", + phase_enum.value, + state_enum.value, + phase_enum.value, + ProcessingState.APPLIED.value, + self.plan_id, + ) + state_enum = ProcessingState.APPLIED + elif ( + state_enum in (ProcessingState.APPLIED, ProcessingState.CONSTRAINED) + and phase_enum != PlanPhase.APPLY + ): + _logger.warning( + "Coercing legacy phase/state combo: %s/%s -> %s/%s (plan_id=%s)", + phase_enum.value, + state_enum.value, + phase_enum.value, + ProcessingState.ERRORED.value, + self.plan_id, + ) + state_enum = ProcessingState.ERRORED + elif state_enum == ProcessingState.COMPLETE and phase_enum == PlanPhase.ACTION: + _logger.warning( + "Coercing legacy phase/state combo: %s/%s -> %s/%s (plan_id=%s)", + phase_enum.value, + state_enum.value, + phase_enum.value, + ProcessingState.QUEUED.value, + self.plan_id, + ) + state_enum = ProcessingState.QUEUED + # Build project links from child table project_links_list = [ ProjectLink( -- 2.52.0