cleveragents/cleveragents-core

Fork 3

UAT: A2aLocalFacade session.close stub silently succeeds without session_id when no service is wired — inconsistent with wired behavior #1838

New issue

Open

opened 2026-04-02 23:56:45 +00:00 by freemo · 1 comment

freemo commented

2026-04-02 23:56:45 +00:00

Owner

Metadata

Branch: fix/a2a-session-close-stub-validation
Commit Message: fix(a2a): validate session_id before service check in _handle_session_close
Milestone: v3.4.0
Parent Epic: #933

Bug Report

Feature Area: REST API endpoints / A2A Protocol — A2aLocalFacade
Severity: Low
Found by: UAT tester uat-tester-3993412-1775170781

What Was Tested

Verified that A2aLocalFacade.dispatch("session.close", {}) (without session_id) behaves consistently regardless of whether a session_service is wired.

Expected Behavior (from spec)

The session.close operation requires a session_id parameter. When session_id is absent, the operation should return an error response regardless of whether a service is wired. The BDD feature file features/a2a_facade_wiring.feature explicitly states:

Scenario: session.close without session_id returns error
  Given a wired A2aLocalFacade with a mock SessionService
  When I dispatch wired operation "session.close" with params {}
  Then the wired response status should be "error"

Actual Behavior

The behavior differs based on whether a session_service is wired:

With service wired (correct):

facade = A2aLocalFacade(services={'session_service': mock_service})
resp = facade.dispatch(A2aRequest(operation='session.close', params={}))
# status='error', error.code='INTERNAL_ERROR', message='session_id is required'

Without service wired (incorrect — silent success):

facade = A2aLocalFacade()  # no services
resp = facade.dispatch(A2aRequest(operation='session.close', params={}))
# status='ok', data={'status': 'closed'}  ← WRONG: should be an error

The stub path in _handle_session_close skips the session_id validation:

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
        self._cleanup_session_devcontainers(session_id)
        return {"status": "closed"}  # ← Returns ok even with empty session_id!

    if not session_id:
        raise ValueError("session_id is required")  # ← Only validates when service is wired

Steps to Reproduce

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

# No service wired
facade = A2aLocalFacade()
resp = facade.dispatch(A2aRequest(operation='session.close', params={}))
print(resp.status)  # 'ok' — should be 'error'
print(resp.data)    # {'status': 'closed'} — should be an error response

Code Location

src/cleveragents/a2a/facade.py, method _handle_session_close (lines ~200-220).

The fix should validate session_id before the svc is None 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

    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"}

Impact

Low — this only affects stub mode (no service wired), which is primarily used in testing. However, it creates inconsistent behavior that could mask bugs in tests that rely on the stub returning an error for missing session_id.

Subtasks

Move session_id validation before the svc is None guard in _handle_session_close
Verify the fix produces consistent error responses in both wired and unwired modes
Tests (Behave): Add/update scenario in features/a2a_facade_wiring.feature for unwired session.close without session_id
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.
A2aLocalFacade._handle_session_close validates session_id before checking whether a service is wired, producing a consistent error response in both modes when session_id is absent.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, 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.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.
All nox stages pass.
Coverage >= 97%.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

## Metadata - **Branch**: `fix/a2a-session-close-stub-validation` - **Commit Message**: `fix(a2a): validate session_id before service check in _handle_session_close` - **Milestone**: v3.4.0 - **Parent Epic**: #933 ## Bug Report **Feature Area**: REST API endpoints / A2A Protocol — A2aLocalFacade **Severity**: Low **Found by**: UAT tester uat-tester-3993412-1775170781 ### What Was Tested Verified that `A2aLocalFacade.dispatch("session.close", {})` (without `session_id`) behaves consistently regardless of whether a `session_service` is wired. ### Expected Behavior (from spec) The `session.close` operation requires a `session_id` parameter. When `session_id` is absent, the operation should return an error response regardless of whether a service is wired. The BDD feature file `features/a2a_facade_wiring.feature` explicitly states: ```gherkin Scenario: session.close without session_id returns error Given a wired A2aLocalFacade with a mock SessionService When I dispatch wired operation "session.close" with params {} Then the wired response status should be "error" ``` ### Actual Behavior The behavior differs based on whether a `session_service` is wired: **With service wired** (correct): ```python facade = A2aLocalFacade(services={'session_service': mock_service}) resp = facade.dispatch(A2aRequest(operation='session.close', params={})) # status='error', error.code='INTERNAL_ERROR', message='session_id is required' ``` **Without service wired** (incorrect — silent success): ```python facade = A2aLocalFacade() # no services resp = facade.dispatch(A2aRequest(operation='session.close', params={})) # status='ok', data={'status': 'closed'} ← WRONG: should be an error ``` The stub path in `_handle_session_close` skips the `session_id` validation: ```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: # R7-F4 fix: still run container cleanup even without a session service self._cleanup_session_devcontainers(session_id) return {"status": "closed"} # ← Returns ok even with empty session_id! if not session_id: raise ValueError("session_id is required") # ← Only validates when service is wired ``` ### Steps to Reproduce ```python from cleveragents.a2a.facade import A2aLocalFacade from cleveragents.a2a.models import A2aRequest # No service wired facade = A2aLocalFacade() resp = facade.dispatch(A2aRequest(operation='session.close', params={})) print(resp.status) # 'ok' — should be 'error' print(resp.data) # {'status': 'closed'} — should be an error response ``` ### Code Location `src/cleveragents/a2a/facade.py`, method `_handle_session_close` (lines ~200-220). The fix should validate `session_id` before the `svc is None` 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 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"} ``` ### Impact Low — this only affects stub mode (no service wired), which is primarily used in testing. However, it creates inconsistent behavior that could mask bugs in tests that rely on the stub returning an error for missing `session_id`. ## Subtasks - [ ] Move `session_id` validation before the `svc is None` guard in `_handle_session_close` - [ ] Verify the fix produces consistent error responses in both wired and unwired modes - [ ] Tests (Behave): Add/update scenario in `features/a2a_facade_wiring.feature` for unwired `session.close` without `session_id` - [ ] 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. - `A2aLocalFacade._handle_session_close` validates `session_id` before checking whether a service is wired, producing a consistent error response in both modes when `session_id` is absent. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, 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. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. - All nox stages pass. - Coverage >= 97%. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo added this to the v3.4.0 milestone

2026-04-02 23:57:11 +00:00

freemo referenced this issue

2026-04-02 23:58:16 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 461 — v3.7.0 #1850

freemo added a new dependency

2026-04-02 23:59:35 +00:00

#933 Epic: A2A Protocol Compliance — JSON-RPC 2.0 Framing, Standard Operations, and Extension Methods

freemo commented

2026-04-03 00:04:59 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
MoSCoW: MoSCoW/Should Have — bug or error handling improvement.

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

Issue triaged by project owner: - **State**: Verified - **MoSCoW**: MoSCoW/Should Have — bug or error handling improvement. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo referenced this issue

2026-04-03 00:08:26 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 471 — v3.7.0 #1891