BUG-HUNT: [error-handling] _handle_session_close silently accepts empty session_id when session_service is None #6393

New issue

Open

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

HAL9000 commented

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

Owner

Bug Report: Error Handling — Inconsistent `session_id` validation in `_handle_session_close`

Severity Assessment

Impact: Callers that pass an empty session_id to session.close in test/local mode (where session_service is None) receive a {"status": "closed"} success response with no validation error. The same call with a real session_service raises ValueError("session_id is required"). This inconsistency can mask integration bugs where callers omit the session_id entirely.
Likelihood: Medium — local/test mode is the primary development path, so this silent-success path is the one exercised most frequently.
Priority: Medium (backlog)

Location

File: src/cleveragents/a2a/facade.py
Class: A2aLocalFacade
Function: _handle_session_close
Lines: ~234–255

Description

_handle_session_close has two separate branches for the case where session_service is None vs. present. In the svc is not None branch, a missing session_id raises ValueError. In the svc is None branch, the session_id check is completely absent:

def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]:
    session_id = params.get("session_id", "")

    svc = self._session_service
    if svc is None:
        # No session_id validation here ← BUG
        self._cleanup_session_devcontainers(session_id)
        return {"status": "closed"}

    if not session_id:
        raise ValueError("session_id is required")   # ← only validated when svc exists
    svc.delete(session_id)
    self._cleanup_session_devcontainers(session_id)
    return {"status": "closed"}

The _cleanup_session_devcontainers method guards against empty session_id with an early return (if not session_id: return), so no crash occurs — but the successful return value of {"status": "closed"} is misleading: no session was closed, no cleanup ran, yet the caller sees success.

Evidence

_handle_session_close — empty session_id path when svc is None (facade.py ~lines 234–255):

def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]:
    session_id = params.get("session_id", "")   # ← empty string if key absent

    svc = self._session_service
    if svc is None:
        self._cleanup_session_devcontainers(session_id)   # ← no-op for empty string
        return {"status": "closed"}    # ← FALSE success returned for empty session_id

    if not session_id:
        raise ValueError("session_id is required")   # ← raised only when svc is not None

_cleanup_session_devcontainers early exit (facade.py ~lines 257–273):

def _cleanup_session_devcontainers(self, session_id: str) -> None:
    if not session_id:
        return   # ← silently returns; no cleanup, no error

The net effect for session.close called without session_id in local/test mode:

session_id = ""
svc is None → enters stub branch
_cleanup_session_devcontainers("") → returns immediately (early guard)
Returns {"status": "closed"} — a silent false success

Expected Behavior

session_id should be validated before returning success regardless of whether a real session_service is available. An empty session_id should raise ValueError("session_id is required") in both branches, so callers in test/local mode receive the same contract enforcement as production callers.

Actual Behavior

When session_service is None (local/test mode), passing an empty or absent session_id silently returns {"status": "closed"} with no error, misleading callers into thinking the session was closed successfully.

Suggested Fix

Move the session_id validation before the svc is None branch check:

def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]:
    session_id = params.get("session_id", "")
    if not session_id:
        raise ValueError("session_id is required")   # validate first, always

    svc = self._session_service
    if svc is None:
        self._cleanup_session_devcontainers(session_id)
        return {"status": "closed"}

    svc.delete(session_id)
    self._cleanup_session_devcontainers(session_id)
    return {"status": "closed"}

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 — Inconsistent `session_id` validation in `_handle_session_close` ### Severity Assessment - **Impact**: Callers that pass an empty `session_id` to `session.close` in test/local mode (where `session_service` is `None`) receive a `{"status": "closed"}` success response with no validation error. The same call with a real `session_service` raises `ValueError("session_id is required")`. This inconsistency can mask integration bugs where callers omit the `session_id` entirely. - **Likelihood**: Medium — local/test mode is the primary development path, so this silent-success path is the one exercised most frequently. - **Priority**: Medium (backlog) ### Location - **File**: `src/cleveragents/a2a/facade.py` - **Class**: `A2aLocalFacade` - **Function**: `_handle_session_close` - **Lines**: ~234–255 ### Description `_handle_session_close` has two separate branches for the case where `session_service` is `None` vs. present. In the `svc is not None` branch, a missing `session_id` raises `ValueError`. In the `svc is None` branch, the `session_id` check is completely absent: ```python def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]: session_id = params.get("session_id", "") svc = self._session_service if svc is None: # No session_id validation here ← BUG self._cleanup_session_devcontainers(session_id) return {"status": "closed"} if not session_id: raise ValueError("session_id is required") # ← only validated when svc exists svc.delete(session_id) self._cleanup_session_devcontainers(session_id) return {"status": "closed"} ``` The `_cleanup_session_devcontainers` method guards against empty `session_id` with an early return (`if not session_id: return`), so no crash occurs — but the successful return value of `{"status": "closed"}` is misleading: no session was closed, no cleanup ran, yet the caller sees success. ### Evidence **`_handle_session_close` — empty `session_id` path when `svc is None`** (facade.py ~lines 234–255): ```python def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]: session_id = params.get("session_id", "") # ← empty string if key absent svc = self._session_service if svc is None: self._cleanup_session_devcontainers(session_id) # ← no-op for empty string return {"status": "closed"} # ← FALSE success returned for empty session_id if not session_id: raise ValueError("session_id is required") # ← raised only when svc is not None ``` **`_cleanup_session_devcontainers` early exit** (facade.py ~lines 257–273): ```python def _cleanup_session_devcontainers(self, session_id: str) -> None: if not session_id: return # ← silently returns; no cleanup, no error ``` The net effect for `session.close` called without `session_id` in local/test mode: 1. `session_id = ""` 2. `svc is None` → enters stub branch 3. `_cleanup_session_devcontainers("")` → returns immediately (early guard) 4. Returns `{"status": "closed"}` — a silent false success ### Expected Behavior `session_id` should be validated before returning success regardless of whether a real `session_service` is available. An empty `session_id` should raise `ValueError("session_id is required")` in both branches, so callers in test/local mode receive the same contract enforcement as production callers. ### Actual Behavior When `session_service` is `None` (local/test mode), passing an empty or absent `session_id` silently returns `{"status": "closed"}` with no error, misleading callers into thinking the session was closed successfully. ### Suggested Fix Move the `session_id` validation before the `svc is None` branch check: ```python def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]: session_id = params.get("session_id", "") if not session_id: raise ValueError("session_id is required") # validate first, always svc = self._session_service if svc is None: self._cleanup_session_devcontainers(session_id) return {"status": "closed"} svc.delete(session_id) self._cleanup_session_devcontainers(session_id) return {"status": "closed"} ``` ### 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:28 +00:00

HAL9000 added this to the v3.2.0 milestone

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

HAL9000 referenced this issue