From 601f4088602734d8a110571b6ff090c80c2ef9e3 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Sat, 28 Mar 2026 23:12:07 +0000 Subject: [PATCH] bug(cli): automation_profile._get_service() bypasses DI container Route automation profile CLI service resolution through the DI container by using container.automation_profile_service() and wiring a dedicated automation_profile_service provider in the application container. This removes ad-hoc repository/session construction from the CLI path so dependency lifecycle and configuration stay centralized and consistent with specification DI rules. Add integration-style, non-mocked Behave proof that _get_service() resolves through real container/provider wiring, while keeping focused delegation assertions in the existing coverage feature. Remove the unrelated robot/resource_dag.robot changes from this PR and clean up duplicate _get_service test overlap by dropping the extra TDD-only feature file pair. ISSUES CLOSED: #990 --- ...tion_profile_cli_coverage_boost_r2.feature | 37 ++--- ...ion_profile_cli_coverage_boost_r2_steps.py | 145 ++++++++++-------- src/cleveragents/application/container.py | 29 ++++ .../cli/commands/automation_profile.py | 17 +- 4 files changed, 136 insertions(+), 92 deletions(-) diff --git a/features/automation_profile_cli_coverage_boost_r2.feature b/features/automation_profile_cli_coverage_boost_r2.feature index 96a0b831f..212c2999b 100644 --- a/features/automation_profile_cli_coverage_boost_r2.feature +++ b/features/automation_profile_cli_coverage_boost_r2.feature @@ -1,33 +1,34 @@ Feature: Automation Profile CLI _get_service coverage boost As a developer I want to cover the _get_service function in automation_profile CLI module - So that the uncovered lines 51, 53, 54, 56, 57, 59, 63-66 are exercised + So that DI-container delegation behavior is exercised Background: Given the automation profile CLI module is imported for r2 coverage - # ----------------------------------------------------------------- - # Direct invocation of _get_service (lines 51-66) - # ----------------------------------------------------------------- - - Scenario: _get_service creates AutomationProfileService with database-backed repo - Given a mock container returning an in-memory SQLite database URL + Scenario: _get_service returns the service provided by the container + Given a mock container returning an automation profile service instance When I call _get_service directly Then the returned object should be an AutomationProfileService instance - And the service should have a repository attached + And the returned service should be the same object provided by the container - Scenario: _get_service uses the database URL from the container - Given a mock container returning a custom SQLite database URL "sqlite:///tmp/test_r2.db" + Scenario: _get_service calls the container provider exactly once + Given a mock container returning an automation profile service instance When I call _get_service directly - Then the returned object should be an AutomationProfileService instance + Then the container automation profile service provider should be called once - Scenario: _get_service creates a working service that can list profiles - Given a mock container returning an in-memory SQLite database URL with tables created + Scenario: _get_service does not consult container database_url directly + Given a mock container returning an automation profile service instance + When I call _get_service directly + Then the container database_url provider should not be called + + Scenario: _get_service returns a working DB-backed service supplied by container + Given a mock container returning a working db-backed automation profile service When I call _get_service directly Then the returned service should be able to list profiles without error - Scenario: _get_service imports and wires all required components - Given a mock container returning an in-memory SQLite database URL - When I call _get_service and inspect the internals - Then the service repository should use a sessionmaker bound to an engine - And the repository should have auto_commit enabled + Scenario: _get_service resolves through real container/provider wiring + Given a real DI container configured with a temporary sqlite database + When I call _get_service directly without mocking container access + Then the returned object should be an AutomationProfileService instance + And the resolved service should include a database-backed automation profile repository diff --git a/features/steps/automation_profile_cli_coverage_boost_r2_steps.py b/features/steps/automation_profile_cli_coverage_boost_r2_steps.py index b1f19acf9..9363c5fe3 100644 --- a/features/steps/automation_profile_cli_coverage_boost_r2_steps.py +++ b/features/steps/automation_profile_cli_coverage_boost_r2_steps.py @@ -1,17 +1,12 @@ """Step definitions for automation_profile_cli_coverage_boost_r2.feature. -Targets uncovered lines 51, 53, 54, 56, 57, 59, 63, 64, 65, 66 in -src/cleveragents/cli/commands/automation_profile.py — all inside -the ``_get_service()`` helper which constructs an -``AutomationProfileService`` from the DI container's database URL. - -Existing tests mock ``_get_service`` entirely; these scenarios call -it *for real* while mocking only the container to supply an in-memory -SQLite URL so every import / wiring line is exercised. +These scenarios validate that ``automation_profile._get_service()`` delegates +to the DI container's ``automation_profile_service`` provider. """ from __future__ import annotations +import contextlib from unittest.mock import MagicMock, patch from behave import given, then, when @@ -42,10 +37,10 @@ def step_import_ap_module(context: Context) -> None: # --------------------------------------------------------------------------- -def _make_mock_container(database_url: str = "sqlite:///:memory:") -> MagicMock: - """Return a mock container whose ``database_url()`` returns *database_url*.""" - mock_container = MagicMock() - mock_container.database_url.return_value = database_url +def _make_mock_container(service: AutomationProfileService) -> MagicMock: + """Return a mock container whose provider returns *service*.""" + mock_container = MagicMock(name="container") + mock_container.automation_profile_service.return_value = service return mock_container @@ -59,38 +54,39 @@ def _ensure_tables(database_url: str) -> None: Base.metadata.create_all(engine) -# --------------------------------------------------------------------------- -# Scenario: _get_service creates AutomationProfileService (lines 51-66) -# --------------------------------------------------------------------------- +@given("a mock container returning an automation profile service instance") +def step_mock_container_service(context: Context) -> None: + context.r2_service_instance = AutomationProfileService() + context.r2_mock_container = _make_mock_container(context.r2_service_instance) -@given("a mock container returning an in-memory SQLite database URL") -def step_mock_container_memory(context: Context) -> None: - context.r2_db_url = "sqlite:///:memory:" - context.r2_mock_container = _make_mock_container(context.r2_db_url) - - -@given('a mock container returning a custom SQLite database URL "{url}"') -def step_mock_container_custom_url(context: Context, url: str) -> None: - context.r2_db_url = url - context.r2_mock_container = _make_mock_container(url) - - -@given( - "a mock container returning an in-memory SQLite database URL with tables created" -) -def step_mock_container_with_tables(context: Context) -> None: +@given("a mock container returning a working db-backed automation profile service") +def step_mock_container_working_db_service(context: Context) -> None: # Use a file-based temp db so the tables persist across connections import os import tempfile fd, path = tempfile.mkstemp(suffix=".db") os.close(fd) - context.r2_db_url = f"sqlite:///{path}" - context.r2_mock_container = _make_mock_container(context.r2_db_url) - _ensure_tables(context.r2_db_url) + db_url = f"sqlite:///{path}" + _ensure_tables(db_url) context.r2_temp_db_path = path + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + from cleveragents.infrastructure.database.repositories import ( + AutomationProfileRepository, + ) + + engine = create_engine(db_url, echo=False) + session_factory = sessionmaker(bind=engine, expire_on_commit=False) + repo = AutomationProfileRepository( + session_factory=session_factory, auto_commit=True + ) + context.r2_service_instance = AutomationProfileService(repo=repo) + context.r2_mock_container = _make_mock_container(context.r2_service_instance) + def cleanup(): import contextlib @@ -110,13 +106,9 @@ def step_call_get_service(context: Context) -> None: context.r2_service = ap_mod._get_service() -@when("I call _get_service and inspect the internals") -def step_call_get_service_inspect(context: Context) -> None: - with patch( - "cleveragents.application.container.get_container", - return_value=context.r2_mock_container, - ): - context.r2_service = ap_mod._get_service() +@when("I call _get_service directly without mocking container access") +def step_call_get_service_without_mock(context: Context) -> None: + context.r2_service = ap_mod._get_service() # --------------------------------------------------------------------------- @@ -131,13 +123,19 @@ def step_verify_service_type(context: Context) -> None: ) -@then("the service should have a repository attached") -def step_verify_repo_attached(context: Context) -> None: - repo = context.r2_service._repo - assert repo is not None, "Expected a non-None repository on the service" - assert isinstance(repo, AutomationProfileRepository), ( - f"Expected AutomationProfileRepository, got {type(repo)}" - ) +@then("the returned service should be the same object provided by the container") +def step_verify_same_service(context: Context) -> None: + assert context.r2_service is context.r2_service_instance + + +@then("the container automation profile service provider should be called once") +def step_verify_provider_called_once(context: Context) -> None: + context.r2_mock_container.automation_profile_service.assert_called_once_with() + + +@then("the container database_url provider should not be called") +def step_verify_database_url_not_called(context: Context) -> None: + context.r2_mock_container.database_url.assert_not_called() @then("the returned service should be able to list profiles without error") @@ -148,18 +146,45 @@ def step_verify_service_list_profiles(context: Context) -> None: assert len(profiles) > 0, "Expected at least one built-in profile" -@then("the service repository should use a sessionmaker bound to an engine") -def step_verify_sessionmaker(context: Context) -> None: - repo = context.r2_service._repo - assert repo is not None - # The session_factory should be callable (a sessionmaker) - assert callable(repo._session_factory), "Expected session_factory to be callable" +@given("a real DI container configured with a temporary sqlite database") +def step_real_container_configured(context: Context) -> None: + import os + import tempfile + from dependency_injector import providers + from sqlalchemy import create_engine -@then("the repository should have auto_commit enabled") -def step_verify_auto_commit(context: Context) -> None: - repo = context.r2_service._repo - assert repo is not None - assert repo._auto_commit is True, ( - f"Expected auto_commit=True, got {repo._auto_commit}" + from cleveragents.application.container import get_container, reset_container + from cleveragents.infrastructure.database.models import ( + AutomationProfileModel, + Base, ) + + fd, path = tempfile.mkstemp(suffix=".db") + os.close(fd) + db_url = f"sqlite:///{path}" + + engine = create_engine(db_url, echo=False) + Base.metadata.create_all(engine, tables=[AutomationProfileModel.__table__]) + + reset_container() + container = get_container() + container.database_url.override(providers.Object(db_url)) + + context.r2_temp_db_path = path + context.r2_real_container = container + + def cleanup() -> None: + reset_container() + with contextlib.suppress(OSError): + os.unlink(path) + + context.add_cleanup(cleanup) + + +@then( + "the resolved service should include a database-backed automation profile repository" +) +def step_verify_db_backed_repository(context: Context) -> None: + repo = context.r2_service._repo + assert isinstance(repo, AutomationProfileRepository) diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 0c494505f..4006f8e3e 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -23,6 +23,9 @@ from cleveragents.application.services.audit_event_subscriber import ( AuditEventSubscriber, ) from cleveragents.application.services.audit_service import AuditService +from cleveragents.application.services.automation_profile_service import ( + AutomationProfileService, +) from cleveragents.application.services.autonomy_controller import ( AutonomyController, ) @@ -440,6 +443,26 @@ def _build_skill_service( return SkillService() +def _build_automation_profile_service( + database_url: str, +) -> AutomationProfileService: + """Build an ``AutomationProfileService`` with DB-backed persistence.""" + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + from cleveragents.infrastructure.database.repositories import ( + AutomationProfileRepository, + ) + + engine = create_engine(database_url, echo=False) + factory = sessionmaker(bind=engine, expire_on_commit=False) + profile_repo = AutomationProfileRepository( + session_factory=factory, + auto_commit=True, + ) + return AutomationProfileService(repo=profile_repo) + + def _build_session_service( database_url: str, event_bus: ReactiveEventBus | None = None, @@ -680,6 +703,12 @@ class Container(containers.DeclarativeContainer): database_url=database_url, ) + # Automation Profile Service - DB-backed profile CRUD/resolution + automation_profile_service = providers.Factory( + _build_automation_profile_service, + database_url=database_url, + ) + # Session Service - database-backed session management (Forgejo #554, #570, #680) session_service = providers.Factory( _build_session_service, diff --git a/src/cleveragents/cli/commands/automation_profile.py b/src/cleveragents/cli/commands/automation_profile.py index 17250a574..10745a015 100644 --- a/src/cleveragents/cli/commands/automation_profile.py +++ b/src/cleveragents/cli/commands/automation_profile.py @@ -47,23 +47,12 @@ _FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich) def _get_service() -> AutomationProfileService: - """Get the AutomationProfileService with database-backed repository.""" + """Resolve ``AutomationProfileService`` from the DI container.""" from cleveragents.application.container import get_container container = get_container() - database_url: str = container.database_url() - - from sqlalchemy import create_engine - from sqlalchemy.orm import sessionmaker - - from cleveragents.infrastructure.database.repositories import ( - AutomationProfileRepository, - ) - - engine = create_engine(database_url, echo=False) - factory = sessionmaker(bind=engine, expire_on_commit=False) - repo = AutomationProfileRepository(session_factory=factory, auto_commit=True) - return AutomationProfileService(repo=repo) + service: AutomationProfileService = container.automation_profile_service() + return service def _guards_dict(guards: AutomationGuard | None) -> dict[str, object] | None: -- 2.52.0