Revert "Verify skill resources through the client extension"

This reverts commit 5e4fb31.

Author: Kludex

docs/advanced/skills.md

@@ -66,17 +66,17 @@ These three validate the server's response against the SEP-2640 conformance rule
 it. A name that doesn't match its URI, a digest in the wrong shape, or an incomplete manifest raises
 `ValueError` rather than reaching your code.
 
-`read_skill_resource(skill, uri)` fetches a file and verifies it against the manifest in the `Skill`
-entry you pass. It returns a `ReadResourceResult` with text or blob content only after the URI,
-size, and SHA-256 digest match. Keep that entry while you act on the skill so every file is checked
-against the same manifest.
+`read_skill_uri` is the exception. It's a thin, discoverable alias for `resources/read` that returns
+a `ReadResourceResult` (text or blob contents) and validates nothing itself — that's the next step.
 
 !!! tip
-    `read_skill_resource` checks the bytes returned by `resources/read` against the held manifest.
-    A `"dynamic"` skill carries no digests, so this method raises `ValueError` for one. You can use
-    `Client.read_resource` to fetch dynamic content, but it cannot be integrity-verified.
-    If you fetch or cache bytes outside this method, `mcp.shared.skills.verify_skill_resource`
-    performs the same check on those bytes.
+    `verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256
+    digest — against the entry you hold for it. Call it after `read_skill_uri` and *before* you
+    treat the content as trustworthy.
+
+    `resources/read` returns whatever bytes the server sends *right now*; verification is what ties
+    those bytes back to the manifest you already validated. It applies to a static manifest only —
+    a `"dynamic"` skill carries no digests, so calling it on one raises `ValueError`.
 
 !!! warning
     Skill content is untrusted model input, exactly like any other server-provided text. SEP-2640

docs_src/skills/tutorial001_client.py

@@ -2,6 +2,7 @@
 
 from mcp import Client
 from mcp.client.skills import Skills
+from mcp.shared.skills import verify_skill_resource
 from mcp.types import TextResourceContents
 
 
@@ -12,9 +13,10 @@ async def main() -> None:
             print(skill.uri, skill.frontmatter["description"])
 
         skill = await catalog.get_skill("skill://git-workflow/SKILL.md")
-        result = await catalog.read_skill_resource(skill, skill.uri)
+        result = await catalog.read_skill_uri(skill.uri)
         content = result.contents[0]
         if isinstance(content, TextResourceContents):
+            verify_skill_resource(skill, skill.uri, content.text.encode())
             print(content.text)
 
 

src/mcp/client/skills.py

@@ -3,7 +3,7 @@
 `Skills` is an opt-in [`ClientExtension`](../advanced/extensions.md) for talking
 to a skills catalog server. Register it with `Client(extensions=[Skills()])`,
 then call `client.extension(Skills)` for the SEP-2640 verbs — `list_skills`, `get_skill`,
-`read_skill_resource`, and `read_directory` — tied to that connection:
+`read_skill_uri`, and `read_directory` — tied to that connection:
 
     async with Client("http://localhost:8000/mcp", extensions=[Skills()]) as client:
         for skill in await client.extension(Skills).list_skills():
@@ -13,15 +13,13 @@
 `get_skill`, and `read_directory` — check that the server advertises the
 extension and validate its response; `list_skills` and `read_directory` follow
 `nextCursor` to completion, so one call returns every page's results.
-`read_skill_resource` reads one file and verifies it against the held `Skill` entry.
+`read_skill_uri` is a thin `resources/read` alias that does neither — verify its
+result with `verify_skill_resource`.
 """
 
 from __future__ import annotations
 
-import base64
-import binascii
-
-from mcp_types import ReadResourceResult, Resource, TextResourceContents
+from mcp_types import ReadResourceResult, Resource
 
 from mcp.client.extension import ClientExtension
 from mcp.client.session import ClientSession
@@ -38,7 +36,6 @@
     ReadDirectoryResult,
     Skill,
     validate_directory_result,
-    verify_skill_resource,
 )
 
 __all__ = ["BoundSkills", "Skills"]
@@ -66,8 +63,9 @@ class BoundSkills:
     `get_skill`, and `read_directory` — check that the server advertises the
     extension and validate its response against the SEP-2640 conformance rules
     before returning; `list_skills` and `read_directory` also follow
-    `nextCursor` to completion. `read_skill_resource` verifies a fetched file
-    against the held skill's manifest.
+    `nextCursor` to completion. `read_skill_uri` is the exception: a thin
+    `resources/read` alias that neither checks advertisement nor validates —
+    pair it with `verify_skill_resource`.
     """
 
     def __init__(self, session: ClientSession) -> None:
@@ -138,37 +136,20 @@ async def get_skill(self, uri: str) -> Skill:
             raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}")
         return result.skill
 
-    async def read_skill_resource(self, skill: Skill, uri: str) -> ReadResourceResult:
-        """Read and verify one file against a held `Skill` entry.
+    async def read_skill_uri(self, uri: str) -> ReadResourceResult:
+        """Read a skill file's content via `resources/read`.
 
-        Pass the entry obtained from `list_skills` or `get_skill`. A dynamic
-        skill has no manifest to verify; use `Client.read_resource` if you
-        choose to read its unverified content.
+        A thin, discoverable alias: works for any `skill://` (or other-scheme)
+        file regardless of whether the skill was ever enumerated. Verify the
+        result against a held `Skill` entry with `verify_skill_resource` before
+        treating it as trusted content — this call does not verify anything itself.
 
         Raises:
-            ValueError: If the file is not in the manifest, or the server's
-                content does not match its URI, size, or digest.
             MCPError: If the server returns an error response.
             RuntimeError: If the server returns an `InputRequiredResult`; this
-                method does not drive the input-required loop.
+                alias does not drive the input-required loop.
         """
-        if skill.resources == "dynamic":
-            raise ValueError(f"skill {skill.uri!r} has dynamic resources and cannot be integrity-verified")
-        if not any(resource.uri == uri for resource in skill.resources):
-            raise ValueError(f"{uri!r} is not in skill {skill.uri!r}'s held manifest")
-        result = await self._session.read_resource(uri)
-        if len(result.contents) != 1 or result.contents[0].uri != uri:
-            raise ValueError(f"resource {uri!r} did not return exactly one matching content item")
-        content = result.contents[0]
-        if isinstance(content, TextResourceContents):
-            data = content.text.encode("utf-8")
-        else:
-            try:
-                data = base64.b64decode(content.blob, validate=True)
-            except binascii.Error as exc:
-                raise ValueError(f"resource {uri!r} returned invalid base64 content") from exc
-        verify_skill_resource(skill, uri, data)
-        return result
+        return await self._session.read_resource(uri)
 
     async def read_directory(self, uri: str, params: ReadDirectoryParams | None = None) -> list[Resource]:
         """Call `resources/directory/read` for `uri`, following `nextCursor` to completion.

tests/client/test_skills.py

@@ -1,30 +1,20 @@
 """Tests for the client-side Skills extension (SEP-2640, `mcp.client.skills`)."""
 
-import base64
 import hashlib
 from collections.abc import AsyncIterator
 from contextlib import asynccontextmanager
 from typing import Any
 
 import pytest
-from inline_snapshot import snapshot
-from mcp_types import (
-    INVALID_PARAMS,
-    BlobResourceContents,
-    ReadResourceRequestParams,
-    ReadResourceResult,
-    Resource,
-    TextResourceContents,
-)
+from mcp_types import INVALID_PARAMS, Resource, TextResourceContents
 
 from mcp.client.client import Client
 from mcp.client.skills import BoundSkills
 from mcp.client.skills import Skills as ClientSkills
-from mcp.server import Server
 from mcp.server.context import ServerRequestContext
 from mcp.server.extension import Extension, MethodBinding
 from mcp.server.mcpserver import MCPServer
-from mcp.server.mcpserver.resources import FunctionResource, TextResource
+from mcp.server.mcpserver.resources import TextResource
 from mcp.server.skills import Skills
 from mcp.shared.exceptions import MCPError
 from mcp.shared.skills import (
@@ -38,6 +28,7 @@
     ReadDirectoryResult,
     Skill,
     SkillResource,
+    verify_skill_resource,
 )
 
 pytestmark = pytest.mark.anyio
@@ -205,117 +196,22 @@ async def test_get_skill_propagates_the_servers_unknown_skill_error() -> None:
     assert exc_info.value.code == INVALID_PARAMS
 
 
-async def test_read_skill_resource_returns_verified_text() -> None:
-    """SDK-defined: a matching manifest allows a fetched text file."""
+async def test_read_skill_uri_reads_the_registered_resource() -> None:
     async with _skills(_server()) as skills:
-        skill = await skills.get_skill(_SKILL_URI)
-        result = await skills.read_skill_resource(skill, _SKILL_URI)
+        result = await skills.read_skill_uri(_SKILL_URI)
     contents = result.contents[0]
     assert isinstance(contents, TextResourceContents)
     assert contents.text == _SKILL_CONTENT
 
 
-async def test_read_skill_resource_rejects_a_changed_file() -> None:
-    """SEP-2640: a host rejects content that differs from the held manifest."""
-    stale = Skill(
-        uri=_SKILL_URI,
-        frontmatter={"name": "git-workflow", "description": "d"},
-        resources=[SkillResource(uri=_SKILL_URI, digest="sha256:" + "0" * 64, size=len(_SKILL_CONTENT))],
-    )
-    async with _skills(_server()) as skills:
-        with pytest.raises(ValueError) as exc_info:
-            await skills.read_skill_resource(stale, _SKILL_URI)
-    assert str(exc_info.value) == snapshot(
-        "resource 'skill://git-workflow/SKILL.md' has digest "
-        "'sha256:7187b725da92d7447b6f50e21d0bf4f3b9db3387acb39239c4a4edc193ddcd4b', "
-        "expected 'sha256:0000000000000000000000000000000000000000000000000000000000000000'"
-    )
-
-
-async def test_read_skill_resource_rejects_an_unlisted_uri() -> None:
-    """SEP-2640: the host must not fetch a URI absent from the held manifest."""
-    async with _skills(_server()) as skills:
-        with pytest.raises(ValueError) as exc_info:
-            await skills.read_skill_resource(_skill(), "skill://git-workflow/unlisted.md")
-    assert str(exc_info.value) == snapshot(
-        "'skill://git-workflow/unlisted.md' is not in skill 'skill://git-workflow/SKILL.md''s held manifest"
-    )
-
-
-async def test_read_skill_resource_rejects_a_dynamic_skill() -> None:
-    """SDK-defined: a dynamic skill has no digest to verify against."""
-    dynamic = Skill(uri=_SKILL_URI, frontmatter={"name": "git-workflow", "description": "d"}, resources="dynamic")
+async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None:
+    """End-to-end: `skills/get`'s digest and `resources/read`'s bytes agree."""
     async with _skills(_server()) as skills:
-        with pytest.raises(ValueError) as exc_info:
-            await skills.read_skill_resource(dynamic, _SKILL_URI)
-    assert str(exc_info.value) == snapshot(
-        "skill 'skill://git-workflow/SKILL.md' has dynamic resources and cannot be integrity-verified"
-    )
-
-
-async def test_read_skill_resource_returns_verified_binary_content() -> None:
-    """SDK-defined: binary resources are decoded before digest verification."""
-    data = b"\x00\x01skill\xff"
-    server = MCPServer("catalog")
-    server.add_resource(FunctionResource(uri=_SKILL_URI, name="SKILL.md", fn=lambda: data))
-    skill = Skill(
-        uri=_SKILL_URI,
-        frontmatter={"name": "git-workflow", "description": "d"},
-        resources=[SkillResource(uri=_SKILL_URI, digest=f"sha256:{hashlib.sha256(data).hexdigest()}", size=len(data))],
-    )
-    async with _skills(server) as skills:
-        result = await skills.read_skill_resource(skill, _SKILL_URI)
+        skill = await skills.get_skill(_SKILL_URI)
+        result = await skills.read_skill_uri(_SKILL_URI)
     contents = result.contents[0]
-    assert isinstance(contents, BlobResourceContents)
-    assert contents.blob == base64.b64encode(data).decode("ascii")
-
-
-@pytest.mark.parametrize(
-    "contents",
-    [
-        [],
-        [TextResourceContents(uri="skill://other/SKILL.md", text=_SKILL_CONTENT)],
-        [TextResourceContents(uri=_SKILL_URI, text=_SKILL_CONTENT)] * 2,
-    ],
-)
-async def test_read_skill_resource_rejects_an_invalid_resource_response(
-    contents: list[TextResourceContents | BlobResourceContents],
-) -> None:
-    """SDK-defined: malformed peer responses cannot become verified content.
-
-    A low-level server is needed because `MCPServer` always produces conformant resource responses.
-    """
-
-    async def handler(ctx: ServerRequestContext[Any, Any], params: ReadResourceRequestParams) -> ReadResourceResult:
-        assert params.uri == _SKILL_URI
-        return ReadResourceResult(contents=contents)
-
-    server = Server("catalog")
-    server.add_request_handler("resources/read", ReadResourceRequestParams, handler)
-    async with Client(server, extensions=[ClientSkills()]) as client:
-        with pytest.raises(ValueError) as exc_info:
-            await client.extension(ClientSkills).read_skill_resource(_skill(), _SKILL_URI)
-    assert str(exc_info.value) == snapshot(
-        "resource 'skill://git-workflow/SKILL.md' did not return exactly one matching content item"
-    )
-
-
-async def test_read_skill_resource_rejects_invalid_base64() -> None:
-    """SDK-defined: a malformed binary response cannot become verified content.
-
-    A low-level server is needed because `MCPServer` always base64-encodes binary resources.
-    """
-
-    async def handler(ctx: ServerRequestContext[Any, Any], params: ReadResourceRequestParams) -> ReadResourceResult:
-        assert params.uri == _SKILL_URI
-        return ReadResourceResult(contents=[BlobResourceContents(uri=_SKILL_URI, blob="??")])
-
-    server = Server("catalog")
-    server.add_request_handler("resources/read", ReadResourceRequestParams, handler)
-    async with Client(server, extensions=[ClientSkills()]) as client:
-        with pytest.raises(ValueError) as exc_info:
-            await client.extension(ClientSkills).read_skill_resource(_skill(), _SKILL_URI)
-    assert str(exc_info.value) == snapshot("resource 'skill://git-workflow/SKILL.md' returned invalid base64 content")
+    assert isinstance(contents, TextResourceContents)
+    verify_skill_resource(skill, _SKILL_URI, contents.text.encode())
 
 
 async def test_read_directory_follows_next_cursor_to_completion() -> None:

tests/docs_src/test_skills.py

@@ -7,6 +7,7 @@
 from mcp import Client
 from mcp.client.skills import Skills
 from mcp.shared.exceptions import MCPError
+from mcp.shared.skills import verify_skill_resource
 
 pytestmark = pytest.mark.anyio
 
@@ -47,13 +48,14 @@ async def test_the_skill_file_is_served_as_an_ordinary_resource() -> None:
     assert contents.text == tutorial001.SKILL_MD
 
 
-async def test_read_skill_resource_returns_verified_content() -> None:
-    """tutorial001_client: a fetched file matches the held skill's manifest."""
+async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None:
+    """tutorial001_client: fetch the entry, read the file, verify the bytes against it -
+    the digest/size check `verify_skill_resource` performs."""
     skills = Skills()
     async with Client(tutorial001.mcp, extensions=[skills]) as client:
         catalog = client.extension(Skills)
         skill = await catalog.get_skill(tutorial001.SKILL_URI)
-        result = await catalog.read_skill_resource(skill, skill.uri)
+        result = await catalog.read_skill_uri(skill.uri)
     contents = result.contents[0]
     assert isinstance(contents, TextResourceContents)
-    assert contents.text == tutorial001.SKILL_MD
+    verify_skill_resource(skill, skill.uri, contents.text.encode())