Python: Add MCP-based skills discovery (McpSkillsSource)#6169
Conversation
…source) Implement Agent Skills discovery over MCP following the SEP-2640 convention: - McpSkillsSource: reads skill://index.json to discover skills served by an MCP server - McpSkill: lazily fetches SKILL.md content via resources/read on demand - McpSkillResource: wraps MCP resource results (text and binary) - Path traversal protection in get_resource for defense in depth - Samples for Foundry Toolbox and standalone MCP skills server - Comprehensive unit tests (514 lines) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR extends the Python skills system to support discovery and consumption of Agent Skills served over the Model Context Protocol (MCP), following the SEP-2640 skill://index.json convention, and adds samples and tests to demonstrate/validate the behavior.
Changes:
- Add
McpSkillsSource,McpSkill, andMcpSkillResourceto enable MCP-based skill discovery and lazy SKILL.md/resource loading. - Add two new samples showing generic MCP skills discovery and Foundry Toolbox-hosted skills discovery with AAD auth.
- Add a comprehensive new unit test suite covering index parsing, caching, text/binary resources, and traversal rejection.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| python/samples/02-agents/skills/README.md | Adds the MCP-based skills sample to the skills samples index. |
| python/samples/02-agents/skills/mcp_based_skill/README.md | Documents how to run the new MCP-based skills sample. |
| python/samples/02-agents/skills/mcp_based_skill/mcp_based_skill.py | New sample that discovers MCP skills and injects them via McpSkillsSource. |
| python/samples/02-agents/providers/foundry/README.md | Adds the Foundry Toolbox skills discovery sample to the Foundry samples index. |
| python/samples/02-agents/providers/foundry/foundry_chat_client_with_toolbox_skills.py | New sample that authenticates to a Foundry Toolbox MCP endpoint and discovers skills. |
| python/packages/core/tests/core/test_mcp_skills.py | New unit tests covering MCP skills discovery and resource loading behaviors. |
| python/packages/core/agent_framework/_skills.py | Implements MCP-based skills discovery and resource/content loading. |
| python/packages/core/agent_framework/init.py | Exposes the new MCP skills classes as part of the public API. |
![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: 4 | Confidence: 88%
✓ Correctness
The implementation of McpSkillsSource, McpSkill, and McpSkillResource is well-structured and correct. Path traversal protection is sound, caching logic is straightforward, error handling gracefully degrades, and the test suite comprehensively covers edge cases. No correctness issues found.
✓ Security Reliability
The MCP skills implementation is well-structured from a security and reliability perspective. Path traversal protection in _validate_resource_name correctly handles literal traversal patterns (../, backslash variants, absolute paths, URI schemes). Exception handling is fail-closed (returns None on errors). The broad
except Exceptionin get_resource correctly allows asyncio.CancelledError and KeyboardInterrupt to propagate since they inherit from BaseException. The caching in get_content is straightforward and idempotent. No blocking security issues found.
✓ Test Coverage
Test coverage is strong overall (514 lines covering parsing, caching, text/binary resources, path traversal rejection, and end-to-end discovery flows). The main gap is that two explicit exception-handling branches in the production code — the non-McpError exception path in
McpSkill.get_resourceand the non-McpError exception path inMcpSkillsSource._try_read_index— have no corresponding test coverage. These are defensive code paths with distinct behavior (different log levels, different messaging) that warrant at least minimal testing to prevent regression.
✓ Design Approach
The core MCP-skills shape looks reasonable, but there is one blocking design issue:
McpSkill.get_resource()currently masks every MCP/transport failure as a missing resource, so higher layers will tell the model "resource not found" for session/auth/server errors instead of surfacing a read failure.
Automated review by semenshi's agents
…ling and samples - Rename McpSkill/McpSkillResource/McpSkillsSource to MCPSkill/MCPSkillResource/MCPSkillsSource - Add data-URI prefix stripping for blob resource decoding - Let non-McpError exceptions propagate from get_resource() - Fix contradictory test comment - Use interactive input() in mcp_based_skill sample - Remove misleading sample output block Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
semenshi
force-pushed
the
feature/mcp-skills-source
branch
from
6113cc3 to
f37ddb5
Compare
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 90%
✓ Correctness
The MCP-based skills discovery implementation is well-structured and correct. MCPSkillsSource properly reads the index, validates entries, and constructs MCPSkill instances with lazy content loading. Path traversal prevention in get_resource is sound. Error handling in _try_read_index gracefully degrades (returning empty list) on failures. The MCPSkillResource.read() method correctly prioritizes binary over text content and handles the data-URI prefix edge case. All abstract method contracts from SkillsSource, Skill, and SkillResource are satisfied. The previously resolved review comments (broad McpError catch, data-URI normalization, naming consistency) are either addressed in the current diff or accepted by reviewers.
✓ Security Reliability
The MCP skills implementation is well-designed from a security and reliability standpoint. Path traversal validation in
_validate_resource_nameis thorough and well-tested (rejects..segments, absolute paths, URI schemes, backslash-normalized traversals). The data-URI normalization inMCPSkillResource.read()handles the prefix-strip correctly. Error handling follows a defensible pattern: discovery (_try_read_index) gracefully degrades to empty on any failure, whileget_resourceswallows only MCP errors and re-raises unexpected exceptions. No secrets in code, no unsafe deserialization, no resource leaks identified.
✓ Test Coverage
The test suite is comprehensive for the happy path and many edge cases (path traversal, invalid names, malformed JSON, etc.). However, three distinct code branches in the production code lack any test coverage: (1) the data-URI prefix stripping logic in MCPSkillResource.read(), (2) the re-raise of non-McpError exceptions in MCPSkill.get_resource(), and (3) the non-McpError exception handling path in MCPSkillsSource._try_read_index(). These are all explicitly coded behaviors that could silently regress without tests.
✗ Design Approach
I found one design issue in the new MCP skill resource path:
MCPSkill.get_resource()currently converts everyMcpErrorintoNone, which makes unrelated MCP/session/auth failures look like a normal missing resource to the rest of the skills stack.
Flagged Issues
-
MCPSkill.get_resource()swallows allMcpErrors and returnsNone, butSkillsProvider._read_skill_resource()treatsNoneas "resource not found" (line 2316-2318). Non-not-found MCP failures (session/auth/transport) will be silently misreported as missing resources instead of surfacing a real read error.
Automated review by semenshi's agents
Replace DefaultAzureCredential with AzureCliCredential to match the credential convention used in all other samples. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace raw mcp library imports (ClientSession, streamable_http_client) with the framework's MCPStreamableHTTPTool to keep MCP server connections consistent regardless of whether skills are enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously _try_read_index() and get_resource() swallowed every McpError as 'no skills available', making auth failures, server crashes, and connection drops indistinguishable from a server that simply has no skills. Now only two codes are treated as not-found: - -32002 (MCP-spec Resource not found) - -32601 (METHOD_NOT_FOUND — server lacks resources/read) All other McpError codes and non-McpError exceptions propagate with a warning log, surfacing real failures visibly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… skills Cover the re-raise branch in MCPSkill.get_resource for plain ConnectionError/TimeoutError, the generic McpError (code 0) propagation on get_resource, and TimeoutError propagation in _try_read_index. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit f31ed0d. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a separate MCP_SKILLS feature ID to ExperimentalFeature enum and use it for MCPSkillResource, MCPSkill, and MCPSkillsSource, since their promotion timeline is partly outside of our control. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
Python port of the .NET MCP skills discovery added in #6108.
Description
Adds
McpSkillsSource,McpSkill, andMcpSkillResource— enabling Agent Skills discovery over MCP following the SEP-2640 convention (skill://index.json→ lazySKILL.mdfetch viaresources/read).Includes samples for standalone MCP and Foundry Toolbox integration, plus comprehensive unit tests.
Contribution Checklist
Closes: #6087