fix(skill): resolve skill add persistence regression after PR #640
CI / build (pull_request) Successful in 25s
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 3m46s
CI / quality (pull_request) Successful in 4m9s
CI / security (pull_request) Successful in 4m13s
CI / typecheck (pull_request) Successful in 4m23s
CI / unit_tests (pull_request) Successful in 6m31s
CI / integration_tests (pull_request) Successful in 7m8s
CI / docker (pull_request) Successful in 1m14s
CI / e2e_tests (pull_request) Successful in 8m28s
CI / coverage (pull_request) Successful in 10m19s
CI / status-check (pull_request) Successful in 1s
CI / build (push) Successful in 24s
CI / lint (push) Successful in 3m36s
CI / quality (push) Successful in 3m41s
CI / typecheck (push) Successful in 3m56s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 4m4s
CI / integration_tests (push) Successful in 7m1s
CI / coverage (push) Failing after 12m22s
CI / e2e_tests (push) Failing after 16m19s
CI / unit_tests (push) Successful in 18m40s
CI / docker (push) Successful in 57s
CI / status-check (push) Failing after 3s
CI / benchmark-publish (push) Successful in 28m38s
CI / benchmark-regression (pull_request) Successful in 52m22s
CI / build (pull_request) Successful in 25s
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 3m46s
CI / quality (pull_request) Successful in 4m9s
CI / security (pull_request) Successful in 4m13s
CI / typecheck (pull_request) Successful in 4m23s
CI / unit_tests (pull_request) Successful in 6m31s
CI / integration_tests (pull_request) Successful in 7m8s
CI / docker (pull_request) Successful in 1m14s
CI / e2e_tests (pull_request) Successful in 8m28s
CI / coverage (pull_request) Successful in 10m19s
CI / status-check (pull_request) Successful in 1s
CI / build (push) Successful in 24s
CI / lint (push) Successful in 3m36s
CI / quality (push) Successful in 3m41s
CI / typecheck (push) Successful in 3m56s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 4m4s
CI / integration_tests (push) Successful in 7m1s
CI / coverage (push) Failing after 12m22s
CI / e2e_tests (push) Failing after 16m19s
CI / unit_tests (push) Successful in 18m40s
CI / docker (push) Successful in 57s
CI / status-check (push) Failing after 3s
CI / benchmark-publish (push) Successful in 28m38s
CI / benchmark-regression (pull_request) Successful in 52m22s
Root cause: _build_skill_service in container.py created a SkillRepository pointing at the database but did not ensure the skills/skill_items tables existed. When the tables were missing, SkillRepository.list_all() and create() failed silently (caught by SkillService._load_from_db and _persist_skill exception handlers), causing the service to operate in in-memory-only mode. Skills added in one CLI process were lost when a new process created a fresh SkillService. Additionally, SkillRepository lacked auto_commit support. Each call to session_factory() returned a new session, so the flush in create/update/ delete operated on a different session than the commit in SkillService._commit(), meaning data was never actually persisted even when the tables existed. Fix: 1. Add targeted table creation in _build_skill_service (following the pattern in _build_session_service) — checks for missing skills and skill_items tables and creates them via Base.metadata.create_all. 2. Add auto_commit parameter to SkillRepository (following the pattern in SessionRepository) so each mutating method commits and closes its own session. 3. Pass auto_commit=True from the container builder. 4. Remove @tdd_expected_fail from TDD test (leaving @tdd_bug and @tdd_bug_980 as permanent regression guards). ISSUES CLOSED: #980
This commit was merged in pull request #1120.
This commit is contained in:
@@ -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 <file>` 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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user