fix(tool): persist tool registration to database after add

ToolRegistryRepository.create(), .update(), and .delete() called
session.flush() but never session.commit().  The CLI factory creates a
raw sessionmaker without a UnitOfWork wrapper, so the transaction was
never committed and SQLAlchemy performed an implicit rollback when the
session was garbage-collected.

The same bug existed in ValidationAttachmentRepository.attach() and
.detach().

Changes:
- Add session.commit() after session.flush() in all five mutating
  methods across ToolRegistryRepository and
  ValidationAttachmentRepository.
- Add finally: session.close() to guarantee session cleanup regardless
  of success or failure.
- Update class docstrings to reflect the new commit-on-write semantics.
- Add Behave BDD feature (tool_add_persist.feature) with scenarios for
  single-tool round-trip, multi-tool persistence, and duplicate
  rejection, using file-based SQLite to reproduce the cross-session
  issue.
- Add Robot Framework integration test (tool_add_persist.robot) with
  add-then-list and fresh-list-empty scenarios.
- Add ASV benchmark (tool_add_persist_bench.py) with
  track_list_after_add_count metric.

Key decisions:
- File-based SQLite (not in-memory) is used in tests because the bug
  only manifests when the session/engine is fully disposed between add
  and list, simulating separate CLI invocations.
- Step patterns are prefixed with "tool-persist" to avoid AmbiguousStep
  collisions with existing tool_registry_steps.py.
- The commit-in-repository approach was chosen over adding a UnitOfWork
  to the CLI factory because the CLI commands are simple CRUD operations
  that should auto-persist without requiring callers to remember to
  commit.

ISSUES CLOSED: #621

benchmarks/tool_add_persist_bench.py

@@ -0,0 +1,98 @@
+"""ASV benchmarks for tool-add persistence (issue #621).
+
+Measures the round-trip: create a tool ➜ list from a fresh session.
+"""
+
+from __future__ import annotations
+
+import importlib
+import os
+import sys
+import tempfile
+from datetime import datetime
+from pathlib import Path
+from typing import Any
+
+# 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.engine import Engine  # noqa: E402
+from sqlalchemy.orm import Session, sessionmaker  # noqa: E402
+
+from cleveragents.infrastructure.database.models import Base  # noqa: E402
+from cleveragents.infrastructure.database.repositories import (  # noqa: E402
+    ToolRegistryRepository,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _make_tool(name: str) -> dict[str, Any]:
+    now_iso: str = datetime.now().isoformat()
+    ns: str = name.split("/", 1)[0] if "/" in name else ""
+    short: str = name.split("/", 1)[1] if "/" in name else name
+    return {
+        "name": name,
+        "namespace": ns,
+        "short_name": short,
+        "description": f"Bench tool {name}",
+        "tool_type": "tool",
+        "source": "custom",
+        "timeout": 300,
+        "created_at": now_iso,
+        "updated_at": now_iso,
+        "resource_bindings": [],
+    }
+
+
+# ---------------------------------------------------------------------------
+# Benchmark suite
+# ---------------------------------------------------------------------------
+
+
+class ToolAddPersistSuite:
+    """Benchmark the add-then-list persistence round-trip."""
+
+    timeout = 60
+
+    def setup(self) -> None:
+        fd, self._db_path = tempfile.mkstemp(suffix=".db", prefix="tool_bench_")
+        os.close(fd)
+        engine: Engine = create_engine(f"sqlite:///{self._db_path}", echo=False)
+        Base.metadata.create_all(engine)
+        engine.dispose()
+
+    def teardown(self) -> None:
+        if os.path.exists(self._db_path):
+            os.unlink(self._db_path)
+
+    def _repo(self) -> ToolRegistryRepository:
+        engine: Engine = create_engine(f"sqlite:///{self._db_path}", echo=False)
+        factory: sessionmaker[Session] = sessionmaker(
+            bind=engine, expire_on_commit=False
+        )
+        return ToolRegistryRepository(session_factory=factory)
+
+    # -- tracking benchmark ------------------------------------------------
+
+    def track_list_after_add_count(self) -> int:
+        """Returns the count of tools after a single add (should be 1)."""
+        repo: ToolRegistryRepository = self._repo()
+        repo.create(_make_tool("bench/persist-check"))
+        # Open a new repo (fresh engine) to prove persistence
+        repo2: ToolRegistryRepository = self._repo()
+        tools: list[Any] = repo2.list_all()
+        return len(tools)
+
+    track_list_after_add_count.unit = "tools"  # type: ignore[attr-defined]

features/steps/tool_add_persist_steps.py

@@ -0,0 +1,168 @@
+"""Step definitions for tool-add persistence (issue #621).
+
+Uses *file-based* SQLite so that opening a brand-new session factory
+against the same database file exercises the same code-path as the CLI
+(``tool add`` followed by ``tool list`` in separate process invocations).
+
+All step patterns are prefixed with ``tool-persist`` to avoid
+AmbiguousStep collisions with existing tool_registry_steps.py.
+"""
+
+from __future__ import annotations
+
+import os
+import tempfile
+from datetime import datetime
+from typing import Any
+
+from behave import given, then, when
+from behave.runner import Context
+from sqlalchemy import create_engine
+from sqlalchemy.engine import Engine
+from sqlalchemy.orm import Session, sessionmaker
+
+from cleveragents.infrastructure.database.models import Base
+from cleveragents.infrastructure.database.repositories import (
+    DuplicateToolError,
+    ToolRegistryRepository,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _make_persist_tool_dict(name: str) -> dict[str, Any]:
+    """Create a minimal valid tool dict for persistence testing."""
+    now_iso: str = datetime.now().isoformat()
+    ns: str = name.split("/", 1)[0] if "/" in name else ""
+    short: str = name.split("/", 1)[1] if "/" in name else name
+    return {
+        "name": name,
+        "namespace": ns,
+        "short_name": short,
+        "description": f"Persist-test tool {name}",
+        "tool_type": "tool",
+        "source": "custom",
+        "timeout": 300,
+        "created_at": now_iso,
+        "updated_at": now_iso,
+        "resource_bindings": [],
+    }
+
+
+def _file_engine(db_path: str) -> Engine:
+    """Create a SQLAlchemy engine for a file-based SQLite DB."""
+    return create_engine(f"sqlite:///{db_path}", echo=False)
+
+
+def _file_session_factory(engine: Engine) -> sessionmaker[Session]:
+    """Create a sessionmaker bound to *engine*."""
+    return sessionmaker(bind=engine, expire_on_commit=False)
+
+
+# ---------------------------------------------------------------------------
+# Given
+# ---------------------------------------------------------------------------
+
+
+@given("a tool-persist file-based SQLite database")
+def step_tool_persist_file_db(context: Context) -> None:
+    """Create a temporary file-based SQLite database."""
+    fd, path = tempfile.mkstemp(suffix=".db", prefix="tool_persist_")
+    os.close(fd)
+    engine: Engine = _file_engine(path)
+    Base.metadata.create_all(engine)
+    engine.dispose()
+    context.tool_persist_db_path = path
+
+
+@given("a tool-persist registry repository backed by the file database")
+def step_tool_persist_repo(context: Context) -> None:
+    """Open a ToolRegistryRepository against the file database."""
+    db_path: str = context.tool_persist_db_path
+    engine: Engine = _file_engine(db_path)
+    factory: sessionmaker[Session] = _file_session_factory(engine)
+    context.tool_persist_engine = engine
+    context.tool_persist_factory = factory
+    context.tool_persist_repo = ToolRegistryRepository(session_factory=factory)
+
+
+# ---------------------------------------------------------------------------
+# When
+# ---------------------------------------------------------------------------
+
+
+@when('a tool-persist tool named "{name}" is created')
+def step_tool_persist_create(context: Context, name: str) -> None:
+    """Create a tool via the repository (should commit)."""
+    repo: ToolRegistryRepository = context.tool_persist_repo
+    tool_dict: dict[str, Any] = _make_persist_tool_dict(name)
+    repo.create(tool_dict)
+
+
+@when('a tool-persist tool named "{name}" is created expecting a duplicate error')
+def step_tool_persist_create_dup(context: Context, name: str) -> None:
+    """Attempt to create a duplicate tool and capture the error."""
+    repo: ToolRegistryRepository = context.tool_persist_repo
+    tool_dict: dict[str, Any] = _make_persist_tool_dict(name)
+    try:
+        repo.create(tool_dict)
+        context.tool_persist_dup_error = None
+    except DuplicateToolError as exc:
+        context.tool_persist_dup_error = exc
+
+
+@when("a fresh tool-persist registry repository is opened against the same file")
+def step_tool_persist_fresh_repo(context: Context) -> None:
+    """Open a brand-new engine + session factory against the same DB file."""
+    # Dispose the previous engine to ensure no shared state
+    if hasattr(context, "tool_persist_engine"):
+        context.tool_persist_engine.dispose()
+
+    db_path: str = context.tool_persist_db_path
+    engine: Engine = _file_engine(db_path)
+    factory: sessionmaker[Session] = _file_session_factory(engine)
+    context.tool_persist_fresh_engine = engine
+    context.tool_persist_fresh_factory = factory
+    context.tool_persist_fresh_repo = ToolRegistryRepository(session_factory=factory)
+
+
+@when("all tools are listed via the fresh tool-persist repository")
+def step_tool_persist_list_fresh(context: Context) -> None:
+    """List all tools from the fresh repository."""
+    repo: ToolRegistryRepository = context.tool_persist_fresh_repo
+    context.tool_persist_listed = repo.list_all()
+
+
+# ---------------------------------------------------------------------------
+# Then
+# ---------------------------------------------------------------------------
+
+
+@then('the tool-persist list should contain "{name}"')
+def step_tool_persist_list_contains(context: Context, name: str) -> None:
+    """Assert the listed tools contain a tool with the given name."""
+    listed: list[Any] = context.tool_persist_listed
+    names: list[str] = []
+    for t in listed:
+        if isinstance(t, dict):
+            names.append(str(t.get("name", "")))
+        else:
+            names.append(str(getattr(t, "name", "")))
+    assert name in names, f"Expected '{name}' in {names}"
+
+
+@then("the tool-persist list should have {count:d} entries")
+def step_tool_persist_list_count(context: Context, count: int) -> None:
+    """Assert the number of listed tools."""
+    listed: list[Any] = context.tool_persist_listed
+    assert len(listed) == count, f"Expected {count}, got {len(listed)}"
+
+
+@then("a tool-persist DuplicateToolError should have been raised")
+def step_tool_persist_dup_error(context: Context) -> None:
+    """Assert that a DuplicateToolError was captured."""
+    err: DuplicateToolError | None = getattr(context, "tool_persist_dup_error", None)
+    assert err is not None, "Expected DuplicateToolError but none was raised"
+    assert isinstance(err, DuplicateToolError)

features/tool_add_persist.feature

@@ -0,0 +1,49 @@
+@phase1 @domain @repository @tool_add_persist
+Feature: Tool Add Persistence across sessions
+  As a system operator adding tools via the CLI
+  I want tool registrations to be committed to the database
+  So that a subsequent "tool list" invocation sees the added tools
+
+  # Uses file-based SQLite to reproduce the cross-session persistence
+  # issue described in issue #621.
+
+  # ---------------------------------------------------------------------------
+  # Single tool add/list round-trip
+  # ---------------------------------------------------------------------------
+
+  @tool_persist_single
+  Scenario: Tool add followed by tool list shows the added tool
+    Given a tool-persist file-based SQLite database
+    And a tool-persist registry repository backed by the file database
+    When a tool-persist tool named "local/my-widget" is created
+    And a fresh tool-persist registry repository is opened against the same file
+    And all tools are listed via the fresh tool-persist repository
+    Then the tool-persist list should contain "local/my-widget"
+
+  # ---------------------------------------------------------------------------
+  # Multiple tools
+  # ---------------------------------------------------------------------------
+
+  @tool_persist_multi
+  Scenario: Multiple tools added and listed
+    Given a tool-persist file-based SQLite database
+    And a tool-persist registry repository backed by the file database
+    When a tool-persist tool named "local/alpha" is created
+    And a tool-persist tool named "local/beta" is created
+    And a fresh tool-persist registry repository is opened against the same file
+    And all tools are listed via the fresh tool-persist repository
+    Then the tool-persist list should contain "local/alpha"
+    And the tool-persist list should contain "local/beta"
+    And the tool-persist list should have 2 entries
+
+  # ---------------------------------------------------------------------------
+  # Duplicate rejection
+  # ---------------------------------------------------------------------------
+
+  @tool_persist_dup
+  Scenario: Tool add with duplicate name produces error
+    Given a tool-persist file-based SQLite database
+    And a tool-persist registry repository backed by the file database
+    When a tool-persist tool named "local/dup-tool" is created
+    And a tool-persist tool named "local/dup-tool" is created expecting a duplicate error
+    Then a tool-persist DuplicateToolError should have been raised

robot/helper_tool_add_persist.py

@@ -0,0 +1,125 @@
+"""Helper script for tool_add_persist.robot integration tests.
+
+Each subcommand exercises the real ToolRegistryRepository against a
+file-based SQLite database to verify that data survives session disposal.
+"""
+
+from __future__ import annotations
+
+import os
+import sys
+import tempfile
+from datetime import datetime
+from pathlib import Path
+from typing import Any
+
+# Ensure local source tree is importable
+_SRC = str(Path(__file__).resolve().parents[1] / "src")
+if _SRC not in sys.path:
+    sys.path.insert(0, _SRC)
+
+from sqlalchemy import create_engine  # noqa: E402
+from sqlalchemy.engine import Engine  # noqa: E402
+from sqlalchemy.orm import Session, sessionmaker  # noqa: E402
+
+from cleveragents.infrastructure.database.models import Base  # noqa: E402
+from cleveragents.infrastructure.database.repositories import (  # noqa: E402
+    ToolRegistryRepository,
+)
+
+
+def _make_tool(name: str) -> dict[str, Any]:
+    """Build a minimal tool dict."""
+    now_iso: str = datetime.now().isoformat()
+    ns: str = name.split("/", 1)[0] if "/" in name else ""
+    short: str = name.split("/", 1)[1] if "/" in name else name
+    return {
+        "name": name,
+        "namespace": ns,
+        "short_name": short,
+        "description": f"Integration test tool {name}",
+        "tool_type": "tool",
+        "source": "custom",
+        "timeout": 300,
+        "created_at": now_iso,
+        "updated_at": now_iso,
+        "resource_bindings": [],
+    }
+
+
+def _engine(db_path: str) -> Engine:
+    return create_engine(f"sqlite:///{db_path}", echo=False)
+
+
+def _factory(engine: Engine) -> sessionmaker[Session]:
+    return sessionmaker(bind=engine, expire_on_commit=False)
+
+
+# -------------------------------------------------------------------
+# Subcommands
+# -------------------------------------------------------------------
+
+
+def add_then_list() -> None:
+    """Add a tool, open a new session, list — should find it."""
+    fd, db_path = tempfile.mkstemp(suffix=".db", prefix="tool_persist_robot_")
+    os.close(fd)
+    try:
+        eng: Engine = _engine(db_path)
+        Base.metadata.create_all(eng)
+        repo1: ToolRegistryRepository = ToolRegistryRepository(
+            session_factory=_factory(eng),
+        )
+        repo1.create(_make_tool("local/robot-test"))
+        eng.dispose()
+
+        eng2: Engine = _engine(db_path)
+        repo2: ToolRegistryRepository = ToolRegistryRepository(
+            session_factory=_factory(eng2),
+        )
+        tools: list[Any] = repo2.list_all()
+        eng2.dispose()
+
+        names: list[str] = [
+            str(t.get("name", "") if isinstance(t, dict) else getattr(t, "name", ""))
+            for t in tools
+        ]
+        assert "local/robot-test" in names, f"Expected tool in {names}"
+        print("tool-persist-add-then-list-ok")
+    finally:
+        os.unlink(db_path)
+
+
+def fresh_list_empty() -> None:
+    """List on a fresh (empty) database should return no tools."""
+    fd, db_path = tempfile.mkstemp(suffix=".db", prefix="tool_persist_robot_")
+    os.close(fd)
+    try:
+        eng: Engine = _engine(db_path)
+        Base.metadata.create_all(eng)
+        repo: ToolRegistryRepository = ToolRegistryRepository(
+            session_factory=_factory(eng),
+        )
+        tools: list[Any] = repo.list_all()
+        eng.dispose()
+
+        assert len(tools) == 0, f"Expected 0 tools, got {len(tools)}"
+        print("tool-persist-fresh-list-empty-ok")
+    finally:
+        os.unlink(db_path)
+
+
+_COMMANDS: dict[str, Any] = {
+    "add-then-list": add_then_list,
+    "fresh-list-empty": fresh_list_empty,
+}
+
+if __name__ == "__main__":
+    cmd: str = sys.argv[1] if len(sys.argv) > 1 else ""
+    fn = _COMMANDS.get(cmd)
+    if fn is None:
+        print(
+            f"Unknown command: {cmd!r}. Available: {list(_COMMANDS)}", file=sys.stderr
+        )
+        sys.exit(1)
+    fn()

robot/tool_add_persist.robot

@@ -0,0 +1,27 @@
+*** Settings ***
+Documentation    Integration tests for tool-add persistence (issue #621)
+Resource         ${CURDIR}/common.resource
+Suite Setup      Setup Test Environment
+Suite Teardown   Cleanup Test Environment
+
+*** Variables ***
+${HELPER}    ${CURDIR}/helper_tool_add_persist.py
+
+*** Test Cases ***
+Tool Add Then List Shows Tool
+    [Documentation]    Verify that a tool added via the repository is visible
+    ...    from a brand-new session against the same file database.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    add-then-list    cwd=${WORKSPACE}
+    Log    ${result.stdout}
+    Log    ${result.stderr}
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Contain    ${result.stdout}    tool-persist-add-then-list-ok
+
+Tool List On Fresh Init Shows No Tools
+    [Documentation]    Verify that listing tools on a newly created
+    ...    database returns an empty set.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    fresh-list-empty    cwd=${WORKSPACE}
+    Log    ${result.stdout}
+    Log    ${result.stderr}
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Contain    ${result.stdout}    tool-persist-fresh-list-empty-ok

src/cleveragents/infrastructure/database/repositories.py

@@ -3214,8 +3214,9 @@ class ToolRegistryRepository:
     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 flush **and commit** the transaction, then close
+    the session.  This guarantees that changes are persisted even when no
+    external ``UnitOfWork`` wrapper is present (e.g. the CLI factory).
     """
 
     def __init__(self, session_factory: Callable[[], Session]) -> None:
@@ -3245,6 +3246,7 @@ class ToolRegistryRepository:
             db_model = ToolModel.from_domain(tool)
             session.add(db_model)
             session.flush()
+            session.commit()
             return tool
         except IntegrityError as exc:
             session.rollback()
@@ -3259,6 +3261,8 @@ class ToolRegistryRepository:
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
             session.rollback()
             raise DatabaseError(f"Failed to create tool: {exc}") from exc
+        finally:
+            session.close()
 
     @database_retry
     def get_by_name(self, name: str) -> Any | None:
@@ -3375,10 +3379,13 @@ class ToolRegistryRepository:
                 )
 
             session.flush()
+            session.commit()
             return tool
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
             session.rollback()
             raise DatabaseError(f"Failed to update tool {name_str}: {exc}") from exc
+        finally:
+            session.close()
 
     @database_retry
     def delete(self, name: str) -> bool:
@@ -3412,12 +3419,15 @@ class ToolRegistryRepository:
                 return False
             session.delete(row)
             session.flush()
+            session.commit()
             return True
         except ToolInUseError:
             raise
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
             session.rollback()
             raise DatabaseError(f"Failed to delete tool {name}: {exc}") from exc
+        finally:
+            session.close()
 
 
 class ToolRepository(ToolRegistryRepository):
@@ -3610,6 +3620,9 @@ class ValidationAttachmentRepository:
     """Repository for validation attachment persistence.
 
     Uses a session-factory pattern matching ``ActionRepository``.
+    All mutating methods flush **and commit** the transaction, then close
+    the session to ensure data is persisted even without an external
+    ``UnitOfWork``.
     """
 
     def __init__(self, session_factory: Callable[[], Session]) -> None:
@@ -3697,6 +3710,7 @@ class ValidationAttachmentRepository:
             )
             session.add(model)
             session.flush()
+            session.commit()
             return {
                 "attachment_id": attachment_id,
                 "validation_name": validation_name,
@@ -3720,6 +3734,8 @@ class ValidationAttachmentRepository:
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
             session.rollback()
             raise DatabaseError(f"Failed to attach validation: {exc}") from exc
+        finally:
+            session.close()
 
     @database_retry
     def detach(self, attachment_id: str, validation_name: str | None = None) -> bool:
@@ -3743,12 +3759,15 @@ class ValidationAttachmentRepository:
                 return False
             session.delete(row)
             session.flush()
+            session.commit()
             return True
         except (OperationalError, SQLAlchemyDatabaseError) as exc:
             session.rollback()
             raise DatabaseError(
                 f"Failed to detach validation {attachment_id}: {exc}"
             ) from exc
+        finally:
+            session.close()
 
     @database_retry
     def list_for_resource(