removed separate skill validations as per review suggestion

Author: vijay

src/mcp/client/skills.py

@@ -38,8 +38,6 @@
     ReadDirectoryResult,
     Skill,
     validate_directory_result,
-    validate_list_result,
-    validate_skill,
 )
 from mcp.shared.skills import verify_skill_resource as verify_skill_resource
 
@@ -105,10 +103,11 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil
         skills: list[Skill] = []
         seen_cursors: set[str] = {cursor} if cursor is not None else 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
             )
-            validate_list_result(page)
             skills.extend(page.skills)
             if page.next_cursor is None:
                 return skills
@@ -130,10 +129,11 @@ async def get_skill(self, uri: str) -> Skill:
                 for a URI it does not serve.
         """
         self._require_extension()
+        # Parsing `GetSkillResult` validates the skill's own conformance; the requested-URI match
+        # is the one rule the skill body can't carry, so it stays an explicit check here.
         result = await self._session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult)
         if result.skill.uri != uri:
             raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}")
-        validate_skill(result.skill)
         return result.skill
 
     async def read_skill_uri(self, uri: str) -> ReadResourceResult:

src/mcp/server/skills.py

@@ -35,6 +35,7 @@ async def get_skill(ctx, params):
 from mcp_types import CacheableResult
 from mcp_types.jsonrpc import INTERNAL_ERROR, INVALID_PARAMS
 from mcp_types.version import MODERN_PROTOCOL_VERSIONS
+from pydantic import ValidationError
 
 from mcp.server.context import HandlerResult, ServerRequestContext
 from mcp.server.extension import Extension, MethodBinding
@@ -53,8 +54,6 @@ async def get_skill(ctx, params):
     parse_directory_uri,
     skill_name_from_uri,
     validate_directory_result,
-    validate_list_result,
-    validate_skill,
 )
 
 __all__ = ["Skills"]
@@ -103,25 +102,27 @@ def methods(self) -> Sequence[MethodBinding]:
         return bindings
 
     async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> HandlerResult:
-        result = await self._list_skills(ctx, params)
+        # `ListSkillsResult`/`Skill` self-validate on construction, so a handler that builds a
+        # non-conformant listing raises `ValidationError` here — a server fault, surfaced as an
+        # Internal error rather than the framework's default Invalid params for a bad body.
         try:
-            validate_list_result(result)
-        except ValueError:
+            result = await self._list_skills(ctx, params)
+        except ValidationError:
             logger.exception("list_skills handler returned an invalid result")
             raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None
         return _finalize_cacheable(result, ctx.protocol_version)
 
     async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> HandlerResult:
         _require_skill_md_uri(params.uri)
-        result = await self._get_skill(ctx, params)
-        if result.skill.uri != params.uri:
-            logger.error("get_skill handler returned %r for requested %r", result.skill.uri, params.uri)
-            raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result")
+        # `Skill` self-validates on construction (see `_handle_list`).
         try:
-            validate_skill(result.skill)
-        except ValueError:
+            result = await self._get_skill(ctx, params)
+        except ValidationError:
             logger.exception("get_skill handler returned an invalid result")
             raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None
+        if result.skill.uri != params.uri:
+            logger.error("get_skill handler returned %r for requested %r", result.skill.uri, params.uri)
+            raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result")
         return _finalize_cacheable(result, ctx.protocol_version)
 
     async def _handle_read_directory(

src/mcp/shared/skills.py

@@ -9,11 +9,11 @@
 
 import hashlib
 import re
-from typing import Any, Literal
+from typing import Annotated, Any, Literal
 from urllib.parse import urlsplit
 
 from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource
-from pydantic import BaseModel, ConfigDict
+from pydantic import AfterValidator, BaseModel, ConfigDict, Field, model_validator
 from pydantic.alias_generators import to_camel
 
 EXTENSION_ID = "io.modelcontextprotocol/skills"
@@ -27,8 +27,8 @@
 """SEP-2640 per-skill resource-count threshold (`SKILL.md` included).
 
 A SHOULD NOT limit, not a hard cap: the spec requires a host to support skills
-*up to and including* 512 entries and permits it to support larger ones, so
-`validate_skill` does not reject an over-count manifest."""
+*up to and including* 512 entries and permits it to support larger ones, so a
+`Skill` does not reject an over-count manifest."""
 
 MAX_TOTAL_SIZE = 16 * 1024 * 1024
 """SEP-2640 per-skill total-byte-size threshold (16 MiB), summed over `resources[].size`.
@@ -40,6 +40,12 @@
 _DIGEST_RE = re.compile(r"^sha256:[0-9a-f]{64}$")
 
 
+def _check_digest(value: str) -> str:
+    if not _DIGEST_RE.fullmatch(value):
+        raise ValueError(f"invalid SHA-256 digest {value!r}; expected 'sha256:' + 64 lowercase hex characters")
+    return value
+
+
 class _SkillModel(BaseModel):
     """Base for Skills value types: matches `mcp_types`' internal `MCPModel` config.
 
@@ -51,12 +57,16 @@ class _SkillModel(BaseModel):
 
 
 class SkillResource(_SkillModel):
-    """One file in a skill's manifest: `{uri, digest, size}`."""
+    """One file in a skill's manifest: `{uri, digest, size}`.
+
+    Shape rules are intrinsic: a `digest` that isn't `sha256:` + 64 lowercase
+    hex characters, or a negative `size`, is rejected at construction.
+    """
 
     uri: str
-    digest: str
+    digest: Annotated[str, AfterValidator(_check_digest)]
     """SHA-256 digest of the file's raw bytes, formatted `sha256:{64 hex chars}`."""
-    size: int
+    size: Annotated[int, Field(ge=0)]
     """Length in bytes of the file's raw content."""
 
 
@@ -68,13 +78,49 @@ class SkillResource(_SkillModel):
 
 
 class Skill(_SkillModel):
-    """An entry returned by `skills/list` or `skills/get`."""
+    """An entry returned by `skills/list` or `skills/get`.
+
+    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.
+    """
 
     uri: str
     """Resource URI of the skill's `SKILL.md`."""
     frontmatter: Frontmatter
     resources: SkillResources
 
+    @model_validator(mode="after")
+    def _check_conformance(self) -> Skill:
+        name = skill_name_from_uri(self.uri)
+        frontmatter_name = self.frontmatter.get("name")
+        if (
+            not isinstance(frontmatter_name, str)
+            or not _NAME_RE.fullmatch(frontmatter_name)
+            or len(frontmatter_name) > 64
+        ):
+            raise ValueError(f"skill {self.uri!r} frontmatter name must be 1-64 lowercase, digits, or hyphens")
+        if frontmatter_name != name:
+            raise ValueError(
+                f"skill {self.uri!r} frontmatter name {frontmatter_name!r} does not match URI name {name!r}"
+            )
+        description = self.frontmatter.get("description")
+        if not isinstance(description, str) or not (1 <= len(description) <= 1024):
+            raise ValueError(f"skill {self.uri!r} frontmatter description must contain 1 to 1024 characters")
+        if self.resources == "dynamic":
+            return self
+        seen: set[str] = set()
+        for resource in self.resources:
+            _validate_resource_uri_in_skill(self.uri, resource.uri)
+            if resource.uri in seen:
+                raise ValueError(f"skill {self.uri!r} lists resource {resource.uri!r} more than once")
+            seen.add(resource.uri)
+        if self.uri not in seen:
+            raise ValueError(f"skill {self.uri!r} resources does not include its own SKILL.md")
+        return self
+
 
 class ListSkillsParams(PaginatedRequestParams):
     """Parameters for `skills/list`."""
@@ -83,6 +129,9 @@ class ListSkillsParams(PaginatedRequestParams):
 class ListSkillsResult(PaginatedResult, CacheableResult):
     """Result of `skills/list`.
 
+    Each skill self-validates; on top of that, constructing (or parsing) this
+    result rejects two entries that share a `uri`.
+
     `ttl_ms`/`cache_scope` are SEP-2549 fields inherited from `CacheableResult`;
     unlike a core spec method, nothing sieves them off the wire for a
     pre-2026-07-28 connection automatically (see `mcp.server.skills`), so
@@ -91,6 +140,15 @@ class ListSkillsResult(PaginatedResult, CacheableResult):
 
     skills: list[Skill]
 
+    @model_validator(mode="after")
+    def _check_unique_uris(self) -> ListSkillsResult:
+        seen: set[str] = set()
+        for skill in self.skills:
+            if skill.uri in seen:
+                raise ValueError(f"skills/list result lists skill {skill.uri!r} more than once")
+            seen.add(skill.uri)
+        return self
+
 
 class GetSkillParams(RequestParams):
     """Parameters for `skills/get`."""
@@ -183,60 +241,6 @@ def _validate_resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None:
         raise ValueError(f"resource URI {resource_uri!r} contains a traversal segment")
 
 
-def validate_skill(skill: Skill) -> None:
-    """Validate `skill` against the SEP-2640 and Agent Skills conformance rules.
-
-    Checks the frontmatter's `name`/`description` fields, that `resources` (when
-    not `"dynamic"`) is complete — every entry names a file within the skill's
-    own directory, has a well-formed digest, and `SKILL.md` is present. The
-    512-entry/16-MiB limits are SEP-2640 SHOULD NOT thresholds, not MUST NOT, so
-    an over-limit manifest is accepted (a conforming host must support up to the
-    limits and may support larger).
-
-    Raises:
-        ValueError: If `skill` violates any of the above.
-    """
-    name = skill_name_from_uri(skill.uri)
-    frontmatter_name = skill.frontmatter.get("name")
-    if not isinstance(frontmatter_name, str) or not _NAME_RE.fullmatch(frontmatter_name) or len(frontmatter_name) > 64:
-        raise ValueError(f"skill {skill.uri!r} frontmatter name must be 1-64 lowercase, digits, or hyphens")
-    if frontmatter_name != name:
-        raise ValueError(f"skill {skill.uri!r} frontmatter name {frontmatter_name!r} does not match URI name {name!r}")
-    description = skill.frontmatter.get("description")
-    if not isinstance(description, str) or not (1 <= len(description) <= 1024):
-        raise ValueError(f"skill {skill.uri!r} frontmatter description must contain 1 to 1024 characters")
-
-    if skill.resources == "dynamic":
-        return
-    resources = skill.resources
-    seen: set[str] = set()
-    for resource in resources:
-        _validate_resource_uri_in_skill(skill.uri, resource.uri)
-        if resource.uri in seen:
-            raise ValueError(f"skill {skill.uri!r} lists resource {resource.uri!r} more than once")
-        seen.add(resource.uri)
-        if not _DIGEST_RE.fullmatch(resource.digest):
-            raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has an invalid SHA-256 digest")
-        if resource.size < 0:
-            raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has a negative size")
-    if skill.uri not in seen:
-        raise ValueError(f"skill {skill.uri!r} resources does not include its own SKILL.md")
-
-
-def validate_list_result(result: ListSkillsResult) -> None:
-    """Validate every skill in `result.skills` and reject duplicate URIs.
-
-    Raises:
-        ValueError: If any skill is invalid, or two entries share a `uri`.
-    """
-    seen: set[str] = set()
-    for skill in result.skills:
-        validate_skill(skill)
-        if skill.uri in seen:
-            raise ValueError(f"skills/list result lists skill {skill.uri!r} more than once")
-        seen.add(skill.uri)
-
-
 def parse_directory_uri(uri: str) -> tuple[str, str, str]:
     """Split a directory resource URI into `(scheme, netloc, path)`.