bug(cli): automation_profile._get_service() bypasses DI container #1181

Merged
freemo merged 1 commit from bugfix/m4-automation-profile-di-bypass into master 2026-04-02 17:54:14 +00:00
Member

Summary

  • Route automation_profile._get_service() through DI by resolving container.automation_profile_service() instead of manually constructing SQLAlchemy engine/session/repository.
  • Add DI wiring in application.container via _build_automation_profile_service() and automation_profile_service provider for consistent CLI service lifecycle/configuration behavior.
  • Strengthen coverage with both delegation assertions and a non-mocked integration-style proof that _get_service() resolves through real container/provider wiring.
  • Keep PR #990 scope clean by removing unrelated robot/resource_dag.robot changes and consolidating overlapping _get_service() test coverage scenarios.

Validation

  • nox -e lint
  • nox -e typecheck
  • nox -e unit_tests
  • nox -e coverage_report (summary coverage: 97%)
  • nox -e integration_tests (existing integration-suite failures/timeouts in unrelated workflows)
  • nox -e e2e_tests (existing E2E suite failures unrelated to #990 scope)

Closes #990

## Summary - Route `automation_profile._get_service()` through DI by resolving `container.automation_profile_service()` instead of manually constructing SQLAlchemy engine/session/repository. - Add DI wiring in `application.container` via `_build_automation_profile_service()` and `automation_profile_service` provider for consistent CLI service lifecycle/configuration behavior. - Strengthen coverage with both delegation assertions and a non-mocked integration-style proof that `_get_service()` resolves through real container/provider wiring. - Keep PR #990 scope clean by removing unrelated `robot/resource_dag.robot` changes and consolidating overlapping `_get_service()` test coverage scenarios. ## Validation - `nox -e lint` ✅ - `nox -e typecheck` ✅ - `nox -e unit_tests` ✅ - `nox -e coverage_report` ✅ (summary coverage: 97%) - `nox -e integration_tests` ❌ (existing integration-suite failures/timeouts in unrelated workflows) - `nox -e e2e_tests` ❌ (existing E2E suite failures unrelated to #990 scope) Closes #990
brent.edwards added this to the v3.3.0 milestone 2026-03-28 23:12:49 +00:00
brent.edwards force-pushed bugfix/m4-automation-profile-di-bypass from cab36ef8f8
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / security (pull_request) Successful in 58s
CI / lint (pull_request) Successful in 3m19s
CI / build (pull_request) Successful in 20s
CI / helm (pull_request) Successful in 21s
CI / typecheck (pull_request) Successful in 3m53s
CI / quality (pull_request) Successful in 3m42s
CI / integration_tests (pull_request) Successful in 6m11s
CI / unit_tests (pull_request) Successful in 9m40s
CI / docker (pull_request) Successful in 1m22s
CI / e2e_tests (pull_request) Successful in 11m58s
CI / coverage (pull_request) Successful in 13m26s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-regression (pull_request) Successful in 1h1m56s
to d16e9963ce
All checks were successful
CI / lint (pull_request) Successful in 3m21s
CI / quality (pull_request) Successful in 3m47s
CI / typecheck (pull_request) Successful in 4m3s
CI / security (pull_request) Successful in 4m10s
CI / build (pull_request) Successful in 34s
CI / helm (pull_request) Successful in 35s
CI / unit_tests (pull_request) Successful in 4m22s
CI / integration_tests (pull_request) Successful in 4m5s
CI / e2e_tests (pull_request) Successful in 9m59s
CI / benchmark-publish (pull_request) Has been skipped
CI / docker (pull_request) Successful in 1m19s
CI / coverage (pull_request) Successful in 11m54s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-regression (pull_request) Successful in 56m51s
2026-03-30 01:07:26 +00:00
Compare
freemo approved these changes 2026-03-30 04:22:15 +00:00
Dismissed
freemo left a comment

Review: APPROVED

Excellent DI alignment refactoring. Properly centralizes AutomationProfileService construction in the container and tests delegation behavior rather than implementation details. Clean application of the Dependency Inversion Principle.

## Review: APPROVED Excellent DI alignment refactoring. Properly centralizes `AutomationProfileService` construction in the container and tests delegation behavior rather than implementation details. Clean application of the Dependency Inversion Principle.
freemo self-assigned this 2026-04-02 06:15:17 +00:00
freemo force-pushed bugfix/m4-automation-profile-di-bypass from d16e9963ce
All checks were successful
CI / lint (pull_request) Successful in 3m21s
CI / quality (pull_request) Successful in 3m47s
CI / typecheck (pull_request) Successful in 4m3s
CI / security (pull_request) Successful in 4m10s
CI / build (pull_request) Successful in 34s
CI / helm (pull_request) Successful in 35s
CI / unit_tests (pull_request) Successful in 4m22s
CI / integration_tests (pull_request) Successful in 4m5s
CI / e2e_tests (pull_request) Successful in 9m59s
CI / benchmark-publish (pull_request) Has been skipped
CI / docker (pull_request) Successful in 1m19s
CI / coverage (pull_request) Successful in 11m54s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-regression (pull_request) Successful in 56m51s
to 601f408860
Some checks failed
CI / lint (pull_request) Successful in 23s
CI / typecheck (pull_request) Successful in 54s
CI / quality (pull_request) Successful in 3m44s
CI / security (pull_request) Successful in 4m5s
CI / build (pull_request) Successful in 17s
CI / helm (pull_request) Successful in 23s
CI / unit_tests (pull_request) Successful in 6m39s
CI / docker (pull_request) Successful in 1m23s
CI / coverage (pull_request) Successful in 13m11s
CI / e2e_tests (pull_request) Successful in 17m23s
CI / integration_tests (pull_request) Successful in 21m36s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 36m14s
2026-04-02 06:54:44 +00:00
Compare
freemo dismissed freemo's review 2026-04-02 06:54:44 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Owner

🔒 Claimed by pr-reviewer-1. Starting independent code review.

🔒 Claimed by pr-reviewer-1. Starting independent code review.
freemo approved these changes 2026-04-02 08:13:27 +00:00
Dismissed
freemo left a comment

Independent Code Review: APPROVED

Summary

Reviewed the complete diff (4 files, +136/-92 lines) against the specification and project conventions.

What was reviewed

Core Fix — _get_service() in automation_profile.py:

  • Correctly replaces manual create_engine/sessionmaker/AutomationProfileRepository construction with container.automation_profile_service() delegation
  • The old code was the only CLI command that bypassed the DI container — this fix brings it into alignment with all other CLI commands
  • Clean, minimal change: 17 lines removed, 3 lines added

Container Wiring — container.py:

  • New _build_automation_profile_service() factory function follows the exact same pattern as _build_skill_service, _build_session_service, etc.
  • automation_profile_service registered as providers.Factory — appropriate for CLI per-invocation lifecycle
  • Import of AutomationProfileService correctly placed at module top level

Test Quality — Behave scenarios:

  • Tests shifted from testing implementation details (sessionmaker binding, auto_commit flag) to testing behavior (delegation, provider call count, no direct database_url access)
  • Integration-style scenario 5 uses a real DI container with temp SQLite DB — strong end-to-end proof
  • All 5 scenarios are meaningful and well-structured

Verification Checklist

  • Spec alignment: DI container pattern matches specification requirements
  • Commit message: Conventional Changelog format with ISSUES CLOSED: #990 footer
  • No # type: ignore introduced (pre-existing ones in get_ai_provider are unrelated)
  • No secrets or credentials in code
  • automation_profile.py at 473 lines (under 500-line limit)
  • PR body has Closes #990, milestone v3.3.0, Type/Bug label
  • Single atomic commit — merge (not squash) appropriate
  • PR is mergeable with no conflicts

Note

container.py is at 921 lines (exceeds 500-line limit), but this is a pre-existing condition — the PR adds only 29 lines to an already-oversized file. Not a blocker for this PR.

## Independent Code Review: APPROVED ✅ ### Summary Reviewed the complete diff (4 files, +136/-92 lines) against the specification and project conventions. ### What was reviewed **Core Fix — `_get_service()` in `automation_profile.py`:** - Correctly replaces manual `create_engine`/`sessionmaker`/`AutomationProfileRepository` construction with `container.automation_profile_service()` delegation - The old code was the only CLI command that bypassed the DI container — this fix brings it into alignment with all other CLI commands - Clean, minimal change: 17 lines removed, 3 lines added **Container Wiring — `container.py`:** - New `_build_automation_profile_service()` factory function follows the exact same pattern as `_build_skill_service`, `_build_session_service`, etc. - `automation_profile_service` registered as `providers.Factory` — appropriate for CLI per-invocation lifecycle - Import of `AutomationProfileService` correctly placed at module top level **Test Quality — Behave scenarios:** - Tests shifted from testing implementation details (sessionmaker binding, auto_commit flag) to testing behavior (delegation, provider call count, no direct database_url access) - Integration-style scenario 5 uses a real DI container with temp SQLite DB — strong end-to-end proof - All 5 scenarios are meaningful and well-structured ### Verification Checklist - [x] Spec alignment: DI container pattern matches specification requirements - [x] Commit message: Conventional Changelog format with `ISSUES CLOSED: #990` footer - [x] No `# type: ignore` introduced (pre-existing ones in `get_ai_provider` are unrelated) - [x] No secrets or credentials in code - [x] `automation_profile.py` at 473 lines (under 500-line limit) - [x] PR body has `Closes #990`, milestone v3.3.0, Type/Bug label - [x] Single atomic commit — merge (not squash) appropriate - [x] PR is mergeable with no conflicts ### Note `container.py` is at 921 lines (exceeds 500-line limit), but this is a pre-existing condition — the PR adds only 29 lines to an already-oversized file. Not a blocker for this PR.
Owner

Review claimed by reviewer pool instance reviewer-pool-2. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-2. Dispatching independent code review.
Owner

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.
freemo approved these changes 2026-04-02 17:53:44 +00:00
freemo left a comment

Independent Code Review: APPROVED

Review Scope

Reviewed the complete diff (4 files changed) against the specification, CONTRIBUTING.md conventions, and DI container patterns established in the codebase.

Findings

Core Fix — _get_service() in automation_profile.py:

  • Correctly eliminates the only CLI command that bypassed the DI container by replacing manual create_engine/sessionmaker/AutomationProfileRepository construction with a single container.automation_profile_service() call
  • The fix is minimal and surgical: 17 lines removed, 3 lines of clean delegation added
  • Fully consistent with how every other CLI command resolves services

Container Wiring — container.py:

  • _build_automation_profile_service() follows the identical pattern used by _build_skill_service, _build_session_service, _build_repo_indexing_service, etc.
  • Registered as providers.Factory — correct for CLI per-invocation lifecycle (no stale state between commands)
  • Import of AutomationProfileService correctly placed at module top level alongside other service imports

Test Quality — 5 BDD Scenarios:

  1. Service identity verification (same object from container) — tests delegation, not implementation
  2. Provider called exactly once — ensures no redundant resolution
  3. No direct database_url access — proves the old bypass path is gone
  4. Working DB-backed service via mock container — functional proof
  5. Real DI container with temp SQLite — integration-style end-to-end proof without mocking

Tests appropriately shifted from testing implementation details (sessionmaker binding, auto_commit flags) to testing behavioral contracts. Proper cleanup of temp files via context.add_cleanup().

Compliance Checklist

  • Specification alignment: DI container pattern matches spec requirements
  • Commit message: Conventional Changelog format with ISSUES CLOSED: #990 footer
  • No # type: ignore introduced
  • No secrets or credentials in code
  • automation_profile.py at ~473 lines (under 500-line limit)
  • PR body has Closes #990, milestone v3.3.0, Type/Bug label
  • Single atomic commit — merge strategy appropriate
  • PR is mergeable with no conflicts

Note

container.py exceeds the 500-line limit at ~921 lines, but this is a pre-existing condition. This PR adds only ~29 lines following established patterns — not a blocker.

Approving for merge.

## Independent Code Review: APPROVED ✅ ### Review Scope Reviewed the complete diff (4 files changed) against the specification, CONTRIBUTING.md conventions, and DI container patterns established in the codebase. ### Findings **Core Fix — `_get_service()` in `automation_profile.py`:** - Correctly eliminates the only CLI command that bypassed the DI container by replacing manual `create_engine`/`sessionmaker`/`AutomationProfileRepository` construction with a single `container.automation_profile_service()` call - The fix is minimal and surgical: 17 lines removed, 3 lines of clean delegation added - Fully consistent with how every other CLI command resolves services **Container Wiring — `container.py`:** - `_build_automation_profile_service()` follows the identical pattern used by `_build_skill_service`, `_build_session_service`, `_build_repo_indexing_service`, etc. - Registered as `providers.Factory` — correct for CLI per-invocation lifecycle (no stale state between commands) - Import of `AutomationProfileService` correctly placed at module top level alongside other service imports **Test Quality — 5 BDD Scenarios:** 1. Service identity verification (same object from container) — tests delegation, not implementation 2. Provider called exactly once — ensures no redundant resolution 3. No direct `database_url` access — proves the old bypass path is gone 4. Working DB-backed service via mock container — functional proof 5. Real DI container with temp SQLite — integration-style end-to-end proof without mocking Tests appropriately shifted from testing implementation details (sessionmaker binding, auto_commit flags) to testing behavioral contracts. Proper cleanup of temp files via `context.add_cleanup()`. ### Compliance Checklist - [x] Specification alignment: DI container pattern matches spec requirements - [x] Commit message: Conventional Changelog format with `ISSUES CLOSED: #990` footer - [x] No `# type: ignore` introduced - [x] No secrets or credentials in code - [x] `automation_profile.py` at ~473 lines (under 500-line limit) - [x] PR body has `Closes #990`, milestone v3.3.0, Type/Bug label - [x] Single atomic commit — merge strategy appropriate - [x] PR is mergeable with no conflicts ### Note `container.py` exceeds the 500-line limit at ~921 lines, but this is a pre-existing condition. This PR adds only ~29 lines following established patterns — not a blocker. Approving for merge.
freemo merged commit 2a266929fd into master 2026-04-02 17:54:14 +00:00
freemo deleted branch bugfix/m4-automation-profile-di-bypass 2026-04-02 17:54:14 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveragents-core!1181
No description provided.