fix(project): persist project to database during creation (#591)

## 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: #591
Reviewed-by: Rui Hu <rui.hu@cleverthis.com>
Co-authored-by: Brent E. Edwards <brent.edwards@cleverthis.com>
Co-committed-by: Brent E. Edwards <brent.edwards@cleverthis.com>

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()`

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]

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"

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 <name>`` 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}"
+    )

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

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

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: