Address PR review comments: rename to MCP* convention, fix error handling 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>
This commit is contained in:
SergeyMenshykh
2026-05-29 11:23:59 +01:00
Unverified
parent b1b79d5cca
commit 6e8f7aa03f
8 changed files with 92 additions and 103 deletions
@@ -157,9 +157,9 @@ from ._skills import (
InlineSkillResource,
InlineSkillScript,
InMemorySkillsSource,
McpSkill,
McpSkillResource,
McpSkillsSource,
MCPSkill,
MCPSkillResource,
MCPSkillsSource,
Skill,
SkillFrontmatter,
SkillResource,
@@ -428,9 +428,9 @@ __all__ = [
"MCPStdioTool",
"MCPStreamableHTTPTool",
"MCPWebsocketTool",
"McpSkill",
"McpSkillResource",
"McpSkillsSource",
"MCPSkill",
"MCPSkillResource",
"MCPSkillsSource",
"MemoryContextProvider",
"MemoryFileStore",
"MemoryIndexEntry",
+29 -25
View File
@@ -3390,16 +3390,16 @@ def _parse_mcp_skill_index(text: str) -> _McpSkillIndex:
@experimental(feature_id=ExperimentalFeature.SKILLS)
class McpSkillResource(SkillResource):
class MCPSkillResource(SkillResource):
"""A :class:`SkillResource` backed by content fetched from an MCP server.
The :class:`~mcp.types.ReadResourceResult` is fetched eagerly by
:meth:`McpSkill.get_resource` at construction time; :meth:`read`
:meth:`MCPSkill.get_resource` at construction time; :meth:`read`
extracts text or binary content from the result.
"""
def __init__(self, *, name: str, result: ReadResourceResult) -> None:
"""Initialize an McpSkillResource.
"""Initialize an MCPSkillResource.
Args:
name: The resource name (e.g. a relative path or identifier).
@@ -3420,14 +3420,19 @@ class McpSkillResource(SkillResource):
for content in self._result.contents:
if isinstance(content, BlobResourceContents):
return base64.b64decode(content.blob)
blob = content.blob
# Strip data-URI prefix if present (some MCP servers send
# full data URIs instead of raw base64).
if blob.startswith("data:"):
blob = blob.split(",", 1)[-1]
return base64.b64decode(blob)
text = "\n".join(c.text for c in self._result.contents if isinstance(c, TextResourceContents))
return text if text else None
@experimental(feature_id=ExperimentalFeature.SKILLS)
class McpSkill(Skill):
class MCPSkill(Skill):
"""A :class:`Skill` discovered from an MCP server exposing the Agent Skills convention.
The skill is constructed from ``skill://index.json`` discovery metadata;
@@ -3435,9 +3440,9 @@ class McpSkill(Skill):
server on demand via ``resources/read``.
Per SEP-2640, resources referenced inside SKILL.md are fetched on demand
via the originating MCP server: :meth:`get_resource` resolves a relative
via the originating MCP server: :meth:`get_resource` resolves a relative
resource name against the skill's root URI, issues a ``resources/read``
request, and returns an :class:`McpSkillResource` with pre-fetched content.
request, and returns an :class:`MCPSkillResource` with pre-fetched content.
"""
_SKILL_MD_SUFFIX: Final[str] = "SKILL.md"
@@ -3448,7 +3453,7 @@ class McpSkill(Skill):
skill_md_uri: str,
client: ClientSession,
) -> None:
"""Initialize an McpSkill.
"""Initialize an MCPSkill.
Args:
frontmatter: The parsed frontmatter metadata for this skill.
@@ -3498,13 +3503,13 @@ class McpSkill(Skill):
Resolves *name* as a relative path against the skill's root URI,
issues a ``resources/read`` request to the MCP server, and returns
an :class:`McpSkillResource` with the pre-fetched content.
an :class:`MCPSkillResource` with the pre-fetched content.
Args:
name: The resource name (e.g. ``references/checklist.md``).
Returns:
An :class:`McpSkillResource`, or ``None`` when the name is empty
An :class:`MCPSkillResource`, or ``None`` when the name is empty
or the resource does not exist on the server.
"""
if not name or not name.strip():
@@ -3522,10 +3527,9 @@ class McpSkill(Skill):
if isinstance(ex, McpError):
return None
logger.debug("Unexpected error reading MCP resource '%s'", uri, exc_info=True)
return None
raise
return McpSkillResource(name=name, result=result)
return MCPSkillResource(name=name, result=result)
@staticmethod
def _validate_resource_name(name: str) -> str | None:
@@ -3561,20 +3565,20 @@ class McpSkill(Skill):
If the URI doesn't end with ``SKILL.md``, ensures it ends with a
trailing slash.
"""
if skill_md_uri.endswith(McpSkill._SKILL_MD_SUFFIX):
return skill_md_uri[: -len(McpSkill._SKILL_MD_SUFFIX)]
if skill_md_uri.endswith(MCPSkill._SKILL_MD_SUFFIX):
return skill_md_uri[: -len(MCPSkill._SKILL_MD_SUFFIX)]
if skill_md_uri.endswith("/"):
return skill_md_uri
return skill_md_uri + "/"
@experimental(feature_id=ExperimentalFeature.SKILLS)
class McpSkillsSource(SkillsSource):
class MCPSkillsSource(SkillsSource):
"""A :class:`SkillsSource` that discovers Agent Skills served over MCP.
Discovery follows the SEP-2640 recommended approach: the source reads
the well-known ``skill://index.json`` resource and constructs one
:class:`McpSkill` per ``skill-md`` entry directly from the entry's
:class:`MCPSkill` per ``skill-md`` entry directly from the entry's
``name``, ``description``, and ``url`` fields.
The referenced ``SKILL.md`` resource is **not** read during discovery;
@@ -3592,7 +3596,7 @@ class McpSkillsSource(SkillsSource):
from mcp.client.session import ClientSession
source = McpSkillsSource(client=session)
source = MCPSkillsSource(client=session)
skills = await source.get_skills()
"""
@@ -3600,7 +3604,7 @@ class McpSkillsSource(SkillsSource):
_SKILL_MD_TYPE: Final[str] = "skill-md"
def __init__(self, client: ClientSession) -> None:
"""Initialize an McpSkillsSource.
"""Initialize an MCPSkillsSource.
Args:
client: An MCP client session connected to a server that
@@ -3612,10 +3616,10 @@ class McpSkillsSource(SkillsSource):
"""Discover and return skills from the MCP server.
Reads ``skill://index.json``, parses it, and creates an
:class:`McpSkill` for each valid ``skill-md`` entry.
:class:`MCPSkill` for each valid ``skill-md`` entry.
Returns:
A list of discovered :class:`McpSkill` instances.
A list of discovered :class:`MCPSkill` instances.
"""
index = await self._try_read_index()
if index is None:
@@ -3665,14 +3669,14 @@ class McpSkillsSource(SkillsSource):
logger.warning("Failed to parse skill://index.json JSON document.", exc_info=True)
return None
def _try_create_skill(self, entry: _McpSkillIndexEntry) -> McpSkill | None:
"""Attempt to create an :class:`McpSkill` from an index entry.
def _try_create_skill(self, entry: _McpSkillIndexEntry) -> MCPSkill | None:
"""Attempt to create an :class:`MCPSkill` from an index entry.
Args:
entry: A single entry from the skill index.
Returns:
An :class:`McpSkill` if the entry is valid, or ``None`` if the
An :class:`MCPSkill` if the entry is valid, or ``None`` if the
entry should be skipped.
"""
if entry.type != self._SKILL_MD_TYPE:
@@ -3701,7 +3705,7 @@ class McpSkillsSource(SkillsSource):
logger.debug("Skipping entry '%s': invalid metadata: %s", entry.name, ex)
return None
return McpSkill(frontmatter=fm, skill_md_uri=entry.url, client=self._client)
return MCPSkill(frontmatter=fm, skill_md_uri=entry.url, client=self._client)
# endregion
@@ -1,6 +1,6 @@
# Copyright (c) Microsoft. All rights reserved.
"""Tests for MCP-based skills (McpSkillsSource, McpSkill, McpSkillResource)."""
"""Tests for MCP-based skills (MCPSkillsSource, MCPSkill, MCPSkillResource)."""
from __future__ import annotations
@@ -18,7 +18,7 @@ from mcp.types import (
)
from pydantic import AnyUrl
from agent_framework import McpSkill, McpSkillResource, McpSkillsSource
from agent_framework import MCPSkill, MCPSkillResource, MCPSkillsSource
from agent_framework._skills import _parse_mcp_skill_index
# ---------------------------------------------------------------------------
@@ -97,7 +97,7 @@ def _make_client(**read_resource_responses: ReadResourceResult) -> AsyncMock:
# ---------------------------------------------------------------------------
class TestParseMcpSkillIndex:
class TestParseMCPSkillIndex:
"""Tests for the _parse_mcp_skill_index helper."""
def test_parses_valid_index(self) -> None:
@@ -131,17 +131,17 @@ class TestParseMcpSkillIndex:
# ---------------------------------------------------------------------------
# McpSkillResource tests
# MCPSkillResource tests
# ---------------------------------------------------------------------------
class TestMcpSkillResource:
"""Tests for McpSkillResource."""
class TestMCPSkillResource:
"""Tests for MCPSkillResource."""
@pytest.mark.asyncio
async def test_read_text_content(self) -> None:
result = _make_text_result("hello world")
resource = McpSkillResource(name="test.md", result=result)
resource = MCPSkillResource(name="test.md", result=result)
content = await resource.read()
assert content == "hello world"
@@ -149,14 +149,14 @@ class TestMcpSkillResource:
async def test_read_binary_content(self) -> None:
data = bytes([0x01, 0x02, 0x03, 0x04])
result = _make_blob_result(data)
resource = McpSkillResource(name="icon.bin", result=result)
resource = MCPSkillResource(name="icon.bin", result=result)
content = await resource.read()
assert content == data
@pytest.mark.asyncio
async def test_read_empty_returns_none(self) -> None:
result = _make_empty_result()
resource = McpSkillResource(name="empty", result=result)
resource = MCPSkillResource(name="empty", result=result)
content = await resource.read()
assert content is None
@@ -168,7 +168,7 @@ class TestMcpSkillResource:
TextResourceContents(uri=AnyUrl("skill://b"), text="line2", mimeType="text/plain"),
]
)
resource = McpSkillResource(name="multi", result=result)
resource = MCPSkillResource(name="multi", result=result)
content = await resource.read()
assert content == "line1\nline2"
@@ -185,23 +185,20 @@ class TestMcpSkillResource:
),
]
)
resource = McpSkillResource(name="mixed", result=result)
resource = MCPSkillResource(name="mixed", result=result)
content = await resource.read()
# 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.
# The implementation iterates all contents checking for BlobResourceContents
# first, so when both text and binary are present, binary is returned.
assert content == data
# ---------------------------------------------------------------------------
# McpSkill tests
# MCPSkill tests
# ---------------------------------------------------------------------------
class TestMcpSkill:
"""Tests for McpSkill."""
class TestMCPSkill:
"""Tests for MCPSkill."""
@pytest.mark.asyncio
async def test_get_content_fetches_and_caches(self) -> None:
@@ -209,7 +206,7 @@ class TestMcpSkill:
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)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client)
content1 = await skill.get_content()
content2 = await skill.get_content()
@@ -225,7 +222,7 @@ class TestMcpSkill:
from agent_framework import SkillFrontmatter
fm = SkillFrontmatter(name="empty-skill", description="Empty skill.")
skill = McpSkill(frontmatter=fm, skill_md_uri="skill://empty/SKILL.md", client=client)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://empty/SKILL.md", client=client)
with pytest.raises(ValueError, match="no text content"):
await skill.get_content()
@@ -241,7 +238,7 @@ class TestMcpSkill:
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)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client)
resource = await skill.get_resource("references/checklist.md")
assert resource is not None
@@ -260,7 +257,7 @@ class TestMcpSkill:
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)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client)
resource = await skill.get_resource("assets/icon.bin")
assert resource is not None
@@ -273,7 +270,7 @@ class TestMcpSkill:
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)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client)
resource = await skill.get_resource("references/does-not-exist.md")
assert resource is None
@@ -300,7 +297,7 @@ class TestMcpSkill:
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)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client)
resource = await skill.get_resource(name)
assert resource is None
@@ -312,7 +309,7 @@ class TestMcpSkill:
from agent_framework import SkillFrontmatter
fm = SkillFrontmatter(name="test-skill", description="Test.")
skill = McpSkill(frontmatter=fm, skill_md_uri="skill://test/SKILL.md", client=client)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://test/SKILL.md", client=client)
assert await skill.get_resource("") is None
assert await skill.get_resource(" ") is None
@@ -323,27 +320,27 @@ class TestMcpSkill:
from agent_framework import SkillFrontmatter
fm = SkillFrontmatter(name="test-skill", description="Test.")
skill = McpSkill(frontmatter=fm, skill_md_uri="skill://test/SKILL.md", client=client)
skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://test/SKILL.md", client=client)
assert await skill.get_script("anything") is None
def test_compute_skill_root_uri_strips_suffix(self) -> None:
assert McpSkill._compute_skill_root_uri("skill://unit-converter/SKILL.md") == "skill://unit-converter/"
assert MCPSkill._compute_skill_root_uri("skill://unit-converter/SKILL.md") == "skill://unit-converter/"
def test_compute_skill_root_uri_trailing_slash(self) -> None:
assert McpSkill._compute_skill_root_uri("skill://unit-converter/") == "skill://unit-converter/"
assert MCPSkill._compute_skill_root_uri("skill://unit-converter/") == "skill://unit-converter/"
def test_compute_skill_root_uri_no_suffix_adds_slash(self) -> None:
assert McpSkill._compute_skill_root_uri("skill://unit-converter") == "skill://unit-converter/"
assert MCPSkill._compute_skill_root_uri("skill://unit-converter") == "skill://unit-converter/"
# ---------------------------------------------------------------------------
# McpSkillsSource tests
# MCPSkillsSource tests
# ---------------------------------------------------------------------------
class TestMcpSkillsSource:
"""Tests for McpSkillsSource."""
class TestMCPSkillsSource:
"""Tests for MCPSkillsSource."""
@pytest.mark.asyncio
async def test_index_based_discovery_returns_skill(self) -> None:
@@ -353,7 +350,7 @@ class TestMcpSkillsSource:
"skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD),
}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert len(skills) == 1
@@ -367,7 +364,7 @@ class TestMcpSkillsSource:
@pytest.mark.asyncio
async def test_no_index_returns_empty(self) -> None:
client = _make_client() # No resources at all
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -378,7 +375,7 @@ class TestMcpSkillsSource:
client = _make_client(
**{"skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json")}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert len(skills) == 1
@@ -400,7 +397,7 @@ class TestMcpSkillsSource:
}
)
client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")})
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -419,7 +416,7 @@ class TestMcpSkillsSource:
}
)
client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")})
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -439,7 +436,7 @@ class TestMcpSkillsSource:
}
)
client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")})
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -458,7 +455,7 @@ class TestMcpSkillsSource:
}
)
client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")})
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -467,7 +464,7 @@ class TestMcpSkillsSource:
client = _make_client(
**{"skill://index.json": _make_text_result('{"skills": []}', uri="skill://index.json")}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -476,7 +473,7 @@ class TestMcpSkillsSource:
client = _make_client(
**{"skill://index.json": _make_text_result("not valid json", uri="skill://index.json")}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skills = await source.get_skills()
assert skills == []
@@ -489,7 +486,7 @@ class TestMcpSkillsSource:
"skill://unit-converter/references/checklist.md": _make_text_result("- check thing 1\n- check thing 2"),
}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skill = (await source.get_skills())[0]
resource = await skill.get_resource("references/checklist.md")
assert resource is not None
@@ -506,7 +503,7 @@ class TestMcpSkillsSource:
"skill://unit-converter/assets/icon.bin": _make_blob_result(data),
}
)
source = McpSkillsSource(client=client)
source = MCPSkillsSource(client=client)
skill = (await source.get_skills())[0]
resource = await skill.get_resource("assets/icon.bin")
assert resource is not None