fix(skills): a punctuated skill name must not cost the whole skill - #117
Merged
Conversation
Publishing spectr's toolkit dropped `intelligence:update` with `invalid_name`. The colon is the `/namespace:command` convention and only ever mattered to a DIRECTORY name — the skill keeps whatever its frontmatter says, and fromBundle() records the folder separately as `bundleName`. Losing a skill over its folder name is a far worse outcome than renaming the folder. Names are now folded to a safe directory (`intelligence:update` -> `intelligence-update`) instead of rejected. The original "reject totally, never sanitise" existed so `..`, `/` and `\` could never reach a path concatenation. That guarantee is KEPT, and the distinction is made explicit: a PATH is never a name. Anything carrying a separator or traversal sequence is still rejected outright, because laundering `../../etc/passwd` into a tidy `etc` folder would accept a hostile value under a clean-looking name — worse than refusing it. Only genuinely punctuated names reach the sanitiser, which is an allowlist (`[^a-z0-9]+` -> `-`) and still validates against NAME_PATTERN before returning, so a non-conforming value cannot escape the method. Sanitising introduces a collision the old code could not have: two names can fold onto one directory. Writing both would silently overwrite the first — a dropped skill wearing a successful publish, the exact defect this programme keeps finding. Reported as `duplicate_directory_name` instead. Mutation-checked, and the FIRST attempt was vacuous: disabling only the `/` check left the `..` check catching the input, so the suite stayed green and proved nothing. Disabling the whole guard turns 2 tests red; restoring returns to green. 1424 tests OK, phpcs 0, phpstan 0, phpmd 0.
Contributor
Quality Report — ConductionNL/hermiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 117/117 | |||
| npm | ✅ | ✅ 657/657 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ |
Coverage: 85.3% (93/109 statements)
Quality workflow — 2026-08-02 11:44 UTC
Download the full PDF report from the workflow artifacts.
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.
Found while publishing spectr's toolkit:
intelligence:updatewas dropped from the bundle withinvalid_name.Why that was wrong
The colon is the
/namespace:commandconvention, and it only ever mattered to a directory name. The skill keeps whatever its frontmatter says, andfromBundle()records the folder separately asbundleName— the two are already distinct concepts. Losing a skill over its folder name is a far worse outcome than renaming the folder.Names are now folded to a safe directory (
intelligence:update→intelligence-update) instead of rejected.The security guarantee is kept, and sharpened
The original "reject totally, never sanitise" existed so
..,/and\could never reach a path concatenation. That still holds, and the distinction is now explicit:A path is never a name. Anything carrying a separator or traversal sequence is still rejected outright — laundering
../../etc/passwdinto a tidyetcfolder would accept a hostile value under a clean-looking name, which is worse than refusing it. Only genuinely punctuated names reach the sanitiser, which is an allowlist ([^a-z0-9]+→-) and still validates againstNAME_PATTERNbefore returning, so a non-conforming value cannot escape the method.A collision the old code could not have
Sanitising means two names can fold onto one directory. Writing both would silently overwrite the first — a dropped skill wearing a successful publish, the exact defect this programme keeps turning up. Reported as
duplicate_directory_nameinstead.Mutation check — and my first attempt was vacuous
Disabling only the
/check left the..check still catching the input, so the suite stayed green and proved nothing. Disabling the whole guard turns 2 tests red; restoring returns to green.1424 tests OK · phpcs 0 · phpstan 0 · phpmd 0.