Python: Support archive-type MCP skills (source, toolbox, sample)#7121
Python: Support archive-type MCP skills (source, toolbox, sample)#7121giles17 wants to merge 4 commits into
Conversation
Add `archive`-type skill support to `MCPSkillsSource` so an MCP server can advertise packaged skills (ZIP / TAR / gzip-compressed TAR) that are downloaded, safely unpacked to a local directory, and served like file-based skills, while keeping the guarantee that MCP-delivered scripts are never executed. - Dispatch `skill://index.json` entries by `type`: `skill-md` (existing, fetched on demand) and `archive` (new). Unknown types are skipped. - `_ArchiveEntryLoader` downloads, extracts, and prunes archive skills and delegates discovery to an internal `FileSkillsSource` created with no script extensions and no runner, so bundled scripts surface as read-only resources only. - Hardened stdlib extraction: path-traversal (zip-slip) guard, non-regular TAR member skipping, and file-count / uncompressed-size / download-size limits. - Configure via `archive_*` constructor kwargs (no options object, per Python conventions); use `CachingSkillsSource` for refresh rather than a source level refresh interval. - Fix `FileSkillsSource` to treat `None` extensions as "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults). Port of .NET PR microsoft#6631. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e358a4e-538f-46be-8c58-128b6182352d
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 91%
✓ Correctness
The production code changes are well-structured and correct. The archive extraction hardening (path-traversal guards, link skipping, size/count limits) is sound. The
FileSkillsSourceNonevs empty-tuple fix is clean and well-tested. The_mcp_first_blobhelper, format detection, and archive entry lifecycle are all logically correct. The only issue found is in AGENTS.md where theSkillsSourcedecorators bullet point has its entire second half duplicated verbatim (fromSkillsSourceContext (frozen) carries...onward), roughly doubling the paragraph length with identical content.
✓ Security Reliability
The archive extraction implementation is well-hardened from a security perspective. Path-traversal ('zip-slip') is guarded by both segment-level '..' checks and a defense-in-depth
_is_path_within_directorycall. TAR symlinks/hardlinks/devices are correctly skipped viamember.isreg(). Decompression-bomb defense uses actual byte counting in_copy_stream_with_limitrather than trusting archive metadata. The 'scripts are never runnable' guarantee is enforced by passingscript_extensions=()andscript_runner=Noneto the internalFileSkillsSource, and theFileSkillsSourceNone-vs-empty-tuple fix correctly differentiates 'use defaults' from 'discover none'. Download size, file count, and uncompressed size limits are all enforced. No blocking security or reliability issues were found.
✓ Test Coverage
The PR adds substantial test coverage for the new archive-type MCP skill support, including integration tests through MCPSkillsSource (ZIP/TAR/TAR.GZ discovery, script non-runnability, pruning, oversized/invalid/unsupported archives, mixed entries) and unit tests for extraction hardening (format detection, path-traversal rejection, file-count/size limits, symlink skipping). Two meaningful test gaps exist: (1) the semantic change to
FileSkillsSourcewhere empty-tuplescript_extensions/resource_extensionsnow means 'discover none' instead of falling back to defaults has no direct unit test in test_skills.py — it's only indirectly validated throughtest_bundled_script_is_never_runnable; and (2) the_mcp_first_blobhelper's data-URI prefix stripping and invalid-base64 error handling paths have no test coverage.
✓ Failure Modes
The archive-type MCP skill support is well-implemented with solid extraction hardening (path-traversal guards, decompression-bomb limits, symlink skipping) and proper cleanup of partial extractions. The "scripts are never runnable" invariant is correctly enforced via
script_extensions=()and no runner. Error handling throughout the archive loader is thorough — download failures, format detection failures, and extraction errors are all caught, logged, and result in the skill being skipped. TheFileSkillsSourceNonevs empty-tuple semantics fix is clean and backwards-compatible. No blocking failure-mode issues were found.
✓ Design Approach
The archive support is close, but I found one design-level regression: archive resource reads now swallow all non-not-found failures and silently drop the skill. That breaks the existing MCPSkillsSource/MCPSkill error-handling contract in this repo and can turn transient/auth/server failures into successful-but-incomplete refreshes.
Automated review by giles17's agents
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
Only swallow "resource not found" MCP errors when downloading an archive resource; re-raise every other error (auth failure, INTERNAL_ERROR, connection drop, timeout) so a transient transport failure is not silently turned into a missing skill. This matches the existing failure model used by `_try_read_index` and `MCPSkill.get_resource`, and avoids a failed `CachingSkillsSource` refresh overwriting a previously cached list with a partial result. Add tests asserting archive-download INTERNAL_ERROR and ConnectionError propagate out of `get_skills`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e358a4e-538f-46be-8c58-128b6182352d
…mple - FoundryToolbox.as_skills_provider() now forwards the MCPSkillsSource archive options (archive_skills_directory, archive_resource_extensions, archive_resource_search_depth, archive_max_file_count, archive_max_size_bytes, archive_max_uncompressed_size_bytes). Only explicitly-set options are forwarded so unset ones keep the MCPSkillsSource defaults. This lets a hosted toolbox agent redirect archive extraction to a writable directory (the default is under the cwd, which may be read-only in a container). - Add unit tests covering default (no options forwarded) and override forwarding. - Update the 12_foundry_toolbox_mcp_skills sample to demonstrate all three progressive-disclosure stages with an archive skill: escalation-policy now ships a references/refund-matrix.md resource and is uploaded as a ZIP archive; main.py disables load_skill and read_skill_resource approval and points archive extraction at a temp directory. README, toolbox.yaml, and ignore files updated accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f14f83d-1868-45c1-be1a-12f49a58ac36
Cast provider._source to _FoundryToolboxSkillsSource before accessing the private _archive_options, so the ty checker (which runs over tests) resolves the concrete type instead of the SkillsSource base. Replaces the mypy-style type: ignore that ty did not honor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f14f83d-1868-45c1-be1a-12f49a58ac36
Motivation & Context
The MCP skills source previously discovered only
skill-mdindex entries, where a skill'sSKILL.mdand sibling resources are fetched on demand from the server. The Agent Skills discovery spec (SEP-2640) also definesarchiveentries, where a skill is distributed as a single packaged archive (ZIP / TAR / gzip-compressed TAR) that unpacks into the skill's namespace. Without archive support, skills published in that format are silently unusable.This change adds
archive-type skill support end to end: the coreMCPSkillsSourcedownloads and safely unpacks archives and serves them like file-based skills (never executing MCP-delivered scripts);FoundryToolbox.as_skills_provider()exposes the archive knobs so a hosted toolbox agent can configure extraction; and the toolbox MCP skills sample is updated to demonstrate the full advertise → load → read-resources flow with an archive skill. It brings the Python side to parity with the .NET implementation.Description & Review Guide
What are the major changes?
agent-framework-core) —MCPSkillsSourcenow dispatchesskill://index.jsonentries by theirtype(case-insensitive):skill-md(existing, fetched on demand) andarchive(new). Entries whose type has no handler (e.g.mcp-resource-template) are skipped._ArchiveEntryLoaderdownloads each archive resource, extracts it into{dir}/{skill-name}/, prunes directories the server no longer advertises, and delegates discovery to an internalFileSkillsSourcecreated with no script extensions and no runner, so bundled scripts surface as read-only resources only and are never runnable.zipfile/tarfile/gzip): path-traversal ("zip-slip") guard, non-regular TAR member skipping (symlinks/hardlinks/devices), and file-count / total-uncompressed-size / download-size limits (decompression-bomb defense). Format detection uses magic bytes, then media type, then URL suffix.archive_*constructor kwargs (archive_skills_directory,archive_resource_extensions,archive_resource_search_depth,archive_max_file_count,archive_max_size_bytes,archive_max_uncompressed_size_bytes) — Python uses plain kwargs rather than a*Optionsobject as in .NET. Refresh/caching is provided by wrapping inCachingSkillsSource, consistent with how every other Python skill source composes caching.FileSkillsSourceto treatNoneextensions as "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults) — this is what lets the archive loader disable script discovery.agent-framework-foundry-hosting) —FoundryToolbox.as_skills_provider()now forwards thearchive_*options to the underlyingMCPSkillsSource. Only explicitly-set options are forwarded, so unset ones keep theMCPSkillsSourcedefaults. This lets a hosted toolbox agent redirect archive extraction to a writable directory (the default is under the cwd, which may be read-only in a container).04-hosting/.../12_foundry_toolbox_mcp_skillsnow demonstrates all three progressive-disclosure stages with an archive skill:escalation-policyships areferences/refund-matrix.mdresource and is uploaded as a ZIP archive, whilesupport-stylestays a singleSKILL.md.main.pydisablesload_skill/read_skill_resourceapproval (the Responses host runs without anAgentSession) and points archive extraction at a temp directory. README,toolbox.yaml, and ignore files are updated to match.What is the impact of these changes?
skill-mddiscovery is unchanged,MCPSkillsSource(client=...)keeps working, and the newas_skills_provider()archive kwargs all default to "unset" (i.e.MCPSkillsSourcedefaults). Archive-bundled scripts are surfaced as readable resources only; the inner file source is created with no allowed script extensions and no script runner, so they are never discovered as runnable scripts.What do you want reviewers to focus on?
_resolve_archive_destination,_copy_stream_with_limit, non-regular TAR member skipping) and the "scripts are never runnable" guarantee viascript_extensions=()+ no runner.MCPSkillsSourcedefaults) onas_skills_provider()reads well.Related Issue
Fixes #6088
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.