diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a0d7a0c4..7616b475d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ Changed `wf10_batch.robot` to be less likely to create files, and ## [Unreleased] +- 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. + - Hardened the TDD bug-fix quality gate for issue #629: PR parsing now requires whole-word closing keywords (avoids false positives like "prefixes #12"), TDD bug tag discovery now uses exact token matching @@ -122,6 +124,8 @@ Changed `wf10_batch.robot` to be less likely to create files, and - **Plan Rollback Command** (#8557): Implemented `agents plan rollback []` 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 @@ -291,6 +295,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 @@ -402,6 +407,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 @@ -860,6 +866,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 @@ -973,6 +980,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 diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index c33eb0f9b..12023ed4b 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -39,6 +39,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. @@ -47,4 +48,5 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the PyYAML security upgrade (PR #11012 / issue #9055): added `pyyaml>=6.0.3` dependency constraint to address known YAML parsing vulnerabilities. * HAL 9000 has contributed the DecisionService wiring for PlanExecutor strategize persistence fix (#10813): added decision_service to the PlanExecutor constructor and wired it from the CLI dependency-injection container in `_get_plan_executor()`, plus implemented `_persist_strategy_decisions()` to persist strategy decisions as domain `Decision` objects. * HAL 9000 has contributed the A2A module rename standardization BDD tests (PR #10583 / issue #8615): comprehensive Behave test suite validating that all 22 A2A symbols are properly exported from `cleveragents.a2a`, no legacy ACP references remain in the module source, and documentation uses correct A2A naming conventions — fixing inline imports, unused behave symbols, cross-scenario context dependencies, and missing type annotations. +* 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 `ActorSelectionOverlay._render` → `_refresh_display` rename fix (PR #11176 / issue #11039, Epic #8174): renamed `_render()` method to `_refresh_display()` to avoid shadowing Textual's `Widget._render()`, fixing a crash in textual >=1.0 where `get_content_height()` would receive `None` and raise `AttributeError: 'NoneType' object has no attribute 'get_height'`. diff --git a/features/a2a_facade_coverage.feature b/features/a2a_facade_coverage.feature index eaee174d2..448d5d800 100644 --- a/features/a2a_facade_coverage.feature +++ b/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) @@ -308,6 +316,20 @@ 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" + @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": ""} 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" + # ------------------------------------------------------------------- # Event subscribe with wired queue (lines 486-490) # ------------------------------------------------------------------- diff --git a/features/steps/a2a_facade_coverage_steps.py b/features/steps/a2a_facade_coverage_steps.py index d2413bd4b..9058d4370 100644 --- a/features/steps/a2a_facade_coverage_steps.py +++ b/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 | @@ -233,6 +233,17 @@ def step_fc_dispatch(context: Context, operation: str, params_json: str) -> None context.fc_response = context.fc_facade.dispatch(request) +@when( + r'I dispatch facade-cov operation "(?P[^"]+)" ' + r'with params (?P.+) expecting error response' +) +def step_fc_dispatch_expect_error(context, operation, params_json): + """Dispatch and always capture the A2aResponse (errors are returned, not raised).""" + 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 try to dispatch a non-A2aRequest object via the facade-cov facade") def step_fc_dispatch_non_request(context: Context) -> None: context.fc_caught_error = None @@ -242,6 +253,22 @@ def step_fc_dispatch_non_request(context: Context) -> None: context.fc_caught_error = exc +@when( + r'I dispatch facade-cov operation "(?P[^"]+)" ' + r'with params (?P.+) 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[^"]+)" 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 +290,7 @@ def step_fc_register_empty_name(context: Context) -> None: @then(r'the facade-cov response status should be "(?P[^"]+)"') 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 +300,21 @@ def step_fc_response_status(context: Context, status: str) -> None: r'the facade-cov response data key "(?P[^"]+)" should equal "(?P[^"]+)"' ) 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[^"]+)" 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[^"]+)"') 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 +323,7 @@ def step_fc_data_has_key(context: Context, key: str) -> None: @then(r'the facade-cov response data should not contain key "(?P[^"]+)"') 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 +331,22 @@ 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 error message should contain (?P[^]+)") +def step_fc_error_message(context, msg): + assert context.fc_response is not None + assert context.fc_response.error is not None + error_text = str(context.fc_response.error) + assert msg in error_text, (f"Expected error message containing '{msg}', got '{error_text}'") + + @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 +371,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\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\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[^"]+)") +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") diff --git a/robot/helper_session_id_validation_integration.py b/robot/helper_session_id_validation_integration.py new file mode 100644 index 000000000..6c0843f8b --- /dev/null +++ b/robot/helper_session_id_validation_integration.py @@ -0,0 +1,86 @@ +"""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 + + +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 diff --git a/robot/session_id_validation_integration.robot b/robot/session_id_validation_integration.robot new file mode 100644 index 000000000..835b28474 --- /dev/null +++ b/robot/session_id_validation_integration.robot @@ -0,0 +1,55 @@ +*** 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). + +Library Collections +Library String +Library /tmp/fix-repo/robot/common_vars.py +Library /tmp/fix-repo/robot/helper_a2a_facade.py + +*** 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 Facade 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 Facade 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 Facade 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 Facade Operation session.close {"session_id": "SESS-WIRED-001"} + Response Status Should Be ok + Response Data Key Should Equal status closed diff --git a/src/cleveragents/a2a/facade.py b/src/cleveragents/a2a/facade.py index 43be9f4c2..632c1ba4f 100644 --- a/src/cleveragents/a2a/facade.py +++ b/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.