Experimental/skills over mcp v2 base#810
Draft
olaservo wants to merge 18 commits into
Draft
Conversation
Implements the io.modelcontextprotocol/skills extension: skills served by connected MCP servers are discovered via the well-known skill://index.json resource and surfaced through the existing skills pipeline so they behave identically to filesystem skills. - SkillManifest gains optional uri/server_name; path becomes optional. Construction-time invariants reject manifests with neither backing, and URI-backed manifests with no server_name (without it the aggregator falls through every connected server, collapsing the per-server trust boundary). - format_skills_for_prompt renders resource URIs as <location>/ <directory> for MCP-backed skills (any scheme -- skill:// is the SEP default, but github://, repo://, etc. are valid) and adds an MCP-specific preamble note when any URI-backed skill is present. - New mcp_skills_loader module: fetches skill://index.json with a size bound, parses concrete skill-md entries, rejects file:// entries (the MCP-server-is-authority trust model breaks down if local-disk paths enter the allow list), and rejects degenerate URIs with no skill-path segment. - SkillReader accepts URIs of any scheme, dispatches via aggregator.get_resource, and enforces a URI-root trust boundary mirroring the filesystem allowed-dirs check (rejects raw and percent-encoded ../. traversal markers, query/fragment suffixes, and file:// URIs as defense in depth). - McpAgent.initialize fetches MCP skills post-connect and merges with filesystem manifests; filesystem wins on name collision. Discovery failures are logged but never abort agent startup. - MCPServerSettings gains mcp_skills bool for per-server opt-out (independent of include_instructions). Includes unit tests for the loader, reader, URI helpers, prompt formatting, and SkillManifest invariants. SEP: https://github.com/modelcontextprotocol/experimental-ext-skills Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per SEP-2640, hosts SHOULD match the index $schema URI against known versions before processing. An unknown schema doesn't abort parsing (forward compatibility — preserve entries we still recognize), but emits a warning so operators know the host may lag the server's index format. A missing $schema is treated as tolerant: $schema is a SHOULD on servers and warning when it's absent would be noise for legitimate older / minimal indexes.
Per SEP-2640, hosts MUST honor archive-distributed skills (.tar.gz and .zip) and apply the Agent Skills archive-safety requirements: reject path traversal, absolute paths, symlinks, oversized members, and decompression bombs. Archives are unpacked in memory at discovery time and seeded into a per-skill file map keyed by the skill's root URI; the SkillReader checks this cache before issuing resources/read so supporting-file reads after the initial fetch stay local. The trust boundary is unchanged — archive roots seed the same `_allowed_uri_roots` set, and traversal URIs are rejected before the cache lookup. The loader's return type widens to `LoadedSkills` to absorb the archive cache (and a future `template_entries` field) without further breaking changes.
Per SEP-2640, hosts SHOULD surface `mcp-resource-template` index
entries as interactive discovery points: user fills RFC 6570 variables
via the MCP completion API, then the resolved URI registers as a
regular `skill-md` manifest. Previously the loader collected templates
into `LoadedSkills.template_entries` but never exposed them.
New surface:
- `expand_uri_template` for simple `{var}` form (the SEP's example
case). Percent-encodes reserved characters per RFC 6570 §3.2.2 so a
user-supplied value with `/` can't escape the template's segment
boundary.
- `resolve_skill_template(aggregator, template, variables)` shared
helper that expands, fetches, and parses — reusing the concrete
`skill-md` pipeline so resolved templates and direct entries take
identical paths through the host.
- `McpAgent.complete_skill_template_argument` and
`register_resolved_skill_template` so the slash-command handler can
drive completion + registration through the agent without reaching
into the aggregator.
- `/skills templates` and `/skills resolve <number> [var=value ...]`
REPL commands. Resolve walks template variables via completion,
falls back to free-form input when the server returns no candidates,
and accepts `var=value` overrides for scripted runs.
Completion uses the existing `MCPAggregator.complete_resource_argument`
wrapper — no new aggregator plumbing needed.
ACP slash surface is untouched in this round; templates are CLI-only.
Per SEP-2640 §Discovery, hosts MAY subscribe to `skill://index.json` and re-run discovery on update notifications. This commit wires that end-to-end: - `MCPAggregator.subscribe_to_resource` gates on the server's declared `resources.subscribe` capability and soft-fails — subscribe is a SHOULD on servers, so failure is expected, not exceptional. - `McpAgent` registers itself as the aggregator's notification callback (defensively only when the slot is unset so other agents sharing the aggregator aren't clobbered). - `_on_server_notification` filters for `ResourceUpdatedNotification` whose URI is `skill://index.json`; other notifications pass through untouched. - `_refresh_skills_after_index_update` serializes via a per-agent lock, re-reads the index for just that server, computes adds/removes, rebuilds the manifest list (filesystem and other-server entries preserved), refreshes the archive cache, and surfaces a runtime- toolbar notice summarizing the diff. The system prompt's `<available_skills>` block is intentionally not rewritten — it's part of conversation history at this point — but the SkillReader's allow-list updates, so removed skills become unreadable on the next attempt.
Per SEP-2640 §Security Implications, hosts SHOULD indicate which server a skill originates from and SHOULD let users gate individual skills: - `format_skills_for_prompt` now emits a `<source>` element per skill. URI-backed manifests render `mcp-server: <name>`; filesystem skills render `filesystem: <dir>` for symmetry, so the model never has to special-case the absence of provenance. - New `disabled_skill_names` parameter filters disabled entries out of the rendered output before the prompt is built. Empty result returns the empty string instead of an empty `<available_skills>` block — the model shouldn't see "here's nothing". - `McpAgent.disable_skill()` / `enable_skill()` mutate an in-process set and rebuild the SkillReader so disabled skills become both invisible (next prompt build) and unreadable (allow-list strips them immediately). The system prompt at the head of conversation history is intentionally not rewritten — the model may still see disabled skills there, but `read_skill` refuses the load. - `/skills disable <name>` and `/skills enable <name>` REPL commands drive the agent-level toggles. Unknown names short-circuit to a warning instead of silently adding to the disabled set so a typo is distinguishable from a no-op. - `_append_manifest_entry` now handles URI-backed manifests (no filesystem path) by showing `source: mcp-server <name>` instead of crashing on `manifest.path.parent`.
SEP-2640 §Discovery makes this a MUST: "hosts MUST support loading a
skill given only its URI" — even when the URI never appeared in any
index. The model may receive a `skill://` URI from server instructions,
from another skill's body, or from the user; previously read_skill
rejected anything outside a discovered manifest root.
The trust boundary now admits two cases:
- Inside a discovered manifest root: any scheme, dispatch to the
recorded server (unchanged).
- `skill://` scheme outside any root: dispatch via aggregator fanout
(`get_resource(uri)` without `server_name`). First responder wins —
the documented ambiguity in the SEP's read_resource example. Other
schemes outside any root remain rejected because the host has no
evidence they're skills at all ("outside the index, hosts recognize
skills by the skill:// scheme prefix").
Reworked the orphan-manifest defensive test: that defense was
specifically guarding against fanout, which is now the SEP-required
behavior. Added two new tests covering unenumerated load (success when
a server responds, useful error when none do) and confirming the
non-`skill://` rejection still holds.
Also corrected a subscribe test that asserted a removed skill becomes
"not within an allowed skill root" — that's no longer true; the URI
remains readable via fanout (removal from the index doesn't mean the
server stopped serving it). What changes on removal is the
<available_skills> rendering, not the read path.
SEP-2640 §Security Implications: hosts SHOULD let users inspect a skill's content before it is loaded into model context. The model decides autonomously when to call `read_skill`, so this command is the only pre-load inspection surface available to users. The handler routes through the agent's SkillReader so the read goes through the same trust boundary, archive cache, and (for MCP-backed skills) aggregator dispatch as a model-driven read. Critically, the output is rendered to the user's UI and does NOT enter model context — a preview never plants skill text in the conversation history, so `/skills preview` is a no-op for the model's reasoning. Honoring the disable toggle is automatic because the SkillReader's allow-list strips disabled entries: preview of a disabled skill returns "Access denied" the same way a model call would. Aliased as `/skills inspect` and `/skills show` so users discover the surface under whichever name reads naturally.
SEP-2640 §Security Implications makes this a MUST: "Hosts MUST treat MCP-served skill content as untrusted model input, subject to the same prompt-injection defenses applied to any server-provided text." The wrapper is the host's explicit defense layer — without it the model has no signal distinguishing skill bodies from text the user typed. `SkillReader._wrap_untrusted_mcp_content` envelopes the body in `<untrusted-skill-content source="mcp-server: <name>" uri="<uri>">` tags on both the live aggregator path and the archive-cache path. The unenumerated-URI fanout doesn't know which server answered, so it attributes `(unknown)` rather than skipping the wrapper — a weaker but still-honest signal is better than no signal. Filesystem skills are deliberately NOT wrapped. They were installed by the user and inherit the user's trust level; wrapping them too would dilute the signal until it became ambient noise. The `format_skills_for_prompt` preamble explains both halves of this contract to the model: wrapped content is reference material describing workflows, not authoritative instructions to obey. Updated four pre-existing reader tests whose strict equality assertions broke under the wrapper; new test_untrusted_wrapper.py covers the new behavior across the three MCP paths and verifies filesystem reads stay unwrapped.
- Drop the `INDEX_URI as SKILL_INDEX_URI` rename in mcp_agent.py — the bare name doesn't conflict with anything in scope. - Type `_skill_template_entries`, `complete_skill_template_argument`, and `register_resolved_skill_template` against `SkillTemplateEntry` instead of leaving them as bare `list` / untyped params. Now the agent's template surface is statically checkable. - Replace the `body += block.text` loop in `handle_preview_skill` with a single `"".join(...)` over a generator.
…-over-mcp-v2 # Conflicts: # src/fast_agent/agents/mcp_agent.py
Keep the SEP-mandated invariants and security WHYs but compress block comments that narrated the next line or restated the canonical `file://`/trust rationale. Net -124 lines across mcp_agent.py, mcp_skills_loader.py, skill_archive.py, and skill_reader.py.
Drops the in-process tar/zip unpacker and the per-skill archive cache. The feature was orthogonal to the rest of the Skills-over-MCP host: the `skill-md` URI-fetch path and `mcp-resource-template` resolution work without it. Removing it shrinks the foundation, deletes a non-trivial security surface (path-traversal defenses, decompression- bomb limits, symlink rejection in `skill_archive.py`), and defers the feature until a reference server actually emits `type: "archive"` entries. Runtime behavior for servers still emitting archive entries: the loader's dispatcher logs "Skipping MCP skill entry with unrecognized type" at DEBUG and continues — the index is not aborted, other entries load normally. Deleted: - `src/fast_agent/mcp/skill_archive.py` (unpacker module) - `tests/unit/fast_agent/skills/test_skill_archive.py` Removed from `mcp_skills_loader.py`: - `unpack_skill_archive` import - `MAX_ARCHIVE_BLOB_BYTES` / `ARCHIVE_SUFFIXES` constants - `LoadedSkills.archive_cache` field (no longer populated) - `_first_blob`, `_strip_archive_suffix`, `_load_archive_entry` helpers - The `if entry_type == "archive":` dispatch branch - Archive references in module/function docstrings Removed from `tools/skill_reader.py`: - `archive_cache` parameter on `SkillReader.__init__` + docstring entry - `self._archive_cache` field - `_read_from_archive_cache` method - Early-return cache check in `_read_mcp_uri` Removed from `agents/mcp_agent.py`: - `self._skill_archive_cache` instance var - `archive_cache` parameter on `set_skill_manifests` and its call chain - Archive-cache argument in `SkillReader(...)` construction - Archive-cache scoping in `_refresh_skills_after_index_update` Test cleanup: dropped the `TestArchiveEntries` class from `test_mcp_skills_loader.py`, `test_archive_cache_read_is_wrapped` from `test_untrusted_wrapper.py`, and the six archive-cache cases from `test_skill_reader_uri.py`. 152/152 remaining skills tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-over-mcp-v2-base # Conflicts: # src/fast_agent/tools/skill_reader.py
Owner
|
Just wondering about the ergonomics of this feature, seems there are 2 slightly conflicting goals:
1.5) A remote client with sandbox access that can get skills/scripts for deployment to "do stuff". (This is basically what we have with the Anthropic Messages API today).
Skills that are useful in scenario 2 probably aren't useful in scenario 1. Wdyt? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding a draft PR to cover the following from SEP-2640:
Decided to keep these out of the first PR, for brevity's sake: