cleveragents/cleveragents-core

Fork 3

UAT: MemoryService instantiated ad-hoc inside PlanService instead of being injected via DI #3929

New issue

Open

opened 2026-04-06 07:32:38 +00:00 by freemo · 1 comment

freemo commented

2026-04-06 07:32:38 +00:00

Owner

Metadata

Branch: fix/plan-service-memory-service-di-injection
Commit Message: fix(plan-service): inject MemoryService via DI instead of instantiating ad-hoc
Milestone: None — backlog (see backlog note below)
Parent Epic: TBD — needs manual linking

Backlog note: This issue was discovered during autonomous operation
on milestone v3.6.0. It does not block milestone completion and has been
placed in the backlog for human review and future milestone assignment.

Bug Report

Feature Area: Dependency Injection and Service Layer

Severity: Medium — violates DI principles; MemoryService instances are created inside PlanService rather than being injected, making them untestable

What Was Tested

Code-level analysis of src/cleveragents/application/services/plan_service.py against DI requirements.

Expected Behavior (from spec)

Per the specification and CONTRIBUTING.md, Dependency Injection is required for all service dependencies. Services should receive their dependencies through constructor injection, not create them internally. This is required for testability (substituting test doubles) and inversion of control.

Actual Behavior

PlanService creates MemoryService instances internally in _get_memory_service() (lines ~443-470 in plan_service.py):

def _get_memory_service(self, session_id: str, ...) -> MemoryService:
    """Retrieve or initialize a MemoryService instance for a session."""
    from cleveragents.application.services.memory_service import MemoryService
    
    if session_id not in self._memory_services:
        if self._ai_provider is not None:
            service = MemoryService(
                session_id=session_id,
                ...
            )
        else:
            service = MemoryService(session_id=session_id, max_messages=max_messages)
        self._memory_services[session_id] = service
    return self._memory_services[session_id]

PlanService maintains an internal _memory_services: dict[str, MemoryService] cache and creates MemoryService instances on demand. MemoryService is not registered in the DI container and cannot be injected or mocked via the container's override_providers() mechanism.

Impact

Untestable: Unit tests cannot inject a mock MemoryService because PlanService creates them internally.
Hidden dependency: PlanService's dependency on MemoryService is not visible in its constructor signature.
DI bypass: The DI container (container.py) has no memory_service provider, so the service cannot be resolved or overridden via the standard DI mechanism.
Spec violation: The spec requires DI for all service dependencies.

Steps to Reproduce (Code Analysis)

Open src/cleveragents/application/services/plan_service.py
Search for _memory_services and _get_memory_service
Observe that MemoryService is created inside the method body, not injected via constructor

Code Locations

File: src/cleveragents/application/services/plan_service.py
Method: _get_memory_service(), lines ~443-470
Field: self._memory_services: dict[str, MemoryService] = {} in __init__

Recommended Fix

Register MemoryService (or a MemoryServiceFactory) in the DI container.
Inject a factory callable into PlanService.__init__():

def __init__(
    self,
    settings: Settings,
    unit_of_work: UnitOfWork,
    ai_provider: AIProviderInterface | None = None,
    provider_registry: ProviderRegistry | None = None,
    actor_service: ActorService | None = None,
    memory_service_factory: Callable[[str], MemoryService] | None = None,  # ADD
) -> None:
    ...
    self._memory_service_factory = memory_service_factory

Subtasks

Register MemoryService (or a MemoryServiceFactory) in the DI container (container.py)
Add memory_service_factory: Callable[[str], MemoryService] | None parameter to PlanService.__init__()
Refactor _get_memory_service() to use the injected factory instead of creating instances directly
Remove or adapt the internal _memory_services cache to work with the injected factory
Update unit tests (Behave/Gherkin in features/) to inject a mock MemoryService factory via container.override_providers()
Verify all existing tests pass with the refactored DI approach
Ensure all type annotations are explicit and pass nox -e typecheck

Definition of Done

MemoryService is registered in the DI container (container.py)
PlanService.__init__() accepts memory_service_factory as an injected dependency
_get_memory_service() uses the injected factory, not internal instantiation
Unit tests can inject a mock MemoryService via container.override_providers()
No # type: ignore suppressions added
All nox stages pass
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

## Metadata - **Branch**: `fix/plan-service-memory-service-di-injection` - **Commit Message**: `fix(plan-service): inject MemoryService via DI instead of instantiating ad-hoc` - **Milestone**: None — backlog (see backlog note below) - **Parent Epic**: TBD — needs manual linking > **Backlog note:** This issue was discovered during autonomous operation > on milestone v3.6.0. It does not block milestone completion and has been > placed in the backlog for human review and future milestone assignment. --- ## Bug Report **Feature Area:** Dependency Injection and Service Layer **Severity:** Medium — violates DI principles; `MemoryService` instances are created inside `PlanService` rather than being injected, making them untestable ### What Was Tested Code-level analysis of `src/cleveragents/application/services/plan_service.py` against DI requirements. ### Expected Behavior (from spec) Per the specification and CONTRIBUTING.md, **Dependency Injection** is required for all service dependencies. Services should receive their dependencies through constructor injection, not create them internally. This is required for testability (substituting test doubles) and inversion of control. ### Actual Behavior `PlanService` creates `MemoryService` instances internally in `_get_memory_service()` (lines ~443-470 in `plan_service.py`): ```python def _get_memory_service(self, session_id: str, ...) -> MemoryService: """Retrieve or initialize a MemoryService instance for a session.""" from cleveragents.application.services.memory_service import MemoryService if session_id not in self._memory_services: if self._ai_provider is not None: service = MemoryService( session_id=session_id, ... ) else: service = MemoryService(session_id=session_id, max_messages=max_messages) self._memory_services[session_id] = service return self._memory_services[session_id] ``` `PlanService` maintains an internal `_memory_services: dict[str, MemoryService]` cache and creates `MemoryService` instances on demand. `MemoryService` is not registered in the DI container and cannot be injected or mocked via the container's `override_providers()` mechanism. ### Impact 1. **Untestable**: Unit tests cannot inject a mock `MemoryService` because `PlanService` creates them internally. 2. **Hidden dependency**: `PlanService`'s dependency on `MemoryService` is not visible in its constructor signature. 3. **DI bypass**: The DI container (`container.py`) has no `memory_service` provider, so the service cannot be resolved or overridden via the standard DI mechanism. 4. **Spec violation**: The spec requires DI for all service dependencies. ### Steps to Reproduce (Code Analysis) 1. Open `src/cleveragents/application/services/plan_service.py` 2. Search for `_memory_services` and `_get_memory_service` 3. Observe that `MemoryService` is created inside the method body, not injected via constructor ### Code Locations - File: `src/cleveragents/application/services/plan_service.py` - Method: `_get_memory_service()`, lines ~443-470 - Field: `self._memory_services: dict[str, MemoryService] = {}` in `__init__` ### Recommended Fix 1. Register `MemoryService` (or a `MemoryServiceFactory`) in the DI container. 2. Inject a factory callable into `PlanService.__init__()`: ```python def __init__( self, settings: Settings, unit_of_work: UnitOfWork, ai_provider: AIProviderInterface | None = None, provider_registry: ProviderRegistry | None = None, actor_service: ActorService | None = None, memory_service_factory: Callable[[str], MemoryService] | None = None, # ADD ) -> None: ... self._memory_service_factory = memory_service_factory ``` --- ## Subtasks - [ ] Register `MemoryService` (or a `MemoryServiceFactory`) in the DI container (`container.py`) - [ ] Add `memory_service_factory: Callable[[str], MemoryService] | None` parameter to `PlanService.__init__()` - [ ] Refactor `_get_memory_service()` to use the injected factory instead of creating instances directly - [ ] Remove or adapt the internal `_memory_services` cache to work with the injected factory - [ ] Update unit tests (Behave/Gherkin in `features/`) to inject a mock `MemoryService` factory via `container.override_providers()` - [ ] Verify all existing tests pass with the refactored DI approach - [ ] Ensure all type annotations are explicit and pass `nox -e typecheck` ## Definition of Done - [ ] `MemoryService` is registered in the DI container (`container.py`) - [ ] `PlanService.__init__()` accepts `memory_service_factory` as an injected dependency - [ ] `_get_memory_service()` uses the injected factory, not internal instantiation - [ ] Unit tests can inject a mock `MemoryService` via `container.override_providers()` - [ ] No `# type: ignore` suppressions added - [ ] All nox stages pass - Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo commented

2026-04-06 07:32:55 +00:00

Author

Owner

⚠️ Orphan Issue — Needs Manual Linking

This issue was created by an automated agent and does not currently have a parent Epic linked. No existing open Epic was found that clearly covers DI refactoring of the PlanService / MemoryService service layer.

Action required: A human reviewer should link this issue to the appropriate parent Epic (or create a new Epic for "Service Layer DI Compliance") and set the correct dependency relationship so that this issue blocks the parent Epic.

Candidate epics to consider:

A new Epic covering "Service Layer DI Compliance / Testability Hardening"
An existing plan-lifecycle Epic if one is created in a future milestone

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

⚠️ **Orphan Issue — Needs Manual Linking** This issue was created by an automated agent and does not currently have a parent Epic linked. No existing open Epic was found that clearly covers DI refactoring of the `PlanService` / `MemoryService` service layer. **Action required:** A human reviewer should link this issue to the appropriate parent Epic (or create a new Epic for "Service Layer DI Compliance") and set the correct dependency relationship so that this issue **blocks** the parent Epic. Candidate epics to consider: - A new Epic covering "Service Layer DI Compliance / Testability Hardening" - An existing plan-lifecycle Epic if one is created in a future milestone --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo referenced this issue

2026-04-06 07:33:38 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo added the

Points

label

2026-04-06 07:37:24 +00:00

freemo referenced this issue

2026-04-06 07:39:33 +00:00

[Automated] CleverAgents Build Session - 2026-04-06 #3775

freemo referenced this issue

2026-04-06 07:49:16 +00:00