diff --git a/docker-compose.yml b/docker-compose.yml index cd997179d..9b0f7be31 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -13,6 +13,7 @@ services: image: postgres:16-alpine environment: POSTGRES_USER: cleveragents + # WARNING: Development credentials only — do not use in production POSTGRES_PASSWORD: cleveragents POSTGRES_DB: cleveragents ports: diff --git a/features/postgresql_backend.feature b/features/postgresql_backend.feature index 5e7ef9c57..c6fd665dd 100644 --- a/features/postgresql_backend.feature +++ b/features/postgresql_backend.feature @@ -15,10 +15,10 @@ Feature: PostgreSQL Storage Backend for Server Mode Then the resolved database URL should start with "sqlite" @settings @database_url - Scenario: Server mode with default URL resolves to PostgreSQL + Scenario: Server mode with default URL raises ValueError Given I create settings with server_mode enabled And the database_url is the default SQLite value - Then the resolved database URL should start with "postgresql" + Then resolving the database URL should raise a ValueError @settings @database_url Scenario: Server mode with explicit PostgreSQL URL uses that URL diff --git a/features/steps/postgresql_backend_steps.py b/features/steps/postgresql_backend_steps.py index e2a5ef595..22a0154cc 100644 --- a/features/steps/postgresql_backend_steps.py +++ b/features/steps/postgresql_backend_steps.py @@ -136,6 +136,17 @@ def step_settings_custom_pool(context: Context) -> None: # --------------------------------------------------------------------------- +@then("resolving the database URL should raise a ValueError") +def step_resolve_url_raises_value_error(context: Context) -> None: + """Assert that resolve_database_url raises ValueError in server mode.""" + try: + context.settings.resolve_database_url() + except ValueError: + pass # expected + else: + raise AssertionError("Expected ValueError from resolve_database_url()") + + @then('the resolved database URL should start with "{prefix}"') def step_resolved_url_starts_with(context: Context, prefix: str) -> None: """Assert the resolved database URL prefix.""" @@ -231,20 +242,20 @@ def step_engine_dialect(context: Context, dialect: str) -> None: @then("the UnitOfWork pool_size should be {value:d}") def step_uow_pool_size(context: Context, value: int) -> None: - """Assert UnitOfWork._pool_size.""" - assert context.uow._pool_size == value + """Assert UnitOfWork.pool_size via read-only property.""" + assert context.uow.pool_size == value @then("the UnitOfWork max_overflow should be {value:d}") def step_uow_max_overflow(context: Context, value: int) -> None: - """Assert UnitOfWork._max_overflow.""" - assert context.uow._max_overflow == value + """Assert UnitOfWork.max_overflow via read-only property.""" + assert context.uow.max_overflow == value @then("the UnitOfWork pool_recycle should be {value:d}") def step_uow_pool_recycle(context: Context, value: int) -> None: - """Assert UnitOfWork._pool_recycle.""" - assert context.uow._pool_recycle == value + """Assert UnitOfWork.pool_recycle via read-only property.""" + assert context.uow.pool_recycle == value # --------------------------------------------------------------------------- @@ -268,9 +279,13 @@ _PORTABLE_TYPE_NAMES = frozenset( "TIME", "TIMESTAMP", "JSON", + "JSONB", "BLOB", "LARGEBINARY", "ENUM", + "UUID", + "INTERVAL", + "ARRAY", } ) diff --git a/features/steps/uow_coverage_boost_steps.py b/features/steps/uow_coverage_boost_steps.py index 9a5bf3698..128494398 100644 --- a/features/steps/uow_coverage_boost_steps.py +++ b/features/steps/uow_coverage_boost_steps.py @@ -53,20 +53,31 @@ def step_uow_non_sqlite_url(context: Context) -> None: @when("I access the engine property with mocked create_engine") def step_access_engine_property_mocked(context: Context) -> None: """Access the engine property, mocking _ensure_database_initialized and create_engine.""" + import sys as _sys + from sqlalchemy.engine import Engine mock_engine = MagicMock(spec=Engine) - with ( - patch.object(context.uow, "_ensure_database_initialized"), - patch( - "cleveragents.infrastructure.database.unit_of_work.create_engine", - return_value=mock_engine, - ) as mock_create, - ): - engine = context.uow.engine - context.result_engine = engine - context.mock_create_engine = mock_create + # Ensure psycopg2 can be imported (it may not be installed in test env) + _needs_psycopg2_stub = "psycopg2" not in _sys.modules + if _needs_psycopg2_stub: + _sys.modules["psycopg2"] = MagicMock() + + try: + with ( + patch.object(context.uow, "_ensure_database_initialized"), + patch( + "cleveragents.infrastructure.database.unit_of_work.create_engine", + return_value=mock_engine, + ) as mock_create, + ): + engine = context.uow.engine + context.result_engine = engine + context.mock_create_engine = mock_create + finally: + if _needs_psycopg2_stub: + _sys.modules.pop("psycopg2", None) @then("the engine should be created via the non-SQLite code path") @@ -232,20 +243,31 @@ def step_uow_postgresql_url(context: Context) -> None: @when("I trigger engine creation with mocked dependencies") def step_trigger_engine_creation_mocked(context: Context) -> None: """Access engine property with mocked create_engine and migration runner.""" + import sys as _sys + from sqlalchemy.engine import Engine mock_engine = MagicMock(spec=Engine) - with ( - patch.object(context.uow, "_ensure_database_initialized"), - patch( - "cleveragents.infrastructure.database.unit_of_work.create_engine", - return_value=mock_engine, - ) as mock_create, - ): - engine = context.uow.engine - context.result_engine = engine - context.mock_create_engine = mock_create + # Ensure psycopg2 can be imported (it may not be installed in test env) + _needs_psycopg2_stub = "psycopg2" not in _sys.modules + if _needs_psycopg2_stub: + _sys.modules["psycopg2"] = MagicMock() + + try: + with ( + patch.object(context.uow, "_ensure_database_initialized"), + patch( + "cleveragents.infrastructure.database.unit_of_work.create_engine", + return_value=mock_engine, + ) as mock_create, + ): + engine = context.uow.engine + context.result_engine = engine + context.mock_create_engine = mock_create + finally: + if _needs_psycopg2_stub: + _sys.modules.pop("psycopg2", None) @then("create_engine should have been called without isolation_level") diff --git a/pyproject.toml b/pyproject.toml index 3226f6d09..bb3240fad 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,10 +49,12 @@ dependencies = [ "aiohttp>=3.13.4", # CVE-2026-34515 mitigation: open redirect vulnerability "pyyaml>=6.0.3", # Security: address known YAML parsing vulnerabilities "a2a-sdk>=0.3.0,<1.0.0", # A2A Python SDK — required transport for local (stdio) and server (HTTP) modes (ADR-047); pinned <1.0.0 (removed legacy A2AClient) - "psycopg2-binary>=2.9.9", # PostgreSQL adapter for server-mode storage ] [project.optional-dependencies] +server = [ + "psycopg2-binary>=2.9.9", # PostgreSQL adapter for server-mode storage +] tui = [ "textual>=1.0.0,<2.0.0", ] diff --git a/robot/postgresql_backend.robot b/robot/postgresql_backend.robot index e4286f171..c16548e86 100644 --- a/robot/postgresql_backend.robot +++ b/robot/postgresql_backend.robot @@ -31,10 +31,11 @@ Settings Resolve Database URL Local Mode Settings Resolve Database URL Server Mode [Documentation] Verify resolve_database_url returns PostgreSQL when server_mode is on + ... and CLEVERAGENTS_DATABASE_URL points to a PostgreSQL instance. ${script}= Catenate SEPARATOR=\n ... import os ... os.environ['CLEVERAGENTS_SERVER_MODE'] = 'true' - ... os.environ.pop('CLEVERAGENTS_DATABASE_URL', None) + ... os.environ['CLEVERAGENTS_DATABASE_URL'] = 'postgresql://localhost/cleveragents' ... from cleveragents.config.settings import Settings ... s = Settings() ... print(s.resolve_database_url()) diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index ab1b47517..03a79d5f4 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -251,6 +251,9 @@ def get_database_url() -> str: relative to ``CLEVERAGENTS_HOME`` (if set) rather than CWD, ensuring test isolation and correct data placement. + .. todo:: (#878) Use ``Settings.resolve_database_url()`` once + PostgreSQL wiring is complete so that *server_mode* is honoured. + Returns: SQLAlchemy database URL with absolute path for SQLite """ diff --git a/src/cleveragents/cli/commands/db.py b/src/cleveragents/cli/commands/db.py index 0d641d23c..bdee2b7b5 100644 --- a/src/cleveragents/cli/commands/db.py +++ b/src/cleveragents/cli/commands/db.py @@ -37,6 +37,7 @@ def _get_runner() -> MigrationRunner: from cleveragents.application.container import get_database_url from cleveragents.infrastructure.database.migration_runner import MigrationRunner + # TODO(#878): Use Settings.resolve_database_url() once PostgreSQL wiring is complete return MigrationRunner(get_database_url()) diff --git a/src/cleveragents/cli/commands/project.py b/src/cleveragents/cli/commands/project.py index 7b95d6942..9fb55de18 100644 --- a/src/cleveragents/cli/commands/project.py +++ b/src/cleveragents/cli/commands/project.py @@ -126,6 +126,7 @@ def _store_project_extras( from cleveragents.application.container import get_database_url + # TODO(#878): Use Settings.resolve_database_url() once PostgreSQL wiring is complete db_url = get_database_url() engine = create_engine(db_url, echo=False) session = sessionmaker(bind=engine, expire_on_commit=False)() diff --git a/src/cleveragents/cli/commands/system.py b/src/cleveragents/cli/commands/system.py index 31a3dccc8..63282ab9a 100644 --- a/src/cleveragents/cli/commands/system.py +++ b/src/cleveragents/cli/commands/system.py @@ -413,6 +413,8 @@ def _check_stale_locks() -> dict[str, Any]: try: from sqlalchemy.orm import Session, sessionmaker + # TODO(#878): Use Settings.resolve_database_url() + # once PostgreSQL wiring is complete. db_url = get_database_url() engine = create_engine(db_url, echo=False) diff --git a/src/cleveragents/config/settings.py b/src/cleveragents/config/settings.py index 108fe53a5..87e52b802 100644 --- a/src/cleveragents/config/settings.py +++ b/src/cleveragents/config/settings.py @@ -929,11 +929,10 @@ class Settings(BaseSettings): """Return the database URL, honouring *server_mode*. In **server mode** the database URL must point to a PostgreSQL - instance. When the user has not explicitly overridden - ``database_url`` (i.e. it still has its SQLite default), this - method returns the conventional PostgreSQL URL - ``postgresql://localhost/cleveragents`` so that the application - falls through to the correct backend. + instance. If the caller has not provided an explicit PostgreSQL + URL via ``CLEVERAGENTS_DATABASE_URL``, a ``ValueError`` is + raised to prevent silent fallback to a possibly unreachable + default. In **local mode** the existing SQLite URL is returned unchanged. @@ -942,15 +941,25 @@ class Settings(BaseSettings): Returns: A SQLAlchemy-compatible database URL string. + + Raises: + ValueError: When *server_mode* is True but no explicit + PostgreSQL URL has been configured. """ base_url = self.get_database_url(test=test) if self.server_mode and base_url.startswith("sqlite"): - return "postgresql://localhost/cleveragents" + raise ValueError( + "server_mode=True requires an explicit database URL. " + "Set CLEVERAGENTS_DATABASE_URL to a PostgreSQL connection string." + ) return base_url def is_postgresql(self) -> bool: """Return True when the active database URL targets PostgreSQL.""" - return self.resolve_database_url().startswith("postgresql") + try: + return self.resolve_database_url().startswith("postgresql") + except ValueError: + return False def is_production(self) -> bool: """Whether the runtime matches production settings.""" diff --git a/src/cleveragents/infrastructure/database/migration_runner.py b/src/cleveragents/infrastructure/database/migration_runner.py index f63b838e8..c8205f668 100644 --- a/src/cleveragents/infrastructure/database/migration_runner.py +++ b/src/cleveragents/infrastructure/database/migration_runner.py @@ -167,9 +167,12 @@ class MigrationRunner: ) else: engine = create_engine(self.database_url) - with engine.connect() as connection: - context = MigrationContext.configure(connection) - return context.get_current_revision() + try: + with engine.connect() as connection: + context = MigrationContext.configure(connection) + return context.get_current_revision() + finally: + engine.dispose() def get_pending_migrations(self) -> list[str]: """Get list of pending migrations. diff --git a/src/cleveragents/infrastructure/database/unit_of_work.py b/src/cleveragents/infrastructure/database/unit_of_work.py index 9a8d2c864..80cec9fb7 100644 --- a/src/cleveragents/infrastructure/database/unit_of_work.py +++ b/src/cleveragents/infrastructure/database/unit_of_work.py @@ -43,6 +43,10 @@ class UnitOfWork: database_url: str, prompt_for_migration: Callable[[str], bool] | None = None, require_confirmation: bool = True, + # Pool defaults are intentionally duplicated from Settings.db_pool_* + # so that UnitOfWork remains usable without a Settings instance + # (e.g. in tests or standalone scripts). When constructed via the + # DI container the caller should forward Settings values explicitly. pool_size: int = 5, max_overflow: int = 10, pool_recycle: int = 1800, @@ -60,6 +64,16 @@ class UnitOfWork: max_overflow: Extra connections above *pool_size* under load. pool_recycle: Seconds before a pooled connection is recycled. """ + # Validate arguments per CONTRIBUTING.md fail-fast principles + if not database_url: + raise ValueError("database_url must be a non-empty string") + if pool_size < 1: + raise ValueError(f"pool_size must be >= 1, got {pool_size}") + if max_overflow < 0: + raise ValueError(f"max_overflow must be >= 0, got {max_overflow}") + if pool_recycle < -1: + raise ValueError(f"pool_recycle must be >= -1, got {pool_recycle}") + self.database_url = database_url self._engine: Engine | None = None self._session_factory: sessionmaker[Session] | None = None @@ -70,6 +84,21 @@ class UnitOfWork: self._max_overflow = max_overflow self._pool_recycle = pool_recycle + @property + def pool_size(self) -> int: + """Read-only access to the configured connection-pool size.""" + return self._pool_size + + @property + def max_overflow(self) -> int: + """Read-only access to the configured max overflow.""" + return self._max_overflow + + @property + def pool_recycle(self) -> int: + """Read-only access to the configured pool recycle interval.""" + return self._pool_recycle + @property def engine(self) -> Engine: """Get or create database engine.""" @@ -104,10 +133,24 @@ class UnitOfWork: else: # PostgreSQL (or other non-SQLite): use connection pooling # suitable for multi-user server mode. + # Use READ COMMITTED isolation level for concurrent multi-user access. + # This allows non-repeatable reads and phantom reads, which is + # acceptable for the concurrent plan execution use case where + # individual plan steps are atomic operations. For stronger + # consistency, callers can use explicit locking or application- + # level coordination. + try: + import psycopg2 # noqa: F401 + except ImportError as exc: + raise ImportError( + "PostgreSQL support requires the 'server' extra: " + "pip install cleveragents[server]" + ) from exc self._engine = create_engine( self.database_url, echo=False, # Set to True for SQL debugging future=True, # Use SQLAlchemy 2.0 style + isolation_level="READ COMMITTED", pool_size=self._pool_size, max_overflow=self._max_overflow, pool_recycle=self._pool_recycle, @@ -151,6 +194,25 @@ class UnitOfWork: finally: session.close() + def dispose(self) -> None: + """Release all database connections and dispose the engine. + + Must be called when the UnitOfWork is no longer needed, especially + for PostgreSQL backends where the connection pool holds TCP connections. + """ + if self._engine is not None and self.database_url != "sqlite:///:memory:": + self._engine.dispose() + self._engine = None + self._session_factory = None + + def __enter__(self) -> UnitOfWork: + """Enter context manager.""" + return self + + def __exit__(self, *args: object) -> None: + """Exit context manager and dispose resources.""" + self.dispose() + def _ensure_database_initialized(self, force: bool = False) -> None: """Run migrations if needed before database access.""" if self._database_initialized and not force: