Repository navigation
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. |
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>
|
There are some changes that are unrelated to the skills SEP, can you remove them from this PR to allow an easier review?
|
… resource checks Accept opt-in SkipInvalidSkills with diagnostics, require resource capabilities, remove configurable client caps, and register custom methods automatically. Update against main and remove unrelated conformance, documentation, and x/sync classification changes from the protocol PR.
|
Addressed the scope cleanup from 4 October in 21ad3c2: removed the conformance runner/fixture, CONTRIBUTING edits, unrelated auth/extauth bullets, docs intro/pagination edits, and x/sync reclassification. Only Skills documentation and its YAML dependency remain. Updated #1240 to include this protocol revision while preserving the filesystem provider; the helper-only comparison is now seven files. Both branches preserve their previous heads without force-pushing. Replied separately to all four current review threads. Full Go/race checks and standard + Skills conformance pass. |
| cs.client.mu.Lock() | ||
| defer cs.client.mu.Unlock() | ||
| return cs.client.sendMethods | ||
| return maps.Clone(cs.client.sendMethods) |
There was a problem hiding this comment.
I'd rather add copy on write. Writes are a lot less frequent
| // | ||
| // The method must have been registered on the session's client via | ||
| // [AddSendingCustomMethod]. | ||
| // The method is registered automatically on first use. An existing registration |
There was a problem hiding this comment.
Why is this change necessary? I'd rather we require AddSendingCustomMethod and reconsider ergonomics on the next major release instead of changing a core method behaviour now
|
|
||
| To list a server's resources and resource templates, use the | ||
| [`ClientSession.Resources`](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk/mcp#ClientSession.Resources) | ||
| [`ClientSession.Resource`](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk/mcp#ClientSession.Resource) |
| *resultType = resultTypeComplete | ||
| } | ||
| if cache.CacheScope == "" { | ||
| cache.CacheScope = cacheScopePublic |
| ) | ||
|
|
||
| // ListSkillsHandler handles skills/list requests. | ||
| type ListSkillsHandler func(context.Context, *mcp.ServerSession, *ListSkillsParams) (*ListSkillsResult, error) |
There was a problem hiding this comment.
this will make it hard to add new fields and doesn't have request extra access, let't do similar to core methods
type (
ListSkillsRequest = mcp.ServerRequest[*ListSkillsParams]
GetSkillRequest = mcp.ServerRequest[*GetSkillParams]
ReadDirectoryRequest = mcp.ServerRequest[*ReadDirectoryParams]
)
type ListSkillsHandler func(context.Context, *ListSkillsRequest) (*ListSkillsResult, error)| // A nil params starts at the first page. Each page is validated as in [List]. | ||
| // The session, options, and parameters are captured when All is called. | ||
| // The iterator stops after yielding its first error. | ||
| func All(ctx context.Context, session *mcp.ClientSession, params *ListSkillsParams, options ...ListOptions) iter.Seq2[*Skill, error] { |
There was a problem hiding this comment.
nit: I'd also have skills.List instead of skills.All for consistency with other methods like ListResources
| } | ||
|
|
||
| // ValidateDirectoryResult validates that result contains direct children of uri. | ||
| func ValidateDirectoryResult(uri string, result *ReadDirectoryResult) error { |
There was a problem hiding this comment.
Is there really a demand for ValidateSkill, ValidateSkillWithLimits, ValidateDirectoryResult to be exported? should SDK just do this and return an error if validation fails? exported API in the public SDK is a commitment and I'd rather wait for someone to ask for these before adding them
| URI: uri, MIMEType: "text/markdown", Text: content, | ||
| }}}, nil | ||
| }) | ||
| err := skills.AddHandlers(server, &skills.Handlers{ |
There was a problem hiding this comment.
can add only export skills.Add() similar to how the core package is doing and handle getters automatically? this way we won't need to export PaginateSkills
| // so that clients observe them during capability negotiation. If settings is | ||
| // nil, an empty object is advertised. The settings map is copied shallowly; | ||
| // nested maps, slices, and pointers must not be modified after the call. | ||
| func (s *Server) AddExtension(name string, settings map[string]any) { |
There was a problem hiding this comment.
I'd probably suggest we don't add this method. mostly because skills should eventually be packaged as modelcontextprotocol/modelcontextprotocol#3371 and will not require this helper
| // VerifySkillMD verifies SKILL.md with [VerifyResource] and compares every | ||
| // frontmatter field with the held entry. For a dynamic manifest it still checks | ||
| // frontmatter, returning [ErrDynamicResources] only if the frontmatter matches. | ||
| func VerifySkillMD(skill *Skill, content []byte) error { |
There was a problem hiding this comment.
AI says this will fail on "yes|no" decoded into a boolean as well as dates like "2024-01-01" decoded into time.Time. please add test cases
This branch has not been deployed
4 participants
Summary
Add opt-in Go support for the Skills extension: skills/list, skills/get, optional resources/directory/read, manifests, pagination, and content verification.
This is PR 1 of 2. Draft #1240 now includes this revision and preserves the optional filesystem provider. Its helper-only comparison contains only filesystem code, tests, and documentation.
Review changes
Compatibility and scope
On protocol 2026-07-28 and later, list/get include required cache hints and all three methods return resultType: complete. The compatibility backport omits these fields on earlier protocols. Verification remains lazy and checks manifest membership, sizes, digests, and all frontmatter fields. Entries remain scoped to their originating server.
Removed the conformance runner changes and fixture, CONTRIBUTING edits, unrelated auth/extauth bullets, documentation intro/pagination changes, and x/sync reclassification. The only added dependency is gopkg.in/yaml.v3, used by Skills frontmatter verification. Skills documentation sources and generated files are updated together.
Validation
Both protocol and helper trees passed full Go tests and Skills race tests. Automatic registration concurrency and capability snapshots have MCP regression/race coverage. Vet, build, formatting, and generated documentation were checked; the helper worktree required disabling VCS stamping for vet/build.
Standard conformance: 40 server checks and 239 client checks passed, zero failures.
Skills scenarios were rerun using conformance #330's chore/sep-2640-yaml branch at c4d79493f8013f18a7eb0bbe8aef584cff7b31aa, with temporary fixtures outside this PR:
Zero failures or warnings. Legacy enumeration also has two version-conditioned skips.