Python: Support archive-type MCP skills (source, toolbox, sample)#7121
Python: Support archive-type MCP skills (source, toolbox, sample)#7121giles17 wants to merge 8 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
giles17
added
documentation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
giles17
changed the title
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
entirely in memory instead of extracting them to a local directory, and apply reviewer feedback.
| TAR_GZ = "tar_gz" | ||
|
|
||
|
|
||
| def _detect_archive_format(data: bytes, media_type: str | None, url: str | None) -> _ArchiveFormat: |
There was a problem hiding this comment.
might it be useful to use something like: https://pypi.org/project/filetype/ for this instead, I see some maintenance when we need to extend or support some other types, that we might circumvent here, not sure myself...
There was a problem hiding this comment.
Looked at it and yes, I think it could be useful. Though this adds an extra dependency in our core package. Also, filetype is magic-bytes only but this function also falls back to the MCP-advertised mimeType and the URL suffix.
I'd lean toward a follow-up issue to standardize detection across core rather than swapping just this one function and I'm happy to open that. Or would you prefer I adopt it here now?
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 unpacks archives entirely in memory 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 tune extraction; and the toolbox MCP skills sample demonstrates the full advertise → load → read-resources flow with an archive skill. It brings the Python side to parity with the .NET implementation while adapting the approach to Python conventions.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 and unpacks it entirely in memory into a{path: bytes}map, then assembles aFileSkillwhoseSKILL.mdbody drives it and whose sibling files (matching the resource extensions, within the search depth) become in-memoryInlineSkillResourceresources. Nothing is written to disk — there are no temporary directories to create, own, or prune. The archiveSKILL.mdfrontmatternamemust match the advertised index-entryname.SkillScript, so a bundled script can at most surface as a readable resource (and only if it matches the resource extensions —.pyis not a default resource extension).zipfile/tarfile/gzip): zip-slip member-name rejection (_normalize_archive_member_name), non-regular TAR member skipping (symlinks/hardlinks/devices), and file-count / total-uncompressed-size (_read_member_with_limit) / download-size limits (decompression-bomb defense). Format detection uses magic bytes, then media type, then URL suffix. A non-"resource not found" error while downloading an archive propagates, so a failedCachingSkillsSourcerefresh cannot overwrite a cached list with a partial result.archive_*constructor kwargs (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.FileSkillsSourcescript_extensions/resource_extensionsnow default to the built-in tuples in the signature (so the real default is visible in the API) and treatNoneas "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults).agent-framework-foundry-hosting) —FoundryToolbox.as_skills_provider()forwards thearchive_*options to the underlyingMCPSkillsSource. Only explicitly-set options are forwarded, so unset ones keep theMCPSkillsSourcedefaults.04-hosting/.../12_foundry_toolbox_mcp_skillsdemonstrates 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); because extraction is in memory, no writable working directory is needed. README,toolbox.yaml, and ignore files match.What is the impact of these changes?
skill-mddiscovery is unchanged andMCPSkillsSource(client=...)keeps working. Archive-bundled scripts are surfaced as readable resources only and are never discovered as runnable scripts.What do you want reviewers to focus on?
_normalize_archive_member_name,_read_member_with_limit, non-regular TAR member skipping) and the "scripts are never runnable" guarantee (noSkillScriptis ever emitted).FileSkillwith in-memoryInlineSkillResources) over extracting to disk, and whether thearchive_*kwarg surface reads well on bothMCPSkillsSourceandFoundryToolbox.as_skills_provider().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.