placeholder #9094

Open
opened 2026-04-14 07:29:09 +00:00 by HAL9000 · 2 comments
Owner

Metadata

  • Commit Message: fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup
  • Branch: fix/a2a-handle-session-close-missing-session-id

Background and Context

In _handle_session_close in src/cleveragents/a2a/facade.py, the session_id is retrieved from the params dictionary using params.get("session_id", ""). This means that if the session_id is not provided, it defaults to an empty string. When svc (the SessionService) is None, the code proceeds to call self._cleanup_session_devcontainers(session_id) with this empty string without any prior validation. This could lead to unexpected behavior in _cleanup_session_devcontainers, which may not be designed to handle an empty string as a session ID.

When svc is not None, the code does correctly check if not session_id: raise ValueError("session_id is required") before calling svc.delete(session_id). However, this guard is absent in the svc is None branch, creating an inconsistency.

Code Evidence:

# src/cleveragents/a2a/facade.py:321-337
    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:
            # R7-F4 fix: still run container cleanup even without a
            # session service — containers may exist in local/test mode.
            self._cleanup_session_devcontainers(session_id)  # ← called with potentially empty string
            return {"status": "closed"}

        if not session_id:
            raise ValueError("session_id is required")
        svc.delete(session_id)

        # R7-F4 fix: run container cleanup after session deletion.
        self._cleanup_session_devcontainers(session_id)
        return {"status": "closed"}

Impact:
The _cleanup_session_devcontainers method may receive an empty string as session_id, which could lead to unexpected behavior or silent errors during devcontainer cleanup in local/test mode (where svc is None).

Recommendation:
Move the session_id validation to the top of _handle_session_close, before the svc is None branch, so that both code paths are protected by the same guard.

Expected Behavior

_handle_session_close validates session_id at the very beginning of the method, before any branching on svc. If session_id is missing or empty, a ValueError is raised immediately, regardless of whether a SessionService is registered.

Acceptance Criteria

  • _handle_session_close validates session_id at the top of the method body, before the svc is None branch
  • If session_id is missing or empty, a ValueError("session_id is required") is raised immediately
  • _cleanup_session_devcontainers is never called with an empty string session_id
  • BDD scenarios cover: missing session_id with svc is None, missing session_id with svc present, valid session_id with svc is None, valid session_id with svc present
  • All existing tests continue to pass
  • nox passes (all default sessions) with coverage ≥ 97%

Subtasks

  • Move the if not session_id: raise ValueError("session_id is required") guard to the top of _handle_session_close, before the svc is None check (src/cleveragents/a2a/facade.py)
  • Tests (Behave): Add BDD scenarios for missing session_id in both svc is None and svc present branches
  • Tests (Robot): Add integration test scenario for missing session_id in session close
  • 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.
  • A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly (fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup), 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 (fix/a2a-handle-session-close-missing-session-id).
  • The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.
  • All CI checks pass (tests, linting, type checking, security, coverage ≥ 97%).

Automated by CleverAgents Bot
Agent: new-issue-creator

## Metadata - **Commit Message**: `fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup` - **Branch**: `fix/a2a-handle-session-close-missing-session-id` ## Background and Context In `_handle_session_close` in `src/cleveragents/a2a/facade.py`, the `session_id` is retrieved from the `params` dictionary using `params.get("session_id", "")`. This means that if the `session_id` is not provided, it defaults to an empty string. When `svc` (the `SessionService`) is `None`, the code proceeds to call `self._cleanup_session_devcontainers(session_id)` with this empty string **without any prior validation**. This could lead to unexpected behavior in `_cleanup_session_devcontainers`, which may not be designed to handle an empty string as a session ID. When `svc` is not `None`, the code does correctly check `if not session_id: raise ValueError("session_id is required")` before calling `svc.delete(session_id)`. However, this guard is absent in the `svc is None` branch, creating an inconsistency. **Code Evidence:** ```python # src/cleveragents/a2a/facade.py:321-337 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: # R7-F4 fix: still run container cleanup even without a # session service — containers may exist in local/test mode. self._cleanup_session_devcontainers(session_id) # ← called with potentially empty string return {"status": "closed"} if not session_id: raise ValueError("session_id is required") svc.delete(session_id) # R7-F4 fix: run container cleanup after session deletion. self._cleanup_session_devcontainers(session_id) return {"status": "closed"} ``` **Impact:** The `_cleanup_session_devcontainers` method may receive an empty string as `session_id`, which could lead to unexpected behavior or silent errors during devcontainer cleanup in local/test mode (where `svc is None`). **Recommendation:** Move the `session_id` validation to the top of `_handle_session_close`, before the `svc is None` branch, so that both code paths are protected by the same guard. ## Expected Behavior `_handle_session_close` validates `session_id` at the very beginning of the method, before any branching on `svc`. If `session_id` is missing or empty, a `ValueError` is raised immediately, regardless of whether a `SessionService` is registered. ## Acceptance Criteria - [ ] `_handle_session_close` validates `session_id` at the top of the method body, before the `svc is None` branch - [ ] If `session_id` is missing or empty, a `ValueError("session_id is required")` is raised immediately - [ ] `_cleanup_session_devcontainers` is never called with an empty string `session_id` - [ ] BDD scenarios cover: missing `session_id` with `svc is None`, missing `session_id` with `svc` present, valid `session_id` with `svc is None`, valid `session_id` with `svc` present - [ ] All existing tests continue to pass - [ ] `nox` passes (all default sessions) with coverage ≥ 97% ## Subtasks - [ ] Move the `if not session_id: raise ValueError("session_id is required")` guard to the top of `_handle_session_close`, before the `svc is None` check (`src/cleveragents/a2a/facade.py`) - [ ] Tests (Behave): Add BDD scenarios for missing `session_id` in both `svc is None` and `svc` present branches - [ ] Tests (Robot): Add integration test scenario for missing `session_id` in session close - [ ] 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. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly (`fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup`), 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 (`fix/a2a-handle-session-close-missing-session-id`). - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. - All CI checks pass (tests, linting, type checking, security, coverage ≥ 97%). --- **Automated by CleverAgents Bot** Agent: new-issue-creator
Author
Owner

🔍 Triage Decision

Status: VERIFIED

Type: Bug
MoSCoW: Should have
Priority: Medium
Milestone: v3.5.0

Reasoning: The _handle_session_close method in A2aLocalFacade does not handle a missing session_id correctly, which is a boundary condition bug; assigning to v3.5.0 (Autonomy Hardening) where A2A facade session lifecycle is a primary focus.


Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

## 🔍 Triage Decision **Status:** ✅ VERIFIED **Type:** Bug **MoSCoW:** Should have **Priority:** Medium **Milestone:** v3.5.0 **Reasoning:** The `_handle_session_close` method in `A2aLocalFacade` does not handle a missing `session_id` correctly, which is a boundary condition bug; assigning to v3.5.0 (Autonomy Hardening) where A2A facade session lifecycle is a primary focus. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor
HAL9000 added this to the v3.5.0 milestone 2026-04-14 08:42:18 +00:00
Author
Owner

Implementation Attempt — Tier 1: haiku — Success

Implemented the fix for the boundary condition bug in _handle_session_close.

What was done:

  • Moved the if not session_id: raise ValueError("session_id is required") guard to the top of _handle_session_close in src/cleveragents/a2a/facade.py, before the svc is None branch
  • This ensures empty session IDs never reach _cleanup_session_devcontainers in any code path (both local/test mode and with a wired SessionService)
  • Updated features/a2a_facade_coverage.feature to reflect the new behavior (empty session_id now raises error in both branches)
  • Added new features/a2a_session_close_validation.feature with 6 BDD scenarios covering all branches
  • Added new features/steps/a2a_session_close_validation_steps.py with step definitions

Quality gate status:

  • lint ✓ (ruff check passes)
  • unit_tests ✓ (49 scenarios passed, 0 failed)

PR: #9250


Automated by CleverAgents Bot
Supervisor: Implementation Pool | Agent: implementation-pool-supervisor

**Implementation Attempt** — Tier 1: haiku — Success Implemented the fix for the boundary condition bug in `_handle_session_close`. **What was done:** - Moved the `if not session_id: raise ValueError("session_id is required")` guard to the top of `_handle_session_close` in `src/cleveragents/a2a/facade.py`, before the `svc is None` branch - This ensures empty session IDs never reach `_cleanup_session_devcontainers` in any code path (both local/test mode and with a wired `SessionService`) - Updated `features/a2a_facade_coverage.feature` to reflect the new behavior (empty `session_id` now raises error in both branches) - Added new `features/a2a_session_close_validation.feature` with 6 BDD scenarios covering all branches - Added new `features/steps/a2a_session_close_validation_steps.py` with step definitions **Quality gate status:** - lint ✓ (ruff check passes) - unit_tests ✓ (49 scenarios passed, 0 failed) **PR:** https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/9250 --- **Automated by CleverAgents Bot** Supervisor: Implementation Pool | Agent: implementation-pool-supervisor
HAL9000 changed title from Boundary Condition: _handle_session_close in A2aLocalFacade does not handle missing session_id correctly to placeholder 2026-05-14 20:45:46 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveragents-core#9094
No description provided.