diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e632e073..5e1879cb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,17 @@ - Polymorphic handler resolution with ancestor-type fallback - CLI: `agents resource type list` shows Inherits column; `type show` displays inheritance chain - Alembic migration `m6_004_resource_type_inherits` adds `inherits` column to `resource_types` +- Added TDD regression tests for `agents session list` DI container wiring + error (bug #554). `_get_session_service()` calls `container.db()` but the + `Container` class has no `db` provider, raising `AttributeError`. Includes + 10 Behave BDD scenarios (`@tdd_bug @tdd_bug_554 @tdd_expected_fail`) + covering empty list, empty-list format validation (JSON/YAML/plain), + init-then-list lifecycle, post-create list, rich/JSON/plain/YAML output + formats, and stderr error-path assertions. Robot Framework integration + smoke tests and ASV service-layer benchmarks. Implements + `@tdd_expected_fail` infrastructure (Behave `after_scenario` hook and Robot + listener) and migrates 18 existing TDD scenarios from `@tdd @bugNNN` to + `@tdd_bug @tdd_bug_NNN` convention. (#554) - Added TDD regression tests for `agents session create` DI container wiring error (bug #570). `_get_session_service()` calls `container.db()` but the `Container` class has no `db` provider, raising `AttributeError`. Same root diff --git a/benchmarks/session_list_bench.py b/benchmarks/session_list_bench.py new file mode 100644 index 000000000..d17abfd11 --- /dev/null +++ b/benchmarks/session_list_bench.py @@ -0,0 +1,157 @@ +"""ASV benchmarks for session list service-layer performance baseline (bug #554). + +Measures the cost of listing sessions through ``PersistentSessionService`` +using a file-based SQLite database. These benchmarks construct the service +directly (bypassing DI wiring) and therefore do **not** exercise the +``_get_session_service()`` code path that triggers bug #554. Their purpose +is to establish a performance baseline for the service layer itself. +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile +from pathlib import Path + +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from sqlalchemy import create_engine # noqa: E402 +from sqlalchemy.orm import sessionmaker # noqa: E402 + +from cleveragents.application.services.session_service import ( # noqa: E402 + PersistentSessionService, +) +from cleveragents.infrastructure.database.models import Base # noqa: E402 +from cleveragents.infrastructure.database.repositories import ( # noqa: E402 + SessionMessageRepository, + SessionRepository, +) + + +class SessionListDISuite: + """Benchmark session list through the service layer (direct construction). + + Engine and sessionmaker are built once in ``setup()`` and reused across + ``time_list_empty`` iterations. Methods that mutate state + (``time_list_after_create``, ``track_list_after_create_count``) use + per-method setup/teardown to get a fresh database each time, preventing + row accumulation across ASV iterations. + + Does **not** exercise ``_get_session_service()`` or the DI container + wiring. + """ + + timeout = 30.0 + + # -- suite-level setup (shared engine for read-only benchmarks) ---------- + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_sle_554_") + self._db_path = os.path.join(self._tmpdir, "bench.db") + self._engine = create_engine( + f"sqlite:///{self._db_path}", + echo=False, + ) + Base.metadata.create_all(self._engine) + self._session_factory = sessionmaker( + bind=self._engine, + expire_on_commit=False, + ) + + def teardown(self) -> None: + self._engine.dispose() + shutil.rmtree(self._tmpdir, ignore_errors=True) + + def _make_service(self) -> PersistentSessionService: + """Build a PersistentSessionService using the shared session factory.""" + return PersistentSessionService( + session_repo=SessionRepository( + session_factory=self._session_factory, + ), + message_repo=SessionMessageRepository( + session_factory=self._session_factory, + ), + ) + + # -- per-method setup for mutating benchmarks ---------------------------- + + def _fresh_engine(self) -> None: + """Create an isolated engine+factory so create() doesn't accumulate.""" + self._mut_tmpdir = tempfile.mkdtemp(prefix="bench_sle_554_mut_") + db_path = os.path.join(self._mut_tmpdir, "bench.db") + self._mut_engine = create_engine( + f"sqlite:///{db_path}", + echo=False, + ) + Base.metadata.create_all(self._mut_engine) + self._mut_factory = sessionmaker( + bind=self._mut_engine, + expire_on_commit=False, + ) + + def _dispose_fresh(self) -> None: + if hasattr(self, "_mut_engine"): + self._mut_engine.dispose() + if hasattr(self, "_mut_tmpdir"): + shutil.rmtree(self._mut_tmpdir, ignore_errors=True) + + def _make_mut_service(self) -> PersistentSessionService: + return PersistentSessionService( + session_repo=SessionRepository(session_factory=self._mut_factory), + message_repo=SessionMessageRepository( + session_factory=self._mut_factory, + ), + ) + + # -- ASV per-method hooks ------------------------------------------------ + + def setup_time_list_after_create(self) -> None: + self._fresh_engine() + + def teardown_time_list_after_create(self) -> None: + self._dispose_fresh() + + def setup_track_list_after_create_count(self) -> None: + self._fresh_engine() + + def teardown_track_list_after_create_count(self) -> None: + self._dispose_fresh() + + def setup_time_list_empty(self) -> None: + self._empty_svc = self._make_service() + + # -- benchmarks ---------------------------------------------------------- + + def time_list_empty(self) -> None: + """List sessions when DB is empty (service-layer only, no DI).""" + self._empty_svc.list() + + def time_list_after_create(self) -> None: + """Create then list (service-layer only, fresh DB per iteration). + + Bypasses ``_get_session_service()`` / DI container — measures the + service-layer round-trip cost, not the DI wiring path. + """ + svc = self._make_mut_service() + svc.create() + svc.list() + + def track_list_after_create_count(self) -> int: + """Track session list persistence at the service layer. + + Returns the count of sessions visible after a create. Uses a + fresh DB per iteration so the count is always exactly 1. + This benchmark constructs ``PersistentSessionService`` directly, + bypassing the DI container — it does **not** reproduce bug #554. + """ + svc = self._make_mut_service() + svc.create(actor_name="bench/test") + sessions = svc.list() + return len(sessions) + + +SessionListDISuite.track_list_after_create_count.unit = "sessions" diff --git a/features/session_list_error.feature b/features/session_list_error.feature new file mode 100644 index 000000000..3be95a257 --- /dev/null +++ b/features/session_list_error.feature @@ -0,0 +1,85 @@ +# 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. +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 + So that I can view my sessions without a DI container error + + Background: + Given a session-list-error CLI runner using the real DI path + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + 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 + 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 + 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 + 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" + 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 not contain "AttributeError" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + 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 + 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 (" + + @tdd_bug @tdd_bug_554 @tdd_expected_fail + 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" + Then the session-list-error command should exit successfully + And the session-list-error output should be valid YAML containing "sessions" + + # Empty-list format scenarios (F2/F3) — exercises the empty-list code path + # 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 + 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 + 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 + 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 + And the session-list-error output should contain "No sessions found" + And the session-list-error output should not contain "AttributeError" diff --git a/features/steps/session_list_error_steps.py b/features/steps/session_list_error_steps.py new file mode 100644 index 000000000..3001d99de --- /dev/null +++ b/features/steps/session_list_error_steps.py @@ -0,0 +1,217 @@ +"""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. + +Design rationale +~~~~~~~~~~~~~~~~ +``_get_session_service()`` calls ``container.db()`` but the DI ``Container`` +class has no ``db`` provider, raising ``AttributeError``. + +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. + +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. +""" + +from __future__ import annotations + +import json +import os +import shutil +import tempfile + +import yaml +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine +from sqlalchemy.orm import scoped_session, sessionmaker +from typer.testing import CliRunner + +from cleveragents.application.container import reset_container +from cleveragents.application.services.session_service import ( + PersistentSessionService, +) +from cleveragents.cli.commands import session as session_mod +from cleveragents.cli.commands.session import app as session_app +from cleveragents.infrastructure.database.models import Base +from cleveragents.infrastructure.database.repositories import ( + SessionMessageRepository, + SessionRepository, +) + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _setup_real_di_path(context: Context) -> None: + """Prepare a temp dir with a fresh SQLite DB and override container.""" + # Store original _service so cleanup can restore it. + context.sle_original_service = session_mod._service + + # Reset any stale DI container singleton before configuring the env var + # so that cached providers don't carry over from a prior test suite (F17). + reset_container() + + context.sle_tmpdir = tempfile.mkdtemp(prefix="session_list_err_554_") + + # Register cleanup immediately after mkdtemp so the temp directory is + # always removed even if the rest of setup fails (F21). + context.add_cleanup(_cleanup_sle, context) + + context.sle_db_path = os.path.join(context.sle_tmpdir, "test.db") + db_url = f"sqlite:///{context.sle_db_path}" + + # Create schema so the DB file exists with all tables. + engine = create_engine(db_url, echo=False) + 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 + + # Reset the module-level _service so _get_session_service() is used. + # This direct attribute mutation is fragile — if the module's internal + # caching mechanism changes (e.g. lazy singleton via descriptor), this + # line will need to be updated. See module docstring for rationale. + session_mod._service = None + + context.sle_result = None + + +def _cleanup_sle(context: Context) -> None: + """Remove temp dir, restore env, original _service, and container.""" + session_mod._service = context.sle_original_service + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + + # Reset the DI container singleton to avoid polluting later scenarios. + reset_container() + + shutil.rmtree(context.sle_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a session-list-error CLI runner using the real DI path") +def step_session_list_error_runner(context: Context) -> None: + _setup_real_di_path(context) + + +# --------------------------------------------------------------------------- +# Given - pre-populated session +# --------------------------------------------------------------------------- + + +@given("a session-list-error service with a pre-populated session") +def step_pre_populate_session(context: Context) -> None: + """Insert a session directly via the repository so list has data.""" + db_url = f"sqlite:///{context.sle_db_path}" + engine = create_engine(db_url, echo=False) + try: + factory = scoped_session( + sessionmaker(bind=engine, expire_on_commit=False), + ) + repo = SessionRepository(session_factory=factory) + msg_repo = SessionMessageRepository(session_factory=factory) + svc = PersistentSessionService(repo, msg_repo) + svc.create(actor_name="openai/gpt-4") + # Commit via the scoped session so the data is visible to later queries. + factory().commit() + factory.remove() + finally: + engine.dispose() + + +# --------------------------------------------------------------------------- +# When - list +# --------------------------------------------------------------------------- + + +@when("I invoke session-list-error list with default format") +def step_invoke_list_default(context: Context) -> None: + context.sle_result = runner.invoke(session_app, ["list"]) + + +@when('I invoke session-list-error list with format "{fmt}"') +def step_invoke_list_format(context: Context, fmt: str) -> None: + context.sle_result = runner.invoke(session_app, ["list", "--format", fmt]) + + +# --------------------------------------------------------------------------- +# Then - assertions +# --------------------------------------------------------------------------- + + +@then("the session-list-error command should exit successfully") +def step_exit_success(context: Context) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert result.exit_code == 0, ( + f"Expected exit code 0, got {result.exit_code}.\n" + f"Output: {result.output}\n" + f"Exception: {result.exception!r}" + ) + + +@then('the session-list-error output should contain "{text}"') +def step_output_contains(context: Context, text: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert text in result.output, ( + f"Expected '{text}' in output but got:\n{result.output}" + ) + + +@then('the session-list-error output should not contain "{text}"') +def step_output_not_contains(context: Context, text: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + assert text not in result.output, ( + f"Did not expect '{text}' in output but found it:\n{result.output}" + ) + + +@then('the session-list-error output should be valid JSON containing "{key}"') +def step_output_json_key(context: Context, key: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + try: + data = json.loads(result.output) + except json.JSONDecodeError as exc: + raise AssertionError(f"Output is not valid JSON:\n{result.output}") from exc + assert isinstance(data, dict), f"Expected JSON object, got {type(data)}: {data}" + assert key in data, f"Key '{key}' not in JSON: {data}" + assert isinstance(data[key], list), ( + f"Expected '{key}' to be a list, got {type(data[key])}: {data[key]}" + ) + + +@then('the session-list-error output should be valid YAML containing "{key}"') +def step_output_yaml_key(context: Context, key: str) -> None: + result = context.sle_result + assert result is not None, "No command was invoked" + try: + data = yaml.safe_load(result.output) + except yaml.YAMLError as exc: + raise AssertionError(f"Output is not valid YAML:\n{result.output}") from exc + assert isinstance(data, dict), f"Expected YAML dict, got {type(data)}: {data}" + assert key in data, f"Key '{key}' not in YAML: {data}" + assert isinstance(data[key], list), ( + f"Expected '{key}' to be a list, got {type(data[key])}: {data[key]}" + ) diff --git a/robot/session_list_error.robot b/robot/session_list_error.robot new file mode 100644 index 000000000..e7d0ecc62 --- /dev/null +++ b/robot/session_list_error.robot @@ -0,0 +1,58 @@ +*** 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``. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Comments *** +# NOTE: ``Should Contain`` and ``Should Not Contain`` are case-sensitive +# by default in Robot Framework. The assertions below rely on the exact +# output format produced by the CLI: +# - "AttributeError" (Python exception class name, title-case) +# If the production code changes its error message casing, update these +# assertions accordingly. + +*** Test Cases *** +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 + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_') + ${init}= Run Process ${PYTHON} -m cleveragents init sle-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=session list should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Not Contain ${list.stderr} AttributeError + ... msg=session list should not raise AttributeError in stderr: ${list.stderr} + Should Not Contain ${list.stdout} AttributeError + ... msg=session list should not raise AttributeError in stdout: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True + +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 + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sle_554_json_') + ${init}= Run Process ${PYTHON} -m cleveragents init sle-json + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init should exit 0 but got ${init.rc}. stderr: ${init.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list --format json + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=session list --format json should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Not Contain ${list.stderr} AttributeError + ... msg=session list --format json should not raise AttributeError in stderr: ${list.stderr} + Should Not Contain ${list.stdout} AttributeError + ... msg=session list --format json should not raise AttributeError in stdout: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True