Merge #595: hold command frontmatter to the same YAML validity rules

scripts/lib/skill-lint-test.js

@@ -249,3 +249,91 @@ test('CRLF line endings are handled', () => {
   const result = lintSkillContent('fenced', content, FENCE_KNOWN);
   assert.equal(overviewMissing(result), true);
 });
+
+// ── Frontmatter must be valid YAML, not merely splittable ────────────────────
+// `parseFrontmatter` splits each line on its first colon, which is forgiving by
+// design. The hosts that read these skills are not: Cursor parses the
+// frontmatter as YAML when a skill is attached to a message, and a parse
+// failure fails the whole request and takes the chat's context with it (#494).
+// Each shape below was confirmed rejected by a strict parser (ruby psych) while
+// passing every other check in this linter.
+
+/** Frontmatter that is otherwise complete, so only YAML validity varies. */
+function fmLines(...lines) {
+  return withAllSections(['---', 'name: alpha', ...lines, '---'].join('\n'));
+}
+
+const yamlErrors = result => result.errors.filter(e => e.startsWith('Frontmatter line '));
+
+test('a valid frontmatter reports no YAML error', () => {
+  const result = lintSkillContent('alpha', fmLines('description: Use when you need alpha'), KNOWN);
+  assert.deepEqual(yamlErrors(result), []);
+});
+
+test('an unquoted value containing a colon is rejected', () => {
+  // YAML reads `Use when: X` as a nested mapping and errors; the split-on-first
+  // -colon parser reads it as a plain string and never notices.
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: Use when you need alpha: auth, secrets and review'),
+    KNOWN,
+  );
+  assert.equal(yamlErrors(result).length, 1);
+  assert.match(yamlErrors(result)[0], /unquoted value containing/);
+});
+
+test('quoting the same value makes it valid again', () => {
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: "Use when you need alpha: auth, secrets and review"'),
+    KNOWN,
+  );
+  assert.deepEqual(yamlErrors(result), []);
+});
+
+test('a colon with no trailing space is left alone', () => {
+  // `https://example.com` is a perfectly good YAML scalar. The rule keys on
+  // colon-space, not on colons, so ordinary URLs do not trip it.
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: Use when you need alpha', 'docs: https://example.com/a:b'),
+    KNOWN,
+  );
+  assert.deepEqual(yamlErrors(result), []);
+});
+
+test('a tab used for indentation is rejected', () => {
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: Use when you need alpha', 'meta:', '\tlevel: core'),
+    KNOWN,
+  );
+  assert.equal(yamlErrors(result).length, 1);
+  assert.match(yamlErrors(result)[0], /indents with a tab/);
+});
+
+test('an unterminated quote is rejected', () => {
+  const result = lintSkillContent('alpha', fmLines('description: "Use when you need alpha'), KNOWN);
+  assert.equal(yamlErrors(result).length, 1);
+  assert.match(yamlErrors(result)[0], /never closes/);
+});
+
+test('a duplicate key is not reported, because YAML accepts it', () => {
+  // Deliberate boundary: `safe_load` accepts duplicate keys, so flagging them
+  // here would fail files no host rejects. The rule tracks the parser, not taste.
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: Use when you need alpha', 'description: Use when you need alpha'),
+    KNOWN,
+  );
+  assert.deepEqual(yamlErrors(result), []);
+});
+
+test('the error names the line so the fix is obvious', () => {
+  const result = lintSkillContent(
+    'alpha',
+    fmLines('description: Use when you need alpha', 'owner: team: platform'),
+    KNOWN,
+  );
+  assert.match(yamlErrors(result)[0], /^Frontmatter line 4 /);
+});

scripts/lib/skill-lint.js

@@ -146,6 +146,85 @@ function parseFrontmatter(content) {
   return result;
 }
 
+/**
+ * Report frontmatter that this file's own parser accepts but a real YAML
+ * parser rejects.
+ *
+ * `parseFrontmatter` above splits each line on its first colon. That is enough
+ * to read `name` and `description`, and it is deliberately forgiving — but it
+ * means a SKILL.md whose frontmatter is not valid YAML passes every check here.
+ * The hosts that consume these skills do not share that forgiveness: Cursor
+ * parses the frontmatter as YAML when a skill is attached to a message, and a
+ * parse failure there fails the whole request with a generic server error and
+ * takes the rest of the chat's context with it (#494).
+ *
+ * So the repo could ship frontmatter that breaks a host while CI stayed green.
+ * That was verified once by hand, against all 25 skills, in the #494 thread;
+ * this makes it a check instead of a memory.
+ *
+ * Scope is deliberately narrow — the three shapes a strict parser rejects and
+ * the split-on-first-colon parser does not, confirmed against ruby's psych:
+ *
+ *   description: Use this: when X    unquoted value with a colon-space: YAML
+ *                                    reads a nested mapping and errors
+ *   \tkey: value                      tab indentation: invalid YAML whitespace
+ *   description: "unterminated       an unclosed quote
+ *
+ * It is not a YAML implementation. This repo has no package.json and therefore
+ * no parser to depend on, so the check stays a small set of rules aimed at the
+ * classes actually observed to break a host. No current frontmatter uses block
+ * scalars or multi-line values, and the check assumes that stays true; add a
+ * rule here rather than loosening one if it changes.
+ */
+function frontmatterYamlErrors(content) {
+  const match = content.match(/^---[ \t]*\r?\n([\s\S]*?)\r?\n---[ \t]*\r?\n/);
+  if (!match) return [];
+
+  const errors = [];
+  const lines = match[1].split(/\r?\n/);
+  lines.forEach((line, i) => {
+    const lineNo = i + 2; // the opening `---` is line 1
+    if (!line.trim()) return;
+
+    if (/^[ ]*\t/.test(line)) {
+      errors.push(
+        `Frontmatter line ${lineNo} indents with a tab — YAML forbids tabs in indentation, ` +
+        `so a host that parses this frontmatter rejects the whole file`
+      );
+      return;
+    }
+
+    const colonIdx = line.indexOf(':');
+    if (colonIdx === -1) return;
+    const value = line.slice(colonIdx + 1).trim();
+    if (!value) return;
+
+    const quote = value[0] === '"' || value[0] === "'" ? value[0] : null;
+    if (quote) {
+      // An opening quote must close. `endsWith` is enough here because a
+      // trailing comment after a quoted scalar does not appear in this repo.
+      if (value.length < 2 || !value.endsWith(quote)) {
+        errors.push(
+          `Frontmatter line ${lineNo} opens a ${quote === '"' ? 'double' : 'single'} quote that never closes — ` +
+          `a host parsing this frontmatter as YAML rejects the whole file`
+        );
+      }
+      return;
+    }
+
+    // Unquoted scalar. A colon followed by a space (or ending the line) makes
+    // YAML read a nested mapping where a plain string was meant.
+    if (/:(\s|$)/.test(value)) {
+      errors.push(
+        `Frontmatter line ${lineNo} has an unquoted value containing ': ' — YAML reads that as a ` +
+        `nested mapping and rejects the file; wrap the value in quotes`
+      );
+    }
+  });
+
+  return errors;
+}
+
 /**
  * Collect all explicit skill cross-references from content.
  * Only matches against the SKILL_REF_PATTERNS list to avoid
@@ -183,6 +262,10 @@ function lintSkillContent(dirName, content, knownSkills) {
     return { errors, warnings, exempt };
   }
 
+  // The parser above is forgiving by design; the hosts that read this
+  // frontmatter are not (#494).
+  errors.push(...frontmatterYamlErrors(content));
+
   if (!fm.name) {
     errors.push("Frontmatter missing required field: 'name'");
   } else if (fm.name !== dirName) {
@@ -311,6 +394,7 @@ function lintSkill(dirName, skillsDir, knownSkills) {
 module.exports = {
   stripFencedCodeBlocks,
   parseFrontmatter,
+  frontmatterYamlErrors,
   extractSkillReferences,
   lintSkillContent,
   lintSkill,

scripts/validate-commands-test.js

@@ -10,6 +10,9 @@ const { spawnSync } = require('node:child_process');
 const { afterEach, test } = require('node:test');
 
 const VALIDATOR = path.join(__dirname, 'validate-commands.js');
+// The validator shares the frontmatter-validity rules with validate-skills, so
+// the sandbox needs the lib alongside it, not just the script.
+const SKILL_LINT = path.join(__dirname, 'lib', 'skill-lint.js');
 const sandboxes = [];
 
 function makeSandbox() {
@@ -17,6 +20,8 @@ function makeSandbox() {
   const scriptsDir = path.join(root, 'scripts');
   fs.mkdirSync(scriptsDir, { recursive: true });
   fs.copyFileSync(VALIDATOR, path.join(scriptsDir, 'validate-commands.js'));
+  fs.mkdirSync(path.join(scriptsDir, 'lib'), { recursive: true });
+  fs.copyFileSync(SKILL_LINT, path.join(scriptsDir, 'lib', 'skill-lint.js'));
   sandboxes.push(root);
   return root;
 }
@@ -148,3 +153,64 @@ test('parses escaped quotes in double-quoted TOML descriptions', () => {
   assert.equal(result.status, 0, result.stdout + result.stderr);
   assert.match(result.stdout, /1 commands checked — 0 error\(s\) — PASSED/);
 });
+
+// Claude parses a command's frontmatter as YAML when the command is loaded, and
+// `descriptionFromMd` splits each line on its first colon — so a command whose
+// frontmatter is not valid YAML passed every check here. Same class as the
+// SKILL.md gap, on the other set of files the #494 thread checked by hand.
+
+test('a command whose frontmatter is valid YAML passes', () => {
+  const root = makeSandbox();
+  writeMatchingCommands(root, 'build', 'Build the thing');
+  const result = run(root);
+  assert.equal(result.status, 0, result.stdout);
+  assert.match(result.stdout, /Checking Claude command frontmatter/);
+});
+
+test('an unquoted colon in a command description is rejected', () => {
+  const root = makeSandbox();
+  // Valid to the splitter, rejected by a real YAML parser: it reads a nested
+  // mapping. The TOML siblings keep the same text so only the .md is at fault.
+  writeClaudeCommand(root, 'build', 'description: Build the thing: quickly');
+  writeTomlCommand(root, path.join('.gemini', 'commands'), 'build', 'description = "Build the thing: quickly"');
+  writeTomlCommand(root, 'commands', 'build', 'description = "Build the thing: quickly"');
+
+  const result = run(root);
+  assert.equal(result.status, 1, result.stdout);
+  assert.match(result.stdout, /unquoted value containing/);
+});
+
+test('quoting the same description makes it pass', () => {
+  const root = makeSandbox();
+  writeClaudeCommand(root, 'build', 'description: "Build the thing: quickly"');
+  writeTomlCommand(root, path.join('.gemini', 'commands'), 'build', 'description = "Build the thing: quickly"');
+  writeTomlCommand(root, 'commands', 'build', 'description = "Build the thing: quickly"');
+
+  const result = run(root);
+  assert.equal(result.status, 0, result.stdout);
+});
+
+test('a tab-indented command frontmatter is rejected', () => {
+  const root = makeSandbox();
+  writeFile(
+    root,
+    path.join('.claude', 'commands', 'build.md'),
+    '---\ndescription: Build the thing\nmeta:\n\tlevel: core\n---\n\n# Command\n',
+  );
+  writeTomlCommand(root, path.join('.gemini', 'commands'), 'build', 'description = "Build the thing"');
+  writeTomlCommand(root, 'commands', 'build', 'description = "Build the thing"');
+
+  const result = run(root);
+  assert.equal(result.status, 1, result.stdout);
+  assert.match(result.stdout, /indents with a tab/);
+});
+
+test('the TOML directories are not put through the YAML rules', () => {
+  const root = makeSandbox();
+  // A TOML description legitimately carries a colon. Running the YAML rules over
+  // these files would fail every command that has one.
+  writeMatchingCommands(root, 'build', 'Build the thing quickly');
+  const result = run(root);
+  assert.equal(result.status, 0, result.stdout);
+  assert.doesNotMatch(result.stdout, /\.gemini.*unquoted value/);
+});

scripts/validate-commands.js

@@ -10,6 +10,7 @@
  * Checks (errors block CI):
  *   - Every command present in one directory exists in all three
  *   - The 'description' field is identical across all three equivalents
+ *   - Claude command frontmatter is valid YAML, not merely splittable
  *
  * What this does NOT check:
  *   Prompt body differences are intentional — each tool has its own
@@ -23,6 +24,14 @@
 const fs   = require('fs');
 const path = require('path');
 
+// The same frontmatter-validity rules validate-skills applies to SKILL.md.
+// `descriptionFromMd` below splits each line on its first colon, exactly as the
+// skill reader used to, so a command whose frontmatter is not valid YAML passes
+// every check here — and Claude Code parses that frontmatter when the command is
+// loaded. The #494 thread verified all 25 SKILL.md files AND the command files by
+// hand; this makes the second half a check too.
+const { frontmatterYamlErrors } = require(path.join(__dirname, 'lib', 'skill-lint.js'));
+
 // ─── Config ───────────────────────────────────────────────────────────────────
 
 const ROOT = path.resolve(__dirname, '..');
@@ -136,6 +145,32 @@ function main() {
     }
   }
 
+  // ── Claude frontmatter validity ─────────────────────────────────────────────
+  // Only the .md directory: the TOML dirs are parsed by a real TOML parser
+  // already, so a malformed one surfaces as a missing description above.
+  console.log('\nChecking Claude command frontmatter...');
+
+  for (const stem of claudeStems) {
+    const full = path.join(DIRS.claude.dir, `${stem}${DIRS.claude.ext}`);
+    let yamlErrors = [];
+    try {
+      yamlErrors = frontmatterYamlErrors(fs.readFileSync(full, 'utf8'));
+    } catch (e) {
+      console.log(`  ✗  ${stem} — cannot read file: ${e.message}`);
+      errors++;
+      continue;
+    }
+    if (yamlErrors.length === 0) {
+      console.log(`  ✓  ${stem}`);
+      continue;
+    }
+    console.log(`  ✗  ${stem}`);
+    for (const message of yamlErrors) {
+      console.log(`       ${message}`);
+      errors++;
+    }
+  }
+
   // ── Description sync check ──────────────────────────────────────────────────
   console.log('\nChecking description sync...');