feat(skill-quality): enforce frontmatter name matches skill directory

[kyle-sexton]

Closes #712

Summary

docs/PLUGIN-PHILOSOPHY.md:69 requires the frontmatter name to match the skill directory name. Nothing enforced it — check 1 asserted only that name: was present and non-empty. This closes the hole.

Fix

plugins/skill-quality/scripts/check-skill.sh — check 1 gains a deterministic FAIL when frontmatter name differs from the containing directory:

CUR_NAME="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field name <<<"$FRONTMATTER")")"
if [[ -n "$CUR_NAME" && "$CUR_NAME" != "$SKILL_NAME" ]]; then
  err "frontmatter name '$CUR_NAME' does not match skill directory '$SKILL_NAME'"
fi

The -n guard keeps an absent name reporting only the existing missing-name failure rather than a spurious second one, and reuses the established strip_quotes/field helpers so a quoted value compares unquoted. Header check list updated to match.

check-skill.test.sh — two new cases: a mismatching fixture must fail with the directory named in the message, and a quoted matching fixture must not trip the gate (the false-positive direction, which also proves quote stripping reaches the comparison).

Blast radius is why this is worth stating plainly: the directory name is what Claude Code namespaces the skill by, so a divergent frontmatter name silently relocates the invocation the doctrine says the skill has. Because the picker labels rows by the resolved leaf name (see #710), the drift would not surface in the listing either — there is no way to notice it by eye.

Verification

Landing this blocking rather than as a warning required proving the catalog already conforms, otherwise it would turn CI red on unrelated PRs. It does:

checked 144; name!=dir violations: 0

No baseline file, no grandfathering, no stale-guard needed.

Self-test — 15/15, including the two new cases:

$ bash plugins/skill-quality/scripts/check-skill.test.sh
ok   - --help exits 0
ok   - well-formed skill passes
...
ok   - frontmatter name mismatching the directory fails
ok   - quoted name matching the directory does not trip the gate
all assertions passed

Other gates run locally: shellcheck on both changed scripts — clean. scripts/check-changelog-parity.sh --check — "Every versioned plugin has a CHANGELOG.md". markdownlint-cli2 on the CHANGELOG — 0 errors.

Version 0.5.0 → 0.6.0 with a matching ## [0.6.0] heading, satisfying --check-bump.

Related

• Refs docs(naming): namespacing guarantees invocation uniqueness, not picker display uniqueness #710 — the doctrine clarification that surfaced this enforcement hole.
• Refs program: shadowed-skill-renames contract (naming grammar rollout) #280 — the naming-grammar rollout that established the name/directory rule.

[claude]

Claude finished @kyle-sexton's task in 2m 17s —— View job

---

PR Review: feat(skill-quality): enforce frontmatter name matches skill directory

Tasks

• Gather context and understand the PR
• Read changed files
• Review implementation for correctness and edge cases
• Review test coverage
• Check CHANGELOG and versioning
• Post findings

---

Overall Assessment

Solid, well-scoped enforcement patch. The logic is correct, the -n guard is appropriately defensive, and the PR ships a complete evidence trail (catalog scan, self-test output). Ready to merge with one minor test-coverage note.

---

check-skill.sh — Implementation

Correctness ✓

The new block at check-skill.sh:166-169 is correct:

CUR_NAME="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field name <<<"$FRONTMATTER")")"
if [[ -n "$CUR_NAME" && "$CUR_NAME" != "$SKILL_NAME" ]]; then
  err "frontmatter name '$CUR_NAME' does not match skill directory '$SKILL_NAME'"
fi

• The -n guard correctly prevents a spurious second error when name: is absent or blank — skill_frontmatter::field name returns empty in that case, and strip_quotes "" stays empty.
• strip_quotes is called on the outer result rather than inside field, which is the same pattern used for description at line 176 — consistent.
• The err call uses printf 'FAIL: %s\n' "$*" (via err()), so CUR_NAME lands in $*, not the format string. No printf injection risk.

Pre-existing parser quirk (not introduced here)

name: # comment passes grep -qE '^name:[[:space:]]*\S' (because # is \S), and the awk field extractor returns # comment as the value. With this PR that would now generate a spurious mismatch error. This is a limitation of the non-full-YAML-parser design, not something this PR makes worse, and skill name keys with inline YAML comments are contrived enough to not matter in practice. Just noting for awareness.

---

check-skill.test.sh — Test Coverage

Case 14 — mismatch fails ✓
Lines 350–372: asserts rc=1 and that the directory name appears in the error message. Both conditions together prevent a false-pass if the check fires for a different reason. Good.

Case 15 — double-quoted matching name passes ✓
Lines 374–395: validates the false-positive direction and confirms strip_quotes reaches the comparison.

Minor gap — single-quoted YAML name not tested
Case 15 exercises name: "quoted-name" (double-quoted YAML). strip_quotes also handles name: 'quoted-name' (single-quoted YAML) via the second branch of its if/elif, but that path is not exercised in the new cases. The function is shared across multiple callers and is already proven to work by the description/when_to_use checks, so this is low risk — but a single-quoted fixture would give complete quote-stripping coverage for the name-matching gate specifically. Fix this →

---

plugin.json — Version Bump

0.5.0 → 0.6.0 ✓
A new deterministic FAIL condition is a user-visible behavior change; minor bump is correct.

---

CHANGELOG.md

Follows the existing format (no release dates on entries, consistent with all prior versions). Entry is detailed and includes the blast-radius rationale, which is valuable. ✓

---

Summary

One minor test-coverage gap (single-quoted YAML name path in case 15). Everything else — the guard logic, quote stripping, error message fidelity, version bump, and catalog pre-check — is correct and well-justified. Safe to merge as-is.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 691a77ddd6

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 2m 37s —— View job

---

Tasks

• Gather context and understand the PR
• Read changed files at HEAD
• Review implementation for correctness and edge cases
• Review test coverage
• Check CHANGELOG and versioning
• Post findings

---

PR Review: feat(skill-quality): enforce frontmatter name matches skill directory

This PR landed in two commits. The first (691a77d) added the name/directory enforcement and initial test cases 14–15. The second (b97bd36) addressed the Codex feedback about trailing YAML comments on the scalar and added test cases 16–17. Both commits are reviewed together here.

---

check-skill.sh — Implementation

Correctness of the comment-stripping block ✓

RAW_NAME="$(skill_frontmatter::field name <<<"$FRONTMATTER")"
RAW_NAME="${RAW_NAME%%[[:space:]]#*}"
RAW_NAME="${RAW_NAME%"${RAW_NAME##*[![:space:]]}"}"
CUR_NAME="$(skill_frontmatter::strip_quotes "$RAW_NAME")"
if [[ -n "$CUR_NAME" && "$CUR_NAME" != "$SKILL_NAME" ]]; then
  err "frontmatter name '$CUR_NAME' does not match skill directory '$SKILL_NAME'"
fi

(check-skill.sh:166–176)

• %%[[:space:]]#* removes the longest suffix beginning with whitespace-then-#. Because skill names are kebab-case per spec (no spaces, no #), the pattern cannot consume name characters — the design assumption is sound and is documented inline.
• The ${var%"${var##*[![:space:]]}"} idiom is the standard bash rstrip idiom. Correct.
• Comment stripping happens before strip_quotes, which is the correct order: "quoted-name" # comment → "quoted-name" → quoted-name. Reversing the order would leave a dangling quote.
• The -n guard on CUR_NAME continues to prevent a spurious second error when name: is absent or blank. ✓
• err calls printf 'FAIL: %s\n' "$*", so CUR_NAME lands in $* and never reaches the format string. No printf injection risk. ✓

Pre-existing parser quirk (not introduced by this PR)

name: #comment (no space before #) passes the grep -qE '^name:[[:space:]]*\S' regex, so field name returns #comment. The %%[[:space:]]#* pattern requires a space before # and would not match, leaving CUR_NAME="#comment" — which then fires the mismatch error rather than the missing-name error. This is the same inline-comment edge case called out by both the prior Claude review and Codex. It is not made worse by this PR, and name: #... is contrived enough not to matter in practice.

---

check-skill.test.sh — Test Coverage

Four cases added in this PR:

#	What	Direction
14	name: some-other-name in misnamed-skill/	Fail path: directory name appears in error message
15	name: "quoted-name" in quoted-name/	Pass path: double-quoted match doesn't trip gate
16	name: commented-name # kept for the migration note in commented-name/	Pass path: trailing comment is stripped
17	name: wrong-name # with a comment too in commented-bad/	Fail path: comment stripping doesn't mask real mismatch

Cases 16 and 17 directly address the Codex P2 finding. Both the false-positive direction (16) and the false-negative direction (17) are verified — that's the right pair to test. ✓

Minor remaining gap — single-quoted YAML name

strip_quotes handles both "double" and 'single' via separate branches (skill-frontmatter.sh:62–70). The double-quoted path is exercised by case 15; the single-quoted path is not exercised for the name-matching gate specifically. The function is already proven for single quotes by the description/when_to_use checks, so the risk is low, but it is still a gap. Fix this →

No test for quoted-name-with-trailing-comment combination

name: "quoted-name" # comment — comment stripped first to "quoted-name", then unquoted to quoted-name. The combination is not tested, but the two halves are independently covered (case 15 proves unquoting, cases 16/17 prove comment stripping), so this is a very low-risk gap.

---

plugin.json — Version Bump

0.5.0 → 0.6.0 ✓. A new deterministic FAIL condition is user-visible behavior; minor bump is appropriate.

---

CHANGELOG.md

The [0.6.0] entry covers the enforcement logic and its rationale but does not mention trailing YAML comment stripping (added in the second commit). The omission is minor — comment stripping is an implementation detail of making the comparison robust rather than a separate feature — but the phrase "A quoted value is unquoted before comparison" could be expanded to "A quoted value is unquoted and a trailing YAML comment is stripped before comparison" to be complete.

Otherwise the entry is detailed and includes the blast-radius rationale, which is valuable for reviewers. ✓

---

Summary

The Codex P2 finding (false positive on name: foo # comment) has been correctly addressed in the second commit with a well-chosen pattern that respects the kebab-case constraint on skill names. The logic is sound, the test matrix covers all four critical directions (mismatch, quoted match, commented match, commented mismatch), and the version bump is appropriate.

Two minor notes remain from the prior review, both low-risk: a single-quoted YAML name test case, and a CHANGELOG line that could mention comment stripping. Neither is blocking. Safe to merge as-is.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b97bd362d1

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 3m 6s —— View job

---

Tasks

• Gather context and understand the PR
• Read changed files at HEAD (7870b2d)
• Review implementation for correctness and edge cases
• Review test coverage
• Check CHANGELOG and versioning
• Post findings

---

PR Review: feat(skill-quality): enforce frontmatter name matches skill directory

This review covers the full three-commit PR at HEAD (7870b2d). The prior two Claude reviews covered commits 691a77d and b97bd36; this review focuses on the third commit and assesses the PR as a whole.

---

Commit 3 (7870b2d) — Kebab-Case Constraint Gate

What changed: Added an if/elif guard that validates CUR_NAME against ^[a-z0-9]+(-[a-z0-9]+)*$ before the directory comparison. An escape sequence like "escaped\x2dname" — which strip_quotes leaves as the literal string escaped\x2dname — now reports as a charset defect rather than a confusing directory mismatch.

Correctness of the regex ✓

check-skill.sh:180:

if [[ -n "$CUR_NAME" && ! "$CUR_NAME" =~ ^[a-z0-9]+(-[a-z0-9]+)*$ ]]; then

• Requires at least one leading alphanumeric → correctly rejects leading hyphens
• Each hyphen-group requires at least one following alphanumeric → correctly rejects trailing hyphens and consecutive hyphens (--)
• escaped\x2dname contains \, which is outside [a-z0-9-] → fires the charset error ✓

If/elif flow ✓

All four cases are correctly handled:

CUR_NAME	Charset valid?	Matches dir?	Outcome
empty	—	—	silent (existing missing-name error already fired)
non-empty	✗	—	is not kebab-case error
non-empty	✓	✗	does not match skill directory error
non-empty	✓	✓	clean

Pre-existing quirk now resolved

Prior reviews flagged that name: #comment (no space before #) wasn't stripped by %%[[:space:]]#* and would land as CUR_NAME="#comment". Under commits 1–2 this would have fired a spurious mismatch error. Under commit 3, #comment fails the charset gate and reports "is not kebab-case" — a more accurate diagnosis. Not a regression, actually an improvement.

---

Test Coverage — Case 16b

check-skill.test.sh:420–442

The fixture correctly creates a directory named escaped-name (valid kebab-case) with frontmatter name: "escaped\x2dname". After strip_quotes removes the outer double-quotes, the literal \x2d fails the charset regex. The assertion checks both rc=1 and grep -q 'is not kebab-case' — the dual check is the right design, since rc=1 alone would pass even if the mismatch error had fired instead.

Complete test matrix for the name-matching gate across all three commits:

Case	Direction	What
14	Fail	bare name mismatch
15	Pass	double-quoted match
16	Pass	trailing comment stripped (match)
16b	Fail	YAML escape → charset defect, not mismatch
17	Fail	comment stripped but real mismatch still fires

Both false-positive and false-negative directions are covered for each new mechanism. ✓

Remaining gap — single-quoted YAML name

Case 15 exercises name: "double-quoted". The strip_quotes single-quote branch (which handles name: 'single-quoted') remains untested in the name-matching gate. Proven via description/when_to_use checks elsewhere so risk is low — but the gap persists across all three commits. Fix this →

---

CHANGELOG — Incomplete Entry

CHANGELOG.md:17:

A quoted value is unquoted before comparison

This line was written with commit 2 and not updated in commit 3. The [0.6.0] entry now understates what the gate actually does — it also strips trailing YAML comments (commit 2) and rejects names that aren't kebab-case (commit 3). A consumer upgrading to 0.6.0 with a skill named name: "escaped\x2d" would hit the charset error with no CHANGELOG hint that such a gate exists.

Suggested addition to the entry:

A quoted value is unquoted, trailing YAML comments are stripped, and the name must conform to the Agent Skills spec charset (lowercase alphanumerics and hyphens) — a YAML escape sequence is reported as a charset defect rather than a spurious directory mismatch.

Fix this →

---

plugin.json — Version Bump

0.5.0 → 0.6.0 ✓. All three commits ship together as one user-visible version; the minor bump is appropriate.

---

Summary

The third commit cleanly closes the Codex P2 finding (YAML escape sequences). Constraining the accepted charset rather than decoding YAML in bash is the correct trade — well-justified in the commit message and the inline comment. The logic is correct, the test matrix covers all four critical directions, and the overall PR delivers a sound enforcement of the name-matches-directory doctrine.

Two minor notes carry over: the single-quoted name test case, and a CHANGELOG entry that doesn't reflect comment stripping or the kebab-case constraint added in commits 2–3. Neither is blocking. Safe to merge as-is.