diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 410f617e..dda1e713 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -167,6 +167,8 @@ Short version: - [ ] Tested with Claude Code - [ ] Links to related skills - [ ] No sensitive data (API keys, tokens, paths) +- [ ] Frontmatter declares `name:` matching the directory name +- [ ] Frontmatter `description:` is an inline string or folded (`>`) scalar — not a literal block (`|`, `|-`, or `|+`), which preserves internal newlines and breaks flat-table renderers ### Example Skills diff --git a/scripts/ci/validate-skills.js b/scripts/ci/validate-skills.js index 7664b5d6..6ffc8537 100644 --- a/scripts/ci/validate-skills.js +++ b/scripts/ci/validate-skills.js @@ -1,6 +1,22 @@ #!/usr/bin/env node /** * Validate curated skill directories (skills/ in repo). + * + * Checks: + * 1. Each sub-directory of skills/ contains a SKILL.md file. + * 2. SKILL.md is non-empty. + * 3. SKILL.md frontmatter (if present) declares a `name:` field. + * 4. SKILL.md frontmatter `description:` uses an inline scalar — not a + * literal block scalar (`|` / `|-` / `|+`), which preserves internal + * newlines and breaks flat-table renderers keyed off `description`. + * + * Frontmatter findings default to WARN so CI does not break while + * pre-existing data defects are being cleaned up out of band (see #1663). + * Pass `--strict` or set `CI_STRICT_SKILLS=1` to promote frontmatter + * findings to errors (exit 1). + * + * Structural findings (missing/empty SKILL.md) are always errors. + * * Scope: curated only. Learned/imported/evolved roots are out of scope. * If skills/ does not exist, exit 0 (no curated skills to validate). */ @@ -10,6 +26,144 @@ const path = require('path'); const SKILLS_DIR = path.join(__dirname, '../../skills'); +const STRICT = process.argv.includes('--strict') || process.env.CI_STRICT_SKILLS === '1'; + +/** + * Parse the leading YAML frontmatter of a markdown document. + * + * Returns `{ present, lines }` so callers can inspect raw lines + * (needed to detect block-scalar `description:` values). + * + * Tolerant of UTF-8 BOM and CRLF line endings, matching the other + * validators in this directory. + * + * @param {string} content + * @returns {{present: boolean, lines: string[]}} + */ +function extractFrontmatter(content) { + // Strip BOM if present (UTF-8 BOM: U+FEFF). + const clean = content.replace(/^\uFEFF/, ''); + const match = clean.match(/^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)/); + if (!match) return { present: false, lines: [] }; + return { + present: true, + lines: match[1].split(/\r?\n/) + }; +} + +/** + * Extract top-level keys (with trimmed values) and flag block-scalar + * `description:` values. + * + * Lines that continue a block scalar (`|` or `>`) are skipped — we only + * care about the top-level key set and the raw indicator on the + * `description:` line. Block-scalar indicators accept YAML chomp and + * indent modifiers and trailing comments, e.g. `|`, `|-`, `|+`, `|2`, + * `|-2`, `>- # note`. + * + * @param {string[]} lines + * @returns {{values: Record, descriptionIndicator: string|null}} + */ +function inspectFrontmatter(lines) { + const values = Object.create(null); + let descriptionIndicator = null; + let inBlockScalar = false; + let blockScalarIndent = -1; + + for (const rawLine of lines) { + if (inBlockScalar) { + // Stay inside the block until a line with indent <= the opener's + // indent (or an empty continuation). + const leadingSpaces = rawLine.match(/^(\s*)/)[1].length; + if (rawLine.trim() === '' || leadingSpaces > blockScalarIndent) { + continue; + } + inBlockScalar = false; + blockScalarIndent = -1; + } + + const match = rawLine.match(/^([A-Za-z0-9_-]+):\s*(.*)$/); + if (!match) continue; + + const key = match[1]; + const rawValue = match[2]; + // Strip unquoted comments for value/indicator inspection. Handles both + // trailing comments (`foo: bar # note`) and comment-only values + // (`foo: # todo`) so the latter is treated as empty. + const valueNoComment = rawValue + .replace(/^\s*#.*$/, '') + .replace(/\s+#.*$/, '') + .trim(); + values[key] = valueNoComment; + + // Detect literal / folded block-scalar indicators. Accept chomp + // modifiers (`-` / `+`) and optional indent-indicator digits in + // either order, per YAML 1.2. + if (/^[|>](?:[+-]?\d+|\d+[+-]?|[+-])?$/.test(valueNoComment)) { + if (key === 'description') { + descriptionIndicator = valueNoComment; + } + inBlockScalar = true; + blockScalarIndent = rawLine.match(/^(\s*)/)[1].length; + } + } + + return { values, descriptionIndicator }; +} + +/** + * Validate a single skill directory. + * + * Returns `{ fatal }` where `fatal` indicates a structural error that + * should be surfaced via `console.error` and abort CI (missing/empty + * SKILL.md). Frontmatter findings are routed through + * `reportFrontmatterFinding`, which owns the WARN/ERROR decision based + * on strict mode. + * + * @param {string} dir + * @param {string} skillsDir + * @param {(msg: string) => void} reportFrontmatterFinding + * @returns {{fatal: boolean}} + */ +function validateSkillDir(dir, skillsDir, reportFrontmatterFinding) { + const skillMd = path.join(skillsDir, dir, 'SKILL.md'); + if (!fs.existsSync(skillMd)) { + console.error(`ERROR: ${dir}/ - Missing SKILL.md`); + return { fatal: true }; + } + + let content; + try { + content = fs.readFileSync(skillMd, 'utf-8'); + } catch (err) { + console.error(`ERROR: ${dir}/SKILL.md - ${err.message}`); + return { fatal: true }; + } + if (content.trim().length === 0) { + console.error(`ERROR: ${dir}/SKILL.md - Empty file`); + return { fatal: true }; + } + + const fm = extractFrontmatter(content); + if (fm.present) { + const { values, descriptionIndicator } = inspectFrontmatter(fm.lines); + + if (!Object.prototype.hasOwnProperty.call(values, 'name')) { + reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter missing required field: name`); + } else if (values.name === '') { + reportFrontmatterFinding(`${dir}/SKILL.md - frontmatter 'name' is empty`); + } + + if (descriptionIndicator && descriptionIndicator.startsWith('|')) { + reportFrontmatterFinding( + `${dir}/SKILL.md - frontmatter description uses literal block scalar ` + `'${descriptionIndicator}' which preserves internal newlines; ` + `use an inline string or folded '>' scalar instead` + ); + } + } + + return { fatal: false }; +} + function validateSkills() { if (!fs.existsSync(SKILLS_DIR)) { console.log('No curated skills directory (skills/), skipping'); @@ -17,32 +171,28 @@ function validateSkills() { } const entries = fs.readdirSync(SKILLS_DIR, { withFileTypes: true }); - const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); + const dirs = entries.filter(e => e.isDirectory() && !e.name.startsWith('.')).map(e => e.name); + let hasErrors = false; + let warnCount = 0; let validCount = 0; + const reportFrontmatterFinding = msg => { + if (STRICT) { + console.error(`ERROR: ${msg}`); + hasErrors = true; + } else { + console.warn(`WARN: ${msg}`); + warnCount++; + } + }; + for (const dir of dirs) { - const skillMd = path.join(SKILLS_DIR, dir, 'SKILL.md'); - if (!fs.existsSync(skillMd)) { - console.error(`ERROR: ${dir}/ - Missing SKILL.md`); + const { fatal } = validateSkillDir(dir, SKILLS_DIR, reportFrontmatterFinding); + if (fatal) { hasErrors = true; continue; } - - let content; - try { - content = fs.readFileSync(skillMd, 'utf-8'); - } catch (err) { - console.error(`ERROR: ${dir}/SKILL.md - ${err.message}`); - hasErrors = true; - continue; - } - if (content.trim().length === 0) { - console.error(`ERROR: ${dir}/SKILL.md - Empty file`); - hasErrors = true; - continue; - } - validCount++; } @@ -50,7 +200,11 @@ function validateSkills() { process.exit(1); } - console.log(`Validated ${validCount} skill directories`); + let msg = `Validated ${validCount} skill directories`; + if (warnCount > 0) { + msg += ` (${warnCount} warning${warnCount === 1 ? '' : 's'})`; + } + console.log(msg); } validateSkills(); diff --git a/tests/ci/validators.test.js b/tests/ci/validators.test.js index 1b02c41e..a35c760e 100644 --- a/tests/ci/validators.test.js +++ b/tests/ci/validators.test.js @@ -11,7 +11,7 @@ const assert = require('assert'); const path = require('path'); const fs = require('fs'); const os = require('os'); -const { execFileSync } = require('child_process'); +const { execFileSync, spawnSync } = require('child_process'); const validatorsDir = path.join(__dirname, '..', '..', 'scripts', 'ci'); const repoRoot = path.join(__dirname, '..', '..'); @@ -180,6 +180,48 @@ function runCatalogValidator(overrides = {}) { return runSourceViaTempFile(source); } +// Run validate-skills.js against a fixture dir, optionally passing +// extra argv (e.g. '--strict') and env overrides (e.g. +// CI_STRICT_SKILLS=1) so the frontmatter finding suite can exercise +// both warn and strict modes via argv and env code paths. +// +// Captures stderr on both success and failure (the shared +// runSourceViaTempFile helper only surfaces stderr when the child +// exits non-zero, which hides WARN lines in the default mode). +function runSkillsValidator(testDir, argv = [], envOverrides = {}) { + const validatorPath = path.join(validatorsDir, 'validate-skills.js'); + let source = fs.readFileSync(validatorPath, 'utf8'); + source = stripShebang(source); + source = source.replace( + /const SKILLS_DIR = .*?;/, + `const SKILLS_DIR = ${JSON.stringify(testDir)};`, + ); + if (argv.length > 0) { + const argvPreamble = argv + .map(arg => `process.argv.push(${JSON.stringify(arg)});`) + .join('\n'); + source = `${argvPreamble}\n${source}`; + } + const tmpFile = path.join(repoRoot, + `.tmp-validator-${Date.now()}-${Math.random().toString(36).slice(2)}.js`); + try { + fs.writeFileSync(tmpFile, source, 'utf8'); + const r = spawnSync('node', [tmpFile], { + encoding: 'utf8', + timeout: 10000, + cwd: repoRoot, + env: { ...process.env, CI_STRICT_SKILLS: '', ...envOverrides }, + }); + return { + code: typeof r.status === 'number' ? r.status : 1, + stdout: r.stdout || '', + stderr: r.stderr || '', + }; + } finally { + try { fs.unlinkSync(tmpFile); } catch (_) { /* ignore */ } + } +} + function writeCatalogFixture(testDir, options = {}) { const { readmeCounts = { agents: 1, skills: 1, commands: 1 }, @@ -801,6 +843,181 @@ function runTests() { cleanupTestDir(testDir); })) passed++; else failed++; + if (test('warns when frontmatter is missing name (default mode)', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'no-name-skill'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\ndescription: "X"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir); + assert.strictEqual(result.code, 0, + `Default mode must not fail CI; got stderr: ${result.stderr}`); + assert.ok( + result.stderr.includes('WARN') && result.stderr.includes('missing required field: name'), + `Should warn on missing name; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('errors when frontmatter is missing name (strict mode)', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'no-name-skill'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\ndescription: "X"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir, ['--strict']); + assert.strictEqual(result.code, 1, '--strict must fail CI on missing name'); + assert.ok(result.stderr.includes('missing required field: name'), + 'Should report missing name'); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('warns on literal block-scalar description (|-)', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'block-desc-skill'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: block-desc-skill\ndescription: |-\n line one\n line two\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir); + assert.strictEqual(result.code, 0, 'Default mode should not fail CI'); + assert.ok( + result.stderr.includes('WARN') && result.stderr.includes('literal block scalar'), + `Should warn on |- description; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('accepts folded (>) and inline descriptions', () => { + const testDir = createTestDir(); + const folded = path.join(testDir, 'folded-skill'); + fs.mkdirSync(folded); + fs.writeFileSync(path.join(folded, 'SKILL.md'), + '---\nname: folded-skill\ndescription: >\n joined\n on spaces\norigin: ECC\n---\n# Skill'); + const inline = path.join(testDir, 'inline-skill'); + fs.mkdirSync(inline); + fs.writeFileSync(path.join(inline, 'SKILL.md'), + '---\nname: inline-skill\ndescription: "single line"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir, ['--strict']); + assert.strictEqual(result.code, 0, + `Folded and inline should pass strict; got stderr: ${result.stderr}`); + assert.ok(result.stdout.includes('Validated 2'), + `Should count both skills; got stdout: ${result.stdout}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('skips hidden directories under skills/', () => { + const testDir = createTestDir(); + // A dot-prefixed directory (e.g. .DS_Store-adjacent junk or legacy + // cache) must not count as a skill and must not error. + fs.mkdirSync(path.join(testDir, '.cache')); + fs.writeFileSync(path.join(testDir, '.cache', 'SKILL.md'), '# ignored'); + const real = path.join(testDir, 'real-skill'); + fs.mkdirSync(real); + fs.writeFileSync(path.join(real, 'SKILL.md'), + '---\nname: real-skill\ndescription: "x"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir, ['--strict']); + assert.strictEqual(result.code, 0, 'Hidden dirs should be skipped'); + assert.ok(result.stdout.includes('Validated 1'), + `Should only count the non-hidden skill; got stdout: ${result.stdout}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('warns when name: value is empty or whitespace', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'empty-name-skill'); + fs.mkdirSync(skillDir); + // `name:` key present but value is blank. + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: \ndescription: "X"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir); + assert.strictEqual(result.code, 0, + `Default mode must not fail CI; got stderr: ${result.stderr}`); + assert.ok( + result.stderr.includes('WARN') && result.stderr.includes("'name' is empty"), + `Should warn on empty name; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('warns on literal block-scalar description with |+ chomp', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'keep-desc-skill'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: keep-desc-skill\ndescription: |+\n line one\n line two\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir); + assert.strictEqual(result.code, 0, 'Default mode should not fail CI'); + assert.ok(result.stderr.includes('literal block scalar'), + `Should warn on |+ description; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('warns on block-scalar description with indent indicator and trailing comment', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'indent-desc-skill'); + fs.mkdirSync(skillDir); + // `|-2 # note` is still a literal block scalar in YAML 1.2. + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: indent-desc-skill\ndescription: |-2 # trimmed two-space indent\n line one\n line two\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir); + assert.strictEqual(result.code, 0, 'Default mode should not fail CI'); + assert.ok(result.stderr.includes('literal block scalar'), + `Should warn on |-2 description; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('honors CI_STRICT_SKILLS=1 env flag', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'no-name-skill-env'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\ndescription: "X"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir, [], { CI_STRICT_SKILLS: '1' }); + assert.strictEqual(result.code, 1, 'CI_STRICT_SKILLS=1 must fail CI on missing name'); + assert.ok(result.stderr.includes('missing required field: name'), + 'Should report missing name'); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('flags comment-only name value as empty (strict)', () => { + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'comment-only-name'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '---\nname: # todo\ndescription: "X"\norigin: ECC\n---\n# Skill'); + + const result = runSkillsValidator(testDir, ['--strict']); + assert.strictEqual(result.code, 1, 'Strict mode must fail CI on empty name'); + assert.ok(result.stderr.includes("'name' is empty"), + `Should report empty name; got stderr: ${result.stderr}`); + cleanupTestDir(testDir); + })) passed++; else failed++; + + if (test('tolerates ---trailing text outside frontmatter block', () => { + // A SKILL.md whose body contains a line starting with '---text' + // must not be parsed as frontmatter. Regression guard for + // closing-delimiter tightening: the old regex would greedily + // match '---trailing'. + const testDir = createTestDir(); + const skillDir = path.join(testDir, 'no-frontmatter-dashes'); + fs.mkdirSync(skillDir); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), + '# Skill\n\nSome body text.\n\n---trailing content\nmore body\n'); + + const result = runSkillsValidator(testDir, ['--strict']); + assert.strictEqual(result.code, 0, + `Should not flag frontmatter findings when no valid frontmatter exists; got stderr: ${result.stderr}`); + assert.ok(!result.stderr.includes('missing required field: name'), + 'Must not treat ---trailing as a frontmatter closer'); + cleanupTestDir(testDir); + })) passed++; else failed++; + // ========================================== // validate-commands.js // ==========================================