[AUTO-BUG-6] a2a/facade: _handle_plan_cancel, _handle_plan_tree, _handle_plan_artifacts silently return stub success for empty plan_id instead of raising ValueError #10418

New issue

Open

opened 2026-04-18 09:36:47 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-18 09:36:47 +00:00

Owner

Metadata

Field	Value
Branch	`fix/a2a-facade-plan-id-validation`
Commit Message	`fix(a2a/facade): raise ValueError for empty plan_id in cancel/tree/artifacts handlers to match other plan handlers`
Milestone	v3.5.0
Parent Epic	—

Background and Context

A2aLocalFacade in src/cleveragents/a2a/facade.py has inconsistent plan_id validation across its plan operation handlers. When a real plan_lifecycle_service is wired and plan_id is empty, some handlers raise ValueError("plan_id is required") while others silently return stub success responses — falsely claiming the operation succeeded.

Current Behavior

Handlers that correctly raise ValueError for empty plan_id:

_handle_plan_execute: if not plan_id: raise ValueError("plan_id is required")
_handle_plan_status: if not plan_id: raise ValueError("plan_id is required")
_handle_plan_diff: if not plan_id: raise ValueError("plan_id is required")
_handle_plan_apply: if not plan_id: raise ValueError("plan_id is required")

Handlers that silently return stub success for empty plan_id (BUG):

def _handle_plan_cancel(self, params: dict[str, Any]) -> dict[str, Any]:
    plan_id = params.get("plan_id", "")
    svc = self._plan_lifecycle_service
    if svc is None or not plan_id:  # ← BUG: returns stub even when svc is wired
        return {"plan_id": plan_id, "status": "cancelled", "stub": True}
    svc.cancel_plan(plan_id)
    return {"plan_id": plan_id, "status": "cancelled"}

def _handle_plan_tree(self, params: dict[str, Any]) -> dict[str, Any]:
    plan_id = params.get("plan_id", "")
    svc = self._plan_lifecycle_service
    if svc is None or not plan_id:  # ← BUG: same issue
        return {"plan_id": plan_id, "tree": [], "stub": True}
    ...

def _handle_plan_artifacts(self, params: dict[str, Any]) -> dict[str, Any]:
    plan_id = params.get("plan_id", "")
    svc = self._plan_lifecycle_service
    if svc is None or not plan_id:  # ← BUG: same issue
        return {"plan_id": plan_id, "artifacts": [], "stub": True}
    ...

When a service IS wired and plan_id is empty, these handlers return {"status": "cancelled", "stub": True} — falsely claiming the operation succeeded. This masks programming errors in callers and creates inconsistent API behavior.

Expected Behavior

All plan handlers should consistently raise ValueError("plan_id is required") when plan_id is empty and a service is wired. The stub response should only be returned when the service is absent (svc is None).

Correct pattern (matching _handle_plan_apply):

def _handle_plan_cancel(self, params: dict[str, Any]) -> dict[str, Any]:
    plan_id = params.get("plan_id", "")
    svc = self._plan_lifecycle_service
    if svc is None:
        return {"plan_id": plan_id, "status": "cancelled", "stub": True}
    if not plan_id:
        raise ValueError("plan_id is required")
    svc.cancel_plan(plan_id)
    return {"plan_id": plan_id, "status": "cancelled"}

Steps to Reproduce

from cleveragents.a2a.facade import A2aLocalFacade
from cleveragents.a2a.models import A2aRequest

# Wire a mock service
facade = A2aLocalFacade(services={"plan_lifecycle_service": mock_service})
request = A2aRequest(method="_cleveragents/plan/cancel", params={})  # no plan_id
response = facade.dispatch(request)
# BUG: response.result == {"plan_id": "", "status": "cancelled", "stub": True}
# Expected: response.error is set (ValueError mapped to error response)

Impact

Severity: Medium — silent incorrect behavior masks programming errors
Affected operations: _cleveragents/plan/cancel, _cleveragents/plan/tree, _cleveragents/plan/artifacts
Inconsistency: 4 handlers raise ValueError, 3 handlers silently succeed — same API, different behavior

Acceptance Criteria

_handle_plan_cancel raises ValueError("plan_id is required") when plan_id is empty and service is wired
_handle_plan_tree raises ValueError("plan_id is required") when plan_id is empty and service is wired
_handle_plan_artifacts raises ValueError("plan_id is required") when plan_id is empty and service is wired
All three handlers still return stub responses when service is absent (svc is None)
TDD scenarios from #10389 pass after the fix
All existing tests pass

Subtasks

Fix _handle_plan_cancel in src/cleveragents/a2a/facade.py
Fix _handle_plan_tree in src/cleveragents/a2a/facade.py
Fix _handle_plan_artifacts in src/cleveragents/a2a/facade.py
Verify TDD scenarios from #10389 pass
Run nox -s unit_tests to confirm no regressions
Verify coverage >= 97% via nox -s coverage_report

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.
The commit is pushed to the remote on the branch fix/a2a-facade-plan-id-validation.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

Blocked by #10389 (TDD counterpart must be merged first)

Automated by CleverAgents Bot
Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

## Metadata | Field | Value | |---|---| | **Branch** | `fix/a2a-facade-plan-id-validation` | | **Commit Message** | `fix(a2a/facade): raise ValueError for empty plan_id in cancel/tree/artifacts handlers to match other plan handlers` | | **Milestone** | v3.5.0 | | **Parent Epic** | — | ## Background and Context `A2aLocalFacade` in `src/cleveragents/a2a/facade.py` has inconsistent `plan_id` validation across its plan operation handlers. When a real `plan_lifecycle_service` is wired and `plan_id` is empty, some handlers raise `ValueError("plan_id is required")` while others silently return stub success responses — falsely claiming the operation succeeded. ## Current Behavior **Handlers that correctly raise ValueError for empty plan_id:** - `_handle_plan_execute`: `if not plan_id: raise ValueError("plan_id is required")` - `_handle_plan_status`: `if not plan_id: raise ValueError("plan_id is required")` - `_handle_plan_diff`: `if not plan_id: raise ValueError("plan_id is required")` - `_handle_plan_apply`: `if not plan_id: raise ValueError("plan_id is required")` **Handlers that silently return stub success for empty plan_id (BUG):** ```python def _handle_plan_cancel(self, params: dict[str, Any]) -> dict[str, Any]: plan_id = params.get("plan_id", "") svc = self._plan_lifecycle_service if svc is None or not plan_id: # ← BUG: returns stub even when svc is wired return {"plan_id": plan_id, "status": "cancelled", "stub": True} svc.cancel_plan(plan_id) return {"plan_id": plan_id, "status": "cancelled"} def _handle_plan_tree(self, params: dict[str, Any]) -> dict[str, Any]: plan_id = params.get("plan_id", "") svc = self._plan_lifecycle_service if svc is None or not plan_id: # ← BUG: same issue return {"plan_id": plan_id, "tree": [], "stub": True} ... def _handle_plan_artifacts(self, params: dict[str, Any]) -> dict[str, Any]: plan_id = params.get("plan_id", "") svc = self._plan_lifecycle_service if svc is None or not plan_id: # ← BUG: same issue return {"plan_id": plan_id, "artifacts": [], "stub": True} ... ``` When a service IS wired and `plan_id` is empty, these handlers return `{"status": "cancelled", "stub": True}` — falsely claiming the operation succeeded. This masks programming errors in callers and creates inconsistent API behavior. ## Expected Behavior All plan handlers should consistently raise `ValueError("plan_id is required")` when `plan_id` is empty and a service is wired. The stub response should only be returned when the service is absent (`svc is None`). **Correct pattern** (matching `_handle_plan_apply`): ```python def _handle_plan_cancel(self, params: dict[str, Any]) -> dict[str, Any]: plan_id = params.get("plan_id", "") svc = self._plan_lifecycle_service if svc is None: return {"plan_id": plan_id, "status": "cancelled", "stub": True} if not plan_id: raise ValueError("plan_id is required") svc.cancel_plan(plan_id) return {"plan_id": plan_id, "status": "cancelled"} ``` ## Steps to Reproduce ```python from cleveragents.a2a.facade import A2aLocalFacade from cleveragents.a2a.models import A2aRequest # Wire a mock service facade = A2aLocalFacade(services={"plan_lifecycle_service": mock_service}) request = A2aRequest(method="_cleveragents/plan/cancel", params={}) # no plan_id response = facade.dispatch(request) # BUG: response.result == {"plan_id": "", "status": "cancelled", "stub": True} # Expected: response.error is set (ValueError mapped to error response) ``` ## Impact - **Severity**: Medium — silent incorrect behavior masks programming errors - **Affected operations**: `_cleveragents/plan/cancel`, `_cleveragents/plan/tree`, `_cleveragents/plan/artifacts` - **Inconsistency**: 4 handlers raise ValueError, 3 handlers silently succeed — same API, different behavior ## Acceptance Criteria - [ ] `_handle_plan_cancel` raises `ValueError("plan_id is required")` when `plan_id` is empty and service is wired - [ ] `_handle_plan_tree` raises `ValueError("plan_id is required")` when `plan_id` is empty and service is wired - [ ] `_handle_plan_artifacts` raises `ValueError("plan_id is required")` when `plan_id` is empty and service is wired - [ ] All three handlers still return stub responses when service is absent (`svc is None`) - [ ] TDD scenarios from #10389 pass after the fix - [ ] All existing tests pass ## Subtasks - [ ] Fix `_handle_plan_cancel` in `src/cleveragents/a2a/facade.py` - [ ] Fix `_handle_plan_tree` in `src/cleveragents/a2a/facade.py` - [ ] Fix `_handle_plan_artifacts` in `src/cleveragents/a2a/facade.py` - [ ] Verify TDD scenarios from #10389 pass - [ ] Run `nox -s unit_tests` to confirm no regressions - [ ] Verify coverage >= 97% via `nox -s coverage_report` ## 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. - The commit is pushed to the remote on the branch `fix/a2a-facade-plan-id-validation`. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. **Blocked by #10389** (TDD counterpart must be merged first) --- Automated by CleverAgents Bot Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

HAL9000 added the

labels

2026-04-18 09:38:11 +00:00