From d0689573e01a192f2a80b4a445ac69d70db44f1f Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 9 Mar 2026 23:28:07 +0000 Subject: [PATCH 1/2] test(cli): add failing tests for session create DI container error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TDD regression tests for bug #570 where `_get_session_service()` calls `container.db()` but the DI `Container` class has no `db` provider, raising `AttributeError`. Same root cause as bug #554. Includes 4 Behave BDD scenarios tagged `@tdd_bug @tdd_bug_570 @tdd_expected_fail`, Robot Framework integration smoke tests with `--format plain`, and ASV service-layer benchmarks. Tests exercise the real DI path by resetting `_service = None` and using a file-based SQLite database. Implements the `@tdd_expected_fail` inversion infrastructure: - Behave: `after_scenario` hook in `features/environment.py` inverts pass/fail for scenarios tagged `@tdd_expected_fail` - Robot: `robot/tdd_expected_fail_listener.py` listener (API v3) performs the same inversion for Robot test cases - `noxfile.py`: registers the listener via `--listener` in both the `integration_tests` and `slow_integration_tests` sessions Migrates 18 existing TDD scenarios across 5 feature files from the old `@tdd @bugNNN` convention to the standardised `@tdd_bug @tdd_bug_NNN` tags per CONTRIBUTING.md § TDD Bug Test Tags. Refs: #570 --- CHANGELOG.md | 15 ++ benchmarks/session_create_error_bench.py | 100 ++++++++++ features/cli_init_yes_flag.feature | 10 +- features/environment.py | 31 ++++ features/project_create_persist.feature | 8 +- features/project_show_after_create.feature | 6 +- features/resource_type_bootstrap_fs.feature | 6 +- features/resource_type_bootstrap_git.feature | 6 +- features/session_create_error.feature | 38 ++++ features/steps/session_create_error_steps.py | 186 +++++++++++++++++++ noxfile.py | 30 ++- robot/session_create_error.robot | 54 ++++++ robot/tdd_expected_fail_listener.py | 48 +++++ 13 files changed, 515 insertions(+), 23 deletions(-) create mode 100644 benchmarks/session_create_error_bench.py create mode 100644 features/session_create_error.feature create mode 100644 features/steps/session_create_error_steps.py create mode 100644 robot/session_create_error.robot create mode 100644 robot/tdd_expected_fail_listener.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d32928830..8d602bc16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ 3 Robot Framework integration tests, ASV benchmarks (5 time + 2 track), and reference documentation. (#195) + ### Added - Resource type single-inheritance via `inherits` field (ADR-042) (#513) - Inheritance chain resolution, field merging, and polymorphic type matching @@ -20,6 +21,20 @@ - 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 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 + cause as #554. Includes 4 Behave BDD scenarios + (`@tdd_bug @tdd_bug_570 @tdd_expected_fail`), Robot Framework integration + smoke tests, and ASV service-layer benchmarks. Tests exercise the real DI + path with `_service = None` and a file-based SQLite database. + Also implements the `@tdd_expected_fail` inversion infrastructure: + a Behave `after_scenario` hook in `features/environment.py` that flips + pass/fail for `@tdd_expected_fail` scenarios, and a Robot Framework + Listener API v3 plugin (`robot/tdd_expected_fail_listener.py`) with + identical semantics. Migrates 18 existing TDD scenarios from the old + `@tdd @bugNNN` convention to standardised `@tdd_bug @tdd_bug_NNN` tags. + (#570) - Fixed `agents project show` not finding a project immediately after creation. Extended the `session.commit()` fix from #589 to also cover `update()` and `delete()` in `NamespacedProjectRepository`, and updated the class docstring diff --git a/benchmarks/session_create_error_bench.py b/benchmarks/session_create_error_bench.py new file mode 100644 index 000000000..c84f1bf2c --- /dev/null +++ b/benchmarks/session_create_error_bench.py @@ -0,0 +1,100 @@ +"""ASV benchmarks for session create service-layer performance (bug #570). + +Measures the cost of creating a session through ``PersistentSessionService`` +using a file-based SQLite database so that each operation exercises the full +service layer (repository -> SQLAlchemy -> SQLite round-trip). + +Note: these benchmarks construct ``PersistentSessionService`` directly and do +**not** exercise the DI container wiring path (``_get_session_service`` / +``container.db()``). Their purpose is to establish a service-layer create +performance baseline so regressions can be detected after the bug-fix lands. +Same root cause as bug #554. +""" + +from __future__ import annotations + +import importlib +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) + +import cleveragents # noqa: E402 + +importlib.reload(cleveragents) + +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 SessionCreateDISuite: + """Benchmark session create through the service layer. + + Engine and sessionmaker are built once in ``setup()`` and reused across + benchmark iterations to measure service-layer cost without engine + construction overhead. + """ + + timeout = 60 + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_sce_570_") + 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 + ), + ) + + def time_create_session(self) -> None: + """Create a session via the service layer.""" + svc = self._make_service() + svc.create() + + def time_create_with_actor(self) -> None: + """Create a session with a custom actor.""" + svc = self._make_service() + svc.create(actor_name="openai/gpt-4") + + def track_create_persists(self) -> int: + """Track session create persistence at the service layer. + + Returns the count of sessions created via the service. This + benchmark constructs ``PersistentSessionService`` directly, + bypassing the DI container (``_get_session_service`` / + ``container.db()``). It therefore does **not** reproduce bug + #570 — its purpose is to establish a service-layer persistence + baseline so regressions can be detected after the fix lands. + """ + svc = self._make_service() + svc.create(actor_name="bench/create-test") + sessions = svc.list() + return len(sessions) + + +SessionCreateDISuite.track_create_persists.unit = "sessions" diff --git a/features/cli_init_yes_flag.feature b/features/cli_init_yes_flag.feature index 128e206de..f2a3609f8 100644 --- a/features/cli_init_yes_flag.feature +++ b/features/cli_init_yes_flag.feature @@ -13,14 +13,14 @@ Feature: CLI init --yes flag for non-interactive initialization I want to run "agents init --yes" for non-interactive initialization So that I can skip interactive prompts and use sensible defaults - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: agents init --yes completes without error Given I have a temporary project directory for init When I run agents init with the --yes flag Then the init command should exit with code 0 And the project service initialize_project should have been called - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: --yes suppresses interactive prompts Given I have a temporary project directory for init When I run agents init with the --yes flag @@ -28,7 +28,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "Initialized (non-interactive)" And no interactive prompt should have been presented - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: -y short-form alias completes without error Given I have a temporary project directory for init When I run agents init with the -y flag @@ -36,7 +36,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "Initialized (non-interactive)" And the project service initialize_project should have been called - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: Output includes expected initialization summary Given I have a temporary project directory for init When I run agents init with the --yes flag @@ -48,7 +48,7 @@ Feature: CLI init --yes flag for non-interactive initialization And the init output should contain "logs, cache, sessions, contexts" And the init output should contain "Initialized" - @tdd @bug522 + @tdd_bug @tdd_bug_522 Scenario: Interactive mode without --yes presents a prompt Given I have a temporary project directory for init When I run agents init without the --yes flag diff --git a/features/environment.py b/features/environment.py index e71e026f8..97fc13fb4 100644 --- a/features/environment.py +++ b/features/environment.py @@ -307,6 +307,37 @@ def before_scenario(context, scenario): def after_scenario(context, scenario): """Clean up after each scenario.""" + # ── @tdd_expected_fail inversion ────────────────────────────────── + # When a scenario is tagged @tdd_expected_fail the test captures a + # bug that has NOT yet been fixed. The assertions describe the + # *correct* (post-fix) behaviour, so the scenario is expected to + # FAIL while the bug exists. We invert the result so CI stays + # green: + # • scenario FAILED → mark PASSED (expected — bug still exists) + # • scenario PASSED → mark FAILED (unexpected — fix landed but + # the @tdd_expected_fail tag was not removed) + # See CONTRIBUTING.md § TDD Bug Test Tags for the full convention. + if "tdd_expected_fail" in scenario.tags: + from behave.model import Status + + if scenario.status == Status.failed: + # Expected failure — reset all steps and the scenario so + # Behave counts this as a pass. + for step in scenario.steps: + step.status = Status.passed + step.error_message = None + scenario.clear_status() + scenario.set_status(Status.passed) + elif scenario.status == Status.passed: + # Unexpected pass — the bug appears fixed but the tag was + # not removed. Force a failure so the developer notices. + scenario.set_status(Status.failed) + scenario.error_message = ( + "[tdd_expected_fail] Test passed but still has the " + "tdd_expected_fail tag. The bug appears to be fixed " + "— remove the tag." + ) + # Return to original directory first if hasattr(context, "original_cwd"): os.chdir(context.original_cwd) diff --git a/features/project_create_persist.feature b/features/project_create_persist.feature index 23b168b4d..09902d647 100644 --- a/features/project_create_persist.feature +++ b/features/project_create_persist.feature @@ -7,13 +7,13 @@ Feature: Project create persists to database Background: Given a fresh project-persist database is initialised - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Created project appears in project list When I create a project named "local/my-app" via the persist CLI And I list projects via the persist CLI Then the persist project list should contain "local/my-app" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Multiple created projects all appear in list When I create a project named "local/alpha" via the persist CLI And I create a project named "local/beta" via the persist CLI @@ -21,13 +21,13 @@ Feature: Project create persists to database Then the persist project list should contain "local/alpha" And the persist project list should contain "local/beta" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Bare project name uses default namespace and persists When I create a project named "my-app" via the persist CLI And I list projects via the persist CLI Then the persist project list should contain "local/my-app" - @tdd @bug589 + @tdd_bug @tdd_bug_589 Scenario: Creating a duplicate project produces an error When I create a project named "local/dup-proj" via the persist CLI And I attempt to create a duplicate project named "local/dup-proj" via the persist CLI diff --git a/features/project_show_after_create.feature b/features/project_show_after_create.feature index fa8d30de4..01ebbb4e4 100644 --- a/features/project_show_after_create.feature +++ b/features/project_show_after_create.feature @@ -7,14 +7,14 @@ Feature: Project show displays a created project Background: Given a fresh project-show database is initialised - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show displays a project that was just created When I create a project named "local/my-app" via the project-show CLI And I show the project "local/my-app" via the project-show CLI Then the project-show output should contain "local/my-app" And the project-show exit code should be 0 - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show displays correct details for a created project with description When I create a described project named "local/webapp" with description "My web app" via the project-show CLI And I show the project "local/webapp" via the project-show CLI @@ -22,7 +22,7 @@ Feature: Project show displays a created project And the project-show output should contain "My web app" And the project-show exit code should be 0 - @tdd @bug590 + @tdd_bug @tdd_bug_590 Scenario: Show returns error for a project that does not exist When I show the project "local/nonexistent" via the project-show CLI Then the project-show output should contain "not found" diff --git a/features/resource_type_bootstrap_fs.feature b/features/resource_type_bootstrap_fs.feature index e786b3e91..9ae2fc742 100644 --- a/features/resource_type_bootstrap_fs.feature +++ b/features/resource_type_bootstrap_fs.feature @@ -17,7 +17,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ResourceRegistryService.__init__(), you will need to update the Given # step to exercise the init path instead of constructing a bare service. - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: fs-directory type exists after init without explicit bootstrap call Given a fresh in-memory resource registry without bootstrap When I query the fs bootstrap resource type registry for "fs-directory" @@ -25,7 +25,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ── Regression: bootstrap function itself works correctly ── - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: After initialization fs-directory type exists in the registry Given a fresh in-memory resource registry with bootstrap When I query the fs bootstrap resource type registry for "fs-directory" @@ -35,7 +35,7 @@ Feature: Built-in fs-directory Resource Type Bootstrap # ── CLI add command ──────────────────────────────────────── - @tdd @bug523 + @tdd_bug @tdd_bug_523 Scenario: resource add fs-directory succeeds after bootstrap Given a fresh in-memory resource registry with bootstrap When I run resource add for type "fs-directory" named "local/test" with path "/tmp/test" diff --git a/features/resource_type_bootstrap_git.feature b/features/resource_type_bootstrap_git.feature index 34e7e62b8..d8718d8f3 100644 --- a/features/resource_type_bootstrap_git.feature +++ b/features/resource_type_bootstrap_git.feature @@ -16,7 +16,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ResourceRegistryService.__init__(), you will need to update the Given # step to exercise the init path instead of constructing a bare service. - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: git-checkout type is missing when bootstrap is not called during init Given a bootstrap-git fresh in-memory resource registry without bootstrap When I query the bootstrap-git resource type registry for "git-checkout" @@ -24,7 +24,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ── Regression: bootstrap function itself works correctly ── - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: After initialization the git-checkout type exists in the resource type registry Given a bootstrap-git fresh in-memory resource registry with bootstrap When I query the bootstrap-git resource type registry for "git-checkout" @@ -36,7 +36,7 @@ Feature: Built-in git-checkout type bootstrap on initialization # ── CLI resource add succeeds ────────────────────────────── - @tdd @bug524 + @tdd_bug @tdd_bug_524 Scenario: agents resource add git-checkout succeeds without Resource type not found error Given a bootstrap-git fresh in-memory resource registry with bootstrap When I run bootstrap-git resource add for type "git-checkout" named "local/test" with path "/tmp/repo" and branch "main" diff --git a/features/session_create_error.feature b/features/session_create_error.feature new file mode 100644 index 000000000..667a33e00 --- /dev/null +++ b/features/session_create_error.feature @@ -0,0 +1,38 @@ +# 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. +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 + So that I can create interactive sessions without a DI container error + + Background: + Given a session-create-error CLI runner using the real DI path + + @tdd_bug @tdd_bug_570 @tdd_expected_fail + 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 + 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 + 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 + 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 + And the session-create-error output should contain "nonexistent/bogus-actor-999" diff --git a/features/steps/session_create_error_steps.py b/features/steps/session_create_error_steps.py new file mode 100644 index 000000000..7913b6bde --- /dev/null +++ b/features/steps/session_create_error_steps.py @@ -0,0 +1,186 @@ +"""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. + +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. + +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. + +All CLI invocations use ``--format plain`` so output goes through +``typer.echo`` (captured by ``CliRunner``) rather than ``rich.console``. + +Private API access (``session_mod._service``, ``session_mod._reset_session_service``) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +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). +If the module's internal caching mechanism changes, these tests will +need to be updated accordingly. +""" + +from __future__ import annotations + +import os +import shutil +import tempfile +from typing import Any + +from behave import given, then, when +from sqlalchemy import create_engine +from typer.testing import CliRunner + +from cleveragents.application.container import reset_container +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 + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _setup_real_di_path(context: Any) -> None: + """Prepare a temp dir with a fresh SQLite DB and override container. + + Registers cleanup immediately after capturing original state so that + env vars and module state are restored even if later setup lines raise. + """ + # Store original _service so cleanup can restore it. + context.sce_original_service = session_mod._service + context.sce_tmpdir = tempfile.mkdtemp(prefix="session_create_err_570_") + + # Register cleanup early so env/state is always restored (SEC-3). + context.add_cleanup(_cleanup_sce, context) + + context.sce_db_path = os.path.join(context.sce_tmpdir, "test.db") + db_url = f"sqlite:///{context.sce_db_path}" + + # Create schema so the DB file exists with all tables. + engine = create_engine(db_url, echo=False) + Base.metadata.create_all(engine) + engine.dispose() + + # Override the container's database_url so real DI can find the DB. + os.environ["CLEVERAGENTS_DATABASE_URL"] = db_url + + # Reset global DI container so post-fix DI reads the fresh env var. + reset_container() + + # 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.sce_result = None + context.sce_list_result = None + + +def _cleanup_sce(context: Any) -> None: + """Remove temp dir, restore env and original _service. + + Also resets the module-level ``_service`` cache via the public API so + that any DI-created engine is released before the temp dir is removed. + """ + # Ensure module-level cache is cleared; GC will release any engine when + # the container is reset (done separately via reset_container()). + session_mod._reset_session_service() + # Then restore the original _service value. + session_mod._service = context.sce_original_service + # Release any DI-created connections before removing temp dir. + reset_container() + os.environ.pop("CLEVERAGENTS_DATABASE_URL", None) + shutil.rmtree(context.sce_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a session-create-error CLI runner using the real DI path") +def step_session_create_error_runner(context: Any) -> None: + _setup_real_di_path(context) + + +# --------------------------------------------------------------------------- +# When - create +# --------------------------------------------------------------------------- + + +@when("I invoke session-create-error create with no arguments") +def step_invoke_create_no_args(context: Any) -> None: + context.sce_result = runner.invoke(session_app, ["create", "--format", "plain"]) + + +@when('I invoke session-create-error create with actor "{actor}"') +def step_invoke_create_with_actor(context: Any, actor: str) -> None: + context.sce_result = runner.invoke( + session_app, ["create", "--actor", actor, "--format", "plain"] + ) + + +# --------------------------------------------------------------------------- +# When - list (for persistence verification) +# --------------------------------------------------------------------------- + + +@when("I invoke session-create-error list to verify persistence") +def step_invoke_list_after_create(context: Any) -> None: + context.sce_list_result = runner.invoke(session_app, ["list", "--format", "plain"]) + + +# --------------------------------------------------------------------------- +# Then - assertions +# --------------------------------------------------------------------------- + + +@then("the session-create-error command should exit successfully") +def step_exit_success(context: Any) -> None: + result = context.sce_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-create-error output should contain "{text}"') +def step_output_contains(context: Any, text: str) -> None: + result = context.sce_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-create-error list should show at least one session") +def step_list_shows_sessions(context: Any) -> None: + result = context.sce_list_result + assert result is not None, "List was not invoked" + assert result.exit_code == 0, ( + f"Expected list exit code 0, got {result.exit_code}.\n" + f"Output: {result.output}\n" + f"Exception: {result.exception!r}" + ) + # In plain format the output should contain "total:" with a count > 0. + assert "total:" in result.output, ( + f"Expected 'total:' in plain list output but got:\n{result.output}" + ) + assert "total: 0" not in result.output, ( + f"Expected at least one session but got:\n{result.output}" + ) diff --git a/noxfile.py b/noxfile.py index 8489678f6..89bf9b713 100644 --- a/noxfile.py +++ b/noxfile.py @@ -371,7 +371,7 @@ def main(argv=None): if processes <= 1 or coverage_mode or len(feature_paths) == 1: # ---- sequential in-process mode ---- - failed, total = _run_features_inprocess(feature_paths, other_args) + _, total = _run_features_inprocess(feature_paths, other_args) else: # ---- parallel in-process mode (multiprocessing fork) ---- # Pre-import heavy modules so forked children get them for free. @@ -398,21 +398,37 @@ def main(argv=None): [(chunk, other_args) for chunk in chunks], ) - failed = False summaries = [] - for worker_failed, stdout, stderr, summary in results: + for _worker_failed, stdout, stderr, summary in results: if stdout: print(stdout, end="") if stderr: print(stderr, end="", file=sys.stderr) - failed = failed or worker_failed summaries.append(summary) total = _merge_summaries(summaries) wall = time.monotonic() - start _print_overall_summary(total, wall_seconds=wall) - if failed or _has_failures(total): + # Use the summary-based check rather than the raw runner ``failed`` + # boolean. The ``@tdd_expected_fail`` handler in environment.py + # inverts scenario statuses for TDD bug-capture tests, but behave's + # ``runner.run()`` tracks step failures in a local variable that + # cannot be updated by after_scenario hooks. Relying solely on the + # summary (which reflects the corrected scenario statuses) ensures + # that TDD-inverted scenarios do not cause a spurious exit-code 1. + if _has_failures(total): + sys.exit(1) + + # Safety net: if features were requested but zero scenarios ran, the + # runner crashed before executing any scenario (e.g. ``before_all`` + # failure). Treat this as a failure so CI does not silently pass. + if feature_paths and total["scenarios"]["passed"] == 0 and total["scenarios"]["failed"] == 0: + print( + "ERROR: features were requested but no scenarios ran -- " + "possible runner-level crash.", + file=sys.stderr, + ) sys.exit(1) @@ -576,6 +592,8 @@ def integration_tests(session: nox.Session): "code_blocks", "--exclude", "wip", + "--listener", + "robot/tdd_expected_fail_listener.py", *robot_args, "robot/", ) @@ -598,6 +616,8 @@ def slow_integration_tests(session: nox.Session): "log.html", "--xunit", "xunit.xml", + "--listener", + "robot/tdd_expected_fail_listener.py", "robot/", *session.posargs, ) diff --git a/robot/session_create_error.robot b/robot/session_create_error.robot new file mode 100644 index 000000000..a57e239cb --- /dev/null +++ b/robot/session_create_error.robot @@ -0,0 +1,54 @@ +*** 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. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Test Cases *** +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 + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_') + ${init}= Run Process ${PYTHON} -m cleveragents init sce-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} + ${create}= Run Process ${PYTHON} -m cleveragents session create --format plain + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=session create should exit 0 but got ${create.rc}. stderr: ${create.stderr} + Should Not Contain ${create.stderr} AttributeError + ... msg=session create should not raise AttributeError: ${create.stderr} + [Teardown] Remove Directory ${tmpdir} recursive=True + +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 + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='sce_570_list_') + ${init}= Run Process ${PYTHON} -m cleveragents init sce-list + ... 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} + ${create}= Run Process ${PYTHON} -m cleveragents session create --format plain + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=session create should exit 0 but got ${create.rc}. stderr: ${create.stderr} + Should Not Contain ${create.stderr} AttributeError + ... msg=session create should not raise AttributeError: ${create.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents session list --format plain + ... 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 Contain ${list.stdout} total: + ... msg=Expected 'total:' in plain output: ${list.stdout} + Should Not Contain ${list.stdout} total: 0 + ... msg=Expected at least one session in list output: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True diff --git a/robot/tdd_expected_fail_listener.py b/robot/tdd_expected_fail_listener.py new file mode 100644 index 000000000..d89f9ced7 --- /dev/null +++ b/robot/tdd_expected_fail_listener.py @@ -0,0 +1,48 @@ +"""Robot Framework listener that inverts results for ``tdd_expected_fail`` tests. + +When a test is tagged ``tdd_expected_fail``, the listener treats a FAIL as a +PASS (the bug still exists, which is expected) and a PASS as a FAIL (the bug +was fixed but the tag was not removed). + +See CONTRIBUTING.md § TDD Bug Test Tags for the full convention. + +Usage:: + + pabot ... --listener robot/tdd_expected_fail_listener.py ... + +This listener uses the Robot Framework Listener API v3. +""" + +from __future__ import annotations + +from typing import Any + +ROBOT_LISTENER_API_VERSION = 3 + +_TAG = "tdd_expected_fail" + + +def end_test(data: Any, result: Any) -> None: + """Invert the result of tests tagged ``tdd_expected_fail``.""" + tags = getattr(result, "tags", []) + if _TAG not in tags: + return + + status: str = getattr(result, "status", "") + original_message: str = getattr(result, "message", "") + + if status == "FAIL": + # Expected failure — bug still exists. Mark as PASS. + result.status = "PASS" + result.message = ( + f"[tdd_expected_fail] Expected failure (bug still present). " + f"Original: {original_message}" + ) + elif status == "PASS": + # Unexpected pass — bug appears fixed, tag should be removed. + result.status = "FAIL" + result.message = ( + "[tdd_expected_fail] Test passed but still has the " + "tdd_expected_fail tag. The bug appears to be fixed — " + "remove the tag." + ) -- 2.52.0 From 4fff7f67c5b28bc8e6dfd5357b5296164bbd8f0f Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Tue, 10 Mar 2026 22:27:37 +0000 Subject: [PATCH 2/2] fix(test): use scoped_session to prevent GC-induced data loss in r2cov MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQLite in-memory engines use SingletonThreadPool, giving every Session the same underlying connection. Repository methods create a new Session via the factory, flush, then return — letting the Session go out of scope. Under high memory pressure (e.g. 32 parallel behave workers) Python's garbage collector closes these orphaned Sessions, issuing an implicit ROLLBACK on the shared connection and wiping flushed-but-uncommitted rows written by other Sessions. Replace the plain sessionmaker with scoped_session in the r2cov Background step so that every factory() call returns the same Session instance. A single long-lived Session per scenario eliminates the premature close/rollback window entirely. Verified: 3 consecutive green runs with --processes 32 (10 099 scenarios, 0 failures each). Refs: #570 --- .../steps/repositories_coverage_r2_steps.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/features/steps/repositories_coverage_r2_steps.py b/features/steps/repositories_coverage_r2_steps.py index 149f8f014..296bd933f 100644 --- a/features/steps/repositories_coverage_r2_steps.py +++ b/features/steps/repositories_coverage_r2_steps.py @@ -18,7 +18,7 @@ from behave import given, then, when from behave.runner import Context from sqlalchemy import create_engine from sqlalchemy.exc import OperationalError -from sqlalchemy.orm import Session, sessionmaker +from sqlalchemy.orm import Session, scoped_session, sessionmaker from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core.checkpoint import Checkpoint, CheckpointMetadata @@ -234,7 +234,20 @@ def step_fresh_db(context: Context) -> None: engine = create_engine("sqlite:///:memory:", echo=False) Base.metadata.create_all(engine) context.r2_engine = engine - context.r2_session_factory = sessionmaker(bind=engine) + # Use scoped_session so that every ``factory()`` call within the + # same thread returns the *same* Session instance. With plain + # ``sessionmaker``, each ``factory()`` call creates a new Session. + # SQLite in-memory uses ``SingletonThreadPool`` (one connection per + # thread), so all sessions share the same connection. When a + # session created inside a repository method goes out of scope, + # Python's garbage collector may close it, issuing an implicit + # ROLLBACK on the shared connection — wiping flushed-but-uncommitted + # rows written by *other* sessions. Under high memory pressure + # (e.g. 32 parallel worker processes) GC fires often enough to + # cause intermittent data loss between ``flush()`` and ``commit()``. + # ``scoped_session`` avoids the problem entirely: one Session lives + # for the whole scenario, so there is no premature close/rollback. + context.r2_session_factory = scoped_session(sessionmaker(bind=engine)) # Pre-create repos used by multiple scenarios context.r2_skill_repo = SkillRepository( -- 2.52.0