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 <file> --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 <file> --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 <name> --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}"
+    )

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 <file>` 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

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 <file>`` 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()

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 <file>`` 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

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,

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