UAT: DecisionService.mark_superseded() missing fail-fast validation for decision_id parameter #3741

New issue

Open

opened 2026-04-05 22:24:56 +00:00 by freemo · 0 comments

freemo commented

2026-04-05 22:24:56 +00:00

Owner

Metadata

Branch: fix/decision-service-mark-superseded-validation
Commit Message: fix(services): add fail-fast validation for decision_id in DecisionService.mark_superseded()
Milestone: (none — backlog)
Parent Epic: #394

Background

The specification (CONTRIBUTING.md) requires: "All public and protected methods must validate their arguments as the first step of execution (fail-fast). This includes checks for value ranges, nulls, expected types, and empty strings/collections."

DecisionService.mark_superseded() in src/cleveragents/application/services/decision_service.py validates that the new_decision_id (replacement decision) exists, but does not validate that the decision_id parameter itself is non-empty before attempting to look it up. This is an asymmetric validation gap.

Affected File

src/cleveragents/application/services/decision_service.py

Code Location

DecisionService.mark_superseded() (lines ~760-820):

def mark_superseded(self, decision_id: str, new_decision_id: str) -> Decision:
    """Mark a decision as superseded by another."""
    # Validate the replacement decision exists.
    replacement = self._decisions.get(new_decision_id)
    if replacement is None and self._persisted and self.unit_of_work is not None:
        with self.unit_of_work.transaction() as ctx:
            replacement = ctx.decisions.get(new_decision_id)
    if replacement is None:
        raise DecisionNotFoundError(new_decision_id)
    ...

Notice: new_decision_id is validated (existence check), but decision_id has no empty-string check before use. Passing "" or " " as decision_id will silently proceed to the lookup, returning None and raising DecisionNotFoundError with an empty ID — a confusing error message that doesn't indicate the root cause.

Steps to Reproduce

Instantiate DecisionService()
Record a decision to get a valid new_decision_id
Call service.mark_superseded("", new_decision_id) — raises DecisionNotFoundError("") instead of ValidationError
Call service.mark_superseded(" ", new_decision_id) — same confusing error

Expected Behaviour

mark_superseded() should raise ValidationError("decision_id must not be empty") immediately when decision_id is empty or whitespace-only, before any lookup is attempted.

Actual Behaviour

No empty-string validation for decision_id. The method proceeds to look up "" in the decisions dict, finds nothing, and raises DecisionNotFoundError("") — a misleading error that suggests the decision doesn't exist rather than that the input was invalid.

Subtasks

Add fail-fast validation for decision_id at the start of mark_superseded() (before the replacement lookup)
Add Behave scenario verifying ValidationError is raised for empty decision_id
Verify nox -e unit_tests passes

Definition of Done

mark_superseded() raises ValidationError for empty/whitespace decision_id
Behave test covers the validation case
nox -e unit_tests passes
PR merged

Backlog note: This issue was discovered during autonomous operation
on milestone v3.3.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/decision-service-mark-superseded-validation` - **Commit Message**: `fix(services): add fail-fast validation for decision_id in DecisionService.mark_superseded()` - **Milestone**: *(none — backlog)* - **Parent Epic**: #394 ## Background The specification (CONTRIBUTING.md) requires: *"All public and protected methods must validate their arguments as the first step of execution (fail-fast). This includes checks for value ranges, nulls, expected types, and empty strings/collections."* `DecisionService.mark_superseded()` in `src/cleveragents/application/services/decision_service.py` validates that the `new_decision_id` (replacement decision) exists, but does **not** validate that the `decision_id` parameter itself is non-empty before attempting to look it up. This is an asymmetric validation gap. ## Affected File - `src/cleveragents/application/services/decision_service.py` ## Code Location `DecisionService.mark_superseded()` (lines ~760-820): ```python def mark_superseded(self, decision_id: str, new_decision_id: str) -> Decision: """Mark a decision as superseded by another.""" # Validate the replacement decision exists. replacement = self._decisions.get(new_decision_id) if replacement is None and self._persisted and self.unit_of_work is not None: with self.unit_of_work.transaction() as ctx: replacement = ctx.decisions.get(new_decision_id) if replacement is None: raise DecisionNotFoundError(new_decision_id) ... ``` Notice: `new_decision_id` is validated (existence check), but `decision_id` has no empty-string check before use. Passing `""` or `" "` as `decision_id` will silently proceed to the lookup, returning `None` and raising `DecisionNotFoundError` with an empty ID — a confusing error message that doesn't indicate the root cause. ## Steps to Reproduce 1. Instantiate `DecisionService()` 2. Record a decision to get a valid `new_decision_id` 3. Call `service.mark_superseded("", new_decision_id)` — raises `DecisionNotFoundError("")` instead of `ValidationError` 4. Call `service.mark_superseded(" ", new_decision_id)` — same confusing error ## Expected Behaviour `mark_superseded()` should raise `ValidationError("decision_id must not be empty")` immediately when `decision_id` is empty or whitespace-only, before any lookup is attempted. ## Actual Behaviour No empty-string validation for `decision_id`. The method proceeds to look up `""` in the decisions dict, finds nothing, and raises `DecisionNotFoundError("")` — a misleading error that suggests the decision doesn't exist rather than that the input was invalid. ## Subtasks - [ ] Add fail-fast validation for `decision_id` at the start of `mark_superseded()` (before the replacement lookup) - [ ] Add Behave scenario verifying `ValidationError` is raised for empty `decision_id` - [ ] Verify `nox -e unit_tests` passes ## Definition of Done - `mark_superseded()` raises `ValidationError` for empty/whitespace `decision_id` - Behave test covers the validation case - `nox -e unit_tests` passes - PR merged > **Backlog note:** This issue was discovered during autonomous operation > on milestone v3.3.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-05 22:25:02 +00:00

#394 Epic: Decision Framework

freemo referenced this issue

2026-04-05 22:29:07 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:29:26 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:33:37 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654