diff --git a/CHANGELOG.md b/CHANGELOG.md index 257c5d474..5c0148268 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). from the TDD test so both scenarios run as normal regression guards. (#988) ### Fixed + +- **a2a `_handle_session_close` session_id validation bypass before devcontainer cleanup** (#11053): Moved the `session_id` validation guard to the top of `_handle_session_close()` in `A2aLocalFacade` so that empty or missing session IDs are rejected with `ValueError('session_id is required')` regardless of whether `SessionService` is wired. Previously, callers could bypass the guard by omitting `session_service` and passing an empty/null session ID through to `_cleanup_session_devcontainers()` without any error being raised. + - **TUI Prompt Symbol Mode Awareness** (#6431): The prompt widget now displays a mode-dependent symbol (`❯` normal, `/` command, `$` shell, `☰` multi-line), implemented via `_PromptSymbolMixin` and `InputMode.MULTILINE`. The widget uses @@ -222,25 +225,6 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). failure paths. Comprehensive BDD test coverage validates the fix under concurrent execution and confirms proper cleanup behavior. -- **Database resource types (PostgreSQL, SQLite) with transaction-based sandbox strategy** (#8608): - Implemented comprehensive database resource support enabling users to interact with - PostgreSQL and SQLite backends through a unified resource interface. Introduces - `DatabaseResourceHandler` providing full CRUD operations (`read`, `write`, `delete`, - `list_children`), connection validation with automatic credential masking via - :mod:`cleveragents.shared.redaction`, and transaction-based sandbox strategy using - BEGIN/COMMIT/ROLLBACK wrappers for safe, isolated database operations. SQLite-specific - checkpoint and rollback support with SAVEPOINT semantics. Support for multiple backends (PostgreSQL, SQLite, MySQL, DuckDB) via unified "DatabaseResourceHandler" and type-specific routing. BDD test - coverage in ``features/database_resources.feature`` (connection validation, CRUD workflows, - transaction/rollback behavior, error handling, credential masking verification) and - Robot Framework integration tests in ``robot/database_resources.robot``. - -- **TransactionSandbox infrastructure for database resource isolation** (#8608): - Implemented ``TransactionSandbox`` class with BEGIN/COMMIT/ROLLBACK lifecycle - management for transaction-based sandbox strategy. Wired into ``SandboxFactory`` - as the strategy resolver for database resource types. Added ``database`` resource type - registration in bootstrap builtin types and updated ``_resource_registry_data.py`` - to recognize database resource categories. - ### Fixed - **fix(repositories): derive PlanResult.success from result_success column instead of error_message** (#7501): diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index a1ddee27e..961ed6aa1 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -39,4 +39,4 @@ Below are some of the specific details of various contributions. * 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. -* HAL 9000 has contributed database resource types (PostgreSQL, SQLite) with transaction-based sandbox strategy: implemented ``DatabaseResourceHandler`` providing full CRUD operations (`read`, `write`, `delete`, `list_children`) and connection validation with automatic credential masking for PostgreSQL and SQLite backends. Includes ``TransactionSandbox`` infrastructure wired into ``SandboxFactory``, BDD test coverage in ``features/database_resources.feature``, and Robot Framework integration tests in ``robot/database_resources.robot`` (PR #10591 / issue #8608, Epic #8568). +* HAL 9000 has contributed the a2a session_id validation fix (PR #11053 / issue #9094): 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 eaee174d2..39a1774d9 100644 --- a/features/a2a_facade_coverage.feature +++ b/features/a2a_facade_coverage.feature @@ -19,11 +19,24 @@ 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) + # ------------------------------------------------------------------- + + 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" + + 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" + + 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/steps/a2a_facade_coverage_steps.py b/features/steps/a2a_facade_coverage_steps.py index d2413bd4b..1c8dd6233 100644 --- a/features/steps/a2a_facade_coverage_steps.py +++ b/features/steps/a2a_facade_coverage_steps.py @@ -22,6 +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 | +| 324 | _handle_session_close session_id validation guard | | 329-335 | session close with service (empty + valid session_id) | | 486-490 | event subscribe with wired queue | | 441-445 | registry list tools with service | @@ -297,6 +298,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/src/cleveragents/a2a/facade.py b/src/cleveragents/a2a/facade.py index 48f948f97..f82a2933d 100644 --- a/src/cleveragents/a2a/facade.py +++ b/src/cleveragents/a2a/facade.py @@ -321,6 +321,9 @@ class A2aLocalFacade: 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") + svc = self._session_service if svc is None: # R7-F4 fix: still run container cleanup even without a @@ -328,8 +331,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.