diff --git a/docs/adr/ADR-049-sandbox-gateway-domain-abstraction.md b/docs/adr/ADR-049-sandbox-gateway-domain-abstraction.md new file mode 100644 index 000000000..629b43ab3 --- /dev/null +++ b/docs/adr/ADR-049-sandbox-gateway-domain-abstraction.md @@ -0,0 +1,255 @@ +--- +adr_number: 49 +title: "SandboxGateway — Domain-Layer Sandbox Abstraction" +status_history: + - ["2026-04-13", "Draft", "CleverThis"] + - ["2026-04-13", "Proposed", "CleverThis"] +tier: 1 +authors: ["CleverThis"] +superseded_by: +related_adrs: + - number: 1 + title: "Layered Architecture" + relationship: "This ADR enforces the downward-only dependency rule established in ADR-001; the domain layer must never import from infrastructure" + - number: 8 + title: "Resource System" + relationship: "Resource handlers are the primary consumers of SandboxGateway; this ADR defines the domain-layer abstraction they must depend on" + - number: 15 + title: "Sandbox and Checkpoint" + relationship: "SandboxManager (infrastructure) implements SandboxGateway; checkpoint and rollback operations are exposed through the gateway protocol" +acceptance: + votes_for: [] + votes_against: [] + abstentions: [] +--- + +## Context + +The `resource` package (`cleveragents.resource`) belongs to the **Domain layer** as documented in `docs/architecture.md`. The Clean Architecture dependency rule (ADR-001) requires that the Domain layer never imports from the Infrastructure layer — dependencies flow downward only. + +A guard scan (issue #8058) identified the following violations: + +- `src/cleveragents/resource/handlers/protocol.py` lines 41–43 import `SandboxManager` from `cleveragents.infrastructure.sandbox.manager` +- `src/cleveragents/resource/handlers/_base.py` lines 22–24 import from the same infrastructure module +- `src/cleveragents/resource/handlers/devcontainer.py` lines 57–58 also import from infrastructure + +These imports couple the domain layer to a concrete infrastructure class that has file-system and OS-level dependencies, making resource handlers impossible to test without a real sandbox environment and violating the foundational architectural contract. + +## Decision Drivers + +- ADR-001 mandates that outer layers depend on inner layers, never the reverse; domain must not import from infrastructure +- Resource handlers must be unit-testable without a real sandbox environment +- The `SandboxManager` infrastructure class carries OS-level dependencies (file system, subprocess, Git worktree) that are inappropriate in the domain layer +- The violation was detected automatically by the architecture guard; the fix must be enforceable mechanically in CI +- The domain layer must remain portable across deployment modes (local, server) without infrastructure coupling + +## Decision + +Introduce a `SandboxGateway` **protocol** in the domain layer (`cleveragents.resource.sandbox_gateway`). All resource handlers depend exclusively on this protocol. The infrastructure `SandboxManager` is adapted to implement `SandboxGateway` via a thin adapter class (`SandboxManagerGateway`) in the infrastructure layer. The DI container wires the concrete implementation at composition time. + +## Design + +### SandboxGateway Protocol + +The protocol is defined in `cleveragents/resource/sandbox_gateway.py` (domain layer): + +```python +from __future__ import annotations + +from dataclasses import dataclass +from typing import Protocol, runtime_checkable + + +@dataclass(frozen=True) +class SandboxContext: + """Domain-level value object representing an active sandbox session. + + This is intentionally infrastructure-free — no file paths, no OS handles. + Infrastructure details are encapsulated inside the SandboxGateway implementation. + """ + sandbox_id: str + resource_id: str + + +@dataclass(frozen=True) +class CheckpointRef: + """Opaque reference to a sandbox checkpoint. + + The domain layer treats this as an opaque token; the infrastructure + implementation knows how to resolve it to a concrete snapshot. + """ + ref_id: str + + +@runtime_checkable +class SandboxGateway(Protocol): + """Port: domain-layer abstraction over sandbox lifecycle operations. + + Resource handlers depend on this protocol. The infrastructure layer + provides the concrete adapter (SandboxManagerGateway). + """ + + def create_sandbox(self, resource_id: str) -> SandboxContext: + """Create and activate a sandbox for the given resource. + + Args: + resource_id: The domain identifier of the resource being sandboxed. + + Returns: + A SandboxContext value object identifying the active sandbox session. + """ + ... + + def checkpoint(self, sandbox_id: str) -> CheckpointRef: + """Record a checkpoint of the current sandbox state. + + Args: + sandbox_id: The sandbox session identifier from SandboxContext.sandbox_id. + + Returns: + An opaque CheckpointRef that can be passed to rollback(). + """ + ... + + def rollback(self, sandbox_id: str, ref: CheckpointRef) -> None: + """Restore the sandbox to a previously recorded checkpoint. + + Args: + sandbox_id: The sandbox session identifier. + ref: The checkpoint reference returned by a prior checkpoint() call. + """ + ... + + def destroy(self, sandbox_id: str) -> None: + """Tear down and release all resources associated with the sandbox. + + Args: + sandbox_id: The sandbox session identifier to destroy. + """ + ... +``` + +### Infrastructure Adapter + +`SandboxManagerGateway` lives in `cleveragents/infrastructure/sandbox/gateway_adapter.py`: + +```python +from cleveragents.infrastructure.sandbox.manager import SandboxManager +from cleveragents.resource.sandbox_gateway import CheckpointRef, SandboxContext + + +class SandboxManagerGateway: + """Adapter: wraps SandboxManager to satisfy the SandboxGateway protocol.""" + + def __init__(self, manager: SandboxManager) -> None: + self._manager = manager + + def create_sandbox(self, resource_id: str) -> SandboxContext: + internal_id = self._manager.create(resource_id) + return SandboxContext(sandbox_id=internal_id, resource_id=resource_id) + + def checkpoint(self, sandbox_id: str) -> CheckpointRef: + ref_id = self._manager.snapshot(sandbox_id) + return CheckpointRef(ref_id=ref_id) + + def rollback(self, sandbox_id: str, ref: CheckpointRef) -> None: + self._manager.restore(sandbox_id, ref.ref_id) + + def destroy(self, sandbox_id: str) -> None: + self._manager.teardown(sandbox_id) +``` + +### Resource Handler Migration + +Resource handlers are updated to receive `SandboxGateway` via constructor injection: + +```python +# Before (violation) +from cleveragents.infrastructure.sandbox.manager import SandboxManager + +class DevcontainerHandler(BaseResourceHandler): + def __init__(self, sandbox_manager: SandboxManager) -> None: + self._sandbox = sandbox_manager + +# After (compliant) +from cleveragents.resource.sandbox_gateway import SandboxGateway + +class DevcontainerHandler(BaseResourceHandler): + def __init__(self, sandbox_gateway: SandboxGateway) -> None: + self._sandbox = sandbox_gateway +``` + +The same pattern applies to `_base.py` and `protocol.py`. + +### DI Container Wiring + +In `cleveragents/infrastructure/di/container.py`: + +```python +from cleveragents.infrastructure.sandbox.gateway_adapter import SandboxManagerGateway +from cleveragents.infrastructure.sandbox.manager import SandboxManager + +class Container(DeclarativeContainer): + sandbox_manager = providers.Singleton(SandboxManager, ...) + sandbox_gateway = providers.Singleton( + SandboxManagerGateway, + manager=sandbox_manager, + ) + # Resource handlers receive sandbox_gateway, not sandbox_manager + devcontainer_handler = providers.Factory( + DevcontainerHandler, + sandbox_gateway=sandbox_gateway, + ) +``` + +### Module Placement Summary + +| Module | Layer | Purpose | +|--------|-------|---------| +| `cleveragents.resource.sandbox_gateway` | Domain | `SandboxGateway` protocol, `SandboxContext`, `CheckpointRef` | +| `cleveragents.infrastructure.sandbox.gateway_adapter` | Infrastructure | `SandboxManagerGateway` adapter | +| `cleveragents.infrastructure.di.container` | Infrastructure | DI wiring | + +## Constraints + +- `cleveragents.resource.*` **MUST NOT** import from `cleveragents.infrastructure.*` — this is a hard architectural constraint enforced by CI +- `SandboxGateway`, `SandboxContext`, and `CheckpointRef` must reside in `cleveragents.resource.sandbox_gateway` (domain layer) — they must not be moved to infrastructure or application layers +- `SandboxContext` and `CheckpointRef` must be infrastructure-free dataclasses — no file paths, OS handles, or infrastructure library types +- The CI architecture test (`tests/architecture/test_layer_boundaries.py`) must assert that no module under `cleveragents.resource` imports from `cleveragents.infrastructure` +- The `typecheck` CI job must pass with the new protocol — no `type: ignore` suppressions are permitted + +## Consequences + +### Positive + +- The domain layer is fully infrastructure-free; resource handlers have zero infrastructure imports +- Resource handlers are unit-testable with a simple mock or stub implementing `SandboxGateway` +- The Clean Architecture dependency rule is enforced mechanically by CI, not just by convention +- Swapping sandbox strategies (Git worktree, filesystem copy, no-op) requires only a new `SandboxGateway` implementation — no changes to domain handlers +- `SandboxContext` and `CheckpointRef` as frozen dataclasses are safe to pass across layer boundaries without leaking infrastructure state + +### Negative + +- An additional indirection layer is introduced: callers go through `SandboxGateway` → `SandboxManagerGateway` → `SandboxManager` +- DI container wiring must be updated to inject `SandboxGateway` instead of `SandboxManager` into all resource handlers +- Developers must understand the gateway pattern to add new sandbox operations (new methods must be added to the protocol, the adapter, and the mock) + +### Risks + +- **Incomplete migration**: If any resource handler is missed during the refactor, it will still import infrastructure directly. Mitigation: the CI architecture test catches any remaining violations on every commit. +- **Protocol drift**: If `SandboxManager` gains new methods that are not reflected in `SandboxGateway`, callers cannot access them. Mitigation: the adapter is the single point of translation; adding a method requires updating the protocol, adapter, and any mocks. + +## Alternatives Considered + +**Move `SandboxManager` to the domain layer** — Rejected. `SandboxManager` has direct dependencies on the file system, OS subprocess calls, and Git CLI. Moving it to the domain layer would import infrastructure concerns into the domain, which is the opposite of the intended fix. + +**Use the application layer as an intermediary** — Rejected. Resource handlers are already called from the application layer. Routing sandbox calls through an additional application-layer service adds an unnecessary hop and increases complexity without architectural benefit. The gateway pattern achieves the same decoupling with less indirection. + +## Compliance + +- **Architecture test**: `tests/architecture/test_layer_boundaries.py` must contain an assertion that no module matching `cleveragents.resource.*` imports from `cleveragents.infrastructure.*`. This test runs in the `lint` CI job on every commit. +- **Type checking**: The CI `typecheck` job (pyright strict mode) must pass with zero errors on all files in `cleveragents/resource/` after the migration. +- **Unit tests**: Each resource handler must have a BDD scenario that exercises sandbox operations using a mock `SandboxGateway` — no real `SandboxManager` is instantiated in unit tests. +- **Import linter**: The `import-linter` configuration (or equivalent Ruff rule) must include a contract forbidding `cleveragents.resource` → `cleveragents.infrastructure` imports. + +Closes #8058 diff --git a/docs/adr/ADR-050-plan-identifier-canonicalization.md b/docs/adr/ADR-050-plan-identifier-canonicalization.md new file mode 100644 index 000000000..3dea7dc8e --- /dev/null +++ b/docs/adr/ADR-050-plan-identifier-canonicalization.md @@ -0,0 +1,221 @@ +--- +adr_number: 50 +title: "Plan Identifier Canonicalization (ULID-First)" +status_history: + - ["2026-04-13", "Draft", "CleverThis"] + - ["2026-04-13", "Proposed", "CleverThis"] +tier: 2 +authors: ["CleverThis"] +superseded_by: +related_adrs: + - number: 6 + title: "Plan Lifecycle" + relationship: "PlanLifecycleService is the primary source of ULID-keyed plan identifiers; this ADR standardizes the ID type it exposes to all downstream services" + - number: 14 + title: "Context Management ACMS" + relationship: "ContextService and VectorStoreService are the primary consumers that must be updated to accept ULID plan identifiers instead of integer surrogate IDs" +acceptance: + votes_for: [] + votes_against: [] + abstentions: [] +--- + +## Context + +A guard scan (issue #8059) identified a type mismatch in plan identifier usage across application-layer services: + +- `PlanLifecycleService.get_plan(plan_id: str)` documents its argument as "The plan ULID" — a string identifier +- `VectorStoreService` stores internal caches in `dict[int, _FaissPlanStoreProtocol]` (line 99) and exposes `refresh_for_plan(self, plan_id: int)` and `search(self, plan_id: int, ...)` (lines 117–178) +- `ContextService` treats `plan_id` as `int | None` (lines 141–158) and passes it directly into vector store search (lines 932–938) + +When lifecycle-managed plans (identified by ULID strings) interact with context search or vector store operations, callers must guess which ID type to pass. This ambiguity causes runtime `TypeError` exceptions and makes the codebase difficult to reason about statically. + +The root cause is that integer database surrogate IDs — an infrastructure-level implementation detail of the SQLAlchemy ORM — have leaked into the application layer's public service signatures. + +## Decision Drivers + +- Runtime `TypeError` exceptions occur when ULID-keyed plans interact with integer-keyed context/vector services +- Static type checkers cannot catch the mismatch because both `str` and `int` are valid Python types; the error is semantic, not syntactic +- The application layer must present a coherent, infrastructure-free API — integer surrogate IDs are a database implementation detail +- ULID strings are already the canonical identifier in `PlanLifecycleService`; changing lifecycle to use integers would break the plan lifecycle protocol (ADR-006) +- ULIDs are lexicographically sortable and human-readable, making them superior to opaque integers for cross-service contracts +- The fix must be enforceable by static analysis (pyright strict mode) with no `type: ignore` suppressions + +## Decision + +Standardize on **ULID strings** as the canonical plan identifier across all application-layer services. Integer database surrogate IDs are an infrastructure-only concern and must never appear in application-layer service method signatures. + +## Design + +### Canonical Identifier Type + +All application-layer service methods use `plan_id: str` where the value is a ULID string (e.g., `"01HX5Z3K2VQJR8MNTPF6YDCWB"`). The ULID format is defined by the `python-ulid` library already used by `PlanLifecycleService`. + +### PlanIdConverter Utility + +A conversion utility is introduced in `cleveragents/application/services/plan_id.py`: + +```python +from __future__ import annotations + +from sqlalchemy.orm import Session + +from cleveragents.infrastructure.database.models import PlanRecord + + +class PlanIdConverter: + """Translates ULID plan identifiers to database surrogate IDs. + + This utility is used exclusively by repository implementations in the + infrastructure layer. Application-layer services must never call this + directly — they work only with ULID strings. + """ + + @staticmethod + def ulid_to_db_id(ulid: str, session: Session) -> int: + """Look up the integer surrogate ID for a given plan ULID. + + Args: + ulid: The canonical ULID string identifier for the plan. + session: An active SQLAlchemy session. + + Returns: + The integer primary key used by the database for this plan. + + Raises: + KeyError: If no plan with the given ULID exists in the database. + """ + record = session.query(PlanRecord).filter_by(ulid=ulid).one_or_none() + if record is None: + raise KeyError(f"No plan found with ULID: {ulid!r}") + return record.id +``` + +`PlanIdConverter` is placed in the application layer because it is part of the translation contract between application and infrastructure. Repository implementations in `cleveragents.infrastructure.database` call `PlanIdConverter.ulid_to_db_id` internally and never expose the integer ID to callers. + +### VectorStoreService Signature Changes + +```python +# Before (violation) +class VectorStoreService: + _stores: dict[int, _FaissPlanStoreProtocol] + + def refresh_for_plan(self, plan_id: int) -> None: ... + def search(self, plan_id: int, query: str, top_k: int = 5) -> list[SearchResult]: ... + +# After (compliant) +class VectorStoreService: + _stores: dict[str, _FaissPlanStoreProtocol] # keyed by ULID string + + def refresh_for_plan(self, plan_id: str) -> None: ... + def search(self, plan_id: str, query: str, top_k: int = 5) -> list[SearchResult]: ... +``` + +The internal `_stores` cache is re-keyed from `int` to `str` (ULID). The ULID string is a stable, unique key that does not require a database round-trip for cache lookups. + +### ContextService Signature Changes + +```python +# Before (violation) +class ContextService: + def add_context(self, plan_id: int | None, ...) -> None: ... + def search_context(self, plan_id: int | None, ...) -> list[ContextEntry]: ... + +# After (compliant) +class ContextService: + def add_context(self, plan_id: str | None, ...) -> None: ... + def search_context(self, plan_id: str | None, ...) -> list[ContextEntry]: ... +``` + +`None` remains valid for operations not scoped to a specific plan (global context queries). + +### Repository Layer Translation + +Repository implementations translate ULID → integer surrogate internally, invisible to callers: + +```python +# In cleveragents/infrastructure/database/repositories/plan_repository.py +class SqlAlchemyPlanRepository: + def get_by_ulid(self, ulid: str) -> Plan: + db_id = PlanIdConverter.ulid_to_db_id(ulid, self._session) + record = self._session.get(PlanRecord, db_id) + return self._mapper.to_domain(record) +``` + +### ULID Lookup Caching + +To mitigate the performance cost of ULID → integer lookups on every repository call, a request-scoped cache is introduced: + +```python +from functools import lru_cache + +class PlanIdConverter: + @staticmethod + @lru_cache(maxsize=512) + def ulid_to_db_id_cached(ulid: str, session_id: int) -> int: + """Cached variant for use within a single unit-of-work session.""" + ... +``` + +The cache is keyed by `(ulid, session_id)` so it does not persist stale data across sessions. + +### Migration Path + +All callers must be updated in a **single PR** to avoid a mixed-state period where some callers pass `str` and others pass `int`. The migration PR must: + +1. Update `VectorStoreService` signatures and internal cache key type +2. Update `ContextService` signatures +3. Update all call sites in the application layer +4. Update all BDD step definitions that construct service calls with integer plan IDs +5. Update integration tests to use ULID strings end-to-end + +### Type Enforcement + +Pyright strict mode is configured for all files in `cleveragents/application/services/`. The type annotations `plan_id: str` and `plan_id: str | None` are enforced statically. Any attempt to pass an `int` where `str` is expected will be caught at type-check time. + +## Constraints + +- No application-layer service method may accept `plan_id: int` — this is a hard constraint enforced by pyright strict mode +- `plan_id: int` is permitted **only** in repository/ORM layer code (`cleveragents.infrastructure.database.*`) +- `PlanIdConverter` must not be called from application-layer service methods directly — it is for repository use only +- No `type: ignore` suppressions are permitted in service files +- The migration must be atomic — no intermediate state where some services accept `str` and others accept `int` + +## Consequences + +### Positive + +- A single canonical ID type eliminates runtime `TypeError` mismatches between lifecycle and context/vector services +- ULID strings are human-readable, lexicographically sortable, and globally unique — superior to opaque integers for cross-service contracts +- Static type checking catches any future regression where an integer ID leaks into the application layer +- The internal `_stores` cache in `VectorStoreService` no longer requires a database round-trip to compute the cache key +- The application layer is fully decoupled from the database's integer primary key scheme + +### Negative + +- All callers must be updated simultaneously in a single PR — a large, coordinated change with high review burden +- Repository implementations require a ULID → integer lookup on every call (mitigated by the request-scoped cache) +- Existing integration tests that use integer plan IDs must be updated + +### Risks + +- **Missed callers**: Any call site not updated in the migration PR will cause a `TypeError` at runtime. Mitigation: pyright strict mode catches type mismatches statically; the migration PR must include a full pyright run with zero errors before merging. +- **Cache invalidation**: The ULID → integer cache could serve stale data if a plan is deleted and its ULID reused. Mitigation: ULIDs are monotonically increasing and never reused; deletion invalidates the cache entry explicitly. +- **Test coverage gaps**: If integration tests do not exercise the ULID → context search path end-to-end, regressions may go undetected. Mitigation: the compliance section mandates an end-to-end integration test for this path. + +## Alternatives Considered + +**Keep integer IDs everywhere** — Rejected. `PlanLifecycleService` already uses ULID strings as its canonical identifier (documented in ADR-006). Changing the lifecycle service to use integers would require modifying the plan lifecycle protocol and all its callers, a larger and riskier change than updating the context/vector services. + +**Accept both types with overloads** — Rejected. Overloads that accept both `int` and `str` perpetuate the ambiguity and make static analysis harder. The goal is a single, unambiguous type that the type checker can enforce. Accepting both types would allow callers to continue passing integers, defeating the purpose of the canonicalization. + +**Use a `PlanId` newtype** — Considered but deferred. A `NewType("PlanId", str)` would provide stronger type safety than a bare `str`. This can be adopted in a follow-up ADR once the ULID-first migration is complete and stable. + +## Compliance + +- **Static type checking**: `pyright` strict mode must report zero errors on all files in `cleveragents/application/services/` after the migration. This check runs in the CI `typecheck` job on every commit. +- **Integration tests**: A BDD integration scenario must exercise the full path: create a plan via `PlanLifecycleService` (returns ULID) → add context via `ContextService(plan_id=ulid)` → search context via `ContextService.search_context(plan_id=ulid)` → assert results are returned. This scenario must pass in the CI `integration_tests` job. +- **Architecture guard**: The guard configuration must include a rule asserting that no application-layer service method signature contains `plan_id: int`. +- **Code review**: The migration PR must include a pyright output showing zero errors before it is approved. + +Closes #8059 diff --git a/docs/adr/ADR-051-plan-lifecycle-service-decomposition.md b/docs/adr/ADR-051-plan-lifecycle-service-decomposition.md new file mode 100644 index 000000000..4fe0f8f72 --- /dev/null +++ b/docs/adr/ADR-051-plan-lifecycle-service-decomposition.md @@ -0,0 +1,250 @@ +--- +adr_number: 51 +title: "PlanLifecycleService Decomposition" +status_history: + - ["2026-04-13", "Draft", "CleverThis"] + - ["2026-04-13", "Proposed", "CleverThis"] +tier: 2 +authors: ["CleverThis"] +superseded_by: +related_adrs: + - number: 6 + title: "Plan Lifecycle" + relationship: "This ADR refactors the primary implementation of the plan lifecycle without changing its externally observable behavior or protocol" + - number: 3 + title: "Dependency Injection" + relationship: "The decomposed modules are wired through the DI container; each focused service is registered as a provider and injected into the facade" + - number: 17 + title: "Automation Profiles" + relationship: "Automation profile resolution logic currently embedded in PlanLifecycleService is extracted into a dedicated module during decomposition" +acceptance: + votes_for: [] + votes_against: [] + abstentions: [] +--- + +## Context + +A guard scan (issue #8061) identified that `src/cleveragents/application/services/plan_lifecycle_service.py` spans **2,649 lines**. `CONTRIBUTING.md` (lines 382–404) mandates a maximum of 500 lines per file to maintain cohesion and reviewability. + +The file currently mixes the following distinct concerns in a single module: + +- Plan creation and action capture +- Strategize phase orchestration (LLM planning) +- Execute phase orchestration (tool execution) +- Apply phase orchestration (commit, diff, approval) +- Shared validation pipeline +- Invariant reconciliation wiring +- Retry logic and error recovery +- Diff helpers and patch utilities +- Event emission and gateway logic +- ACMS (Advanced Context Management System) orchestration +- DI wiring and provider configuration + +This concentration of concerns makes the file difficult to review, test in isolation, and extend without introducing regressions. It also makes it impossible to assign ownership of individual concerns to separate contributors. + +## Decision Drivers + +- `CONTRIBUTING.md` mandates ≤500 lines per file; the current file is 5× over the limit +- Mixed concerns make individual behaviors impossible to test in isolation +- The file is a frequent merge conflict hotspot because unrelated changes touch the same file +- Phase-specific logic (strategize, execute, apply) should be independently reviewable and deployable +- The DI container must be able to wire focused services without depending on a monolithic class +- External callers (CLI, TUI, A2A endpoint) must not be affected — the public API surface must remain stable + +## Decision + +Decompose `PlanLifecycleService` into focused modules organized by phase and concern, each ≤500 lines. The public `PlanLifecycleService` class becomes a **thin facade** that delegates to the focused services. External callers continue to interact with `PlanLifecycleService` through the same method signatures — no breaking changes to the public API. + +## Design + +### Target Module Structure + +``` +cleveragents/application/services/lifecycle/ + __init__.py # Re-exports PlanLifecycleService facade (thin orchestrator, ≤200 lines) + _action.py # ActionService: plan creation, action capture (≤500 lines) + _strategize.py # StrategizeService: LLM planning phase (≤500 lines) + _execute.py # ExecuteService: tool execution phase (≤500 lines) + _apply.py # ApplyService: commit/diff/approval phase (≤500 lines) + _validation.py # PlanValidationService: shared validation pipeline (≤500 lines) + _events.py # PlanEventService: event emission helpers (≤300 lines) + _correction.py # CorrectionService: revert/append correction logic (≤500 lines) + _invariants.py # InvariantOrchestrator: reconciliation wiring (≤400 lines) +``` + +All modules are prefixed with `_` to signal that they are internal implementation details of the `lifecycle` package. External callers import only from `cleveragents.application.services.lifecycle` (the `__init__.py` facade). + +### Facade Pattern + +`__init__.py` contains the `PlanLifecycleService` class as a thin orchestrator: + +```python +# cleveragents/application/services/lifecycle/__init__.py + +from cleveragents.application.services.lifecycle._action import ActionService +from cleveragents.application.services.lifecycle._apply import ApplyService +from cleveragents.application.services.lifecycle._correction import CorrectionService +from cleveragents.application.services.lifecycle._execute import ExecuteService +from cleveragents.application.services.lifecycle._events import PlanEventService +from cleveragents.application.services.lifecycle._invariants import InvariantOrchestrator +from cleveragents.application.services.lifecycle._strategize import StrategizeService +from cleveragents.application.services.lifecycle._validation import PlanValidationService + + +class PlanLifecycleService: + """Thin facade over the decomposed lifecycle services. + + External callers (CLI, TUI, A2A endpoint) interact only with this class. + All business logic is delegated to the focused service modules. + """ + + def __init__( + self, + action_service: ActionService, + strategize_service: StrategizeService, + execute_service: ExecuteService, + apply_service: ApplyService, + validation_service: PlanValidationService, + event_service: PlanEventService, + correction_service: CorrectionService, + invariant_orchestrator: InvariantOrchestrator, + ) -> None: + self._action = action_service + self._strategize = strategize_service + self._execute = execute_service + self._apply = apply_service + self._validation = validation_service + self._events = event_service + self._correction = correction_service + self._invariants = invariant_orchestrator + + # Public API — signatures unchanged from the monolith + def create_plan(self, ...) -> Plan: + return self._action.create_plan(...) + + def strategize(self, plan_id: str, ...) -> StrategizeResult: + return self._strategize.run(plan_id, ...) + + def execute(self, plan_id: str, ...) -> ExecuteResult: + return self._execute.run(plan_id, ...) + + def apply(self, plan_id: str, ...) -> ApplyResult: + return self._apply.run(plan_id, ...) + + def correct(self, plan_id: str, mode: CorrectionMode, ...) -> CorrectionResult: + return self._correction.run(plan_id, mode, ...) + + def get_plan(self, plan_id: str) -> Plan: + return self._action.get_plan(plan_id) +``` + +### Module Responsibilities + +| Module | Responsibility | Key Classes/Functions | +|--------|---------------|----------------------| +| `_action.py` | Plan creation, action capture, plan retrieval | `ActionService`, `create_plan()`, `capture_action()`, `get_plan()` | +| `_strategize.py` | LLM planning phase, prompt assembly, decision recording | `StrategizeService`, `run()`, `_build_strategy_prompt()` | +| `_execute.py` | Tool execution phase, tool dispatch, result collection | `ExecuteService`, `run()`, `_dispatch_tool()` | +| `_apply.py` | Commit, diff generation, approval gating, patch application | `ApplyService`, `run()`, `_generate_diff()`, `_request_approval()` | +| `_validation.py` | Shared validation pipeline, schema checks, pre/post-phase guards | `PlanValidationService`, `validate_pre_strategize()`, `validate_pre_execute()` | +| `_events.py` | Domain event construction and emission | `PlanEventService`, `emit_phase_changed()`, `emit_tool_invoked()` | +| `_correction.py` | Revert and append correction modes, subtree recomputation | `CorrectionService`, `run()`, `_revert_to_decision()`, `_append_guidance()` | +| `_invariants.py` | Invariant reconciliation wiring, enforcement orchestration | `InvariantOrchestrator`, `reconcile()`, `_enforce_invariants()` | + +### DI Container Update + +Each focused service is registered as a separate provider in the DI container: + +```python +# cleveragents/infrastructure/di/container.py + +class Container(DeclarativeContainer): + # Focused lifecycle services + plan_event_service = providers.Singleton(PlanEventService, event_bus=event_bus) + plan_validation_service = providers.Singleton(PlanValidationService, ...) + invariant_orchestrator = providers.Singleton(InvariantOrchestrator, ...) + action_service = providers.Singleton(ActionService, plan_repo=plan_repo, ...) + strategize_service = providers.Singleton(StrategizeService, llm=llm_provider, ...) + execute_service = providers.Singleton(ExecuteService, tool_registry=tool_registry, ...) + apply_service = providers.Singleton(ApplyService, sandbox_gateway=sandbox_gateway, ...) + correction_service = providers.Singleton(CorrectionService, ...) + + # Facade wires all focused services + plan_lifecycle_service = providers.Singleton( + PlanLifecycleService, + action_service=action_service, + strategize_service=strategize_service, + execute_service=execute_service, + apply_service=apply_service, + validation_service=plan_validation_service, + event_service=plan_event_service, + correction_service=correction_service, + invariant_orchestrator=invariant_orchestrator, + ) +``` + +### Migration Strategy + +The decomposition is performed in phases to minimize risk: + +1. **Phase 1 — Extract without behavior change**: Move code verbatim into the new modules. The facade delegates to the extracted 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 target the focused services directly. +3. **Phase 3 — Add focused tests**: Write BDD scenarios targeting each focused service in isolation using mocks for its dependencies. + +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/application/services/lifecycle/` exceeds 500 lines. This prevents the monolith from re-emerging over time. + +## Constraints + +- The public API of `PlanLifecycleService` (method names, signatures, return types) must remain unchanged — no breaking changes to external callers +- Each module in `cleveragents/application/services/lifecycle/` must be ≤500 lines (enforced by CI) +- The `__init__.py` facade must be ≤200 lines — it contains only delegation, no business logic +- Internal modules (`_action.py`, `_strategize.py`, etc.) must not be imported directly by code outside the `lifecycle` package — all external access goes through the facade +- The original `plan_lifecycle_service.py` file must be deleted after Phase 1 is complete; it must not coexist with the new package +- All existing BDD scenarios for plan lifecycle must continue to pass after Phase 1 + +## Consequences + +### Positive + +- Each module is ≤500 lines, compliant with `CONTRIBUTING.md` +- Phase-specific logic is independently reviewable, testable, and assignable to contributors +- Merge conflicts are dramatically reduced — changes to strategize logic no longer touch the same file as apply logic +- Each focused service can be mocked independently in unit tests, enabling true isolation +- The DI container can wire focused services with fine-grained control over their dependencies +- New lifecycle phases or concerns can be added as new modules without modifying existing ones + +### 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 the full lifecycle flow; the monolith was self-contained +- The DI container configuration grows more complex as each focused service is registered separately +- Import paths change from `cleveragents.application.services.plan_lifecycle_service` to `cleveragents.application.services.lifecycle` — any direct imports (outside the DI container) must be updated + +### Risks + +- **Behavior regression during extraction**: Moving code verbatim may inadvertently break shared state or ordering assumptions. Mitigation: Phase 1 is a pure structural refactor with no logic changes; 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 over time. Mitigation: CI file size check fails the build if any module exceeds the limit. +- **Incomplete extraction**: If some logic is left in the original file and the new package is created alongside it, two sources of truth exist. Mitigation: the original `plan_lifecycle_service.py` must be deleted in Phase 1; CI import checks verify no code imports from the old path. + +## 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 file in the codebase would undermine the rule and set a precedent for other files to grow without bound. + +**Split into two files (lifecycle_core.py and lifecycle_helpers.py)** — Rejected. A two-file split does not address the mixed-concern problem — it merely distributes the monolith across two files. The phase-based decomposition into eight focused modules is the correct granularity for the concerns present in the file. + +**Rewrite from scratch** — Rejected. A rewrite introduces high risk of behavior regression and requires re-implementing 2,649 lines of battle-tested logic. The phased extraction approach preserves existing behavior while improving structure. + +## Compliance + +- **File size check**: CI `lint` job must assert that no file in `cleveragents/application/services/lifecycle/` exceeds 500 lines. The `__init__.py` facade must not exceed 200 lines. +- **Import check**: CI must assert that no code outside `cleveragents/application/services/lifecycle/` imports from the internal modules (`_action`, `_strategize`, etc.) directly. +- **Regression tests**: All existing BDD scenarios for plan lifecycle (`features/plan_lifecycle.feature`, `features/plan_correction.feature`, etc.) must pass after Phase 1 without modification. +- **Focused service tests**: After Phase 3, each focused service must have at least one BDD scenario that exercises it in isolation with mocked dependencies. +- **Old file deletion**: The CI `lint` job must assert that `src/cleveragents/application/services/plan_lifecycle_service.py` does not exist after Phase 1 is merged. + +Closes #8061 diff --git a/docs/adr/ADR-052-lifecycle-coverage-step-integrity.md b/docs/adr/ADR-052-lifecycle-coverage-step-integrity.md new file mode 100644 index 000000000..599794b21 --- /dev/null +++ b/docs/adr/ADR-052-lifecycle-coverage-step-integrity.md @@ -0,0 +1,226 @@ +--- +adr_number: 52 +title: "Lifecycle Coverage Step Integrity — Replace Placeholder Assertions" +status_history: + - ["2026-04-13", "Draft", "CleverThis"] + - ["2026-04-13", "Proposed", "CleverThis"] +tier: 2 +authors: ["CleverThis"] +superseded_by: +related_adrs: + - number: 6 + title: "Plan Lifecycle" + relationship: "The placeholder steps purport to test plan lifecycle edge cases; this ADR mandates that they exercise real lifecycle behavior" + - number: 51 + title: "PlanLifecycleService Decomposition" + relationship: "The decomposed lifecycle services provide the focused targets for the replacement assertions" +acceptance: + votes_for: [] + votes_against: [] + abstentions: [] +--- + +## Context + +A guard scan (issue #8062) identified placeholder BDD step definitions in `features/steps/coverage_boost_extra_steps.py` that provide no behavioral verification: + +- Lines 646–662 define `step_lifecycle_edges` (`@when`) and `step_no_errors` (`@then`) +- The `@when` step sets an internal flag without invoking any lifecycle code +- The `@then` step contains only `assert True` — it passes unconditionally regardless of system state +- No assertions check lifecycle side effects, emitted events, phase transitions, or error conditions + +These steps exist solely to inflate code coverage metrics. They provide a false signal: the CI coverage report shows the lifecycle code paths as "covered," but no behavior is actually verified. Any regression in the covered code paths would still pass these steps. + +`CONTRIBUTING.md` (Testing Philosophy section) states: "Every coding task must include or update tests at multiple levels: unit tests, integration tests, and performance benchmarks. Testing is non-optional and is part of the definition of done for any task." Placeholder assertions violate this principle by creating the appearance of testing without the substance. + +## Decision Drivers + +- Placeholder `assert True` steps provide zero behavioral verification — they are worse than no test because they create false confidence +- CI coverage metrics are misleading when coverage comes from inert steps rather than verified behavior +- `CONTRIBUTING.md` mandates that tests verify real behavior; placeholder steps violate this mandate +- The lifecycle edge cases that the steps claim to cover (error conditions, phase transitions) are genuinely important and deserve real verification +- Removing the steps without replacement would reduce coverage; the correct fix is replacement with meaningful assertions + +## Decision + +Replace the placeholder `step_lifecycle_edges` and `step_no_errors` step definitions with BDD scenarios that exercise real lifecycle behavior and assert observable outcomes. The replacement scenarios must invoke actual lifecycle code paths and verify side effects (phase transitions, emitted events, error conditions, or returned state). + +## Design + +### Identified Placeholder Steps + +The following step definitions in `features/steps/coverage_boost_extra_steps.py` must be replaced: + +```python +# BEFORE — placeholder (lines 646-662) +@when("lifecycle edge cases are exercised") +def step_lifecycle_edges(context): + context.lifecycle_exercised = True # No lifecycle code invoked + +@then("no errors should occur") +def step_no_errors(context): + assert True # Unconditional pass — verifies nothing +``` + +### Replacement Strategy + +Each placeholder step is replaced by a scenario that: + +1. **Invokes real lifecycle code** — calls a method on `PlanLifecycleService` or one of its decomposed services (per ADR-051) +2. **Asserts an observable outcome** — checks phase state, emitted events, returned values, or raised exceptions +3. **Uses a mock or in-memory implementation** — does not require a running database or LLM provider for unit-level scenarios + +### Replacement Scenarios + +The following scenarios replace the placeholder steps. They are organized by the lifecycle edge case they cover: + +#### Scenario 1: Phase transition emits domain event + +```gherkin +# features/plan_lifecycle_edge_cases.feature + +Scenario: Strategize phase emits PlanPhaseChanged event + Given a plan in the "created" phase + When the strategize phase is initiated + Then a "PlanPhaseChanged" event is emitted with phase "strategizing" + And the plan phase is "strategizing" +``` + +Step implementation: + +```python +@given('a plan in the "{phase}" phase') +def step_plan_in_phase(context, phase): + context.event_bus = MockEventBus() + context.lifecycle = PlanLifecycleService( + strategize_service=MockStrategizeService(), + event_service=PlanEventService(event_bus=context.event_bus), + # ... other mocked services + ) + context.plan = context.lifecycle.create_plan(phase=phase, ...) + +@when('the strategize phase is initiated') +def step_initiate_strategize(context): + context.result = context.lifecycle.strategize(plan_id=context.plan.ulid) + +@then('a "{event_type}" event is emitted with phase "{phase}"') +def step_event_emitted(context, event_type, phase): + emitted = context.event_bus.emitted_events + matching = [e for e in emitted if e.event_type == event_type and e.phase == phase] + assert len(matching) == 1, ( + f"Expected exactly one {event_type} event with phase={phase!r}, " + f"got: {emitted}" + ) + +@then('the plan phase is "{expected_phase}"') +def step_plan_phase(context, expected_phase): + plan = context.lifecycle.get_plan(context.plan.ulid) + assert plan.phase == expected_phase, ( + f"Expected phase {expected_phase!r}, got {plan.phase!r}" + ) +``` + +#### Scenario 2: Validation failure raises structured error + +```gherkin +Scenario: Strategize phase raises ValidationError when plan has no actions + Given a plan with no actions defined + When the strategize phase is initiated + Then a "PlanValidationError" is raised + And the error message contains "no actions" +``` + +Step implementation: + +```python +@given('a plan with no actions defined') +def step_plan_no_actions(context): + context.plan = context.lifecycle.create_plan(actions=[], ...) + +@when('the strategize phase is initiated') +def step_initiate_strategize_no_actions(context): + try: + context.lifecycle.strategize(plan_id=context.plan.ulid) + context.raised_error = None + except PlanValidationError as exc: + context.raised_error = exc + +@then('a "PlanValidationError" is raised') +def step_validation_error_raised(context): + assert context.raised_error is not None, "Expected PlanValidationError but no error was raised" + assert isinstance(context.raised_error, PlanValidationError) + +@then('the error message contains "{fragment}"') +def step_error_message_contains(context, fragment): + assert fragment in str(context.raised_error), ( + f"Expected error message to contain {fragment!r}, got: {context.raised_error!r}" + ) +``` + +#### Scenario 3: Correction revert restores prior phase + +```gherkin +Scenario: Correction in revert mode restores plan to the targeted decision point + Given a plan that has completed the strategize phase + And a decision checkpoint exists at the start of strategize + When correction is applied in "revert" mode targeting the strategize checkpoint + Then the plan phase is "created" + And the decision tree is truncated to the checkpoint +``` + +### Removal of Placeholder Steps + +The original `step_lifecycle_edges` and `step_no_errors` step definitions are **deleted** from `coverage_boost_extra_steps.py`. Any feature file that references these steps must be updated to use the replacement scenarios. + +If `coverage_boost_extra_steps.py` becomes empty after removing the placeholder steps, the file itself is deleted. Coverage boost files that contain only inert steps are not permitted. + +### Coverage Verification + +After replacement, the CI coverage report must show that the lifecycle code paths previously "covered" by `assert True` are now covered by assertions that can actually fail. This is verified by temporarily introducing a deliberate regression (e.g., commenting out an event emission call) and confirming that the replacement scenario fails. + +## Constraints + +- No BDD step definition may contain `assert True` as its only assertion — this is a hard rule enforced by a Ruff custom lint rule or pre-commit hook +- No `@when` step may set a flag without invoking production code — steps must exercise real behavior +- Replacement scenarios must use mocks or in-memory implementations; they must not require a running database, LLM provider, or sandbox for unit-level execution +- The replacement scenarios must be placed in a feature file named after the behavior they test (e.g., `features/plan_lifecycle_edge_cases.feature`), not in a generic "coverage boost" file +- `coverage_boost_extra_steps.py` must not contain any step that exists solely to inflate coverage metrics + +## Consequences + +### Positive + +- Coverage metrics accurately reflect verified behavior — false positives are eliminated +- Lifecycle edge cases (phase transitions, validation failures, correction revert) are now regression-tested +- The replacement scenarios serve as living documentation of expected lifecycle behavior +- Future regressions in the covered code paths will cause CI failures, providing an early warning signal +- The codebase is more trustworthy: coverage numbers mean something + +### Negative + +- Writing meaningful replacement scenarios requires understanding the lifecycle domain in depth — more effort than `assert True` +- The replacement scenarios may expose pre-existing bugs in the lifecycle edge cases that were previously hidden by the placeholder assertions +- Coverage percentage may temporarily decrease if the placeholder steps were covering code that the replacement scenarios do not reach — this must be addressed by writing additional targeted scenarios + +### Risks + +- **Pre-existing bugs surfaced**: The replacement scenarios may fail immediately because the lifecycle edge cases they test contain bugs. Mitigation: treat any failure as a bug discovery (positive outcome) and fix the bug before merging the replacement scenarios. +- **Coverage regression**: If the replacement scenarios cover fewer lines than the placeholder steps, the coverage threshold check may fail. Mitigation: write additional scenarios to cover the remaining lines; do not lower the coverage threshold. +- **Incomplete replacement**: If some placeholder steps are missed, the false-positive coverage signal persists. Mitigation: the Ruff lint rule for `assert True`-only steps catches any remaining placeholders in CI. + +## Alternatives Considered + +**Delete the placeholder steps without replacement** — Rejected. Deletion without replacement would reduce coverage and leave genuine lifecycle edge cases untested. The correct fix is replacement with meaningful assertions, not removal. + +**Mark the placeholder scenarios with `@tdd_expected_fail`** — Rejected. The `@tdd_expected_fail` tag (per `CONTRIBUTING.md`) is for scenarios that capture a known bug that is not yet fixed. The placeholder steps do not capture a bug — they are inert assertions that never fail. Using `@tdd_expected_fail` would not address the underlying problem. + +**Keep the placeholder steps and add additional real scenarios alongside them** — Rejected. The placeholder steps would continue to provide false coverage signal. The correct approach is to replace them entirely so that coverage metrics are accurate. + +## Compliance + +- **Lint rule**: A Ruff custom rule or pre-commit hook must flag any step definition whose body contains only `assert True` (or equivalent no-op assertions). This check runs in the CI `lint` job on every commit. +- **Coverage verification**: After the replacement PR is merged, the CI coverage report must be manually inspected to confirm that the previously placeholder-covered lines are now covered by assertions that can fail. This is documented in the PR description. +- **Feature file naming**: Replacement scenarios must be in a feature file named after the behavior being tested. The CI `lint` job must assert that no file named `coverage_boost*.py` or `coverage_boost*.feature` exists in the test directories. +- **Step review**: The PR replacing the placeholder steps must include a reviewer sign-off confirming that each replacement step invokes real production code and asserts a non-trivial outcome. + +Closes #8062