fix(cli): handle missing database in session list command #680
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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.
|
||||
CoreRasurae
commented
[M2 — MEDIUM: Stale documentation] Line 4 says "fails due to the DI container lacking a **[M2 — MEDIUM: Stale documentation]** Line 4 says *"fails due to the DI container lacking a `db` provider"* but the fix has been applied and `@tdd_expected_fail` tags removed. Update documentation to reflect these are now regression tests.
|
||||
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}
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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)
|
||||
|
CoreRasurae
commented
M3/M8 (MEDIUM): **M3/M8 (MEDIUM):** `_build_db_session_factory()` calls `Base.metadata.create_all(engine)` but none of the other 6 `_build_*` functions do. This creates an inconsistency: session tables auto-create on a fresh DB, but other services won't work without explicit migration. Additionally, `create_all` bypasses Alembic migrations, potentially creating tables with outdated schemas in production databases.
|
||||
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,
|
||||
|
||||
@@ -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:
|
||||
|
CoreRasurae
commented
L1 (LOW - Architecture): This logging fix addresses a real stdout contamination issue, but it's scoped only to the session module. If other CLI command modules have the same problem, they'll need their own copies. Consider moving this to the CLI entrypoint (e.g., the main Typer app callback) so all commands benefit. **L1 (LOW - Architecture):** This logging fix addresses a real stdout contamination issue, but it's scoped only to the session module. If other CLI command modules have the same problem, they'll need their own copies. Consider moving this to the CLI entrypoint (e.g., the main Typer app callback) so all commands benefit.
|
||||
"""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(
|
||||
|
CoreRasurae
commented
[M5 — MEDIUM: **[M5 — MEDIUM: `force=True` may override application logging]** `logging.basicConfig(force=True)` forcefully reconfigures the root logger, potentially overriding logging setup from other modules. Consider using `force=False` or adding a handler directly to avoid side effects.
|
||||
format="%(message)s",
|
||||
|
CoreRasurae
commented
H4 (HIGH - Architecture): The session service is manually wired outside the DI container. Every other service ( **H4 (HIGH - Architecture):** The session service is manually wired outside the DI container. Every other service (`ProjectService`, `ActorService`, `PlanService`, etc.) is registered as a proper container provider. This should follow the same pattern with a `providers.Factory(PersistentSessionService, ...)` registration in `Container`. This would also eliminate the need for the private `_service` global and make testing cleaner via `provider.override()`.
|
||||
level=logging.WARNING,
|
||||
stream=sys.stderr,
|
||||
force=False,
|
||||
)
|
||||
|
CoreRasurae
commented
H1 (HIGH - Bug): **H1 (HIGH - Bug):** `_get_session_service()` declares `global _service` and checks it, but never assigns the newly built service to `_service`. This means the service is rebuilt on every call. Add `_service = PersistentSessionService(session_repo, message_repo)` before the return statement.
|
||||
|
||||
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:
|
||||
|
CoreRasurae
commented
M1 (MEDIUM - Bug): Catching bare **M1 (MEDIUM - Bug):** Catching bare `Exception` is overly broad. This could mask unexpected bugs (`TypeError`, `ImportError`, etc.) in the service layer. Consider catching specific expected exceptions such as `DatabaseError` and `OperationalError`.
|
||||
console.print(f"[red]Error:[/red] {exc}")
|
||||
|
CoreRasurae
commented
[H2 — HIGH: Suggested fix: **[H2 — HIGH: `color` format excluded from human-friendly path]** This condition doesn't include `OutputFormat.COLOR.value`. The `color` format should show `"No sessions found."` (same layout as `plain`, per spec), but instead it falls through to `format_output(empty_data, "color")` which produces structured output like `sessions:\ntotal: 0`.
Suggested fix:
```python
if fmt not in (OutputFormat.RICH.value, OutputFormat.PLAIN.value, OutputFormat.COLOR.value):
```
|
||||
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)
|
||||
|
||||
@@ -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
|
||||
|
CoreRasurae
commented
H3 (HIGH - Architecture): Changing from flush-only to flush-and-commit per method breaks the Unit of Work pattern. All 20+ other repositories in this file follow flush-only with caller-managed commits. If any future consumer uses **H3 (HIGH - Architecture):** Changing from flush-only to flush-and-commit per method breaks the Unit of Work pattern. All 20+ other repositories in this file follow flush-only with caller-managed commits. If any future consumer uses `SessionRepository` within a `UnitOfWork`, these auto-commits will prematurely commit partial transactions. Consider using the UoW properly from the CLI instead, or adding an `auto_commit` constructor flag.
|
||||
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)
|
||||
|
CoreRasurae
commented
[H1 — HIGH: Contract violation] The class docstring (line 3872-3873) states: "All mutating methods flush (but do NOT commit); the caller or a Adding **[H1 — HIGH: Contract violation]** The class docstring (line 3872-3873) states: *"All mutating methods flush (but do NOT commit); the caller or a `UnitOfWork` wrapper is responsible for committing the transaction."*
Adding `commit()` here breaks this contract. Either all mutating methods should commit (and the docstring should be updated), or the CLI should be changed to use UnitOfWork for transaction management.
|
||||
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()
|
||||
|
||||
Reference in New Issue
Block a user
[M2 — MEDIUM: Stale documentation] This comment still says "expected to fail until the DI container fix lands" but the
@tdd_expected_failtags have been removed because the fix is applied. Update to describe these as regression tests for the resolved bug.