fix(skills): require ttlMs and cacheScope on modern skills/get results - #2469
Merged
Merged
Conversation
#2404) The stable ext-skills spec settles the question SEP-2640 left open: GetSkillResult extends CacheableResult, so ttlMs and cacheScope are REQUIRED. Add ModernGetSkillEnvelopeSchema and select it from the negotiated era in InspectorClient.getSkillResult, mirroring skills/list. Legacy results stay permissive. Update the "left open" comments and the roadmap's open-gap note. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the specification and issue requirements with comprehensive era-aware coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Enforces stable ext-skills cache metadata requirements for modern skills/get responses while preserving legacy compatibility.
Changes:
- Adds era-aware
skills/getresult validation. - Covers schema, client, and real-transport behavior.
- Updates roadmap documentation to close the validation gap.
| File | Description |
|---|---|
core/mcp/skillsSchemas.ts |
Adds the modern cacheable envelope schema. |
core/mcp/inspectorClient.ts |
Selects the schema by negotiated era. |
clients/web/src/test/core/mcp/skillsSchemas.test.ts |
Tests modern and legacy schema rules. |
clients/web/src/test/core/mcp/inspectorClient-skills.test.ts |
Tests client validation and returned metadata. |
clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts |
Verifies cache fields over real transports. |
docs/inspector-roadmap-2026-h2.md |
Marks the validation gap as resolved. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Copilot review loop closed: round 1 came back clean ("Findings: None", no inline comments, no suppressed block), so I'm stopping on the first clean round per pr-flow step 7c. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2404
What
The stable ext-skills spec settles the question SEP-2640 left open:
GetSkillResultextendsCacheableResult, sottlMsandcacheScopeare REQUIRED onskills/get, as they are onresources/read. Before this change the Inspector accepted a modernskills/getresult without them, so a non-conforming server was reported as clean.Changes
core/mcp/skillsSchemas.tsaddsModernGetSkillEnvelopeSchema: the{ skill }envelope plusttlMs(non-negative integer) andcacheScope(public|private). These are the same field shapes asModernListSkillsResultSchema. The legacyGetSkillEnvelopeSchemastays permissive. I rewrote the "left open" / "not era-aware, settled" comments, including the module-level note on why onlyresources/directory/readhas no modern variant.core/mcp/inspectorClient.ts:getSkillResultnow picks its schema from the negotiated era, the same waylistSkillsdoes. The SDK codec checks and liftsresultType(Inspector never declares the skills extension in its client capabilities #2373) but never checks the caching attributes of a consumer-owned method, so this schema is the only thing that validates them. When a modern result is rejected, it is still attributed to its Protocol entry through the existing decode-rejection path.ttlMsof-1or0.5, rejects an unknowncacheScope, and still requires the envelope.getSkillResultreturns the fields whole.docs/inspector-roadmap-2026-h2.md: the two notes that listed this as an open gap now describe it as closed.The skills fixture server already sends
ttlMs: 0, cacheScope: "public"onskills/get, so the modern integration leg passes unchanged.No UI change, so no screenshots.
npm run local:gateis green.🤖 Generated with Claude Code