diff --git a/features/steps/tdd_skill_add_regression_steps.py b/features/steps/tdd_skill_add_regression_steps.py new file mode 100644 index 000000000..931c36728 --- /dev/null +++ b/features/steps/tdd_skill_add_regression_steps.py @@ -0,0 +1,199 @@ +"""Step definitions for TDD Bug #980 — skill add cross-process persistence. + +These steps capture bug #980 by exercising the *real* CLI code path +across independent process-like invocations. The existing persistence +tests (``skill_add_persist.feature``) verify round-trip within the +same Python process — they create two ``SkillService`` instances +sharing the same in-memory SQLAlchemy session factory. That approach +cannot detect the cross-process regression introduced by PR #640. + +This test: + +1. Points the CLI at the per-scenario SQLite database (already set up + by ``before_scenario`` with all migrations applied). +2. Runs ``agents skill add --config --format json`` via the + CLI app with a fresh DI container (simulating process 1). +3. Resets the DI container and runs ``agents skill list --format json`` + (simulating process 2) — the SkillService is rebuilt from scratch. +4. Asserts the skill appears in the listing. + +The ``@tdd_expected_fail`` tag on the feature inverts the assertion +failure to a CI pass while the bug is unfixed. The root cause is that +``_build_skill_service`` creates a standalone engine that connects to +the database but the ``skills`` table does not exist in databases +initialised by ``agents init`` (the Alembic migration for skills was +never added, or the skill service bypasses migration-managed tables). +""" + +from __future__ import annotations + +import contextlib +import os +import tempfile + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner + +from cleveragents.application.container import reset_container +from cleveragents.cli.main import app as root_app + +runner = CliRunner() + +_SKILL_YAML = """\ +name: local/tdd-cross-process +description: "TDD cross-process persistence test skill" + +tools: + - name: builtin/read_file +""" + +_SKILL_NAME = "local/tdd-cross-process" + + +# --------------------------------------------------------------------------- +# Given +# --------------------------------------------------------------------------- + + +@given("a cross-process skill persistence environment") +def step_cross_process_env(context: Context) -> None: + """Write a skill YAML config file for use in subsequent steps. + + The per-scenario database is already set up by ``before_scenario`` + in ``environment.py`` (with migrations applied via the template-DB + fast path). We only need to create the skill YAML and ensure the + DI container starts from a clean state. + """ + # Write skill YAML config to a temp file + fd, config_path = tempfile.mkstemp(suffix=".yaml", prefix="tdd_skill_") + with os.fdopen(fd, "w") as fh: + fh.write(_SKILL_YAML) + context.tdd_skill_config_path = config_path + + # Register cleanup for the temp config file + def _cleanup() -> None: + with contextlib.suppress(OSError): + os.unlink(config_path) + reset_container() + + context.add_cleanup(_cleanup) + + # Start with a clean container + reset_container() + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when("I add a skill via a CLI subprocess") +def step_add_skill_subprocess(context: Context) -> None: + """Run ``agents skill add --config --format json``. + + Simulates CLI process 1. After this call, the DI container and its + in-memory SkillService hold the newly added skill. The skill + *should* also be persisted to the SQLite database so that process 2 + can see it. + """ + result = runner.invoke( + root_app, + [ + "skill", + "add", + "--config", + context.tdd_skill_config_path, + "--format", + "json", + ], + ) + context.tdd_skill_add_output = result.output + context.tdd_skill_add_exit_code = result.exit_code + + +@when("I list skills via a separate CLI subprocess") +def step_list_skills_subprocess(context: Context) -> None: + """Run ``agents skill list --format json`` with a fresh DI container. + + The DI container is reset before the call to simulate a completely + independent CLI process — this is the cross-process boundary that + exposes bug #980. The new SkillService instance must load skills + from the database to find the skill added in the previous step. + """ + # Simulate cross-process boundary: fresh container = fresh SkillService + reset_container() + + result = runner.invoke( + root_app, + ["skill", "list", "--format", "json"], + ) + context.tdd_skill_list_output = result.output + context.tdd_skill_list_exit_code = result.exit_code + + +@when("I show the skill via a separate CLI subprocess") +def step_show_skill_subprocess(context: Context) -> None: + """Run ``agents skill show --format json`` with a fresh container. + + The DI container is reset before the call to simulate a completely + independent CLI process. + """ + # Simulate cross-process boundary + reset_container() + + result = runner.invoke( + root_app, + ["skill", "show", _SKILL_NAME, "--format", "json"], + ) + context.tdd_skill_show_output = result.output + context.tdd_skill_show_exit_code = result.exit_code + + +# --------------------------------------------------------------------------- +# Then +# --------------------------------------------------------------------------- + + +@then("the cross-process skill list should contain the added skill") +def step_assert_skill_in_list(context: Context) -> None: + """Assert that the skill added in one process is visible in another. + + Checks the output of ``skill list`` for the skill name. This + assertion will FAIL while bug #980 is present because the second + process creates a fresh SkillService that does not find the skill + in the database (the ``skills`` table is missing or was never + populated due to the DB-fallback in ``_build_skill_service``). + + The ``@tdd_expected_fail`` tag inverts this failure to a CI pass. + """ + list_output = context.tdd_skill_list_output + + assert _SKILL_NAME in list_output, ( + f"Expected skill '{_SKILL_NAME}' in cross-process skill list output " + f"but it was not found.\n" + f"Add exit code: {context.tdd_skill_add_exit_code}\n" + f"Add output:\n{context.tdd_skill_add_output}\n" + f"List exit code: {context.tdd_skill_list_exit_code}\n" + f"List output:\n{list_output}" + ) + + +@then("the cross-process skill show output should contain the skill name") +def step_assert_skill_show_contains_name(context: Context) -> None: + """Assert that ``skill show`` in a separate process finds the skill. + + This assertion will FAIL while bug #980 is present because the + second process cannot find the skill in its fresh service instance. + """ + show_output = context.tdd_skill_show_output + show_exit = context.tdd_skill_show_exit_code + + assert show_exit == 0 and _SKILL_NAME in show_output, ( + f"Expected skill '{_SKILL_NAME}' visible via cross-process " + f"'skill show' but it was not found.\n" + f"Show exit code: {show_exit}\n" + f"Show output:\n{show_output}\n" + f"Add exit code: {context.tdd_skill_add_exit_code}\n" + f"Add output:\n{context.tdd_skill_add_output}" + ) diff --git a/features/tdd_skill_add_regression.feature b/features/tdd_skill_add_regression.feature new file mode 100644 index 000000000..6fac00d13 --- /dev/null +++ b/features/tdd_skill_add_regression.feature @@ -0,0 +1,30 @@ +@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 + skills across separate CLI process invocations + So that the bug is captured and will be caught by a regression test + + Bug #980 reports that skills registered via `agents skill add` in one + CLI process are not visible when `agents skill list` is run in a + separate CLI process. Existing persistence tests pass because they + verify round-trip within the same Python process (creating two + SkillService instances sharing the same in-memory database). + + 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. 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 + When I add a skill via a CLI subprocess + And I list skills via a separate CLI subprocess + Then the cross-process skill list should contain the added skill + + Scenario: skill add persists config path across processes + Given a cross-process skill persistence environment + When I add a skill via a CLI subprocess + And I show the skill via a separate CLI subprocess + Then the cross-process skill show output should contain the skill name diff --git a/robot/helper_tdd_skill_add_regression.py b/robot/helper_tdd_skill_add_regression.py new file mode 100644 index 000000000..b8c497c16 --- /dev/null +++ b/robot/helper_tdd_skill_add_regression.py @@ -0,0 +1,223 @@ +"""Helper script for tdd_skill_add_regression.robot smoke tests. + +Each subcommand exercises the real CLI code path (no mocks) to reproduce +bug #980: skills registered via ``agents skill add --config `` in +one CLI invocation are not visible when ``agents skill list`` is run in +a separate CLI invocation. + +The existing in-process persistence tests pass because they share an +in-memory SQLAlchemy session factory between two SkillService instances. +This helper uses ``reset_container()`` between invocations to simulate +the cross-process boundary — each CLI invocation starts with a clean +DI container and must load skill data from the on-disk database. + +The helper reports the **real** outcome: it exits 0 and prints the +sentinel when the skill is found cross-process (bug is fixed), and +exits 1 when the skill is not found (bug still present). The +``tdd_expected_fail_listener`` on the Robot side handles pass/fail +inversion while the bug remains open. +""" + +from __future__ import annotations + +import os +import shutil +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import NoReturn + +# Ensure local source tree is importable. +_ROOT = Path(__file__).resolve().parents[1] +_SRC = str(_ROOT / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from typer.testing import CliRunner # noqa: E402 + +from cleveragents.application.container import reset_container # noqa: E402 +from cleveragents.cli.main import app as root_app # noqa: E402 + +runner = CliRunner() + +_SKILL_YAML = """\ +name: local/tdd-cross-process +description: "TDD cross-process persistence test skill" + +tools: + - name: builtin/read_file +""" + +_SKILL_NAME = "local/tdd-cross-process" + + +def _fail(message: str) -> NoReturn: + """Print an error message to stderr and exit with code 1.""" + print(message, file=sys.stderr) + sys.exit(1) + + +def _setup_env() -> tuple[str, str, dict[str, str | None]]: + """Create a temp directory with a skill config and initialised database. + + Returns: + Tuple of (tmpdir, config_path, saved_env_vars). + """ + tmpdir = tempfile.mkdtemp(prefix="tdd_skill_robot_xproc_") + + # Write skill YAML config + config_path = os.path.join(tmpdir, "test-skill.yaml") + with open(config_path, "w") as fh: + fh.write(_SKILL_YAML) + + # Database path + db_dir = os.path.join(tmpdir, ".cleveragents") + os.makedirs(db_dir, exist_ok=True) + db_url = f"sqlite:///{db_dir}/db.sqlite" + + # Save and override env vars + saved_env: dict[str, str | None] = {} + for key in ( + "CLEVERAGENTS_DATABASE_URL", + "CLEVERAGENTS_TEST_DATABASE_URL", + "CLEVERAGENTS_HOME", + ): + saved_env[key] = os.environ.pop(key, None) + + os.environ["CLEVERAGENTS_HOME"] = tmpdir + os.environ["CLEVERAGENTS_DATABASE_URL"] = db_url + + # Initialise the project (creates database + migrations) + reset_container() + init_result = runner.invoke( + root_app, ["init", "--yes", "--force", "--path", tmpdir] + ) + reset_container() + + if init_result.exit_code != 0: + _restore_env(saved_env, tmpdir) + _fail( + f"Project init failed with exit code {init_result.exit_code}.\n" + f"Output: {init_result.output}" + ) + + return tmpdir, config_path, saved_env + + +def _restore_env(saved_env: dict[str, str | None], tmpdir: str) -> None: + """Restore saved env vars and clean up temp directory.""" + for key, val in saved_env.items(): + if val is not None: + os.environ[key] = val + else: + os.environ.pop(key, None) + reset_container() + shutil.rmtree(tmpdir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def cross_process_list() -> None: + """Add a skill in one CLI invocation, list in another. + + Simulates cross-process boundary by resetting the DI container + between invocations. Exits 0 with sentinel when the skill is + found (bug fixed). Exits 1 when not found (bug present). + """ + tmpdir, config_path, saved_env = _setup_env() + try: + # Invocation 1: add the skill + reset_container() + add_result = runner.invoke( + root_app, + ["skill", "add", "--config", config_path, "--format", "json"], + ) + if add_result.exit_code != 0: + _fail( + f"skill add failed with exit code {add_result.exit_code}.\n" + f"Output: {add_result.output}" + ) + + # Invocation 2: list skills (cross-process boundary) + reset_container() + list_result = runner.invoke( + root_app, + ["skill", "list", "--format", "json"], + ) + + if _SKILL_NAME not in list_result.output: + _fail( + f"Skill '{_SKILL_NAME}' not found in cross-process list.\n" + f"Add output: {add_result.output}\n" + f"List exit code: {list_result.exit_code}\n" + f"List output: {list_result.output}" + ) + + print("tdd-skill-add-cross-process-list-ok") + finally: + _restore_env(saved_env, tmpdir) + + +def cross_process_show() -> None: + """Add a skill in one CLI invocation, show in another. + + Simulates cross-process boundary by resetting the DI container + between invocations. Exits 0 with sentinel when the skill show + succeeds (bug fixed). Exits 1 when not found (bug present). + """ + tmpdir, config_path, saved_env = _setup_env() + try: + # Invocation 1: add the skill + reset_container() + add_result = runner.invoke( + root_app, + ["skill", "add", "--config", config_path, "--format", "json"], + ) + if add_result.exit_code != 0: + _fail( + f"skill add failed with exit code {add_result.exit_code}.\n" + f"Output: {add_result.output}" + ) + + # Invocation 2: show the skill (cross-process boundary) + reset_container() + show_result = runner.invoke( + root_app, + ["skill", "show", _SKILL_NAME, "--format", "json"], + ) + + if show_result.exit_code != 0 or _SKILL_NAME not in show_result.output: + _fail( + f"Skill '{_SKILL_NAME}' not found in cross-process show.\n" + f"Add output: {add_result.output}\n" + f"Show exit code: {show_result.exit_code}\n" + f"Show output: {show_result.output}" + ) + + print("tdd-skill-add-cross-process-show-ok") + finally: + _restore_env(saved_env, tmpdir) + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "cross-process-list": cross_process_list, + "cross-process-show": cross_process_show, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) + sys.exit(1) + cmd = _COMMANDS[sys.argv[1]] + cmd() diff --git a/robot/tdd_skill_add_regression.robot b/robot/tdd_skill_add_regression.robot new file mode 100644 index 000000000..b5cd930ca --- /dev/null +++ b/robot/tdd_skill_add_regression.robot @@ -0,0 +1,36 @@ +*** Settings *** +Documentation TDD Bug #980 — skill add cross-process persistence +... Integration smoke tests verifying that skills registered via +... ``agents skill add --config `` in one CLI process are +... visible when ``agents skill list`` is run in a separate CLI +... 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 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 + +*** Variables *** +${HELPER} ${CURDIR}/helper_tdd_skill_add_regression.py + +*** Test Cases *** +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 + ${result}= Run Process ${PYTHON} ${HELPER} cross-process-list cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-skill-add-cross-process-list-ok + +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 + ${result}= Run Process ${PYTHON} ${HELPER} cross-process-show cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-skill-add-cross-process-show-ok 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()