feat(skill-catalog-subscription): synced catalog + HTTP sync + MCP tools
Consumes the external Subscription Platform as source of truth for skill content; reuses skills_learning domain models (extended with KnowledgeSkill) and workflow.skill_exec resolver. HTTP/MCP deps land in api/ per CONSTITUTION.md; synced skills use a physically separate SQLite file (tasks/skills.sqlite3) to preserve the skill-authoring capability boundary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -0,0 +1,332 @@
|
||||
"""Tests for api/skill_catalog_mcp.py.
|
||||
|
||||
Covers task 4.5: list/search/get via MCP-style handler calls, resolve_flow_template
|
||||
success and failure cases, semantic error translation, and an explicit
|
||||
assertion that no batch-execute tool is registered (design D5 / task 4.4).
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from api.skill_catalog_mcp import (
|
||||
SKILL_TOOL_NAMES,
|
||||
InvalidFlowTemplateError,
|
||||
MissingParameterError,
|
||||
SkillCatalogError,
|
||||
SkillNotFoundError,
|
||||
register_skill_catalog_tools,
|
||||
skill_tool_handlers,
|
||||
)
|
||||
from skills_learning.models import (
|
||||
FlowStep,
|
||||
FlowTemplateSkill,
|
||||
KnowledgeSkill,
|
||||
SkillMetadata,
|
||||
)
|
||||
from storage.skill_catalog import SkillCatalogStore
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# Fixtures and helpers
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
|
||||
def _knowledge(skill_id: str, name: str, content: str, tags=None) -> KnowledgeSkill:
|
||||
return KnowledgeSkill(
|
||||
metadata=SkillMetadata(
|
||||
id=skill_id,
|
||||
name=name,
|
||||
kind="knowledge",
|
||||
tags=tags or [],
|
||||
source="subscription",
|
||||
),
|
||||
content=content,
|
||||
)
|
||||
|
||||
|
||||
def _flow(
|
||||
skill_id: str,
|
||||
name: str,
|
||||
*,
|
||||
steps: list[tuple[str, dict]],
|
||||
parameters: dict | None = None,
|
||||
tags=None,
|
||||
) -> FlowTemplateSkill:
|
||||
return FlowTemplateSkill(
|
||||
metadata=SkillMetadata(
|
||||
id=skill_id,
|
||||
name=name,
|
||||
kind="flow_template",
|
||||
tags=tags or [],
|
||||
source="subscription",
|
||||
),
|
||||
steps=[FlowStep(tool_name=t, args=a) for t, a in steps],
|
||||
parameters=parameters or {},
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def store(tmp_path):
|
||||
return SkillCatalogStore(db_path=tmp_path / "skills.sqlite3")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def seeded_store(store):
|
||||
store._register_subscription("sub-a")
|
||||
store._apply_sync_replace_all(
|
||||
"sub-a",
|
||||
[
|
||||
_knowledge("k1", "Knowledge One", "body of knowledge", tags=["docs"]),
|
||||
_flow(
|
||||
"f1",
|
||||
"Search Flow",
|
||||
steps=[
|
||||
("tap", {"x": "{x_coord}"}),
|
||||
("input_text", {"text": "{query}"}),
|
||||
],
|
||||
parameters={
|
||||
"x_coord": {"type": "number", "required": True},
|
||||
"query": {"type": "string", "required": True},
|
||||
},
|
||||
tags=["search"],
|
||||
),
|
||||
],
|
||||
)
|
||||
return store
|
||||
|
||||
|
||||
def _handlers(store, *, registered_tools=None, active=None):
|
||||
return skill_tool_handlers(
|
||||
store=store,
|
||||
get_active_subscriptions=lambda: set(active if active is not None else {"sub-a"}),
|
||||
get_registered_tools=(lambda: set(registered_tools or {"tap", "input_text"})),
|
||||
)
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# list_skills / search_skills / get_skill via handlers
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_list_skills_returns_summaries_without_platform_fields(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["list_skills"]()
|
||||
assert result["ok"] is True
|
||||
assert len(result["skills"]) == 2
|
||||
# Summaries contain only id/name/description/kind/tags — no source/version
|
||||
serialized = repr(result)
|
||||
assert "subscription" not in serialized.lower()
|
||||
assert "parent_version_id" not in serialized
|
||||
assert "originating_goal" not in serialized
|
||||
names = {s["name"] for s in result["skills"]}
|
||||
assert names == {"Knowledge One", "Search Flow"}
|
||||
|
||||
|
||||
def test_list_skills_empty_when_no_active_subscriptions(seeded_store):
|
||||
handlers = _handlers(seeded_store, active=set())
|
||||
result = handlers["list_skills"]()
|
||||
assert result == {"ok": True, "skills": []}
|
||||
|
||||
|
||||
def test_search_skills_finds_by_tag_and_name(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
by_name = handlers["search_skills"](query="Search")
|
||||
assert {s["id"] for s in by_name["skills"]} == {"f1"}
|
||||
|
||||
by_tag = handlers["search_skills"](query="docs")
|
||||
assert {s["id"] for s in by_tag["skills"]} == {"k1"}
|
||||
|
||||
|
||||
def test_get_skill_returns_full_knowledge_content(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["get_skill"](skill_id="k1")
|
||||
assert result["ok"] is True
|
||||
skill = result["skill"]
|
||||
assert skill["kind"] == "knowledge"
|
||||
assert skill["content"] == "body of knowledge"
|
||||
assert skill["name"] == "Knowledge One"
|
||||
|
||||
|
||||
def test_get_skill_returns_full_flow_template(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["get_skill"](skill_id="f1")
|
||||
assert result["ok"] is True
|
||||
skill = result["skill"]
|
||||
assert skill["kind"] == "flow_template"
|
||||
assert [s["tool_name"] for s in skill["steps"]] == ["tap", "input_text"]
|
||||
assert "x_coord" in skill["parameters"]
|
||||
|
||||
|
||||
def test_get_skill_unknown_returns_skill_not_found(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["get_skill"](skill_id="does-not-exist")
|
||||
assert result["ok"] is False
|
||||
assert result["error"] == "skill not found"
|
||||
|
||||
|
||||
def test_get_skill_not_visible_indistinguishable_from_not_found(seeded_store):
|
||||
"""Existence must not leak: not-visible returns the same error as not-found."""
|
||||
handlers = _handlers(seeded_store, active=set()) # no active subs
|
||||
invisible = handlers["get_skill"](skill_id="k1")
|
||||
unknown = handlers["get_skill"](skill_id="never-existed")
|
||||
assert invisible == unknown == {"ok": False, "error": "skill not found"}
|
||||
|
||||
|
||||
def test_get_skill_with_dangling_tool_reference_returns_unavailable(seeded_store):
|
||||
# f1 references tap + input_text; drop input_text from the registered set
|
||||
handlers = _handlers(seeded_store, registered_tools={"tap"})
|
||||
result = handlers["get_skill"](skill_id="f1")
|
||||
assert result["ok"] is False
|
||||
# Per task 2.6, get_skill returns None for dangling refs, which surfaces
|
||||
# as "skill not found" at the MCP layer (no existence leak on the cause).
|
||||
assert result["error"] == "skill not found"
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# resolve_flow_template
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_resolve_flow_template_succeeds_with_valid_params(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["resolve_flow_template"](
|
||||
skill_id="f1",
|
||||
params={"x_coord": 100, "query": "hello"},
|
||||
)
|
||||
assert result["ok"] is True
|
||||
steps = result["steps"]
|
||||
assert steps[0]["tool_name"] == "tap"
|
||||
assert steps[0]["args"]["x"] == 100 # {x_coord} substituted
|
||||
assert steps[1]["tool_name"] == "input_text"
|
||||
assert steps[1]["args"]["text"] == "hello"
|
||||
|
||||
|
||||
def test_resolve_flow_template_missing_required_param(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["resolve_flow_template"](
|
||||
skill_id="f1",
|
||||
params={"x_coord": 100}, # missing 'query'
|
||||
)
|
||||
assert result["ok"] is False
|
||||
assert result["error"].startswith("missing parameter:")
|
||||
assert "query" in result["error"]
|
||||
|
||||
|
||||
def test_resolve_flow_template_unknown_skill(seeded_store):
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["resolve_flow_template"](
|
||||
skill_id="no-such",
|
||||
params={},
|
||||
)
|
||||
assert result["ok"] is False
|
||||
assert result["error"] == "skill not found"
|
||||
|
||||
|
||||
def test_resolve_flow_template_on_knowledge_skill_returns_unavailable(seeded_store):
|
||||
"""Cannot resolve a knowledge skill as a flow template."""
|
||||
handlers = _handlers(seeded_store)
|
||||
result = handlers["resolve_flow_template"](skill_id="k1", params={})
|
||||
assert result["ok"] is False
|
||||
assert result["error"] == "skill unavailable"
|
||||
|
||||
|
||||
def test_resolve_flow_template_with_dangling_tool_reference(seeded_store):
|
||||
"""A skill whose steps reference an unregistered tool is treated as
|
||||
not-found at resolve time (consistent with get_skill's behavior)."""
|
||||
handlers = _handlers(seeded_store, registered_tools={"tap"}) # missing input_text
|
||||
result = handlers["resolve_flow_template"](
|
||||
skill_id="f1",
|
||||
params={"x_coord": 1, "query": "x"},
|
||||
)
|
||||
assert result["ok"] is False
|
||||
assert result["error"] == "skill not found"
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# Registration: tools registered on FastMCP server, no batch-execute tool
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
|
||||
def _fastmcp_tool_names(server) -> set[str]:
|
||||
"""Read the FastMCP tool registry directly (sync, no event loop needed)."""
|
||||
# FastMCP stores tools in ToolManager._tools (mcp>=1.27). Walk the attrs
|
||||
# defensively so the test doesn't break on minor internal refactors.
|
||||
if hasattr(server, "_tool_manager"):
|
||||
manager = server._tool_manager
|
||||
if hasattr(manager, "_tools"):
|
||||
return set(manager._tools.keys())
|
||||
raise AssertionError(
|
||||
"Could not introspect FastMCP tool registry — internal API changed"
|
||||
)
|
||||
|
||||
|
||||
def test_register_skill_catalog_tools_registers_expected_names(seeded_store):
|
||||
from mcp.server.fastmcp import FastMCP
|
||||
|
||||
server = FastMCP("test")
|
||||
register_skill_catalog_tools(
|
||||
server,
|
||||
store=seeded_store,
|
||||
get_active_subscriptions=lambda: {"sub-a"},
|
||||
get_registered_tools=lambda: {"tap", "input_text"},
|
||||
)
|
||||
names = _fastmcp_tool_names(server)
|
||||
for expected in SKILL_TOOL_NAMES:
|
||||
assert expected in names, f"missing tool: {expected}"
|
||||
|
||||
|
||||
def test_no_batch_execute_tool_is_registered(seeded_store):
|
||||
"""Task 4.4: assert no run_skill_flow / execute / batch tool is registered."""
|
||||
from mcp.server.fastmcp import FastMCP
|
||||
|
||||
server = FastMCP("test")
|
||||
register_skill_catalog_tools(
|
||||
server,
|
||||
store=seeded_store,
|
||||
get_active_subscriptions=lambda: {"sub-a"},
|
||||
get_registered_tools=lambda: {"tap", "input_text"},
|
||||
)
|
||||
names = _fastmcp_tool_names(server)
|
||||
forbidden_fragments = ("run_", "execute", "batch", "invoke_skill")
|
||||
for name in names:
|
||||
lower = name.lower()
|
||||
for fragment in forbidden_fragments:
|
||||
assert fragment not in lower, (
|
||||
f"Forbidden tool name '{name}' contains '{fragment}' — "
|
||||
"design D5 forbids server-side flow execution tools"
|
||||
)
|
||||
|
||||
|
||||
def test_create_mcp_server_registers_skill_tools_when_store_provided(seeded_store):
|
||||
"""Wire-up: api.mcp.create_mcp_server must register skill tools when
|
||||
skill_catalog_store is provided."""
|
||||
from api.mcp import create_mcp_server
|
||||
|
||||
server = create_mcp_server(
|
||||
skill_catalog_store=seeded_store,
|
||||
skill_active_subscriptions={"sub-a"},
|
||||
)
|
||||
names = _fastmcp_tool_names(server)
|
||||
assert "list_skills" in names
|
||||
assert "tap" in names # existing device tools still present
|
||||
|
||||
|
||||
def test_create_mcp_server_omits_skill_tools_when_no_store():
|
||||
"""Wire-up must not break existing behavior when no store is provided."""
|
||||
from api.mcp import create_mcp_server
|
||||
|
||||
server = create_mcp_server()
|
||||
names = _fastmcp_tool_names(server)
|
||||
assert "list_skills" not in names
|
||||
assert "tap" in names # existing device tools present
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------
|
||||
# Error class hierarchy
|
||||
# ----------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_skill_error_hierarchy():
|
||||
assert issubclass(SkillNotFoundError, SkillCatalogError)
|
||||
assert issubclass(InvalidFlowTemplateError, SkillCatalogError)
|
||||
assert issubclass(MissingParameterError, SkillCatalogError)
|
||||
Reference in New Issue
Block a user