cleveragents/cleveragents-core

Fork 3

UAT: `PlanLifecycleService.get_plan()` does not validate `plan_id` argument — empty string or None causes confusing `NotFoundError` instead of `ValidationError` #5785

New issue

Open

opened 2026-04-09 09:28:37 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-09 09:28:37 +00:00

Owner

Summary

src/cleveragents/application/services/plan_lifecycle_service.py get_plan() method does not validate the plan_id argument before querying the database. Per CONTRIBUTING.md, all public methods must validate arguments as the first step (fail-fast behavior). An empty string or whitespace-only plan_id should raise ValidationError immediately, not NotFoundError after a database query.

Code Location

# plan_lifecycle_service.py lines 1219-1247
def get_plan(self, plan_id: str) -> Plan:
    """Get a plan by ID."""
    plan = self._plans.get(plan_id)  # <-- No validation of plan_id
    if plan is not None:
        return plan
    # Try persistence layer
    if self._persisted and self.unit_of_work is not None:
        with self.unit_of_work.transaction() as ctx:
            persisted = ctx.lifecycle_plans.get(plan_id)  # <-- DB query with empty string
        if persisted is not None:
            self._plans[plan_id] = persisted
            return persisted
    raise NotFoundError(
        resource_type="plan",
        resource_id=plan_id,  # <-- Shows empty string in error message
    )

Expected Behavior (per CONTRIBUTING.md)

Per CONTRIBUTING.md:

All public and protected class methods must validate arguments as the first guard. Perform these checks before any other logic.

Empty Strings: Reject empty strings where non-empty strings are required.

Null Checks: Reject null values where not expected.

When plan_id is empty or None:

Should raise ValidationError("plan_id must not be empty") immediately
Should NOT perform a database query
Should NOT raise NotFoundError("Plan '' not found") (confusing message)

Actual Behavior

When plan_id is empty string "":

self._plans.get("") returns None (no match)
A database query is executed with empty string
NotFoundError("Plan '' not found") is raised — confusing message

Similar Pattern Already Correct

The LLMStrategizeActor.__init__() in llm_actors.py correctly validates:

if not plan_id:
    raise ValidationError("plan_id must not be empty")

Fix Required

Add argument validation at the start of get_plan():

def get_plan(self, plan_id: str) -> Plan:
    """Get a plan by ID."""
    if not plan_id or not plan_id.strip():
        raise ValidationError("plan_id must not be empty")
    # ... rest of method

The same pattern should be applied to other service methods that accept plan_id:

start_strategize(plan_id: str)
fail_strategize(plan_id: str, error_message: str)
execute_plan(plan_id: str)
apply_plan(plan_id: str)
cancel_plan(plan_id: str)
rollback_plan(plan_id: str, checkpoint_id: str | None)

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

## Summary `src/cleveragents/application/services/plan_lifecycle_service.py` `get_plan()` method does not validate the `plan_id` argument before querying the database. Per CONTRIBUTING.md, all public methods must validate arguments as the first step (fail-fast behavior). An empty string or whitespace-only `plan_id` should raise `ValidationError` immediately, not `NotFoundError` after a database query. ## Code Location ```python # plan_lifecycle_service.py lines 1219-1247 def get_plan(self, plan_id: str) -> Plan: """Get a plan by ID.""" plan = self._plans.get(plan_id) # <-- No validation of plan_id if plan is not None: return plan # Try persistence layer if self._persisted and self.unit_of_work is not None: with self.unit_of_work.transaction() as ctx: persisted = ctx.lifecycle_plans.get(plan_id) # <-- DB query with empty string if persisted is not None: self._plans[plan_id] = persisted return persisted raise NotFoundError( resource_type="plan", resource_id=plan_id, # <-- Shows empty string in error message ) ``` ## Expected Behavior (per CONTRIBUTING.md) Per CONTRIBUTING.md: > **All public and protected class methods must validate arguments as the first guard.** Perform these checks before any other logic. > - **Empty Strings:** Reject empty strings where non-empty strings are required. > - **Null Checks:** Reject null values where not expected. When `plan_id` is empty or None: - Should raise `ValidationError("plan_id must not be empty")` immediately - Should NOT perform a database query - Should NOT raise `NotFoundError("Plan '' not found")` (confusing message) ## Actual Behavior When `plan_id` is empty string `""`: 1. `self._plans.get("")` returns None (no match) 2. A database query is executed with empty string 3. `NotFoundError("Plan '' not found")` is raised — confusing message ## Similar Pattern Already Correct The `LLMStrategizeActor.__init__()` in `llm_actors.py` correctly validates: ```python if not plan_id: raise ValidationError("plan_id must not be empty") ``` ## Fix Required Add argument validation at the start of `get_plan()`: ```python def get_plan(self, plan_id: str) -> Plan: """Get a plan by ID.""" if not plan_id or not plan_id.strip(): raise ValidationError("plan_id must not be empty") # ... rest of method ``` The same pattern should be applied to other service methods that accept `plan_id`: - `start_strategize(plan_id: str)` - `fail_strategize(plan_id: str, error_message: str)` - `execute_plan(plan_id: str)` - `apply_plan(plan_id: str)` - `cancel_plan(plan_id: str)` - `rollback_plan(plan_id: str, checkpoint_id: str | None)` --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: uat-tester

HAL9000 added the

labels

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

HAL9000 added this to the v3.2.0 milestone

2026-04-09 09:49:07 +00:00

HAL9000 commented

2026-04-09 09:52:26 +00:00

Author

Owner

Label compliance fix applied:

Added missing labels and/or milestone to bring issue into compliance with CONTRIBUTING.md

Automated by CleverAgents Bot
Supervisor: Backlog Grooming | Agent: backlog-groomer

Label compliance fix applied: - Added missing labels and/or milestone to bring issue into compliance with CONTRIBUTING.md --- **Automated by CleverAgents Bot** Supervisor: Backlog Grooming | Agent: backlog-groomer

HAL9000 added

and removed

labels

2026-04-09 09:52:48 +00:00

HAL9000 referenced this issue

2026-04-09 09:54:45 +00:00