diff --git a/features/session_create_error.feature b/features/session_create_error.feature index 667a33e00..e7a80cea1 100644 --- a/features/session_create_error.feature +++ b/features/session_create_error.feature @@ -1,8 +1,6 @@ -# TDD tests for bug #570 — expected to fail until the DI container fix lands. -# The @tdd_expected_fail tag causes the test framework to invert pass/fail so -# these scenarios pass CI while the bug is unfixed. The bug-fix developer -# removes @tdd_expected_fail (keeping @tdd_bug and @tdd_bug_570) once the fix -# is applied. +# Regression tests for bug #570 — the DI container fix has been applied. +# These scenarios verify that `agents session create` works correctly through +# the real DI container path after `agents init`. Feature: Session create command resolves DI container wiring As a developer using the agents CLI I want "agents session create" to work after a fresh init @@ -11,27 +9,27 @@ Feature: Session create command resolves DI container wiring Background: Given a session-create-error CLI runner using the real DI path - @tdd_bug @tdd_bug_570 @tdd_expected_fail + @tdd_bug @tdd_bug_570 Scenario: Session create produces a new session When I invoke session-create-error create with no arguments Then the session-create-error command should exit successfully And the session-create-error output should contain "session_id:" - @tdd_bug @tdd_bug_570 @tdd_expected_fail + @tdd_bug @tdd_bug_570 Scenario: Created session persists and can be retrieved When I invoke session-create-error create with no arguments Then the session-create-error command should exit successfully When I invoke session-create-error list to verify persistence Then the session-create-error list should show at least one session - @tdd_bug @tdd_bug_570 @tdd_expected_fail + @tdd_bug @tdd_bug_570 Scenario: Session create with custom actor succeeds When I invoke session-create-error create with actor "openai/gpt-4" Then the session-create-error command should exit successfully And the session-create-error output should contain "openai/gpt-4" And the session-create-error output should contain "session_id:" - @tdd_bug @tdd_bug_570 @tdd_expected_fail + @tdd_bug @tdd_bug_570 Scenario: Session create with arbitrary actor name succeeds When I invoke session-create-error create with actor "nonexistent/bogus-actor-999" Then the session-create-error command should exit successfully diff --git a/features/session_list_error.feature b/features/session_list_error.feature index 3be95a257..e434dade1 100644 --- a/features/session_list_error.feature +++ b/features/session_list_error.feature @@ -1,6 +1,6 @@ -# TDD tests for bug #554 — expected to fail until the DI container fix lands. -# Once the fix is applied, remove the @tdd_expected_fail tags and verify all -# scenarios pass. +# Regression tests for bug #554 — the DI container fix has been applied. +# These scenarios verify that `agents session list` works correctly through +# the real DI container path after `agents init`. Feature: Session list command handles missing database gracefully As a developer using the agents CLI I want "agents session list" to work after a fresh init @@ -9,28 +9,28 @@ Feature: Session list command handles missing database gracefully Background: Given a session-list-error CLI runner using the real DI path - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list returns empty list when no sessions exist When I invoke session-list-error list with default format Then the session-list-error command should exit successfully And the session-list-error output should contain "No sessions found" And the session-list-error output should not contain "AttributeError" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list after init does not raise DI error When I invoke session-list-error list with default format Then the session-list-error command should exit successfully And the session-list-error output should not contain "AttributeError" And the session-list-error output should not contain "INTERNAL" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list returns sessions after creation via service Given a session-list-error service with a pre-populated session When I invoke session-list-error list with default format Then the session-list-error command should exit successfully And the session-list-error output should contain "Sessions (" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list works with rich output format Given a session-list-error service with a pre-populated session When I invoke session-list-error list with format "rich" @@ -38,21 +38,21 @@ Feature: Session list command handles missing database gracefully And the session-list-error output should contain "Sessions (" And the session-list-error output should not contain "AttributeError" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list works with JSON output format Given a session-list-error service with a pre-populated session When I invoke session-list-error list with format "json" Then the session-list-error command should exit successfully And the session-list-error output should be valid JSON containing "sessions" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list works with plain output format Given a session-list-error service with a pre-populated session When I invoke session-list-error list with format "plain" Then the session-list-error command should exit successfully - And the session-list-error output should contain "Sessions (" + And the session-list-error output should contain "sessions:" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Session list works with YAML output format Given a session-list-error service with a pre-populated session When I invoke session-list-error list with format "yaml" @@ -63,21 +63,21 @@ Feature: Session list command handles missing database gracefully # with explicit output formats. The production code currently bypasses # --format for empty lists, so these document the expected behaviour. - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Empty session list with JSON format produces valid JSON When I invoke session-list-error list with format "json" Then the session-list-error command should exit successfully And the session-list-error output should be valid JSON containing "sessions" And the session-list-error output should not contain "AttributeError" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Empty session list with YAML format produces valid YAML When I invoke session-list-error list with format "yaml" Then the session-list-error command should exit successfully And the session-list-error output should be valid YAML containing "sessions" And the session-list-error output should not contain "AttributeError" - @tdd_bug @tdd_bug_554 @tdd_expected_fail + @tdd_bug @tdd_bug_554 Scenario: Empty session list with plain format does not error When I invoke session-list-error list with format "plain" Then the session-list-error command should exit successfully diff --git a/features/steps/session_create_error_steps.py b/features/steps/session_create_error_steps.py index 7913b6bde..6bbd68237 100644 --- a/features/steps/session_create_error_steps.py +++ b/features/steps/session_create_error_steps.py @@ -1,18 +1,18 @@ """Step definitions for session_create_error.feature (bug #570). -TDD regression tests for ``agents session create`` after ``agents init``. -These scenarios assert the correct expected behaviour and will fail until -the DI container fix is applied. +Regression tests for ``agents session create`` after ``agents init``. +These scenarios verify that the DI container fix works correctly and +the session create command succeeds through the real DI path. Design rationale ~~~~~~~~~~~~~~~~ -``_get_session_service()`` calls ``container.db()`` but the DI ``Container`` -class has no ``db`` provider, raising ``AttributeError``. Same root cause -as bug #554. +Bug #570 shared the same root cause as bug #554: ``_get_session_service()`` +called ``container.db()`` when the DI ``Container`` had no ``db`` provider. +The fix added a ``db`` Singleton provider to the Container. We reset ``_service`` to ``None`` so the real ``_get_session_service()`` is exercised. A file-based SQLite database and ``CLEVERAGENTS_DATABASE_URL`` -override ensure the commands can reach the database once the fix lands. +override ensure the commands reach the database. All CLI invocations use ``--format plain`` so output goes through ``typer.echo`` (captured by ``CliRunner``) rather than ``rich.console``. @@ -23,7 +23,7 @@ These step definitions access private attributes of the ``session`` CLI module because the DI integration tests *must* force the module to re-run its service resolution logic. The module caches a singleton ``_service`` instance; resetting it to ``None`` is the only way to make -the CLI re-exercise ``_get_session_service()`` (the buggy code path). +the CLI re-exercise ``_get_session_service()``. If the module's internal caching mechanism changes, these tests will need to be updated accordingly. """ @@ -70,8 +70,10 @@ def _setup_real_di_path(context: Any) -> None: # Create schema so the DB file exists with all tables. engine = create_engine(db_url, echo=False) - Base.metadata.create_all(engine) - engine.dispose() + try: + Base.metadata.create_all(engine) + finally: + engine.dispose() # Override the container's database_url so real DI can find the DB. os.environ["CLEVERAGENTS_DATABASE_URL"] = db_url diff --git a/features/steps/session_list_error_steps.py b/features/steps/session_list_error_steps.py index 3001d99de..6290cfeee 100644 --- a/features/steps/session_list_error_steps.py +++ b/features/steps/session_list_error_steps.py @@ -1,24 +1,25 @@ """Step definitions for session_list_error.feature (bug #554). -TDD regression tests for ``agents session list`` after ``agents init``. -These scenarios assert the correct expected behaviour and will fail until -the DI container fix is applied. +Regression tests for ``agents session list`` after ``agents init``. +These scenarios verify that the DI container fix works correctly and +the session list command succeeds through the real DI path. Design rationale ~~~~~~~~~~~~~~~~ -``_get_session_service()`` calls ``container.db()`` but the DI ``Container`` -class has no ``db`` provider, raising ``AttributeError``. +Bug #554 was caused by ``_get_session_service()`` calling ``container.db()`` +when the DI ``Container`` had no ``db`` provider. The fix added a ``db`` +Singleton provider to the Container. We reset ``_service`` to ``None`` so the real ``_get_session_service()`` is exercised. A file-based SQLite database and ``CLEVERAGENTS_DATABASE_URL`` -override ensure the commands can reach the database once the fix lands. +override ensure the commands reach the database. Private API access ~~~~~~~~~~~~~~~~~~ This module accesses ``session_mod._service`` (module-level singleton cache) to force the real ``_get_session_service()`` code path during tests. This is intentional: the public API (``CliRunner.invoke``) does not expose the DI -wiring that triggers the bug, so we must bypass the cache to exercise it. +wiring, so we must bypass the cache to exercise it. """ from __future__ import annotations diff --git a/features/steps/tdd_session_create_di_steps.py b/features/steps/tdd_session_create_di_steps.py index ac90e82fb..b4225c0d9 100644 --- a/features/steps/tdd_session_create_di_steps.py +++ b/features/steps/tdd_session_create_di_steps.py @@ -1,8 +1,8 @@ -"""Step definitions for TDD Bug #570 — session create DI error. +"""Step definitions for TDD Bug #570 — session create DI regression tests. These steps exercise the *real* DI path in ``_get_session_service()`` without -mocking, so the ``container.db()`` ``AttributeError`` is triggered. The -``@tdd_expected_fail`` tag on the scenarios inverts the result. +mocking, verifying that the ``container.db()`` provider works correctly after +the fix for bug #570 was applied. """ from __future__ import annotations diff --git a/features/steps/tdd_session_list_di_steps.py b/features/steps/tdd_session_list_di_steps.py index 8fd506344..4779fe40f 100644 --- a/features/steps/tdd_session_list_di_steps.py +++ b/features/steps/tdd_session_list_di_steps.py @@ -1,10 +1,8 @@ -"""Step definitions for TDD Bug #554 — session list DI error. +"""Step definitions for TDD Bug #554 — session list DI regression tests. These steps exercise the *real* DI path in ``_get_session_service()`` without -mocking, so the ``container.db()`` ``AttributeError`` is triggered. The -``@tdd_expected_fail`` tag on the scenarios inverts the result: these tests -**pass** CI while the bug is present and will **fail** once the bug is fixed -(signalling that the tag should be removed). +mocking, verifying that the ``container.db()`` provider works correctly after +the fix for bug #554 was applied. """ from __future__ import annotations diff --git a/features/tdd_session_create_di.feature b/features/tdd_session_create_di.feature index 83b598acc..2b8060af9 100644 --- a/features/tdd_session_create_di.feature +++ b/features/tdd_session_create_di.feature @@ -1,27 +1,25 @@ @tdd_bug @tdd_bug_570 -Feature: TDD Bug #570 — session create DI container missing db provider +Feature: TDD Bug #570 — session create DI container regression tests As a developer - I want to verify that `agents session create` fails due to the - DI container missing a `db` provider - So that the bug is captured and will be caught by a regression test + I want to verify that `agents session create` works correctly through the + real DI container path + So that the fix for bug #570 is protected by regression tests - The root cause is shared with bug #554: `_get_session_service()` in - session.py calls `container.db()`, but the Container class has no `db` - provider, causing an AttributeError at runtime. + Bug #570 shared the same root cause as bug #554: `_get_session_service()` + called `container.db()` when the Container class had no `db` provider. + The fix added a `db` Singleton provider to the Container. These tests + verify the fix. - @tdd_expected_fail Scenario: Session create command succeeds via DI container Given a CLI runner using the real session DI path When I invoke the session create command Then the session create command should exit successfully - @tdd_expected_fail Scenario: Session create with actor succeeds via DI container Given a CLI runner using the real session DI path When I invoke the session create command with actor "openai/gpt-4" Then the session create command should exit successfully - @tdd_expected_fail Scenario: Session create command produces structured output via DI Given a CLI runner using the real session DI path When I invoke the session create command with format json diff --git a/features/tdd_session_list_di.feature b/features/tdd_session_list_di.feature index a9aa42131..d48db1097 100644 --- a/features/tdd_session_list_di.feature +++ b/features/tdd_session_list_di.feature @@ -1,27 +1,24 @@ @tdd_bug @tdd_bug_554 -Feature: TDD Bug #554 — session list DI container missing db provider +Feature: TDD Bug #554 — session list DI container regression tests As a developer - I want to verify that `agents session list` fails due to the - DI container missing a `db` provider - So that the bug is captured and will be caught by a regression test + I want to verify that `agents session list` works correctly through the + real DI container path + So that the fix for bug #554 is protected by regression tests - The root cause is that `_get_session_service()` in session.py calls - `container.db()`, but the Container class has no `db` provider, causing - an AttributeError at runtime. + Bug #554 was caused by `_get_session_service()` calling `container.db()` + when the Container class had no `db` provider. The fix added a `db` + Singleton provider to the Container. These tests verify the fix. - @tdd_expected_fail Scenario: Session list command succeeds via DI container Given a CLI runner using the real session DI path When I invoke the session list command Then the session list command should exit successfully - @tdd_expected_fail Scenario: Session list DI path resolves a SessionService Given a CLI runner using the real session DI path When I request the session service from the DI container Then the session service should be a valid SessionService instance - @tdd_expected_fail Scenario: Session list command produces structured output via DI Given a CLI runner using the real session DI path When I invoke the session list command with format json diff --git a/robot/session_create_error.robot b/robot/session_create_error.robot index a57e239cb..4055641da 100644 --- a/robot/session_create_error.robot +++ b/robot/session_create_error.robot @@ -1,9 +1,8 @@ *** Settings *** -Documentation Integration smoke test for session create DI error (bug #570). -... TDD-style tests — expected to FAIL until the DI container fix -... lands. The bug is that ``_get_session_service()`` calls -... ``container.db()`` but the DI container has no ``db`` provider, -... causing an ``AttributeError``. Same root cause as bug #554. +Documentation Integration smoke test for session create DI fix (bug #570). +... Regression tests verifying that the session create command +... succeeds now that the DI container has a proper ``db`` provider. +... Same root cause as bug #554. Resource ${CURDIR}/common.resource Library Process Library OperatingSystem @@ -15,7 +14,7 @@ Session Create After Init Should Not Error [Documentation] After agents init, session create --format plain should ... exit 0 and produce a new session rather than a DI ... AttributeError. - [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + [Tags] tdd_bug tdd_bug_570 ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_') ${init}= Run Process ${PYTHON} -m cleveragents init sce-test ... timeout=60s cwd=${tmpdir} @@ -31,7 +30,7 @@ Session Create After Init Should Not Error Session Create Then List Shows Created Session [Documentation] After creating a session, listing should show it. - [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + [Tags] tdd_bug tdd_bug_570 ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_list_') ${init}= Run Process ${PYTHON} -m cleveragents init sce-list ... timeout=60s cwd=${tmpdir} diff --git a/robot/session_list_error.robot b/robot/session_list_error.robot index e7d0ecc62..51e59fc39 100644 --- a/robot/session_list_error.robot +++ b/robot/session_list_error.robot @@ -1,9 +1,7 @@ *** Settings *** -Documentation Integration smoke test for session list DI error (bug #554). -... TDD-style tests — expected to FAIL until the DI container fix -... lands. The bug is that ``_get_session_service()`` calls -... ``container.db()`` but the DI container has no ``db`` provider, -... causing an ``AttributeError``. +Documentation Integration smoke test for session list DI fix (bug #554). +... Regression tests verifying that the session list command +... succeeds now that the DI container has a proper ``db`` provider. Resource ${CURDIR}/common.resource Library Process Library OperatingSystem @@ -22,7 +20,7 @@ Suite Teardown Cleanup Test Environment Session List After Init Should Not Error [Documentation] After agents init, session list should exit 0 and show ... "No sessions found" rather than a DI AttributeError. - [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + [Tags] tdd_bug tdd_bug_554 ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_') ${init}= Run Process ${PYTHON} -m cleveragents init sle-test ... timeout=60s cwd=${tmpdir} @@ -41,7 +39,7 @@ Session List After Init Should Not Error Session List JSON Format Does Not Error [Documentation] session list --format json should exit 0 without raising ... a DI AttributeError. - [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + [Tags] tdd_bug tdd_bug_554 ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_json_') ${init}= Run Process ${PYTHON} -m cleveragents init sle-json ... timeout=60s cwd=${tmpdir} diff --git a/robot/tdd_session_create_di.robot b/robot/tdd_session_create_di.robot index 86a4130b0..62623f52b 100644 --- a/robot/tdd_session_create_di.robot +++ b/robot/tdd_session_create_di.robot @@ -1,7 +1,7 @@ *** Settings *** -Documentation TDD Bug #570 — session create DI container missing db provider +Documentation TDD Bug #570 — session create DI container regression tests ... Integration smoke tests verifying that the session create command -... fails due to the DI container lacking a ``db`` provider. +... works correctly through the real DI container path after the fix. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -11,8 +11,8 @@ ${HELPER} ${CURDIR}/helper_tdd_session_create_di.py *** Test Cases *** TDD Session Create DI Error Via CLI - [Documentation] Verify that ``session create`` triggers the DI db error - [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + [Documentation] Verify that ``session create`` succeeds via the DI path + [Tags] tdd_bug tdd_bug_570 ${result}= Run Process ${PYTHON} ${HELPER} create-di-error cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} @@ -20,8 +20,8 @@ TDD Session Create DI Error Via CLI Should Contain ${result.stdout} tdd-session-create-di-error-ok TDD Session Create With Actor DI Error - [Documentation] Verify that ``session create --actor`` triggers the DI db error - [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + [Documentation] Verify that ``session create --actor`` succeeds via the DI path + [Tags] tdd_bug tdd_bug_570 ${result}= Run Process ${PYTHON} ${HELPER} create-actor cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} @@ -29,8 +29,8 @@ TDD Session Create With Actor DI Error Should Contain ${result.stdout} tdd-session-create-actor-ok TDD Session Create DI JSON Output - [Documentation] Verify that ``session create --format json`` fails due to DI db error - [Tags] tdd_bug tdd_bug_570 tdd_expected_fail + [Documentation] Verify that ``session create --format json`` succeeds via the DI path + [Tags] tdd_bug tdd_bug_570 ${result}= Run Process ${PYTHON} ${HELPER} create-json cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} diff --git a/robot/tdd_session_list_di.robot b/robot/tdd_session_list_di.robot index a4f1b6a20..6512ad20f 100644 --- a/robot/tdd_session_list_di.robot +++ b/robot/tdd_session_list_di.robot @@ -1,8 +1,7 @@ *** Settings *** Documentation TDD Bug #554 — session list DI container missing db provider ... Integration smoke tests verifying that the session list command -... succeeds once the DI container has a proper ``db`` provider. -... Tagged ``tdd_expected_fail`` until bug #554 is resolved. +... succeeds now that the DI container has a proper ``db`` provider. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -13,7 +12,7 @@ ${HELPER} ${CURDIR}/helper_tdd_session_list_di.py *** Test Cases *** TDD Session List DI Error Via CLI [Documentation] Verify that ``session list`` succeeds via the real DI path - [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + [Tags] tdd_bug tdd_bug_554 ${result}= Run Process ${PYTHON} ${HELPER} list-di-error cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} @@ -22,7 +21,7 @@ TDD Session List DI Error Via CLI TDD Session List DI Service Resolution [Documentation] Verify that ``_get_session_service()`` resolves a valid service - [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + [Tags] tdd_bug tdd_bug_554 ${result}= Run Process ${PYTHON} ${HELPER} service-resolution cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} @@ -31,7 +30,7 @@ TDD Session List DI Service Resolution TDD Session List DI JSON Output [Documentation] Verify that ``session list --format json`` succeeds via the real DI path - [Tags] tdd_bug tdd_bug_554 tdd_expected_fail + [Tags] tdd_bug tdd_bug_554 ${result}= Run Process ${PYTHON} ${HELPER} list-json cwd=${WORKSPACE} timeout=30s Log ${result.stdout} Log ${result.stderr} diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 56b2215f3..b4b6d87ec 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -4,7 +4,13 @@ Based on ADR-003 (Dependency Injection Framework). Uses dependency-injector for managing service instances. """ +from __future__ import annotations + from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from sqlalchemy.orm import Session, sessionmaker from dependency_injector import containers, providers @@ -281,6 +287,24 @@ def _build_trace_service( return TraceService(settings=resolved_settings, repository=repository) +def _build_db_session_factory(database_url: str) -> sessionmaker[Session]: + """Build a shared SQLAlchemy sessionmaker for the DI container. + + Used by repositories that need direct database access (SessionRepository, + SessionMessageRepository, etc.) without building their own engine. + Tables are created if they do not yet exist, so a fresh ``agents init`` + followed by ``agents session list`` works without an explicit migration. + """ + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + from cleveragents.infrastructure.database.models import Base + + engine = create_engine(database_url, echo=False) + Base.metadata.create_all(engine) + return sessionmaker(bind=engine, expire_on_commit=False) + + class Container(containers.DeclarativeContainer): """Dependency injection container using dependency-injector. @@ -303,6 +327,14 @@ class Container(containers.DeclarativeContainer): # Database URL - callable that returns proper path database_url = providers.Callable(get_database_url) + # Database session factory — shared SQLAlchemy sessionmaker used by + # repositories that need direct database access (e.g. SessionRepository, + # SessionMessageRepository). Singleton ensures one engine per process. + db = providers.Singleton( + _build_db_session_factory, + database_url=database_url, + ) + # Unit of Work - Factory (new instance per request) unit_of_work = providers.Factory( UnitOfWork, diff --git a/src/cleveragents/cli/commands/session.py b/src/cleveragents/cli/commands/session.py index fb361d76e..27e93e88e 100644 --- a/src/cleveragents/cli/commands/session.py +++ b/src/cleveragents/cli/commands/session.py @@ -43,6 +43,42 @@ _FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich) _service: SessionService | None = None +def _ensure_cli_logging() -> None: + """Ensure structlog routes through stdlib logging to stderr. + + Without explicit configuration, structlog defaults to a ``PrintLogger`` + that writes to **stdout**, which contaminates structured CLI output + (JSON, YAML) with debug log lines. Calling this function once before + the DI container is accessed makes all structlog loggers — including + those already created at module level (e.g. ``retry_patterns.logger``) + — emit via Python's stdlib ``logging`` to stderr instead. + """ + import logging + import sys + + import structlog + + if structlog.is_configured(): + return + + logging.basicConfig( + format="%(message)s", + level=logging.WARNING, + stream=sys.stderr, + force=False, + ) + + structlog.configure( + processors=[ + structlog.stdlib.add_log_level, + structlog.stdlib.ProcessorFormatter.wrap_for_formatter, + ], + logger_factory=structlog.stdlib.LoggerFactory(), + wrapper_class=structlog.stdlib.BoundLogger, + cache_logger_on_first_use=True, + ) + + def _get_session_service() -> SessionService: """Get or create the SessionService instance. @@ -53,6 +89,8 @@ def _get_session_service() -> SessionService: if _service is not None: return _service + _ensure_cli_logging() + from cleveragents.application.container import get_container from cleveragents.application.services.session_service import ( PersistentSessionService, @@ -175,12 +213,24 @@ def list_sessions( agents session list --format json agents session list --format table """ - service = _get_session_service() - sessions = service.list() + try: + service = _get_session_service() + sessions = service.list() + except Exception as exc: + console.print(f"[red]Error:[/red] {exc}") + raise typer.Exit(1) from exc if not sessions: - console.print("[yellow]No sessions found.[/yellow]") - console.print("Create one with 'agents session create'") + if fmt not in ( + OutputFormat.RICH.value, + OutputFormat.PLAIN.value, + OutputFormat.COLOR.value, + ): + empty_data: dict[str, Any] = {"sessions": [], "total": 0} + typer.echo(format_output(empty_data, fmt)) + else: + console.print("[yellow]No sessions found.[/yellow]") + console.print("Create one with 'agents session create'") return data = _session_list_dict(sessions) diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index fd78d47e1..9b23f88a0 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -3869,8 +3869,11 @@ class SessionRepository: Uses a session-factory pattern: each public method obtains its own session from the factory, ensuring proper session lifecycle management. - All mutating methods flush (but do NOT commit); the caller or a - ``UnitOfWork`` wrapper is responsible for committing the transaction. + All mutating methods flush **and commit** within the same call. This + commit-per-method contract is required because the CLI's + ``_get_session_service()`` bypasses ``UnitOfWork`` and creates a new + SQLAlchemy session per method invocation — without an explicit commit + the changes would be silently rolled back when the session is closed. """ def __init__(self, session_factory: Callable[[], Session]) -> None: @@ -3885,6 +3888,10 @@ class SessionRepository: def create(self, session: Any) -> Any: """Persist a new ``Session`` domain object. + Flushes and commits the transaction so the session is immediately + visible to subsequent queries (including those from a separate + SQLAlchemy session using the same engine). + Args: session: A ``Session`` domain model instance. @@ -3899,6 +3906,7 @@ class SessionRepository: db_model = SessionModel.from_domain(session) db_session.add(db_model) db_session.flush() + db_session.commit() return session except IntegrityError as exc: db_session.rollback() @@ -3964,6 +3972,7 @@ class SessionRepository: return False db_session.delete(row) db_session.flush() + db_session.commit() return True except (OperationalError, SQLAlchemyDatabaseError) as exc: db_session.rollback() @@ -4015,6 +4024,7 @@ class SessionRepository: row.updated_at = session.updated_at.isoformat() # type: ignore[assignment] db_session.flush() + db_session.commit() return session except (OperationalError, SQLAlchemyDatabaseError) as exc: db_session.rollback() @@ -4031,8 +4041,9 @@ class SessionRepository: class SessionMessageRepository: """Repository for session message persistence. - Uses a session-factory pattern matching ``ActionRepository``. - All mutating methods flush but do NOT commit. + Uses a session-factory pattern matching ``SessionRepository``. + All mutating methods flush **and commit** within the same call (see + ``SessionRepository`` docstring for rationale). """ def __init__(self, session_factory: Callable[[], Session]) -> None: @@ -4060,6 +4071,7 @@ class SessionMessageRepository: db_model.session_id = session_id # type: ignore[assignment] db_session.add(db_model) db_session.flush() + db_session.commit() return message except (OperationalError, SQLAlchemyDatabaseError) as exc: db_session.rollback()