UAT: Missing fail-fast empty-string validation for plan_id in plan lifecycle service public methods #3891

New issue

Open

opened 2026-04-06 07:12:03 +00:00 by freemo · 0 comments

freemo commented

2026-04-06 07:12:03 +00:00

Owner

Metadata

Branch: fix/plan-lifecycle-service-plan-id-validation
Commit Message: fix(plan-lifecycle-service): add fail-fast empty-string validation for plan_id in public methods
Milestone: (none — backlog per Milestone Scope Guard)
Parent Epic: #362

Bug Report

Feature Area: Error Handling and Resilience
Severity: Medium
Found by: UAT automated testing

What Was Tested

The public methods start_strategize(), start_execute(), start_apply(), complete_strategize(), complete_execute(), complete_apply(), and get_plan() in src/cleveragents/application/services/plan_lifecycle_service.py were analyzed for compliance with the CONTRIBUTING.md fail-fast validation requirements.

Expected Behavior (from CONTRIBUTING.md)

All public and protected class methods must validate their arguments as the very first step before any other logic.
Validation checks must include:

Empty Strings and Collections: Reject empty inputs where they are not explicitly allowed.

Actual Behavior

The lifecycle service public methods that accept plan_id: str do NOT validate that plan_id is non-empty before proceeding. They immediately call self.get_plan(plan_id) which will raise NotFoundError (not ValidationError) for an empty string:

# src/cleveragents/application/services/plan_lifecycle_service.py
def start_strategize(self, plan_id: str) -> Plan:
    plan = self.get_plan(plan_id)  # No empty-string check first!
    ...

def start_execute(self, plan_id: str) -> Plan:
    plan = self.get_plan(plan_id)  # No empty-string check first!
    ...

def start_apply(self, plan_id: str) -> Plan:
    plan = self.get_plan(plan_id)  # No empty-string check first!
    ...

def complete_strategize(self, plan_id: str) -> Plan:
    plan = self.get_plan(plan_id)  # No empty-string check first!
    ...

Compare with PlanExecutor.run_execute() which DOES have the correct fail-fast check:

# src/cleveragents/application/services/plan_executor.py line 762-763
def run_execute(self, plan_id: str, ...) -> ...:
    if not plan_id:
        raise ValidationError("plan_id must not be empty")  # Correct!

Why This Is a Problem

Violates CONTRIBUTING.md fail-fast principle: Required string arguments are not validated at the method boundary.
Wrong exception type: An empty plan_id raises NotFoundError (resource not found) instead of ValidationError (invalid input). This is semantically incorrect and makes error handling harder for callers.
Inconsistent: PlanExecutor.run_execute() and PlanExecutor.run_strategize() both have the correct if not plan_id: raise ValidationError(...) check, but the underlying lifecycle service methods do not.
Late failure: Without early validation, the error manifests as a database query with an empty string, which may produce confusing error messages.

Affected Methods

get_plan(plan_id: str) — line ~1314
start_strategize(plan_id: str) — line ~1395
complete_strategize(plan_id: str) — line ~1494
start_execute(plan_id: str) — line ~1667
complete_execute(plan_id: str) — line ~1697
start_apply(plan_id: str) — line ~1825
complete_apply(plan_id: str, ...) — line ~1847

Recommended Fix

Add fail-fast validation at the start of each affected method, consistent with PlanExecutor:

def start_strategize(self, plan_id: str) -> Plan:
    if not plan_id or not plan_id.strip():
        raise ValidationError("plan_id must not be empty")
    plan = self.get_plan(plan_id)
    ...

Code Location

File: src/cleveragents/application/services/plan_lifecycle_service.py
Multiple methods (see list above)

Subtasks

Add if not plan_id or not plan_id.strip(): raise ValidationError("plan_id must not be empty") to get_plan()
Add fail-fast plan_id validation to start_strategize()
Add fail-fast plan_id validation to complete_strategize()
Add fail-fast plan_id validation to start_execute()
Add fail-fast plan_id validation to complete_execute()
Add fail-fast plan_id validation to start_apply()
Add fail-fast plan_id validation to complete_apply()
Tests (Behave/Robot): Add/update scenarios asserting ValidationError is raised for empty plan_id in each affected method
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.
Every affected public method in plan_lifecycle_service.py raises ValidationError (not NotFoundError) when called with an empty or whitespace-only plan_id.
Behaviour is consistent with PlanExecutor.run_execute() and PlanExecutor.run_strategize().
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%.

Backlog note: This issue was discovered during autonomous operation
on milestone v3.2.0. It does not block milestone completion and has been
placed in the backlog for human review and future milestone assignment.

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

## Metadata - **Branch**: `fix/plan-lifecycle-service-plan-id-validation` - **Commit Message**: `fix(plan-lifecycle-service): add fail-fast empty-string validation for plan_id in public methods` - **Milestone**: *(none — backlog per Milestone Scope Guard)* - **Parent Epic**: #362 ## Bug Report **Feature Area:** Error Handling and Resilience **Severity:** Medium **Found by:** UAT automated testing ### What Was Tested The public methods `start_strategize()`, `start_execute()`, `start_apply()`, `complete_strategize()`, `complete_execute()`, `complete_apply()`, and `get_plan()` in `src/cleveragents/application/services/plan_lifecycle_service.py` were analyzed for compliance with the CONTRIBUTING.md fail-fast validation requirements. ### Expected Behavior (from CONTRIBUTING.md) > All `public` and `protected` class methods **must** validate their arguments as the very first step before any other logic. > Validation checks must include: > - **Empty Strings and Collections:** Reject empty inputs where they are not explicitly allowed. ### Actual Behavior The lifecycle service public methods that accept `plan_id: str` do NOT validate that `plan_id` is non-empty before proceeding. They immediately call `self.get_plan(plan_id)` which will raise `NotFoundError` (not `ValidationError`) for an empty string: ```python # src/cleveragents/application/services/plan_lifecycle_service.py def start_strategize(self, plan_id: str) -> Plan: plan = self.get_plan(plan_id) # No empty-string check first! ... def start_execute(self, plan_id: str) -> Plan: plan = self.get_plan(plan_id) # No empty-string check first! ... def start_apply(self, plan_id: str) -> Plan: plan = self.get_plan(plan_id) # No empty-string check first! ... def complete_strategize(self, plan_id: str) -> Plan: plan = self.get_plan(plan_id) # No empty-string check first! ... ``` Compare with `PlanExecutor.run_execute()` which DOES have the correct fail-fast check: ```python # src/cleveragents/application/services/plan_executor.py line 762-763 def run_execute(self, plan_id: str, ...) -> ...: if not plan_id: raise ValidationError("plan_id must not be empty") # Correct! ``` ### Why This Is a Problem 1. **Violates CONTRIBUTING.md fail-fast principle**: Required string arguments are not validated at the method boundary. 2. **Wrong exception type**: An empty `plan_id` raises `NotFoundError` (resource not found) instead of `ValidationError` (invalid input). This is semantically incorrect and makes error handling harder for callers. 3. **Inconsistent**: `PlanExecutor.run_execute()` and `PlanExecutor.run_strategize()` both have the correct `if not plan_id: raise ValidationError(...)` check, but the underlying lifecycle service methods do not. 4. **Late failure**: Without early validation, the error manifests as a database query with an empty string, which may produce confusing error messages. ### Affected Methods - `get_plan(plan_id: str)` — line ~1314 - `start_strategize(plan_id: str)` — line ~1395 - `complete_strategize(plan_id: str)` — line ~1494 - `start_execute(plan_id: str)` — line ~1667 - `complete_execute(plan_id: str)` — line ~1697 - `start_apply(plan_id: str)` — line ~1825 - `complete_apply(plan_id: str, ...)` — line ~1847 ### Recommended Fix Add fail-fast validation at the start of each affected method, consistent with `PlanExecutor`: ```python def start_strategize(self, plan_id: str) -> Plan: if not plan_id or not plan_id.strip(): raise ValidationError("plan_id must not be empty") plan = self.get_plan(plan_id) ... ``` ### Code Location - File: `src/cleveragents/application/services/plan_lifecycle_service.py` - Multiple methods (see list above) ## Subtasks - [ ] Add `if not plan_id or not plan_id.strip(): raise ValidationError("plan_id must not be empty")` to `get_plan()` - [ ] Add fail-fast `plan_id` validation to `start_strategize()` - [ ] Add fail-fast `plan_id` validation to `complete_strategize()` - [ ] Add fail-fast `plan_id` validation to `start_execute()` - [ ] Add fail-fast `plan_id` validation to `complete_execute()` - [ ] Add fail-fast `plan_id` validation to `start_apply()` - [ ] Add fail-fast `plan_id` validation to `complete_apply()` - [ ] Tests (Behave/Robot): Add/update scenarios asserting `ValidationError` is raised for empty `plan_id` in each affected method - [ ] 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. - Every affected public method in `plan_lifecycle_service.py` raises `ValidationError` (not `NotFoundError`) when called with an empty or whitespace-only `plan_id`. - Behaviour is consistent with `PlanExecutor.run_execute()` and `PlanExecutor.run_strategize()`. - 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%. > **Backlog note:** This issue was discovered during autonomous operation > on milestone v3.2.0. It does not block milestone completion and has been > placed in the backlog for human review and future milestone assignment. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo added a new dependency

2026-04-06 07:12:22 +00:00

#362 Epic: Security & Safety Hardening

freemo referenced this issue

2026-04-06 07:13:06 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo referenced this issue

2026-04-06 07:21:26 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo referenced this issue

2026-04-06 10:35:53 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775