fix(ci): flag SKILL.md frontmatter defects in validate-skills (#1669)

* fix(ci): flag SKILL.md frontmatter defects in validate-skills

Issue #1663 reported two SKILL.md frontmatter defects (missing `name:`
on skill-stocktake; literal block-scalar `description: |-` on
openclaw-persona-forge) that PR #1664 addresses at the data level.

This change is complementary: it extends `scripts/ci/validate-skills.js`
to catch the same class of defect statically going forward, so the
frontmatter-vs-renderer problems do not silently reappear as new skills
land.

## Checks added
- Frontmatter must declare a `name:` field.
- Frontmatter `description:` must not use a literal block scalar
  (`|` / `|-` / `|+`) — these preserve internal newlines and break
  flat-table renderers keyed off `description`. Folded (`>`) and inline
  strings are accepted.

## Behavior
- Frontmatter findings default to WARN (exit 0) so this PR does not
  break CI while the two known offenders are still on main. Pass
  `--strict` or set `CI_STRICT_SKILLS=1` to promote them to ERROR
  (exit 1). Structural findings (missing / empty SKILL.md) remain
  errors as before.
- Today against main, the validator reports exactly two warnings —
  the same two files called out in #1663 — and exits 0. When #1664
  lands, the validator reports zero warnings, at which point strict
  mode can be enabled in CI.

## Parser notes
- Bespoke frontmatter parser mirrors the style of `validate-agents.js`
  (tolerant of UTF-8 BOM and CRLF; no new npm dependency).
- Block-scalar continuation lines are skipped so keys inside a block
  scalar are not mistaken for top-level keys.
- Hidden directories (`.something/`) under skills/ are now skipped.

## Tests
Adds five focused tests to `tests/ci/validators.test.js`:
- warns when frontmatter is missing `name` (default mode)
- errors when frontmatter is missing `name` (--strict mode)
- warns on literal block-scalar description (|-)
- accepts folded (>) and inline descriptions under --strict
- skips hidden directories under skills/

## Docs
Adds two bullets to the `Skill Checklist` in CONTRIBUTING.md covering
the two rules now surfaced by the validator.

Refs #1663. Complements (does not compete with) #1664.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ci): harden SKILL.md frontmatter checks after bot review

Address findings from CodeRabbit, Greptile, and cubic on #1669:

- Guard empty or whitespace-only `name:` values. Previously
  `name:    ` silently passed because the presence check only
  tested key-set membership; now inspectFrontmatter captures
  trimmed values and validate flags an explicit 'name is empty'
  WARN/ERROR.
- Broaden block-scalar detection to cover YAML 1.2 indent
  indicators (`|2`, `|-2`, `>2-`) and trailing comments
  (`|-  # note`). The old regex required a bare `|`/`>` with
  optional `+`/`-`, which let valid-but-disallowed forms slip
  through.
- Update CONTRIBUTING.md checklist to list `|+` alongside `|`
  and `|-` for parity with the validator.
- Extend runSkillsValidator to accept env overrides and add four
  regression tests: empty name, |+ description, |-2 + comment, and
  CI_STRICT_SKILLS=1.

* fix(ci): address round-2 review on validate-skills frontmatter

- Tighten extractFrontmatter closing delimiter to require a newline or
  end-of-file after the closing `---`, so body lines beginning with
  `---text` are not parsed as frontmatter (CodeRabbit).
- Strip both trailing and comment-only values in inspectFrontmatter, so
  `name: # todo` is surfaced as empty rather than silently passing
  (cubic P2).
- Extract validateSkillDir helper so the per-directory validation
  block moves out of validateSkills, keeping both functions under the
  50-line guideline (CodeRabbit nit).
- Hoist runSkillsValidator to module scope in the test harness and
  share the spawnSync import with execFileSync so the helper stops
  re-requiring child_process on every invocation (CodeRabbit nit).
- Add regression tests: comment-only `name:` values must fail strict
  mode; `---trailing` body lines must not be parsed as frontmatter.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Update tests/ci/validators.test.js

Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
This commit is contained in:
Gaurav Dubey
2026-05-11 10:44:38 +05:30
committed by GitHub
parent 600072ebd8
commit e196f8a4cb
3 changed files with 394 additions and 21 deletions

View File

@@ -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

View File

@@ -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<string,string>, 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();

View File

@@ -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
// ==========================================