fix: rephrase colon in thurview SKILL.md description to fix YAML frontmatter parsing - #24
Merged
Merged
Conversation
`Two kinds: ` inside the unquoted `description` scalar reads as a mapping key nested under `description`, so every skill installer refused the file with "Nested mappings are not allowed in compact mappings". An em-dash says the same thing and leaves no colon for the next edit to trip over. The suite now loads every SKILL.md frontmatter with a real YAML parser, and asserts the broken shape is rejected, so the next one fails here instead of at a user's install. CI's `code` filter gains `skills/**/SKILL.md` so a skill-only edit still runs it. Claude-Session: https://claude.ai/code/session_017RnFWXocJXkVezJKUvJi4H
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.
Intent
A user is blocked:
npx skills add Thurbeen/thurview --skill thurviewrefuses to install the skill with 'Skipped .../skills/thurview/SKILL.md - YAML parse error: Nested mappings are not allowed in compact mappings'. The cause was already diagnosed before this work started, and re-deriving it was out of scope: in skills/thurview/SKILL.md's frontmatter the unquoteddescriptionscalar containsTwo kinds:- a colon followed by a space, which YAML reads as a mapping key nested underdescription.The goal: make the frontmatter load again WITHOUT losing what the description says. The 'two kinds' wording arrived with the code-explainer work (#22) and naming both document kinds - the review and the code explainer - is the whole point of that rewrite. Shortening the description back to the old one-kind wording would be a regression, not a fix. So the deliberate choice was to rephrase the colon away (
Two kinds - a review ... and a code explainer ..., em-dash) rather than quote the scalar: quoting leaves a scalar the next editor can break the same way, and the value already contains double quotes so the quoting would have to be single.argument-hintwas already correctly quoted and was deliberately left untouched.The test is the actual deliverable, not the one-line fix: nothing in this repo noticed that a SKILL.md frontmatter had stopped being loadable YAML, which is the only reason this reached a user. test/skill-frontmatter.test.ts loads the frontmatter of every SKILL.md in the repo with the repo's real YAML parser (the
yamlpackage the CLI already uses) and asserts each names the skill; it also asserts the brokenTwo kinds:shape is rejected, so the check is proven to go red on the defect it exists to catch. It was written first and watched fail with the installer's exact message before the description was fixed. The description assertion pins that both kinds survive.Wiring, deliberately inside the existing gate rather than beside it: the suite is already the repo gate (
pnpm test, run by CI's 'check' job and by the prek hooks), but CI's dorny/paths-filter 'code' filter did not list skills/, so a skill-only edit would have skipped the very job that now guards it - exactly the hole this defect walked through. Added 'skills/**/SKILL.md' to that filter. No second gate was introduced.Deliberately NOT widened: a user is blocked right now, so anything else noticed goes in a report, not this branch. Only one SKILL.md exists in the repo and it was checked for any other unquoted ': '; there is none.
Verified: pnpm check green (51 tests), rumdl and shellcheck clean, and the fixed frontmatter also loads under python yaml.safe_load - the same load the installer's error came from. The full installer path (npx skills add) clones from GitHub and can only be exercised once this branch is pushed.
Note on provenance: the broken description came from the code-explainer work whose own task is still open elsewhere; that is known and no coordination is needed - the defect is already on main and this fixes main.
What Changed
descriptionscalar inskills/thurview/SKILL.mdfrontmatter, replacingTwo kinds:(a colon-space that YAML parses as a nested mapping key) withTwo kinds —(em-dash), preserving both named document kinds (review and code explainer) without quoting the scalar.test/skill-frontmatter.test.ts, which parses everySKILL.mdfrontmatter in the repo with theyamlpackage, asserts each has anameanddescription, asserts the thurview description still names both document kinds, and asserts the previously-brokenTwo kinds:shape is rejected by the parser.skills/**/SKILL.mdto thecodepath filter in.github/workflows/ci.ymlso skill-only edits still trigger the test job that now guards frontmatter validity.Risk Assessment
✅ Low: A minimal, well-scoped fix: the diff only rephrases the offending colon in SKILL.md's description (preserving the required "two kinds" content), adds a CI paths-filter entry so skill-only edits run the guarding suite, and adds a regression test that parses every SKILL.md's frontmatter with the same
yamlpackage the installer uses (a legitimate contract test since no in-repo code path actually parses SKILL.md frontmatter to call instead) and asserts the broken shape is rejected.Testing
Baseline
pnpm test && bats scripts/ci/*.batsalready passed; targeted rerun of the new test/skill-frontmatter.test.ts (4/4) confirmed the fixed SKILL.md parses and keeps both document kinds, and reverting just that file reproduced the installer's exact 'Nested mappings are not allowed in compact mappings' error, proving the test is a genuine regression guard rather than a tautology. Cross-checked the fixed description under python's yaml.safe_load (the loader family behind the installer's error) and confirmed the CI paths-filter now covers skills/**/SKILL.md. No findings.Evidence: test/skill-frontmatter.test.ts — pass on fixed content, fail (installer error) on reverted content
Evidence: python yaml.safe_load of the fixed frontmatter's description field (same loader the installer's error came from) — both document kinds present, colon removed via em-dash
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
pnpm test && bats scripts/ci/*.batsnpx vitest run test/skill-frontmatter.test.ts— 4/4 pass on the fixed frontmatterReverted skills/thurview/SKILL.md to the pre-fix (main) content and reran the same test file: it failed with the installer's exact error, 'Nested mappings are not allowed in compact mappings', confirming the test is a real regression guard — then restored the fixed file (worktree left clean)Manually parsed the fixed frontmatter's description withpython3 -c "import yaml; ..."(yaml.safe_load), the same loader family the installer's reported error came from, and confirmed both 'review of a branch' and 'code explainer' wording surviveConfirmed .github/workflows/ci.yml's 'code' paths-filter now lists 'skills/**/SKILL.md' (line 38), closing the gap that let a skill-only edit skip the guarding CI job✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.