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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user