From 3918d9f05b0e6ba4d5df6a7cb4e1c3aba7ca2e5a Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 15 May 2026 02:23:52 +0000 Subject: [PATCH] fix: InvariantService persistence across CLI invocations (Bug #8573) Delegate global/project scope CRUD to DI-wired repository instead of ad-hoc sessions; switch CLI service resolution to DI container wiring. --- src/cleveragents/application/container.py | 31 +++- .../application/services/invariant_service.py | 82 +++++++-- src/cleveragents/cli/commands/invariant.py | 39 +++-- .../infrastructure/database/repositories.py | 155 +++++++++++++++++- 4 files changed, 277 insertions(+), 30 deletions(-) diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 8005851f0..17caded7f 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -104,6 +104,7 @@ from cleveragents.infrastructure.database.llm_trace_repository import ( ) from cleveragents.infrastructure.database.repositories import ( CheckpointRepository, + GlobalInvariantRepository, NamespacedProjectRepository, ProjectResourceLinkRepository, SessionMessageRepository, @@ -541,6 +542,27 @@ def _build_automation_profile_service( return AutomationProfileService(repo=profile_repo) +def _build_global_invariant_repository( + database_url: str, +) -> GlobalInvariantRepository: + """Build a ``GlobalInvariantRepository`` backed by the database URL. + + Follows the same ``_build_*`` pattern used by ``_build_session_factory``, + ``_build_skill_service``, and other DB-backed helpers. The repository is + constructed with ``auto_commit=True`` so that CLI commands outside a + ``UnitOfWork`` can commit inline immediately (Bug #8573). + """ + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + engine = create_engine(database_url, echo=False) + factory = sessionmaker(bind=engine, expire_on_commit=False) + return GlobalInvariantRepository( + session_factory=factory, + auto_commit=True, + ) + + def _build_tool_registry_service( database_url: str, ) -> ToolRegistryService: @@ -726,10 +748,17 @@ class Container(containers.DeclarativeContainer): event_bus=event_bus, ) - # Invariant Service - Singleton (in-memory invariant management) + # Global Invariant Repository - Singleton (Bug #8573: persistence for CLI global/project invariants) + invariant_repo = providers.Singleton( + _build_global_invariant_repository, + database_url=database_url, + ) + + # Invariant Service - Singleton (delegates global/project scope CRUD to ``invariant_repo`` when wired) invariant_service = providers.Singleton( InvariantService, database_url=database_url, + global_invariant_repo=invariant_repo, ) # Lock Service - Singleton (shared advisory-lock state per process, #7989) diff --git a/src/cleveragents/application/services/invariant_service.py b/src/cleveragents/application/services/invariant_service.py index 68a666c25..a216cce2b 100644 --- a/src/cleveragents/application/services/invariant_service.py +++ b/src/cleveragents/application/services/invariant_service.py @@ -60,16 +60,24 @@ class InvariantService: """ def __init__( - self, event_bus: EventBus | None = None, database_url: str | None = None + self, + *, + event_bus: EventBus | None = None, + database_url: str | None = None, + global_invariant_repo: Any | None = None, ) -> None: """Initialise the invariant service. Args: event_bus: Optional EventBus for domain event emission. - database_url: SQLAlchemy database URL for persistence - (e.g. ``"sqlite:///~/.cleveragents/cleveragents.db"``). - When provided, all CRUD operations use the database. - When ``None``, operates in pure in-memory mode. + database_url: SQLAlchemy database URL for persistence (action/plan + scope invariants). When provided, all CRUD operations use the + database. When ``None``, operates in pure in-memory mode. + global_invariant_repo: Optional repository for global/project- + scoped invariant persistence (Bug #8573). When provided, + add/list/remove operations delegate to this repo instead of + building ad-hoc sessions. Useful when DI container manages + the repository lifecycle. """ self._invariants: dict[str, Invariant] = {} self._enforcement_records: list[InvariantEnforcementRecord] = [] @@ -77,7 +85,10 @@ class InvariantService: self._sanitizer = PromptSanitizer() self._event_bus = event_bus - # Database-backed path + # Repository-based path (Bug #8573) + self._global_invariant_repo = global_invariant_repo + + # Legacy session-factory path (fallback for standalone usage) self._database_url = database_url self._session_factory: sessionmaker[Session] | None = None self._has_loaded_from_db: bool = False @@ -177,7 +188,13 @@ class InvariantService: source_name=source_name.strip(), ) - if self._database_url is not None and self._ensure_session_factory() is not None: + # Delegate to DI-wired repository for global/project scope (Bug #8573) + if self._global_invariant_repo is not None and scope in ( + InvariantScope.GLOBAL, + InvariantScope.PROJECT, + ): + self._global_invariant_repo.create(invariant) + elif self._database_url is not None and self._ensure_session_factory() is not None: session = self._session_factory() try: from cleveragents.infrastructure.database.models import InvariantModel @@ -185,12 +202,11 @@ class InvariantService: model = InvariantModel.from_domain(invariant) session.add(model) session.flush() - # Cache in memory as well - self._invariants[invariant.id] = invariant finally: session.close() - else: - self._invariants[invariant.id] = invariant + + # Cache in memory regardless of storage path + self._invariants[invariant.id] = invariant logger.info( "Invariant added", @@ -223,6 +239,17 @@ class InvariantService: project_name=source_name if scope == InvariantScope.PROJECT else None, ) + # Delegate to DI-wired repository for global/project scope (Bug #8573) + if self._global_invariant_repo is not None and scope in ( + InvariantScope.GLOBAL, + InvariantScope.PROJECT, + ): + result = self._global_invariant_repo.list_scope(scope=scope) + # Build / refresh cache + for inv in result: + self._invariants.setdefault(inv.id, inv) + return result + # Database-backed query path (one-shot cache population) if ( self._database_url is not None @@ -292,12 +319,25 @@ class InvariantService: inactive = inv.model_copy(update={"active": False}) # Persist to DB when configured - if self._database_url is not None and self._session_factory is not None: + # Delegate to DI-wired repository for global/project scope (Bug #8573) + if self._global_invariant_repo is not None: + removed = self._global_invariant_repo.remove(invariant_id) + if not removed: + raise NotFoundError( + resource_type="invariant", + resource_id=invariant_id, + ) + elif self._database_url is not None and self._session_factory is not None: session = self._session_factory() try: from cleveragents.infrastructure.database.models import InvariantModel - row = session.query(InvariantModel).get(invariant_id) + # Use filter_by instead of deprecated .get() + row = ( + session.query(InvariantModel) + .filter(InvariantModel.id == invariant_id) + .first() + ) if row is None: raise NotFoundError( resource_type="invariant", @@ -313,16 +353,28 @@ class InvariantService: return inactive def _get_invariant_by_id(self, invariant_id: str) -> Invariant | None: - """Lookup a single invariant by ID (cache or database).""" + """Lookup a single invariant by ID (cache, repo, or DB).""" if invariant_id in self._invariants: return self._invariants[invariant_id] + # Check DI-wired repository first (Bug #8573) + if self._global_invariant_repo is not None: + inv = self._global_invariant_repo.get_by_id(invariant_id) + if inv is not None: + self._invariants[invariant_id] = inv + return inv + + # Fallback to legacy session-factory path if self._database_url is not None and self._session_factory is not None: session = self._session_factory() try: from cleveragents.infrastructure.database.models import InvariantModel - row = session.query(InvariantModel).get(invariant_id) + row = ( + session.query(InvariantModel) + .filter_by(id=invariant_id) + .first() + ) if row is not None: inv = row.to_domain() self._invariants[invariant_id] = inv diff --git a/src/cleveragents/cli/commands/invariant.py b/src/cleveragents/cli/commands/invariant.py index 3d53ff780..d8d8c825a 100644 --- a/src/cleveragents/cli/commands/invariant.py +++ b/src/cleveragents/cli/commands/invariant.py @@ -43,9 +43,9 @@ import typer from rich.console import Console from rich.table import Table +from cleveragents.application.container import get_container from cleveragents.application.services.invariant_service import InvariantService from cleveragents.cli.formatting import OutputFormat, format_output -from cleveragents.config.settings import get_settings from cleveragents.core.exceptions import CleverAgentsError, NotFoundError from cleveragents.domain.models.core.invariant import Invariant, InvariantScope @@ -56,21 +56,19 @@ console = Console() _FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich)" -# Module-level service instance (persists across CLI invocations via DB) -_service: InvariantService | None = None +def _get_service() -> InvariantService | None: + """Return the DI-wired InvariantService for the current process. -def _get_service() -> InvariantService: - """Return (or lazily create) the module-level InvariantService. - - Uses the configured ``database_url`` so invariants persist across - CLI invocations (separate processes share the SQLite database). + Uses the global container so that the invariant service is properly wired + with its repository dependency (Bug #8573). Returns ``None`` when no + container is available (e.g. in tests without a DB). """ - global _service - if _service is None: - settings = get_settings() - _service = InvariantService(database_url=settings.database_url) - return _service + try: + container = get_container() + return container.invariant_service() + except Exception: + return None def _resolve_scope( @@ -137,6 +135,11 @@ def add( try: scope, source_name = _resolve_scope(is_global, project, plan, action) service = _get_service() + if service is None: + console.print( + "[red]Error:[/red] Invariant service not available (no database?)" + ) + raise typer.Abort() inv = service.add_invariant(text=text, scope=scope, source_name=source_name) if fmt != OutputFormat.RICH.value: @@ -186,6 +189,11 @@ def list_invariants( """ try: service = _get_service() + if service is None: + console.print( + "[red]Error:[/red] Invariant service not available (no database?)" + ) + raise typer.Abort() scope: InvariantScope | None = None source_name: str | None = None @@ -271,6 +279,11 @@ def remove( raise typer.Abort() service = _get_service() + if service is None: + console.print( + "[red]Error:[/red] Invariant service not available (no database?)" + ) + raise typer.Abort() inv = service.remove_invariant(invariant_id) if fmt != OutputFormat.RICH.value: diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index c8f9e6144..9bc766b79 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -13,6 +13,7 @@ Provides database-backed repositories following the session-factory pattern | ``ProjectResourceLinkRepository``| Project-resource link management | | ``DecisionRepository`` | CRUD for decision tree nodes | | ``CorrectionAttemptRepository``| CRUD for correction attempt records | +| ``GlobalInvariantRepository`` | Persistence for CLI global/project invariants (Bug #8573) | ## Session-Factory Pattern @@ -112,6 +113,7 @@ from cleveragents.infrastructure.database.models import ( CorrectionAttemptModel, DebugAttemptModel, DecisionModel, + InvariantModel, LifecycleActionModel, LifecyclePlanModel, NamespacedProjectModel, @@ -147,6 +149,7 @@ from cleveragents.domain.models.core.correction import ( CorrectionAttemptState, validate_correction_state_transition, ) +from cleveragents.domain.models.core.invariant import InvariantScope _log = structlog.get_logger(__name__) @@ -6227,8 +6230,158 @@ class CorrectionAttemptRepository: session.delete(row) session.flush() return True + except (OperationalError, SQLAlchemyDatabaseError) as exc: + session.rollback() + raise DatabaseError( + f"Failed to delete correction attempt {correction_attempt_id}: {exc}", + ) from exc + + +# --------------------------------------------------------------------------- +# Global Invariant Repository (Bug #8573 - persistence for CLI global/project invariants) +# --------------------------------------------------------------------------- + + +class GlobalInvariantRepository: + """Repository for persisted global and project-scoped invariants. + + Uses the session-factory pattern consistent with other repositories. + Unlike action/plan invariants that are children of their parent resources, + global and project invariants are standalone rows in the ``invariants`` + table filtered by scope ('global' | 'project'). + + All mutating methods flush but do NOT commit by default; the caller + or a ``UnitOfWork`` wrapper is responsible for committing the + transaction. When ``auto_commit`` is ``True`` (e.g. CLI usage + outside a UoW), each method commits and closes its own session. + """ + + def __init__( + self, + session_factory: Callable[[], Session], + *, + auto_commit: bool = False, + ) -> None: + """Initialise with a callable that returns a new SQLAlchemy Session. + + Args: + session_factory: Factory returning a new SQLAlchemy ``Session``. + auto_commit: When ``True``, each public method commits and + closes its session automatically. Useful for CLI commands + that operate outside a ``UnitOfWork``. + """ + self._session_factory = session_factory + self._auto_commit = auto_commit + + def _session(self) -> Session: + """Convenience helper to obtain a session.""" + return self._session_factory() + + @database_retry + def create(self, invariant: Any) -> Any: + """Persist a new global/project ``Invariant`` domain object. + + Args: + invariant: A domain ``Invariant`` instance with scope 'global' or + 'project'. + + Returns: + The invariant after persistence. + + Raises: + DatabaseError: On transient or unexpected DB errors. + """ + session = self._session() + try: + db_model = InvariantModel.from_domain(invariant) + session.add(db_model) + session.flush() + if self._auto_commit: + session.commit() + session.close() + return invariant + except IntegrityError as exc: + session.rollback() + raise DatabaseError(f"Failed to create global invariant: {exc}") from exc + except (OperationalError, SQLAlchemyDatabaseError) as exc: + session.rollback() + raise DatabaseError(f"Failed to create global invariant: {exc}") from exc + + @database_retry + def list_scope( + self, + scope: InvariantScope | None = None, + ) -> list[Any]: + """List global (or project-scoped) invariants. + + Args: + scope: Optional scope filter ('global' or 'project'). + + Returns: + List of ``Invariant`` domain objects. + """ + session = self._session() + try: + query = session.query(InvariantModel).filter( + InvariantModel.active == True, # noqa: E712 + ) + if scope is not None: + query = query.filter(InvariantModel.scope == scope.value) + rows = query.order_by(InvariantModel.created_at).all() + return [row.to_domain() for row in rows] + except (OperationalError, SQLAlchemyDatabaseError) as exc: + raise DatabaseError(f"Failed to list global invariants: {exc}") from exc + finally: + if self._auto_commit: + session.close() + + @database_retry + def remove(self, invariant_id: str) -> bool: + """Soft-delete a global/project invariant by setting ``active=False``. + + Args: + invariant_id: ULID of the invariant to remove. + + Returns: + ``True`` if an invariant was deactivated, ``False`` if not found. + + Raises: + DatabaseError: On transient or unexpected DB errors. + """ + session = self._session() + try: + row = session.query(InvariantModel).filter_by(id=invariant_id).first() + if row is None: + return False + row.active = False + session.flush() + if self._auto_commit: + session.commit() + session.close() + return True except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise DatabaseError( - f"Failed to delete correction attempt {correction_attempt_id}: {exc}" + f"Failed to remove global invariant {invariant_id}: {exc}", + ) from exc + + @database_retry + def get_by_id(self, invariant_id: str) -> Any | None: + """Look up a single global/project invariant by ID. + + Args: + invariant_id: ULID of the invariant. + + Returns: + An ``Invariant`` domain object, or ``None``. + """ + session = self._session() + try: + row = session.query(InvariantModel).filter_by(id=invariant_id).first() + if row is None: + return None + return row.to_domain() + except (OperationalError, SQLAlchemyDatabaseError) as exc: + raise DatabaseError( + f"Failed to get global invariant {invariant_id}: {exc}", ) from exc