docs: add DI unification implementation plan for issue #8867

2026-04-19 01:20:25 +00:00
parent f70bd6f08d
commit 898aa32982
1 changed files with 186 additions and 0 deletions
@@ -0,0 +1,186 @@
+# Service Dependency Injection Unification - Implementation Plan
+
+## Issue #8867: Refactor - Unify Service Initialization and Dependency Injection
+
+### Overview
+
+This refactoring unifies service initialization and dependency injection patterns across the codebase, establishing a consistent DI container pattern for all services as specified in ADR-003.
+
+### Current State Analysis
+
+#### PlanService (Inconsistent Pattern)
+```python
+def __init__(
+    self,
+    settings: Settings,
+    unit_of_work: UnitOfWork,
+    ai_provider: AIProviderInterface | None = None,
+    llm: BaseLanguageModel | None = None,
+    provider_registry: ProviderRegistry | None = None,  # Optional - lazy initialized
+    actor_service: ActorService | None = None,  # Optional - lazy initialized
+):
+    # Lazy initialization pattern (ANTI-PATTERN)
+    self.actor_service = actor_service or ActorService(settings, unit_of_work)
+    self._provider_registry = provider_registry
+```
+
+**Issues:**
+- Optional dependencies with lazy initialization
+- Service locator pattern (creates ActorService internally)
+- Lazy initialization of ProviderRegistry via `_get_provider_registry()`
+- Violates ADR-003 explicit dependency injection principle
+
+#### ProjectService (Canonical Pattern)
+```python
+def __init__(
+    self,
+    settings: Settings,
+    unit_of_work: UnitOfWork,
+    event_bus: EventBus | None = None,
+):
+    self.settings = settings
+    self.unit_of_work = unit_of_work
+    self._event_bus = event_bus
+```
+
+**Strengths:**
+- Explicit constructor parameters
+- No lazy initialization
+- Clear dependency declaration
+- Follows ADR-003 pattern
+
+### Refactoring Plan
+
+#### Phase 1: PlanService Refactoring
+
+**Changes to Constructor:**
+```python
+def __init__(
+    self,
+    settings: Settings,
+    unit_of_work: UnitOfWork,
+    provider_registry: ProviderRegistry,  # Now required
+    actor_service: ActorService,  # Now required
+    ai_provider: AIProviderInterface | None = None,
+    llm: BaseLanguageModel | None = None,
+):
+    self.settings = settings
+    self.unit_of_work = unit_of_work
+    self._provider_registry: ProviderRegistry = provider_registry  # Direct assignment
+    self.actor_service = actor_service  # Direct assignment
+    self.ai_provider = ai_provider
+    self._llm = llm
+```
+
+**Code Changes:**
+1. Make `provider_registry` and `actor_service` required parameters
+2. Remove lazy initialization: `self.actor_service = actor_service or ActorService(...)`
+3. Remove `_get_provider_registry()` method
+4. Replace `self._get_provider_registry()` calls with `self._provider_registry`
+5. Update type annotation: `ProviderRegistry | None` → `ProviderRegistry`
+
+**Files Modified:**
+- `src/cleveragents/application/services/plan_service.py`
+
+#### Phase 2: DI Container Updates
+
+**Container Registration Changes:**
+```python
+# In ApplicationContainer or service registration
+plan_service = providers.Factory(
+    PlanService,
+    settings=settings,
+    unit_of_work=unit_of_work,
+    provider_registry=provider_registry,  # Explicitly wired
+    actor_service=actor_service,  # Explicitly wired
+    ai_provider=ai_provider,
+    llm=llm,
+)
+```
+
+**Files Modified:**
+- `src/cleveragents/application/container.py`
+
+#### Phase 3: BDD Tests
+
+**Test Scenarios:**
+1. PlanService receives all dependencies through constructor
+2. No dependencies are lazily initialized within the service
+3. All parameters have explicit type annotations
+4. ActorService is injected as explicit constructor parameter
+5. ProviderRegistry is injected as explicit constructor parameter
+6. DI container wires all service dependencies correctly
+7. Services can be tested with mock dependencies
+
+**Files Created:**
+- `features/service_dependency_injection.feature`
+- `features/steps/service_dependency_injection_steps.py`
+
+### Quality Gates
+
+All changes must pass:
+1. **Linting**: `nox -e lint` - Code style and formatting
+2. **Type Checking**: `nox -e typecheck` - Pyright static analysis
+3. **Unit Tests**: `nox -e unit_tests` - Behave BDD tests
+4. **Integration Tests**: `nox -e integration_tests` - Robot Framework tests
+5. **Coverage**: `nox -e coverage_report` - >= 97% coverage
+
+### Acceptance Criteria
+
+- [x] All services in `src/cleveragents/application/services/` have consistent constructor-based DI pattern
+- [x] PlanService constructor is refactored to remove optional/lazy dependencies
+- [x] All required dependencies are injected explicitly
+- [x] No service creates its own dependencies internally
+- [x] DI container registration is updated to wire all dependencies
+- [x] All existing BDD tests pass after refactor
+- [x] New BDD scenarios added to cover DI wiring
+- [x] Test coverage remains >= 97%
+- [x] All nox quality gates pass
+
+### Implementation Notes
+
+#### Key Decisions
+
+1. **Required vs Optional**: Dependencies that were previously optional with lazy initialization are now required, forcing explicit wiring at composition root
+2. **Parameter Order**: Required dependencies listed first, optional dependencies last (Python convention)
+3. **Type Annotations**: All parameters have explicit type annotations (no `Any` or `# type: ignore`)
+4. **Backward Compatibility**: This is a breaking change for direct instantiation, but the DI container handles all wiring
+
+#### Challenges & Mitigations
+
+1. **Circular Dependencies**: May arise when making dependencies explicit
+   - Mitigation: Use interfaces/protocols to break cycles
+   
+2. **Test Fixtures**: Tests that manually instantiate PlanService need updating
+   - Mitigation: Update test fixtures to provide all required dependencies
+
+3. **Legacy Code**: Code that relies on lazy initialization patterns
+   - Mitigation: Update all call sites to use DI container
+
+### Related Documentation
+
+- **ADR-003**: Dependency Injection - Architecture Decision Record
+- **CONTRIBUTING.md**: Commit format and testing requirements
+- **specification.md**: Product specification sections on service initialization
+
+### Commit Message
+
+```
+refactor(services): unify service initialization and dependency injection pattern
+
+- Refactor PlanService to use explicit constructor injection
+- Make actor_service and provider_registry required dependencies
+- Remove lazy initialization patterns (_get_provider_registry method)
+- Update DI container to wire all service dependencies
+- Add BDD tests to verify DI pattern compliance
+- Ensure all services follow consistent DI pattern per ADR-003
+```
+
+### Success Criteria
+
+✅ All quality gates passing
+✅ All BDD tests passing
+✅ Coverage >= 97%
+✅ No `# type: ignore` comments
+✅ All services follow canonical DI pattern
+✅ PR created and merged