fix(cli): replace manual DI wiring in _get_tool_registry_service with container resolution #3325

2026-04-05T09:50:52Z

freemo commented

2026-04-05 09:50:52 +00:00

Summary

This PR fixes a dependency injection anti-pattern in _get_tool_registry_service within cli/commands/validation.py, where the full ToolRegistryService dependency graph was being manually constructed instead of delegating to the DI container. The fix registers tool_registry_service as a proper providers.Singleton in the container and reduces the function to a clean one-liner, consistent with the established patterns used elsewhere in the codebase.

Changes

src/cleveragents/application/container.py: Added a _build_tool_registry_service(database_url) builder function following the established _build_skill_service / _build_session_service pattern. The builder encapsulates the engine, sessionmaker, ToolRegistryRepository, and ValidationAttachmentRepository wiring that previously leaked into the CLI layer. Registered tool_registry_service as a providers.Singleton in the Container class so the service is resolved through the container like every other service.
src/cleveragents/cli/commands/validation.py: Refactored _get_tool_registry_service() from ~20 lines of manual dependency wiring down to a single line: return get_container().tool_registry_service(). This is now consistent with the _get_skill_service() pattern in cli/commands/skill.py and eliminates the duplicated wiring logic that belonged exclusively in the container.
features/tdd_di_tool_registry_service.feature (new): Added a TDD Behave feature file with two scenarios that were failing before the fix:
- Scenario 1: _get_tool_registry_service delegates to container.tool_registry_service().
- Scenario 2: The Container class exposes a tool_registry_service provider.
features/steps/tdd_di_tool_registry_service_steps.py (new): Step definitions for the new TDD feature file.
features/steps/validation_cli_uncovered_branches_steps.py (updated): Updated mock setup to patch container.tool_registry_service() instead of the now-removed container.database_url() call path, keeping existing branch-coverage scenarios aligned with the refactored implementation.

Design Decisions

providers.Singleton over providers.Factory: tool_registry_service is registered as a Singleton to match the skill_service pattern. Registry services share a single database connection pool within a process lifetime; creating a new engine and sessionmaker on every call would be wasteful and inconsistent with how other services are wired.

Builder function pattern: The _build_tool_registry_service(database_url) function mirrors _build_skill_service and _build_session_service exactly — it is a plain function (not a method) that accepts the database URL and returns a fully constructed service. This keeps the Container class declaration clean and makes the wiring easy to test in isolation.

No in-memory fallback: Unlike _build_skill_service, no fallback to an in-memory mode is provided. ToolRegistryService requires a real database and has no in-memory implementation, so adding a fallback would be misleading. If the database URL is unavailable the container will raise, which is the correct failure mode.

Minimal blast radius: Only the container registration and the single CLI helper function were changed. No public API, no serialisation format, and no database schema were touched.

Testing

Unit tests (Behave): PASSED — 31 scenarios, 138 steps (all affected feature files)
lint: PASSED
typecheck: PASSED (0 errors, 0 warnings)
New TDD scenarios: 2 scenarios added (tdd_di_tool_registry_service.feature), both were failing before the fix and pass after

Modules Affected

Module	Change
`src/cleveragents/application/container.py`	Added `_build_tool_registry_service` builder; registered `tool_registry_service` Singleton
`src/cleveragents/cli/commands/validation.py`	Replaced ~20-line manual DI wiring with `get_container().tool_registry_service()`
`features/tdd_di_tool_registry_service.feature`	New TDD feature file (2 scenarios)
`features/steps/tdd_di_tool_registry_service_steps.py`	New step definitions for TDD feature
`features/steps/validation_cli_uncovered_branches_steps.py`	Updated mock target to match refactored implementation

Related Issues

Closes #3006

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

## Summary This PR fixes a dependency injection anti-pattern in `_get_tool_registry_service` within `cli/commands/validation.py`, where the full `ToolRegistryService` dependency graph was being manually constructed instead of delegating to the DI container. The fix registers `tool_registry_service` as a proper `providers.Singleton` in the container and reduces the function to a clean one-liner, consistent with the established patterns used elsewhere in the codebase. ## Changes - **`src/cleveragents/application/container.py`**: Added a `_build_tool_registry_service(database_url)` builder function following the established `_build_skill_service` / `_build_session_service` pattern. The builder encapsulates the engine, sessionmaker, `ToolRegistryRepository`, and `ValidationAttachmentRepository` wiring that previously leaked into the CLI layer. Registered `tool_registry_service` as a `providers.Singleton` in the `Container` class so the service is resolved through the container like every other service. - **`src/cleveragents/cli/commands/validation.py`**: Refactored `_get_tool_registry_service()` from ~20 lines of manual dependency wiring down to a single line: `return get_container().tool_registry_service()`. This is now consistent with the `_get_skill_service()` pattern in `cli/commands/skill.py` and eliminates the duplicated wiring logic that belonged exclusively in the container. - **`features/tdd_di_tool_registry_service.feature`** *(new)*: Added a TDD Behave feature file with two scenarios that were failing before the fix: - Scenario 1: `_get_tool_registry_service` delegates to `container.tool_registry_service()`. - Scenario 2: The `Container` class exposes a `tool_registry_service` provider. - **`features/steps/tdd_di_tool_registry_service_steps.py`** *(new)*: Step definitions for the new TDD feature file. - **`features/steps/validation_cli_uncovered_branches_steps.py`** *(updated)*: Updated mock setup to patch `container.tool_registry_service()` instead of the now-removed `container.database_url()` call path, keeping existing branch-coverage scenarios aligned with the refactored implementation. ## Design Decisions **`providers.Singleton` over `providers.Factory`**: `tool_registry_service` is registered as a `Singleton` to match the `skill_service` pattern. Registry services share a single database connection pool within a process lifetime; creating a new engine and sessionmaker on every call would be wasteful and inconsistent with how other services are wired. **Builder function pattern**: The `_build_tool_registry_service(database_url)` function mirrors `_build_skill_service` and `_build_session_service` exactly — it is a plain function (not a method) that accepts the database URL and returns a fully constructed service. This keeps the `Container` class declaration clean and makes the wiring easy to test in isolation. **No in-memory fallback**: Unlike `_build_skill_service`, no fallback to an in-memory mode is provided. `ToolRegistryService` requires a real database and has no in-memory implementation, so adding a fallback would be misleading. If the database URL is unavailable the container will raise, which is the correct failure mode. **Minimal blast radius**: Only the container registration and the single CLI helper function were changed. No public API, no serialisation format, and no database schema were touched. ## Testing - **Unit tests (Behave):** PASSED — 31 scenarios, 138 steps (all affected feature files) - **lint:** PASSED - **typecheck:** PASSED (0 errors, 0 warnings) - **New TDD scenarios:** 2 scenarios added (`tdd_di_tool_registry_service.feature`), both were failing before the fix and pass after ## Modules Affected | Module | Change | |---|---| | `src/cleveragents/application/container.py` | Added `_build_tool_registry_service` builder; registered `tool_registry_service` Singleton | | `src/cleveragents/cli/commands/validation.py` | Replaced ~20-line manual DI wiring with `get_container().tool_registry_service()` | | `features/tdd_di_tool_registry_service.feature` | New TDD feature file (2 scenarios) | | `features/steps/tdd_di_tool_registry_service_steps.py` | New step definitions for TDD feature | | `features/steps/validation_cli_uncovered_branches_steps.py` | Updated mock target to match refactored implementation | ## Related Issues Closes #3006 --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker

freemo added this to the v3.3.0 milestone 2026-04-05 09:55:15 +00:00

freemo referenced this pull request

2026-04-05 09:58:04 +00:00

BUG-HUNT: [consistency] Manual dependency injection in `_get_tool_registry_service` #3006

freemo commented

2026-04-05 10:06:29 +00:00

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775374800]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775374800] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

freemo commented

2026-04-05 10:13:44 +00:00

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775375100]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775375100] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

freemo commented

2026-04-05 10:21:33 +00:00

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775373400]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-3325-1775373400] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

freemo referenced this pull request

2026-04-05 10:22:46 +00:00

[CA-AUTO] Backlog Groomer — Session Tracker — groomer-1 — 2026-04-05 #3182

freemo reviewed 2026-04-05 16:22:26 +00:00

freemo left a comment

Review Summary

Reviewed PR #3325 with focus on concurrency-safety, race-conditions, and deadlock-risks.

This PR replaces manual DI wiring in _get_tool_registry_service() with proper container resolution, following the established _build_* pattern used by _build_skill_service, _build_session_service, and other DB-backed services. The change is clean, well-scoped, and correctly addresses issue #3006.

Reviewer verdict: APPROVED ✅ (posted as COMMENT due to Forgejo self-review restriction)

✅ Concurrency Safety Deep Dive

Given special attention to concurrency implications of the Singleton pattern:

Thread-safe Singleton creation: The dependency-injector library's providers.Singleton uses an internal lock for lazy initialization, ensuring the builder function is called exactly once even under concurrent access. This is the same mechanism used by all other Singleton providers in the container (skill_service, event_bus, audit_service, etc.). No new concurrency risk introduced.

Improved connection pooling: The old code in _get_tool_registry_service() called create_engine() and sessionmaker() on every invocation, creating a new engine and connection pool each time. The new Singleton pattern creates a single engine with proper connection pooling for the process lifetime. This is actually a concurrency improvement — multiple concurrent callers now share a properly pooled engine rather than each creating their own.

SQLAlchemy engine thread safety: The create_engine() call produces a thread-safe engine with a built-in connection pool. The sessionmaker is thread-safe to call (each invocation produces a new session). The repositories receive the session_factory and create sessions per-operation, which is the correct pattern for concurrent access.

No deadlock risk: No new locks are introduced. The Singleton provider's internal lock is the same mechanism used throughout the container. The _build_tool_registry_service function performs no blocking I/O that could cause lock contention during initialization.

get_container() global state: The existing get_container() function uses global variables without thread synchronization — this is a pre-existing concern not introduced or worsened by this PR.

✅ Specification Compliance

Implementation correctly follows ADR-003 (Dependency Injection Framework) by centralizing service wiring in the container
Module boundaries respected: CLI layer delegates to application container, no infrastructure imports leak into CLI
Builder function pattern (_build_tool_registry_service) mirrors established patterns exactly

✅ CONTRIBUTING.md Compliance

Commit message: fix(cli): replace manual DI wiring in _get_tool_registry_service with container resolution — correct Conventional Changelog format ✅
Issue linking: ISSUES CLOSED: #3006 in commit footer, Closes #3006 in PR body ✅
Milestone: v3.3.0 assigned (matches issue) ✅
Single atomic commit: Yes ✅
No # type: ignore added: Confirmed ✅

✅ Code Correctness

_build_tool_registry_service follows the exact same pattern as _build_skill_service, _build_automation_profile_service, etc.
Singleton choice is well-justified: registry services share a single DB connection pool within a process lifetime
No in-memory fallback is intentionally omitted (documented in PR description) — ToolRegistryService requires a real database
The refactored _get_tool_registry_service() is now a clean one-liner consistent with _get_skill_service() in cli/commands/skill.py

✅ Test Quality

Two TDD Behave scenarios properly verify:
1. _get_tool_registry_service delegates to container.tool_registry_service() (not manual wiring)
2. The Container class exposes a tool_registry_service provider
Tests use proper mock isolation with unittest.mock.patch
Updated validation_cli_uncovered_branches_steps.py correctly adapts mock targets to the new container-delegation pattern
Scenario 2 uses the real Container class to verify provider registration — good integration-level verification

Minor Suggestions (Non-blocking)

PR label mismatch: The PR carries Type/Refactor but the linked issue #3006 is labeled Type/Bug. The commit message prefix fix(cli) correctly identifies this as a bug fix. Consider updating the PR label to Type/Bug for consistency with the issue and commit convention.
Return type annotation opportunity: _get_tool_registry_service() still returns -> Any. Since the function now simply delegates to the container, it could return -> ToolRegistryService for better type safety. This is pre-existing and non-blocking, but would be a nice improvement if addressed.
container.py file size: At ~36KB / ~600+ lines, container.py continues to grow. This is a pre-existing concern — the PR adds only ~30 lines following established patterns, so it's not a blocker here. Worth noting for future architectural consideration.

Decision: APPROVED ✅

The implementation is clean, follows established patterns, and the concurrency implications are actually positive (shared connection pool via Singleton vs. per-call engine creation). No race conditions, deadlocks, or thread-safety issues identified.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Review Summary Reviewed PR #3325 with focus on **concurrency-safety**, **race-conditions**, and **deadlock-risks**. This PR replaces manual DI wiring in `_get_tool_registry_service()` with proper container resolution, following the established `_build_*` pattern used by `_build_skill_service`, `_build_session_service`, and other DB-backed services. The change is clean, well-scoped, and correctly addresses issue #3006. **Reviewer verdict: APPROVED** ✅ (posted as COMMENT due to Forgejo self-review restriction) --- ### ✅ Concurrency Safety Deep Dive Given special attention to concurrency implications of the Singleton pattern: **Thread-safe Singleton creation**: The `dependency-injector` library's `providers.Singleton` uses an internal lock for lazy initialization, ensuring the builder function is called exactly once even under concurrent access. This is the same mechanism used by all other Singleton providers in the container (`skill_service`, `event_bus`, `audit_service`, etc.). **No new concurrency risk introduced.** **Improved connection pooling**: The old code in `_get_tool_registry_service()` called `create_engine()` and `sessionmaker()` on *every invocation*, creating a new engine and connection pool each time. The new Singleton pattern creates a single engine with proper connection pooling for the process lifetime. This is actually a **concurrency improvement** — multiple concurrent callers now share a properly pooled engine rather than each creating their own. **SQLAlchemy engine thread safety**: The `create_engine()` call produces a thread-safe engine with a built-in connection pool. The `sessionmaker` is thread-safe to call (each invocation produces a new session). The repositories receive the `session_factory` and create sessions per-operation, which is the correct pattern for concurrent access. **No deadlock risk**: No new locks are introduced. The Singleton provider's internal lock is the same mechanism used throughout the container. The `_build_tool_registry_service` function performs no blocking I/O that could cause lock contention during initialization. **`get_container()` global state**: The existing `get_container()` function uses `global` variables without thread synchronization — this is a pre-existing concern not introduced or worsened by this PR. ### ✅ Specification Compliance - Implementation correctly follows ADR-003 (Dependency Injection Framework) by centralizing service wiring in the container - Module boundaries respected: CLI layer delegates to application container, no infrastructure imports leak into CLI - Builder function pattern (`_build_tool_registry_service`) mirrors established patterns exactly ### ✅ CONTRIBUTING.md Compliance - **Commit message**: `fix(cli): replace manual DI wiring in _get_tool_registry_service with container resolution` — correct Conventional Changelog format ✅ - **Issue linking**: `ISSUES CLOSED: #3006` in commit footer, `Closes #3006` in PR body ✅ - **Milestone**: v3.3.0 assigned (matches issue) ✅ - **Single atomic commit**: Yes ✅ - **No `# type: ignore` added**: Confirmed ✅ ### ✅ Code Correctness - `_build_tool_registry_service` follows the exact same pattern as `_build_skill_service`, `_build_automation_profile_service`, etc. - Singleton choice is well-justified: registry services share a single DB connection pool within a process lifetime - No in-memory fallback is intentionally omitted (documented in PR description) — `ToolRegistryService` requires a real database - The refactored `_get_tool_registry_service()` is now a clean one-liner consistent with `_get_skill_service()` in `cli/commands/skill.py` ### ✅ Test Quality - Two TDD Behave scenarios properly verify: 1. `_get_tool_registry_service` delegates to `container.tool_registry_service()` (not manual wiring) 2. The `Container` class exposes a `tool_registry_service` provider - Tests use proper mock isolation with `unittest.mock.patch` - Updated `validation_cli_uncovered_branches_steps.py` correctly adapts mock targets to the new container-delegation pattern - Scenario 2 uses the real `Container` class to verify provider registration — good integration-level verification ### Minor Suggestions (Non-blocking) 1. **PR label mismatch**: The PR carries `Type/Refactor` but the linked issue #3006 is labeled `Type/Bug`. The commit message prefix `fix(cli)` correctly identifies this as a bug fix. Consider updating the PR label to `Type/Bug` for consistency with the issue and commit convention. 2. **Return type annotation opportunity**: `_get_tool_registry_service()` still returns `-> Any`. Since the function now simply delegates to the container, it could return `-> ToolRegistryService` for better type safety. This is pre-existing and non-blocking, but would be a nice improvement if addressed. 3. **container.py file size**: At ~36KB / ~600+ lines, `container.py` continues to grow. This is a pre-existing concern — the PR adds only ~30 lines following established patterns, so it's not a blocker here. Worth noting for future architectural consideration. **Decision: APPROVED** ✅ The implementation is clean, follows established patterns, and the concurrency implications are actually positive (shared connection pool via Singleton vs. per-call engine creation). No race conditions, deadlocks, or thread-safety issues identified. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer

freemo referenced this pull request

2026-04-05 17:45:54 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

2026-04-05 17:51:27 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo force-pushed fix/bug-hunt-manual-di-get-tool-registry-service from 7c3dd1541b to 32b352832f

2026-04-05 17:53:17 +00:00

Compare

freemo commented

2026-04-05 17:53:28 +00:00

PR Status Update

Analyzed PR #3325 status:

Review: One APPROVED review from freemo (posted as COMMENT due to Forgejo self-review restriction). Needs 1 more approving review per project policy (2+ required).

CI Status (previous run):

✅ lint, typecheck, security, quality, unit_tests, integration_tests, coverage, build, docker, helm — all passing
❌ e2e_tests — failing after 19m57s
❌ status-check — failing (depends on e2e_tests)

Action taken: Rebased onto latest master (1783f0a2) to pick up recent changes and trigger a fresh CI run. The e2e failure may have been a flaky test or related to master drift — the PR's changes (DI wiring in container.py and validation.py) do not touch any e2e test files.

Monitoring new CI run for commit 32b35283.

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

## PR Status Update Analyzed PR #3325 status: **Review**: One APPROVED review from `freemo` (posted as COMMENT due to Forgejo self-review restriction). Needs 1 more approving review per project policy (2+ required). **CI Status** (previous run): - ✅ lint, typecheck, security, quality, unit_tests, integration_tests, coverage, build, docker, helm — all passing - ❌ e2e_tests — failing after 19m57s - ❌ status-check — failing (depends on e2e_tests) **Action taken**: Rebased onto latest master (`1783f0a2`) to pick up recent changes and trigger a fresh CI run. The e2e failure may have been a flaky test or related to master drift — the PR's changes (DI wiring in container.py and validation.py) do not touch any e2e test files. Monitoring new CI run for commit `32b35283`. --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker

freemo reviewed 2026-04-05 19:38:33 +00:00

freemo left a comment

Review Summary (architecture-alignment, module-boundaries, interface-contracts)

Reviewed PR #3325 with focus on architecture-alignment, module-boundaries, and interface-contracts.

This PR replaces manual DI wiring in _get_tool_registry_service() with proper container resolution, fixing a consistency anti-pattern identified in issue #3006. The change is clean, well-scoped, and correctly follows established project patterns.

Reviewer verdict: APPROVED ✅ (posted as COMMENT due to Forgejo self-review restriction)

✅ Architecture Alignment (Deep Dive)

Given special attention to architectural consistency:

Builder function pattern: _build_tool_registry_service(database_url) follows the exact same pattern as _build_skill_service, _build_session_service, _build_automation_profile_service, _build_trace_service, and other DB-backed service builders. The signature (database_url: str → ToolRegistryService), the lazy SQLAlchemy imports, and the engine/sessionmaker/repository construction are all consistent.
Singleton registration: tool_registry_service is registered as providers.Singleton in the Container class, matching the skill_service pattern. The justification is sound — registry services share a single DB connection pool within a process lifetime. This is actually a concurrency improvement over the old code, which called create_engine() on every invocation.
No in-memory fallback: The deliberate omission of an in-memory fallback (unlike _build_skill_service) is well-reasoned and documented. ToolRegistryService requires a real database; a fallback would be misleading. Fail-fast is the correct behavior here, consistent with the project's error handling principles.
ADR-003 compliance: The change correctly centralizes service wiring in the DI container per ADR-003 (Dependency Injection Framework).

✅ Module Boundaries (Deep Dive)

This PR fixes a module boundary violation:

Before: cli/commands/validation.py (presentation layer) was importing infrastructure concerns: create_engine, sessionmaker, ToolRegistryRepository, ValidationAttachmentRepository. This violated the layered architecture by having the CLI layer directly depend on infrastructure internals.
After: cli/commands/validation.py only imports get_container from the application layer. All infrastructure wiring is properly encapsulated in container.py (application layer). The builder function correctly imports from the infrastructure layer — this is the right place for those imports.
Consistency with existing patterns: _get_tool_registry_service() is now a one-liner (return get_container().tool_registry_service()), consistent with _get_skill_service() in cli/commands/skill.py. The CLI layer uniformly delegates to the container.

✅ Interface Contracts (Deep Dive)

Service constructor contract: _build_tool_registry_service constructs ToolRegistryService with tool_repo and attachment_repo parameters, correctly satisfying the service's constructor interface.
Container provider contract: The Container class exposes tool_registry_service as a callable Singleton provider, consistent with how all other services are exposed. Callers resolve via container.tool_registry_service().
CLI helper contract: _get_tool_registry_service() -> Any maintains the same return type as before. The Any return type is pre-existing and not introduced by this PR.

✅ CONTRIBUTING.md Compliance

Commit message: fix(cli): replace manual DI wiring in _get_tool_registry_service with container resolution — correct Conventional Changelog format ✅
Issue linking: ISSUES CLOSED: #3006 in commit footer, Closes #3006 in PR body ✅
Milestone: v3.3.0 assigned (matches issue) ✅
Label: Type/Bug ✅
Branch name: fix/bug-hunt-manual-di-get-tool-registry-service (matches issue metadata) ✅
Single atomic commit: Yes ✅
No # type: ignore added: Confirmed ✅

✅ Test Quality

TDD Scenario 1: Verifies _get_tool_registry_service delegates to container.tool_registry_service() — properly tests the behavioral contract with mock isolation via unittest.mock.patch. Also verifies container.database_url() was NOT called, confirming no manual DI pattern remains.
TDD Scenario 2: Verifies the Container class exposes a tool_registry_service provider using the real Container class — good structural/integration-level verification.
Updated existing tests: validation_cli_uncovered_branches_steps.py correctly adapts mock targets to the new container-delegation pattern, ensuring existing branch-coverage scenarios remain aligned.
Test location: All tests in features/ directory using Behave/Gherkin ✅

Minor Suggestions (Non-blocking)

Return type annotation opportunity: _get_tool_registry_service() still returns -> Any. Since the function now simply delegates to the container, it could return -> ToolRegistryService for better type safety. This is pre-existing and non-blocking.
container.py file size: At ~600+ lines, container.py exceeds the 500-line guideline. This is a pre-existing concern — the PR adds only ~30 lines following established patterns. Worth noting for future architectural consideration (e.g., splitting into sub-modules by domain area).

Decision: APPROVED ✅

The implementation is clean, follows established patterns exactly, and correctly fixes a module boundary violation. Architecture alignment is excellent — the builder function, Singleton registration, and CLI delegation all match the project's established conventions. No issues found in the focus areas.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Review Summary (architecture-alignment, module-boundaries, interface-contracts) Reviewed PR #3325 with focus on **architecture-alignment**, **module-boundaries**, and **interface-contracts**. This PR replaces manual DI wiring in `_get_tool_registry_service()` with proper container resolution, fixing a consistency anti-pattern identified in issue #3006. The change is clean, well-scoped, and correctly follows established project patterns. **Reviewer verdict: APPROVED** ✅ (posted as COMMENT due to Forgejo self-review restriction) --- ### ✅ Architecture Alignment (Deep Dive) Given special attention to architectural consistency: - **Builder function pattern**: `_build_tool_registry_service(database_url)` follows the exact same pattern as `_build_skill_service`, `_build_session_service`, `_build_automation_profile_service`, `_build_trace_service`, and other DB-backed service builders. The signature (`database_url: str → ToolRegistryService`), the lazy SQLAlchemy imports, and the engine/sessionmaker/repository construction are all consistent. - **Singleton registration**: `tool_registry_service` is registered as `providers.Singleton` in the `Container` class, matching the `skill_service` pattern. The justification is sound — registry services share a single DB connection pool within a process lifetime. This is actually a **concurrency improvement** over the old code, which called `create_engine()` on every invocation. - **No in-memory fallback**: The deliberate omission of an in-memory fallback (unlike `_build_skill_service`) is well-reasoned and documented. `ToolRegistryService` requires a real database; a fallback would be misleading. Fail-fast is the correct behavior here, consistent with the project's error handling principles. - **ADR-003 compliance**: The change correctly centralizes service wiring in the DI container per ADR-003 (Dependency Injection Framework). ### ✅ Module Boundaries (Deep Dive) This PR **fixes** a module boundary violation: - **Before**: `cli/commands/validation.py` (presentation layer) was importing infrastructure concerns: `create_engine`, `sessionmaker`, `ToolRegistryRepository`, `ValidationAttachmentRepository`. This violated the layered architecture by having the CLI layer directly depend on infrastructure internals. - **After**: `cli/commands/validation.py` only imports `get_container` from the application layer. All infrastructure wiring is properly encapsulated in `container.py` (application layer). The builder function correctly imports from the infrastructure layer — this is the right place for those imports. - **Consistency with existing patterns**: `_get_tool_registry_service()` is now a one-liner (`return get_container().tool_registry_service()`), consistent with `_get_skill_service()` in `cli/commands/skill.py`. The CLI layer uniformly delegates to the container. ### ✅ Interface Contracts (Deep Dive) - **Service constructor contract**: `_build_tool_registry_service` constructs `ToolRegistryService` with `tool_repo` and `attachment_repo` parameters, correctly satisfying the service's constructor interface. - **Container provider contract**: The `Container` class exposes `tool_registry_service` as a callable Singleton provider, consistent with how all other services are exposed. Callers resolve via `container.tool_registry_service()`. - **CLI helper contract**: `_get_tool_registry_service() -> Any` maintains the same return type as before. The `Any` return type is pre-existing and not introduced by this PR. ### ✅ CONTRIBUTING.md Compliance - **Commit message**: `fix(cli): replace manual DI wiring in _get_tool_registry_service with container resolution` — correct Conventional Changelog format ✅ - **Issue linking**: `ISSUES CLOSED: #3006` in commit footer, `Closes #3006` in PR body ✅ - **Milestone**: v3.3.0 assigned (matches issue) ✅ - **Label**: `Type/Bug` ✅ - **Branch name**: `fix/bug-hunt-manual-di-get-tool-registry-service` (matches issue metadata) ✅ - **Single atomic commit**: Yes ✅ - **No `# type: ignore` added**: Confirmed ✅ ### ✅ Test Quality - **TDD Scenario 1**: Verifies `_get_tool_registry_service` delegates to `container.tool_registry_service()` — properly tests the behavioral contract with mock isolation via `unittest.mock.patch`. Also verifies `container.database_url()` was NOT called, confirming no manual DI pattern remains. - **TDD Scenario 2**: Verifies the `Container` class exposes a `tool_registry_service` provider using the real `Container` class — good structural/integration-level verification. - **Updated existing tests**: `validation_cli_uncovered_branches_steps.py` correctly adapts mock targets to the new container-delegation pattern, ensuring existing branch-coverage scenarios remain aligned. - **Test location**: All tests in `features/` directory using Behave/Gherkin ✅ ### Minor Suggestions (Non-blocking) 1. **Return type annotation opportunity**: `_get_tool_registry_service()` still returns `-> Any`. Since the function now simply delegates to the container, it could return `-> ToolRegistryService` for better type safety. This is pre-existing and non-blocking. 2. **container.py file size**: At ~600+ lines, `container.py` exceeds the 500-line guideline. This is a pre-existing concern — the PR adds only ~30 lines following established patterns. Worth noting for future architectural consideration (e.g., splitting into sub-modules by domain area). **Decision: APPROVED** ✅ The implementation is clean, follows established patterns exactly, and correctly fixes a module boundary violation. Architecture alignment is excellent — the builder function, Singleton registration, and CLI delegation all match the project's established conventions. No issues found in the focus areas. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer

freemo referenced this pull request

2026-04-05 19:39:43 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

2026-04-05 19:48:58 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

2026-04-05 20:07:24 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3572

freemo referenced this pull request

2026-04-05 20:13:35 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3572