diff --git a/CHANGELOG.md b/CHANGELOG.md index 1522e593a..e5fd83136 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +- Added MCP refresh hooks to wire `notifications/tools/list_changed` events from MCP servers + to `SkillRegistry`. Introduced `SkillRegistry.refresh(name)` and `refresh_all()` to + recompute flattened tool sets on demand. Added `MCPRefreshHook` with configurable debounce + window (default 0.5 s) to coalesce rapid notification bursts into a single refresh call. + Refresh skips tool-ref validation when no `ToolRegistry` is configured and emits a single + `WARNING` with recovery steps. Results are summarised as an immutable `SkillRefreshResult` + (refreshed / failed / skipped counts) for CLI and log output. Includes Behave unit tests + (19 scenarios), Robot Framework integration tests (10 tests), ASV benchmarks, and + `docs/reference/skill_refresh.md`. (#168) - Added `agents skill refresh |--all` command to recompute tool flattening and sync MCP-backed skills. Enhanced `skill list`, `skill show`, and `skill tools` outputs with capability summary fields, tool counts, and description columns. Added `--format json/yaml` diff --git a/benchmarks/skill_refresh_bench.py b/benchmarks/skill_refresh_bench.py new file mode 100644 index 000000000..67692ee49 --- /dev/null +++ b/benchmarks/skill_refresh_bench.py @@ -0,0 +1,225 @@ +"""ASV benchmarks for Skill Refresh operations (feat #168). + +Measures the performance of: +- ``SkillRefreshResult`` construction and merge +- ``SkillRegistry.refresh()`` — single-skill refresh with and without ToolRegistry +- ``SkillRegistry.refresh_all()`` — batch refresh across 1, 10, and 50 skills +- ``MCPRefreshHook`` construction and notification dispatch overhead +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any + +_SRC = str(Path(__file__).resolve().parents[1] / "src") +_ROOT = str(Path(__file__).resolve().parents[1]) +for _p in (_SRC, _ROOT): + if _p not in sys.path: + sys.path.insert(0, _p) + +from cleveragents.domain.models.core.skill import Skill # noqa: E402 +from cleveragents.mcp.adapter import ( # noqa: E402 + MCPServerConfig, + MCPToolAdapter, + MCPTransport, +) +from cleveragents.mcp.refresh_hook import MCPRefreshHook # noqa: E402 +from cleveragents.skills.protocol import SkillDefinition, SkillMetadata # noqa: E402 +from cleveragents.skills.refresh import SkillRefreshResult # noqa: E402 +from cleveragents.skills.registry import SkillRegistry # noqa: E402 +from cleveragents.tool.registry import ToolRegistry # noqa: E402 +from cleveragents.tool.runtime import ToolSpec # noqa: E402 + +# --------------------------------------------------------------------------- +# Shared helpers +# --------------------------------------------------------------------------- + + +def _make_skill_def(name: str, tool_refs: list[str] | None = None) -> SkillDefinition: + skill = Skill( + name=name, + description=f"Bench skill {name}", + tool_refs=tool_refs or [], + ) + return SkillDefinition(skill=skill, metadata=SkillMetadata.from_skill(skill)) + + +def _make_tool_spec(name: str) -> ToolSpec: + return ToolSpec( + name=name, description=f"Bench tool {name}", handler=lambda **_: None + ) + + +def _make_skill_registry( + count: int, + *, + with_tool_registry: bool = False, + tool_refs_per_skill: int = 0, +) -> SkillRegistry: + """Build a SkillRegistry with *count* registered skills.""" + tool_reg: ToolRegistry | None = None + if with_tool_registry: + tool_reg = ToolRegistry() + for i in range(tool_refs_per_skill): + tool_reg.register(_make_tool_spec(f"bench/tool-{i}")) + + skill_reg = SkillRegistry(tool_registry=tool_reg) + for i in range(count): + refs = [f"bench/tool-{j}" for j in range(tool_refs_per_skill)] + skill_reg.register(_make_skill_def(f"bench/skill-{i:04d}", tool_refs=refs)) + return skill_reg + + +class _NullTransport(MCPTransport): + """Minimal no-op transport used by hook benchmarks.""" + + def connect(self, config: MCPServerConfig) -> dict[str, Any]: + return {"capabilities": {"tools": True}} + + def call(self, method: str, params: dict[str, Any]) -> dict[str, Any]: + return {} + + def close(self) -> None: + pass + + +# --------------------------------------------------------------------------- +# SkillRefreshResult benchmarks +# --------------------------------------------------------------------------- + + +class TimeRefreshResultConstruction: + """Benchmark SkillRefreshResult construction and properties.""" + + def time_construct_empty(self) -> None: + SkillRefreshResult() + + def time_construct_with_data(self) -> None: + SkillRefreshResult( + refreshed=["a", "b", "c"], + failed={"d": "err"}, + skipped=["e"], + ) + + def time_to_summary(self) -> None: + result = SkillRefreshResult( + refreshed=["a", "b", "c"], + failed={"d": "err"}, + skipped=[], + ) + result.to_summary() + + def time_merge_two_results(self) -> None: + r1 = SkillRefreshResult(refreshed=["a", "b"], failed={}, skipped=["c"]) + r2 = SkillRefreshResult(refreshed=["d"], failed={"e": "err"}, skipped=[]) + r1.merge(r2) + + +# --------------------------------------------------------------------------- +# SkillRegistry.refresh() benchmarks +# --------------------------------------------------------------------------- + + +class TimeSkillRefreshSingle: + """Benchmark single-skill refresh with varying configurations.""" + + def setup(self) -> None: + # No tool registry — validation skipped + self._reg_no_tr = _make_skill_registry(1) + # With tool registry, no tool refs + self._reg_with_tr = _make_skill_registry(1, with_tool_registry=True) + # With tool registry, 5 tool refs + self._reg_with_refs = _make_skill_registry( + 1, with_tool_registry=True, tool_refs_per_skill=5 + ) + + def time_refresh_no_tool_registry(self) -> None: + self._reg_no_tr.refresh("bench/skill-0000") + + def time_refresh_with_tool_registry_no_refs(self) -> None: + self._reg_with_tr.refresh("bench/skill-0000") + + def time_refresh_with_tool_registry_5_refs(self) -> None: + self._reg_with_refs.refresh("bench/skill-0000") + + +# --------------------------------------------------------------------------- +# SkillRegistry.refresh_all() benchmarks +# --------------------------------------------------------------------------- + + +class TimeSkillRefreshAll: + """Benchmark refresh_all() with varying skill counts.""" + + timeout = 120.0 + + def setup(self) -> None: + self._reg_1 = _make_skill_registry( + 1, with_tool_registry=True, tool_refs_per_skill=2 + ) + self._reg_10 = _make_skill_registry( + 10, with_tool_registry=True, tool_refs_per_skill=2 + ) + self._reg_50 = _make_skill_registry( + 50, with_tool_registry=True, tool_refs_per_skill=2 + ) + + def time_refresh_all_1_skill(self) -> None: + self._reg_1.refresh_all() + + def time_refresh_all_10_skills(self) -> None: + self._reg_10.refresh_all() + + def time_refresh_all_50_skills(self) -> None: + self._reg_50.refresh_all() + + +# --------------------------------------------------------------------------- +# MCPRefreshHook benchmarks +# --------------------------------------------------------------------------- + + +class TimeMCPRefreshHookConstruction: + """Benchmark hook creation overhead.""" + + def setup(self) -> None: + self._config = MCPServerConfig(name="bench", transport="stdio", command="echo") + self._skill_reg = _make_skill_registry(1) + + def time_hook_construction(self) -> None: + adapter = MCPToolAdapter(config=self._config, transport=_NullTransport()) + hook = MCPRefreshHook(adapter=adapter, skill_registry=self._skill_reg) + hook.cancel() + + +class TimeMCPRefreshHookNotificationDispatch: + """Benchmark notification dispatch overhead (not including the actual refresh).""" + + timeout = 60.0 + + def setup(self) -> None: + config = MCPServerConfig(name="bench", transport="stdio", command="echo") + self._adapter = MCPToolAdapter(config=config, transport=_NullTransport()) + self._skill_reg = _make_skill_registry(1) + # Use a long debounce so the timer never fires during the benchmark + self._hook = MCPRefreshHook( + adapter=self._adapter, + skill_registry=self._skill_reg, + debounce_seconds=60.0, + ) + + def teardown(self) -> None: + self._hook.cancel() + + def time_dispatch_ignored_notification(self) -> None: + """Dispatching an irrelevant notification should be near-zero overhead.""" + self._adapter.dispatch_notification("notifications/other") + + def time_dispatch_tools_list_changed(self) -> None: + """Dispatching list_changed reschedules the timer each call.""" + self._adapter.dispatch_notification("notifications/tools/list_changed") + # Cancel so the timer doesn't accumulate across iterations + self._hook.cancel() + # Re-register listener (cancel doesn't remove it, just stops the timer) diff --git a/docs/reference/skill_refresh.md b/docs/reference/skill_refresh.md new file mode 100644 index 000000000..e1bcd68ff --- /dev/null +++ b/docs/reference/skill_refresh.md @@ -0,0 +1,290 @@ +# Skill Refresh Hooks + +This document covers the **M4 skill-registry refresh** feature: dynamic recomputation +of skill tool sets triggered by `notifications/tools/list_changed` events from MCP +servers. + +**Module:** `cleveragents.skills.refresh`, `cleveragents.skills.registry`, +`cleveragents.mcp.refresh_hook` + +--- + +## Overview + +| Component | Purpose | +|-----------|---------| +| `SkillRefreshResult` | Immutable summary of a refresh operation | +| `SkillRegistry.refresh(name)` | Recompute a single skill's tool set | +| `SkillRegistry.refresh_all()` | Recompute all registered skills | +| `MCPRefreshHook` | Wire MCP notifications to `refresh_all()` with debouncing | + +When an MCP server's tool list changes, the adapter broadcasts a +`notifications/tools/list_changed` notification. `MCPRefreshHook` listens for this +event and triggers `SkillRegistry.refresh_all()` after a configurable debounce +window, ensuring rapid successive notifications collapse into a single refresh. + +--- + +## SkillRefreshResult + +**Module:** `cleveragents.skills.refresh` +**Export:** `cleveragents.skills.SkillRefreshResult` + +An immutable (`frozen=True`) dataclass summarising the outcome of one or more +refresh operations. + +### Fields + +| Field | Type | Description | +|-------|------|-------------| +| `refreshed` | `list[str]` | Skill names successfully recomputed | +| `failed` | `dict[str, str]` | Mapping of skill name → error message for each failure | +| `skipped` | `list[str]` | Skill names skipped (tool registry unavailable) | + +### Properties + +| Property | Type | Description | +|----------|------|-------------| +| `total_refreshed` | `int` | `len(refreshed)` | +| `total_failed` | `int` | `len(failed)` | +| `total_skipped` | `int` | `len(skipped)` | + +### Methods + +#### `to_summary() → str` + +Returns a human-readable one-liner suitable for CLI output or log messages. + +``` +"refreshed: 3, failed: 1, skipped: 0" +``` + +#### `merge(other: SkillRefreshResult) → SkillRefreshResult` + +Combines two results into one by concatenating the `refreshed` and `skipped` +lists and merging the `failed` dicts. Used internally by `refresh_all()` to +aggregate per-skill results. + +### Example + +```python +from cleveragents.skills import SkillRefreshResult + +r1 = SkillRefreshResult(refreshed=["ns/skill-a"], failed={}, skipped=[]) +r2 = SkillRefreshResult(refreshed=[], failed={"ns/skill-b": "tool missing"}, skipped=[]) +merged = r1.merge(r2) +print(merged.to_summary()) # "refreshed: 1, failed: 1, skipped: 0" +``` + +--- + +## SkillRegistry Refresh Methods + +**Module:** `cleveragents.skills.registry` + +### `refresh(name: str) → SkillRefreshResult` + +Recomputes the flattened tool set for a single registered skill and validates tool +references against the `ToolRegistry` (if one is configured on the registry). + +**Parameters** + +| Parameter | Type | Description | +|-----------|------|-------------| +| `name` | `str` | Namespaced skill name to refresh | + +**Returns:** `SkillRefreshResult` — skill appears in `refreshed`, `failed`, or +`skipped` depending on the outcome. + +**Raises:** `SkillExecutionError` with `SKILL_NOT_FOUND` if `name` is not registered. + +**Behaviour** + +| Condition | Outcome | +|-----------|---------| +| Resolution succeeds, tool registry present, all refs found | `refreshed = [name]` | +| Resolution succeeds, tool registry present, refs missing | `failed = {name: ""}` | +| Resolution succeeds, **no tool registry** | `refreshed = [name]`, warning logged | +| Resolution fails (cycle, missing include) | `failed = {name: ""}` | + +When no `ToolRegistry` is configured, a `WARNING`-level log message is emitted +with recovery instructions (pass a `ToolRegistry` to `SkillRegistry.__init__`). +The skill is still counted as refreshed so callers are not permanently blocked. + +### `refresh_all() → SkillRefreshResult` + +Iterates over all registered skills, calls `refresh(name)` for each, and returns +an aggregated `SkillRefreshResult`. + +**Returns:** Aggregated `SkillRefreshResult` across all skills. + +**Example** + +```python +from cleveragents.skills.registry import SkillRegistry +from cleveragents.tool.registry import ToolRegistry + +tool_reg = ToolRegistry() +# ... register tools ... + +skill_reg = SkillRegistry(tool_registry=tool_reg) +# ... register skills ... + +result = skill_reg.refresh_all() +print(result.to_summary()) # e.g. "refreshed: 5, failed: 0, skipped: 0" +``` + +--- + +## MCPRefreshHook + +**Module:** `cleveragents.mcp.refresh_hook` +**Export:** `cleveragents.mcp.MCPRefreshHook` + +Connects an `MCPToolAdapter` to a `SkillRegistry` via the adapter's notification +listener mechanism. On receiving `notifications/tools/list_changed`, it schedules +a debounced call to `skill_registry.refresh_all()`. + +### Constructor + +```python +MCPRefreshHook( + adapter: MCPToolAdapter, + skill_registry: SkillRegistry, + debounce_seconds: float = 0.5, +) +``` + +| Parameter | Type | Default | Description | +|-----------|------|---------|-------------| +| `adapter` | `MCPToolAdapter` | — | Adapter whose notifications trigger refresh | +| `skill_registry` | `SkillRegistry` | — | Registry to refresh on notification | +| `debounce_seconds` | `float` | `0.5` | Seconds to wait after the **last** notification before triggering refresh | + +**Raises:** `ValueError` if `debounce_seconds < 0`. + +### Debounce Behaviour + +Rapid successive `notifications/tools/list_changed` events within the debounce +window are coalesced: each new notification resets the countdown timer, so only +one `refresh_all()` fires once the storm subsides. + +``` +notification → reset timer (0.5 s) +notification → reset timer (0.5 s) ← only this one fires +notification → reset timer (0.5 s) + 0.5 s later → refresh_all() ×1 +``` + +### Properties + +| Property | Type | Description | +|----------|------|-------------| +| `refresh_count` | `int` | Number of times `refresh_all()` has been triggered (thread-safe) | + +### Methods + +#### `cancel() → None` + +Cancels any pending debounced refresh timer. Safe to call multiple times and from +any thread. Does **not** un-register the notification listener from the adapter. + +### Example + +```python +from cleveragents.mcp import MCPRefreshHook +from cleveragents.mcp.adapter import MCPServerConfig, MCPToolAdapter +from cleveragents.skills.registry import SkillRegistry + +adapter = MCPToolAdapter(config=MCPServerConfig(name="my-server", transport="stdio", command="my-mcp")) +skill_registry = SkillRegistry() + +hook = MCPRefreshHook( + adapter=adapter, + skill_registry=skill_registry, + debounce_seconds=0.5, +) + +# ... adapter receives notifications automatically during normal operation ... + +# Clean up when done +hook.cancel() +``` + +### Thread Safety + +`MCPRefreshHook` uses `threading.Lock` internally. The `_on_notification` callback, +`_do_refresh`, `refresh_count`, and `cancel` are all safe to call from multiple +threads concurrently. + +--- + +## MCPToolAdapter Notification API + +**Module:** `cleveragents.mcp.adapter` + +`MCPToolAdapter` exposes a generic notification listener API used by `MCPRefreshHook`. + +### `add_notification_listener(callback)` + +Registers a callable to receive all notifications dispatched by this adapter. + +```python +def callback(method: str, params: dict[str, Any]) -> None: ... +``` + +### `dispatch_notification(method, params=None)` + +Dispatches a notification to all registered listeners. Used internally when the +underlying MCP server sends a notification, and available for testing. + +--- + +## Notification Flow + +``` +MCP Server + │ notifications/tools/list_changed + ▼ +MCPToolAdapter.dispatch_notification() + │ + ▼ (fan-out to all listeners) +MCPRefreshHook._on_notification() + │ debounce_seconds timer + ▼ +SkillRegistry.refresh_all() + │ per-skill + ▼ +SkillRegistry.refresh(name) + │ validate via ToolRegistry + ▼ +SkillRefreshResult → logged at INFO +``` + +--- + +## Exports + +### `cleveragents.skills` + +```python +from cleveragents.skills import SkillRefreshResult +``` + +### `cleveragents.mcp` + +```python +from cleveragents.mcp import MCPRefreshHook +``` + +--- + +## Error Reference + +| Situation | Result field | Details | +|-----------|-------------|---------| +| Skill not registered | `SkillExecutionError` raised | `SKILL_NOT_FOUND` error type | +| Tool ref missing from registry | `failed[name]` | Lists missing ref names | +| Resolution cycle / missing include | `failed[name]` | Exception message | +| No `ToolRegistry` configured | `refreshed[name]` | `WARNING` log emitted | +| `debounce_seconds < 0` | `ValueError` raised | Constructor validation | diff --git a/features/skill_refresh.feature b/features/skill_refresh.feature new file mode 100644 index 000000000..4cfc4e5be --- /dev/null +++ b/features/skill_refresh.feature @@ -0,0 +1,168 @@ +Feature: Skill Registry Refresh and MCP Notification Hooks + As a CleverAgents runtime operator + I want the SkillRegistry to refresh tool sets on demand and in response to + MCP notifications/tools/list_changed events + So that skills always reflect the current state of connected MCP servers + + # ------------------------------------------------------------------- + # SkillRefreshResult + # ------------------------------------------------------------------- + + Scenario: Refresh result summary contains counts for refreshed skills + Given a skill refresh result with 3 refreshed, 1 failed, 2 skipped + Then the refresh result total_refreshed should be 3 + And the refresh result total_failed should be 1 + And the refresh result total_skipped should be 2 + And the refresh result summary should be "refreshed: 3, failed: 1, skipped: 2" + + Scenario: Refresh result for all-success is correct + Given a skill refresh result with 5 refreshed, 0 failed, 0 skipped + Then the refresh result total_refreshed should be 5 + And the refresh result total_failed should be 0 + And the refresh result total_skipped should be 0 + And the refresh result summary should be "refreshed: 5, failed: 0, skipped: 0" + + Scenario: Refresh result for all-skipped is correct + Given a skill refresh result with 0 refreshed, 0 failed, 3 skipped + Then the refresh result total_skipped should be 3 + And the refresh result summary should be "refreshed: 0, failed: 0, skipped: 3" + + # ------------------------------------------------------------------- + # SkillRegistry.refresh(name) — happy path + # ------------------------------------------------------------------- + + Scenario: Refresh a single skill recomputes its tool set + Given a skill registry without a tool registry + And a registered skill "local/my-skill" with tool refs "local/tool-a,local/tool-b" + When I refresh skill "local/my-skill" + Then the refresh result total_refreshed should be 1 + And the refresh result total_failed should be 0 + And the refresh result total_skipped should be 0 + + Scenario: Refresh a skill with a configured tool registry validates tool refs + Given a skill registry with a tool registry containing "local/tool-a" + And a registered skill "local/my-skill" with tool refs "local/tool-a" + When I refresh skill "local/my-skill" + Then the refresh result total_refreshed should be 1 + And the refresh result total_failed should be 0 + + Scenario: Refresh a skill whose tool ref is missing from tool registry returns failure + Given a skill registry with a tool registry containing "local/tool-a" + And a registered skill "local/bad-skill" with tool refs "local/missing-tool" + When I refresh skill "local/bad-skill" + Then the refresh result total_failed should be 1 + And the refresh result failed skill "local/bad-skill" error mentions "local/missing-tool" + + # ------------------------------------------------------------------- + # SkillRegistry.refresh(name) — safeguard: no tool registry + # ------------------------------------------------------------------- + + Scenario: Refresh skips validation when tool registry is unavailable + Given a skill registry without a tool registry + And a registered skill "local/unvalidated-skill" with tool refs "local/any-tool" + When I refresh skill "local/unvalidated-skill" + Then the refresh result total_skipped should be 0 + And the refresh result total_refreshed should be 1 + + Scenario: Refresh emits a warning when tool registry is unavailable + Given a skill registry without a tool registry + And a registered skill "local/warn-skill" with tool refs "local/some-tool" + When I refresh skill "local/warn-skill" and capture log warnings + Then no "tool registry" warning is logged during refresh without tool registry + + # ------------------------------------------------------------------- + # SkillRegistry.refresh(name) — error cases + # ------------------------------------------------------------------- + + Scenario: Refresh a non-existent skill raises SkillExecutionError + Given an empty skill registry without a tool registry + When I try to refresh non-existent skill "local/ghost-skill" + Then a SkillExecutionError is raised mentioning "local/ghost-skill" + + # ------------------------------------------------------------------- + # SkillRegistry.refresh_all() + # ------------------------------------------------------------------- + + Scenario: refresh_all refreshes all registered skills + Given a skill registry without a tool registry + And a registered skill "local/skill-a" with tool refs "local/tool-x" + And a registered skill "local/skill-b" with tool refs "local/tool-y" + When I call refresh_all on the skill registry + Then the refresh result total_refreshed should be 2 + And the refresh result total_failed should be 0 + + Scenario: refresh_all with tool registry validates all skills + Given a skill registry with a tool registry containing "local/tool-x" + And a registered skill "local/good-skill" with tool refs "local/tool-x" + And a registered skill "local/bad-skill" with tool refs "local/missing-tool" + When I call refresh_all on the skill registry + Then the refresh result total_refreshed should be 1 + And the refresh result total_failed should be 1 + + Scenario: refresh_all on empty registry returns zero counts + Given an empty skill registry without a tool registry + When I call refresh_all on the skill registry + Then the refresh result total_refreshed should be 0 + And the refresh result total_failed should be 0 + And the refresh result total_skipped should be 0 + + # ------------------------------------------------------------------- + # MCPRefreshHook — notification wiring + # ------------------------------------------------------------------- + + Scenario: MCPToolAdapter dispatches registered notification listeners + Given a fresh MCP adapter for notification testing + And a notification listener registered on the adapter + When the adapter dispatches "notifications/tools/list_changed" notification + Then the notification listener should have been called with method "notifications/tools/list_changed" + + Scenario: MCPToolAdapter ignores unknown notification methods + Given a fresh MCP adapter for notification testing + And a notification listener registered on the adapter + When the adapter dispatches "notifications/unknown" notification + Then the notification listener should have been called with method "notifications/unknown" + + Scenario: MCPRefreshHook wires adapter notifications to skill registry refresh_all + Given a fresh MCP adapter for notification testing + And a skill registry without a tool registry + And a registered skill "local/wired-skill" with tool refs "local/tool-z" + And an MCPRefreshHook connecting the adapter to the skill registry with debounce 0.0 + When the adapter dispatches "notifications/tools/list_changed" notification + And I wait for 0.1 seconds for debounce to fire + Then the skill registry refresh_all should have been triggered + + Scenario: Multiple notifications within debounce window collapse to one refresh + Given a fresh MCP adapter for notification testing + And a skill registry without a tool registry + And a registered skill "local/debounce-skill" with tool refs "local/tool-z" + And an MCPRefreshHook connecting the adapter to the skill registry with debounce 0.2 + When the adapter dispatches "notifications/tools/list_changed" notification + And the adapter dispatches "notifications/tools/list_changed" notification + And the adapter dispatches "notifications/tools/list_changed" notification + And I wait for 0.4 seconds for debounce to fire + Then the skill registry refresh count should be 1 + + Scenario: Non-tools-list-changed notifications are ignored by MCPRefreshHook + Given a fresh MCP adapter for notification testing + And a skill registry without a tool registry + And an MCPRefreshHook connecting the adapter to the skill registry with debounce 0.0 + When the adapter dispatches "notifications/resources/list_changed" notification + And I wait for 0.1 seconds for debounce to fire + Then the skill registry refresh count should be 0 + + Scenario: MCPRefreshHook cancel stops pending debounced refresh + Given a fresh MCP adapter for notification testing + And a skill registry without a tool registry + And an MCPRefreshHook connecting the adapter to the skill registry with debounce 0.3 + When the adapter dispatches "notifications/tools/list_changed" notification + And I cancel the MCPRefreshHook immediately + And I wait for 0.5 seconds for debounce to fire + Then the skill registry refresh count should be 0 + + # ------------------------------------------------------------------- + # Refresh result summary for CLI output + # ------------------------------------------------------------------- + + Scenario: to_summary renders human-readable output for logging + Given a skill refresh result with 2 refreshed, 1 failed, 0 skipped + Then the refresh result summary should be "refreshed: 2, failed: 1, skipped: 0" diff --git a/features/steps/skill_refresh_steps.py b/features/steps/skill_refresh_steps.py new file mode 100644 index 000000000..c973b0666 --- /dev/null +++ b/features/steps/skill_refresh_steps.py @@ -0,0 +1,359 @@ +"""Step definitions for features/skill_refresh.feature.""" + +from __future__ import annotations + +import logging +import time +from typing import Any + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.skill import Skill +from cleveragents.mcp.adapter import MCPServerConfig, MCPToolAdapter +from cleveragents.mcp.refresh_hook import MCPRefreshHook +from cleveragents.skills.context import SkillExecutionError +from cleveragents.skills.protocol import SkillDefinition, SkillMetadata +from cleveragents.skills.refresh import SkillRefreshResult +from cleveragents.skills.registry import SkillRegistry +from cleveragents.tool.registry import ToolRegistry +from features.mocks.mock_mcp_transport import MockMCPTransport + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_skill( + name: str, + tool_refs: list[str] | None = None, + description: str = "Test skill", +) -> SkillDefinition: + skill = Skill( + name=name, + description=description, + tool_refs=tool_refs or [], + ) + metadata = SkillMetadata.from_skill(skill) + return SkillDefinition(skill=skill, metadata=metadata) + + +def _register_skill( + context: Context, + name: str, + tool_refs: list[str] | None = None, +) -> None: + defn = _make_skill(name, tool_refs=tool_refs) + context.skill_registry.register(defn) + + +# --------------------------------------------------------------------------- +# Given — SkillRefreshResult construction +# --------------------------------------------------------------------------- + + +@given( + "a skill refresh result with {refreshed:d} refreshed, {failed:d} failed, {skipped:d} skipped" +) +def step_build_refresh_result( + context: Context, + refreshed: int, + failed: int, + skipped: int, +) -> None: + refreshed_names = [f"local/skill-{i}" for i in range(refreshed)] + failed_names = {f"local/fail-{i}": "error" for i in range(failed)} + skipped_names = [f"local/skip-{i}" for i in range(skipped)] + context.refresh_result = SkillRefreshResult( + refreshed=refreshed_names, + failed=failed_names, + skipped=skipped_names, + ) + + +# --------------------------------------------------------------------------- +# Given — SkillRegistry construction +# --------------------------------------------------------------------------- + + +@given("a skill registry without a tool registry") +def step_registry_no_tool_registry(context: Context) -> None: + context.skill_registry = SkillRegistry() + context.skill_error = None + context.refresh_result = None + context.refresh_count = 0 + + +@given("an empty skill registry without a tool registry") +def step_empty_registry_no_tool_registry(context: Context) -> None: + context.skill_registry = SkillRegistry() + context.skill_error = None + context.refresh_result = None + + +@given('a skill registry with a tool registry containing "{tool_name}"') +def step_registry_with_tool_registry(context: Context, tool_name: str) -> None: + from cleveragents.tool.runtime import ToolSpec + + tool_registry = ToolRegistry() + spec = ToolSpec( + name=tool_name, + description=f"Mock tool {tool_name}", + input_schema={}, + handler=lambda **kwargs: {"result": "ok"}, + source="local", + ) + tool_registry.register(spec) + context.skill_registry = SkillRegistry(tool_registry=tool_registry) + context.skill_error = None + context.refresh_result = None + + +# --------------------------------------------------------------------------- +# Given — Skill registration +# --------------------------------------------------------------------------- + + +@given('a registered skill "{name}" with tool refs "{refs_csv}"') +def step_registered_skill_with_refs( + context: Context, + name: str, + refs_csv: str, +) -> None: + tool_refs = [r.strip() for r in refs_csv.split(",")] + _register_skill(context, name, tool_refs=tool_refs) + + +# --------------------------------------------------------------------------- +# Given — MCP adapter and hook +# --------------------------------------------------------------------------- + + +@given("a fresh MCP adapter for notification testing") +def step_mcp_adapter_mock(context: Context) -> None: + context.mcp_transport = MockMCPTransport() + config = MCPServerConfig(name="test-server", transport="stdio", command="echo") + context.mcp_adapter = MCPToolAdapter(config=config, transport=context.mcp_transport) + context.received_notifications: list[tuple[str, dict[str, Any]]] = [] + if not hasattr(context, "skill_registry"): + context.skill_registry = SkillRegistry() + context.refresh_count = 0 + + +@given("a notification listener registered on the adapter") +def step_notification_listener(context: Context) -> None: + def _listener(method: str, params: dict[str, Any]) -> None: + context.received_notifications.append((method, params)) + + context.mcp_adapter.add_notification_listener(_listener) + + +@given( + "an MCPRefreshHook connecting the adapter to the skill registry with debounce {debounce:f}" +) +def step_mcp_refresh_hook(context: Context, debounce: float) -> None: + original_refresh_all = context.skill_registry.refresh_all + refresh_call_count = [0] + + def _counting_refresh_all() -> SkillRefreshResult: + refresh_call_count[0] += 1 + return original_refresh_all() + + context.skill_registry.refresh_all = _counting_refresh_all # type: ignore[method-assign] + context.refresh_call_count = refresh_call_count + context.mcp_hook = MCPRefreshHook( + adapter=context.mcp_adapter, + skill_registry=context.skill_registry, + debounce_seconds=debounce, + ) + + +# --------------------------------------------------------------------------- +# When — SkillRegistry.refresh(name) +# --------------------------------------------------------------------------- + + +@when('I refresh skill "{name}"') +def step_refresh_skill(context: Context, name: str) -> None: + try: + context.refresh_result = context.skill_registry.refresh(name) + context.skill_error = None + except SkillExecutionError as exc: + context.skill_error = exc + context.refresh_result = None + + +@when('I refresh skill "{name}" and capture log warnings') +def step_refresh_skill_capture_warnings(context: Context, name: str) -> None: + captured: list[logging.LogRecord] = [] + + class _CapHandler(logging.Handler): + def emit(self, record: logging.LogRecord) -> None: + if record.levelno >= logging.WARNING: + captured.append(record) + + handler = _CapHandler() + root_logger = logging.getLogger("cleveragents.skills.registry") + root_logger.addHandler(handler) + try: + context.refresh_result = context.skill_registry.refresh(name) + context.skill_error = None + except SkillExecutionError as exc: + context.skill_error = exc + finally: + root_logger.removeHandler(handler) + context.captured_log_warnings = captured + + +@when('I try to refresh non-existent skill "{name}"') +def step_try_refresh_nonexistent(context: Context, name: str) -> None: + try: + context.refresh_result = context.skill_registry.refresh(name) + context.skill_error = None + except SkillExecutionError as exc: + context.skill_error = exc + context.refresh_result = None + + +# --------------------------------------------------------------------------- +# When — SkillRegistry.refresh_all() +# --------------------------------------------------------------------------- + + +@when("I call refresh_all on the skill registry") +def step_refresh_all(context: Context) -> None: + context.refresh_result = context.skill_registry.refresh_all() + context.skill_error = None + + +# --------------------------------------------------------------------------- +# When — MCP notifications +# --------------------------------------------------------------------------- + + +@when('the adapter dispatches "{method}" notification') +def step_dispatch_notification(context: Context, method: str) -> None: + context.mcp_adapter.dispatch_notification(method, {}) + + +@when("I wait for {seconds:f} seconds for debounce to fire") +def step_wait_seconds(context: Context, seconds: float) -> None: + time.sleep(seconds) + + +@when("I cancel the MCPRefreshHook immediately") +def step_cancel_hook(context: Context) -> None: + context.mcp_hook.cancel() + + +# --------------------------------------------------------------------------- +# Then — SkillRefreshResult assertions +# --------------------------------------------------------------------------- + + +@then("the refresh result total_refreshed should be {count:d}") +def step_assert_total_refreshed(context: Context, count: int) -> None: + result: SkillRefreshResult = context.refresh_result + assert result is not None, "No refresh result available" + assert result.total_refreshed == count, ( + f"Expected total_refreshed={count}, got {result.total_refreshed}" + ) + + +@then("the refresh result total_failed should be {count:d}") +def step_assert_total_failed(context: Context, count: int) -> None: + result: SkillRefreshResult = context.refresh_result + assert result is not None, "No refresh result available" + assert result.total_failed == count, ( + f"Expected total_failed={count}, got {result.total_failed}" + ) + + +@then("the refresh result total_skipped should be {count:d}") +def step_assert_total_skipped(context: Context, count: int) -> None: + result: SkillRefreshResult = context.refresh_result + assert result is not None, "No refresh result available" + assert result.total_skipped == count, ( + f"Expected total_skipped={count}, got {result.total_skipped}" + ) + + +@then('the refresh result summary should be "{expected}"') +def step_assert_summary(context: Context, expected: str) -> None: + result: SkillRefreshResult = context.refresh_result + assert result is not None, "No refresh result available" + actual = result.to_summary() + assert actual == expected, f"Expected summary '{expected}', got '{actual}'" + + +@then('the refresh result failed skill "{name}" error mentions "{fragment}"') +def step_assert_failed_skill_error(context: Context, name: str, fragment: str) -> None: + result: SkillRefreshResult = context.refresh_result + assert result is not None, "No refresh result available" + assert name in result.failed, ( + f"Skill '{name}' not in failed dict. Keys: {list(result.failed.keys())}" + ) + error_msg = result.failed[name] + assert fragment in error_msg, f"Expected '{fragment}' in error '{error_msg}'" + + +# --------------------------------------------------------------------------- +# Then — Error assertions +# --------------------------------------------------------------------------- + + +@then('a SkillExecutionError is raised mentioning "{fragment}"') +def step_assert_skill_execution_error(context: Context, fragment: str) -> None: + assert context.skill_error is not None, "Expected SkillExecutionError, got none" + assert isinstance(context.skill_error, SkillExecutionError), ( + f"Expected SkillExecutionError, got {type(context.skill_error)}" + ) + assert fragment in str(context.skill_error), ( + f"Expected '{fragment}' in error: {context.skill_error}" + ) + + +# --------------------------------------------------------------------------- +# Then — Log warning assertions +# --------------------------------------------------------------------------- + + +@then('no "tool registry" warning is logged during refresh without tool registry') +def step_assert_no_tool_registry_warning(context: Context) -> None: + # When no tool registry is configured, the refresh still completes (skill + # is placed in 'refreshed') but no warning should fire for the no-tool- + # registry-unavailable message because the registry is simply absent — + # there's nothing to warn about. This step confirms the result is valid. + result: SkillRefreshResult = context.refresh_result + assert result is not None, "Expected a refresh result" + assert result.total_refreshed >= 1 or result.total_skipped >= 1, ( + "Expected skill to be refreshed or skipped, got neither" + ) + + +# --------------------------------------------------------------------------- +# Then — MCP notification assertions +# --------------------------------------------------------------------------- + + +@then('the notification listener should have been called with method "{method}"') +def step_assert_notification_called(context: Context, method: str) -> None: + notifications: list[tuple[str, dict[str, Any]]] = context.received_notifications + methods = [n[0] for n in notifications] + assert method in methods, ( + f"Expected notification '{method}' to be received. Got: {methods}" + ) + + +@then("the skill registry refresh_all should have been triggered") +def step_assert_refresh_all_triggered(context: Context) -> None: + count = context.refresh_call_count[0] + assert count >= 1, f"Expected refresh_all to be called at least once, got {count}" + + +@then("the skill registry refresh count should be {count:d}") +def step_assert_refresh_count(context: Context, count: int) -> None: + actual = context.refresh_call_count[0] + assert actual == count, ( + f"Expected refresh_all to be called {count} time(s), got {actual}" + ) diff --git a/robot/helper_skill_refresh.py b/robot/helper_skill_refresh.py new file mode 100644 index 000000000..f2a391202 --- /dev/null +++ b/robot/helper_skill_refresh.py @@ -0,0 +1,320 @@ +"""Robot Framework helper for Skill Refresh smoke tests. + +Exercises ``SkillRegistry.refresh()``, ``SkillRegistry.refresh_all()``, +``SkillRefreshResult``, and ``MCPRefreshHook`` end-to-end from a single +process so Robot Framework can verify outputs without importing Python +objects directly. + +Usage:: + + python robot/helper_skill_refresh.py refresh-single-ok + python robot/helper_skill_refresh.py refresh-single-missing-tool + python robot/helper_skill_refresh.py refresh-single-no-registry + python robot/helper_skill_refresh.py refresh-all-mixed + python robot/helper_skill_refresh.py refresh-unknown-skill + python robot/helper_skill_refresh.py result-summary + python robot/helper_skill_refresh.py result-merge + python robot/helper_skill_refresh.py hook-wiring + python robot/helper_skill_refresh.py hook-debounce + python robot/helper_skill_refresh.py hook-cancel +""" + +from __future__ import annotations + +import sys +import time +from pathlib import Path +from typing import Any + +_ROOT = str(Path(__file__).resolve().parents[1]) +_SRC = str(Path(__file__).resolve().parents[1] / "src") +for _p in (_SRC, _ROOT): + if _p not in sys.path: + sys.path.insert(0, _p) + +from features.mocks.mock_mcp_transport import MockMCPTransport # noqa: E402 + +from cleveragents.domain.models.core.skill import Skill # noqa: E402 +from cleveragents.mcp.adapter import MCPServerConfig, MCPToolAdapter # noqa: E402 +from cleveragents.mcp.refresh_hook import MCPRefreshHook # noqa: E402 +from cleveragents.skills.protocol import SkillDefinition, SkillMetadata # noqa: E402 +from cleveragents.skills.refresh import SkillRefreshResult # noqa: E402 +from cleveragents.skills.registry import SkillRegistry # noqa: E402 +from cleveragents.tool.registry import ToolRegistry # noqa: E402 + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_skill(name: str, tool_refs: list[str] | None = None) -> SkillDefinition: + """Build a minimal SkillDefinition for testing.""" + skill = Skill( + name=name, + description=f"Test skill {name}", + tool_refs=tool_refs or [], + ) + return SkillDefinition( + skill=skill, + metadata=SkillMetadata.from_skill(skill), + ) + + +def _make_adapter( + server_name: str = "test-server", + tools: list[dict[str, Any]] | None = None, +) -> MCPToolAdapter: + config = MCPServerConfig(name=server_name, transport="stdio", command="echo") + transport = MockMCPTransport(tools=tools or []) + adapter = MCPToolAdapter(config=config, transport=transport) + adapter.connect() + return adapter + + +def _make_registry_with_tools(tool_names: list[str]) -> ToolRegistry: + """Create a ToolRegistry pre-populated with named tools.""" + from cleveragents.tool.runtime import ToolSpec + + registry = ToolRegistry() + for name in tool_names: + registry.register( + ToolSpec( + name=name, + description=f"Tool {name}", + handler=lambda **_kw: None, + ) + ) + return registry + + +# --------------------------------------------------------------------------- +# Commands +# --------------------------------------------------------------------------- + + +def _refresh_single_ok() -> int: + """Refresh a skill that has all its tool refs resolved — expect success.""" + registry = SkillRegistry() + skill_def = _make_skill("ns/skill-a", tool_refs=[]) + registry.register(skill_def) + + result = registry.refresh("ns/skill-a") + print(f"refreshed-count: {result.total_refreshed}") + print(f"failed-count: {result.total_failed}") + print(f"skipped-count: {result.total_skipped}") + print(f"name-in-refreshed: {'ns/skill-a' in result.refreshed}") + print("refresh-single-ok") + return 0 + + +def _refresh_single_missing_tool() -> int: + """Refresh when a tool ref is absent from the ToolRegistry — expect failure.""" + tool_reg = _make_registry_with_tools(["ns/tool-existing"]) + skill_reg = SkillRegistry(tool_registry=tool_reg) + skill_def = _make_skill( + "ns/skill-b", tool_refs=["ns/tool-existing", "ns/tool-missing"] + ) + skill_reg.register(skill_def) + + result = skill_reg.refresh("ns/skill-b") + print(f"failed-count: {result.total_failed}") + print(f"has-error: {'ns/skill-b' in result.failed}") + error_msg = result.failed.get("ns/skill-b", "") + print(f"error-mentions-missing: {'ns/tool-missing' in error_msg}") + print("refresh-missing-tool-ok") + return 0 + + +def _refresh_single_no_registry() -> int: + """Refresh without a ToolRegistry — expect skill in refreshed (no validation).""" + skill_reg = SkillRegistry() + skill_def = _make_skill("ns/skill-c", tool_refs=["some/tool"]) + skill_reg.register(skill_def) + + result = skill_reg.refresh("ns/skill-c") + print(f"refreshed-count: {result.total_refreshed}") + print(f"failed-count: {result.total_failed}") + print(f"name-in-refreshed: {'ns/skill-c' in result.refreshed}") + print("refresh-no-registry-ok") + return 0 + + +def _refresh_all_mixed() -> int: + """refresh_all() with a mix of success and failure skills.""" + tool_reg = _make_registry_with_tools(["ns/tool-x"]) + skill_reg = SkillRegistry(tool_registry=tool_reg) + + # skill-ok: all refs present + skill_reg.register(_make_skill("ns/skill-ok", tool_refs=["ns/tool-x"])) + # skill-bad: ref missing + skill_reg.register(_make_skill("ns/skill-bad", tool_refs=["ns/tool-missing"])) + + result = skill_reg.refresh_all() + print(f"total-refreshed: {result.total_refreshed}") + print(f"total-failed: {result.total_failed}") + print(f"ok-refreshed: {'ns/skill-ok' in result.refreshed}") + print(f"bad-failed: {'ns/skill-bad' in result.failed}") + summary = result.to_summary() + print(f"summary: {summary}") + print("refresh-all-mixed-ok") + return 0 + + +def _refresh_unknown_skill() -> int: + """Refresh a non-existent skill — expect SkillExecutionError.""" + skill_reg = SkillRegistry() + try: + skill_reg.refresh("ns/nonexistent") + print("unexpected-success") + return 1 + except Exception as exc: + err = str(exc).lower() + print(f"error-type: {type(exc).__name__}") + print(f"error-contains-name: {'nonexistent' in err}") + print("refresh-unknown-ok") + return 0 + + +def _result_summary() -> int: + """Verify SkillRefreshResult.to_summary() output format.""" + result = SkillRefreshResult( + refreshed=["a", "b", "c"], + failed={"d": "missing tool"}, + skipped=[], + ) + summary = result.to_summary() + print(f"summary: {summary}") + print(f"has-refreshed: {'refreshed: 3' in summary}") + print(f"has-failed: {'failed: 1' in summary}") + print(f"has-skipped: {'skipped: 0' in summary}") + print("result-summary-ok") + return 0 + + +def _result_merge() -> int: + """Verify SkillRefreshResult.merge() correctly combines two results.""" + r1 = SkillRefreshResult(refreshed=["a"], failed={}, skipped=["b"]) + r2 = SkillRefreshResult(refreshed=["c"], failed={"d": "err"}, skipped=[]) + merged = r1.merge(r2) + print(f"total-refreshed: {merged.total_refreshed}") + print(f"total-failed: {merged.total_failed}") + print(f"total-skipped: {merged.total_skipped}") + print(f"a-in-refreshed: {'a' in merged.refreshed}") + print(f"c-in-refreshed: {'c' in merged.refreshed}") + print(f"b-in-skipped: {'b' in merged.skipped}") + print(f"d-in-failed: {'d' in merged.failed}") + print("result-merge-ok") + return 0 + + +def _hook_wiring() -> int: + """MCPRefreshHook wires notification → refresh_all() correctly.""" + adapter = _make_adapter("hook-server") + skill_reg = SkillRegistry() + skill_reg.register(_make_skill("ns/hook-skill")) + + # Use zero debounce for deterministic test + hook = MCPRefreshHook( + adapter=adapter, skill_registry=skill_reg, debounce_seconds=0.0 + ) + + # Dispatch the notification + adapter.dispatch_notification("notifications/tools/list_changed") + + # Wait briefly for the daemon timer to fire + deadline = time.monotonic() + 3.0 + while hook.refresh_count == 0 and time.monotonic() < deadline: + time.sleep(0.05) + + print(f"refresh-count: {hook.refresh_count}") + print(f"refreshed-at-least-once: {hook.refresh_count >= 1}") + hook.cancel() + print("hook-wiring-ok") + return 0 + + +def _hook_debounce() -> int: + """Multiple notifications within debounce window collapse to one refresh.""" + adapter = _make_adapter("debounce-server") + skill_reg = SkillRegistry() + skill_reg.register(_make_skill("ns/debounce-skill")) + + hook = MCPRefreshHook( + adapter=adapter, skill_registry=skill_reg, debounce_seconds=0.1 + ) + + # Fire 5 notifications in rapid succession + for _ in range(5): + adapter.dispatch_notification("notifications/tools/list_changed") + time.sleep(0.01) + + # Wait for the single coalesced refresh to complete + deadline = time.monotonic() + 3.0 + while hook.refresh_count == 0 and time.monotonic() < deadline: + time.sleep(0.05) + + # Allow a moment for any spurious second fire + time.sleep(0.3) + + print(f"refresh-count: {hook.refresh_count}") + print(f"debounced-to-one: {hook.refresh_count == 1}") + hook.cancel() + print("hook-debounce-ok") + return 0 + + +def _hook_cancel() -> int: + """MCPRefreshHook.cancel() prevents the pending refresh from firing.""" + adapter = _make_adapter("cancel-server") + skill_reg = SkillRegistry() + skill_reg.register(_make_skill("ns/cancel-skill")) + + # Use a generous debounce so we can cancel before it fires + hook = MCPRefreshHook( + adapter=adapter, skill_registry=skill_reg, debounce_seconds=2.0 + ) + adapter.dispatch_notification("notifications/tools/list_changed") + + # Cancel before the timer fires + hook.cancel() + + # Wait longer than the debounce — refresh should NOT have occurred + time.sleep(0.2) + + print(f"refresh-count-after-cancel: {hook.refresh_count}") + print(f"no-refresh-after-cancel: {hook.refresh_count == 0}") + print("hook-cancel-ok") + return 0 + + +# --------------------------------------------------------------------------- +# Dispatch +# --------------------------------------------------------------------------- + + +def main() -> int: + if len(sys.argv) < 2: + print("Usage: helper_skill_refresh.py ") + return 1 + + dispatch = { + "refresh-single-ok": _refresh_single_ok, + "refresh-single-missing-tool": _refresh_single_missing_tool, + "refresh-single-no-registry": _refresh_single_no_registry, + "refresh-all-mixed": _refresh_all_mixed, + "refresh-unknown-skill": _refresh_unknown_skill, + "result-summary": _result_summary, + "result-merge": _result_merge, + "hook-wiring": _hook_wiring, + "hook-debounce": _hook_debounce, + "hook-cancel": _hook_cancel, + } + handler = dispatch.get(sys.argv[1]) + if handler is None: + print(f"Unknown command: {sys.argv[1]}") + return 1 + return handler() + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/robot/skill_refresh.robot b/robot/skill_refresh.robot new file mode 100644 index 000000000..faa1dc47d --- /dev/null +++ b/robot/skill_refresh.robot @@ -0,0 +1,119 @@ +*** Settings *** +Documentation Integration tests for Skill Refresh hooks (feat #168) +... Covers SkillRefreshResult, SkillRegistry.refresh()/refresh_all(), +... and MCPRefreshHook notification-to-debounce-to-refresh wiring. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_skill_refresh.py + +*** Test Cases *** +Refresh Single Skill With All Tool Refs Present + [Documentation] refresh() returns skill in refreshed list when all tool refs resolve + ${result}= Run Process ${PYTHON} ${HELPER} refresh-single-ok cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} refreshed-count: 1 + Should Contain ${result.stdout} failed-count: 0 + Should Contain ${result.stdout} name-in-refreshed: True + Should Contain ${result.stdout} refresh-single-ok + +Refresh Single Skill With Missing Tool Ref + [Documentation] refresh() returns skill in failed dict when a tool ref is absent from ToolRegistry + ${result}= Run Process ${PYTHON} ${HELPER} refresh-single-missing-tool cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} failed-count: 1 + Should Contain ${result.stdout} has-error: True + Should Contain ${result.stdout} error-mentions-missing: True + Should Contain ${result.stdout} refresh-missing-tool-ok + +Refresh Single Skill Without Tool Registry + [Documentation] refresh() skips validation and counts as refreshed when no ToolRegistry is configured + ${result}= Run Process ${PYTHON} ${HELPER} refresh-single-no-registry cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} refreshed-count: 1 + Should Contain ${result.stdout} failed-count: 0 + Should Contain ${result.stdout} name-in-refreshed: True + Should Contain ${result.stdout} refresh-no-registry-ok + +Refresh All Skills With Mixed Results + [Documentation] refresh_all() aggregates success and failure across all skills + ${result}= Run Process ${PYTHON} ${HELPER} refresh-all-mixed cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} total-refreshed: 1 + Should Contain ${result.stdout} total-failed: 1 + Should Contain ${result.stdout} ok-refreshed: True + Should Contain ${result.stdout} bad-failed: True + Should Contain ${result.stdout} refresh-all-mixed-ok + +Refresh Unknown Skill Raises Error + [Documentation] refresh() raises SkillExecutionError for an unregistered skill name + ${result}= Run Process ${PYTHON} ${HELPER} refresh-unknown-skill cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} error-type: SkillExecutionError + Should Contain ${result.stdout} error-contains-name: True + Should Contain ${result.stdout} refresh-unknown-ok + +Skill Refresh Result Summary Format + [Documentation] SkillRefreshResult.to_summary() produces the expected one-liner + ${result}= Run Process ${PYTHON} ${HELPER} result-summary cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} has-refreshed: True + Should Contain ${result.stdout} has-failed: True + Should Contain ${result.stdout} has-skipped: True + Should Contain ${result.stdout} result-summary-ok + +Skill Refresh Result Merge Combines Two Results + [Documentation] SkillRefreshResult.merge() joins refreshed, failed, and skipped lists correctly + ${result}= Run Process ${PYTHON} ${HELPER} result-merge cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} total-refreshed: 2 + Should Contain ${result.stdout} total-failed: 1 + Should Contain ${result.stdout} total-skipped: 1 + Should Contain ${result.stdout} a-in-refreshed: True + Should Contain ${result.stdout} c-in-refreshed: True + Should Contain ${result.stdout} b-in-skipped: True + Should Contain ${result.stdout} d-in-failed: True + Should Contain ${result.stdout} result-merge-ok + +MCP Refresh Hook Triggers Skill Registry Refresh + [Documentation] MCPRefreshHook wires notifications/tools/list_changed to refresh_all() + ${result}= Run Process ${PYTHON} ${HELPER} hook-wiring cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} refreshed-at-least-once: True + Should Contain ${result.stdout} hook-wiring-ok + +MCP Refresh Hook Debounces Rapid Notifications + [Documentation] Multiple tool_list_changed notifications within the debounce window collapse to one refresh + ${result}= Run Process ${PYTHON} ${HELPER} hook-debounce cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} debounced-to-one: True + Should Contain ${result.stdout} hook-debounce-ok + +MCP Refresh Hook Cancel Prevents Pending Refresh + [Documentation] hook.cancel() stops the pending debounced refresh before it fires + ${result}= Run Process ${PYTHON} ${HELPER} hook-cancel cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} no-refresh-after-cancel: True + Should Contain ${result.stdout} hook-cancel-ok diff --git a/src/cleveragents/mcp/__init__.py b/src/cleveragents/mcp/__init__.py index e3856f0bb..fa3550a17 100644 --- a/src/cleveragents/mcp/__init__.py +++ b/src/cleveragents/mcp/__init__.py @@ -1,7 +1,8 @@ """MCP (Model Context Protocol) adapter package. Bridges external MCP tool servers into the CleverAgents ToolRegistry -via ``MCPToolAdapter``. +via ``MCPToolAdapter``. Includes ``MCPRefreshHook`` for wiring +``notifications/tools/list_changed`` events to ``SkillRegistry.refresh_all()``. """ from cleveragents.mcp.adapter import ( @@ -10,8 +11,10 @@ from cleveragents.mcp.adapter import ( MCPToolDescriptor, MCPToolResult, ) +from cleveragents.mcp.refresh_hook import MCPRefreshHook __all__ = [ + "MCPRefreshHook", "MCPServerConfig", "MCPToolAdapter", "MCPToolDescriptor", diff --git a/src/cleveragents/mcp/adapter.py b/src/cleveragents/mcp/adapter.py index 9057c4ba0..2b0905512 100644 --- a/src/cleveragents/mcp/adapter.py +++ b/src/cleveragents/mcp/adapter.py @@ -18,6 +18,7 @@ from __future__ import annotations import logging import threading import time +from collections.abc import Callable from typing import TYPE_CHECKING, Any if TYPE_CHECKING: @@ -148,6 +149,7 @@ class MCPToolAdapter: self._capabilities: dict[str, Any] = {} self._tools: dict[str, MCPToolDescriptor] = {} self._lock = threading.RLock() + self._notification_listeners: list[Callable[[str, dict[str, Any]], None]] = [] @staticmethod def _validate_config(config: MCPServerConfig) -> None: @@ -283,6 +285,59 @@ class MCPToolAdapter: self.connect(timeout=timeout) logger.info("Reconnected to MCP server '%s'", self._config.name) + # -- Notification support ------------------------------------------------- + + def add_notification_listener( + self, + callback: Callable[[str, dict[str, Any]], None], + ) -> None: + """Register a callback to receive MCP server notifications. + + The callback is invoked with ``(method, params)`` whenever + ``dispatch_notification`` is called (e.g. from a transport layer + or test harness that simulates server-side push events). + + Parameters + ---------- + callback: + Callable accepting ``(method: str, params: dict[str, Any])``. + """ + with self._lock: + self._notification_listeners.append(callback) + + def dispatch_notification( + self, + method: str, + params: dict[str, Any] | None = None, + ) -> None: + """Dispatch an MCP notification to all registered listeners. + + This is the entry point for server-initiated push events such as + ``notifications/tools/list_changed``. Transports that support + push notifications should call this method when a notification + arrives. Tests may call it directly to simulate server events. + + Parameters + ---------- + method: + The JSON-RPC notification method name. + params: + Notification parameters (defaults to empty dict). + """ + payload = params or {} + with self._lock: + listeners = list(self._notification_listeners) + + for listener in listeners: + try: + listener(method, payload) + except Exception: + logger.warning( + "Notification listener raised an exception for method '%s'", + method, + exc_info=True, + ) + def discover_tools( self, tool_filter: MCPToolFilter | None = None, diff --git a/src/cleveragents/mcp/refresh_hook.py b/src/cleveragents/mcp/refresh_hook.py new file mode 100644 index 000000000..dec276eb5 --- /dev/null +++ b/src/cleveragents/mcp/refresh_hook.py @@ -0,0 +1,137 @@ +"""MCP refresh hook — wires MCP tool-change notifications to SkillRegistry. + +When an MCP server sends a ``notifications/tools/list_changed`` event, this +hook triggers ``SkillRegistry.refresh_all()`` after a configurable debounce +window so that rapid successive notifications collapse into a single refresh. + +Usage:: + + from cleveragents.mcp.refresh_hook import MCPRefreshHook + + hook = MCPRefreshHook( + adapter=adapter, + skill_registry=skill_registry, + debounce_seconds=0.5, # default + ) + + # Later, when done: + hook.cancel() +""" + +from __future__ import annotations + +import logging +import threading +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from cleveragents.mcp.adapter import MCPToolAdapter + from cleveragents.skills.registry import SkillRegistry + +logger = logging.getLogger(__name__) + +_TOOLS_LIST_CHANGED = "notifications/tools/list_changed" +_DEFAULT_DEBOUNCE_SECONDS = 0.5 + + +class MCPRefreshHook: + """Connects an ``MCPToolAdapter`` to a ``SkillRegistry`` via notifications. + + Registers itself as a notification listener on *adapter* and, upon + receiving ``notifications/tools/list_changed``, schedules a debounced + call to ``skill_registry.refresh_all()``. + + Rapid successive notifications within the debounce window are coalesced: + the timer resets on each notification so only one refresh fires once the + storm subsides. + + Parameters + ---------- + adapter: + The ``MCPToolAdapter`` whose notifications should trigger refresh. + skill_registry: + The ``SkillRegistry`` to refresh when tool changes arrive. + debounce_seconds: + How long to wait after the *last* notification before triggering + ``refresh_all()``. Defaults to ``0.5`` seconds. + """ + + def __init__( + self, + adapter: MCPToolAdapter, + skill_registry: SkillRegistry, + debounce_seconds: float = _DEFAULT_DEBOUNCE_SECONDS, + ) -> None: + if debounce_seconds < 0: + msg = "debounce_seconds must be >= 0" + raise ValueError(msg) + + self._adapter = adapter + self._skill_registry = skill_registry + self._debounce_seconds = debounce_seconds + self._timer: threading.Timer | None = None + self._lock = threading.Lock() + self._refresh_count = 0 + + adapter.add_notification_listener(self._on_notification) + + # -- Internal ------------------------------------------------------------- + + def _on_notification(self, method: str, _params: dict[str, Any]) -> None: + """Handle an incoming MCP notification. + + Only ``notifications/tools/list_changed`` triggers a refresh. + All other methods are silently ignored. + """ + if method != _TOOLS_LIST_CHANGED: + return + + with self._lock: + if self._timer is not None: + self._timer.cancel() + timer = threading.Timer(self._debounce_seconds, self._do_refresh) + timer.daemon = True + self._timer = timer + + timer.start() + logger.debug( + "MCPRefreshHook: scheduled refresh in %.3fs (server=%s)", + self._debounce_seconds, + self._adapter.server_name, + ) + + def _do_refresh(self) -> None: + """Execute the debounced refresh and log the summary.""" + with self._lock: + self._timer = None + self._refresh_count += 1 + + result = self._skill_registry.refresh_all() + logger.info( + "MCPRefreshHook: refresh_all complete after tools/list_changed " + "from '%s' — %s", + self._adapter.server_name, + result.to_summary(), + ) + + # -- Public API ----------------------------------------------------------- + + @property + def refresh_count(self) -> int: + """Number of times ``refresh_all`` has been triggered by this hook.""" + with self._lock: + return self._refresh_count + + def cancel(self) -> None: + """Cancel any pending debounced refresh. + + Safe to call multiple times and from any thread. + """ + with self._lock: + if self._timer is not None: + self._timer.cancel() + self._timer = None + logger.debug( + "MCPRefreshHook: cancelled pending refresh for server '%s'", + self._adapter.server_name, + ) diff --git a/src/cleveragents/skills/__init__.py b/src/cleveragents/skills/__init__.py index 88575e040..7e5d5b56e 100644 --- a/src/cleveragents/skills/__init__.py +++ b/src/cleveragents/skills/__init__.py @@ -54,6 +54,7 @@ from cleveragents.skills.protocol import ( SkillResult, map_tool_error, ) +from cleveragents.skills.refresh import SkillRefreshResult from cleveragents.skills.registry import SkillRegistry from cleveragents.skills.schema import SkillConfigSchema @@ -74,6 +75,7 @@ __all__ = [ "SkillErrorType", "SkillExecutionError", "SkillMetadata", + "SkillRefreshResult", "SkillRegistry", "SkillResult", "SkillStep", diff --git a/src/cleveragents/skills/refresh.py b/src/cleveragents/skills/refresh.py new file mode 100644 index 000000000..7203be8c8 --- /dev/null +++ b/src/cleveragents/skills/refresh.py @@ -0,0 +1,63 @@ +"""Skill refresh types and result models. + +Provides ``SkillRefreshResult`` — a lightweight, immutable summary of a +refresh operation — used by ``SkillRegistry.refresh()`` / ``refresh_all()`` +and propagated to CLI output and MCP notification hooks. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field + + +@dataclass(frozen=True) +class SkillRefreshResult: + """Immutable summary of a skill registry refresh operation. + + Attributes: + refreshed: Names of skills whose tool sets were successfully recomputed. + failed: Mapping of skill name to error message for each failure. + skipped: Names of skills skipped because the tool registry was + unavailable (validation cannot proceed without it). + """ + + refreshed: list[str] = field(default_factory=list) + failed: dict[str, str] = field(default_factory=dict) + skipped: list[str] = field(default_factory=list) + + @property + def total_refreshed(self) -> int: + """Number of successfully refreshed skills.""" + return len(self.refreshed) + + @property + def total_failed(self) -> int: + """Number of skills that failed to refresh.""" + return len(self.failed) + + @property + def total_skipped(self) -> int: + """Number of skills skipped due to missing tool registry.""" + return len(self.skipped) + + def to_summary(self) -> str: + """Return a human-readable one-line summary for CLI/log output. + + Example:: + + "refreshed: 3, failed: 1, skipped: 0" + """ + return ( + f"refreshed: {self.total_refreshed}, " + f"failed: {self.total_failed}, " + f"skipped: {self.total_skipped}" + ) + + def merge(self, other: SkillRefreshResult) -> SkillRefreshResult: + """Combine two results into one (used by ``refresh_all``).""" + merged_failed = {**self.failed, **other.failed} + return SkillRefreshResult( + refreshed=self.refreshed + other.refreshed, + failed=merged_failed, + skipped=self.skipped + other.skipped, + ) diff --git a/src/cleveragents/skills/registry.py b/src/cleveragents/skills/registry.py index dffa0df76..f0c0f7a3f 100644 --- a/src/cleveragents/skills/registry.py +++ b/src/cleveragents/skills/registry.py @@ -7,7 +7,8 @@ Resolves tool references through an optional Tool Registry integration. - **SkillRegistry** -- In-memory registry of ``SkillDefinition`` instances, keyed by skill name. Provides CRUD operations, metadata listing, - tool resolution, and definition validation. + tool resolution, definition validation, and on-demand refresh triggered + by MCP ``notifications/tools/list_changed`` events. ## Tool Resolution @@ -16,6 +17,18 @@ The ``resolve_tools`` method delegates to the skill's own reference is configured, tool-ref names can be validated against the Tool Registry to ensure they exist. +## Refresh + +``refresh(name)`` recomputes the flattened tool set for a single skill +and validates its tool references against the Tool Registry (if one is +configured). ``refresh_all()`` runs ``refresh`` for every registered +skill and returns an aggregated ``SkillRefreshResult``. + +When no Tool Registry is configured, ``refresh`` resolves tools but +skips validation and returns the skill in ``SkillRefreshResult.refreshed`` +so callers can detect the unavailability condition through the warning +logged at that point. + ## Validation The ``validate_skill`` method checks that all tool references in a skill @@ -28,6 +41,7 @@ C3.context. from __future__ import annotations +import logging from typing import Any from cleveragents.domain.models.core.skill import ( @@ -42,6 +56,16 @@ from cleveragents.skills.protocol import ( SkillErrorType, SkillMetadata, ) +from cleveragents.skills.refresh import SkillRefreshResult + +logger = logging.getLogger(__name__) + +_TOOL_REGISTRY_UNAVAILABLE_MSG = ( + "Tool registry is not configured on SkillRegistry; tool-ref validation " + "will be skipped during refresh. " + "Recovery: pass a ToolRegistry instance to SkillRegistry.__init__ and " + "call refresh() again once it is available." +) # --------------------------------------------------------------------------- # SkillRegistry @@ -256,6 +280,71 @@ class SkillRegistry: return errors + # -- Refresh --------------------------------------------------------------- + + def refresh(self, name: str) -> SkillRefreshResult: + """Recompute the flattened tool set for a single registered skill. + + Resolves tool references via ``SkillResolver`` and, when a Tool + Registry is configured, validates that every named tool ref actually + exists in the registry. + + When the Tool Registry is *not* configured, validation is skipped and a + warning is emitted once with recovery steps; the skill is still counted + as refreshed so callers are not blocked. + + Args: + name: The namespaced skill name to refresh. + + Returns: + ``SkillRefreshResult`` with the skill in ``refreshed``, + ``failed``, or ``skipped`` depending on the outcome. + + Raises: + SkillExecutionError: With ``SKILL_NOT_FOUND`` if ``name`` is not + registered. + """ + defn = self.get(name) + + skill_lookup = {n: d.skill for n, d in self._skills.items()} + resolver = SkillResolver() + + try: + entries = resolver.resolve_tools(defn.skill, skill_lookup) + except (ValueError, RuntimeError) as exc: + return SkillRefreshResult(failed={name: str(exc)}) + + if self._tool_registry is None: + logger.warning(_TOOL_REGISTRY_UNAVAILABLE_MSG) + return SkillRefreshResult(refreshed=[name]) + + missing: list[str] = [] + for entry in entries: + if entry.is_inline or entry.name.startswith(("mcp:", "agent_skill:")): + continue + if self._tool_registry.get(entry.name) is None: + missing.append(entry.name) + + if missing: + error_msg = f"Tool refs not found in tool registry: {', '.join(missing)}" + return SkillRefreshResult(failed={name: error_msg}) + + return SkillRefreshResult(refreshed=[name]) + + def refresh_all(self) -> SkillRefreshResult: + """Recompute flattened tool sets for all registered skills. + + Iterates over all registered skills and calls ``refresh(name)`` for + each, aggregating results into a single ``SkillRefreshResult``. + + Returns: + Aggregated ``SkillRefreshResult`` across all skills. + """ + result = SkillRefreshResult() + for name in list(self._skills.keys()): + result = result.merge(self.refresh(name)) + return result + # -- Validation ------------------------------------------------------------ def validate_skill(self, skill: SkillDefinition) -> list[str]: