Repository navigation
fix(skills): validate percent-encoded skill names - #1431
tsarlandie-oai wants to merge 4 commits into
Conversation
Codex Review Summary: ✅ CompletedThis 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. |
|
@jamadeo 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94982693ab
ℹ️ 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".
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks for chasing this down, @tsarlandie-oai!
jokemanfire
left a comment
There was a problem hiding this comment.
The parsing side looks right to me, and I checked the parts that are easy to get subtly wrong:
- the hyphen state machine rejects
-two,two-,two--wordsand accepts東京-٢, which is what the reference validator does; decode_utf8()makes invalid escapes (r%FFsum) fail closed asNameMismatchinstead of falling back to the raw segment;- normalizing both sides with NFKC handles the composed/decomposed pair (
re%CC%81sume%CC%81vsrésumé); validate_skillsnormalizes the URI before the duplicate check, so%72/ lowercase-hex / literal-Unicode aliases are caught.
Three questions before this lands:
-
Which string does the 64-character limit apply to?
is_valid_skill_namecounts characters after.nfkc(), while the stored name is the raw frontmatter string. A 100-character name that NFKC-composes below 64 would pass while the manifest still carries 100 characters — and any consumer that re-validates the raw string will disagree. Either count the rawchars()(and separately bound the normalized form), or state explicitly that the limit is on the normalized form. -
Three unconditional dependencies.
mod skillis not feature-gated, sopercent-encoding,unicode-general-category(a full Unicode table) andunicode-normalizationland on everyrmcpbuild, including servers that never touch skills. If that is intentional, fine — but it is a measurable binary/count bump, so it deserves a line in the PR description or a gate behind the skills feature. -
NFKC equality accepts aliases. With this change,
skill://cafe/SKILL.mdvalidates against a frontmatter name ofcafe. That is the reference validator's behavior, but the URI is what clients fetch and log, so a server can advertise a confusable spelling for a name clients will render as ASCII. If you want that closed, the cheap version is to require the decoded segment to equal the name exactly after canonicalization on the emitting side, rather than accepting any NFKC-equivalent spelling on the receiving side.
Minor: the InvalidName message says "1–64 characters" while the check is on the normalized form; align the wording with whichever answer (1) gets.
Motivation and context
Skill::validatecompares a URI segment with the UTF-8 name fromSKILL.mdfrontmatter. A skill namedcafétherefore fails validation withNameMismatchwhen a server advertisesskill://caf%C3%A9/SKILL.md. This came up while reviewing the filesystem catalog in #1409 and also affects servers that construct their own entries.Percent-decode the name segment once and compare it with the frontmatter using the NFKC form used by the Agent Skills reference validator. Enforce the 1–64 character limit and allow lowercase letters, letters from scripts without case, numbers, and single internal hyphens. Invalid names now produce a distinct validation error. Agent Skills naming rules
When checking a listing for duplicate entries, use the same percent-encoding normalization as the manifest checker. This catches equivalent spellings of a skill URI while preserving the advertised URI for reads.
Tests
NameMismatchonmainwith the new Unicode regression test. The fix accepts top-level and nested names with either hex case. Tests also cover invalid UTF-8, double encoding, and an encoded slash.DuplicateSkill. The regression test covers the hex case, an escaped unreserved character, and a literal Unicode name.cargo test -p rmcp --lib --no-default-featurespassed all 193 tests.cargo test -p rmcp --features client --test test_skillspassed all 14 tests.cargo clippy -p rmcp --lib --no-default-features -- -D warningsandcargo +nightly fmt --all -- --checkpassed.café/SKILL.mdappears under the encoded URI and returns the original Markdown throughread_resource.