modified the docs and added check for de-dup across pages

Author: vijay

docs/advanced/skills.md

@@ -84,7 +84,12 @@ a `ReadResourceResult` (text or blob contents) and validates nothing itself —
     never grant the frontmatter's `allowed-tools` field (or any other permission-widening field)
     without explicit per-skill user approval.
 
-    Both are host responsibilities the SDK cannot discharge for you — read the SEP's
+    A digest match confirms the *bytes*, not the *frontmatter*: it doesn't prove the
+    `frontmatter` the server advertised in `skills/get` matches the frontmatter inside the fetched
+    `SKILL.md`. If you act on `skill.frontmatter` — especially `allowed-tools` — parse the fetched
+    file and compare its frontmatter yourself.
+
+    These are host responsibilities the SDK cannot discharge for you — read the SEP's
     [Security Implications](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640)
     section before building a host on top of this extension.
 

src/mcp/client/skills.py

@@ -94,25 +94,33 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil
 
         Raises:
             ValueError: If the server doesn't advertise the Skills extension, its
-                response is not SEP-2640 conformant, or it repeats a pagination cursor.
+                response is not SEP-2640 conformant, it repeats a pagination
+                cursor, or it lists the same skill URI on two pages.
             MCPError: If the server returns an error response.
         """
         self._require_extension()
         base = params if params is not None else ListSkillsParams()
         cursor = base.cursor
         skills: list[Skill] = []
         seen_cursors: set[str] = {cursor} if cursor is not None else set()
+        seen_uris: set[str] = set()
         while True:
             # `send_request` parses each page into `ListSkillsResult`, whose validators reject a
             # non-conformant skill or a duplicate URI — no separate conformance call is needed.
             page = await self._session.send_request(
                 ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult
             )
+            # A server stuck repeating its cursor is a loop; catch that before the content checks.
+            if page.next_cursor is not None and page.next_cursor in seen_cursors:
+                raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}")
+            # Per-page validation can't catch a URI repeated *across* pages, so track that here.
+            for skill in page.skills:
+                if skill.uri in seen_uris:
+                    raise ValueError(f"server listed skill {skill.uri!r} on more than one skills/list page")
+                seen_uris.add(skill.uri)
             skills.extend(page.skills)
             if page.next_cursor is None:
                 return skills
-            if page.next_cursor in seen_cursors:
-                raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}")
             seen_cursors.add(page.next_cursor)
             cursor = page.next_cursor
 
@@ -156,25 +164,32 @@ async def read_directory(self, uri: str, params: ReadDirectoryParams | None = No
 
         Raises:
             ValueError: If the server doesn't advertise the `directoryRead`
-                setting, its response is not a valid child listing of `uri`, or
-                it repeats a pagination cursor.
+                setting, its response is not a valid child listing of `uri`, it
+                repeats a pagination cursor, or it lists the same child on two pages.
             MCPError: If the server returns an error response.
         """
         self._require_extension(directory_read=True)
         base = params if params is not None else ReadDirectoryParams(uri=uri)
         cursor = base.cursor
         resources: list[Resource] = []
         seen_cursors: set[str] = {cursor} if cursor is not None else set()
+        seen_uris: set[str] = set()
         while True:
             page = await self._session.send_request(
                 ReadDirectoryRequest(params=base.model_copy(update={"uri": uri, "cursor": cursor})),
                 ReadDirectoryResult,
             )
+            # A server stuck repeating its cursor is a loop; catch that before the content checks.
+            if page.next_cursor is not None and page.next_cursor in seen_cursors:
+                raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}")
             validate_directory_result(uri, page)
+            # `validate_directory_result` dedupes within a page; catch a child repeated across pages.
+            for resource in page.resources:
+                if resource.uri in seen_uris:
+                    raise ValueError(f"server listed child {resource.uri!r} on more than one directory page")
+                seen_uris.add(resource.uri)
             resources.extend(page.resources)
             if page.next_cursor is None:
                 return resources
-            if page.next_cursor in seen_cursors:
-                raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}")
             seen_cursors.add(page.next_cursor)
             cursor = page.next_cursor

src/mcp/server/skills.py

@@ -74,6 +74,12 @@ class Skills(Extension):
     setting and serves `resources/directory/read`; omitting it advertises
     neither. Handlers run per request, so a catalog that changes over time
     (or is too large to enumerate) can return a partial or empty listing.
+
+    Handler error contract: raise `MCPError` to return a specific error to the
+    caller (e.g. `INVALID_PARAMS` from `get_skill` for a URI it doesn't serve).
+    Any other failure — a raised exception, or a result that isn't SEP-2640
+    conformant — is treated as a server fault and reported as `INTERNAL_ERROR`,
+    never the caller's bad params.
     """
 
     identifier = EXTENSION_ID

src/mcp/shared/skills.py

@@ -16,6 +16,32 @@
 from pydantic import AfterValidator, BaseModel, ConfigDict, Field, model_validator
 from pydantic.alias_generators import to_camel
 
+__all__ = [
+    "EXTENSION_ID",
+    "METHOD_LIST",
+    "METHOD_GET",
+    "METHOD_READ_DIRECTORY",
+    "MAX_RESOURCES_PER_SKILL",
+    "MAX_TOTAL_SIZE",
+    "Frontmatter",
+    "SkillResource",
+    "SkillResources",
+    "Skill",
+    "ListSkillsParams",
+    "ListSkillsResult",
+    "GetSkillParams",
+    "GetSkillResult",
+    "ReadDirectoryParams",
+    "ReadDirectoryResult",
+    "ListSkillsRequest",
+    "GetSkillRequest",
+    "ReadDirectoryRequest",
+    "skill_name_from_uri",
+    "parse_directory_uri",
+    "validate_directory_result",
+    "verify_skill_resource",
+]
+
 EXTENSION_ID = "io.modelcontextprotocol/skills"
 """The Skills extension identifier, advertised under `ServerCapabilities.extensions`."""
 
@@ -83,8 +109,9 @@ class Skill(_SkillModel):
     SEP-2640 conformance is intrinsic: constructing (or parsing) a `Skill`
     validates the frontmatter `name`/`description`, and — unless `resources` is
     `"dynamic"` — that every entry names a file within the skill's own directory,
-    with no duplicates and `SKILL.md` present. The 512-entry/16-MiB limits are
-    SHOULD NOT thresholds, not MUST NOT, so an over-limit manifest is accepted.
+    with no duplicates and `SKILL.md` present. The `MAX_RESOURCES_PER_SKILL`
+    (512-entry) and `MAX_TOTAL_SIZE` (16-MiB) limits are SHOULD NOT thresholds,
+    not MUST NOT, so an over-limit manifest is accepted.
     """
 
     uri: str
@@ -287,16 +314,16 @@ def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None:
 
 
 def verify_skill_resource(skill: Skill, uri: str, content: bytes) -> None:
-    """Verify `content` (the bytes read from `uri`) against `skill`'s held manifest.
+    """Check that `content` (the bytes read from `uri`) matches `skill`'s manifest entry.
 
-    Implements the SEP-2640 Integrity and verification requirement: a host
-    MUST verify a retrieved file's bytes against its manifest entry before
-    using them. Not applicable to a skill whose `resources` is `"dynamic"`,
-    which offers no digest to verify against.
+    Recomputes the size and SHA-256 digest of `content` and compares them to the
+    entry `skill` holds for `uri` — the byte-integrity check SEP-2640 requires
+    before a host trusts a fetched file. A `"dynamic"` skill carries no digests,
+    so it has nothing to verify against.
 
     Raises:
         ValueError: If `uri` is not one of `skill`'s resources, `skill.resources`
-            is `"dynamic"`, or `content` does not match the entry's `size`/`digest`.
+            is `"dynamic"`, or `content`'s size or digest doesn't match the entry.
     """
     if skill.resources == "dynamic":
         raise ValueError(f"skill {skill.uri!r} has dynamic resources and cannot be integrity-verified")

tests/client/test_skills.py

@@ -160,6 +160,21 @@ async def test_list_skills_raises_on_a_server_that_repeats_its_cursor() -> None:
             await skills.list_skills()
 
 
+async def test_list_skills_rejects_a_skill_repeated_across_pages() -> None:
+    """SEP-2640 pagination: a page's own validation dedupes within that page, so the client also
+    rejects a skill URI the server hands back on a second page rather than returning it twice."""
+
+    async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult:
+        if params.cursor is None:
+            return ListSkillsResult(skills=[_skill()], next_cursor="page-2")
+        return ListSkillsResult(skills=[_skill()], next_cursor=None)
+
+    server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)])
+    async with _skills(server) as skills:
+        with pytest.raises(ValueError, match="more than one skills/list page"):
+            await skills.list_skills()
+
+
 async def test_list_skills_requires_the_extension_to_be_advertised() -> None:
     """No `Skills` extension at all: the server never advertises `io.modelcontextprotocol/skills`."""
     async with _skills(MCPServer("plain")) as skills:
@@ -230,6 +245,24 @@ async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> No
             await skills.read_directory("skill://git-workflow/references")
 
 
+async def test_read_directory_rejects_a_child_repeated_across_pages() -> None:
+    """The client dedupes directory children across pages, not just within a single page."""
+    child = Resource(uri="skill://git-workflow/references/A.md", name="A.md")
+
+    async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult:
+        if params.cursor is None:
+            return ReadDirectoryResult(resources=[child], next_cursor="page-2")
+        return ReadDirectoryResult(resources=[child], next_cursor=None)
+
+    server = MCPServer(
+        "catalog",
+        extensions=[Skills(list_skills=_paginated_list_handler(), get_skill=_get_skill, read_directory=handler)],
+    )
+    async with _skills(server) as skills:
+        with pytest.raises(ValueError, match="more than one directory page"):
+            await skills.read_directory("skill://git-workflow/references")
+
+
 async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() -> None:
     server = MCPServer("catalog", extensions=[_NonConformantGetSkill()])
     async with _skills(server) as skills:

tests/server/test_skills.py

@@ -271,6 +271,7 @@ async def test_skills_get_rejects_a_skill_mutated_after_the_result_is_built() ->
 
     async def mutating_get(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult:
         result = GetSkillResult(skill=_git_workflow_skill())
+        assert isinstance(result.skill.resources, list)
         result.skill.resources.clear()
         return result