fix(validation): accept any-case SKILL.md filename on upload#460
Open
XiaoSeS wants to merge 1 commit into
Open
fix(validation): accept any-case SKILL.md filename on upload#460XiaoSeS wants to merge 1 commit into
XiaoSeS wants to merge 1 commit into
Conversation
Users packaging skills on case-insensitive filesystems frequently submit
`skill.md` (lowercase). All five `equals("SKILL.md")` call sites then fail
with "Missing required file: SKILL.md at root".
Fold the basename to the canonical `SKILL.md` inside
`SkillPackagePolicy.normalizeEntryPath` — the single point both the
validator and the archive extractor route every path through — so every
downstream comparison succeeds without further changes.
There was a problem hiding this comment.
Code Review
This pull request introduces automatic canonicalization of the SKILL.md filename to ensure case-insensitive matching. It includes logic in SkillPackagePolicy to fold case variants (like skill.md or Skill.MD) to the uppercase SKILL.md and adds comprehensive unit tests in SkillPackageArchiveExtractorTest and SkillPackageValidatorTest to verify this behavior across different directory structures and validation scenarios. I have no feedback to provide.
|
PR Review Helper seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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 #458.
Summary
skill.md(lowercase, or any other case variant) previously failed withMissing required file: SKILL.md at root. This was a frequent papercut on macOS / Windows because their case-insensitive filesystems happily let authors save the file with non-canonical casing.SKILL_MD_PATH = "SKILL.md"using case-sensitive.equals(...).SKILL.mdinsideSkillPackagePolicy.normalizeEntryPath(...)— the single normalization point bothSkillPackageValidatorandSkillPackageArchiveExtractoralready route every path through. Every downstream comparison then succeeds without further changes.The OpenSkills protocol declares uppercase
SKILL.mdas the canonical filename, so accepting any case but storing the canonical form is the right semantic — not a workaround.Test plan
SkillPackageArchiveExtractorTest— 3 new regression tests: lowercaseskill.mdat root, mixed-caseSkill.MDinside a single root directory, lowercaseskill.mdrequiring directory promotion. 18/18 pass.SkillPackageValidatorTest— 1 new test covering the validator-only path (e.g., CLI dry-run that doesn't go through the extractor). 17/17 pass.SkillPublishControllerTest,CliDryRunValidateTest,BasicPrePublishValidatorTest,SkillPublishServiceTest— no regressions. 27 tests, 0 failures.