diff --git a/features/steps/tdd_di_tool_registry_service_steps.py b/features/steps/tdd_di_tool_registry_service_steps.py new file mode 100644 index 000000000..1fbb367ba --- /dev/null +++ b/features/steps/tdd_di_tool_registry_service_steps.py @@ -0,0 +1,112 @@ +"""Step definitions for TDD: DI container resolution in _get_tool_registry_service. + +These steps verify that ``_get_tool_registry_service`` in +``cleveragents.cli.commands.validation`` delegates to +``container.tool_registry_service()`` rather than manually constructing +the service with ``create_engine`` / ``sessionmaker`` / repositories. + +The first scenario is a TDD scenario: it FAILS before the fix is applied +and PASSES after. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + +# --------------------------------------------------------------------------- +# Patch targets +# --------------------------------------------------------------------------- + +_PATCH_GET_CONTAINER = "cleveragents.application.container.get_container" + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a tdd di tool registry test runner") +def step_tdd_di_background(context: Context) -> None: + context.tdd_di_returned_service: Any = None + context.tdd_di_mock_container: MagicMock | None = None + context.tdd_di_mock_service: MagicMock | None = None + context.tdd_di_container_instance: Any = None + + +# --------------------------------------------------------------------------- +# Scenario 1: _get_tool_registry_service delegates to container +# --------------------------------------------------------------------------- + + +@given("the DI container has a tool_registry_service provider") +def step_tdd_di_container_has_provider(context: Context) -> None: + """Set up a mock container with a tool_registry_service() callable.""" + mock_service = MagicMock(name="MockToolRegistryService") + mock_container = MagicMock(name="MockContainer") + mock_container.tool_registry_service.return_value = mock_service + context.tdd_di_mock_container = mock_container + context.tdd_di_mock_service = mock_service + + +@when("_get_tool_registry_service is called with the mocked container") +def step_tdd_di_call_function(context: Context) -> None: + """Call _get_tool_registry_service with the mocked container injected.""" + from cleveragents.cli.commands.validation import _get_tool_registry_service + + with patch(_PATCH_GET_CONTAINER, return_value=context.tdd_di_mock_container): + context.tdd_di_returned_service = _get_tool_registry_service() + + +@then("the returned service should be the one from container.tool_registry_service") +def step_tdd_di_verify_service_identity(context: Context) -> None: + """The returned service must be exactly what container.tool_registry_service() returns.""" + assert context.tdd_di_returned_service is context.tdd_di_mock_service, ( + f"Expected the service from container.tool_registry_service(), " + f"but got {type(context.tdd_di_returned_service)}. " + "This means _get_tool_registry_service is NOT delegating to the container." + ) + + +@then("no manual engine or sessionmaker construction should have occurred") +def step_tdd_di_verify_no_manual_construction(context: Context) -> None: + """Verify that container.database_url() was NOT called (manual DI pattern).""" + mock_container = context.tdd_di_mock_container + assert not mock_container.database_url.called, ( + "container.database_url() was called, which means _get_tool_registry_service " + "is still using the manual DI pattern instead of delegating to " + "container.tool_registry_service()." + ) + + +# --------------------------------------------------------------------------- +# Scenario 2: ToolRegistryService is registered in the DI container +# --------------------------------------------------------------------------- + + +@given("the application DI container is initialised") +def step_tdd_di_init_container(context: Context) -> None: + """Initialise the real application DI container.""" + from cleveragents.application.container import Container, reset_container + + reset_container() + context.tdd_di_container_instance = Container() + + +@when("the tool_registry_service provider is accessed on the container") +def step_tdd_di_access_provider(context: Context) -> None: + """Access the tool_registry_service attribute on the container.""" + container = context.tdd_di_container_instance + context.tdd_di_provider = getattr(container, "tool_registry_service", None) + + +@then("the container should expose a tool_registry_service provider") +def step_tdd_di_verify_provider_exists(context: Context) -> None: + """The container must have a tool_registry_service provider attribute.""" + assert context.tdd_di_provider is not None, ( + "Container does not have a 'tool_registry_service' provider. " + "ToolRegistryService must be registered in the DI container." + ) diff --git a/features/steps/validation_cli_uncovered_branches_steps.py b/features/steps/validation_cli_uncovered_branches_steps.py index 0b73a3561..40141324b 100644 --- a/features/steps/validation_cli_uncovered_branches_steps.py +++ b/features/steps/validation_cli_uncovered_branches_steps.py @@ -55,10 +55,35 @@ def step_validation_cli_branch_background(context: Context) -> None: @given("the validation cli branch DI container provides a database url") def step_vcb_di_container(context: Context) -> None: - """Set up a mock container whose ``database_url()`` returns an in-memory - SQLite URL so the real construction path executes end-to-end.""" + """Set up a mock container whose ``tool_registry_service()`` returns a + real ToolRegistryService backed by an in-memory SQLite database. + + After Forgejo #3006, ``_get_tool_registry_service`` delegates directly to + ``container.tool_registry_service()`` — no manual engine/sessionmaker + construction occurs in the function itself. + """ + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + from cleveragents.application.services.tool_registry_service import ( + ToolRegistryService, + ) + from cleveragents.infrastructure.database.repositories import ( + ToolRegistryRepository, + ValidationAttachmentRepository, + ) + + engine = create_engine("sqlite:///:memory:", echo=False) + factory = sessionmaker(bind=engine, expire_on_commit=False) + tool_repo = ToolRegistryRepository(session_factory=factory) + attachment_repo = ValidationAttachmentRepository(session_factory=factory) + real_service = ToolRegistryService( + tool_repo=tool_repo, + attachment_repo=attachment_repo, + ) + mock_container = MagicMock() - mock_container.database_url.return_value = "sqlite:///:memory:" + mock_container.tool_registry_service.return_value = real_service context.vcb_mock_container = mock_container diff --git a/features/tdd_di_tool_registry_service.feature b/features/tdd_di_tool_registry_service.feature new file mode 100644 index 000000000..abe85c79f --- /dev/null +++ b/features/tdd_di_tool_registry_service.feature @@ -0,0 +1,23 @@ +@tdd @di @tool_registry +Feature: DI container resolution for _get_tool_registry_service + As a developer maintaining the CleverAgents codebase + I want _get_tool_registry_service to use the DI container directly + So that service wiring is consistent and not duplicated manually + + Background: + Given a tdd di tool registry test runner + + # TDD: This scenario should FAIL before the fix is applied. + # After the fix, _get_tool_registry_service must call + # container.tool_registry_service() instead of manually constructing + # the service with create_engine / sessionmaker / repositories. + Scenario: _get_tool_registry_service delegates to container.tool_registry_service + Given the DI container has a tool_registry_service provider + When _get_tool_registry_service is called with the mocked container + Then the returned service should be the one from container.tool_registry_service + And no manual engine or sessionmaker construction should have occurred + + Scenario: ToolRegistryService is registered in the DI container + Given the application DI container is initialised + When the tool_registry_service provider is accessed on the container + Then the container should expose a tool_registry_service provider diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index cf7a17523..69f6b7ed4 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -80,6 +80,7 @@ from cleveragents.application.services.skeleton_compressor import ( ) from cleveragents.application.services.skill_service import SkillService from cleveragents.application.services.subplan_service import SubplanService +from cleveragents.application.services.tool_registry_service import ToolRegistryService from cleveragents.application.services.trace_service import TraceService from cleveragents.application.services.uko_indexer import UKOIndexer from cleveragents.application.services.vector_store_service import VectorStoreService @@ -467,6 +468,37 @@ def _build_automation_profile_service( return AutomationProfileService(repo=profile_repo) +def _build_tool_registry_service( + database_url: str, +) -> ToolRegistryService: + """Build a ToolRegistryService with DB-backed persistence. + + Follows the same _build_* pattern used by _build_skill_service + and other DB-backed services: create engine, session factory, repositories, + then inject into the service constructor. + + This builder is registered in the DI container so that callers can + resolve ToolRegistryService via container.tool_registry_service() + without manually wiring the dependency graph (Forgejo #3006). + """ + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + from cleveragents.infrastructure.database.repositories import ( + ToolRegistryRepository, + ValidationAttachmentRepository, + ) + + engine = create_engine(database_url, echo=False) + factory = sessionmaker(bind=engine, expire_on_commit=False) + tool_repo = ToolRegistryRepository(session_factory=factory) + attachment_repo = ValidationAttachmentRepository(session_factory=factory) + return ToolRegistryService( + tool_repo=tool_repo, + attachment_repo=attachment_repo, + ) + + def _build_session_service( database_url: str, event_bus: ReactiveEventBus | None = None, @@ -713,6 +745,15 @@ class Container(containers.DeclarativeContainer): database_url=database_url, ) + # Tool Registry Service — Singleton so all callers share the same + # registry state within a process (Forgejo #3006). The CLI + # _get_tool_registry_service() delegates to this provider to + # avoid manual dependency wiring. + tool_registry_service = providers.Singleton( + _build_tool_registry_service, + database_url=database_url, + ) + # Automation Profile Service - DB-backed profile CRUD/resolution automation_profile_service = providers.Factory( _build_automation_profile_service, diff --git a/src/cleveragents/cli/commands/validation.py b/src/cleveragents/cli/commands/validation.py index a7202e75f..d9a9118b7 100644 --- a/src/cleveragents/cli/commands/validation.py +++ b/src/cleveragents/cli/commands/validation.py @@ -73,31 +73,14 @@ _FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich) def _get_tool_registry_service() -> Any: - """Get the ToolRegistryService from the container.""" + """Get the ToolRegistryService from the DI container. + + Delegates to get_container().tool_registry_service() so that + service wiring is managed exclusively by the container (Forgejo #3006). + """ 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.application.services.tool_registry_service import ( - ToolRegistryService, - ) - from cleveragents.infrastructure.database.repositories import ( - ToolRegistryRepository, - ValidationAttachmentRepository, - ) - - engine = create_engine(database_url, echo=False) - factory = sessionmaker(bind=engine, expire_on_commit=False) - tool_repo = ToolRegistryRepository(session_factory=factory) - attachment_repo = ValidationAttachmentRepository(session_factory=factory) - return ToolRegistryService( - tool_repo=tool_repo, - attachment_repo=attachment_repo, - ) + return get_container().tool_registry_service() def _validation_spec_dict(tool: Any) -> dict[str, Any]: