diff --git a/features/tdd_skill_add_regression.feature b/features/tdd_skill_add_regression.feature index 8a3331d50..6fac00d13 100644 --- a/features/tdd_skill_add_regression.feature +++ b/features/tdd_skill_add_regression.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_980 +@tdd_bug @tdd_bug_980 Feature: TDD Bug #980 — skill add cross-process persistence As a developer I want to verify that `agents skill add --config ` persists @@ -14,8 +14,8 @@ Feature: TDD Bug #980 — skill add cross-process persistence This TDD test captures the regression by using real subprocess invocations — the skill is added via one CLI invocation and listed via an independent CLI invocation, both sharing the same on-disk - SQLite database. The @tdd_expected_fail tag inverts the result so - CI passes while the bug is unfixed. + SQLite database. Originally tagged @tdd_expected_fail while the bug was unfixed; + tag removed after fix in #980. Scenario: skill add in one process is visible to skill list in another Given a cross-process skill persistence environment diff --git a/robot/helper_tdd_skill_add_regression.py b/robot/helper_tdd_skill_add_regression.py index 81102c1c6..b8c497c16 100644 --- a/robot/helper_tdd_skill_add_regression.py +++ b/robot/helper_tdd_skill_add_regression.py @@ -90,7 +90,9 @@ def _setup_env() -> tuple[str, str, dict[str, str | None]]: # Initialise the project (creates database + migrations) reset_container() - init_result = runner.invoke(root_app, ["init", "--yes", "--path", tmpdir]) + init_result = runner.invoke( + root_app, ["init", "--yes", "--force", "--path", tmpdir] + ) reset_container() if init_result.exit_code != 0: diff --git a/robot/tdd_skill_add_regression.robot b/robot/tdd_skill_add_regression.robot index 67e5b4a87..b5cd930ca 100644 --- a/robot/tdd_skill_add_regression.robot +++ b/robot/tdd_skill_add_regression.robot @@ -6,7 +6,7 @@ Documentation TDD Bug #980 — skill add cross-process persistence ... process. Bug #980 reports that the skill is lost across ... process boundaries because the SkillService falls back to ... in-memory storage when the database is not properly initialised. -... Tests are tagged tdd_expected_fail so CI passes via result inversion. +... Tests were originally tagged tdd_expected_fail; tag removed after bug fix. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -18,7 +18,7 @@ ${HELPER} ${CURDIR}/helper_tdd_skill_add_regression.py TDD Skill Add Cross Process Persistence [Documentation] Verify that a skill added via CLI in one process is ... visible via ``skill list`` in a separate process. - [Tags] tdd_bug tdd_bug_980 tdd_expected_fail + [Tags] tdd_bug tdd_bug_980 ${result}= Run Process ${PYTHON} ${HELPER} cross-process-list cwd=${WORKSPACE} timeout=120s on_timeout=kill Log ${result.stdout} Log ${result.stderr} @@ -28,7 +28,7 @@ TDD Skill Add Cross Process Persistence TDD Skill Add Cross Process Show [Documentation] Verify that a skill added via CLI in one process can be ... shown via ``skill show`` in a separate process. - [Tags] tdd_bug tdd_bug_980 tdd_expected_fail + [Tags] tdd_bug tdd_bug_980 ${result}= Run Process ${PYTHON} ${HELPER} cross-process-show cwd=${WORKSPACE} timeout=120s on_timeout=kill Log ${result.stdout} Log ${result.stderr} diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index 93327fbff..7ad6a7148 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -385,16 +385,40 @@ def _build_skill_service( ) return SkillService() + from sqlalchemy import inspect as sa_inspect from sqlalchemy.exc import DatabaseError, OperationalError try: + from cleveragents.infrastructure.database.models import ( + Base, + SkillItemModel, + SkillModel, + ) from cleveragents.infrastructure.database.repositories import ( SkillRepository, ) engine = create_engine(database_url, echo=False) + + # Targeted table creation — only skill tables, never the full + # schema. Mirrors the pattern in _build_session_service where + # session tables are created if missing. Without this, the + # SkillRepository fails silently on every query because the + # skills and skill_items tables do not exist in databases + # initialised before the skill registry migration was added. + # See bug #980. + inspector = sa_inspect(engine) + existing_tables = set(inspector.get_table_names()) + tables_to_create = [ + model.__table__ + for model in (SkillModel, SkillItemModel) + if model.__tablename__ not in existing_tables + ] + if tables_to_create: + Base.metadata.create_all(engine, tables=tables_to_create) + factory = sessionmaker(bind=engine, expire_on_commit=False) - skill_repo = SkillRepository(session_factory=factory) + skill_repo = SkillRepository(session_factory=factory, auto_commit=True) return SkillService( skill_repo=skill_repo, session_factory=factory, diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index a2bed1921..a3d8f7a46 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -4611,13 +4611,28 @@ class SkillRepository: 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 (but do NOT commit) by default; the caller + or a ``UnitOfWork`` wrapper is responsible for committing the + transaction. When ``auto_commit`` is ``True`` (e.g. CLI usage + outside a UoW), each method commits and closes its own session. """ - def __init__(self, session_factory: Callable[[], Session]) -> None: - """Initialise with a callable that returns a new SQLAlchemy Session.""" + def __init__( + self, + session_factory: Callable[[], Session], + *, + auto_commit: bool = False, + ) -> None: + """Initialise with a callable that returns a new SQLAlchemy Session. + + Args: + session_factory: Factory returning a new SQLAlchemy ``Session``. + auto_commit: When ``True``, each public method commits and + closes its session automatically. Useful for CLI commands + that operate outside a ``UnitOfWork``. + """ self._session_factory = session_factory + self._auto_commit = auto_commit def _session(self) -> Session: """Convenience helper to obtain a session.""" @@ -4642,6 +4657,9 @@ class SkillRepository: db_model = SkillModel.from_domain(skill) session.add(db_model) session.flush() + if self._auto_commit: + session.commit() + session.close() return skill except IntegrityError as exc: session.rollback() @@ -4753,6 +4771,9 @@ class SkillRepository: ) session.flush() + if self._auto_commit: + session.commit() + session.close() return skill except SkillNotFoundError: raise @@ -4782,6 +4803,9 @@ class SkillRepository: return False session.delete(row) session.flush() + if self._auto_commit: + session.commit() + session.close() return True except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback()