Merge pull request 'feat(skill): add MCP refresh hooks' (#455) from feature/m4-skill-registry-refresh into master
CI / lint (push) Successful in 13s
CI / build (push) Successful in 16s
CI / quality (push) Successful in 19s
CI / typecheck (push) Successful in 47s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 50s
CI / integration_tests (push) Successful in 5m21s
CI / unit_tests (push) Successful in 15m1s
CI / benchmark-publish (push) Successful in 15m10s
CI / docker (push) Successful in 41s
CI / coverage (push) Successful in 54m46s
CI / lint (push) Successful in 13s
CI / build (push) Successful in 16s
CI / quality (push) Successful in 19s
CI / typecheck (push) Successful in 47s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 50s
CI / integration_tests (push) Successful in 5m21s
CI / unit_tests (push) Successful in 15m1s
CI / benchmark-publish (push) Successful in 15m10s
CI / docker (push) Successful in 41s
CI / coverage (push) Successful in 54m46s
Reviewed-on: #455 Reviewed-by: Luis Mendes <luis.mendes@cleverthis.com>
This commit was merged in pull request #455.
This commit is contained in:
@@ -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 <name>|--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`
|
||||
|
||||
@@ -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)
|
||||
@@ -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: "<missing refs>"}` |
|
||||
| Resolution succeeds, **no tool registry** | `refreshed = [name]`, warning logged |
|
||||
| Resolution fails (cycle, missing include) | `failed = {name: "<error>"}` |
|
||||
|
||||
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 |
|
||||
@@ -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"
|
||||
@@ -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}"
|
||||
)
|
||||
@@ -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 <command>")
|
||||
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())
|
||||
@@ -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
|
||||
@@ -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",
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
@@ -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",
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
@@ -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]:
|
||||
|
||||
Reference in New Issue
Block a user