Merge #603: reject frontmatter values starting with a YAML indicator

scripts/lib/skill-lint-test.js

@@ -351,6 +351,109 @@ test('an unterminated quote is rejected', () => {
   assert.match(yamlErrors(result)[0], /never closes/);
 });
 
+// A plain (unquoted) YAML scalar may not BEGIN with certain indicator characters.
+// The set below was not read off the spec — it was measured against js-yaml, both
+// with and without a following space, and only characters invalid in *both* forms
+// with no legitimate single-line use are rejected here. Anything ambiguous is left
+// alone on purpose, and the second test pins that so the rule cannot be widened
+// into false positives later.
+//
+// The backtick is the one that actually bites this repo: descriptions routinely
+// name other skills, and `\`alpha\` designs things` is a natural way to start one.
+for (const [label, value] of [
+  ['a backtick', '`alpha` designs things. Use when alpha.'],
+  ['an at sign', '@team owns this. Use when alpha.'],
+  ['a percent sign', '%complete coverage. Use when alpha.'],
+]) {
+  test(`an unquoted value starting with ${label} is rejected`, () => {
+    const result = lintSkillContent(
+      'alpha',
+      fmLines(`description: ${value}`),
+      KNOWN,
+    );
+    assert.equal(yamlErrors(result).length, 1);
+    assert.match(yamlErrors(result)[0], /reserved|cannot begin|indicator/i);
+  });
+}
+
+for (const [label, value] of [
+  ['a dash', '- Designs things. Use when alpha.'],
+  ['a question mark', '? Designs things. Use when alpha.'],
+  ['an ampersand', '& Designs things. Use when alpha.'],
+]) {
+  test(`an unquoted value starting with ${label} and a space is rejected`, () => {
+    const result = lintSkillContent(
+      'alpha',
+      fmLines(`description: ${value}`),
+      KNOWN,
+    );
+    assert.equal(yamlErrors(result).length, 1);
+  });
+}
+
+test('quoting the value makes every reserved start valid again', () => {
+  for (const value of ['`alpha` x', '@team x', '%x', '- x', '? x', '& x']) {
+    const result = lintSkillContent(
+      'alpha',
+      fmLines(`description: "${value}"`),
+      KNOWN,
+    );
+    assert.deepEqual(yamlErrors(result), [], `quoted ${value} must be accepted`);
+  }
+});
+
+test('starts that YAML accepts are deliberately NOT rejected', () => {
+  // Each parses cleanly under PyYAML and psych, so flagging them would be a
+  // false positive on valid frontmatter. Pinned so the rule stays narrow.
+  //
+  // `,leading comma is fine` used to be in this list. It is not fine — both
+  // parsers reject it, and this test was pinning a claim I had asserted without
+  // measuring. It now appears in the rejected set below instead.
+  for (const value of [
+    ':platform is fine',          // a colon not followed by a space
+    '-hyphenated is fine',        // a dash not followed by a space
+    'Designs `alpha` things',     // a backtick anywhere but the first character
+    'Designs @team things',       // an at sign anywhere but the first character
+    '#not-a-comment-here',
+    '&anchor-like but valid',     // `&foo` parses; only `& ` is an indicator
+    '[a, b]',                     // a flow sequence is valid, so `[` stays allowed
+    // `{a: b}` is valid YAML too, and `{` is likewise not flagged here — but it
+    // trips the pre-existing colon-space rule, so it is not asserted as accepted.
+    // That false positive predates this change and is left alone.
+  ]) {
+    const result = lintSkillContent('alpha', fmLines(`description: ${value}`), KNOWN);
+    assert.deepEqual(yamlErrors(result), [], `${value} must be accepted`);
+  }
+});
+
+test('indicator characters both parsers reject are flagged', () => {
+  // Measured, not read off the spec: each of these is rejected by PyYAML and by
+  // psych, and none has a legitimate use at the start of a plain scalar.
+  for (const value of [
+    ',leading comma',
+    '*alias-like',
+    '`skill` does X',
+    '@team owns this',
+    '%directive-like',
+  ]) {
+    const result = lintSkillContent('alpha', fmLines(`description: ${value}`), KNOWN);
+    assert.equal(yamlErrors(result).length, 1, `${value} must be rejected`);
+  }
+});
+
+test('a one-line block scalar header is flagged but a real block scalar is not', () => {
+  // `|` with content on the same line is not a block scalar — it is a plain
+  // scalar opening with an indicator, and both parsers reject it. `|` alone,
+  // with indented lines under it, is valid and must stay allowed.
+  for (const value of ['|folded text', '>folded text']) {
+    const result = lintSkillContent('alpha', fmLines(`description: ${value}`), KNOWN);
+    assert.equal(yamlErrors(result).length, 1, `${value} must be rejected`);
+  }
+
+  const genuine = lintSkillContent('alpha', fmLines('description: |'), KNOWN);
+  assert.deepEqual(yamlErrors(genuine), [], 'a bare block-scalar header must be accepted');
+});
+
 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.

scripts/lib/skill-lint.js

@@ -177,13 +177,16 @@ function parseFrontmatter(content) {
  * 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:
+ * Scope is deliberately narrow — the shapes a strict parser rejects and the
+ * split-on-first-colon parser does not, each confirmed by measurement against
+ * both PyYAML and ruby's psych rather than read off the spec:
  *
  *   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
+ *   description: `skill` does X      a value opening with a YAML indicator
+ *                                    (` @ % * ,) or a one-line | / > header
  *
  * 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
@@ -227,8 +230,54 @@ function frontmatterYamlErrors(content) {
       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.
+    // Unquoted scalar. A plain YAML scalar may not BEGIN with certain indicator
+    // characters. The set below was measured against a real parser rather than
+    // read off the spec, both with and without a following space, and only the
+    // characters invalid in *both* forms with no legitimate single-line use are
+    // rejected — a guard that flags valid frontmatter gets switched off.
+    //
+    // Deliberately not rejected, because each parses cleanly: `:` or `-` when NOT
+    // followed by a space (`:platform`, `-hyphenated`), `#`, `&anchor`, and
+    // `[`/`{`, which begin valid flow collections — `[a, b]` and `{a: b}` both
+    // parse, so flagging the opening bracket would reject valid frontmatter.
+    //
+    // An earlier version of this comment claimed a leading `,`, `]` or `}` also
+    // parsed cleanly. Measured against PyYAML and psych, all three are rejected;
+    // `,` is flagged below. `]` and `}` are left out only because no frontmatter
+    // has ever started with them — say so and they can be added.
+    //
+    // The backtick is the one that bites this repo: descriptions routinely name
+    // other skills, and "`other-skill` designs things" is a natural way to start.
+    if (/^[`@%*,]/.test(value)) {
+      errors.push(
+        `Frontmatter line ${lineNo} starts an unquoted value with '${value[0]}' — YAML reserves ` +
+        `that character and cannot begin a plain scalar with it, so a host parsing this ` +
+        `frontmatter rejects the whole file; wrap the value in quotes`
+      );
+      return;
+    }
+    // `|` and `>` open a block scalar only when nothing follows them on the line.
+    // `|` with indented lines under it is valid and stays allowed; `|foo` is not
+    // a block scalar, it is a plain scalar starting with an indicator, and both
+    // parsers reject it.
+    if (/^[|>]\S/.test(value)) {
+      errors.push(
+        `Frontmatter line ${lineNo} starts an unquoted value with '${value[0]}' followed by text — ` +
+        `YAML reads that as a block scalar header, which may not carry content on its own line, ` +
+        `so a host parsing this frontmatter rejects the whole file; wrap the value in quotes`
+      );
+      return;
+    }
+    if (/^[-?&](\s|$)/.test(value)) {
+      errors.push(
+        `Frontmatter line ${lineNo} starts an unquoted value with '${value[0]} ' — YAML reads ` +
+        `that as an indicator rather than text and rejects the file; wrap the value in quotes`
+      );
+      return;
+    }
+
+    // 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 ` +