From d3cc7d30d755b482e23199277598d02af8dda03e Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 8 Mar 2026 03:14:49 +0000 Subject: [PATCH] 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 | 98 ++++++++++ features/steps/tool_add_persist_steps.py | 168 ++++++++++++++++++ features/tool_add_persist.feature | 49 +++++ robot/helper_tool_add_persist.py | 125 +++++++++++++ robot/tool_add_persist.robot | 27 +++ .../infrastructure/database/repositories.py | 23 ++- 6 files changed, 488 insertions(+), 2 deletions(-) create mode 100644 benchmarks/tool_add_persist_bench.py create mode 100644 features/steps/tool_add_persist_steps.py create mode 100644 features/tool_add_persist.feature create mode 100644 robot/helper_tool_add_persist.py create mode 100644 robot/tool_add_persist.robot diff --git a/benchmarks/tool_add_persist_bench.py b/benchmarks/tool_add_persist_bench.py new file mode 100644 index 000000000..34074bb4d --- /dev/null +++ b/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] diff --git a/features/steps/tool_add_persist_steps.py b/features/steps/tool_add_persist_steps.py new file mode 100644 index 000000000..677cddf85 --- /dev/null +++ b/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) diff --git a/features/tool_add_persist.feature b/features/tool_add_persist.feature new file mode 100644 index 000000000..5c701e7f1 --- /dev/null +++ b/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 diff --git a/robot/helper_tool_add_persist.py b/robot/helper_tool_add_persist.py new file mode 100644 index 000000000..36327f5e9 --- /dev/null +++ b/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() diff --git a/robot/tool_add_persist.robot b/robot/tool_add_persist.robot new file mode 100644 index 000000000..8d100f33b --- /dev/null +++ b/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 diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 35226e32e..95f5deeea 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/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(