From 880be3acdd7cdbe4302880e71d8d9b0b7d499c68 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 1 Sep 2026 10:32:41 -0400 Subject: [PATCH] docs: address Copilot review round 2 on #2203 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enforces the five-positive standard this PR established rather than only describing it. `validateEvalCases` checked `positives === 0`, so a skill could regress to a single prompt and still pass the gate. At RUNS=5 one case is 20 points of that skill's reading, so a one- or two-case skill reports a rate a single flake can move across the threshold — well-formed to the checker, meaningless as a measurement. Zero positives keeps its own clearer message so the common mistake keeps its diagnosis. Scopes the eval requirement to model-invoked skills in the checklist. A name-only skill has no eval file because nothing can measure a trigger only a human types, so an eval there is unrunnable rather than redundant. Aligns checklist item 1 with the `#` hazard from round 1; it named only the unquoted colon, which is the louder of the two failure modes. Four test fixtures asserted on one-positive eval files and legitimately went red; they now build positives from the exported MIN_POSITIVE_CASES. The all-positive row keeps testing the missing-negative path, with a new row for the below-floor case. 388/388 script tests pass. No skill description or eval case changed, so trigger behaviour is unaffected and skills:eval is unchanged from the previous round. Signed-off-by: cliffhall --- AGENTS.md | 4 ++- docs/skill-authoring.md | 19 ++++++++--- scripts/lib/skill-manifest.mjs | 15 +++++++++ scripts/lib/skill-manifest.test.mjs | 52 ++++++++++++++++++++++++++--- scripts/skill-eval.test.mjs | 49 +++++++++++++++++---------- scripts/verify-skills.main.test.mjs | 26 +++++++++++++-- 6 files changed, 136 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a5af8932..1ad3a0f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -222,7 +222,9 @@ that from happening: `evals/evals.json` — positives **and negatives**. Seeing a skill fire once tells you Claude found it, not that it finds it reliably, and a skill that fires on everything is a context regression that nobody notices by hand. `verify:skills` - requires both kinds; `npm run skills:eval` actually runs them (it needs the + requires both kinds — and at least **five** positives, since at `RUNS=5` a + skill measured by one or two prompts reports a rate a single flake can move + across the threshold; `npm run skills:eval` actually runs them (it needs the `claude` CLI and real model calls, so it is deliberately **not** in the gate — run it when adding a skill or editing a model-invoked description). Two things learned writing the first set, both of which make a case measure diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 6ba91097..83cb1a94 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -209,13 +209,22 @@ break. ## Checklist for a new or edited skill -1. Frontmatter opens on line 1 (no BOM, no blank line), YAML is valid — an - unquoted colon in a description loads the body with an _empty_ description, - so `/name` still works while the skill can never auto-fire again. +1. Frontmatter opens on line 1 (no BOM, no blank line), YAML is valid, and any + description containing `:` or `#` is **quoted**. An unquoted colon loads the + body with an _empty_ description, so `/name` still works while the skill can + never auto-fire again; an unquoted `#` is worse-behaved, truncating the + description from that point on so it stays non-empty and every check passes. 2. `disable-model-invocation` is declared explicitly; default it to `false`. 3. Description is action-first, with `Use when …; when …; when …`. -4. `evals/evals.json` has both positives and at least one negative, every - positive shaped by the first-move rule and none answerable from `AGENTS.md`. +4. **If the skill is model-invoked**, `evals/evals.json` carries **at least + five positives and at least one negative** — every positive shaped by the + first-move rule, none answerable from `AGENTS.md`. A name-only skill + (`disable-model-invocation: true`, as `release` is) has no eval file at all: + nothing can measure a trigger that only a human types, so an eval there would + be unrunnable rather than merely redundant. `verify:skills` enforces the + floor — fewer than five positives fails the gate, because at `RUNS=5` one + case is 20 points of that skill's reading and a two-case skill reports a rate + a single flake can move across the threshold. 5. `npm run verify:skills` passes and the listing is under budget. 6. `RUNS=5 npm run skills:eval` — the **whole** suite — is ≥80% on every case, including the skills you did not touch. diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index 76c7b584..65107b23 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -235,6 +235,12 @@ export function listingCost(skills) { * @param {unknown} cases Parsed JSON. * @returns {string[]} errors */ +/** + * The number of positive cases a model-invoked skill's eval file must carry. + * Raised from an implicit 3 to 5 so a single flake cannot cross the threshold. + */ +export const MIN_POSITIVE_CASES = 5; + export function validateEvalCases(skillName, cases) { if (!Array.isArray(cases) || cases.length === 0) { return ["evals.json must be a non-empty array of cases"]; @@ -268,6 +274,15 @@ export function validateEvalCases(skillName, cases) { 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) { + // A non-zero floor is not enough. At RUNS=5 one case is worth 20 points of + // that skill's reading, so a skill measured by one or two prompts reports a + // rate that a single flake can move across the threshold — and the gate, + // seeing a positive, would call that well-formed. Five is the count every + // skill here was raised to for exactly that reason (Copilot). + errors.push( + `only ${positives} positive case(s); a model-invoked skill needs at least ${MIN_POSITIVE_CASES} for its rate to mean anything`, + ); } if (negatives === 0) { errors.push( diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 8136d174..6f66686e 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -9,6 +9,7 @@ import assert from "node:assert/strict"; import { splitFrontmatter, parseSkill, + MIN_POSITIVE_CASES, validateEvalCases, listingCost, parseClaudeVersion, @@ -175,15 +176,18 @@ test("listingCost counts only the skills that occupy the listing", () => { }); test("validateEvalCases requires a positive and a negative", () => { + const positives = (n, expect = "testing") => + Array.from({ length: n }, (_, i) => ({ prompt: `p${i}`, expect })); + assert.deepEqual( validateEvalCases("testing", [ - { prompt: "a", expect: "testing" }, + ...positives(MIN_POSITIVE_CASES), { prompt: "b", expect: null }, ]), [], ); assert.match( - validateEvalCases("testing", [{ prompt: "a", expect: "testing" }]).join(), + validateEvalCases("testing", positives(MIN_POSITIVE_CASES)).join(), /no negative case/, ); assert.match( @@ -335,7 +339,10 @@ test("an eval file may only expect its own skill", () => { assert.match( validateEvalCases("testing", [ { prompt: "a", expect: "local-dev" }, - { prompt: "b", expect: "testing" }, + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `p${i}`, + expect: "testing", + })), { prompt: "c", expect: null }, ]).join(), /expects `local-dev`, but this file only measures `testing`/, @@ -350,7 +357,10 @@ test("an eval file may only expect its own skill", () => { // Null and own-name cases are unaffected. assert.deepEqual( validateEvalCases("testing", [ - { prompt: "a", expect: "testing" }, + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `p${i}`, + expect: "testing", + })), { prompt: "b", expect: null }, ]), [], @@ -399,3 +409,37 @@ test("a `#` inside a quoted description is not treated as truncation", () => { ].join("\n"); assert.deepEqual(parseSkill("alpha", quoted).errors, []); }); + +test("a model-invoked skill needs at least MIN_POSITIVE_CASES positives", () => { + // `positives === 0` alone let a skill regress to a single prompt and still + // pass the gate. At RUNS=5 one case is 20 points of that skill's reading, so + // a one- or two-case skill reports a rate a single flake can move across the + // threshold — well-formed to the checker, meaningless as a measurement. + const withPositives = (n) => [ + ...Array.from({ length: n }, (_, i) => ({ + prompt: `p${i}`, + expect: "alpha", + })), + { prompt: "n", expect: null }, + ]; + + assert.match( + validateEvalCases("alpha", withPositives(1)).join(" "), + /only 1 positive case\(s\)/, + ); + assert.match( + validateEvalCases("alpha", withPositives(MIN_POSITIVE_CASES - 1)).join(" "), + /needs at least 5/, + ); + assert.deepEqual( + validateEvalCases("alpha", withPositives(MIN_POSITIVE_CASES)), + [], + ); + + // Zero still reports the clearer "no positive case" message rather than the + // count one, so the common mistake keeps its specific diagnosis. + assert.match( + validateEvalCases("alpha", [{ prompt: "n", expect: null }]).join(" "), + /no positive case expects/, + ); +}); diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index a6b3ae99..7386f927 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -22,6 +22,7 @@ import { runPrompt, sampleHit, } from "./skill-eval.mjs"; +import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs"; const assistant = (...blocks) => JSON.stringify({ type: "assistant", message: { content: blocks } }); @@ -285,14 +286,17 @@ function skillsFixture() { ); } }; - write("alpha", "disable-model-invocation: false\n", [ - { prompt: "a+", expect: "alpha" }, - { prompt: "a-", expect: null }, - ]); - write("beta", "disable-model-invocation: false\n", [ - { prompt: "b+", expect: "beta" }, - { prompt: "b-", expect: null }, - ]); + // Enough positives to satisfy the same MIN_POSITIVE_CASES floor the real eval + // files must clear; the prompts stay predictable so assertions can name them. + const evalsFor = (letter, skill) => [ + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `${letter}+${i}`, + expect: skill, + })), + { prompt: `${letter}-`, expect: null }, + ]; + write("alpha", "disable-model-invocation: false\n", evalsFor("a", "alpha")); + write("beta", "disable-model-invocation: false\n", evalsFor("b", "beta")); write("gamma", "disable-model-invocation: true\n"); return root; } @@ -305,13 +309,13 @@ test("focused mode narrows the cases but not the repo's own skill set", () => { const all = collectCases(undefined, root); assert.deepEqual([...all.ours].sort(), ["alpha", "beta"]); - assert.equal(all.cases.length, 4); + assert.equal(all.cases.length, (MIN_POSITIVE_CASES + 1) * 2); const focused = collectCases("alpha", root); assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]); assert.deepEqual( focused.cases.map((c) => c.prompt), - ["a+", "a-"], + ["a+0", "a+1", "a+2", "a+3", "a+4", "a-"], ); // The consequence, stated as the assertion that matters: assert.equal( @@ -326,12 +330,23 @@ test("focused mode accepts several skill names", () => { const root = skillsFixture(); const focused = collectCases(["alpha", "beta"], root); - assert.deepEqual(focused.cases.map((c) => c.prompt).sort(), [ - "a+", - "a-", - "b+", - "b-", - ]); + assert.deepEqual( + focused.cases.map((c) => c.prompt).sort(), + [ + "a+0", + "a+1", + "a+2", + "a+3", + "a+4", + "a-", + "b+0", + "b+1", + "b+2", + "b+3", + "b+4", + "b-", + ].sort(), + ); // Narrowing to every skill is still not the same as no filter — `ours` is // unchanged either way, which is what keeps negative scoring honest. assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]); @@ -339,7 +354,7 @@ test("focused mode accepts several skill names", () => { // A single name keeps working, so the one-argument form is unaffected. assert.deepEqual( collectCases("beta", root).cases.map((c) => c.prompt), - ["b+", "b-"], + ["b+0", "b+1", "b+2", "b+3", "b+4", "b-"], ); rmSync(root, { recursive: true, force: true }); diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index a4d268ca..0dd9d3cc 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -23,6 +23,7 @@ import { import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs"; import { checkWiring, @@ -66,9 +67,14 @@ const byName = (name, extra = "") => const modelInvoked = (name) => `---\nname: ${name}\ndescription: A description.\ndisable-model-invocation: false\n---\n\nBody\n`; +// Clears the same MIN_POSITIVE_CASES floor the real eval files must, so this +// fixture keeps meaning "well-formed" as that floor moves. const goodEvals = (name) => JSON.stringify([ - { prompt: "fires", expect: name }, + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `fires ${i}`, + expect: name, + })), { prompt: "does not", expect: null }, ]); @@ -135,7 +141,23 @@ test("fails a model-invoked skill with no eval cases", () => { test("fails eval cases that are unreadable, all-positive, or all-negative", () => { for (const [evals, pattern] of [ ["{ not json", /not valid JSON/], - [JSON.stringify([{ prompt: "a", expect: "beta" }]), /no negative case/], + [ + JSON.stringify( + Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `a${i}`, + expect: "beta", + })), + ), + /no negative case/, + ], + // Below the floor is its own failure, distinct from having none at all. + [ + JSON.stringify([ + { prompt: "a", expect: "beta" }, + { prompt: "b", expect: null }, + ]), + /needs at least 5/, + ], [JSON.stringify([{ prompt: "a", expect: null }]), /no positive case/], [JSON.stringify([{ expect: null }]), /prompt/], ]) {