UAT: `PlanLifecycleService.revert_plan()` allows forward phase transitions — Execute→Apply permitted as 'reversion' #1779

New issue

Open

opened 2026-04-02 23:48:29 +00:00 by freemo · 1 comment

freemo commented

2026-04-02 23:48:29 +00:00

Owner

Metadata

Branch: fix/plan-lifecycle-service-revert-forward-transition
Commit Message: fix(plan): restrict revert_plan() to valid backward phase transitions only
Milestone: v3.7.0
Parent Epic: #372

Background and Context

The PlanLifecycleService.revert_plan() method in src/cleveragents/application/services/plan_lifecycle_service.py uses can_transition(plan.phase, to_phase) to validate reversion targets. Since VALID_PHASE_TRANSITIONS[EXECUTE] includes both [APPLY, STRATEGIZE], calling revert_plan(plan_id, PlanPhase.APPLY) from an Execute-phase plan succeeds without error — performing a forward transition disguised as a reversion.

Spec Requirement (docs/specification.md line 18231):

"While the normal phase progression is forward (Action → Strategize → Execute → Apply), both the Execute and Apply phases may revert to Strategize under specific conditions. In all cases, the reversion target is always Strategize — there is no reversion to Execute or any other intermediate phase."

This is the service-layer manifestation of the same root cause as #1737 (Plan.can_revert_to() using VALID_PHASE_TRANSITIONS which conflates forward and backward transitions). Both bugs must be fixed together to fully close the reversion validation gap.

Current Behavior

revert_plan(plan_id, PlanPhase.APPLY) from an Execute-phase plan succeeds and transitions the plan to Apply/QUEUED state — bypassing the normal execute_plan() flow and its guards (estimation actor, error pattern consultation, async job enqueuing).

Steps to reproduce:

from cleveragents.application.services.plan_lifecycle_service import PlanLifecycleService
from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState
from cleveragents.config.settings import Settings

settings = Settings()
service = PlanLifecycleService(settings=settings)

action = service.create_action(
    name='local/exec-to-apply-revert',
    description='Test action',
    definition_of_done='Tests pass',
    strategy_actor='local/strategy-actor',
    execution_actor='local/execution-actor',
)
plan = service.use_action('local/exec-to-apply-revert')
plan_id = plan.identity.plan_id
service.start_strategize(plan_id)
service.complete_strategize(plan_id)
service.execute_plan(plan_id)
service.start_execute(plan_id)

# BUG: This should raise InvalidPhaseTransitionError but succeeds
plan = service.revert_plan(plan_id, PlanPhase.APPLY, reason='Invalid forward revert')
print(plan.phase)  # Prints 'apply' — BUG: forward transition allowed as reversion

Expected Behavior

revert_plan(plan_id, PlanPhase.APPLY) from Execute phase should raise InvalidPhaseTransitionError because APPLY is a forward transition, not a reversion. The only valid reversion target from any phase is STRATEGIZE.

Root Cause

revert_plan() validates the target phase using can_transition(plan.phase, to_phase) (line ~2261 of plan_lifecycle_service.py), which consults VALID_PHASE_TRANSITIONS. This map conflates forward and backward transitions. The fix is to validate against a separate VALID_REVERSION_TARGETS map that only permits backward transitions (i.e., only STRATEGIZE is a valid reversion target from EXECUTE or APPLY).

Code location: src/cleveragents/application/services/plan_lifecycle_service.py, method revert_plan() (line ~2218), specifically line ~2261:

if not can_transition(plan.phase, to_phase):
    raise InvalidPhaseTransitionError(...)

Impact

A caller can bypass the normal execute_plan() → apply_plan() flow by using revert_plan() as a shortcut, skipping:

Estimation actor invocation
Error pattern consultation
Async job enqueuing

Subtasks

Define VALID_REVERSION_TARGETS map in the plan domain (or reuse the fix from #1737 if it introduces a shared constant)
Update revert_plan() in PlanLifecycleService to validate to_phase against VALID_REVERSION_TARGETS instead of VALID_PHASE_TRANSITIONS
Raise InvalidPhaseTransitionError when to_phase is a forward transition (e.g., Execute→Apply)
Tests (Behave): Add scenario — revert_plan() from Execute phase to Apply raises InvalidPhaseTransitionError
Tests (Behave): Add scenario — revert_plan() from Execute phase to Strategize succeeds (valid reversion)
Tests (Behave): Add scenario — revert_plan() from Apply phase to Strategize succeeds (valid reversion)
Tests (Robot): Add integration test for revert_plan() reversion guard
Verify coverage >= 97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
revert_plan(plan_id, PlanPhase.APPLY) from Execute phase raises InvalidPhaseTransitionError.
revert_plan(plan_id, PlanPhase.STRATEGIZE) from Execute or Apply phase succeeds as before.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.
All nox stages pass
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-uat-tester

## Metadata - **Branch**: `fix/plan-lifecycle-service-revert-forward-transition` - **Commit Message**: `fix(plan): restrict revert_plan() to valid backward phase transitions only` - **Milestone**: v3.7.0 - **Parent Epic**: #372 ## Background and Context The `PlanLifecycleService.revert_plan()` method in `src/cleveragents/application/services/plan_lifecycle_service.py` uses `can_transition(plan.phase, to_phase)` to validate reversion targets. Since `VALID_PHASE_TRANSITIONS[EXECUTE]` includes both `[APPLY, STRATEGIZE]`, calling `revert_plan(plan_id, PlanPhase.APPLY)` from an Execute-phase plan succeeds without error — performing a forward transition disguised as a reversion. **Spec Requirement (`docs/specification.md` line 18231):** > "While the normal phase progression is forward (Action → Strategize → Execute → Apply), both the Execute and Apply phases may **revert to Strategize** under specific conditions. In all cases, the reversion target is always Strategize — there is no reversion to Execute or any other intermediate phase." This is the service-layer manifestation of the same root cause as #1737 (`Plan.can_revert_to()` using `VALID_PHASE_TRANSITIONS` which conflates forward and backward transitions). Both bugs must be fixed together to fully close the reversion validation gap. ## Current Behavior `revert_plan(plan_id, PlanPhase.APPLY)` from an Execute-phase plan succeeds and transitions the plan to Apply/QUEUED state — bypassing the normal `execute_plan()` flow and its guards (estimation actor, error pattern consultation, async job enqueuing). **Steps to reproduce:** ```python from cleveragents.application.services.plan_lifecycle_service import PlanLifecycleService from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState from cleveragents.config.settings import Settings settings = Settings() service = PlanLifecycleService(settings=settings) action = service.create_action( name='local/exec-to-apply-revert', description='Test action', definition_of_done='Tests pass', strategy_actor='local/strategy-actor', execution_actor='local/execution-actor', ) plan = service.use_action('local/exec-to-apply-revert') plan_id = plan.identity.plan_id service.start_strategize(plan_id) service.complete_strategize(plan_id) service.execute_plan(plan_id) service.start_execute(plan_id) # BUG: This should raise InvalidPhaseTransitionError but succeeds plan = service.revert_plan(plan_id, PlanPhase.APPLY, reason='Invalid forward revert') print(plan.phase) # Prints 'apply' — BUG: forward transition allowed as reversion ``` ## Expected Behavior `revert_plan(plan_id, PlanPhase.APPLY)` from Execute phase should raise `InvalidPhaseTransitionError` because APPLY is a forward transition, not a reversion. The only valid reversion target from any phase is `STRATEGIZE`. ## Root Cause `revert_plan()` validates the target phase using `can_transition(plan.phase, to_phase)` (line ~2261 of `plan_lifecycle_service.py`), which consults `VALID_PHASE_TRANSITIONS`. This map conflates forward and backward transitions. The fix is to validate against a separate `VALID_REVERSION_TARGETS` map that only permits backward transitions (i.e., only `STRATEGIZE` is a valid reversion target from `EXECUTE` or `APPLY`). **Code location:** `src/cleveragents/application/services/plan_lifecycle_service.py`, method `revert_plan()` (line ~2218), specifically line ~2261: ```python if not can_transition(plan.phase, to_phase): raise InvalidPhaseTransitionError(...) ``` ## Impact A caller can bypass the normal `execute_plan()` → `apply_plan()` flow by using `revert_plan()` as a shortcut, skipping: - Estimation actor invocation - Error pattern consultation - Async job enqueuing ## Subtasks - [ ] Define `VALID_REVERSION_TARGETS` map in the plan domain (or reuse the fix from #1737 if it introduces a shared constant) - [ ] Update `revert_plan()` in `PlanLifecycleService` to validate `to_phase` against `VALID_REVERSION_TARGETS` instead of `VALID_PHASE_TRANSITIONS` - [ ] Raise `InvalidPhaseTransitionError` when `to_phase` is a forward transition (e.g., Execute→Apply) - [ ] Tests (Behave): Add scenario — `revert_plan()` from Execute phase to Apply raises `InvalidPhaseTransitionError` - [ ] Tests (Behave): Add scenario — `revert_plan()` from Execute phase to Strategize succeeds (valid reversion) - [ ] Tests (Behave): Add scenario — `revert_plan()` from Apply phase to Strategize succeeds (valid reversion) - [ ] Tests (Robot): Add integration test for `revert_plan()` reversion guard - [ ] Verify coverage >= 97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - `revert_plan(plan_id, PlanPhase.APPLY)` from Execute phase raises `InvalidPhaseTransitionError`. - `revert_plan(plan_id, PlanPhase.STRATEGIZE)` from Execute or Apply phase succeeds as before. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. - All nox stages pass - Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester

freemo added this to the v3.7.0 milestone

2026-04-02 23:49:50 +00:00

freemo commented

2026-04-02 23:51:08 +00:00

Author

Owner

Closing as duplicate of #1737 (Plan.can_revert_to() allows forward transitions).

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: ca-project-owner

Closing as duplicate of #1737 (Plan.can_revert_to() allows forward transitions). --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo

2026-04-02 23:51:09 +00:00