Python: Add MCP-based skills support (skill-md type) - #6149
Python: Add MCP-based skills support (skill-md type)#6149semenshi-m (semenshi) wants to merge 3 commits into
Conversation
…okup Port of .NET commit 08541ee. Replace property-based Skill.content/resources/scripts with async by-name lookup methods: - content property -> async get_content() -> str - resources property -> async get_resource(name) -> SkillResource | None - scripts property -> async get_script(name) -> SkillScript | None SkillsProvider now always includes all three tools (load_skill, read_skill_resource, run_skill_script) and both instruction blocks regardless of whether any skills have resources or scripts. ClassSkill retains resources/scripts properties as overridable hooks for subclass reflection-based discovery. 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
Adds MCP-backed skill discovery/loading to the Python Agent Framework (porting the .NET “skill-md” MCP skills work), and updates the core skills plumbing to support async/lazy content + on-demand resource reads.
Changes:
- Introduces
McpSkillsSource,McpSkill, andMcpSkillResourceto discover skills fromskill://index.jsonand lazily loadSKILL.mdviaresources/read. - Refactors
Skillto use asyncget_content()plus async lookup methods (get_resource(),get_script()), and updatesSkillsProvidertool wiring accordingly. - Adds a new MCP-skills unit test suite and updates existing skills tests for the async API/tool behavior changes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_skills.py | Core implementation changes: async skill content API + new MCP skills source/skill/resource types + provider tool wiring updates. |
| python/packages/core/agent_framework/init.py | Exposes MCP skill types as public exports. |
| python/packages/core/tests/core/test_skills.py | Updates existing skills tests for async content loading and updated tool set. |
| python/packages/core/tests/core/test_mcp_skills.py | Adds coverage for MCP index parsing, MCP skill lazy content loading, and MCP resource reading. |
| if not name or not name.strip(): | ||
| return None | ||
|
|
||
| uri = self._skill_root_uri + name | ||
| try: |
| try: | ||
| result = await self._client.read_resource(_mcp_any_url(uri)) | ||
| except Exception as ex: | ||
| from mcp.shared.exceptions import McpError | ||
|
|
||
| if isinstance(ex, McpError): |
| try: | ||
| result = await self._client.read_resource(_mcp_any_url(self._INDEX_URI)) | ||
| except Exception as ex: | ||
| from mcp.shared.exceptions import McpError | ||
|
|
||
| if isinstance(ex, McpError): | ||
| logger.debug("No skill://index.json resource available on MCP server: %s", ex) | ||
| else: | ||
| logger.warning("Failed to read skill://index.json from MCP server.", exc_info=True) | ||
| return None |
| instructions = self._create_instructions( | ||
| prompt_template=self._instruction_template, | ||
| skills=skills, | ||
| include_script_runner_instructions=has_scripts, | ||
| include_resource_instructions=has_resources, | ||
| ) | ||
|
|
| def resource( | ||
| self, |
| # Binary content from BlobResourceContents (second item) should NOT take precedence | ||
| # since the first item is text. The loop finds text first, but the implementation | ||
| # checks BlobResourceContents first in the iteration order. | ||
| # Actually: the implementation iterates all contents looking for BlobResourceContents first. | ||
| # So the text item (first) is not a blob, the blob item (second) IS a blob -> returns bytes. |
![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: 2 | Confidence: 81%
✓ Correctness
No actionable issues found in this dimension.
✓ Test Coverage
The test suite for the new MCP skills support is generally comprehensive with 35 tests covering core functionality. However, the path traversal test (
test_get_resource_path_traversal_returns_none) provides a false sense of security: it passes only because the mock client lacks the traversal URI, not because the implementation actually validates against path traversal. The productionMcpSkill.get_resourcesimply concatenates the root URI with the user-provided name and makes an MCP request — no traversal check exists. Additionally, there's no test forMcpSkill.get_contentpropagating non-McpError exceptions (e.g., network failures). Test coverage is thorough with meaningful assertions for caching, error cases, tool registration, and script discovery. One minor naming issue: test_custom_template_without_runner_placeholder_raises no longer tests that it raises, but the docstring was updated to reflect the new behavior.
Automated review by semenshi's agents
| assert await skill.get_resource("") is None | ||
| assert await skill.get_resource(" ") is None | ||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
This test gives a false sense of security: get_resource returns None here only because the mock raises McpError for unknown URIs—not because the implementation validates against path traversal. To properly test traversal protection (once the implementation-side fix from the existing review comment is applied), assert that client.read_resource is never called when a traversal name is provided, confirming the name is rejected before any I/O.
| @pytest.mark.asyncio | |
| @pytest.mark.asyncio | |
| @pytest.mark.parametrize("name", ["../escape.md", "references/../../escape.md", "."]) | |
| async def test_get_resource_path_traversal_returns_none(self, name: str) -> None: | |
| client = _make_client(**{"skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD)}) | |
| from agent_framework import SkillFrontmatter | |
| fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") | |
| skill = McpSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client) | |
| resource = await skill.get_resource(name) | |
| assert resource is None | |
| # Verify traversal is rejected before making any MCP call | |
| client.read_resource.assert_not_called() |
Adds McpSkill, McpSkillResource, and McpSkillsSource so agents can discover and load skills served over the Model Context Protocol following the SEP-2640 convention (skill://index.json + per-skill SKILL.md resources). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Demonstrates discovering Agent Skills served over MCP using McpSkillsSource by connecting to a remote MCP server (configured via MCP_SKILLS_SERVER_URL) that exposes skill resources following the SEP-2640 convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8e2513f to
8b8da86
Compare
Evan Mattson (moonbox3)
added
documentation
Description
Port of .NET PR #6108 to Python. Adds MCP-based skills support (skill-md type) following the SEP-2640 convention.
New classes
Design decisions
Validation
Motivation and Context
Enables Python agents to discover and load skills from MCP servers, matching .NET PR #6108.