From ec2ef5656da1f52288351f3a9975028fd1d95012 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 1 Sep 2026 11:31:22 -0400 Subject: [PATCH] docs: address Copilot review round 5 on #2203 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The truncation guard read only the first physical line of the scalar, so a multiline plain scalar slipped past it: description: Covers boards #28 and #11 YAML parses that as `Covers boards`, and the first-line-only comparison saw `Covers boards` on both sides and reported no loss — passing the exact silent truncation the guard exists to reject. It now walks the whole plain scalar: the value line plus every indented continuation, whitespace-collapsed to match YAML's folding of newlines to spaces. Quoted and block scalars are still skipped, since neither loses text to a comment. Two regression cases: the multiline truncation, and a multiline scalar with no `#`, which must stay clean — the fix cannot be "reject anything multiline". No skill description or eval case changed, so skills:eval is unchanged from round 4's 63/63. local:gate passes, 34/34. 390/390 script tests. Signed-off-by: cliffhall --- scripts/lib/skill-manifest.mjs | 43 ++++++++++++++++++++++------- scripts/lib/skill-manifest.test.mjs | 33 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index 7e31c5a9..691669b3 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -156,27 +156,50 @@ export function parseSkill(dirName, text) { // non-empty string. `board-ops` shipped for a while with its two board numbers // and the option-deletion hazard cut off this way. Compare the raw scalar // against what YAML actually kept and make any loss an error. + // + // The comparison has to span the WHOLE plain scalar, not its first physical + // line. A plain scalar continues onto more-indented lines, so + // + // description: Covers boards + // #28 and #11 + // + // parses to `Covers boards` — and a first-line-only check compares that + // against `Covers boards` and sees no loss, passing the exact truncation this + // guard exists to reject (Copilot). + const lines = frontmatter.split("\n"); for (const [field, parsed] of [ ["description", description], ["when_to_use", whenToUse], ]) { if (typeof parsed !== "string") continue; - const raw = new RegExp("^" + field + ":[ \\t]*(.*)$", "m").exec( - frontmatter, - )?.[1]; - if (raw === undefined) continue; - const trimmed = raw.trim(); + const start = lines.findIndex((l) => + new RegExp("^" + field + ":([ \\t]|$)").test(l), + ); + if (start === -1) continue; + const head = lines[start].slice(field.length + 1).trim(); // Only an unquoted scalar can lose text to a comment; a quoted one is safe, - // and a block scalar (`|`/`>`) puts nothing on this line to compare. - if (/^["'|>]/.test(trimmed)) continue; - if (trimmed.length > parsed.trim().length) { + // and a block scalar (`|`/`>`) is read literally rather than comment-stripped. + if (/^["'|>]/.test(head)) continue; + // Continuation lines are indented and are not the next `key:` of the map. + const parts = [head]; + for (let i = start + 1; i < lines.length; i++) { + const l = lines[i]; + if (!/^[ \t]/.test(l) || l.trim() === "") break; + parts.push(l.trim()); + } + // YAML folds a plain scalar's newlines to single spaces, so compare on + // whitespace-collapsed text rather than on the raw characters. + const collapse = (s) => s.replace(/\s+/g, " ").trim(); + const rawText = collapse(parts.join(" ")); + const keptText = collapse(parsed); + if (rawText.length > keptText.length) { errors.push( "`" + field + "` is truncated by an unquoted `#` — YAML kept " + - parsed.trim().length + + keptText.length + " of " + - trimmed.length + + rawText.length + " characters; quote the value", ); } diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 6b9ec301..826d35b4 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -471,3 +471,36 @@ test("the truncation guard covers `when_to_use`, not just `description`", () => assert.deepEqual(ok.errors, []); assert.equal(ok.whenToUse, "Use when filing against board #28 or board #11."); }); + +test("the truncation guard spans a multiline plain scalar", () => { + // A plain scalar continues onto more-indented lines, so a first-line-only + // comparison sees `Covers boards` on both sides and reports no loss — passing + // the exact truncation this guard exists to reject. + const multiline = [ + "---", + "name: alpha", + "description: Covers boards", + " #28 and #11 and the hazard.", + "disable-model-invocation: false", + "---", + "body", + ].join("\n"); + const parsed = parseSkill("alpha", multiline); + assert.equal(parsed.description, "Covers boards"); + assert.match(parsed.errors.join(" "), /`description` is truncated/); + + // A multiline plain scalar with no `#` folds to one line and must stay clean, + // so the fix cannot be "reject anything multiline". + const folded = [ + "---", + "name: alpha", + "description: Covers boards", + " and the option-deletion hazard.", + "disable-model-invocation: false", + "---", + "body", + ].join("\n"); + const ok = parseSkill("alpha", folded); + assert.deepEqual(ok.errors, []); + assert.equal(ok.description, "Covers boards and the option-deletion hazard."); +});