diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 9d758483c..8b384231f 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -130,3 +130,4 @@ Below are some specific details of individual PR contributions. * HAL 9000 has contributed the PyYAML security hardening fix (PR #11017 / issue #11012): added `pyyaml>=6.0.3` as an explicit runtime dependency in `pyproject.toml` to mitigate CVE-2025-8045, replacing the previous implicit transitive-only dependency chain that left YAML config loading vulnerable to silent supply-chain breakage from upstream dependency changes. * HAL 9000 has contributed the plan explain structured alternatives format fix (PR #11090): updated `_build_explain_dict()` in `src/cleveragents/cli/commands/plan.py` to convert the `alternatives_considered` list into structured objects with `index` (1-based), `description`, and `chosen` fields in the `alternatives` output key, aligning the `agents plan explain` output with the spec-required format. * HAL 9000 has contributed the plan tree JSON/YAML spec-compliant envelope fix (issue #11041): wrapped `agents plan tree` JSON and YAML output in the spec-required command envelope (`command`, `status`, `exit_code`, `data`, `timing`, `messages`), updated BDD step definitions to validate envelope structure, and removed the `@tdd_expected_fail` tag from the previously-failing JSON tree format test (issue #4254). +* HAL 9000 has contributed the a2a session_id validation fix (PR #11098 / issue #9250): moved the session_id validation guard to the top of `_handle_session_close()` in `A2aLocalFacade`, closing the validation bypass path where empty or null session IDs could slip through to devcontainer cleanup when `SessionService` was not wired. diff --git a/features/a2a_facade_coverage.feature b/features/a2a_facade_coverage.feature index 6609bf92f..9ca9287ae 100644 --- a/features/a2a_facade_coverage.feature +++ b/features/a2a_facade_coverage.feature @@ -20,11 +20,27 @@ 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 + # ------------------------------------------------------------------- + # Session close — session_id validation guard (lines 321-324) + # ------------------------------------------------------------------- + + @tdd_issue @tdd_issue_9250 + Scenario: Session close with empty session_id and no service raises ValueError 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" + Then the facade-cov response status should be "error" + + @tdd_issue @tdd_issue_9250 + Scenario: Session close with missing session_id key and no service raises ValueError error + Given a facade-cov facade with no services + When I dispatch facade-cov operation "session.close" with params {} + Then the facade-cov response status should be "error" + + @tdd_issue @tdd_issue_9250 + Scenario: Session close with empty session_id and wired service raises ValueError error + Given a facade-cov facade with a mock SessionService + When I dispatch facade-cov operation "session.close" with params {"session_id": ""} + Then the facade-cov response status should be "error" # ------------------------------------------------------------------- # Plan cancel — with service wired (lines 501-502) diff --git a/features/a2a_facade_wiring.feature b/features/a2a_facade_wiring.feature index cf27d1eac..c2ee8f207 100644 --- a/features/a2a_facade_wiring.feature +++ b/features/a2a_facade_wiring.feature @@ -34,6 +34,15 @@ Feature: A2A local facade wiring to live services When I dispatch wired operation "session.close" with params {} Then the wired response status should be "error" + # --------------------------------------------------------------- + # Validate session_id at entry — no service scenario (PR #9250) + # --------------------------------------------------------------- + + Scenario: session.close without session_id and no service returns error + Given a wired A2aLocalFacade with no services + When I dispatch wired operation "session.close" with params {"session_id": ""} + Then the wired response status should be "error" + # --------------------------------------------------------------- # Plan lifecycle wiring # --------------------------------------------------------------- diff --git a/features/consolidated_misc.feature b/features/consolidated_misc.feature index e8e21a0a5..575371f6a 100644 --- a/features/consolidated_misc.feature +++ b/features/consolidated_misc.feature @@ -42,7 +42,7 @@ Feature: Consolidated Misc Scenario: Dispatch session.close returns status closed Given a new A2aLocalFacade with no services - When I dispatch operation "session.close" with params {} + When I dispatch operation "session.close" with params {"session_id": "01M6SM0KESESS0N00000000000"} Then the response status should be "ok" And response data key "status" equals "closed" @@ -563,7 +563,7 @@ Feature: Consolidated Misc Scenario: M6 smoke A2A session close returns closed status Given a m6 smoke test runner And a m6 smoke A2A local facade - When I m6 smoke dispatch "session.close" with params {} + When I m6 smoke dispatch "session.close" with params {"session_id": "01M6SM0KESESS0N00000000000"} Then the m6 smoke response status should be "ok" And the m6 smoke response data "status" should equal "closed" diff --git a/features/m6_autonomy_acceptance.feature b/features/m6_autonomy_acceptance.feature index d2a832996..ae38e7698 100644 --- a/features/m6_autonomy_acceptance.feature +++ b/features/m6_autonomy_acceptance.feature @@ -35,7 +35,7 @@ Feature: M6 autonomy acceptance smoke tests And the m6 smoke response data should contain key "status" Scenario: M6 smoke A2A session close returns closed status - When I m6 smoke dispatch "session.close" with params {} + When I m6 smoke dispatch "session.close" with params {"session_id": "01M6SM0KESESS0N00000000000"} Then the m6 smoke response status should be "ok" And the m6 smoke response data "status" should equal "closed" diff --git a/features/steps/a2a_facade_coverage_steps.py b/features/steps/a2a_facade_coverage_steps.py index d2413bd4b..3a0c7d100 100644 --- a/features/steps/a2a_facade_coverage_steps.py +++ b/features/steps/a2a_facade_coverage_steps.py @@ -297,6 +297,14 @@ def step_fc_data_no_key(context: Context, key: str) -> None: ) +@then(r"the facade-cov response error should contain 'session_id is required'") +def step_fc_error_contains_session_id(context: Context) -> None: + assert context.fc_response.error is not None, "Expected an error in the response" + assert "session_id is required" in context.fc_response.error.message, ( + f"Expected error containing 'session_id is required', got: {context.fc_response.error.message}" + ) + + @then(r"the facade-cov response error should not be None") def step_fc_error_not_none(context: Context) -> None: assert context.fc_response.error is not None, "Expected an error in the response" diff --git a/robot/helper_m6_autonomy_acceptance.py b/robot/helper_m6_autonomy_acceptance.py index a0239448c..63627b72c 100644 --- a/robot/helper_m6_autonomy_acceptance.py +++ b/robot/helper_m6_autonomy_acceptance.py @@ -48,8 +48,11 @@ def facade_session() -> None: resp_create = facade.dispatch(A2aRequest(method="session.create", params={})) assert resp_create.result is not None, f"Expected ok, got {resp_create.status}" assert "session_id" in resp_create.result + session_id = resp_create.result["session_id"] - resp_close = facade.dispatch(A2aRequest(method="session.close", params={})) + resp_close = facade.dispatch( + A2aRequest(method="session.close", params={"session_id": session_id}) + ) assert resp_close.result is not None assert resp_close.result["status"] == "closed" diff --git a/src/cleveragents/a2a/facade.py b/src/cleveragents/a2a/facade.py index fcafae1b1..f5d27ce56 100644 --- a/src/cleveragents/a2a/facade.py +++ b/src/cleveragents/a2a/facade.py @@ -364,6 +364,13 @@ class A2aLocalFacade: def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]: session_id = params.get("session_id", "") + # Validate session_id before any cleanup or service operations. + # This prevents best-effort devcontainer cleanup from running on + # an invalid or omitted session identifier, which could trigger + # incorrect container lifecycle operations on wrong/unknown sessions. + 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 @@ -371,8 +378,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. @@ -503,10 +508,9 @@ class A2aLocalFacade: """Best-effort stop of devcontainers associated with a session. Failures are logged but never propagated so session close always - succeeds. + succeeds. ``session_id`` is guaranteed non-empty by the entry + validation in :meth:`_handle_session_close`. """ - if not session_id: - return try: from cleveragents.application.services.cleanup_service import ( CleanupService,