docs(adr): add ADR-053 resolving database repositories monolith violation #8222
@@ -0,0 +1,299 @@
|
||||
---
|
||||
adr_number: 53
|
||||
title: "Database Repositories Decomposition"
|
||||
status_history:
|
||||
- ["2026-04-13", "Draft", "CleverThis"]
|
||||
- ["2026-04-13", "Proposed", "CleverThis"]
|
||||
tier: 2
|
||||
authors: ["CleverThis"]
|
||||
superseded_by:
|
||||
related_adrs:
|
||||
- number: 1
|
||||
title: "Layered Architecture"
|
||||
relationship: "Repository implementations belong to the Infrastructure layer; this ADR refactors their internal structure without changing their layer placement"
|
||||
- number: 3
|
||||
title: "Dependency Injection"
|
||||
relationship: "Each focused repository module is registered as a separate provider in the DI container; the decomposition enables fine-grained DI wiring"
|
||||
- number: 51
|
||||
title: "PlanLifecycleService Decomposition"
|
||||
relationship: "This ADR applies the same decomposition pattern used for PlanLifecycleService to the database repositories monolith"
|
||||
acceptance:
|
||||
votes_for: []
|
||||
votes_against: []
|
||||
abstentions: []
|
||||
---
|
||||
|
||||
## Context
|
||||
|
||||
A guard scan (issue #8210) identified that `src/cleveragents/infrastructure/database/repositories.py` spans **6,086 lines** — more than 12× the 500-line maximum mandated by `CONTRIBUTING.md` (lines 382–404).
|
||||
|
||||
The file currently mixes the following distinct repository concerns in a single module:
|
||||
|
||||
- `ProjectRepository`: project CRUD and membership management
|
||||
- `PlanRepository`: plan lifecycle persistence (create, update, phase transitions)
|
||||
- `ContextRepository`: context entry storage and retrieval
|
||||
- `SkillRepository`: skill definition and version management
|
||||
- `SessionRepository`: session lifecycle and token management
|
||||
- `ActorRepository`: actor configuration persistence
|
||||
- `ToolRepository`: tool definition and registry persistence
|
||||
- `InvariantRepository`: invariant storage and enforcement state
|
||||
- `DecisionRepository`: decision tree node persistence
|
||||
- `ResourceRepository`: resource handler configuration persistence
|
||||
- Custom exception types (`RepositoryError`, `NotFoundError`, `ConflictError`, etc.)
|
||||
- Shared data mappers and DTOs used across multiple repositories
|
||||
- Helper utilities (pagination, filtering, bulk operations)
|
||||
|
||||
This concentration of concerns makes the file impossible to review in a single sitting, creates frequent merge conflicts when unrelated repository changes touch the same file, and prevents targeted testing of individual repository implementations.
|
||||
|
||||
## Decision Drivers
|
||||
|
||||
- `CONTRIBUTING.md` mandates ≤500 lines per file; the current file is 12× over the limit
|
||||
- Each repository class manages a distinct aggregate root — they have no business being in the same file
|
||||
- Merge conflicts are frequent because unrelated changes (e.g., plan phase transitions and skill versioning) touch the same file
|
||||
- Individual repository implementations cannot be tested in isolation without importing the entire 6,086-line module
|
||||
- The DI container cannot register individual repositories with fine-grained dependency control when they are all in one file
|
||||
- The violation was detected automatically by the architecture guard; the fix must be enforceable mechanically in CI
|
||||
|
||||
## Decision
|
||||
|
||||
Decompose `repositories.py` into a `repositories/` package where each aggregate root has its own focused module, each ≤500 lines. Shared infrastructure (exceptions, base classes, mappers) is extracted into dedicated modules. The `repositories/__init__.py` re-exports all public classes for backwards compatibility.
|
||||
|
||||
## Design
|
||||
|
||||
### Target Module Structure
|
||||
|
||||
````
|
||||
cleveragents/infrastructure/database/repositories/
|
||||
__init__.py # Re-exports all public repository classes (backwards compat)
|
||||
_base.py # BaseRepository, shared query helpers, pagination utilities (≤300 lines)
|
||||
_exceptions.py # RepositoryError, NotFoundError, ConflictError, etc. (≤150 lines)
|
||||
_mappers.py # Shared data mappers and DTOs used across repositories (≤400 lines)
|
||||
project.py # ProjectRepository: project CRUD and membership (≤500 lines)
|
||||
plan.py # PlanRepository: plan lifecycle persistence (≤500 lines)
|
||||
context.py # ContextRepository: context entry storage and retrieval (≤500 lines)
|
||||
skill.py # SkillRepository: skill definition and versioning (≤500 lines)
|
||||
session.py # SessionRepository: session lifecycle and tokens (≤500 lines)
|
||||
actor.py # ActorRepository: actor configuration persistence (≤500 lines)
|
||||
tool.py # ToolRepository: tool definition and registry (≤500 lines)
|
||||
invariant.py # InvariantRepository: invariant storage and state (≤500 lines)
|
||||
decision.py # DecisionRepository: decision tree node persistence (≤500 lines)
|
||||
resource.py # ResourceRepository: resource handler configuration (≤500 lines)
|
||||
````
|
||||
|
||||
All modules use lowercase names (no `_` prefix) because they are part of the public package API — callers may import `from cleveragents.infrastructure.database.repositories.plan import PlanRepository` directly if needed. The `__init__.py` re-exports everything for backwards compatibility.
|
||||
|
||||
### Backwards Compatibility
|
||||
|
||||
The `__init__.py` re-exports all public classes so existing import paths continue to work:
|
||||
|
||||
````python
|
||||
# cleveragents/infrastructure/database/repositories/__init__.py
|
||||
|
||||
from cleveragents.infrastructure.database.repositories._exceptions import (
|
||||
ConflictError,
|
||||
NotFoundError,
|
||||
RepositoryError,
|
||||
)
|
||||
from cleveragents.infrastructure.database.repositories.actor import ActorRepository
|
||||
from cleveragents.infrastructure.database.repositories.context import ContextRepository
|
||||
from cleveragents.infrastructure.database.repositories.decision import DecisionRepository
|
||||
from cleveragents.infrastructure.database.repositories.invariant import InvariantRepository
|
||||
from cleveragents.infrastructure.database.repositories.plan import PlanRepository
|
||||
from cleveragents.infrastructure.database.repositories.project import ProjectRepository
|
||||
from cleveragents.infrastructure.database.repositories.resource import ResourceRepository
|
||||
from cleveragents.infrastructure.database.repositories.session import SessionRepository
|
||||
from cleveragents.infrastructure.database.repositories.skill import SkillRepository
|
||||
from cleveragents.infrastructure.database.repositories.tool import ToolRepository
|
||||
|
||||
__all__ = [
|
||||
"ActorRepository",
|
||||
"ConflictError",
|
||||
"ContextRepository",
|
||||
"DecisionRepository",
|
||||
"InvariantRepository",
|
||||
"NotFoundError",
|
||||
"PlanRepository",
|
||||
"ProjectRepository",
|
||||
"RepositoryError",
|
||||
"ResourceRepository",
|
||||
"SessionRepository",
|
||||
"SkillRepository",
|
||||
"ToolRepository",
|
||||
]
|
||||
````
|
||||
|
||||
Existing code that imports `from cleveragents.infrastructure.database.repositories import PlanRepository` continues to work without modification.
|
||||
|
||||
### Shared Infrastructure Modules
|
||||
|
||||
**`_base.py`** — Contains `BaseRepository` with shared query helpers:
|
||||
|
||||
````python
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Generic, TypeVar
|
||||
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
T = TypeVar("T")
|
||||
|
||||
|
||||
class BaseRepository(Generic[T]):
|
||||
"""Base class for all SQLAlchemy repository implementations.
|
||||
|
||||
Provides shared session management, pagination helpers, and
|
||||
common query patterns used across all repository implementations.
|
||||
"""
|
||||
|
||||
def __init__(self, session: Session) -> None:
|
||||
self._session = session
|
||||
|
||||
def _paginate(self, query, page: int, page_size: int):
|
||||
"""Apply pagination to a SQLAlchemy query."""
|
||||
return query.offset((page - 1) * page_size).limit(page_size)
|
||||
|
||||
def _require(self, record, identifier: str) -> T:
|
||||
"""Raise NotFoundError if record is None."""
|
||||
if record is None:
|
||||
raise NotFoundError(f"Record not found: {identifier!r}")
|
||||
return record
|
||||
````
|
||||
|
||||
**`_exceptions.py`** — Contains all repository exception types:
|
||||
|
||||
````python
|
||||
class RepositoryError(Exception):
|
||||
"""Base class for all repository errors."""
|
||||
|
||||
class NotFoundError(RepositoryError):
|
||||
"""Raised when a requested record does not exist."""
|
||||
|
||||
class ConflictError(RepositoryError):
|
||||
"""Raised when an operation would violate a uniqueness constraint."""
|
||||
|
||||
class StaleDataError(RepositoryError):
|
||||
"""Raised when optimistic locking detects a concurrent modification."""
|
||||
````
|
||||
|
||||
**`_mappers.py`** — Contains shared data mappers used by multiple repositories:
|
||||
|
||||
````python
|
||||
# Shared mapper utilities for converting between ORM records and domain objects.
|
||||
# Mappers that are used by only one repository belong in that repository's module.
|
||||
````
|
||||
|
||||
### Module Responsibilities
|
||||
|
||||
| Module | Repository Class | Aggregate Root | Key Operations |
|
||||
|--------|-----------------|----------------|----------------|
|
||||
| `project.py` | `ProjectRepository` | Project | CRUD, membership, settings |
|
||||
| `plan.py` | `PlanRepository` | Plan | lifecycle, phase transitions, ULID lookup |
|
||||
| `context.py` | `ContextRepository` | ContextEntry | add, search, budget enforcement |
|
||||
| `skill.py` | `SkillRepository` | Skill | definition, versioning, activation |
|
||||
| `session.py` | `SessionRepository` | Session | lifecycle, token management |
|
||||
| `actor.py` | `ActorRepository` | Actor | configuration, provider binding |
|
||||
| `tool.py` | `ToolRepository` | Tool | definition, registry, capability lookup |
|
||||
| `invariant.py` | `InvariantRepository` | Invariant | storage, enforcement state, precedence |
|
||||
| `decision.py` | `DecisionRepository` | DecisionNode | tree persistence, subtree queries |
|
||||
| `resource.py` | `ResourceRepository` | Resource | handler config, sandbox state |
|
||||
|
||||
### DI Container Update
|
||||
|
||||
Each repository is registered as a separate provider:
|
||||
|
||||
````python
|
||||
# cleveragents/infrastructure/di/container.py
|
||||
|
||||
from cleveragents.infrastructure.database.repositories import (
|
||||
ActorRepository,
|
||||
ContextRepository,
|
||||
DecisionRepository,
|
||||
InvariantRepository,
|
||||
PlanRepository,
|
||||
ProjectRepository,
|
||||
ResourceRepository,
|
||||
SessionRepository,
|
||||
SkillRepository,
|
||||
ToolRepository,
|
||||
)
|
||||
|
||||
class Container(DeclarativeContainer):
|
||||
# Individual repository providers
|
||||
project_repo = providers.Factory(ProjectRepository, session=db_session)
|
||||
plan_repo = providers.Factory(PlanRepository, session=db_session)
|
||||
context_repo = providers.Factory(ContextRepository, session=db_session)
|
||||
skill_repo = providers.Factory(SkillRepository, session=db_session)
|
||||
session_repo = providers.Factory(SessionRepository, session=db_session)
|
||||
actor_repo = providers.Factory(ActorRepository, session=db_session)
|
||||
tool_repo = providers.Factory(ToolRepository, session=db_session)
|
||||
invariant_repo = providers.Factory(InvariantRepository, session=db_session)
|
||||
decision_repo = providers.Factory(DecisionRepository, session=db_session)
|
||||
resource_repo = providers.Factory(ResourceRepository, session=db_session)
|
||||
````
|
||||
|
||||
### Migration Strategy
|
||||
|
||||
The decomposition follows the same three-phase strategy as ADR-051:
|
||||
|
||||
1. **Phase 1 — Extract without behavior change**: Move code verbatim into the new modules. The `__init__.py` re-exports all public classes. All existing tests must pass without modification.
|
||||
2. **Phase 2 — Refactor internals**: Clean up internal APIs within each module (rename private methods, remove dead code, improve type annotations). Tests are updated to import from focused modules directly.
|
||||
3. **Phase 3 — Add focused tests**: Write BDD scenarios targeting each repository in isolation using an in-memory SQLite database.
|
||||
|
||||
Each phase is a separate PR. Phase 1 is a pure structural refactor with no behavior changes.
|
||||
|
||||
### File Size Enforcement
|
||||
|
||||
The CI `lint` job includes a file size check that fails if any file in `cleveragents/infrastructure/database/repositories/` exceeds 500 lines. This prevents the monolith from re-emerging over time.
|
||||
|
||||
## Constraints
|
||||
|
||||
- The public API of all repository classes (method names, signatures, return types) must remain unchanged — no breaking changes to callers
|
||||
- Each module in `cleveragents/infrastructure/database/repositories/` must be ≤500 lines (enforced by CI)
|
||||
- The `__init__.py` must re-export all public classes for backwards compatibility — existing import paths must continue to work
|
||||
- The original `repositories.py` file must be deleted after Phase 1 is complete; it must not coexist with the new package
|
||||
- All existing BDD scenarios for repository operations must continue to pass after Phase 1
|
||||
- `_base.py`, `_exceptions.py`, and `_mappers.py` are prefixed with `_` to signal they are internal implementation details
|
||||
|
||||
## Consequences
|
||||
|
||||
### Positive
|
||||
|
||||
- Each module is ≤500 lines, compliant with `CONTRIBUTING.md`
|
||||
- Each repository is independently reviewable, testable, and assignable to contributors
|
||||
- Merge conflicts are dramatically reduced — changes to plan persistence no longer touch the same file as skill versioning
|
||||
- Each repository can be mocked independently in unit tests
|
||||
- The DI container can wire individual repositories with fine-grained dependency control
|
||||
- New aggregate roots can be added as new modules without modifying existing ones
|
||||
- `PlanRepository` (which implements the ULID → integer lookup from ADR-050) is isolated in its own module
|
||||
|
||||
### Negative
|
||||
|
||||
- The decomposition PR (Phase 1) is a large structural change that touches many files and requires careful review
|
||||
- Developers must navigate multiple files to understand all repository implementations
|
||||
- The `__init__.py` re-export list must be kept in sync as new repositories are added
|
||||
- Import paths change from `cleveragents.infrastructure.database.repositories` (module) to `cleveragents.infrastructure.database.repositories` (package) — Python handles this transparently, but IDE tooling may need cache invalidation
|
||||
|
||||
### Risks
|
||||
|
||||
- **Behavior regression during extraction**: Moving code verbatim may inadvertently break shared state or import ordering. Mitigation: Phase 1 is a pure structural refactor; all existing tests must pass before Phase 1 is merged.
|
||||
- **Re-emergence of the monolith**: Without enforcement, focused modules may grow back toward 500 lines. Mitigation: CI file size check fails the build if any module exceeds the limit.
|
||||
- **Incomplete extraction**: If some code is left in the original file and the new package is created alongside it, two sources of truth exist. Mitigation: the original `repositories.py` must be deleted in Phase 1; CI import checks verify no code imports from the old path.
|
||||
- **Mapper ownership ambiguity**: Some data mappers may be used by multiple repositories, making it unclear which module should own them. Mitigation: shared mappers go in `_mappers.py`; repository-specific mappers go in the repository's module.
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
**Increase the file size limit for this file** — Rejected. The 500-line limit exists to enforce cohesion and reviewability. Granting an exception for the most complex infrastructure file would undermine the rule.
|
||||
|
||||
**Split into two files (repositories_core.py and repositories_extended.py)** — Rejected. A two-file split does not address the mixed-concern problem. The aggregate-root-based decomposition into focused modules is the correct granularity.
|
||||
|
||||
**Rewrite from scratch** — Rejected. A rewrite introduces high risk of behavior regression. The phased extraction approach preserves existing behavior while improving structure.
|
||||
|
||||
## Compliance
|
||||
|
||||
- **File size check**: CI `lint` job must assert that no file in `cleveragents/infrastructure/database/repositories/` exceeds 500 lines.
|
||||
- **Import check**: CI must assert that `src/cleveragents/infrastructure/database/repositories.py` does not exist after Phase 1 is merged.
|
||||
- **Backwards compatibility**: All existing import paths (`from cleveragents.infrastructure.database.repositories import PlanRepository`) must continue to work after Phase 1.
|
||||
- **Regression tests**: All existing BDD scenarios for repository operations must pass after Phase 1 without modification.
|
||||
- **Focused repository tests**: After Phase 3, each repository module must have at least one BDD scenario that exercises it in isolation with an in-memory SQLite database.
|
||||
|
||||
Closes #8210
|
||||
Reference in New Issue
Block a user