Repository navigation
fix(skills): enforce manifest boundaries and capability negotiation - #1412
Conversation
github-actions
Bot
added
T-test
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. |
|
@gocamille 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 757e5b30c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let path = relative.split(['?', '#']).next().unwrap_or_default(); | ||
| if path.is_empty() || path.ends_with('/') { |
There was a problem hiding this comment.
When a manifest contains skill://acme/billing/references/. (or references/sub/.., including percent-encoded forms), this check does not see a trailing slash and the loop finishes with positive depth, so Skill::validate() accepts it. URI dot-segment normalization turns each into .../references/, which is a directory rather than the file a SkillResource promises; this leaves malformed handler manifests passing the new validation. Reject terminal normalized ./.. segments or otherwise require the normalized path to end in a file segment.
Useful? React with 👍 / 👎.
DaleSeo
left a comment
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks for the fix, @tsarlandie-oai ! Approving with a non-blocking suggestion.
Follow-up to #1392.
Skill::validaterequires each static manifest URI to identify a file under the skill root. It rejects sibling directories, paths that escape the root, and paths ending in a literal or percent-encoded dot segment. Duplicate detection compares normalized file paths. A manifest may listrefs/../invoice.mdon its own, but listing it alongsideinvoice.mdreturnsDuplicateResource. The comparison preserves interior empty segments, encoded separators, query strings, and fragments. Resource reads use the original advertised URI. Bothskills/listandskills/getreturn-32603for an invalid handler manifest. Skills manifest requirements-32601forskills/listandskills/getuntil the server declares the extension. One duplex transport test checks that those helpers andlist_all_skillssend zero bytes when peer information or the extension declaration is missing. Skills capability negotiationValidation
cargo +stable test -p rmcp --features client --lib --test test_skillspassed all 286 library tests and 14 Skills integration tests.cargo +stable test -p rmcp --no-default-features --libpassed all 188 tests.cargo +nightly fmt --all -- --checkpassed.-D warnings -A clippy::result_large_err. The allowance covers 13 diagnostics also present on the base commit with Rust 1.99.1.