skills: add SEP-2640 protocol support - #1238
Conversation
adc1ddf to
ca89d93
Compare
|
Woooot great to see this @sambhav. I ran the SEP-2640 conformance scenarios against this branch (conformance PR 330, the traceability extraction and server scenarios Il share soon for the skills extension). 41 checks, 0 failures.
Repro, pointing a minimal Got three tiny notes (none blocking): 1. 2. Two untestable 3. Possible doc gap dueto needing flags against this branch. The runner defaults to the draft stateless wire. here it asserts |
60744e4 to
36000a2
Compare
sambhav
changed the title
|
Thanks for running the SEP scenarios. The PR is now split so this one contains only the generic protocol layer. I addressed note 1 in the latest revision: skills/list now omits ttlMs and cacheScope before protocol version 2026-07-28, with regression coverage. Notes 2 and 3 make sense and do not require changes in this generic layer; the filesystem metadata behavior and usage notes will remain explicit in the follow-up helper PR. |
36000a2 to
db8cd34
Compare
|
I reran conformance PR 330 locally against the split filesystem helper. It caught a nested YAML mapping normalization bug ( |
|
@guglielmo-san 🙏 this is ready. There is another PR stacked on top which provides a neater abstraction and utility for skills with filesystems (#1240). Would appreciate it if we can land this one first and the other one as a quick follow. |
| if !ok { | ||
| return fmt.Errorf("skills: server advertised invalid extension settings") | ||
| } | ||
| enabled, _ := m["directoryRead"].(bool) |
There was a problem hiding this comment.
i would create a const for "directoryRead" as well
| ) | ||
|
|
||
| // AddClient registers the Skills extension methods that client may send. | ||
| func AddClient(client *mcp.Client) error { |
There was a problem hiding this comment.
I would rename it to AddMethods
| return &cloned | ||
| } | ||
|
|
||
| func validateListResponse(ctx context.Context, result *ListSkillsResult) error { |
There was a problem hiding this comment.
why this is defined on server?
also this is extremely similar to validateListResult, the two functions should be collapsed into one, using a wrapper for the additional operation needed on server side maybe
| if err := validateListResponse(ctx, result); err != nil { | ||
| return nil, fmt.Errorf("skills: server returned an invalid skills/list result: %w", err) |
There was a problem hiding this comment.
from server side it is possible to increase the limits, but not from client side as it is using the Defaults.. This means that client <-> server is possibly broken. If we want to allow the server to provide higher limits, then we should pass this option also from client
|
|
||
| // UnsafeOptions permits behavior that may not interoperate with conforming hosts. | ||
| type UnsafeOptions struct { | ||
| DisableDefaultValidation bool |
There was a problem hiding this comment.
I do not understand what is the advantage of disabling the defaultValidation. According to the spec is a must for both Client and Server
| if result == nil || result.Skill == nil { | ||
| return nil, fmt.Errorf("skills/get handler returned a nil skill") |
There was a problem hiding this comment.
If the URI does not identify a skill the server serves, the server MUST return error -32602 (Invalid params
| if parentPath == "" { | ||
| prefix = "/" | ||
| } |
There was a problem hiding this comment.
i think this is dead code?
Replace the repeated result-type and cache-hint assignments in AddHandlers with a single stampEnvelope helper that also validates the hints it settles on, and route the protocol version, result type, and cache scope through named constants instead of repeated literals. paginate now skips the clone-and-sort when the catalog is already ordered, and resolves a cursor with a binary search rather than a linear scan. allPages allocates its seen set only once a server hands out a second cursor, and rejects a cursor equal to the initial one. parseSkillURI returns the parsed URL so validateResourceURI can reuse it instead of reparsing the skill root for every resource. mcp: factor the SDK default capabilities into defaultCapabilities so that AddExtension and capabilities cannot drift apart. conformance/skills-server: index skills by URI instead of rescanning the entry slice, and use path.Base for display names. scripts: factor the repeated argument check into require_value and simplify the work-directory setup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mcp/mrtr.go imports golang.org/x/sync/errgroup directly, so the "// indirect" marking was stale and `go mod tidy -diff` reported a pending change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Group the package tests by what they exercise: validation_test.go holds the pure unit tables, protocol_test.go the wire behavior, skills_test.go the shared helpers and API contracts, and limits_test.go the manifest caps. The limits matrix now calls ValidateSkillWithLimits directly instead of standing up a streamable HTTP server per combination, taking that test from 24 connections to 4. TestLimitsArePlumbed keeps one end-to-end case per side to prove Client.Limits and ServerOptions.Limits reach validation. TestSpecErrorScenarios collects the cases that the sep-2640-skills-* conformance scenarios also cover behind a single server, and carries a note to delete it once those scenarios run in CI against ./conformance/skills-server. Statement coverage rises from 82.2% to 90.8%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Summary
Add opt-in Go support for the Skills extension:
skills/list,skills/get, optionalresources/directory/read, manifests, pagination, and content verification.This is PR 1 of 2, independently usable. Filesystem discovery and caching remain in draft #1240, which needs to adopt these interfaces when rebased.
Limits and validation
The spec's 512-file / 16-MiB values are an interoperability baseline: hosts must support at least that much and may support more; servers should stay within it for broad compatibility. They are not mandatory rejection thresholds.
Peter Alexander's clarification (Anthropic): In our “Size limits and dynamic content” thread in
skills-over-mcp-wg, Peter said:He explained that clients may support larger skills, servers should stay within the baseline for broad compatibility, and dynamic-content budgets should be enforced by applications according to their own disk, context, and network constraints.
Client.LimitsandServerOptions.Limitsare plainskills.Limitsvalues. The zero value imposes no count or size caps. Positive fields opt into exact caps; negative values are rejected before registration or client requests.skills.BaselineLimits()returns a fresh value containing the spec's baseline. Applications can select it explicitly or supply numeric values to pin their own policy. This replaces the earlier implicit defaults, limit pointers, and exported default constants.ValidateSkillperforms structural validation only.ValidateSkillWithLimitsadditionally applies the supplied caps.API and behavior
skills.AddMethods(client)before connecting, then use&skills.Client{Session: session}.List,Get, andAlldiscover entries;ReadDirectoryandDirectoryEntriesprovide optional browsing.skills.AddHandlers(server, handlers, options)and serve bytes through ordinary resource or resource-template handlers. Registration advertises the extension; resource registration advertises the required base capability.(nil, nil), producing-32602. Ordinary handler failures and invalid results produce-32603. Explicit JSON-RPC codes, messages, and data survive wrapping."dynamic". Name validation supports Unicode and counts characters.VerifyResourcechecks membership, byte length, and digest.VerifySkillMDalso compares every frontmatter field. JSON numbers usejson.Number; equivalent decimal representations compare without rounding large integers. Dynamic frontmatter must still match before returningErrDynamicResources.2026-07-28and later, list/get include required cache hints and all three methods returnresultType: "complete". Zero TTL and public scope match repository defaults. Earlier protocols are supported as a compatibility backport with those fields omitted.Entries remain scoped to their originating server. Applications own approvals, caches, and execution policy; directory results do not expand a held manifest.
Go conventions and documentation
The package uses concrete types, context-first handlers,
mcp.ParamsBase/mcp.ResultBase, anditer.Seq2. It builds on existing custom-method and resource APIs, following the applicable patterns in MCP Apps, the repository's custom-method example, andauth/extauth. The only added core API isServer.AddExtension; dependency types are not exposed.The list-validation duplication is already consolidated:
validateListResponsewas removed, and client and server both usevalidateListResultinskills/validation.go. The server handler wraps it with output copying, nil-slice normalization, protocol envelope preparation, and error translation.Client/server docs include the executable example. Sources and generated files are updated together.
Validation
Validated locally against conformance #330's current branch,
chore/sep-2640-yaml, checked at73ac2c4d0f40505fbd597c23399aebd7545900ed:Zero failures or warnings. Skills conformance runs locally through the existing
scripts/server-conformance.sh; the custom Skills script has been removed. CI integration will follow after conformance #330 merges. No fork URL or revision is committed in the runner or CI configuration.The runner accepts
--conformance_repo <path|url>and--conformance_ref <branch|commit|tag>. An explicit revision uses a temporary clone, preserving the source checkout. The default remains the latest npm release and the existing conformance server.--server,--stateless, and arguments after--select the fixture, transport, and conformance options.To reproduce the modern enumeration run locally:
Repeat with
sep-2640-skills-manifestandsep-2640-skills-directory, using separate result directories. For the legacy runs, omit--statelessand use--spec-version 2025-11-25.Runner checks also pass for branch, commit, tag, repository URL, and local checkout inputs; paths and arguments containing spaces; cleanup; and failure exit-code propagation.
Local checks pass:
go test ./...,go test -race ./skills,go vet ./...,go build ./..., and documentation generation. Regression tests cover opt-in caps, unlimited totals, dynamic manifests, configuration ownership, required sizes, Unicode names, escaped markers, numeric fidelity, and error codes/data on both protocol versions.