Files
cleveragents-core/features/steps/tool_add_persist_steps.py
T
freemo d3cc7d30d7
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 19s
CI / security (pull_request) Successful in 34s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 3m45s
CI / docker (pull_request) Successful in 40s
CI / integration_tests (pull_request) Successful in 4m34s
CI / coverage (pull_request) Successful in 4m39s
CI / lint (push) Successful in 12s
CI / build (push) Successful in 15s
CI / quality (push) Successful in 17s
CI / security (push) Successful in 33s
CI / typecheck (push) Successful in 34s
CI / benchmark-regression (push) Has been skipped
CI / unit_tests (push) Successful in 2m21s
CI / integration_tests (push) Successful in 3m9s
CI / docker (push) Successful in 1m0s
CI / coverage (push) Successful in 4m37s
CI / benchmark-publish (push) Has been cancelled
CI / benchmark-regression (pull_request) Successful in 30m5s
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
2026-03-08 22:11:49 +00:00

169 lines
6.4 KiB
Python

"""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)