diff --git a/CHANGELOG.md b/CHANGELOG.md index d19696ced..af4c890a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +- 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 + to reflect that all mutating methods now commit within their own session. + Includes 3 Behave BDD regression scenarios, Robot Framework integration + smoke tests, and ASV benchmarks. (#590) - Fixed `agents project create` not persisting projects to the database. `NamespacedProjectRepository.create()` called `session.flush()` but never `session.commit()`, so projects were invisible to subsequent diff --git a/benchmarks/project_show_after_create_bench.py b/benchmarks/project_show_after_create_bench.py new file mode 100644 index 000000000..7bce7e207 --- /dev/null +++ b/benchmarks/project_show_after_create_bench.py @@ -0,0 +1,115 @@ +"""ASV benchmarks for project create + show round-trip. + +Measures the cost of creating a project and then showing it via the +``NamespacedProjectRepository``, using a file-based SQLite database so +that each operation gets a fresh session (matching real CLI behaviour). + +Targets bug #590 — ``create()`` now calls ``session.commit()`` so data +is visible to a subsequent ``get()`` in a separate session. +""" + +from __future__ import annotations + +import contextlib +import importlib +import os +import shutil +import sys +import tempfile +from pathlib import Path + +# Ensure the local *source* tree is importable even when ASV has an +# older build of the package installed. +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +# Force-reload so ASV picks up the source tree version. +import cleveragents # noqa: E402 + +importlib.reload(cleveragents) + +from sqlalchemy import create_engine # noqa: E402 +from sqlalchemy.orm import sessionmaker # noqa: E402 +from sqlalchemy.pool import NullPool # noqa: E402 + +from cleveragents.domain.models.core.project import ( # noqa: E402 + NamespacedProject, + parse_namespaced_name, +) +from cleveragents.infrastructure.database.models import Base # noqa: E402 +from cleveragents.infrastructure.database.repositories import ( # noqa: E402 + NamespacedProjectRepository, + ProjectNotFoundError, +) + + +def _fresh_repo(db_path: str) -> NamespacedProjectRepository: + """Return a repo backed by a *new* engine/session for *db_path*. + + ``NullPool`` ensures SQLite file handles are released immediately + when the engine is no longer referenced. + """ + engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool) + Base.metadata.create_all(engine) + factory = sessionmaker(bind=engine, expire_on_commit=False) + return NamespacedProjectRepository(session_factory=factory) + + +class ProjectShowAfterCreateSuite: + """Benchmark create-then-show round trip with separate sessions.""" + + timeout = 30.0 + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_show_590_") + self._db_path = os.path.join(self._tmpdir, "bench.db") + engine = create_engine( + f"sqlite:///{self._db_path}", + echo=False, + poolclass=NullPool, + ) + Base.metadata.create_all(engine) + engine.dispose() + self._counter = 0 + + def teardown(self) -> None: + shutil.rmtree(self._tmpdir, ignore_errors=True) + + def time_create_then_show(self) -> None: + """Create a project in one session, show (get) in another.""" + self._counter += 1 + name = f"bench-show-{self._counter}" + parsed = parse_namespaced_name(name) + proj = NamespacedProject(name=parsed.name, namespace=parsed.namespace) + + create_repo = _fresh_repo(self._db_path) + create_repo.create(proj) + + show_repo = _fresh_repo(self._db_path) + with contextlib.suppress(ProjectNotFoundError): + show_repo.get(f"local/{name}") + + def track_show_after_create_found(self) -> int: + """Track whether show finds the created project. + + Returns 1 when get() successfully finds the project; 0 if + ProjectNotFoundError is raised. + """ + self._counter += 1 + name = f"bench-show-track-{self._counter}" + parsed = parse_namespaced_name(name) + proj = NamespacedProject(name=parsed.name, namespace=parsed.namespace) + + create_repo = _fresh_repo(self._db_path) + create_repo.create(proj) + + show_repo = _fresh_repo(self._db_path) + try: + show_repo.get(f"local/{name}") + return 1 + except ProjectNotFoundError: + return 0 + + +ProjectShowAfterCreateSuite.track_show_after_create_found.unit = "found" diff --git a/features/project_show_after_create.feature b/features/project_show_after_create.feature new file mode 100644 index 000000000..fa8d30de4 --- /dev/null +++ b/features/project_show_after_create.feature @@ -0,0 +1,29 @@ +# Regression tests for bug #590: project show must find a project immediately after creation. +Feature: Project show displays a created project + As a developer using the agents CLI + I want "agents project show" to display a project I just created + So that I can verify the project details after creation + + Background: + Given a fresh project-show database is initialised + + @tdd @bug590 + 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 + 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 + Then the project-show output should contain "local/webapp" + And the project-show output should contain "My web app" + And the project-show exit code should be 0 + + @tdd @bug590 + 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" + And the project-show exit code should not be 0 diff --git a/features/steps/project_show_after_create_steps.py b/features/steps/project_show_after_create_steps.py new file mode 100644 index 000000000..32fde680b --- /dev/null +++ b/features/steps/project_show_after_create_steps.py @@ -0,0 +1,180 @@ +"""Step definitions for project_show_after_create.feature (bug #590). + +Regression tests verifying that ``agents project show`` finds a project +immediately after ``agents project create`` commits it. + +Root cause (fixed) +~~~~~~~~~~~~~~~~~~ +Identical to bug #589. ``NamespacedProjectRepository.create()`` called +``session.flush()`` but never ``session.commit()``. The ``show`` command +obtains a fresh session via ``_get_namespaced_project_repo()`` and calls +``repo.get(name)``, which could not see the flushed-but-uncommitted row. + +Test design mirrors bug #589 tests: file-based SQLite with a fresh +``NamespacedProjectRepository`` per CLI invocation. +""" + +from __future__ import annotations + +import os +import shutil +import tempfile +from typing import Any +from unittest.mock import patch + +from behave import given, then, when +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import NullPool +from typer.testing import CliRunner + +from cleveragents.cli.commands.project import app as project_app +from cleveragents.infrastructure.database.models import Base +from cleveragents.infrastructure.database.repositories import ( + NamespacedProjectRepository, +) + +# Patch targets - module-level DI helpers in the project CLI. +_PATCH_PROJECT_REPO = "cleveragents.cli.commands.project._get_namespaced_project_repo" +_PATCH_STORE_EXTRAS = "cleveragents.cli.commands.project._store_project_extras" + +# NOTE: typer.testing.CliRunner always mixes stderr into stdout +# (no mix_stderr parameter exposed). Scenario 3 relies on this +# behaviour to assert "not found" appears in result.output even +# though the CLI writes it to stderr via err_console. +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_fresh_repo(db_path: str) -> NamespacedProjectRepository: + """Build a *new* repo with its own engine/session for *db_path*. + + ``NullPool`` ensures SQLite file handles are released immediately + when the engine is no longer referenced. Schema is pre-created + once during the Background step, so no DDL is needed here. + """ + engine = create_engine(f"sqlite:///{db_path}", echo=False, poolclass=NullPool) + factory = sessionmaker(bind=engine, expire_on_commit=False) + return NamespacedProjectRepository(session_factory=factory) + + +def _cleanup_show_db(context: Any) -> None: + """Remove the temporary directory.""" + shutil.rmtree(context.show_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a fresh project-show database is initialised") +def step_init_show_db(context: Any) -> None: + """Create a temp dir with a fresh SQLite file for show tests.""" + context.show_tmpdir = tempfile.mkdtemp(prefix="show_590_") + context.show_db_path = os.path.join(context.show_tmpdir, "test.db") + + # Pre-create schema. + engine = create_engine( + f"sqlite:///{context.show_db_path}", + echo=False, + poolclass=NullPool, + ) + Base.metadata.create_all(engine) + engine.dispose() + + context.show_result = None + context.add_cleanup(_cleanup_show_db, context) + + +# --------------------------------------------------------------------------- +# When steps - create +# --------------------------------------------------------------------------- + + +@when('I create a project named "{name}" via the project-show CLI') +def step_create_project_show(context: Any, name: str) -> None: + """Invoke ``project create `` with a fresh repo.""" + repo = _make_fresh_repo(context.show_db_path) + with ( + patch(_PATCH_PROJECT_REPO, return_value=repo), + patch(_PATCH_STORE_EXTRAS), + ): + result = runner.invoke(project_app, ["create", name]) + assert result.exit_code == 0, ( + f"project create '{name}' should exit 0 but got " + f"{result.exit_code}. output:\n{result.output}" + ) + + +@when( + 'I create a described project named "{name}" with description "{desc}" ' + "via the project-show CLI" +) +def step_create_project_show_desc(context: Any, name: str, desc: str) -> None: + """Invoke ``project create -d `` with a fresh repo.""" + repo = _make_fresh_repo(context.show_db_path) + with ( + patch(_PATCH_PROJECT_REPO, return_value=repo), + patch(_PATCH_STORE_EXTRAS), + ): + result = runner.invoke(project_app, ["create", name, "-d", desc]) + assert result.exit_code == 0, ( + f"project create '{name}' with description should exit 0 but got " + f"{result.exit_code}. output:\n{result.output}" + ) + + +# --------------------------------------------------------------------------- +# When steps - show +# --------------------------------------------------------------------------- + + +@when('I show the project "{name}" via the project-show CLI') +def step_show_project(context: Any, name: str) -> None: + """Invoke ``project show `` with a fresh repo.""" + repo = _make_fresh_repo(context.show_db_path) + with patch(_PATCH_PROJECT_REPO, return_value=repo): + context.show_result = runner.invoke(project_app, ["show", name]) + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then('the project-show output should contain "{text}"') +def step_show_output_contains(context: Any, text: str) -> None: + """Assert that the show output contains *text* (case-insensitive).""" + result = context.show_result + assert result is not None, "project show was not invoked" + output = result.output + assert text.lower() in output.lower(), ( + f"Expected '{text}' in show output but got:\n{output}" + ) + + +@then("the project-show exit code should be {code:d}") +def step_show_exit_code(context: Any, code: int) -> None: + """Assert the show command exited with the expected code.""" + result = context.show_result + assert result is not None, "project show was not invoked" + actual = result.exit_code + assert actual == code, ( + f"Expected exit code {code}, got {actual}. Output:\n{result.output}" + ) + + +@then("the project-show exit code should not be {code:d}") +def step_show_exit_code_not(context: Any, code: int) -> None: + """Assert the show command did NOT exit with the given code.""" + result = context.show_result + assert result is not None, "project show was not invoked" + actual = result.exit_code + assert actual != code, ( + f"Expected exit code != {code}, got {actual}. Output:\n{result.output}" + ) diff --git a/robot/project_show_after_create.robot b/robot/project_show_after_create.robot new file mode 100644 index 000000000..d9f7e4a38 --- /dev/null +++ b/robot/project_show_after_create.robot @@ -0,0 +1,67 @@ +*** Settings *** +Documentation Integration smoke test for project show after create (bug #590). +... Verifies that ``agents project show`` finds a project +... immediately after ``agents project create`` commits it. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Library String +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Test Cases *** +Project Show Displays Created Project + [Documentation] After creating a project, show should display its details. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_') + ${init}= Run Process ${PYTHON} -m cleveragents init show-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init failed: ${init.stderr} + ${create}= Run Process ${PYTHON} -m cleveragents project create local/show-proj + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=project create failed: ${create.stderr} + ${result}= Run Process ${PYTHON} -m cleveragents project show --format plain local/show-proj + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${result.rc} 0 + ... msg=project show should exit 0 but got ${result.rc}. stderr: ${result.stderr} + Should Contain ${result.stdout} local/show-proj + ... msg=project show output should contain project name but got: ${result.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Project Show With Description Displays Description + [Documentation] Show should display the project description after creation. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_desc_') + ${init}= Run Process ${PYTHON} -m cleveragents init show-desc-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init failed: ${init.stderr} + ${create}= Run Process ${PYTHON} -m cleveragents project create local/desc-proj + ... -d A test project + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=project create failed: ${create.stderr} + ${result}= Run Process ${PYTHON} -m cleveragents project show --format plain local/desc-proj + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${result.rc} 0 + ... msg=project show should exit 0 but got ${result.rc}. stderr: ${result.stderr} + Should Contain ${result.stdout} A test project + ... msg=project show should display description but got: ${result.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Project Show Returns Error For Nonexistent Project + [Documentation] Showing a project that was never created should fail. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='show_590_missing_') + ${init}= Run Process ${PYTHON} -m cleveragents init show-missing-test + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${init.rc} 0 + ... msg=agents init failed: ${init.stderr} + ${result}= Run Process ${PYTHON} -m cleveragents project show --format plain local/nonexistent + ... timeout=60s cwd=${tmpdir} + Should Not Be Equal As Integers ${result.rc} 0 + ... msg=project show for nonexistent project should fail but exited 0 + ${combined}= Set Variable ${result.stdout}${result.stderr} + ${lower}= Convert To Lower Case ${combined} + Should Contain ${lower} not found + ... msg=project show for nonexistent project should mention "not found" but got: ${combined} + [Teardown] Remove Directory ${tmpdir} recursive=True diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 9ac326c3c..fd78d47e1 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -2812,8 +2812,8 @@ class NamespacedProjectRepository: 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 (``create``, ``update``, ``delete``) flush **and + commit** within their own session. No external ``UnitOfWork`` is needed. """ def __init__(self, session_factory: Callable[[], Session]) -> None: @@ -2954,12 +2954,15 @@ class NamespacedProjectRepository: row.updated_at = datetime.now(tz=UTC).isoformat() # type: ignore[assignment] session.flush() + session.commit() return project except ProjectNotFoundError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise DatabaseError(f"Failed to update project '{ns_name}': {exc}") from exc + finally: + session.close() @database_retry def delete(self, namespaced_name: str) -> bool: @@ -2987,12 +2990,15 @@ class NamespacedProjectRepository: return False session.delete(row) session.flush() + session.commit() return True except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise DatabaseError( f"Failed to delete project '{namespaced_name}': {exc}" ) from exc + finally: + session.close() # ---------------------------------------------------------------------------