Repository navigation
Conversation
github-actions
Bot
added
T-test
|
@DaleSeo (assignment failed) is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| /// | ||
| /// Same shape as `resources/list`, so the untagged [`ServerResult`] carries | ||
| /// it as [`ServerResult::ListResourcesResult`]. | ||
| pub type ReadResourceDirectoryResult = ListResourcesResult; |
There was a problem hiding this comment.
Alias, not a new struct. The spec's directory result has the same schema as ListResourcesResult. With the untagged ServerResult, decoding would never choose a separate struct, which is the same kind of issue as #771. So read_resource_directory returns the existing ListResourcesResult variant.
| if !declared { | ||
| return Err(McpError::method_not_found::<M>()); | ||
| } | ||
| if capabilities.resources.is_none() { |
There was a problem hiding this comment.
There are two different failures. If skills aren't declared, it's -32601, as specified for undeclared methods. If skills are declared without resources, it's -32603 because the extension serves its files through resources/read. That means the server configuration is wrong, and the client can't fix it.
| } | ||
| ClientRequest::GetSkillRequest(request) => { | ||
| validate_skills_capability::<GetSkillRequestMethod, _>(self, false)?; | ||
| if skill_name_from_uri(&request.params.uri).is_none() { |
There was a problem hiding this comment.
If a URI can't point to a SKILL.md file, it's rejected with -32602 before the handler runs. That means the handler only sees plausible URIs. After it runs, the returned entry must match the requested URI. If it doesn't, that's a handler bug, so it's reported as -32603.
| ) -> Result<ReadResourceDirectoryResult, ServiceError> { | ||
| let declared = self | ||
| .peer_info() | ||
| .is_some_and(|info| info.capabilities.supports_directory_read()); |
There was a problem hiding this comment.
The spec says clients must not call resources/directory/read unless the server has declared directoryRead. Otherwise, the call fails locally with -32601 and never reaches the server. The test checks how many calls the server receives to prove it.
| /// Settings that do not parse count as declared with the defaults. | ||
| pub fn skills(&self) -> Option<SkillsCapability> { | ||
| let settings = self.extensions.as_ref()?.get(super::SKILLS_EXTENSION_ID)?; | ||
| Some(serde_json::from_value(Value::Object(settings.clone())).unwrap_or_default()) |
There was a problem hiding this comment.
Lenient on purpose here. If the settings object doesn't parse, the extension is still considered declared, and we use the defaults, with directoryRead set to false. That way, skills keep working, and we don't call the optional method. I'm happy to make this stricter if reviewers prefer.
| base64 = { version = "0.23", optional = true } | ||
|
|
||
| # SHA-256 for SEP-2640 skill digests and SEP-2322 requestState sealing | ||
| sha2 = "0.11" |
There was a problem hiding this comment.
sha2 is now a regular dependency instead of an optional one. Clients that verify downloaded skill files shouldn't need filesystem or server features to do that. It's pure Rust and was already in the dependency tree for request-state, which now uses it without dep:.
Motivation and Context
SEP-2640 serves Agent Skills through the Resources primitive, so an MCP server can ship workflow instructions alongside its tools. This PR adds the Skills extension,
io.modelcontextprotocol/skills, withskills/list,skills/get, and the optionalresources/directory/readmethods on both the server and client sides. It only adds the protocol surface. A filesystem catalog will come in a second PR.How Has This Been Tested?
Added tests
Breaking Changes
None
Types of changes
Checklist