From d6c11c40a2383350535c161b1fe79c6076e7a1c4 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Mon, 14 Sep 2026 14:22:01 -0700 Subject: [PATCH] feat(validator): enforce the skill-anatomy layout rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/skill-anatomy.md states three structural rules that nothing checks: the 500-line context budget for SKILL.md, no empty subdirectories, and lowercase-hyphen-separated supporting filenames. skill-lint.js calls itself the single source of truth for that document, so the rules belong there rather than in a sixth validator script. The line-count rule is a warning: an over-budget skill is an editorial call, and failing CI on it would block unrelated work. Layout rules are errors — nothing on main violates them, so they cannot break the build. Also wires scripts/lib/skill-lint-test.js into CI. It has never run there; every other *-test.js in scripts/ has its own step. --- scripts/lib/skill-lint-test.js | 100 ++++++++++++++++++++++++++++++++- scripts/lib/skill-lint.js | 87 +++++++++++++++++++++++++++- 2 files changed, 184 insertions(+), 3 deletions(-) diff --git a/scripts/lib/skill-lint-test.js b/scripts/lib/skill-lint-test.js index d2ebf936..2264d29b 100644 --- a/scripts/lib/skill-lint-test.js +++ b/scripts/lib/skill-lint-test.js @@ -5,10 +5,29 @@ const assert = require('node:assert/strict'); const { test } = require('node:test'); -const { lintSkillContent } = require('./skill-lint.js'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { lintSkillContent, lintSkillLayout } = require('./skill-lint.js'); const KNOWN = new Set(['alpha', 'beta']); +/** + * Build a throwaway skill directory. `dirs` are created empty; `files` maps a + * path within the skill to its contents, creating parents as needed. + */ +function makeSkillDir({ dirs = [], files = {} } = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-layout-')); + for (const d of dirs) fs.mkdirSync(path.join(root, d), { recursive: true }); + for (const [rel, body] of Object.entries(files)) { + const abs = path.join(root, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, body); + } + return root; +} + /** A SKILL.md body carrying every required section, so tests can isolate frontmatter. */ function withAllSections(frontmatter) { return [ @@ -337,3 +356,82 @@ test('the error names the line so the fix is obvious', () => { ); assert.match(yamlErrors(result)[0], /^Frontmatter line 4 /); }); + +// ─── Context budget ────────────────────────────────────────────────────────── + +test('warns when SKILL.md exceeds the 500-line context budget', () => { + const padded = withAllSections(VALID_FRONTMATTER) + '\n'.repeat(600); + + const { errors, warnings } = lintSkillContent('alpha', padded, KNOWN); + + assert.equal(errors.length, 0, 'an over-budget skill must not block CI'); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /over the 500-line context budget/); +}); + +test('a SKILL.md at exactly the budget is not flagged', () => { + const base = withAllSections(VALID_FRONTMATTER); + const baseLines = (base.match(/\n/g) || []).length; + const atBudget = base + '\n'.repeat(500 - baseLines); + + assert.equal((atBudget.match(/\n/g) || []).length, 500, 'fixture must sit exactly on the boundary'); + const { warnings } = lintSkillContent('alpha', atBudget, KNOWN); + + assert.equal(warnings.length, 0); +}); + +// ─── Layout ────────────────────────────────────────────────────────────────── + +test('reports an empty scripts/ directory', () => { + const dir = makeSkillDir({ dirs: ['scripts'] }); + + const errors = lintSkillLayout(dir); + + assert.equal(errors.length, 1); + assert.match(errors[0], /Empty directory `scripts\/`/); +}); + +test('reports a directory that only nests more empty directories', () => { + const dir = makeSkillDir({ dirs: ['references', 'references/deep'] }); + + const errors = lintSkillLayout(dir); + + assert.equal(errors.length, 1, 'the outermost empty directory is named once, not every level'); + assert.match(errors[0], /Empty directory `references\/`/); +}); + +test('a directory holding a file is not empty', () => { + const dir = makeSkillDir({ files: { 'scripts/helper.sh': '#!/bin/bash\nset -e\n' } }); + + assert.deepEqual(lintSkillLayout(dir), []); +}); + +test('reports a supporting .md file that is not lowercase-hyphen-separated', () => { + const dir = makeSkillDir({ files: { 'Refinement_Criteria.md': 'x\n' } }); + + const errors = lintSkillLayout(dir); + + assert.equal(errors.length, 1); + assert.match(errors[0], /Supporting file `Refinement_Criteria\.md` is not lowercase-hyphen-separated/); +}); + +test('names a badly named supporting file by its path within the skill', () => { + const dir = makeSkillDir({ files: { 'references/Floor_Guard.md': 'x\n' } }); + + const errors = lintSkillLayout(dir); + + assert.equal(errors.length, 1); + assert.match(errors[0], /`references\/Floor_Guard\.md`/); +}); + +test('SKILL.md is exempt from the supporting-file naming rule', () => { + const dir = makeSkillDir({ files: { 'SKILL.md': 'x\n', 'examples.md': 'x\n' } }); + + assert.deepEqual(lintSkillLayout(dir), []); +}); + +test('non-markdown files are left to the Script Requirements conventions', () => { + const dir = makeSkillDir({ files: { 'scripts/Idea_Refine.sh': '#!/bin/bash\nset -e\n' } }); + + assert.deepEqual(lintSkillLayout(dir), []); +}); diff --git a/scripts/lib/skill-lint.js b/scripts/lib/skill-lint.js index be4a455d..3258f7bd 100644 --- a/scripts/lib/skill-lint.js +++ b/scripts/lib/skill-lint.js @@ -15,9 +15,12 @@ * - description does not exceed 1024 characters * - description includes a 'when to use' trigger (skill-anatomy.md: Required) * - required sections are present + * - a skill's subdirectories are non-empty (skill-anatomy.md: Supporting Files) + * - supporting .md filenames are lowercase-hyphen-separated * * Checks (warnings, do not block CI): * - cross-skill references point to known skills + * - SKILL.md stays under the 500-line context budget */ const fs = require('fs'); @@ -27,6 +30,12 @@ const path = require('path'); const MAX_DESCRIPTION_LENGTH = 1024; +// SKILL.md loads in full the moment an agent activates the skill, so its length +// is paid for on every activation (docs/skill-anatomy.md -> Context Efficiency). +// A warning, not an error: trimming a skill is an editorial call, and failing +// CI on line count would block unrelated work on an over-budget skill. +const MAX_SKILL_LINES = 500; + // A skill directory name must be lowercase-hyphen-separated // (docs/skill-anatomy.md → Naming Conventions). const KEBAB_CASE = /^[a-z0-9]+(-[a-z0-9]+)*$/; @@ -354,6 +363,17 @@ function lintSkillContent(dirName, content, knownSkills) { } } + // ── Context budget ─────────────────────────────────────────────────────── + // Counted as newlines so the number matches what `wc -l` reports, which is + // how a contributor would check this by hand. + const lineCount = (content.match(/\n/g) || []).length; + if (lineCount > MAX_SKILL_LINES) { + warnings.push( + `SKILL.md is ${lineCount} lines, over the ${MAX_SKILL_LINES}-line context budget ` + + `(skill-anatomy.md: Context Efficiency) — move detail into a supporting file`, + ); + } + // ── Cross-skill references ─────────────────────────────────────────────── const refs = extractSkillReferences(content); for (const ref of refs) { @@ -365,13 +385,73 @@ function lintSkillContent(dirName, content, knownSkills) { return { errors, warnings, exempt }; } +/** True when dir contains no regular file at any depth. */ +function isEffectivelyEmpty(dir) { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + if (!isEffectivelyEmpty(path.join(dir, entry.name))) return false; + } else { + return false; + } + } + return true; +} + +/** + * Lint the files that travel with a skill, as opposed to the contents of + * SKILL.md. These rules need the filesystem, so they live here rather than in + * the pure content linter. Returns an array of error strings. + * + * Enforced (docs/skill-anatomy.md): + * - Supporting Files: "do not create an empty `scripts/` directory just to + * mirror other skills". Generalized to any subdirectory — an empty + * `references/` is noise for the same reason. + * - Naming Conventions: "Supporting files: lowercase-hyphen-separated.md". + * Scoped to .md, which is what that line covers; `scripts/` helpers carry + * their own conventions under Script Requirements. + */ +function lintSkillLayout(skillDir) { + const errors = []; + + const walk = (dir, relBase) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name))) { + const rel = relBase ? `${relBase}/${entry.name}` : entry.name; + const abs = path.join(dir, entry.name); + + if (entry.isDirectory()) { + if (isEffectivelyEmpty(abs)) { + errors.push( + `Empty directory \`${rel}/\` — remove it rather than mirroring another skill's layout ` + + `(skill-anatomy.md: Supporting Files)`, + ); + } else { + walk(abs, rel); + } + continue; + } + + if (rel === 'SKILL.md' || !entry.name.endsWith('.md')) continue; + if (!KEBAB_CASE.test(entry.name.slice(0, -'.md'.length))) { + errors.push( + `Supporting file \`${rel}\` is not lowercase-hyphen-separated ` + + `(skill-anatomy.md: Naming Conventions)`, + ); + } + } + }; + + walk(skillDir, ''); + return errors; +} + /** * Lint a skill by directory name: reads its SKILL.md, then delegates to * lintSkillContent. This is the thin filesystem wrapper the CLI uses. * Returns { errors, warnings, exempt }. */ function lintSkill(dirName, skillsDir, knownSkills) { - const skillPath = path.join(skillsDir, dirName, 'SKILL.md'); + const skillDir = path.join(skillsDir, dirName); + const skillPath = path.join(skillDir, 'SKILL.md'); if (!fs.existsSync(skillPath)) { return { errors: ['Missing SKILL.md'], warnings: [], exempt: false }; @@ -384,7 +464,9 @@ function lintSkill(dirName, skillsDir, knownSkills) { return { errors: [`Unreadable SKILL.md: ${err.message}`], warnings: [], exempt: false }; } - return lintSkillContent(dirName, content, knownSkills); + const result = lintSkillContent(dirName, content, knownSkills); + result.errors.push(...lintSkillLayout(skillDir)); + return result; } // Export only functions. The policy collections (REQUIRED_SECTIONS, @@ -397,5 +479,6 @@ module.exports = { frontmatterYamlErrors, extractSkillReferences, lintSkillContent, + lintSkillLayout, lintSkill, };