fix: resolve PR review blockers - syntax error and hardcoded paths

Move the session_id validation check to the very top of _handle_session_close in A2aLocalFacade so that callers cannot bypass input validation by omitting the session_service dependency.

Key changes:
- Core fix: Moved if not session_id check above service null check in facade.py
- BDD tests: Added error response scenarios for empty and missing session_id
- Step definitions: Rewrote dispatch step to properly handle error responses
- Robot Framework: Added integration test for session_id validation
- Metadata: Updated CHANGELOG.md issue ref from #9250 to #9094, updated CONTRIBUTORS.md

ISSUES CLOSED: #9094

CHANGELOG.md

@@ -96,6 +96,8 @@ Changed `wf10_batch.robot` to be less likely to create files, and
 - **Plan Rollback Command** (#8557): Implemented `agents plan rollback <plan-id> [<checkpoint-id>]` for checkpoint-based plan state restoration in Epic #8493. The command restores a plan's sandbox to the state captured at a given checkpoint, discarding all decisions made after that checkpoint. The checkpoint can be specified as an optional positional second argument or via the `--to-checkpoint` named option. Supports `--yes/-y` flag to skip confirmation prompts and `--format/-f` for output format selection (rich/plain/json/yaml). Included with comprehensive BDD test coverage (>= 97%) and spec-aligned output formatting showing rollback summary, changes reverted, impact analysis, and post-rollback state panels.
 
 ### Fixed
+- **a2a `_handle_session_close` validates `session_id` before devcontainer cleanup** (#9094): Moved the `session_id` validation check to the very top of `_handle_session_close` in `src/cleveragents/a2a/facade.py`. Previously, when no `session_service` was wired, the handler skipped directly to `_cleanup_session_devcontainers(session_id)` without checking whether `session_id` was non-empty. This allowed callers to bypass input validation by omitting the service dependency. The fix ensures `ValueError("session_id is required")` is raised regardless of service availability, closing a validation bypass path while preserving all devcontainer cleanup and deletion behaviour.
+
 - **Guard cleanup_stale against execute/processing and execute/complete plans** (#11121):
   ``_create_sandbox_for_plan()`` in ``src/cleveragents/cli/commands/plan.py`` now
   skips ``GitWorktreeSandbox.cleanup_stale()`` when the plan is in
@@ -251,6 +253,7 @@ Changed `wf10_batch.robot` to be less likely to create files, and
 
 ### Fixed
 
+
 - **Error suppression removed from `reactive_registry_adapter.py`** (#9060): Removed two `try...except Exception:` blocks in `register_registry_agents()` that silently suppressed errors, violating the CONTRIBUTING.md fail-fast policy. Exceptions from `actor_registry.list_actors()` and the route bridge refresh now propagate to the caller instead of being swallowed. Added Behave scenarios verifying RuntimeError, AttributeError, and TypeError propagation.
 
 - **Implementation Supervisor PR Compliance Checklist** (#9824): Added a mandatory
@@ -362,6 +365,7 @@ _ALL_DATA_COLUMNS + ") " "SELECT " + _ALL_DATA_COLUMNS + " FROM v3_plans"`.
 
 ### Fixed
 
+
 - **`invariant_enforced` decisions not propagated to child plans on subplan spawn** (#9131):
   Fixed `SubplanService.spawn()` to propagate all `invariant_enforced` decisions from the
   parent plan's decision tree to each child plan's decision tree. Previously, child plans
@@ -820,6 +824,7 @@ Documentation Report (Cycle N)` issues every 10 cycles (~3.3 hours). The manager
 
 ### Fixed
 
+
 - **Plan Concurrency Race Condition** (#7989): Fixed critical race condition in `execute_plan()` and
   `apply_plan()` where concurrent CLI/worker sessions could simultaneously modify the same plan,
   corrupting plan state. `LockService` is now wired into the plan lifecycle with plan-level advisory
@@ -933,6 +938,7 @@ iteration` and data corruption under concurrent plan execution. All public
 
 ### Fixed
 
+
 - **CLI (`agents actor remove`)** (#6491): Restores output parity with the
   other actor commands by honoring `--format`/`-f` for JSON/YAML/plain/Rich
   envelopes. Adds a Robot Framework regression test to assert the JSON

CONTRIBUTORS.md

@@ -36,6 +36,7 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration.
 * HAL 9000 has contributed the LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch.
 * HAL 9000 has contributed the ACMS Index Data Model and File Traversal Engine (PR #9664 / issue #9579): foundational data structures for indexed context entries with hot/warm/cold/archive storage tier classification, tag system, and a timeout-safe chunked file traversal engine for large projects with 10,000+ files.
+* HAL 9000 has contributed the A2A session close `session_id` validation fix (PR #11053 / issue #9094): moved input validation to the entry of `_handle_session_close` so that empty/null session IDs are rejected before any devcontainer cleanup or service operations, preventing callers from bypassing validation by omitting the `session_service` dependency. Added BDD coverage for empty and missing session_id error paths with Robot Framework integration tests.
 * HAL 9000 has contributed the error-suppression removal fix (PR #9247 / issue #9060): removed both `try...except Exception:` blocks in `register_registry_agents()` that silently suppressed errors from `actor_registry.list_actors()` and the route bridge refresh, enabling exceptions to propagate per CONTRIBUTING.md fail-fast policy. Added three Behave scenarios verifying RuntimeError, AttributeError, and TypeError propagation.
 * HAL 9000 has contributed the Strategize phase full context snapshot fix (issue #9056): added `_build_strategize_context_snapshot()` helper to `PlanLifecycleService`, updated `_try_record_decision()` to accept and forward a `ContextSnapshot` parameter, and added BDD test coverage verifying all four `ContextSnapshot` fields (`hot_context_hash`, `hot_context_ref`, `actor_state_ref`, `relevant_resources`) are populated during the Strategize phase.
 * HAL 9000 has contributed the ACMS context path matching fix (PR #10975 / issue #10972): corrects `_path_matches()` and `_matches_pattern()` to properly match absolute fragment paths against relative glob patterns by auto-prefixing with `**/` before calling `PurePath.full_match()`, preventing silent inefficacy of include/exclude filters for absolute paths in fragment metadata.

features/a2a_facade_coverage.feature

@@ -19,11 +19,19 @@ Feature: A2A local facade coverage — uncovered handler and edge-case paths
     Then the facade-cov response status should be "ok"
     And the facade-cov response data key "status" should equal "closed"
 
-  Scenario: Session close with empty session_id and no service
+  @tdd_issue @tdd_issue_9094
+  Scenario: Session close with empty session_id and no service raises validation error
     Given a facade-cov facade with no services
-    When I dispatch facade-cov operation "session.close" with params {"session_id": ""}
-    Then the facade-cov response status should be "ok"
-    And the facade-cov response data key "status" should equal "closed"
+    When I dispatch facade-cov operation "session.close" with params {"session_id": ""} expecting error response
+    Then the facade-cov response status should be "error"
+    And the facade-cov response error message should contain "session_id is required"
+
+  @tdd_issue @tdd_issue_9094
+  Scenario: Session close with missing session_id key and no service raises validation error
+    Given a facade-cov facade with no services
+    When I dispatch facade-cov operation "session.close" with params {} expecting error response
+    Then the facade-cov response status should be "error"
+    And the facade-cov response error message should contain "session_id is required"
 
   # -------------------------------------------------------------------
   # Plan cancel — with service wired (lines 501-502)

features/steps/a2a_facade_coverage_steps.py

@@ -22,7 +22,7 @@ Targets uncovered lines in ``src/cleveragents/a2a/facade.py``:
 | 169-170   | dispatch TypeError for non-request                         |
 | 212-220   | register_service + cache invalidation                      |
 | 315-317   | session create with service                                |
-| 329-335   | session close with service (empty + valid session_id)      |
+| 329-341   | session close — all validation/error paths                 |
 | 486-490   | event subscribe with wired queue                           |
 | 441-445   | registry list tools with service                           |
 | 451-462   | registry list resources with service                       |
@@ -242,6 +242,22 @@ def step_fc_dispatch_non_request(context: Context) -> None:
         context.fc_caught_error = exc
 
 
+@when(
+    r'I dispatch facade-cov operation "(?P<operation>[^"]+)" '
+    r'with params (?P<params_json>.+) expecting error response'
+)
+def step_fc_dispatch_expect_error(context: Context, operation: str, params_json: str) -> None:
+    """Dispatch and always capture the A2aResponse (errors are returned, not raised).
+
+    This is the robust variant used by session-id validation scenarios that expect
+    an error response rather than a caught exception — _dispatch catches internal
+    exceptions and converts them to error responses.
+    """
+    params: dict[str, Any] = json.loads(params_json)
+    request = A2aRequest(method=operation, params=params)
+    context.fc_response = context.fc_facade.dispatch(request)
+
+
 @when(r'I register a facade-cov service "(?P<name>[^"]+)" with a mock')
 def step_fc_register_service(context: Context, name: str) -> None:
     context.fc_facade.register_service(name, _build_mock_session_service())
@@ -263,6 +279,7 @@ def step_fc_register_empty_name(context: Context) -> None:
 
 @then(r'the facade-cov response status should be "(?P<status>[^"]+)"')
 def step_fc_response_status(context: Context, status: str) -> None:
+    assert context.fc_response is not None, "fc_response was not set before status check"
     assert (context.fc_response.error is None) == (status == "ok"), (
         f"Expected status '{status}', got error={context.fc_response.error}"
     )
@@ -272,18 +289,21 @@ def step_fc_response_status(context: Context, status: str) -> None:
     r'the facade-cov response data key "(?P<key>[^"]+)" should equal "(?P<value>[^"]+)"'
 )
 def step_fc_data_key_equals(context: Context, key: str, value: str) -> None:
+    assert context.fc_response is not None, "fc_response was not set before data check"
     actual = context.fc_response.result.get(key)
     assert str(actual) == value, f"Expected data['{key}'] = '{value}', got '{actual}'"
 
 
 @then(r'the facade-cov response data key "(?P<key>[^"]+)" should be true')
 def step_fc_data_key_true(context: Context, key: str) -> None:
+    assert context.fc_response is not None, "fc_response was not set before data check"
     actual = context.fc_response.result.get(key)
     assert actual is True, f"Expected data['{key}'] to be True, got {actual!r}"
 
 
 @then(r'the facade-cov response data should contain key "(?P<key>[^"]+)"')
 def step_fc_data_has_key(context: Context, key: str) -> None:
+    assert context.fc_response is not None, "fc_response was not set before data check"
     assert key in context.fc_response.result, (
         f"Expected key '{key}' in response data, "
         f"got: {list(context.fc_response.result.keys())}"
@@ -292,6 +312,7 @@ def step_fc_data_has_key(context: Context, key: str) -> None:
 
 @then(r'the facade-cov response data should not contain key "(?P<key>[^"]+)"')
 def step_fc_data_no_key(context: Context, key: str) -> None:
+    assert context.fc_response is not None, "fc_response was not set before check"
     assert key not in context.fc_response.result, (
         f"Expected key '{key}' not in response data, but found it"
     )
@@ -299,12 +320,14 @@ def step_fc_data_no_key(context: Context, key: str) -> None:
 
 @then(r"the facade-cov response error should not be None")
 def step_fc_error_not_none(context: Context) -> None:
+    assert context.fc_response is not None, "fc_response was not set before error check"
     assert context.fc_response.error is not None, "Expected an error in the response"
 
 
 @then(r"the facade-cov response should have timing_ms set")
 def step_fc_timing_set(context: Context) -> None:
     # timing_ms removed from wire format per JSON-RPC 2.0 compliance
+    assert context.fc_response is not None, "fc_response was not set"
     assert context.fc_response.result is not None, "Expected result to be set"
 
 
@@ -329,23 +352,38 @@ def step_fc_value_error_raised(context: Context) -> None:
 def step_fc_session_create_used_service(context: Context) -> None:
     # After registering a new service and dispatching again,
     # the response should reflect the mock service return value
+    assert context.fc_response is not None
     assert context.fc_response.error is None
     assert context.fc_response.result["session_id"] == "MOCK-SESSION-001"
 
 
 @then(r"the facade-cov response tools list should have (?P<count>\d+) items")
 def step_fc_tools_count(context: Context, count: str) -> None:
+    assert context.fc_response is not None
     tools = context.fc_response.result.get("tools", [])
     assert len(tools) == int(count), f"Expected {count} tools, got {len(tools)}"
 
 
 @then(r"the facade-cov response resources list should have (?P<count>\d+) items")
 def step_fc_resources_count(context: Context, count: str) -> None:
+    assert context.fc_response is not None
     resources = context.fc_response.result.get("resources", [])
     assert len(resources) == int(count), (
         f"Expected {count} resources, got {len(resources)}"
     )
 
 
+@then(r'the facade-cov response error message should contain (?P<msg>[^"]+)')
+def step_fc_error_message(context: Context, msg: str) -> None:
+    """Assert that the error response includes a message matching `msg`."""
+    assert context.fc_response is not None
+    assert context.fc_response.error is not None, "Expected an error in the response"
+    # The A2aResponse wraps exceptions; check repr for the expected message.
+    error_text = str(context.fc_response.error)
+    assert msg in error_text, (
+        f"Expected error message containing '{msg}', got '{error_text}'"
+    )
+
+
 # Reset step matcher to parse (default) so subsequent step files are not affected
 use_step_matcher("parse")

robot/helper_session_id_validation_integration.py

@@ -0,0 +1,186 @@
+"""Pybot helper for session_id_validation_integration.robot.
+
+Provides keyword-compatible functions that the Robot Framework test can call
+to interact with A2aLocalFacade dispatch and verify responses.
+"""
+
+from __future__ import annotations
+
+import json
+from typing import Any
+
+# ---------------------------------------------------------------------------
+# Module-level facade context, shared across keywords within a test case.
+# ---------------------------------------------------------------------------
+_current_facade: Any = None
+
+
+def _get_facade() -> Any:
+    """Return the current facade stored by ``A2a_local_facade_should_have_no_services``."""
+    return _current_facade
+
+
+def build_a2a_local_facade(services: dict[str, Any] | None = None):
+    """Build an A2aLocalFacade (lazily imported)."""
+    from cleveragents.a2a.facade import A2aLocalFacade
+
+    if services is None:
+        return A2aLocalFacade()
+    return A2aLocalFacade(services=services)
+
+
+def build_mock_session_service():
+    """Build a minimal mock SessionService."""
+    from unittest.mock import MagicMock
+
+    class _MockSession:
+        def __init__(self):
+            self.session_id = "MOCK-SESSION-001"
+
+    svc = MagicMock()
+    svc.create.return_value = _MockSession()
+    svc.delete.return_value = None
+    return svc
+
+
+def dispatch_facade_operation(facade, operation: str, params_json: str) -> Any:
+    """Dispatch an A2A operation and return the response object."""
+    from cleveragents.a2a.models import A2aRequest
+
+    params: dict[str, Any] = json.loads(params_json)
+    request = A2aRequest(method=operation, params=params)
+    return facade.dispatch(request)
+
+
+class _ResponseAssertions:
+    """Mixin for validating A2aLocalFacade dispatch responses."""
+
+    def __init__(self, response):
+        self.response = response
+
+    def response_status_should_be(self, expected: str) -> None:
+        assert (self.response.error is None) == (expected == "ok"), (
+            f"Expected status '{expected}', got error={self.response.error}"
+        )
+
+    def response_error_should_not_be_none(self) -> None:
+        assert self.response.error is not None, "Expected an error in the response"
+
+    def response_error_message_should_contain(self, text: str) -> None:
+        assert self.response.error is not None, "Expected an error in the response"
+        error_text = str(self.response.error)
+        assert text in error_text, (
+            f"Expected error message containing '{text}', got '{error_text}'"
+        )
+
+    def response_data_key_should_equal(self, key: str, value: str) -> None:
+        actual = self.response.result.get(key)
+        assert str(actual) == value, (
+            f"Expected data['{key}'] = '{value}', got '{actual!r}'"
+        )
+
+
+def assert_response(response, expected_status: str = "ok", **kwargs: Any) -> dict[str, Any]:
+    """Convenience function for Robot to validate responses.
+
+    Returns a dict that Robot can use with 'Should Be Equal As Strings'.
+    """
+    assertions = _ResponseAssertions(response)
+    assertions.response_status_should_be(expected_status)
+
+    result: dict[str, Any] = {"error": str(response.error) if response.error else "None"}
+    if expected_status == "ok" and response.result:
+        for key, value in response.result.items():
+            result[f"data.{key}"] = str(value)
+
+    return result
+
+
+# ---------------------------------------------------------------------------
+# Robot-friendly wrapper keywords needed by session_id_validation_integration.robot
+# ---------------------------------------------------------------------------
+
+# Module-level state shared across keywords within a test case.
+_robocop_facade: Any = None
+_last_dispatched_response: Any = None
+
+
+def a2a_local_facade_should_have_no_services() -> Any:
+    """Create and store a fresh A2aLocalFacade with no services wired.
+
+    This keyword is used before each test case to ensure the test has a clean
+    facade instance (no SessionService, no other dependencies) so that
+    validation of input arguments — rather than service lookup — produces
+    the expected error responses.
+
+    Returns nothing; subsequent keywords read from module-level state.
+    """
+    global _robocop_facade, _last_dispatched_response, _current_facade
+    from cleveragents.a2a.facade import A2aLocalFacade
+
+    _robocop_facade = A2aLocalFacade()
+    _current_facade = _robocop_facade
+    _last_dispatched_response = None
+
+
+def dispatch_operation(operation: str, params_json: str) -> Any:
+    """Dispatch an A2A operation using the facade set up by ``A2a_local_facade_should_have_no_services``.
+
+    Called from Robot as ``Dispatch Operation``.  Reads the current facade
+    from module-level state, dispatches, stores the response for assertion
+    keywords, and returns it (captured in Robot as ${response}).
+    """
+    global _last_dispatched_response
+    resp = dispatch_facade_operation(_robocop_facade, operation, params_json)
+    _last_dispatched_response = resp
+    return resp
+
+
+def a2a_local_facade_with_mock_session_service() -> None:
+    """Create and store a facade with a mock SessionService wired in.
+
+    Used before the happy-path test that verifies closing a valid session
+    still works when all expected services are configured.
+    """
+    global _robocop_facade, _last_dispatched_response
+    svc = build_mock_session_service()
+    from cleveragents.a2a.facade import A2aLocalFacade
+
+    _robocop_facade = A2aLocalFacade(services={"session": svc})
+    _last_dispatched_response = None
+
+
+def response_status_should_be(expected: str) -> None:
+    """Assert the response status matches *expected* (``ok`` or ``error``)."""
+    assert _last_dispatched_response is not None, (
+        "No dispatched response — call Dispatch Facade Operation first."
+    )
+    assertions = _ResponseAssertions(_last_dispatched_response)
+    assertions.response_status_should_be(expected)
+
+
+def response_error_should_not_be_none() -> None:
+    """Assert the last dispatched response contains an error."""
+    assert _last_dispatched_response is not None, (
+        "No dispatched response — call Dispatch Facade Operation first."
+    )
+    assertions = _ResponseAssertions(_last_dispatched_response)
+    assertions.response_error_should_not_be_none()
+
+
+def response_error_message_should_contain(text: str) -> None:
+    """Assert the error message in the last dispatch contains *text*."""
+    assert _last_dispatched_response is not None, (
+        "No dispatched response — call Dispatch Facade Operation first."
+    )
+    assertions = _ResponseAssertions(_last_dispatched_response)
+    assertions.response_error_message_should_contain(text)
+
+
+def response_data_key_should_equal(key: str, value: str) -> None:
+    """Assert data['key'] equals *value* in the last dispatch response."""
+    assert _last_dispatched_response is not None, (
+        "No dispatched response — call Dispatch Facade Operation first."
+    )
+    assertions = _ResponseAssertions(_last_dispatched_response)
+    assertions.response_data_key_should_equal(key, value)

robot/session_id_validation_integration.robot

@@ -0,0 +1,58 @@
+*** Settings ***
+Documentation    Robot Framework integration tests for A2A session close session_id validation.
+
+    Verifies that _handle_session_close validates session_id BEFORE any service checks
+    or devcontainer cleanup, preventing callers from bypassing input validation by
+    omitting the session_service dependency (issue #9094).
+
+Resource    ${CURDIR}/common.resource
+Library    Collections
+Library    String
+Library    ${CURDIR}/helper_session_id_validation_integration.py
+
+Suite Setup       Setup Test Environment With Database Isolation
+Suite Teardown    Cleanup Test Environment
+
+*** Variables ***
+${SESSION_ID_VALIDATION_TIMEOUT}=    30
+
+*** Test Cases ***
+A2A Session Close With Empty Session ID Returns Error Response
+    [Documentation]
+        Verifies that closing a session with an empty string session_id returns
+        an error response, because validation happens at entry before any
+        service checks.
+    A2a Local Facade Should Have No Services
+    ${response}=    Dispatch Operation    session.close    {"session_id": ""}
+    Response Status Should Be    error
+    Response Error Should Not Be None
+    Response Error Message Should Contain    session_id is required
+
+A2A Session Close With Missing Session ID Returns Error Response
+    [Documentation]
+        Verifies that closing a session without providing a session_id key at all
+        also returns an error response with the same validation message.
+    A2a Local Facade Should Have No Services
+    ${response}=    Dispatch Operation    session.close    {}
+    Response Status Should Be    error
+    Response Error Should Not Be None
+    Response Error Message Should Contain    session_id is required
+
+A2A Session Close With Valid Session ID Succeeds Without Service
+    [Documentation]
+        Verifies that with a valid (non-empty) session_id the close operation
+        succeeds even without a wired SessionService, because the validation
+        guard at entry passes and cleanup runs as best-effort.
+    A2a Local Facade Should Have No Services
+    ${response}=    Dispatch Operation    session.close    {"session_id": "orphan-test-sess-001"}
+    Response Status Should Be    ok
+    Response Data Key Should Equal    status    closed
+
+A2A Session Close With Valid Session ID Succeeds With Service
+    [Documentation]
+        Verifies that the normal happy path — valid session_id with a wired
+        SessionService — still works correctly after the validation fix.
+    A2a Local Facade With Mock Session Service
+    ${response}=    Dispatch Operation    session.close    {"session_id": "SESS-WIRED-001"}
+    Response Status Should Be    ok
+    Response Data Key Should Equal    status    closed

src/cleveragents/a2a/facade.py

@@ -354,6 +354,12 @@ class A2aLocalFacade:
     def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]:
         session_id = params.get("session_id", "")
 
+        # Validate session_id at entry — before any service checks or
+        # devcontainer cleanup — so callers cannot bypass validation by
+        # omitting the session_service dependency.
+        if not session_id:
+            raise ValueError("session_id is required")
+
         svc = self._session_service
         if svc is None:
             # R7-F4 fix: still run container cleanup even without a
@@ -361,8 +367,6 @@ class A2aLocalFacade:
             self._cleanup_session_devcontainers(session_id)
             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.