BUG-HUNT: [error-handling] Plan.validate_phase_state_consistency dead code: processing_state is never None due to non-Optional field type #6429

New issue

Open

opened 2026-04-09 21:02:31 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-09 21:02:31 +00:00

Owner

Bug Report: [error-handling] — `Plan.validate_phase_state_consistency` contains dead code that can never execute because `processing_state` is declared as non-Optional with a default value

Severity Assessment

Impact: The dead code indicates a missing guard that was intended to enforce a specific behavior (defaulting ACTION-phase plans to QUEUED state). If someone later changes the field to Optional or the default, the business rule will silently be unenforced without any test failure. The current lack of enforcement is a latent spec-alignment risk.
Likelihood: Low immediate impact; code quality / latent risk
Priority: Low (backlog)

Location

File: src/cleveragents/domain/models/core/plan.py
Function/Class: Plan.validate_phase_state_consistency (model validator)
Lines: 832–842

Description

The validate_phase_state_consistency validator contains this check:

if self.phase == PlanPhase.ACTION and self.processing_state is None:
    self.processing_state = ProcessingState.QUEUED

However, processing_state is declared as:

processing_state: ProcessingState = Field(
    ProcessingState.QUEUED,
    description="Processing state in the current phase",
)

The type annotation is ProcessingState (not Optional[ProcessingState]) with a default value of ProcessingState.QUEUED. In Pydantic v2, passing None for a non-Optional field would raise a ValidationError before the model validator even runs. Therefore, self.processing_state is None can never be True when a valid Plan is constructed.

This means the business rule documented in the validator's docstring ("ACTION phase defaults to QUEUED if no state supplied") is effectively dead code — processing_state is always QUEUED by default anyway (but through the field default, not through this explicit validator logic).

Evidence

# src/cleveragents/domain/models/core/plan.py, lines 606-609
processing_state: ProcessingState = Field(
    ProcessingState.QUEUED,         # ← Always defaults to QUEUED
    description="Processing state in the current phase",
)
# Type is ProcessingState, NOT Optional[ProcessingState]

# Lines 832-842
@model_validator(mode="after")
def validate_phase_state_consistency(self) -> Plan:
    """Ensure phase and state are consistent.

    - ACTION phase defaults to QUEUED if no state supplied.  ← documented intent
    - CANCELLED state is terminal for any phase.
    - Apply phase accepts APPLIED / CONSTRAINED as terminal states.
    """
    if self.phase == PlanPhase.ACTION and self.processing_state is None:
        # ↑ DEAD CODE: processing_state can never be None in a valid Plan
        self.processing_state = ProcessingState.QUEUED
    return self

The condition self.processing_state is None will NEVER be True because:

ProcessingState is a non-optional type
If None is passed, Pydantic v2's validation raises ValidationError before @model_validator(mode="after") runs
If no value is passed, the field default ProcessingState.QUEUED is used

Expected Behavior

The validator should either:

Be updated to document that it's a no-op (and the behavior is provided by the field default), or
Be given a real guard to check (e.g. validate that non-ACTION phases DO have a processing_state set through the plan use flow)

Actual Behavior

The is None check is dead code — it can never trigger. The docstring's intent ("ACTION phase defaults to QUEUED if no state supplied") is achieved through the field default, not this validator.

Suggested Fix

Either remove the dead code and update the docstring to remove the false claim, or convert the validator into a meaningful constraint:

Option A (remove dead code):

@model_validator(mode="after")
def validate_phase_state_consistency(self) -> Plan:
    """Placeholder for future phase/state consistency checks.
    
    Note: ACTION phase defaults to QUEUED via field default.
    """
    # No-op currently; reserved for future phase/state rules
    return self

Option B (add a real check that the field default already satisfies):
Consider removing the validator entirely if no meaningful validation is needed, to avoid confusion.

TDD Note

After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: @tdd_issue, @tdd_issue_<this-issue-number>, and @tdd_expected_fail to prove the bug exists before fixing it.

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: bug-hunter

## Bug Report: [error-handling] — `Plan.validate_phase_state_consistency` contains dead code that can never execute because `processing_state` is declared as non-Optional with a default value ### Severity Assessment - **Impact**: The dead code indicates a missing guard that was intended to enforce a specific behavior (defaulting ACTION-phase plans to QUEUED state). If someone later changes the field to `Optional` or the default, the business rule will silently be unenforced without any test failure. The current lack of enforcement is a latent spec-alignment risk. - **Likelihood**: Low immediate impact; code quality / latent risk - **Priority**: Low (backlog) ### Location - **File**: `src/cleveragents/domain/models/core/plan.py` - **Function/Class**: `Plan.validate_phase_state_consistency` (model validator) - **Lines**: 832–842 ### Description The `validate_phase_state_consistency` validator contains this check: ```python if self.phase == PlanPhase.ACTION and self.processing_state is None: self.processing_state = ProcessingState.QUEUED ``` However, `processing_state` is declared as: ```python processing_state: ProcessingState = Field( ProcessingState.QUEUED, description="Processing state in the current phase", ) ``` The type annotation is `ProcessingState` (not `Optional[ProcessingState]`) with a default value of `ProcessingState.QUEUED`. In Pydantic v2, passing `None` for a non-Optional field would raise a `ValidationError` before the model validator even runs. Therefore, `self.processing_state is None` can **never be True** when a valid `Plan` is constructed. This means the business rule documented in the validator's docstring ("ACTION phase defaults to QUEUED if no state supplied") is effectively dead code — `processing_state` is always `QUEUED` by default anyway (but through the field default, not through this explicit validator logic). ### Evidence ```python # src/cleveragents/domain/models/core/plan.py, lines 606-609 processing_state: ProcessingState = Field( ProcessingState.QUEUED, # ← Always defaults to QUEUED description="Processing state in the current phase", ) # Type is ProcessingState, NOT Optional[ProcessingState] # Lines 832-842 @model_validator(mode="after") def validate_phase_state_consistency(self) -> Plan: """Ensure phase and state are consistent. - ACTION phase defaults to QUEUED if no state supplied. ← documented intent - CANCELLED state is terminal for any phase. - Apply phase accepts APPLIED / CONSTRAINED as terminal states. """ if self.phase == PlanPhase.ACTION and self.processing_state is None: # ↑ DEAD CODE: processing_state can never be None in a valid Plan self.processing_state = ProcessingState.QUEUED return self ``` The condition `self.processing_state is None` will NEVER be `True` because: 1. `ProcessingState` is a non-optional type 2. If `None` is passed, Pydantic v2's validation raises `ValidationError` before `@model_validator(mode="after")` runs 3. If no value is passed, the field default `ProcessingState.QUEUED` is used ### Expected Behavior The validator should either: - Be updated to document that it's a no-op (and the behavior is provided by the field default), or - Be given a real guard to check (e.g. validate that non-ACTION phases DO have a processing_state set through the plan use flow) ### Actual Behavior The `is None` check is dead code — it can never trigger. The docstring's intent ("ACTION phase defaults to QUEUED if no state supplied") is achieved through the field default, not this validator. ### Suggested Fix Either remove the dead code and update the docstring to remove the false claim, or convert the validator into a meaningful constraint: **Option A** (remove dead code): ```python @model_validator(mode="after") def validate_phase_state_consistency(self) -> Plan: """Placeholder for future phase/state consistency checks. Note: ACTION phase defaults to QUEUED via field default. """ # No-op currently; reserved for future phase/state rules return self ``` **Option B** (add a real check that the field default already satisfies): Consider removing the validator entirely if no meaningful validation is needed, to avoid confusion. ### Category error-handling ### TDD Note After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: `@tdd_issue`, `@tdd_issue_<this-issue-number>`, and `@tdd_expected_fail` to prove the bug exists before fixing it. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: bug-hunter

HAL9000 added the

labels

2026-04-09 21:07:11 +00:00

HAL9000 added this to the v3.2.0 milestone

2026-04-09 21:09:47 +00:00

HAL9000 added