fix(skill): persist skill registration to database after add
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 19s
CI / build (pull_request) Successful in 19s
CI / security (pull_request) Successful in 36s
CI / typecheck (pull_request) Successful in 39s
CI / unit_tests (pull_request) Successful in 2m29s
CI / docker (pull_request) Successful in 41s
CI / integration_tests (pull_request) Successful in 3m14s
CI / coverage (pull_request) Successful in 4m26s
CI / benchmark-regression (pull_request) Successful in 30m9s
CI / lint (push) Successful in 14s
CI / build (push) Successful in 16s
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 2m18s
CI / docker (push) Successful in 39s
CI / integration_tests (push) Successful in 3m7s
CI / coverage (push) Successful in 4m29s
CI / benchmark-publish (push) Has been cancelled
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 19s
CI / build (pull_request) Successful in 19s
CI / security (pull_request) Successful in 36s
CI / typecheck (pull_request) Successful in 39s
CI / unit_tests (pull_request) Successful in 2m29s
CI / docker (pull_request) Successful in 41s
CI / integration_tests (pull_request) Successful in 3m14s
CI / coverage (pull_request) Successful in 4m26s
CI / benchmark-regression (pull_request) Successful in 30m9s
CI / lint (push) Successful in 14s
CI / build (push) Successful in 16s
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 2m18s
CI / docker (push) Successful in 39s
CI / integration_tests (push) Successful in 3m7s
CI / coverage (push) Successful in 4m29s
CI / benchmark-publish (push) Has been cancelled
Wire SkillService to use SkillRepository for database persistence, fixing the bug where `agents skill add` stored skills only in an in-memory OrderedDict that was lost when the CLI process exited. Changes: - SkillService now accepts optional skill_repo and session_factory parameters. When provided, add_skill() persists to the database and the constructor pre-loads existing skills from DB rows. - _get_skill_service() in the CLI now creates a DB-backed service following the same engine/sessionmaker/repository pattern used by the tool CLI (tool.py). - _reset_skill_service() now installs a fresh in-memory SkillService (instead of setting None) to avoid DB side-effects during unit testing with parallel runners. - remove_skill() also persists the deletion to the database. Test coverage: - Behave BDD: features/skill_add_persist.feature (4 scenarios) - Robot Framework: robot/skill_add_persist.robot (3 smoke tests) - ASV benchmark: benchmarks/skill_add_persist_bench.py ISSUES CLOSED: #620
This commit was merged in pull request #640.
This commit is contained in:
@@ -0,0 +1,127 @@
|
|||||||
|
"""ASV benchmarks for skill add persistence (issue #620).
|
||||||
|
|
||||||
|
Measures the performance of:
|
||||||
|
- Adding a skill with DB persistence
|
||||||
|
- Listing skills after DB-backed add
|
||||||
|
- Removing a skill with DB persistence
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import importlib
|
||||||
|
import os
|
||||||
|
import sys
|
||||||
|
import tempfile
|
||||||
|
from pathlib import Path
|
||||||
|
from typing import Any
|
||||||
|
|
||||||
|
# Ensure the local *source* tree is importable even when ASV has an
|
||||||
|
# older build of the package installed.
|
||||||
|
_SRC = str(Path(__file__).resolve().parents[1] / "src")
|
||||||
|
if _SRC not in sys.path:
|
||||||
|
sys.path.insert(0, _SRC)
|
||||||
|
|
||||||
|
import cleveragents # noqa: E402
|
||||||
|
|
||||||
|
importlib.reload(cleveragents)
|
||||||
|
|
||||||
|
from sqlalchemy import create_engine, event # noqa: E402
|
||||||
|
from sqlalchemy.orm import Session, sessionmaker # noqa: E402
|
||||||
|
|
||||||
|
from cleveragents.application.services.skill_service import ( # noqa: E402
|
||||||
|
SkillService,
|
||||||
|
)
|
||||||
|
from cleveragents.infrastructure.database.models import Base # noqa: E402
|
||||||
|
from cleveragents.infrastructure.database.repositories import ( # noqa: E402
|
||||||
|
SkillRepository,
|
||||||
|
)
|
||||||
|
from cleveragents.skills.schema import SkillConfigSchema # noqa: E402
|
||||||
|
|
||||||
|
_VALID_YAML = """\
|
||||||
|
name: local/bench-persist
|
||||||
|
description: "Benchmark persistent skill"
|
||||||
|
tools:
|
||||||
|
- name: builtin/read_file
|
||||||
|
- name: builtin/write_file
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
|
def _enable_fk(dbapi_conn: Any, _rec: Any) -> None:
|
||||||
|
cursor = dbapi_conn.cursor()
|
||||||
|
cursor.execute("PRAGMA foreign_keys=ON")
|
||||||
|
cursor.close()
|
||||||
|
|
||||||
|
|
||||||
|
def _make_service(factory: Any) -> SkillService:
|
||||||
|
shared = factory()
|
||||||
|
|
||||||
|
def _sf() -> Session:
|
||||||
|
return shared
|
||||||
|
|
||||||
|
repo = SkillRepository(session_factory=_sf)
|
||||||
|
return SkillService(skill_repo=repo, session_factory=_sf)
|
||||||
|
|
||||||
|
|
||||||
|
class SkillAddPersistSuite:
|
||||||
|
"""Benchmark skill add with DB persistence."""
|
||||||
|
|
||||||
|
timeout = 60.0
|
||||||
|
|
||||||
|
def setup(self) -> None:
|
||||||
|
self.engine = create_engine("sqlite:///:memory:", echo=False)
|
||||||
|
event.listen(self.engine, "connect", _enable_fk)
|
||||||
|
Base.metadata.create_all(self.engine)
|
||||||
|
self.factory: sessionmaker[Session] = sessionmaker(
|
||||||
|
bind=self.engine,
|
||||||
|
)
|
||||||
|
|
||||||
|
fd, self._path = tempfile.mkstemp(suffix=".yaml")
|
||||||
|
with os.fdopen(fd, "w") as fh:
|
||||||
|
fh.write(_VALID_YAML)
|
||||||
|
|
||||||
|
def teardown(self) -> None:
|
||||||
|
Path(self._path).unlink(missing_ok=True)
|
||||||
|
self.engine.dispose()
|
||||||
|
|
||||||
|
def time_add_persist(self) -> None:
|
||||||
|
"""Benchmark add with persistence."""
|
||||||
|
svc = _make_service(self.factory)
|
||||||
|
config = SkillConfigSchema.from_yaml_file(self._path)
|
||||||
|
svc.add_skill(config, config_path=self._path)
|
||||||
|
|
||||||
|
|
||||||
|
class SkillListAfterPersistSuite:
|
||||||
|
"""Benchmark list after persistent adds."""
|
||||||
|
|
||||||
|
timeout = 60.0
|
||||||
|
|
||||||
|
def setup(self) -> None:
|
||||||
|
self.engine = create_engine("sqlite:///:memory:", echo=False)
|
||||||
|
event.listen(self.engine, "connect", _enable_fk)
|
||||||
|
Base.metadata.create_all(self.engine)
|
||||||
|
self.factory: sessionmaker[Session] = sessionmaker(
|
||||||
|
bind=self.engine,
|
||||||
|
)
|
||||||
|
|
||||||
|
svc = _make_service(self.factory)
|
||||||
|
for i in range(20):
|
||||||
|
yaml_text = (
|
||||||
|
f"name: local/bench-list-{i}\n"
|
||||||
|
f'description: "Bench {i}"\n'
|
||||||
|
"tools:\n"
|
||||||
|
" - name: builtin/read_file\n"
|
||||||
|
)
|
||||||
|
fd, path = tempfile.mkstemp(suffix=".yaml")
|
||||||
|
with os.fdopen(fd, "w") as fh:
|
||||||
|
fh.write(yaml_text)
|
||||||
|
config = SkillConfigSchema.from_yaml_file(path)
|
||||||
|
svc.add_skill(config, config_path=path)
|
||||||
|
Path(path).unlink(missing_ok=True)
|
||||||
|
|
||||||
|
def teardown(self) -> None:
|
||||||
|
self.engine.dispose()
|
||||||
|
|
||||||
|
def time_list_persisted_skills(self) -> None:
|
||||||
|
"""Benchmark listing skills from a fresh service."""
|
||||||
|
svc = _make_service(self.factory)
|
||||||
|
svc.list_skills()
|
||||||
@@ -0,0 +1,46 @@
|
|||||||
|
@phase1 @domain @skill_add_persist
|
||||||
|
Feature: Skill add persists to database
|
||||||
|
As a developer
|
||||||
|
I want skills added via the CLI to persist across process invocations
|
||||||
|
So that skill list shows previously added skills
|
||||||
|
|
||||||
|
Background:
|
||||||
|
Given a fresh skill service backed by an in-memory database
|
||||||
|
|
||||||
|
# ───────────────────────────────────────────────────────
|
||||||
|
# Persistence round-trip
|
||||||
|
# ───────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
@skill_add_persist_roundtrip
|
||||||
|
Scenario: Skill add followed by skill list shows the added skill
|
||||||
|
Given a skill config YAML for "local/persist-test" at a temp path
|
||||||
|
When I add the skill via the persistent service
|
||||||
|
And I create a second service instance from the same database
|
||||||
|
And I list skills from the second service instance
|
||||||
|
Then the listed persistent skills should contain "local/persist-test"
|
||||||
|
|
||||||
|
@skill_add_persist_multiple
|
||||||
|
Scenario: Multiple skills added and listed
|
||||||
|
Given a skill config YAML for "local/alpha-skill" at a temp path
|
||||||
|
And a skill config YAML for "local/beta-skill" at a temp path
|
||||||
|
When I add all pending skill configs via the persistent service
|
||||||
|
And I create a second service instance from the same database
|
||||||
|
And I list skills from the second service instance
|
||||||
|
Then the listed persistent skills should contain "local/alpha-skill"
|
||||||
|
And the listed persistent skills should contain "local/beta-skill"
|
||||||
|
And the listed persistent skills count should be 2
|
||||||
|
|
||||||
|
@skill_add_persist_duplicate
|
||||||
|
Scenario: Skill add with duplicate name produces error
|
||||||
|
Given a skill config YAML for "local/dup-skill" at a temp path
|
||||||
|
When I add the skill via the persistent service
|
||||||
|
Then adding a duplicate "local/dup-skill" should raise ValueError
|
||||||
|
|
||||||
|
@skill_add_persist_remove
|
||||||
|
Scenario: Skill remove persists deletion to database
|
||||||
|
Given a skill config YAML for "local/remove-me" at a temp path
|
||||||
|
When I add the skill via the persistent service
|
||||||
|
And I remove the skill "local/remove-me" via the persistent service
|
||||||
|
And I create a second service instance from the same database
|
||||||
|
And I list skills from the second service instance
|
||||||
|
Then the listed persistent skills should not contain "local/remove-me"
|
||||||
@@ -0,0 +1,157 @@
|
|||||||
|
"""Step definitions for skill add persistence (issue #620).
|
||||||
|
|
||||||
|
Verifies that ``SkillService`` with a ``SkillRepository`` correctly
|
||||||
|
persists skills to the database so that a fresh service instance sees
|
||||||
|
the previously added skills.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import os
|
||||||
|
import tempfile
|
||||||
|
from typing import Any
|
||||||
|
|
||||||
|
from behave import given, then, when
|
||||||
|
from behave.runner import Context
|
||||||
|
from sqlalchemy import create_engine, event
|
||||||
|
from sqlalchemy.orm import Session, sessionmaker
|
||||||
|
|
||||||
|
from cleveragents.application.services.skill_service import SkillService
|
||||||
|
from cleveragents.infrastructure.database.models import Base
|
||||||
|
from cleveragents.infrastructure.database.repositories import (
|
||||||
|
SkillRepository,
|
||||||
|
)
|
||||||
|
from cleveragents.skills.schema import SkillConfigSchema
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Helpers
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
_SKILL_YAML_TEMPLATE = """\
|
||||||
|
name: {name}
|
||||||
|
description: "{name} test skill"
|
||||||
|
tools:
|
||||||
|
- name: builtin/read_file
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
|
def _enable_fk(dbapi_conn: Any, _rec: Any) -> None:
|
||||||
|
"""Enable SQLite foreign key enforcement."""
|
||||||
|
cursor = dbapi_conn.cursor()
|
||||||
|
cursor.execute("PRAGMA foreign_keys=ON")
|
||||||
|
cursor.close()
|
||||||
|
|
||||||
|
|
||||||
|
def _make_service(
|
||||||
|
factory: Any,
|
||||||
|
) -> SkillService:
|
||||||
|
"""Build a SkillService backed by the given session factory."""
|
||||||
|
repo = SkillRepository(session_factory=factory)
|
||||||
|
return SkillService(skill_repo=repo, session_factory=factory)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Background
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@given("a fresh skill service backed by an in-memory database")
|
||||||
|
def step_fresh_persistent_service(context: Context) -> None:
|
||||||
|
engine = create_engine("sqlite:///:memory:", echo=False)
|
||||||
|
event.listen(engine, "connect", _enable_fk)
|
||||||
|
Base.metadata.create_all(engine)
|
||||||
|
|
||||||
|
shared_session = sessionmaker(bind=engine)()
|
||||||
|
|
||||||
|
def _sf() -> Session:
|
||||||
|
return shared_session
|
||||||
|
|
||||||
|
context.sap_engine = engine
|
||||||
|
context.sap_session_factory = _sf
|
||||||
|
context.sap_service = _make_service(context.sap_session_factory)
|
||||||
|
context.sap_yaml_paths = []
|
||||||
|
context.sap_configs = []
|
||||||
|
context.sap_listed = []
|
||||||
|
context.sap_error = None
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Given steps
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@given('a skill config YAML for "{name}" at a temp path')
|
||||||
|
def step_skill_config_yaml(context: Context, name: str) -> None:
|
||||||
|
content = _SKILL_YAML_TEMPLATE.format(name=name)
|
||||||
|
fd, path = tempfile.mkstemp(suffix=".yaml")
|
||||||
|
with os.fdopen(fd, "w") as fh:
|
||||||
|
fh.write(content)
|
||||||
|
context.sap_yaml_paths.append(path)
|
||||||
|
schema = SkillConfigSchema.from_yaml_file(path)
|
||||||
|
context.sap_configs.append(schema)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# When steps
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@when("I add the skill via the persistent service")
|
||||||
|
def step_add_skill_persistent(context: Context) -> None:
|
||||||
|
config = context.sap_configs[-1]
|
||||||
|
path = context.sap_yaml_paths[-1]
|
||||||
|
context.sap_service.add_skill(config, config_path=path)
|
||||||
|
|
||||||
|
|
||||||
|
@when("I add all pending skill configs via the persistent service")
|
||||||
|
def step_add_all_skills_persistent(context: Context) -> None:
|
||||||
|
for cfg, path in zip(context.sap_configs, context.sap_yaml_paths, strict=True):
|
||||||
|
context.sap_service.add_skill(cfg, config_path=path)
|
||||||
|
|
||||||
|
|
||||||
|
@when("I create a second service instance from the same database")
|
||||||
|
def step_second_service(context: Context) -> None:
|
||||||
|
context.sap_service2 = _make_service(context.sap_session_factory)
|
||||||
|
|
||||||
|
|
||||||
|
@when("I list skills from the second service instance")
|
||||||
|
def step_list_from_second(context: Context) -> None:
|
||||||
|
context.sap_listed = context.sap_service2.list_skills()
|
||||||
|
|
||||||
|
|
||||||
|
@when('I remove the skill "{name}" via the persistent service')
|
||||||
|
def step_remove_skill_persistent(context: Context, name: str) -> None:
|
||||||
|
context.sap_service.remove_skill(name)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Then steps
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@then('the listed persistent skills should contain "{name}"')
|
||||||
|
def step_listed_contains(context: Context, name: str) -> None:
|
||||||
|
names = [s.name for s in context.sap_listed]
|
||||||
|
assert name in names, f"{name} not in {names}"
|
||||||
|
|
||||||
|
|
||||||
|
@then('the listed persistent skills should not contain "{name}"')
|
||||||
|
def step_listed_not_contains(context: Context, name: str) -> None:
|
||||||
|
names = [s.name for s in context.sap_listed]
|
||||||
|
assert name not in names, f"{name} unexpectedly in {names}"
|
||||||
|
|
||||||
|
|
||||||
|
@then("the listed persistent skills count should be {count:d}")
|
||||||
|
def step_listed_count(context: Context, count: int) -> None:
|
||||||
|
actual = len(context.sap_listed)
|
||||||
|
assert actual == count, f"Expected {count}, got {actual}"
|
||||||
|
|
||||||
|
|
||||||
|
@then('adding a duplicate "{name}" should raise ValueError')
|
||||||
|
def step_duplicate_raises(context: Context, name: str) -> None:
|
||||||
|
config = context.sap_configs[-1]
|
||||||
|
try:
|
||||||
|
context.sap_service.add_skill(config)
|
||||||
|
raise AssertionError("Expected ValueError for duplicate skill")
|
||||||
|
except ValueError as exc:
|
||||||
|
assert "already registered" in str(exc)
|
||||||
@@ -0,0 +1,140 @@
|
|||||||
|
"""Helper script for Robot Framework skill add persistence smoke tests.
|
||||||
|
|
||||||
|
Usage:
|
||||||
|
python helper_skill_add_persist.py <command>
|
||||||
|
|
||||||
|
Commands:
|
||||||
|
persist-round-trip Add a skill, recreate service, verify visible
|
||||||
|
persist-multiple Add two skills, recreate service, verify both
|
||||||
|
persist-remove Add skill, remove, recreate service, verify gone
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import sys
|
||||||
|
from pathlib import Path
|
||||||
|
from typing import Any
|
||||||
|
|
||||||
|
# Ensure src is on the path
|
||||||
|
src_dir = str(Path(__file__).resolve().parents[1] / "src")
|
||||||
|
if src_dir not in sys.path:
|
||||||
|
sys.path.insert(0, src_dir)
|
||||||
|
|
||||||
|
from sqlalchemy import create_engine, event # noqa: E402
|
||||||
|
from sqlalchemy.orm import Session, sessionmaker # noqa: E402
|
||||||
|
|
||||||
|
from cleveragents.application.services.skill_service import ( # noqa: E402
|
||||||
|
SkillService,
|
||||||
|
)
|
||||||
|
from cleveragents.infrastructure.database.models import Base # noqa: E402
|
||||||
|
from cleveragents.infrastructure.database.repositories import ( # noqa: E402
|
||||||
|
SkillRepository,
|
||||||
|
)
|
||||||
|
from cleveragents.skills.schema import SkillConfigSchema # noqa: E402
|
||||||
|
|
||||||
|
|
||||||
|
def _enable_fk(dbapi_conn: Any, _rec: Any) -> None:
|
||||||
|
cursor = dbapi_conn.cursor()
|
||||||
|
cursor.execute("PRAGMA foreign_keys=ON")
|
||||||
|
cursor.close()
|
||||||
|
|
||||||
|
|
||||||
|
def _setup() -> tuple[sessionmaker[Session], Any]:
|
||||||
|
engine = create_engine("sqlite:///:memory:", echo=False)
|
||||||
|
event.listen(engine, "connect", _enable_fk)
|
||||||
|
Base.metadata.create_all(engine)
|
||||||
|
factory: sessionmaker[Session] = sessionmaker(bind=engine)
|
||||||
|
return factory, engine
|
||||||
|
|
||||||
|
|
||||||
|
def _make_service(factory: Any) -> SkillService:
|
||||||
|
shared = factory()
|
||||||
|
|
||||||
|
def _sf() -> Session:
|
||||||
|
return shared
|
||||||
|
|
||||||
|
repo = SkillRepository(session_factory=_sf)
|
||||||
|
return SkillService(skill_repo=repo, session_factory=_sf)
|
||||||
|
|
||||||
|
|
||||||
|
def _make_config(name: str) -> SkillConfigSchema:
|
||||||
|
import os
|
||||||
|
import tempfile
|
||||||
|
|
||||||
|
yaml_text = (
|
||||||
|
f"name: {name}\n"
|
||||||
|
f'description: "{name} test"\n'
|
||||||
|
"tools:\n"
|
||||||
|
" - name: builtin/read_file\n"
|
||||||
|
)
|
||||||
|
fd, path = tempfile.mkstemp(suffix=".yaml")
|
||||||
|
with os.fdopen(fd, "w") as fh:
|
||||||
|
fh.write(yaml_text)
|
||||||
|
schema = SkillConfigSchema.from_yaml_file(path)
|
||||||
|
Path(path).unlink(missing_ok=True)
|
||||||
|
return schema
|
||||||
|
|
||||||
|
|
||||||
|
def cmd_persist_round_trip() -> None:
|
||||||
|
"""Add a skill, recreate service, verify visible."""
|
||||||
|
factory, _engine = _setup()
|
||||||
|
svc = _make_service(factory)
|
||||||
|
|
||||||
|
config = _make_config("local/robot-persist")
|
||||||
|
svc.add_skill(config, config_path="/tmp/robot.yaml")
|
||||||
|
|
||||||
|
# Create a fresh service from the same DB
|
||||||
|
svc2 = _make_service(factory)
|
||||||
|
skills = svc2.list_skills()
|
||||||
|
names = [s.name for s in skills]
|
||||||
|
assert "local/robot-persist" in names, f"Expected skill in {names}"
|
||||||
|
print("skill-add-persist-round-trip-ok")
|
||||||
|
|
||||||
|
|
||||||
|
def cmd_persist_multiple() -> None:
|
||||||
|
"""Add two skills, recreate service, verify both visible."""
|
||||||
|
factory, _engine = _setup()
|
||||||
|
svc = _make_service(factory)
|
||||||
|
|
||||||
|
svc.add_skill(_make_config("local/multi-a"), config_path="/tmp/a.yaml")
|
||||||
|
svc.add_skill(_make_config("local/multi-b"), config_path="/tmp/b.yaml")
|
||||||
|
|
||||||
|
svc2 = _make_service(factory)
|
||||||
|
skills = svc2.list_skills()
|
||||||
|
names = [s.name for s in skills]
|
||||||
|
assert "local/multi-a" in names, f"Expected multi-a in {names}"
|
||||||
|
assert "local/multi-b" in names, f"Expected multi-b in {names}"
|
||||||
|
assert len(names) == 2, f"Expected 2 skills, got {len(names)}"
|
||||||
|
print("skill-add-persist-multiple-ok")
|
||||||
|
|
||||||
|
|
||||||
|
def cmd_persist_remove() -> None:
|
||||||
|
"""Add skill, remove, recreate service, verify gone."""
|
||||||
|
factory, _engine = _setup()
|
||||||
|
svc = _make_service(factory)
|
||||||
|
|
||||||
|
svc.add_skill(_make_config("local/remove-me"), config_path="/tmp/rm.yaml")
|
||||||
|
svc.remove_skill("local/remove-me")
|
||||||
|
|
||||||
|
svc2 = _make_service(factory)
|
||||||
|
skills = svc2.list_skills()
|
||||||
|
names = [s.name for s in skills]
|
||||||
|
assert "local/remove-me" not in names, f"Skill should be gone but found in {names}"
|
||||||
|
print("skill-add-persist-remove-ok")
|
||||||
|
|
||||||
|
|
||||||
|
COMMANDS: dict[str, Any] = {
|
||||||
|
"persist-round-trip": cmd_persist_round_trip,
|
||||||
|
"persist-multiple": cmd_persist_multiple,
|
||||||
|
"persist-remove": cmd_persist_remove,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
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)
|
||||||
|
COMMANDS[sys.argv[1]]()
|
||||||
@@ -0,0 +1,33 @@
|
|||||||
|
*** Settings ***
|
||||||
|
Documentation Smoke tests for skill add persistence (issue #620)
|
||||||
|
Resource ${CURDIR}/common.resource
|
||||||
|
Suite Setup Setup Test Environment
|
||||||
|
Suite Teardown Cleanup Test Environment
|
||||||
|
|
||||||
|
*** Variables ***
|
||||||
|
${HELPER} ${CURDIR}/helper_skill_add_persist.py
|
||||||
|
|
||||||
|
*** Test Cases ***
|
||||||
|
Skill Add Persists Across Service Instances
|
||||||
|
[Documentation] Verify that a skill added via SkillService is visible from a fresh instance
|
||||||
|
${result}= Run Process ${PYTHON} ${HELPER} persist-round-trip cwd=${WORKSPACE}
|
||||||
|
Log ${result.stdout}
|
||||||
|
Log ${result.stderr}
|
||||||
|
Should Be Equal As Integers ${result.rc} 0
|
||||||
|
Should Contain ${result.stdout} skill-add-persist-round-trip-ok
|
||||||
|
|
||||||
|
Multiple Skills Persist
|
||||||
|
[Documentation] Verify that multiple added skills are all visible from a fresh instance
|
||||||
|
${result}= Run Process ${PYTHON} ${HELPER} persist-multiple cwd=${WORKSPACE}
|
||||||
|
Log ${result.stdout}
|
||||||
|
Log ${result.stderr}
|
||||||
|
Should Be Equal As Integers ${result.rc} 0
|
||||||
|
Should Contain ${result.stdout} skill-add-persist-multiple-ok
|
||||||
|
|
||||||
|
Skill Remove Persists Deletion
|
||||||
|
[Documentation] Verify that removing a skill is reflected in fresh service instances
|
||||||
|
${result}= Run Process ${PYTHON} ${HELPER} persist-remove cwd=${WORKSPACE}
|
||||||
|
Log ${result.stdout}
|
||||||
|
Log ${result.stderr}
|
||||||
|
Should Be Equal As Integers ${result.rc} 0
|
||||||
|
Should Contain ${result.stdout} skill-add-persist-remove-ok
|
||||||
@@ -1,10 +1,12 @@
|
|||||||
"""Skill management service for CleverAgents.
|
"""Skill management service for CleverAgents.
|
||||||
|
|
||||||
Provides in-memory skill registration, retrieval, listing, and removal.
|
Provides skill registration, retrieval, listing, and removal with
|
||||||
|
optional database persistence via ``SkillRepository``.
|
||||||
|
|
||||||
Follows the dual-mode storage pattern from ``PlanLifecycleService``:
|
Follows the dual-mode storage pattern from ``PlanLifecycleService``:
|
||||||
an in-memory dict is always maintained; when a ``UnitOfWork`` is wired
|
an in-memory dict is always maintained; when a ``SkillRepository`` is
|
||||||
(via ``C0.skill.registry`` — Luis), persistence calls are added alongside
|
provided, persistence calls are issued alongside the in-memory
|
||||||
the in-memory operations **without** changing the public API.
|
operations **without** changing the public API.
|
||||||
|
|
||||||
Based on implementation_plan.md task C0.skill.cli and specification.md
|
Based on implementation_plan.md task C0.skill.cli and specification.md
|
||||||
"Skill Registration and Management" section.
|
"Skill Registration and Management" section.
|
||||||
@@ -12,7 +14,9 @@ Based on implementation_plan.md task C0.skill.cli and specification.md
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import logging
|
||||||
from collections import OrderedDict
|
from collections import OrderedDict
|
||||||
|
from collections.abc import Callable
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
@@ -23,14 +27,16 @@ from cleveragents.domain.models.core.skill import (
|
|||||||
)
|
)
|
||||||
from cleveragents.skills.schema import SkillConfigSchema
|
from cleveragents.skills.schema import SkillConfigSchema
|
||||||
|
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
||||||
class SkillService:
|
class SkillService:
|
||||||
"""In-memory skill registration service.
|
"""Skill registration service with optional DB persistence.
|
||||||
|
|
||||||
Manages the lifecycle of skills: add, update, remove, list, show, and
|
Manages the lifecycle of skills: add, update, remove, list, show, and
|
||||||
resolve tools. When ``C0.skill.registry`` lands, a ``unit_of_work``
|
resolve tools. When a ``SkillRepository`` and ``session_factory`` are
|
||||||
parameter will be added to ``__init__`` and persistence calls inserted
|
provided at construction, every mutation is persisted to the database
|
||||||
alongside the in-memory dict — the public API stays the same.
|
and the in-memory cache is pre-loaded from existing rows.
|
||||||
|
|
||||||
Example::
|
Example::
|
||||||
|
|
||||||
@@ -40,14 +46,26 @@ class SkillService:
|
|||||||
svc.remove_skill("local/my-skill")
|
svc.remove_skill("local/my-skill")
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def __init__(self) -> None:
|
def __init__(
|
||||||
# In-memory storage (will be supplemented by DB when registry lands)
|
self,
|
||||||
|
skill_repo: Any | None = None,
|
||||||
|
session_factory: Callable[..., Any] | None = None,
|
||||||
|
) -> None:
|
||||||
|
# In-memory storage (primary data store for reads)
|
||||||
self._skills: OrderedDict[str, Skill] = OrderedDict()
|
self._skills: OrderedDict[str, Skill] = OrderedDict()
|
||||||
self._config_paths: dict[str, str] = {}
|
self._config_paths: dict[str, str] = {}
|
||||||
self._created_at: dict[str, datetime] = {}
|
self._created_at: dict[str, datetime] = {}
|
||||||
self._updated_at: dict[str, datetime] = {}
|
self._updated_at: dict[str, datetime] = {}
|
||||||
self._resolver = SkillResolver()
|
self._resolver = SkillResolver()
|
||||||
|
|
||||||
|
# Optional DB persistence layer
|
||||||
|
self._skill_repo = skill_repo
|
||||||
|
self._session_factory = session_factory
|
||||||
|
|
||||||
|
# Pre-load existing skills from DB when persistence is available
|
||||||
|
if self._skill_repo is not None:
|
||||||
|
self._load_from_db()
|
||||||
|
|
||||||
# ------------------------------------------------------------------
|
# ------------------------------------------------------------------
|
||||||
# Public API
|
# Public API
|
||||||
# ------------------------------------------------------------------
|
# ------------------------------------------------------------------
|
||||||
@@ -96,6 +114,9 @@ class SkillService:
|
|||||||
self._created_at[name] = now
|
self._created_at[name] = now
|
||||||
self._updated_at[name] = now
|
self._updated_at[name] = now
|
||||||
|
|
||||||
|
# Persist to database when repository is available
|
||||||
|
self._persist_skill(skill, is_new=is_new)
|
||||||
|
|
||||||
return skill, is_new
|
return skill, is_new
|
||||||
|
|
||||||
def remove_skill(self, name: str) -> Skill:
|
def remove_skill(self, name: str) -> Skill:
|
||||||
@@ -119,6 +140,10 @@ class SkillService:
|
|||||||
self._config_paths.pop(name, None)
|
self._config_paths.pop(name, None)
|
||||||
self._created_at.pop(name, None)
|
self._created_at.pop(name, None)
|
||||||
self._updated_at.pop(name, None)
|
self._updated_at.pop(name, None)
|
||||||
|
|
||||||
|
# Remove from database when repository is available
|
||||||
|
self._delete_skill_from_db(name)
|
||||||
|
|
||||||
return skill
|
return skill
|
||||||
|
|
||||||
def get_skill(self, name: str) -> Skill:
|
def get_skill(self, name: str) -> Skill:
|
||||||
@@ -243,6 +268,66 @@ class SkillService:
|
|||||||
"""Return the number of registered skills."""
|
"""Return the number of registered skills."""
|
||||||
return len(self._skills)
|
return len(self._skills)
|
||||||
|
|
||||||
|
# ------------------------------------------------------------------
|
||||||
|
# Database persistence helpers
|
||||||
|
# ------------------------------------------------------------------
|
||||||
|
|
||||||
|
def _load_from_db(self) -> None:
|
||||||
|
"""Pre-load all skills from the database into the in-memory cache."""
|
||||||
|
if self._skill_repo is None:
|
||||||
|
return
|
||||||
|
try:
|
||||||
|
db_skills: list[Skill] = self._skill_repo.list_all()
|
||||||
|
now = datetime.now()
|
||||||
|
for skill in db_skills:
|
||||||
|
self._skills[skill.name] = skill
|
||||||
|
self._created_at[skill.name] = now
|
||||||
|
self._updated_at[skill.name] = now
|
||||||
|
logger.debug("Loaded %d skills from database", len(db_skills))
|
||||||
|
except Exception:
|
||||||
|
logger.warning("Failed to load skills from database", exc_info=True)
|
||||||
|
|
||||||
|
def _persist_skill(self, skill: Skill, *, is_new: bool) -> None:
|
||||||
|
"""Persist a skill to the database (create or update)."""
|
||||||
|
if self._skill_repo is None:
|
||||||
|
return
|
||||||
|
try:
|
||||||
|
if is_new:
|
||||||
|
self._skill_repo.create(skill)
|
||||||
|
else:
|
||||||
|
self._skill_repo.update(skill)
|
||||||
|
self._commit()
|
||||||
|
except Exception:
|
||||||
|
logger.warning(
|
||||||
|
"Failed to persist skill %s to database",
|
||||||
|
skill.name,
|
||||||
|
exc_info=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
def _delete_skill_from_db(self, name: str) -> None:
|
||||||
|
"""Delete a skill from the database."""
|
||||||
|
if self._skill_repo is None:
|
||||||
|
return
|
||||||
|
try:
|
||||||
|
self._skill_repo.delete(name)
|
||||||
|
self._commit()
|
||||||
|
except Exception:
|
||||||
|
logger.warning(
|
||||||
|
"Failed to delete skill %s from database",
|
||||||
|
name,
|
||||||
|
exc_info=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
def _commit(self) -> None:
|
||||||
|
"""Commit the current database transaction."""
|
||||||
|
if self._session_factory is None:
|
||||||
|
return
|
||||||
|
try:
|
||||||
|
session = self._session_factory()
|
||||||
|
session.commit()
|
||||||
|
except Exception:
|
||||||
|
logger.warning("Failed to commit session", exc_info=True)
|
||||||
|
|
||||||
# ------------------------------------------------------------------
|
# ------------------------------------------------------------------
|
||||||
# Internal helpers
|
# Internal helpers
|
||||||
# ------------------------------------------------------------------
|
# ------------------------------------------------------------------
|
||||||
|
|||||||
@@ -62,25 +62,61 @@ console = Console()
|
|||||||
_FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich)"
|
_FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich)"
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Module-level service (in-memory; replaced by DI container when registry
|
# Module-level service. A module-level singleton keeps state across CLI
|
||||||
# lands). A module-level singleton keeps state across CLI invocations in the
|
# invocations in the same process (e.g. tests). Production usage will use
|
||||||
# same process (e.g. tests). Production usage will use the DI container.
|
# the DI container.
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
_service: SkillService | None = None
|
_service: SkillService | None = None
|
||||||
|
|
||||||
|
|
||||||
def _get_skill_service() -> SkillService:
|
def _get_skill_service() -> SkillService:
|
||||||
"""Get or create the module-level SkillService instance."""
|
"""Get or create the module-level SkillService with DB persistence.
|
||||||
|
|
||||||
|
Follows the same pattern as ``_get_tool_registry_service`` in
|
||||||
|
``tool.py``: obtains the database URL from the DI container,
|
||||||
|
creates an engine/session factory/repository, and injects them
|
||||||
|
into the ``SkillService`` so that every mutation is persisted.
|
||||||
|
"""
|
||||||
global _service
|
global _service
|
||||||
if _service is None:
|
if _service is None:
|
||||||
_service = SkillService()
|
try:
|
||||||
|
from sqlalchemy import create_engine
|
||||||
|
from sqlalchemy.orm import sessionmaker
|
||||||
|
|
||||||
|
from cleveragents.application.container import get_container
|
||||||
|
from cleveragents.infrastructure.database.repositories import (
|
||||||
|
SkillRepository,
|
||||||
|
)
|
||||||
|
|
||||||
|
container = get_container()
|
||||||
|
database_url: str = container.database_url()
|
||||||
|
|
||||||
|
engine = create_engine(database_url, echo=False)
|
||||||
|
factory = sessionmaker(bind=engine, expire_on_commit=False)
|
||||||
|
skill_repo = SkillRepository(session_factory=factory)
|
||||||
|
_service = SkillService(
|
||||||
|
skill_repo=skill_repo,
|
||||||
|
session_factory=factory,
|
||||||
|
)
|
||||||
|
except Exception:
|
||||||
|
# Fallback to pure in-memory if DB is unavailable
|
||||||
|
_service = SkillService()
|
||||||
return _service
|
return _service
|
||||||
|
|
||||||
|
|
||||||
def _reset_skill_service() -> None:
|
def _reset_skill_service(
|
||||||
"""Reset the service (used by tests)."""
|
service: SkillService | None = None,
|
||||||
|
) -> None:
|
||||||
|
"""Reset the service (used by tests).
|
||||||
|
|
||||||
|
Args:
|
||||||
|
service: Optional ``SkillService`` instance to install.
|
||||||
|
When ``None``, the next ``_get_skill_service`` call will
|
||||||
|
create a fresh in-memory service (no DB persistence) to
|
||||||
|
avoid side-effects during unit testing.
|
||||||
|
"""
|
||||||
global _service
|
global _service
|
||||||
_service = None
|
_service = service if service is not None else SkillService()
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user