From 26de6bb7de298416468e4889b072b394095580ce Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 9 Mar 2026 06:59:15 +0000 Subject: [PATCH] fix(project): persist project to database during creation (#591) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix `agents project create` not persisting projects to the database. `NamespacedProjectRepository.create()` called `session.flush()` but never `session.commit()`, so projects were invisible to subsequent `agents project list` invocations that open a separate session. ## Changes **Production fix** (`src/cleveragents/infrastructure/database/repositories.py`): - Replace `session.flush()` with `session.commit()` in `NamespacedProjectRepository.create()` - Add `finally: session.close()` guard for proper session lifecycle **Tests & benchmarks**: - 4 Behave BDD regression scenarios (`features/project_create_persist.feature`) - Robot Framework integration smoke tests (`robot/project_create_persist.robot`) - ASV benchmarks for create-then-list round-trip (`benchmarks/project_create_persist_bench.py`) ## Review feedback addressed - **H3**: Added `finally: session.close()` to `create()` method - **M1**: Removed redundant `session.flush()` before `session.commit()` - **M2**: Updated stale TDD "expected to fail" comments — this PR includes the fix - **M4**: Rewrote CHANGELOG entry to describe the fix, not just tests - **F1**: Updated Robot documentation (Suite Setup/Teardown kept — needed for `${PYTHON}`) - **F2**: Fixed bare assertion `"my-app"` to `"local/my-app"` in namespace scenario - **F3**: Removed redundant `Base.metadata.create_all()` from `_make_fresh_repo()` ## Process - Single squashed commit, rebased onto `master` (no merge commits) - Prescribed commit message from issue #589 metadata ISSUES CLOSED: #589 Reviewed-on: https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/591 Reviewed-by: Rui Hu Co-authored-by: Brent E. Edwards Co-committed-by: Brent E. Edwards --- CHANGELOG.md | 6 + benchmarks/project_create_persist_bench.py | 110 +++++++++++ features/project_create_persist.feature | 34 ++++ .../steps/project_create_persist_steps.py | 184 ++++++++++++++++++ ...ositories_error_handling_coverage_steps.py | 16 +- robot/project_create_persist.robot | 55 ++++++ .../infrastructure/database/repositories.py | 4 +- 7 files changed, 404 insertions(+), 5 deletions(-) create mode 100644 benchmarks/project_create_persist_bench.py create mode 100644 features/project_create_persist.feature create mode 100644 features/steps/project_create_persist_steps.py create mode 100644 robot/project_create_persist.robot diff --git a/CHANGELOG.md b/CHANGELOG.md index 380f92a63..d19696ced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +- 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 + `agents project list` calls. Added `session.commit()` and a `finally: + session.close()` guard. Includes 4 Behave BDD regression scenarios, + Robot Framework integration smoke tests, and ASV benchmarks. (#589) - Added TDD-style Behave BDD tests for the built-in `git-checkout` resource type bootstrap. Three scenarios: one failing TDD test reproducing bug #524 (no bootstrap called during init), and two regression tests verifying `bootstrap_builtin_types()` diff --git a/benchmarks/project_create_persist_bench.py b/benchmarks/project_create_persist_bench.py new file mode 100644 index 000000000..b12a58e96 --- /dev/null +++ b/benchmarks/project_create_persist_bench.py @@ -0,0 +1,110 @@ +"""ASV benchmarks for project create + list persistence round-trip. + +Measures the cost of creating a project and then listing it back via the +``NamespacedProjectRepository``, using a file-based SQLite database so +that each operation gets a fresh session (matching real CLI behaviour). + +Targets bug #589 — ``create()`` now calls ``session.commit()`` so data +is visible to a subsequent ``list()`` in a separate session. +""" + +from __future__ import annotations + +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, +) + + +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 ProjectPersistRoundTripSuite: + """Benchmark create-then-list round trip with separate sessions.""" + + timeout = 30.0 + + def setup(self) -> None: + self._tmpdir = tempfile.mkdtemp(prefix="bench_persist_589_") + self._db_path = os.path.join(self._tmpdir, "bench.db") + # Initialise schema once. + 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_list(self) -> None: + """Create a project in one session, list in another.""" + self._counter += 1 + name = f"bench-{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) + + list_repo = _fresh_repo(self._db_path) + list_repo.list_projects() + + def track_list_after_create_count(self) -> int: + """Track whether list sees the created project. + + Returns the number of projects visible after create + commit. + Expected value is 1 (or more if called repeatedly). + """ + self._counter += 1 + name = f"bench-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) + + list_repo = _fresh_repo(self._db_path) + projects = list_repo.list_projects() + return len(projects) + + +ProjectPersistRoundTripSuite.track_list_after_create_count.unit = "projects" # type: ignore[attr-defined] diff --git a/features/project_create_persist.feature b/features/project_create_persist.feature new file mode 100644 index 000000000..23b168b4d --- /dev/null +++ b/features/project_create_persist.feature @@ -0,0 +1,34 @@ +# Regression tests for bug #589 — project create must commit to the database. +Feature: Project create persists to database + As a developer using the agents CLI + I want projects created with "agents project create" to persist in the database + So that "agents project list" shows previously created projects + + Background: + Given a fresh project-persist database is initialised + + @tdd @bug589 + 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 + 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 + And I list projects via the persist CLI + Then the persist project list should contain "local/alpha" + And the persist project list should contain "local/beta" + + @tdd @bug589 + 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 + 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 + Then the persist duplicate creation should fail with "already exists" diff --git a/features/steps/project_create_persist_steps.py b/features/steps/project_create_persist_steps.py new file mode 100644 index 000000000..c7e335a55 --- /dev/null +++ b/features/steps/project_create_persist_steps.py @@ -0,0 +1,184 @@ +"""Step definitions for project_create_persist.feature (bug #589). + +Regression tests verifying that ``agents project create`` commits data +to the database so that ``agents project list`` shows previously created +projects. + +Design rationale +~~~~~~~~~~~~~~~~ +The bug was that ``NamespacedProjectRepository.create()`` called +``session.flush()`` but never ``session.commit()``. Each CLI invocation +obtains a **new** session from the factory, so flushed-but-uncommitted +rows were invisible to subsequent invocations. + +To exercise the fix we use a **file-based** SQLite database and patch +``_get_namespaced_project_repo`` so that every CLI invocation receives a +*fresh* ``NamespacedProjectRepository`` backed by a **new session +factory** pointing at the same file. This mirrors real CLI behaviour +where each ``agents project …`` invocation gets its own session. +""" + +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 target - the module-level DI helper that returns the project repo. +_PATCH_PROJECT_REPO = "cleveragents.cli.commands.project._get_namespaced_project_repo" +# The --invariant flag is never passed in these scenarios, so +# _store_project_extras is effectively a no-op. We patch it defensively +# to avoid side-effects from DI look-ups in the real implementation. +_PATCH_STORE_EXTRAS = "cleveragents.cli.commands.project._store_project_extras" + +runner = CliRunner() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_fresh_repo(db_path: str) -> NamespacedProjectRepository: + """Build a *new* repo with its own engine/session for *db_path*. + + Each call simulates a separate CLI invocation: a new engine and session + factory are created, so uncommitted data from a prior invocation is not + visible. ``NullPool`` is used so that SQLite file handles are released + immediately when the engine is no longer referenced. + + Schema is pre-created once during the Background step, so we skip + ``Base.metadata.create_all()`` here to avoid redundant DDL. + """ + 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_persist_db(context: Any) -> None: + """Remove the temporary directory created for the test.""" + shutil.rmtree(context.persist_tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a fresh project-persist database is initialised") +def step_init_persist_db(context: Any) -> None: + """Create a temp dir with a fresh SQLite file for persistence tests.""" + context.persist_tmpdir = tempfile.mkdtemp(prefix="persist_589_") + context.persist_db_path = os.path.join(context.persist_tmpdir, "test.db") + + # Pre-create schema so every fresh repo sees the tables. + engine = create_engine( + f"sqlite:///{context.persist_db_path}", + echo=False, + poolclass=NullPool, + ) + Base.metadata.create_all(engine) + engine.dispose() + + # Initialise result holders. + context.persist_create_results = [] # type: ignore[attr-defined] + context.persist_list_result = None + context.persist_dup_result = None + + context.add_cleanup(_cleanup_persist_db, context) + + +# --------------------------------------------------------------------------- +# When steps - create +# --------------------------------------------------------------------------- + + +@when('I create a project named "{name}" via the persist CLI') +def step_create_project_persist(context: Any, name: str) -> None: + """Invoke ``project create `` with a fresh repo per call.""" + repo = _make_fresh_repo(context.persist_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}" + ) + context.persist_create_results.append(result) + + +@when('I attempt to create a duplicate project named "{name}" via the persist CLI') +def step_create_duplicate_persist(context: Any, name: str) -> None: + """Invoke ``project create`` again for the same name (expect error).""" + repo = _make_fresh_repo(context.persist_db_path) + with ( + patch(_PATCH_PROJECT_REPO, return_value=repo), + patch(_PATCH_STORE_EXTRAS), + ): + result = runner.invoke(project_app, ["create", name]) + context.persist_dup_result = result + + +# --------------------------------------------------------------------------- +# When steps - list +# --------------------------------------------------------------------------- + + +@when("I list projects via the persist CLI") +def step_list_projects_persist(context: Any) -> None: + """Invoke ``project list`` with a fresh repo pointing at the same DB.""" + repo = _make_fresh_repo(context.persist_db_path) + with patch(_PATCH_PROJECT_REPO, return_value=repo): + result = runner.invoke(project_app, ["list"]) + assert result.exit_code == 0, ( + f"project list should exit 0 but got {result.exit_code}. " + f"output:\n{result.output}" + ) + context.persist_list_result = result + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then('the persist project list should contain "{name}"') +def step_persist_list_contains(context: Any, name: str) -> None: + """Assert that the list output includes the given project name.""" + result = context.persist_list_result + assert result is not None, "project list was not invoked" + output = result.output + assert name in output, ( + f"Expected '{name}' in project list output but got:\n{output}" + ) + + +@then('the persist duplicate creation should fail with "{text}"') +def step_persist_dup_fails(context: Any, text: str) -> None: + """Assert that the duplicate creation produced an error containing *text*.""" + result = context.persist_dup_result + assert result is not None, "duplicate create was not invoked" + assert result.exit_code != 0, ( + f"Duplicate create should fail (exit_code != 0) but got " + f"{result.exit_code}. output:\n{result.output}" + ) + output = result.output + assert text.lower() in output.lower(), ( + f"Expected '{text}' in duplicate-create output but got:\n{output}" + ) diff --git a/features/steps/repositories_error_handling_coverage_steps.py b/features/steps/repositories_error_handling_coverage_steps.py index df537ada9..6cf184052 100644 --- a/features/steps/repositories_error_handling_coverage_steps.py +++ b/features/steps/repositories_error_handling_coverage_steps.py @@ -61,10 +61,11 @@ def _mock_session_op_error_on_query() -> MagicMock: def _mock_session_op_error_on_flush() -> MagicMock: - """Session whose .flush() raises OperationalError but .add()/.query() work.""" + """Session whose .flush()/.commit() raises OperationalError but .add()/.query() work.""" mock = MagicMock() mock.add.return_value = None mock.flush.side_effect = OperationalError("disk I/O error", {}, None) + mock.commit.side_effect = OperationalError("disk I/O error", {}, None) mock.rollback.return_value = None # query returns a mock that allows filter_by().first() to return None query_mock = MagicMock() @@ -75,12 +76,15 @@ def _mock_session_op_error_on_flush() -> MagicMock: def _mock_session_unique_integrity_on_flush() -> MagicMock: - """Session whose .flush() raises UNIQUE IntegrityError.""" + """Session whose .flush()/.commit() raises UNIQUE IntegrityError.""" mock = MagicMock() mock.add.return_value = None mock.flush.side_effect = IntegrityError( "UNIQUE constraint failed: table.name", {}, None ) + mock.commit.side_effect = IntegrityError( + "UNIQUE constraint failed: table.name", {}, None + ) mock.rollback.return_value = None query_mock = MagicMock() query_mock.filter_by.return_value.first.return_value = None @@ -90,12 +94,15 @@ def _mock_session_unique_integrity_on_flush() -> MagicMock: def _mock_session_non_unique_integrity_on_flush() -> MagicMock: - """Session whose .flush() raises a non-UNIQUE IntegrityError.""" + """Session whose .flush()/.commit() raises a non-UNIQUE IntegrityError.""" mock = MagicMock() mock.add.return_value = None mock.flush.side_effect = IntegrityError( "CHECK constraint failed: some_check", {}, None ) + mock.commit.side_effect = IntegrityError( + "CHECK constraint failed: some_check", {}, None + ) mock.rollback.return_value = None query_mock = MagicMock() query_mock.filter_by.return_value.first.return_value = None @@ -105,10 +112,11 @@ def _mock_session_non_unique_integrity_on_flush() -> MagicMock: def _mock_session_integrity_on_flush() -> MagicMock: - """Session whose .flush() raises IntegrityError (generic).""" + """Session whose .flush()/.commit() raises IntegrityError (generic).""" mock = MagicMock() mock.add.return_value = None mock.flush.side_effect = IntegrityError("FOREIGN KEY constraint failed", {}, None) + mock.commit.side_effect = IntegrityError("FOREIGN KEY constraint failed", {}, None) mock.rollback.return_value = None query_mock = MagicMock() query_mock.filter_by.return_value.first.return_value = None diff --git a/robot/project_create_persist.robot b/robot/project_create_persist.robot new file mode 100644 index 000000000..89dba64fa --- /dev/null +++ b/robot/project_create_persist.robot @@ -0,0 +1,55 @@ +*** Settings *** +Documentation Integration smoke test for project create persistence (bug #589). +... Verifies that ``agents project create`` commits to the database +... so that ``agents project list`` shows previously created projects. +Resource ${CURDIR}/common.resource +Library Process +Library OperatingSystem +Library String +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Test Cases *** +Project Create Then List Shows Created Project + [Documentation] After creating a project, listing projects should include it. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='persist_589_') + ${init}= Run Process ${PYTHON} -m cleveragents init persist-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 project create local/smoke-proj + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create.rc} 0 + ... msg=project create should exit 0 but got ${create.rc}. stderr: ${create.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents project list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=project list should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Contain ${list.stdout} local/smoke-proj + ... msg=project list output should contain 'local/smoke-proj' but got: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True + +Multiple Created Projects Appear In List + [Documentation] Creating two projects should result in both appearing in list. + ${tmpdir}= Evaluate __import__('tempfile').mkdtemp(prefix='persist_589_multi_') + ${init}= Run Process ${PYTHON} -m cleveragents init persist-multi + ... 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_first}= Run Process ${PYTHON} -m cleveragents project create local/first + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create_first.rc} 0 + ... msg=project create local/first should exit 0 but got ${create_first.rc}. stderr: ${create_first.stderr} + ${create_second}= Run Process ${PYTHON} -m cleveragents project create local/second + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${create_second.rc} 0 + ... msg=project create local/second should exit 0 but got ${create_second.rc}. stderr: ${create_second.stderr} + ${list}= Run Process ${PYTHON} -m cleveragents project list + ... timeout=60s cwd=${tmpdir} + Should Be Equal As Integers ${list.rc} 0 + ... msg=project list should exit 0 but got ${list.rc}. stderr: ${list.stderr} + Should Contain ${list.stdout} local/first + ... msg=project list should contain 'local/first' but got: ${list.stdout} + Should Contain ${list.stdout} local/second + ... msg=project list should contain 'local/second' but got: ${list.stdout} + [Teardown] Remove Directory ${tmpdir} recursive=True diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 95f5deeea..9ac326c3c 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -2841,7 +2841,7 @@ class NamespacedProjectRepository: try: db_model = NamespacedProjectModel.from_domain(project) session.add(db_model) - session.flush() + session.commit() return project except IntegrityError as exc: session.rollback() @@ -2853,6 +2853,8 @@ class NamespacedProjectRepository: except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise DatabaseError(f"Failed to create project: {exc}") from exc + finally: + session.close() @database_retry def get(self, namespaced_name: str) -> Any: