docs: address Copilot review round 2 on #2203
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 <cliff@futurescale.com>
This commit is contained in:
parent
50f3360d65
commit
880be3acdd
6 changed files with 136 additions and 29 deletions
|
|
@ -222,7 +222,9 @@ that from happening:
|
||||||
`evals/evals.json` — positives **and negatives**. Seeing a skill fire once tells
|
`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
|
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`
|
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 —
|
`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).
|
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
|
Two things learned writing the first set, both of which make a case measure
|
||||||
|
|
|
||||||
|
|
@ -209,13 +209,22 @@ break.
|
||||||
|
|
||||||
## Checklist for a new or edited skill
|
## Checklist for a new or edited skill
|
||||||
|
|
||||||
1. Frontmatter opens on line 1 (no BOM, no blank line), YAML is valid — an
|
1. Frontmatter opens on line 1 (no BOM, no blank line), YAML is valid, and any
|
||||||
unquoted colon in a description loads the body with an _empty_ description,
|
description containing `:` or `#` is **quoted**. An unquoted colon loads the
|
||||||
so `/name` still works while the skill can never auto-fire again.
|
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`.
|
2. `disable-model-invocation` is declared explicitly; default it to `false`.
|
||||||
3. Description is action-first, with `Use when …; when …; when …`.
|
3. Description is action-first, with `Use when …; when …; when …`.
|
||||||
4. `evals/evals.json` has both positives and at least one negative, every
|
4. **If the skill is model-invoked**, `evals/evals.json` carries **at least
|
||||||
positive shaped by the first-move rule and none answerable from `AGENTS.md`.
|
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.
|
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,
|
6. `RUNS=5 npm run skills:eval` — the **whole** suite — is ≥80% on every case,
|
||||||
including the skills you did not touch.
|
including the skills you did not touch.
|
||||||
|
|
|
||||||
|
|
@ -235,6 +235,12 @@ export function listingCost(skills) {
|
||||||
* @param {unknown} cases Parsed JSON.
|
* @param {unknown} cases Parsed JSON.
|
||||||
* @returns {string[]} errors
|
* @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) {
|
export function validateEvalCases(skillName, cases) {
|
||||||
if (!Array.isArray(cases) || cases.length === 0) {
|
if (!Array.isArray(cases) || cases.length === 0) {
|
||||||
return ["evals.json must be a non-empty array of cases"];
|
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;
|
const negatives = cases.filter((c) => c && c.expect === null).length;
|
||||||
if (positives === 0) {
|
if (positives === 0) {
|
||||||
errors.push(`no positive case expects \`${skillName}\``);
|
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) {
|
if (negatives === 0) {
|
||||||
errors.push(
|
errors.push(
|
||||||
|
|
|
||||||
|
|
@ -9,6 +9,7 @@ import assert from "node:assert/strict";
|
||||||
import {
|
import {
|
||||||
splitFrontmatter,
|
splitFrontmatter,
|
||||||
parseSkill,
|
parseSkill,
|
||||||
|
MIN_POSITIVE_CASES,
|
||||||
validateEvalCases,
|
validateEvalCases,
|
||||||
listingCost,
|
listingCost,
|
||||||
parseClaudeVersion,
|
parseClaudeVersion,
|
||||||
|
|
@ -175,15 +176,18 @@ test("listingCost counts only the skills that occupy the listing", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
test("validateEvalCases requires a positive and a negative", () => {
|
test("validateEvalCases requires a positive and a negative", () => {
|
||||||
|
const positives = (n, expect = "testing") =>
|
||||||
|
Array.from({ length: n }, (_, i) => ({ prompt: `p${i}`, expect }));
|
||||||
|
|
||||||
assert.deepEqual(
|
assert.deepEqual(
|
||||||
validateEvalCases("testing", [
|
validateEvalCases("testing", [
|
||||||
{ prompt: "a", expect: "testing" },
|
...positives(MIN_POSITIVE_CASES),
|
||||||
{ prompt: "b", expect: null },
|
{ prompt: "b", expect: null },
|
||||||
]),
|
]),
|
||||||
[],
|
[],
|
||||||
);
|
);
|
||||||
assert.match(
|
assert.match(
|
||||||
validateEvalCases("testing", [{ prompt: "a", expect: "testing" }]).join(),
|
validateEvalCases("testing", positives(MIN_POSITIVE_CASES)).join(),
|
||||||
/no negative case/,
|
/no negative case/,
|
||||||
);
|
);
|
||||||
assert.match(
|
assert.match(
|
||||||
|
|
@ -335,7 +339,10 @@ test("an eval file may only expect its own skill", () => {
|
||||||
assert.match(
|
assert.match(
|
||||||
validateEvalCases("testing", [
|
validateEvalCases("testing", [
|
||||||
{ prompt: "a", expect: "local-dev" },
|
{ 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 },
|
{ prompt: "c", expect: null },
|
||||||
]).join(),
|
]).join(),
|
||||||
/expects `local-dev`, but this file only measures `testing`/,
|
/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.
|
// Null and own-name cases are unaffected.
|
||||||
assert.deepEqual(
|
assert.deepEqual(
|
||||||
validateEvalCases("testing", [
|
validateEvalCases("testing", [
|
||||||
{ prompt: "a", expect: "testing" },
|
...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({
|
||||||
|
prompt: `p${i}`,
|
||||||
|
expect: "testing",
|
||||||
|
})),
|
||||||
{ prompt: "b", expect: null },
|
{ prompt: "b", expect: null },
|
||||||
]),
|
]),
|
||||||
[],
|
[],
|
||||||
|
|
@ -399,3 +409,37 @@ test("a `#` inside a quoted description is not treated as truncation", () => {
|
||||||
].join("\n");
|
].join("\n");
|
||||||
assert.deepEqual(parseSkill("alpha", quoted).errors, []);
|
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/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -22,6 +22,7 @@ import {
|
||||||
runPrompt,
|
runPrompt,
|
||||||
sampleHit,
|
sampleHit,
|
||||||
} from "./skill-eval.mjs";
|
} from "./skill-eval.mjs";
|
||||||
|
import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs";
|
||||||
|
|
||||||
const assistant = (...blocks) =>
|
const assistant = (...blocks) =>
|
||||||
JSON.stringify({ type: "assistant", message: { content: blocks } });
|
JSON.stringify({ type: "assistant", message: { content: blocks } });
|
||||||
|
|
@ -285,14 +286,17 @@ function skillsFixture() {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
write("alpha", "disable-model-invocation: false\n", [
|
// Enough positives to satisfy the same MIN_POSITIVE_CASES floor the real eval
|
||||||
{ prompt: "a+", expect: "alpha" },
|
// files must clear; the prompts stay predictable so assertions can name them.
|
||||||
{ prompt: "a-", expect: null },
|
const evalsFor = (letter, skill) => [
|
||||||
]);
|
...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({
|
||||||
write("beta", "disable-model-invocation: false\n", [
|
prompt: `${letter}+${i}`,
|
||||||
{ prompt: "b+", expect: "beta" },
|
expect: skill,
|
||||||
{ prompt: "b-", expect: null },
|
})),
|
||||||
]);
|
{ 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");
|
write("gamma", "disable-model-invocation: true\n");
|
||||||
return root;
|
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);
|
const all = collectCases(undefined, root);
|
||||||
assert.deepEqual([...all.ours].sort(), ["alpha", "beta"]);
|
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);
|
const focused = collectCases("alpha", root);
|
||||||
assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]);
|
assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]);
|
||||||
assert.deepEqual(
|
assert.deepEqual(
|
||||||
focused.cases.map((c) => c.prompt),
|
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:
|
// The consequence, stated as the assertion that matters:
|
||||||
assert.equal(
|
assert.equal(
|
||||||
|
|
@ -326,12 +330,23 @@ test("focused mode accepts several skill names", () => {
|
||||||
const root = skillsFixture();
|
const root = skillsFixture();
|
||||||
|
|
||||||
const focused = collectCases(["alpha", "beta"], root);
|
const focused = collectCases(["alpha", "beta"], root);
|
||||||
assert.deepEqual(focused.cases.map((c) => c.prompt).sort(), [
|
assert.deepEqual(
|
||||||
"a+",
|
focused.cases.map((c) => c.prompt).sort(),
|
||||||
"a-",
|
[
|
||||||
"b+",
|
"a+0",
|
||||||
"b-",
|
"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
|
// Narrowing to every skill is still not the same as no filter — `ours` is
|
||||||
// unchanged either way, which is what keeps negative scoring honest.
|
// unchanged either way, which is what keeps negative scoring honest.
|
||||||
assert.deepEqual([...focused.ours].sort(), ["alpha", "beta"]);
|
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.
|
// A single name keeps working, so the one-argument form is unaffected.
|
||||||
assert.deepEqual(
|
assert.deepEqual(
|
||||||
collectCases("beta", root).cases.map((c) => c.prompt),
|
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 });
|
rmSync(root, { recursive: true, force: true });
|
||||||
|
|
|
||||||
|
|
@ -23,6 +23,7 @@ import {
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
import { fileURLToPath } from "node:url";
|
import { fileURLToPath } from "node:url";
|
||||||
|
import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs";
|
||||||
|
|
||||||
import {
|
import {
|
||||||
checkWiring,
|
checkWiring,
|
||||||
|
|
@ -66,9 +67,14 @@ const byName = (name, extra = "") =>
|
||||||
const modelInvoked = (name) =>
|
const modelInvoked = (name) =>
|
||||||
`---\nname: ${name}\ndescription: A description.\ndisable-model-invocation: false\n---\n\nBody\n`;
|
`---\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) =>
|
const goodEvals = (name) =>
|
||||||
JSON.stringify([
|
JSON.stringify([
|
||||||
{ prompt: "fires", expect: name },
|
...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({
|
||||||
|
prompt: `fires ${i}`,
|
||||||
|
expect: name,
|
||||||
|
})),
|
||||||
{ prompt: "does not", expect: null },
|
{ 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", () => {
|
test("fails eval cases that are unreadable, all-positive, or all-negative", () => {
|
||||||
for (const [evals, pattern] of [
|
for (const [evals, pattern] of [
|
||||||
["{ not json", /not valid JSON/],
|
["{ 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([{ prompt: "a", expect: null }]), /no positive case/],
|
||||||
[JSON.stringify([{ expect: null }]), /prompt/],
|
[JSON.stringify([{ expect: null }]), /prompt/],
|
||||||
]) {
|
]) {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue