test: add skill validation battery with node:test#324
Conversation
Add a per-skill test battery and a self-test layer for the linter on top of the existing CI validator, using the native node:test runner (zero deps). Rules now live in a single source of truth (tests/lib/skill-lint.js) shared by the CI validator and the test suites, split into a pure lintSkillContent() (no I/O) and a thin lintSkill() file wrapper so the rules are unit-testable. - tests/skills.test.js: one describe block per skill (24 suites). Hard checks (frontmatter, name match, H1, required sections, Rationalizations table, Verification checkboxes, unique descriptions) fail CI; length thresholds surface as non-failing diagnostics. - tests/skill-lint.test.js: fixture-driven cases that feed invalid content to lintSkillContent and assert the specific error/warning it must raise. This closes the blind spot where a linter that silently accepts everything would still pass, since every real skill is valid. - scripts/validate-skills.js: refactored into a thin CLI over the shared lib. - scripts/skill-metrics.js: length report; renders a markdown table to the GitHub Actions step summary when run in CI. - Length rules: description > 1024 chars is a hard error; description < 100, body outside 1500-15000 bytes, and missing "Use when" are warnings. - package.json: test/validate/metrics scripts, engines node >= 22. - CI: bump Node 20 -> 24 (20 nears EOL), add test-skills job running the battery plus the metrics report. Verified: npm test green (88/88); neutralizing a rule turns the matching self-test red, then green again on revert.
|
Hi @addyosmani @federicobartoli, could you take a look when you have a moment? |
|
Closing this in favor of a cleaner split against current main. Since it was drafted, both adjacent efforts merged: #323 (kebab-case + trigger rules, now in Reworking as two focused PRs instead:
The rationale carries over: the validator can only prove the 24 real skills pass, not that it rejects invalid input (every real skill passes), so the linter self-tests close that blind spot. The metrics run also flagged |
Note
Draft: blocked on #323. This PR overlaps with #323, which was open first and closes #233. The overlap:
scripts/validate-skills.js; this PR refactors that file into a thin CLI over a sharedtests/lib/skill-lint.js, so the two will conflict at merge.Use when/Use before/after/during); this PR currently treats it as a warning with a narrower regex.Plan: keep this as a draft, wait for #323 to merge, then rebase on top of it and adopt both of its rules (kebab-case as error, trigger as a hard error with #323's regex) into the shared lib. Marking draft to avoid review churn until then.
What
Adds a test battery for the skills on top of the existing CI validator, using the native
node:testrunner with zero new dependencies. Three layers:Per-skill battery (
tests/skills.test.js): onedescribeblock per skill (24 suites) so a failure points at the exact skill. Hard checks fail CI: valid frontmatter,namematches the directory, an H1 title is present, the five required sections exist,Common Rationalizationsis a real markdown table,Verificationhas at least one checkbox, and no two skills share a description. Length metrics surface as non-failing diagnostics.Linter self-tests (
tests/skill-lint.test.js): fixture-driven cases that feed crafted invalid content to the linter and assert the specific error or warning it must raise (name mismatch, missing description, over-limit description, missing H1, missing section, rationalizations without a table, verification without a checkbox, malformed frontmatter, exemption-bypass attempt, plus the warning paths).Length metrics (
scripts/skill-metrics.js): a report of description chars and body bytes per skill, flagged against thresholds. In CI it also renders a markdown table to the GitHub Actions step summary.Supporting changes:
tests/lib/skill-lint.js), shared by the CI validator and both test suites. It is split into a purelintSkillContent()with no I/O and a thinlintSkill()file wrapper, which is what makes the rules unit-testable.scripts/validate-skills.jsis refactored into a thin CLI over that shared lib (no behavior change, no duplicated rules).package.jsonwithtest/validate/metricsscripts andengines: node >= 22.test-skillsjob runs the battery plus the metrics report.Why
The existing validator only proves one half of the contract: that the 24 real skills are valid. It cannot prove the other half, that the linter actually rejects invalid input, because every real skill passes. A linter that silently accepted everything (a regex that stops matching, an inverted condition, an early return introduced by a refactor) would still show green and nobody would notice. The self-test layer closes that blind spot: if a rule breaks, the matching fixture test goes red.
Length checks are deliberately split by severity to avoid false friction:
description > 1024chars is a hard error (it is injected into the agent system prompt), while a short description (< 100), an out-of-range body (outside 1500-15000 bytes), and a missing "Use when" trigger clause are warnings, not blockers.The metrics step makes skill size visible on every run, which already surfaces three skills above the soft body threshold (
security-and-hardening,code-review-and-quality,doubt-driven-development) as candidates for moving detail into supporting files.Verification
npm testis green: 88 tests, 26 suites, 0 failures.missing H1 titlefixture test red (87/88), and reverting restores green.node scripts/validate-skills.jsstill passes (24 skills, 0 errors), now via the shared lib.Notes
scripts/validate-commands.jsis untouched; it is only referenced from the newnpm run validatescript.SECTION_EXEMPT_SKILLS), so a skill cannot exempt itself via its own frontmatter.