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. |
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.