From bb930dbc08f03f49ba9a0d8991d937e8f0efb644 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Mon, 13 Apr 2026 04:46:55 +0000 Subject: [PATCH] docs(adr): add ADR-053 database repositories decomposition --- ...053-database-repositories-decomposition.md | 299 ++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 docs/adr/ADR-053-database-repositories-decomposition.md diff --git a/docs/adr/ADR-053-database-repositories-decomposition.md b/docs/adr/ADR-053-database-repositories-decomposition.md new file mode 100644 index 000000000..8660b733d --- /dev/null +++ b/docs/adr/ADR-053-database-repositories-decomposition.md @@ -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 -- 2.52.0