cleveragents/cleveragents-core

Fork 3

UAT: `Plan.can_revert_to()` incorrectly allows forward phase transitions — STRATEGIZE→EXECUTE and EXECUTE→APPLY treated as valid reversions #1737

New issue

Open

opened 2026-04-02 23:38:43 +00:00 by freemo · 2 comments

freemo commented

2026-04-02 23:38:43 +00:00

Owner

Metadata

Branch: fix/plan-can-revert-to-forward-transition
Commit Message: fix(plan): restrict can_revert_to() to valid backward phase transitions only
Milestone: v3.7.0
Parent Epic: (see orphan note below)

Background and Context

The Plan.can_revert_to() method in src/cleveragents/domain/models/core/plan.py uses can_transition(self.phase, phase) to validate reversion targets. However, can_transition() uses VALID_PHASE_TRANSITIONS which conflates both forward transitions AND backward (reversion) transitions in a single map.

This causes can_revert_to() to incorrectly return True for forward transitions:

STRATEGIZE.can_revert_to(EXECUTE) → returns True (BUG: EXECUTE is forward from STRATEGIZE)
EXECUTE.can_revert_to(APPLY) → returns True (BUG: APPLY is forward from EXECUTE)

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."

Current Behavior

can_revert_to() returns True for forward transitions because it reuses VALID_PHASE_TRANSITIONS which includes both forward and backward transitions.

Steps to reproduce:

from ulid import ULID
from cleveragents.domain.models.core.plan import Plan, PlanPhase, ProcessingState, PlanIdentity, NamespacedName

def make_plan(phase, state):
    return Plan(
        identity=PlanIdentity(plan_id=str(ULID())),
        namespaced_name=NamespacedName(namespace='local', name='test-plan'),
        action_name='local/test-action',
        description='Test',
        phase=phase,
        processing_state=state,
    )

# BUG: Both of these return True but should return False
p1 = make_plan(PlanPhase.STRATEGIZE, ProcessingState.ERRORED)
print(p1.can_revert_to(PlanPhase.EXECUTE))  # Returns True (BUG: forward transition)

p2 = make_plan(PlanPhase.EXECUTE, ProcessingState.ERRORED)
print(p2.can_revert_to(PlanPhase.APPLY))  # Returns True (BUG: forward transition)

Expected Behavior

can_revert_to() should only return True when the target phase is a valid backward transition. Per the specification, the only valid reversion target is STRATEGIZE — reachable from EXECUTE or APPLY under specific conditions. No forward transitions should ever be considered valid reversions.

Root Cause

VALID_PHASE_TRANSITIONS (at src/cleveragents/domain/models/core/plan.py, method can_revert_to() ~line 1040, and VALID_PHASE_TRANSITIONS dict ~line 1185) is used for both forward transitions (execute_plan, apply_plan) and backward transitions (revert_plan). The can_revert_to() method should use a separate VALID_REVERSION_TARGETS map that only includes backward transitions:

VALID_REVERSION_TARGETS = {
    PlanPhase.EXECUTE: [PlanPhase.STRATEGIZE],
    PlanPhase.APPLY: [PlanPhase.STRATEGIZE],
}

Acceptance Criteria

Plan.can_revert_to(PlanPhase.EXECUTE) returns False when current phase is STRATEGIZE
Plan.can_revert_to(PlanPhase.APPLY) returns False when current phase is EXECUTE
Plan.can_revert_to(PlanPhase.STRATEGIZE) returns True when current phase is EXECUTE (and other reversion preconditions are met)
Plan.can_revert_to(PlanPhase.STRATEGIZE) returns True when current phase is APPLY (and other reversion preconditions are met)
A separate VALID_REVERSION_TARGETS constant is introduced and used exclusively by can_revert_to()
All existing tests continue to pass

Supporting Information

Code location: src/cleveragents/domain/models/core/plan.py
- Method can_revert_to() (~line 1040)
- VALID_PHASE_TRANSITIONS dict (~line 1185)
Spec reference: docs/specification.md line 18231 — phase reversion rules
Related issues: #1413 (Refactor: Decompose large Plan model), #1414 (Refactor: Simplify as_cli_dict)

Subtasks

Add TDD issue-capture Behave scenario (@tdd_expected_fail) proving can_revert_to() incorrectly returns True for forward transitions
Introduce VALID_REVERSION_TARGETS constant in plan.py containing only backward transition targets
Refactor can_revert_to() to use VALID_REVERSION_TARGETS instead of VALID_PHASE_TRANSITIONS
Update/add Behave unit test scenarios covering all valid and invalid reversion targets
Update/add Robot Framework integration tests for plan reversion phase validation
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.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly (fix(plan): restrict can_revert_to() to valid backward phase transitions only), 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 (fix/plan-can-revert-to-forward-transition).
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-new-issue-creator

## Metadata - **Branch**: `fix/plan-can-revert-to-forward-transition` - **Commit Message**: `fix(plan): restrict can_revert_to() to valid backward phase transitions only` - **Milestone**: v3.7.0 - **Parent Epic**: *(see orphan note below)* ## Background and Context The `Plan.can_revert_to()` method in `src/cleveragents/domain/models/core/plan.py` uses `can_transition(self.phase, phase)` to validate reversion targets. However, `can_transition()` uses `VALID_PHASE_TRANSITIONS` which conflates both forward transitions AND backward (reversion) transitions in a single map. This causes `can_revert_to()` to incorrectly return `True` for forward transitions: - `STRATEGIZE.can_revert_to(EXECUTE)` → returns `True` (**BUG**: EXECUTE is forward from STRATEGIZE) - `EXECUTE.can_revert_to(APPLY)` → returns `True` (**BUG**: APPLY is forward from EXECUTE) **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." ## Current Behavior `can_revert_to()` returns `True` for forward transitions because it reuses `VALID_PHASE_TRANSITIONS` which includes both forward and backward transitions. **Steps to reproduce:** ```python from ulid import ULID from cleveragents.domain.models.core.plan import Plan, PlanPhase, ProcessingState, PlanIdentity, NamespacedName def make_plan(phase, state): return Plan( identity=PlanIdentity(plan_id=str(ULID())), namespaced_name=NamespacedName(namespace='local', name='test-plan'), action_name='local/test-action', description='Test', phase=phase, processing_state=state, ) # BUG: Both of these return True but should return False p1 = make_plan(PlanPhase.STRATEGIZE, ProcessingState.ERRORED) print(p1.can_revert_to(PlanPhase.EXECUTE)) # Returns True (BUG: forward transition) p2 = make_plan(PlanPhase.EXECUTE, ProcessingState.ERRORED) print(p2.can_revert_to(PlanPhase.APPLY)) # Returns True (BUG: forward transition) ``` ## Expected Behavior `can_revert_to()` should only return `True` when the target phase is a valid **backward** transition. Per the specification, the only valid reversion target is `STRATEGIZE` — reachable from `EXECUTE` or `APPLY` under specific conditions. No forward transitions should ever be considered valid reversions. ## Root Cause `VALID_PHASE_TRANSITIONS` (at `src/cleveragents/domain/models/core/plan.py`, method `can_revert_to()` ~line 1040, and `VALID_PHASE_TRANSITIONS` dict ~line 1185) is used for both forward transitions (`execute_plan`, `apply_plan`) and backward transitions (`revert_plan`). The `can_revert_to()` method should use a separate `VALID_REVERSION_TARGETS` map that only includes backward transitions: ```python VALID_REVERSION_TARGETS = { PlanPhase.EXECUTE: [PlanPhase.STRATEGIZE], PlanPhase.APPLY: [PlanPhase.STRATEGIZE], } ``` ## Acceptance Criteria - `Plan.can_revert_to(PlanPhase.EXECUTE)` returns `False` when current phase is `STRATEGIZE` - `Plan.can_revert_to(PlanPhase.APPLY)` returns `False` when current phase is `EXECUTE` - `Plan.can_revert_to(PlanPhase.STRATEGIZE)` returns `True` when current phase is `EXECUTE` (and other reversion preconditions are met) - `Plan.can_revert_to(PlanPhase.STRATEGIZE)` returns `True` when current phase is `APPLY` (and other reversion preconditions are met) - A separate `VALID_REVERSION_TARGETS` constant is introduced and used exclusively by `can_revert_to()` - All existing tests continue to pass ## Supporting Information - **Code location**: `src/cleveragents/domain/models/core/plan.py` - Method `can_revert_to()` (~line 1040) - `VALID_PHASE_TRANSITIONS` dict (~line 1185) - **Spec reference**: `docs/specification.md` line 18231 — phase reversion rules - **Related issues**: #1413 (Refactor: Decompose large Plan model), #1414 (Refactor: Simplify as_cli_dict) ## Subtasks - [ ] Add TDD issue-capture Behave scenario (`@tdd_expected_fail`) proving `can_revert_to()` incorrectly returns `True` for forward transitions - [ ] Introduce `VALID_REVERSION_TARGETS` constant in `plan.py` containing only backward transition targets - [ ] Refactor `can_revert_to()` to use `VALID_REVERSION_TARGETS` instead of `VALID_PHASE_TRANSITIONS` - [ ] Update/add Behave unit test scenarios covering all valid and invalid reversion targets - [ ] Update/add Robot Framework integration tests for plan reversion phase validation - [ ] 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. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly (`fix(plan): restrict can_revert_to() to valid backward phase transitions only`), 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 (`fix/plan-can-revert-to-forward-transition`). - 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-new-issue-creator

freemo added this to the v3.7.0 milestone

2026-04-02 23:38:58 +00:00

freemo commented

2026-04-02 23:40:26 +00:00

Author

Owner

⚠️ Orphan Issue — Manual Parent Epic Linking Required

This issue was created automatically during UAT testing and could not be linked to a parent Epic because no Plan lifecycle / domain model Epic currently exists in the repository.

The only open Epic at time of creation is:

#1678: Epic: CI Execution Time Optimization — which is unrelated to Plan domain models.

Action required for a maintainer: Please either:

Create a new Epic for "Plan Domain Model Correctness" (or similar) and link this issue as a child (this issue blocks the Epic), or
Link this issue to an existing relevant Epic if one is created in the future.

Per CONTRIBUTING.md, orphan issues are not permitted. This comment serves as the tracking flag until the parent link is established.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

⚠️ **Orphan Issue — Manual Parent Epic Linking Required** This issue was created automatically during UAT testing and could not be linked to a parent Epic because no Plan lifecycle / domain model Epic currently exists in the repository. The only open Epic at time of creation is: - **#1678**: Epic: CI Execution Time Optimization — which is unrelated to Plan domain models. **Action required for a maintainer:** Please either: 1. Create a new Epic for "Plan Domain Model Correctness" (or similar) and link this issue as a child (this issue **blocks** the Epic), or 2. Link this issue to an existing relevant Epic if one is created in the future. Per CONTRIBUTING.md, orphan issues are not permitted. This comment serves as the tracking flag until the parent link is established. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo commented

2026-04-02 23:42:19 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Priority/High (confirmed) — incorrect phase transition logic is a correctness bug
MoSCoW: MoSCoW/Must Have — the Plan lifecycle is a core architectural concept per the specification. If can_revert_to() incorrectly allows forward transitions, it undermines the correction engine and decision tree integrity. Must Have.
Milestone: confirmed

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

Issue triaged by project owner: - **State**: Verified - **Priority**: Priority/High (confirmed) — incorrect phase transition logic is a correctness bug - **MoSCoW**: MoSCoW/Must Have — the Plan lifecycle is a core architectural concept per the specification. If `can_revert_to()` incorrectly allows forward transitions, it undermines the correction engine and decision tree integrity. Must Have. - **Milestone**: confirmed --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo added the

MoSCoW

Must have

label

2026-04-02 23:42:19 +00:00

freemo referenced this issue

2026-04-02 23:42:52 +00:00

[Automated] Product Build Session State #1576

freemo referenced this issue

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

[UAT-SUPERVISOR] Progress Report — Cycle 451 — v3.7.0 #1778

freemo referenced this issue