From 0febaa0f6f750b93460cb365e96187150aa0392e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 1 Sep 2026 12:21:09 -0400 Subject: [PATCH] docs: address Copilot review round 7 on #2203 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strip YAML properties (tags, anchors, in either order) before deciding whether a description scalar is quoted — testing the first character alone read `© "Counts # safely"` as unquoted and rejected a `#` the quotes make harmless. Count the positive-case floor by distinct prompt rather than by entry: the floor exists for breadth, so five copies of one prompt cleared it while exercising exactly the single trigger it is meant to look past. Compare on collapsed, case-folded text so trivial edits do not read as a new situation. Re-aim the two positive prompts that measured the wrong thing — the project-structure case now poses a concrete "which piece handles what" question, and the test-servers case names running a real MCP server. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SyBPs7caoMcyvnp9ArMBhV Signed-off-by: cliffhall --- .../skills/project-structure/evals/evals.json | 2 +- .claude/skills/test-servers/evals/evals.json | 2 +- scripts/lib/skill-manifest.mjs | 31 ++++++-- scripts/lib/skill-manifest.test.mjs | 70 ++++++++++++++++++- 4 files changed, 97 insertions(+), 8 deletions(-) diff --git a/.claude/skills/project-structure/evals/evals.json b/.claude/skills/project-structure/evals/evals.json index e7df322d..241aa26f 100644 --- a/.claude/skills/project-structure/evals/evals.json +++ b/.claude/skills/project-structure/evals/evals.json @@ -1,6 +1,6 @@ [ { - "prompt": "This repo has three clients over a shared core. How should I orient myself in it before changing anything?", + "prompt": "Something in the web client's Node backend needs changing but I do not know which piece handles what. How do I work that out?", "expect": "project-structure" }, { diff --git a/.claude/skills/test-servers/evals/evals.json b/.claude/skills/test-servers/evals/evals.json index 237fece3..1456dac6 100644 --- a/.claude/skills/test-servers/evals/evals.json +++ b/.claude/skills/test-servers/evals/evals.json @@ -8,7 +8,7 @@ "expect": "test-servers" }, { - "prompt": "I need a real server to exercise this change against by hand. What do I run?", + "prompt": "I need to run a real MCP server locally to exercise this change by hand. Which one do I run, and how do I start it?", "expect": "test-servers" }, { diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index 683dd89e..6b019914 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -177,11 +177,19 @@ export function parseSkill(dirName, text) { ); if (start === -1) continue; const head = lines[start].slice(field.length + 1).trim(); + // A scalar may carry YAML properties — a tag, an anchor, or both, in either + // order — before its value. Testing the first character alone read + // `© "Counts # safely"` as unquoted and then rejected the `#` that the + // quotes make harmless, so strip the properties before deciding (Copilot). + const value = head.replace( + /^(?:(?:!(?:!?[\w:.-]*|<[^>]*>)|&[^\s]+)\s+)+/, + "", + ); // Only an unquoted scalar can lose text to a comment; a quoted one is safe, // and a block scalar (`|`/`>`) is read literally rather than comment-stripped. - if (/^["'|>]/.test(head)) continue; + if (/^["'|>]/.test(value)) continue; // Continuation lines are indented and are not the next `key:` of the map. - const parts = [head]; + const parts = [value]; for (let i = start + 1; i < lines.length; i++) { const l = lines[i]; if (!/^[ \t]/.test(l) || l.trim() === "") break; @@ -307,18 +315,31 @@ export function validateEvalCases(skillName, cases) { ); } }); - const positives = cases.filter((c) => c && c.expect === skillName).length; + const positiveCases = cases.filter((c) => c && c.expect === skillName); + const positives = positiveCases.length; + // The floor exists for breadth, so it has to count *phrasings*, not entries: + // five copies of one prompt clear an entry count while exercising exactly the + // one trigger the floor is meant to look past (Copilot). Compare on collapsed, + // case-folded text so trivial edits do not read as a new situation. + const distinctPositives = new Set( + positiveCases.map((c) => + String(c.prompt ?? "") + .toLowerCase() + .replace(/\s+/g, " ") + .trim(), + ), + ).size; const negatives = cases.filter((c) => c && c.expect === null).length; if (positives === 0) { errors.push(`no positive case expects \`${skillName}\``); - } else if (positives < MIN_POSITIVE_CASES) { + } else if (distinctPositives < MIN_POSITIVE_CASES) { // A non-zero floor is not enough: one prompt measures one phrasing. Since // each prompt is scored on its own `passes / RUNS`, extra prompts do not // make any single rate steadier — they cover more of the ways someone might // reach the skill, so a description that only fires on one narrow shape is // visible instead of passing on its single lucky case (Copilot). errors.push( - `only ${positives} positive case(s); a model-invoked skill needs at least ${MIN_POSITIVE_CASES} to cover the range of situations it should fire on`, + `only ${distinctPositives} distinct positive prompt(s)${positives === distinctPositives ? "" : ` across ${positives} case(s)`}; a model-invoked skill needs at least ${MIN_POSITIVE_CASES} to cover the range of situations it should fire on`, ); } if (negatives === 0) { diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index c1e5f0a9..125b08c7 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -426,7 +426,7 @@ test("a model-invoked skill needs at least MIN_POSITIVE_CASES positives", () => assert.match( validateEvalCases("alpha", withPositives(1)).join(" "), - /only 1 positive case\(s\)/, + /only 1 distinct positive prompt\(s\)/, ); assert.match( validateEvalCases("alpha", withPositives(MIN_POSITIVE_CASES - 1)).join(" "), @@ -533,3 +533,71 @@ test("the truncation guard ignores anchors and tags, which are not `#`", () => { /`description` is truncated/, ); }); + +test("a quoted scalar stays safe behind a tag or an anchor", () => { + // Properties may precede the value, in either order. Testing the first + // character alone read these as unquoted and then rejected the `#` that the + // quotes make harmless. + const mk = (value) => + [ + "---", + "name: alpha", + `description: ${value}`, + "disable-model-invocation: false", + "---", + "body", + ].join("\n"); + + for (const value of [ + '© "Counts # safely"', + '!!str "Counts # safely"', + '!!str &a "Counts # safely"', + ]) { + const parsed = parseSkill("alpha", mk(value)); + assert.deepEqual(parsed.errors, [], `rejected: ${value}`); + assert.equal(parsed.description, "Counts # safely"); + } +}); + +test("the positive floor counts distinct prompts, not entries", () => { + // The floor exists for breadth, so repeating one prompt five times clears an + // entry count while exercising exactly the single trigger it looks past. + const duplicated = [ + ...Array.from({ length: MIN_POSITIVE_CASES }, () => ({ + prompt: "the same prompt", + expect: "alpha", + })), + { prompt: "n", expect: null }, + ]; + const errors = validateEvalCases("alpha", duplicated).join(" "); + assert.match( + errors, + /only 1 distinct positive prompt\(s\) across 5 case\(s\)/, + ); + + // Whitespace and casing are not a new situation either. + const nearDuplicates = [ + { prompt: "How do I do the thing?", expect: "alpha" }, + { prompt: "how do i do the thing?", expect: "alpha" }, + { prompt: "How do I do the thing? ", expect: "alpha" }, + { prompt: "b", expect: "alpha" }, + { prompt: "c", expect: "alpha" }, + { prompt: "n", expect: null }, + ]; + assert.match( + validateEvalCases("alpha", nearDuplicates).join(" "), + /only 3 distinct positive prompt\(s\)/, + ); + + // Five genuinely different prompts pass. + assert.deepEqual( + validateEvalCases("alpha", [ + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `p${i}`, + expect: "alpha", + })), + { prompt: "n", expect: null }, + ]), + [], + ); +});