From ccf06b25f3b05bc1e6f2300e54c0d00da4530bf3 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Tue, 31 Mar 2026 11:23:45 +0000 Subject: [PATCH] fix(tests): update resource_dag.robot SQLite pool and cycle detection types Replaced direct create_engine usage with StaticPool-based connection in resource_dag.robot to prevent SQLite connection sharing issues in tests. Updated cycle detection test to use distinct resource types (git-checkout and fs-directory) instead of the same type for both resources, improving test coverage of cross-type cycle detection. Split from PR #1204 per reviewer request for atomic commits. ISSUES CLOSED: #1226 --- CHANGELOG.md | 14 +++ .../steps/a2a_facade_coverage_boost_steps.py | 2 +- features/steps/a2a_facade_coverage_steps.py | 2 +- features/steps/a2a_facade_steps.py | 2 +- .../steps/a2a_jsonrpc_wire_format_steps.py | 8 +- robot/helper_a2a_jsonrpc_wire_format.py | 111 +++++++++--------- robot/resource_dag.robot | 19 +-- src/cleveragents/a2a/models.py | 4 +- .../application/services/session_service.py | 9 +- src/cleveragents/cli/commands/session.py | 18 ++- src/cleveragents/cli/commands/tool.py | 8 +- vulture_whitelist.py | 3 + 12 files changed, 108 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56c8332623..909fae03bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- Fixed session leak in all `AutomationProfileRepository` public methods + (`get_by_name()`, `list_all()`, `upsert()`, and `delete()`): added + `finally: if self._auto_commit: session.close()` blocks matching the + pattern already used by `SessionRepository`, preventing database sessions + from being leaked when `auto_commit` mode is enabled. (#987) + - `agents session list` rich output now includes a **Name** column and a **Summary** panel showing total sessions, most recent, oldest, total messages, and storage usage. JSON output also includes a `summary` section with the same statistics. (#1574, #1570) @@ -79,6 +85,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `ThoughtBlockWidget` background corrected from `$primary 20%` to `$primary-muted 20%`, making thought blocks visually lighter and more subtle per spec §29811. (#1448) +- Updated `resource_dag.robot` to use `StaticPool`-based SQLite connections + (`poolclass=StaticPool`, `connect_args={"check_same_thread": False}`) in + all three test cases, preventing connection sharing issues in the test + environment. Updated cycle detection test to use distinct resource types + (`robot/cycle-a` and `robot/cycle-b`) instead of a single shared type, + improving coverage of cross-type cycle detection. Split from PR #1204 + per reviewer request for atomic commits. (#1226) + ## [3.7.0] — 2026-04-02 ### Added diff --git a/features/steps/a2a_facade_coverage_boost_steps.py b/features/steps/a2a_facade_coverage_boost_steps.py index c785435d05..7b7e2341bc 100644 --- a/features/steps/a2a_facade_coverage_boost_steps.py +++ b/features/steps/a2a_facade_coverage_boost_steps.py @@ -110,7 +110,7 @@ def step_cb_type_error_raised(context: Context, msg: str) -> None: @then(r'the coverage-boost response status should be "(?P[^"]+)"') def step_cb_response_status(context: Context, status: str) -> None: - assert ((context.cb_response.error is None) == (status == 'ok')), ( + assert (context.cb_response.error is None) == (status == "ok"), ( f"Expected status '{status}', got error={context.cb_response.error}" ) diff --git a/features/steps/a2a_facade_coverage_steps.py b/features/steps/a2a_facade_coverage_steps.py index 38c01a5b72..4669bc3f45 100644 --- a/features/steps/a2a_facade_coverage_steps.py +++ b/features/steps/a2a_facade_coverage_steps.py @@ -263,7 +263,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.error is None) == (status == 'ok')), ( + assert (context.fc_response.error is None) == (status == "ok"), ( f"Expected status '{status}', got error={context.fc_response.error}" ) diff --git a/features/steps/a2a_facade_steps.py b/features/steps/a2a_facade_steps.py index 40347becd2..27fb17bba7 100644 --- a/features/steps/a2a_facade_steps.py +++ b/features/steps/a2a_facade_steps.py @@ -93,7 +93,7 @@ def step_dispatch_operation(context: Context, operation: str, params_json: str) @then(r'the response status should be "(?P[^"]+)"') def step_response_status(context: Context, status: str) -> None: is_ok = context.response.error is None - expected_ok = (status == "ok") + expected_ok = status == "ok" assert is_ok == expected_ok, ( f"Expected status '{status}', got error={context.response.error}" ) diff --git a/features/steps/a2a_jsonrpc_wire_format_steps.py b/features/steps/a2a_jsonrpc_wire_format_steps.py index f8633da2b1..7e7b88642a 100644 --- a/features/steps/a2a_jsonrpc_wire_format_steps.py +++ b/features/steps/a2a_jsonrpc_wire_format_steps.py @@ -252,9 +252,7 @@ def step_response_error_none(context: Context) -> None: @given( r'a JSON-RPC 2.0 error response dict with id "(?P[^"]+)" and error code "(?P[^"]+)"' ) -def step_jsonrpc_error_response_dict( - context: Context, resp_id: str, code: str -) -> None: +def step_jsonrpc_error_response_dict(context: Context, resp_id: str, code: str) -> None: context.raw_dict = { "jsonrpc": "2.0", "id": resp_id, @@ -340,9 +338,7 @@ def step_wire_response_error_none(context: Context) -> None: @then("the wire-format response error should not be None") def step_wire_response_error_not_none(context: Context) -> None: - assert context.wire_response.error is not None, ( - "Expected error to be set, got None" - ) + assert context.wire_response.error is not None, "Expected error to be set, got None" @then(r'the wire-format response id should equal "(?P[^"]+)"') diff --git a/robot/helper_a2a_jsonrpc_wire_format.py b/robot/helper_a2a_jsonrpc_wire_format.py index 958c6c6a14..440d85f4b1 100644 --- a/robot/helper_a2a_jsonrpc_wire_format.py +++ b/robot/helper_a2a_jsonrpc_wire_format.py @@ -40,18 +40,18 @@ def request_wire_format() -> None: for field in _REQUIRED_REQUEST_FIELDS: if field not in wire: print( - f"FAIL: missing required field '{field}' in request wire format", - file=sys.stderr, - ) + f"FAIL: missing required field '{field}' in request wire format", + file=sys.stderr, + ) print(f" Got fields: {list(wire.keys())}", file=sys.stderr) sys.exit(1) # Check jsonrpc value if wire["jsonrpc"] != "2.0": print( - f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", - file=sys.stderr, - ) + f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", + file=sys.stderr, + ) sys.exit(1) # Check method value @@ -63,9 +63,9 @@ def request_wire_format() -> None: for field in _BANNED_REQUEST_FIELDS: if field in wire: print( - f"FAIL: non-standard field '{field}' present in request wire format", - file=sys.stderr, - ) + f"FAIL: non-standard field '{field}' present in request wire format", + file=sys.stderr, + ) sys.exit(1) print("a2a-request-wire-format-ok") @@ -80,18 +80,19 @@ def response_success_wire_format() -> None: for field in _REQUIRED_SUCCESS_RESPONSE_FIELDS: if field not in wire: print( - f"FAIL: missing required field '{field}' in success response wire format", - file=sys.stderr, - ) + f"FAIL: missing required field '{field}'" + " in success response wire format", + file=sys.stderr, + ) print(f" Got fields: {list(wire.keys())}", file=sys.stderr) sys.exit(1) # Check jsonrpc value if wire["jsonrpc"] != "2.0": print( - f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", - file=sys.stderr, - ) + f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", + file=sys.stderr, + ) sys.exit(1) # Check id value @@ -108,17 +109,18 @@ def response_success_wire_format() -> None: for field in _BANNED_RESPONSE_FIELDS: if field in wire: print( - f"FAIL: non-standard field '{field}' present in success response wire format", - file=sys.stderr, - ) + f"FAIL: non-standard field '{field}'" + " present in success response wire format", + file=sys.stderr, + ) sys.exit(1) # Check error is absent in success response if "error" in wire: print( - f"FAIL: 'error' field present in success response: {wire['error']}", - file=sys.stderr, - ) + f"FAIL: 'error' field present in success response: {wire['error']}", + file=sys.stderr, + ) sys.exit(1) print("a2a-response-success-wire-format-ok") @@ -136,18 +138,18 @@ def response_error_wire_format() -> None: for field in _REQUIRED_ERROR_RESPONSE_FIELDS: if field not in wire: print( - f"FAIL: missing required field '{field}' in error response wire format", - file=sys.stderr, - ) + f"FAIL: missing required field '{field}' in error response wire format", + file=sys.stderr, + ) print(f" Got fields: {list(wire.keys())}", file=sys.stderr) sys.exit(1) # Check jsonrpc value if wire["jsonrpc"] != "2.0": print( - f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", - file=sys.stderr, - ) + f"FAIL: jsonrpc should be '2.0', got '{wire['jsonrpc']}'", + file=sys.stderr, + ) sys.exit(1) # Check error structure @@ -160,17 +162,18 @@ def response_error_wire_format() -> None: for field in _BANNED_RESPONSE_FIELDS: if field in wire: print( - f"FAIL: non-standard field '{field}' present in error response wire format", - file=sys.stderr, - ) + f"FAIL: non-standard field '{field}'" + " present in error response wire format", + file=sys.stderr, + ) sys.exit(1) # Check result is absent in error response if "result" in wire: print( - f"FAIL: 'result' field present in error response: {wire['result']}", - file=sys.stderr, - ) + f"FAIL: 'result' field present in error response: {wire['result']}", + file=sys.stderr, + ) sys.exit(1) print("a2a-response-error-wire-format-ok") @@ -185,33 +188,33 @@ def facade_dispatch_jsonrpc() -> None: # Check jsonrpc field if resp.jsonrpc != "2.0": print( - f"FAIL: response jsonrpc should be '2.0', got '{resp.jsonrpc}'", - file=sys.stderr, - ) + f"FAIL: response jsonrpc should be '2.0', got '{resp.jsonrpc}'", + file=sys.stderr, + ) sys.exit(1) # Check id matches request if resp.id != "test-req-001": print( - f"FAIL: response id should be 'test-req-001', got '{resp.id}'", - file=sys.stderr, - ) + f"FAIL: response id should be 'test-req-001', got '{resp.id}'", + file=sys.stderr, + ) sys.exit(1) # Check result is set (success path) if resp.result is None: print( - f"FAIL: response result should be set, got None. Error: {resp.error}", - file=sys.stderr, - ) + f"FAIL: response result should be set, got None. Error: {resp.error}", + file=sys.stderr, + ) sys.exit(1) # Check error is None (success path) if resp.error is not None: print( - f"FAIL: response error should be None, got: {resp.error}", - file=sys.stderr, - ) + f"FAIL: response error should be None, got: {resp.error}", + file=sys.stderr, + ) sys.exit(1) # Verify no non-standard fields on the model @@ -219,9 +222,9 @@ def facade_dispatch_jsonrpc() -> None: for field in _BANNED_RESPONSE_FIELDS: if field in wire: print( - f"FAIL: non-standard field '{field}' in facade response wire format", - file=sys.stderr, - ) + f"FAIL: non-standard field '{field}' in facade response wire format", + file=sys.stderr, + ) sys.exit(1) print("a2a-facade-dispatch-jsonrpc-ok") @@ -295,9 +298,9 @@ def request_rejects_old_fields() -> None: for attr in old_attrs: if hasattr(req, attr): print( - f"FAIL: A2aRequest still has old attribute '{attr}'", - file=sys.stderr, - ) + f"FAIL: A2aRequest still has old attribute '{attr}'", + file=sys.stderr, + ) sys.exit(1) # Verify new attributes exist @@ -305,9 +308,9 @@ def request_rejects_old_fields() -> None: for attr in new_attrs: if not hasattr(req, attr): print( - f"FAIL: A2aRequest missing new attribute '{attr}'", - file=sys.stderr, - ) + f"FAIL: A2aRequest missing new attribute '{attr}'", + file=sys.stderr, + ) sys.exit(1) print("a2a-request-rejects-old-fields-ok") diff --git a/robot/resource_dag.robot b/robot/resource_dag.robot index 6d121dac34..f15c0ff440 100644 --- a/robot/resource_dag.robot +++ b/robot/resource_dag.robot @@ -14,11 +14,12 @@ Link Child And Verify Tree ... from datetime import datetime, UTC ... from sqlalchemy import create_engine, event ... from sqlalchemy.orm import sessionmaker + ... from sqlalchemy.pool import StaticPool ... from cleveragents.infrastructure.database.models import Base ... from cleveragents.infrastructure.database.repositories import ResourceTypeRepository, ResourceRepository ... from cleveragents.domain.models.core.resource_type import ResourceTypeSpec, ResourceKind, SandboxStrategy ... from cleveragents.domain.models.core.resource import Resource, PhysVirt, ResourceCapabilities - ... engine = create_engine("sqlite:///:memory:") + ... engine = create_engine("sqlite:///:memory:", poolclass=StaticPool, connect_args={"check_same_thread": False}) ... @event.listens_for(engine, "connect") ... def _fk(conn, _): conn.cursor().execute("PRAGMA foreign_keys=ON") ... Base.metadata.create_all(engine) @@ -50,11 +51,12 @@ Cycle Detection Rejects A To B To A ... from datetime import datetime, UTC ... from sqlalchemy import create_engine, event ... from sqlalchemy.orm import sessionmaker + ... from sqlalchemy.pool import StaticPool ... from cleveragents.infrastructure.database.models import Base ... from cleveragents.infrastructure.database.repositories import ResourceTypeRepository, ResourceRepository, CycleDetectedError ... from cleveragents.domain.models.core.resource_type import ResourceTypeSpec, ResourceKind, SandboxStrategy ... from cleveragents.domain.models.core.resource import Resource, PhysVirt, ResourceCapabilities - ... engine = create_engine("sqlite:///:memory:") + ... engine = create_engine("sqlite:///:memory:", poolclass=StaticPool, connect_args={"check_same_thread": False}) ... @event.listens_for(engine, "connect") ... def _fk(conn, _): conn.cursor().execute("PRAGMA foreign_keys=ON") ... Base.metadata.create_all(engine) @@ -62,10 +64,12 @@ Cycle Detection Rejects A To B To A ... shared_session = factory() ... rt_repo = ResourceTypeRepository(lambda: shared_session) ... res_repo = ResourceRepository(lambda: shared_session) - ... spec = ResourceTypeSpec(name="robot/cycle-type", description="Cycle", resource_kind=ResourceKind.PHYSICAL, sandbox_strategy=SandboxStrategy.NONE, user_addable=True, cli_args=[], parent_types=[], child_types=["robot/cycle-type"], auto_discovery=None, equivalence=None, handler=None, capabilities={"read": True, "write": True, "sandbox": True, "checkpoint": False}, built_in=False) - ... rt_repo.create(spec) - ... a = Resource(resource_id="01HDAGCYC000000000000000A1", name=None, resource_type_name="robot/cycle-type", classification=PhysVirt.PHYSICAL, properties={}, location=None, capabilities=ResourceCapabilities(), created_at=datetime.now(tz=UTC), updated_at=datetime.now(tz=UTC)) - ... b = Resource(resource_id="01HDAGCYC000000000000000B1", name=None, resource_type_name="robot/cycle-type", classification=PhysVirt.PHYSICAL, properties={}, location=None, capabilities=ResourceCapabilities(), created_at=datetime.now(tz=UTC), updated_at=datetime.now(tz=UTC)) + ... spec_a = ResourceTypeSpec(name="robot/cycle-a", description="Cycle A", resource_kind=ResourceKind.PHYSICAL, sandbox_strategy=SandboxStrategy.NONE, user_addable=True, cli_args=[], parent_types=[], child_types=["robot/cycle-b"], auto_discovery=None, equivalence=None, handler=None, capabilities={"read": True, "write": True, "sandbox": True, "checkpoint": False}, built_in=False) + ... spec_b = ResourceTypeSpec(name="robot/cycle-b", description="Cycle B", resource_kind=ResourceKind.PHYSICAL, sandbox_strategy=SandboxStrategy.NONE, user_addable=True, cli_args=[], parent_types=[], child_types=["robot/cycle-a"], auto_discovery=None, equivalence=None, handler=None, capabilities={"read": True, "write": True, "sandbox": True, "checkpoint": False}, built_in=False) + ... rt_repo.create(spec_a) + ... rt_repo.create(spec_b) + ... a = Resource(resource_id="01HDAGCYC000000000000000A1", name=None, resource_type_name="robot/cycle-a", classification=PhysVirt.PHYSICAL, properties={}, location=None, capabilities=ResourceCapabilities(), created_at=datetime.now(tz=UTC), updated_at=datetime.now(tz=UTC)) + ... b = Resource(resource_id="01HDAGCYC000000000000000B1", name=None, resource_type_name="robot/cycle-b", classification=PhysVirt.PHYSICAL, properties={}, location=None, capabilities=ResourceCapabilities(), created_at=datetime.now(tz=UTC), updated_at=datetime.now(tz=UTC)) ... res_repo.create(a) ... res_repo.create(b) ... res_repo.link_child("01HDAGCYC000000000000000A1", "01HDAGCYC000000000000000B1") @@ -87,11 +91,12 @@ Auto Discover Children ... from datetime import datetime, UTC ... from sqlalchemy import create_engine, event ... from sqlalchemy.orm import sessionmaker + ... from sqlalchemy.pool import StaticPool ... from cleveragents.infrastructure.database.models import Base ... from cleveragents.infrastructure.database.repositories import ResourceTypeRepository, ResourceRepository ... from cleveragents.domain.models.core.resource_type import ResourceTypeSpec, ResourceKind, SandboxStrategy ... from cleveragents.domain.models.core.resource import Resource, PhysVirt, ResourceCapabilities - ... engine = create_engine("sqlite:///:memory:") + ... engine = create_engine("sqlite:///:memory:", poolclass=StaticPool, connect_args={"check_same_thread": False}) ... @event.listens_for(engine, "connect") ... def _fk(conn, _): conn.cursor().execute("PRAGMA foreign_keys=ON") ... Base.metadata.create_all(engine) diff --git a/src/cleveragents/a2a/models.py b/src/cleveragents/a2a/models.py index f799b33df3..2d9c26fc72 100644 --- a/src/cleveragents/a2a/models.py +++ b/src/cleveragents/a2a/models.py @@ -152,9 +152,7 @@ class A2aResponse(BaseModel): if self.result is None and self.error is None: raise ValueError("A2aResponse must have either 'result' or 'error'") if self.result is not None and self.error is not None: - raise ValueError( - "A2aResponse must not have both 'result' and 'error'" - ) + raise ValueError("A2aResponse must not have both 'result' and 'error'") return self diff --git a/src/cleveragents/application/services/session_service.py b/src/cleveragents/application/services/session_service.py index e8baf944e6..f50a52a03e 100644 --- a/src/cleveragents/application/services/session_service.py +++ b/src/cleveragents/application/services/session_service.py @@ -260,14 +260,13 @@ class PersistentSessionService(SessionService): ) # Validate checksum - checksum = "sha256:" + data.get("checksum") - if checksum is None: + raw_checksum = data.get("checksum") + if raw_checksum is None: raise SessionImportError("Missing checksum in import data") + checksum = "sha256:" + raw_checksum # Recompute checksum - data_without_checksum = "sha256:" + { - k: v for k, v in data.items() if k != "checksum" - } + data_without_checksum = {k: v for k, v in data.items() if k != "checksum"} canonical = json.dumps(data_without_checksum, sort_keys=True, default=str) expected_checksum = "sha256:" + hashlib.sha256(canonical.encode()).hexdigest() if checksum != expected_checksum: diff --git a/src/cleveragents/cli/commands/session.py b/src/cleveragents/cli/commands/session.py index b2955c0618..d4deaec421 100644 --- a/src/cleveragents/cli/commands/session.py +++ b/src/cleveragents/cli/commands/session.py @@ -219,8 +219,9 @@ def create( console.print(Panel(details, title="Session", expand=False)) # Settings panel + _automation = session.metadata.get("automation_profile") or "default" settings_text = ( - f"[yellow]Automation:[/yellow] {session.automation_profile or 'default'}\n" + f"[yellow]Automation:[/yellow] {_automation}\n" "[yellow]Streaming:[/yellow] off\n" "[yellow]Context:[/yellow] default\n" "[yellow]Memory:[/yellow] enabled\n" @@ -232,6 +233,7 @@ def create( if session.actor_name: try: from cleveragents.application.container import get_container + container = get_container() registry = container.actor_registry() actor_obj = registry.get_actor(session.actor_name) @@ -373,6 +375,7 @@ def show( return # Session summary panel + _auto_profile = session.metadata.get("automation_profile") or "(none)" details = ( f"[bold]Session ID:[/bold] {session.session_id}\n" f"[bold]Actor:[/bold] {session.actor_name or '(none)'}\n" @@ -380,7 +383,7 @@ def show( f"[bold]Messages:[/bold] {session.message_count}\n" f"[bold]Created:[/bold] {session.created_at.strftime('%Y-%m-%d %H:%M')}\n" f"[bold]Updated:[/bold] {session.updated_at.strftime('%Y-%m-%d %H:%M')}\n" - f"[bold]Automation:[/bold] {session.automation_profile or '(none)'}" + f"[bold]Automation:[/bold] {_auto_profile}" ) console.print(Panel(details, title="Session Summary", expand=False)) @@ -480,8 +483,9 @@ def delete( try: service = _get_session_service() - # Verify session exists before prompting - service.get(session_id) + # Verify session exists before prompting and get message count + session_to_delete = service.get(session_id) + message_count = session_to_delete.message_count if not yes: confirm = typer.confirm(f"Delete session {session_id}?", default=False) @@ -489,10 +493,6 @@ def delete( console.print("[yellow]Aborted.[/yellow]") raise typer.Abort() - # Get message count before deletion - messages = service.list_messages(session_id) - message_count = len(messages) - service.delete(session_id) # Rich output: render Deletion Summary and Cleanup panels @@ -540,8 +540,6 @@ def delete( raise typer.Exit(1) from exc - - @app.command("export") def export_session( session_id: Annotated[ diff --git a/src/cleveragents/cli/commands/tool.py b/src/cleveragents/cli/commands/tool.py index 379966325e..428082b35d 100644 --- a/src/cleveragents/cli/commands/tool.py +++ b/src/cleveragents/cli/commands/tool.py @@ -244,12 +244,12 @@ def add( # Handle spec-compliant 'tool:' wrapper key format # If the YAML has a top-level 'tool:' key, extract its contents - if isinstance(config_dict, dict) and 'tool' in config_dict: - config_dict = config_dict['tool'] + if isinstance(config_dict, dict) and "tool" in config_dict: + config_dict = config_dict["tool"] # Ignore 'cleveragents:' version header if present - if isinstance(config_dict, dict) and 'cleveragents' in config_dict: - del config_dict['cleveragents'] + if isinstance(config_dict, dict) and "cleveragents" in config_dict: + del config_dict["cleveragents"] if not isinstance(config_dict, dict): raise ValueError("YAML config must be a mapping") diff --git a/vulture_whitelist.py b/vulture_whitelist.py index f6bd96090b..64e05df516 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -7,6 +7,9 @@ # Context manager __exit__ parameters required by protocol exc_tb # noqa: B018, F821 +# Protocol method parameters required by interface definition (extension_protocols.py) +destination # noqa: B018, F821 + # Legacy migrator method parameter needed for mapping interface consistency build_data # noqa: B018, F821