diff --git a/.claude/skills/test-servers/evals/evals.json b/.claude/skills/test-servers/evals/evals.json index 1456dac6..89502105 100644 --- a/.claude/skills/test-servers/evals/evals.json +++ b/.claude/skills/test-servers/evals/evals.json @@ -19,6 +19,20 @@ "prompt": "I need a fixture combination that doesn't exist yet. How do I add one?", "expect": "test-servers" }, + { + "prompt": "Write an integration test that exercises tool listing against a real server.", + "chain": [ + "testing", + "test-servers" + ] + }, + { + "prompt": "Add end-to-end coverage for tool-list pagination against a live server.", + "chain": [ + "testing", + "test-servers" + ] + }, { "prompt": "Sort this list alphabetically: banana, apple, cherry.", "expect": null diff --git a/AGENTS.md b/AGENTS.md index d5103dd7..cc9a5479 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -262,6 +262,20 @@ that from happening: ("how does the `@inspector/core` alias resolve?") invites a `Read`, which is a *better* answer than a skill. Good cases are "how do I / where does this go" questions whose answer is a procedure. + **A pointer from one skill's body to another is measured by a `chain` case, + not an `expect` one.** A first-move case can only observe the model's opening + tool call, so a skill reached only *through* another scores a clean 100% on + its direct cases while the hand-off silently never fires (#2204). A chained + case names the ordered skills one run should load, **ending with the skill + whose file it lives in** — so the file that goes red is the one belonging to + the skill that stopped being reached. It runs on a wider turn budget, is + scored against its own `CHAIN_THRESHOLD`, and is **reported in its own + column**: a hand-off rate and a first-move rate are not comparable, and + folding them together would move a headline everyone reads as trigger + reliability. It counts toward neither the five-positive floor nor the + negative requirement, and it is only worth writing where the first link's + body actually points at the target — a chain through a skill that says + nothing about it is a permanent 0% with no lever. ⚠️ **The gate cannot catch a description that never matches.** `verify:skills` checks that a skill is well-formed and that its cases exist; only `skills:eval` observes whether it actually fires, and that cannot be gated — diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 39c3683e..f65a3461 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -14,6 +14,17 @@ reachability — `npm run skills:eval` — reports a hit rate rather than a verd session** (`claude -p`), `RUNS` times, and scores the fraction of runs in which the `Skill` tool fired with the expected name. +There are **two kinds of case**, measured against different turn budgets and +reported in separate columns: + +| | asserts | budget | column | +| --- | --- | --- | --- | +| `"expect": ""` / `null` | the skill is (or is not) the model's **first move** | 1 turn | first-move | +| `"chain": ["a", …, ""]` | loading `a` **leads to** loading this skill | `CHAIN_MAX_TURNS` (14) | hand-off, `CHAIN_THRESHOLD` 0.5 | + +Almost every case is the first kind, and the four properties below are about +that kind. The hand-off case has its own section further down. + Four properties of that harness drive everything below: - **`--max-turns 1`.** The skill must fire in the model's **first assistant @@ -180,6 +191,86 @@ unrelated to the repo (arithmetic, trivia, a one-line refactor). All 18 in this repo have held at 100% through every reshaping so far — if one starts firing, a description has grown too broad. +### Chained cases: measuring a hand-off + +A skill body may point at another skill — `testing` opens by telling the model +that picking a fixture is `/test-servers` and that it has to load it, and +because `test-servers` is model-invocable that pointer is live rather than a +dead end. **Nothing in a first-move case can observe whether that pointer is +ever taken.** `test-servers` scores 5/5 on its own cases and every one of them +asks for it by name; a skill only ever reached _through_ another would score a +clean 100% while the hand-off silently never fired (#2204). + +A chained case names the ordered skills one run should load: + +```json +{ + "prompt": "Write an integration test that exercises tool listing against a real server.", + "chain": ["testing", "test-servers"] +} +``` + +**Write a chained case when the prompt names nothing about the target skill and +the path to it runs through another skill.** Write an ordinary first-move case +for everything else — a prompt someone would actually type to reach this skill +directly is a first-move case even when a hand-off could also get there, and it +is the cheaper measurement by an order of magnitude. + +Five rules the shape enforces, each for a reason worth knowing: + +- **The chain ends with the skill whose file it lives in.** The case exists to + measure whether _this_ skill is reachable, so the file that must go red when + the hand-off stops working is the one belonging to the skill that stops being + reached. Anchoring on the first link would file the `testing → test-servers` + measurement under `testing`, where a `test-servers` description edit would + never be seen. +- **A chained case satisfies neither floor.** It is not one of the five + positives and it is not the negative. It measures a different thing, so + letting it stand in would let a skill ship with no measurement of the way + users actually reach it. +- **The match is an ordered _subsequence_, not a prefix and not a contiguous + run.** The model may load something before the chain starts and something + unrelated in between; neither changes the claim that A led to B. What does not + score is the reverse order. +- **Repeats and unknown links are rejected.** A repeated link cannot be + observed, and a link naming a skill the model cannot invoke can never fire — + it would score a permanent 0% that reads as a description problem. +- **The two numbers never share a column.** A hand-off rate is a second-hop load + over many turns; a first-move rate is the model's opening move. Summing them + would produce a figure describing neither, and a handful of hand-off cases + would quietly move a headline everyone reads as trigger reliability. + +⚠️ **A chained case only measures a pointer that exists.** `pr-flow` says +nothing about test fixtures, so a `["pr-flow", "test-servers"]` case measured 0% +— correctly, and with no lever to fix it short of broadening a description onto +another skill's ground. Before writing one, confirm the first link's body +actually points at the target; otherwise the case is a permanent zero that reads +as a description problem. + +⚠️ **A hand-off case is a measurement under the harness's tool policy, not a +prediction about an unrestricted session.** `--max-turns 1` was doing much of +the read-only containment on its own; a 14-turn budget removes that, so the deny +list covers the agentic and network tools too (`Task` in particular, whose +subagent the flag does not reach). Denying `Bash` also changes the path a run +can take toward the second skill, since investigating a repo by hand often +starts there. `Read`/`Glob`/`Grep` remain, which is enough to reach a hand-off. + +**A hand-off is far less reliable than a first move, and the threshold says so.** +`CHAIN_THRESHOLD` defaults to **0.5**, not 0.8 — the weakest claim worth +asserting is that the pointer is taken more often than not. The two committed +`testing → test-servers` cases measure **67% and 33%** at `RUNS=3` against a +pointer stated in the first paragraph of `testing`'s body. At 0.8 both would be +red no matter how strongly the first skill pointed at the second, and the column +would stop carrying signal; at 0.5 the difference between them is the signal. +Read a hand-off number as a description-strength measurement, not a verdict — +and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points and +these two are one sample apart. + +⚠️ **Expect a hand-off to cost far more than a first move.** Each sample is up +to 14 turns rather than one, so a chained case is the most expensive line in the +suite by a wide margin — the two above take longer between them than all seven +first-move cases. + ## The tuning loop **Probe first, then measure.** A full suite run is ~63 cases × `RUNS` sessions @@ -211,6 +302,14 @@ npm run skills:eval # every model-invoked skill, RU RUNS=5 CONCURRENCY=6 npm run skills:eval npm run skills:eval -- testing # one skill's cases npm run skills:eval -- testing test-servers # a set of skills +CHAIN_THRESHOLD=0.4 CHAIN_MAX_TURNS=20 npm run skills:eval -- test-servers +``` + +The summary is two lines, never one: + +``` +7/7 first-move cases at or above 80%. +1/2 hand-off cases at or above 50%. ``` Narrowing the run never narrows what a **negative** case is scored against — a @@ -257,6 +356,12 @@ break. prompts do not steady any rate (`RUNS` is the knob for that). Five prompts cover five ways someone might arrive at the skill, which is what catches a description that fires on one narrow phrasing and nothing else. -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. +5. **If this skill is meant to be reachable from another skill's body**, that + hand-off has a `chain` case — a pointer between skills is otherwise measured + by nothing at all, and a skill reached only that way scores a clean 100% on + direct cases while the hand-off never fires. It does not count toward the + floor in 4. +6. `npm run verify:skills` passes and the listing is under budget. +7. `RUNS=5 npm run skills:eval` — the **whole** suite — is ≥80% on every + first-move case, including the skills you did not touch, and the hand-off + column is read on its own rather than against that number. diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index fe8f6147..1d59707b 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -250,6 +250,31 @@ export function listingCost(skills) { */ export const MIN_POSITIVE_CASES = 5; +/** + * The fewest links a chained case may name. + * + * A one-link "chain" is a first-move case written the long way — it asserts + * nothing about a hand-off, and accepting it would let a skill satisfy the + * hand-off column without ever measuring one. + */ +export const MIN_CHAIN_LENGTH = 2; + +/** + * Whether a case measures a hand-off (`chain`) rather than a first move + * (`expect`). + * + * The two are scored against different turn budgets and reported in different + * columns, so every consumer has to tell them apart; doing it by field + * presence in one place keeps that decision from drifting between the + * validator and the runner. + * + * @param {unknown} c + * @returns {boolean} + */ +export function isChainCase(c) { + return c !== null && typeof c === "object" && Array.isArray(c.chain); +} + /** * Validate an `evals/evals.json` payload for a model-invoked skill. * @@ -257,11 +282,30 @@ export const MIN_POSITIVE_CASES = 5; * is the failure nobody notices by hand — so negatives are required, not * optional. * + * A case is one of two shapes, and it must be exactly one: a **first-move** + * case names `expect` (the skill the model should reach with its first tool + * call, or `null`), and a **hand-off** case names `chain` — the ordered skills + * a single run should load, ending with this one. Requiring exactly one of the + * two rather than letting `chain` shadow `expect` means a case that carries + * both is a typo caught here, not a silently half-scored measurement. + * + * A hand-off case counts toward neither the positive floor nor the negative + * requirement. It measures a different thing (a second-hop load, over many + * turns) and it is scored in its own column, so letting one stand in for a + * first-move positive would let a skill ship with no measurement of the way + * users actually reach it. + * * @param {string} skillName * @param {unknown} cases Parsed JSON. + * @param {Set | null} [known] Every model-invoked skill in the repo. + * When supplied, each link of a `chain` is checked against it — a link naming + * a skill that does not exist, or one the model cannot invoke, can never fire + * and would score the case a permanent 0% that reads as a description + * problem. Omitted (null), the link names are left unchecked, so a caller + * that has not yet parsed the whole directory can still validate shape. * @returns {string[]} errors */ -export function validateEvalCases(skillName, cases) { +export function validateEvalCases(skillName, cases, known = null) { if (!Array.isArray(cases) || cases.length === 0) { return ["evals.json must be a non-empty array of cases"]; } @@ -274,11 +318,23 @@ export function validateEvalCases(skillName, cases) { if (typeof c.prompt !== "string" || c.prompt.trim() === "") { errors.push(`case ${i}: \`prompt\` must be a non-empty string`); } + if ("expect" in c && "chain" in c) { + errors.push( + `case ${i}: carries both \`expect\` and \`chain\` — a case is either a first-move case or a hand-off case`, + ); + return; + } + if ("chain" in c) { + errors.push(...validateChain(skillName, i, c.chain, known)); + return; + } if ( !("expect" in c) || (c.expect !== null && typeof c.expect !== "string") ) { - errors.push(`case ${i}: \`expect\` must be a skill name or null`); + errors.push( + `case ${i}: needs \`expect\` (a skill name, or null for a negative case) or \`chain\``, + ); } else if (c.expect !== null && c.expect !== skillName) { // A case living in this skill's evals may only expect THIS skill. A // foreign name passes the eval whenever that other skill fires, so the @@ -325,6 +381,57 @@ export function validateEvalCases(skillName, cases) { return errors; } +/** + * Validate one hand-off case's `chain`. + * + * The last link must be the owning skill, not the first. A hand-off case exists + * to measure whether **this** skill is reachable at all when nothing about the + * prompt names it, so the file that has to go red when the hand-off stops + * working is the one belonging to the skill that stops being reached. Anchoring + * on the first link instead would file the case under whichever skill happened + * to start the run, and a `test-servers` description edit would then be + * measured only inside `testing`. + * + * @param {string} skillName + * @param {number} i Case index, for the message. + * @param {unknown} chain + * @param {Set | null} known + * @returns {string[]} + */ +function validateChain(skillName, i, chain, known) { + if (!Array.isArray(chain) || chain.length < MIN_CHAIN_LENGTH) { + return [ + `case ${i}: \`chain\` must be an ordered array of at least ${MIN_CHAIN_LENGTH} skill names`, + ]; + } + const errors = []; + if (chain.some((n) => typeof n !== "string" || n.trim() === "")) { + errors.push(`case ${i}: every \`chain\` link must be a non-empty string`); + return errors; + } + if (new Set(chain).size !== chain.length) { + // A repeated link cannot be observed: the run records which skills fired + // in what order, and a second load of one already recorded is + // indistinguishable from the first. + errors.push(`case ${i}: \`chain\` repeats a skill name`); + } + if (chain[chain.length - 1] !== skillName) { + errors.push( + `case ${i}: \`chain\` ends with \`${chain[chain.length - 1]}\`, but this file measures whether \`${skillName}\` is reached`, + ); + } + if (known) { + for (const link of chain) { + if (!known.has(link)) { + errors.push( + `case ${i}: \`chain\` names \`${link}\`, which is not a model-invoked skill — it can never fire`, + ); + } + } + } + return errors; +} + /** * Claude Code version the authoritative validator is pinned to when it has to * be fetched. Pinned rather than @latest: a validator that moves on its own can diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 7a00aae8..da6f4de0 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -9,6 +9,8 @@ import assert from "node:assert/strict"; import { splitFrontmatter, parseSkill, + isChainCase, + MIN_CHAIN_LENGTH, MIN_POSITIVE_CASES, validateEvalCases, listingCost, @@ -204,8 +206,116 @@ test("validateEvalCases rejects malformed cases", () => { ); assert.match( validateEvalCases("x", [{ prompt: "a", expect: 7 }]).join(), - /expect. must be a skill name or null/, + /needs .expect./, ); + assert.match( + validateEvalCases("x", [{ prompt: "a" }]).join(), + /needs .expect. .* or .chain./, + ); +}); + +test("a hand-off case names an ordered chain ending in this skill", () => { + const base = [ + ...Array.from({ length: MIN_POSITIVE_CASES }, (_, i) => ({ + prompt: `p${i}`, + expect: "test-servers", + })), + { prompt: "n", expect: null }, + ]; + const withChain = (chain) => [...base, { prompt: "c", chain }]; + const known = new Set(["testing", "test-servers", "pr-flow"]); + + assert.deepEqual( + validateEvalCases( + "test-servers", + withChain(["testing", "test-servers"]), + known, + ), + [], + ); + // The case belongs to the skill that must be REACHED, so a chain anchored on + // its first link would file a `test-servers` measurement under `testing` and + // leave a `test-servers` description edit unmeasured by its own file. + assert.match( + validateEvalCases( + "test-servers", + withChain(["test-servers", "testing"]), + known, + ).join(), + /ends with .testing., but this file measures .* .test-servers./, + ); + assert.match( + validateEvalCases( + "test-servers", + withChain(["test-servers"]), + known, + ).join(), + new RegExp(`at least ${MIN_CHAIN_LENGTH} skill names`), + ); + assert.match( + validateEvalCases( + "test-servers", + withChain(["testing", "testing", "test-servers"]), + known, + ).join(), + /repeats a skill name/, + ); + assert.match( + validateEvalCases( + "test-servers", + withChain(["nope", "test-servers"]), + known, + ).join(), + /.nope., which is not a model-invoked skill/, + ); + assert.match( + validateEvalCases( + "test-servers", + withChain(["", "test-servers"]), + known, + ).join(), + /non-empty string/, + ); + // Without the known set the shape is still checked; only the link names go + // unverified, so a caller that has not parsed the directory can still run. + assert.deepEqual( + validateEvalCases("test-servers", withChain(["nope", "test-servers"])), + [], + ); +}); + +test("a hand-off case satisfies neither floor and never doubles as a first move", () => { + // A hand-off is a second-hop load over many turns and is scored in its own + // column. Letting one stand in for a first-move positive would let a skill + // ship with no measurement of the way users actually reach it. + const chained = Array.from({ length: MIN_POSITIVE_CASES + 1 }, (_, i) => ({ + prompt: `c${i}`, + chain: ["testing", "test-servers"], + })); + const errors = validateEvalCases("test-servers", chained).join(" "); + assert.match(errors, /no positive case/); + assert.match(errors, /no negative case/); +}); + +test("a case carrying both shapes is a typo, not a half-scored measurement", () => { + assert.match( + validateEvalCases("test-servers", [ + { + prompt: "c", + expect: "test-servers", + chain: ["testing", "test-servers"], + }, + ]).join(), + /carries both .expect. and .chain./, + ); +}); + +test("isChainCase tells the two shapes apart", () => { + assert.equal(isChainCase({ chain: ["a", "b"] }), true); + assert.equal(isChainCase({ expect: "a" }), false); + assert.equal(isChainCase({ expect: null }), false); + assert.equal(isChainCase(null), false); + assert.equal(isChainCase("chain"), false); }); test("parseClaudeVersion reads the CLI's version banner", () => { diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 5325c1b3..e759961d 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -29,6 +29,13 @@ // npm run skills:eval -- testing # one skill's cases // npm run skills:eval -- testing test-servers # several skills' cases // RUNS=5 THRESHOLD=0.8 npm run skills:eval +// +// Two kinds of case, measured and reported separately (#2204). A `expect` case +// is a FIRST-MOVE measurement: one turn, does the model reach for the skill +// before anything else. A `chain` case is a HAND-OFF measurement: many turns, +// does loading skill A actually lead the model to load skill B. The two numbers +// are not comparable — a hand-off is a second-hop load that only happens once +// the run has established it needs one — so they never share a column. import { spawn } from "node:child_process"; import { readFileSync, existsSync, readdirSync, statSync } from "node:fs"; @@ -36,6 +43,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { claudeSpawnArgs, probeClaudeVersion } from "./lib/claude-cli.mjs"; import { + isChainCase, parseClaudeVersion, parseSkill, validateEvalCases, @@ -45,9 +53,29 @@ const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const SKILLS_DIR = path.join(ROOT, ".claude", "skills"); const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); +// A hand-off is a harder thing to hit than a first move, and what counts as +// acceptable is a separate judgement rather than one inherited from a number +// tuned for the other measurement. 0.5 is the weakest claim worth asserting — +// the pointer is taken more often than not. It is deliberately not 0.8: the +// committed `testing` -> `test-servers` cases measure 33-67% (RUNS=3) against a +// pointer that is live and stated in the first paragraph of `testing`'s body, +// so an 0.8 bar would mark every hand-off red regardless of how strongly the +// first skill points at the second, and the column would stop carrying signal. +const CHAIN_THRESHOLD = Number(process.env.CHAIN_THRESHOLD ?? 0.5); const RUNS = Number(process.env.RUNS ?? 3); const CONCURRENCY = Number(process.env.CONCURRENCY ?? 4); +/** + * Turns a hand-off case gets. + * + * `--max-turns 1` is what makes a first-move case a first-move case, so a + * chained case needs a budget wide enough for the run to establish that it + * needs the second skill. #2204 measured the `testing` -> `test-servers` + * hand-off going 9-12 tool calls without reaching it; a budget under that + * cannot distinguish "the hand-off does not fire" from "the run was cut short". + */ +const CHAIN_MAX_TURNS = Number(process.env.CHAIN_MAX_TURNS ?? 14); + /** Collect the committed cases for every model-invoked skill (optionally one). */ /** * Collect the committed cases, and the set of skill names that are OURS. @@ -63,6 +91,13 @@ const CONCURRENCY = Number(process.env.CONCURRENCY ?? 4); * empty run: a typo would otherwise enqueue nothing and the eval would report a * green 0/0, which reads exactly like a clean pass of the skill you meant. * + * Collection is two passes. A hand-off case names other skills, and its links + * are checked against the repo's model-invoked set — which is only complete + * once every directory has been parsed. Validating inside the first pass would + * make the check depend on directory order: `test-servers` sorts before + * `testing`, so a chain through `testing` would be rejected as unknown purely + * because of where the alphabet put it. + * * @param {string | string[] | undefined} only One or more skill names. * @param {string} [skillsDir] * @returns {{ cases: object[], ours: Set }} @@ -74,6 +109,7 @@ export function collectCases(only, skillsDir = SKILLS_DIR) { : new Set(Array.isArray(only) ? only : [only]); const cases = []; const ours = new Set(); + const files = []; for (const dir of readdirSync(skillsDir).sort()) { const skillFile = path.join(skillsDir, dir, "SKILL.md"); if ( @@ -108,7 +144,10 @@ export function collectCases(only, skillsDir = SKILLS_DIR) { `${dir}/evals/evals.json is not valid JSON — ${e.message}`, ); } - const invalid = validateEvalCases(dir, parsed); + files.push({ dir, parsed }); + } + for (const { dir, parsed } of files) { + const invalid = validateEvalCases(dir, parsed, ours); if (invalid.length > 0) { throw new Error(`${dir}/evals/evals.json: ${invalid.join("; ")}`); } @@ -136,14 +175,20 @@ export function collectCases(only, skillsDir = SKILLS_DIR) { * of an eval run and would otherwise only ever be exercised by the thing they * are supposed to measure. * + * The invocations come back as an ORDERED array, repeats included, rather than + * a set. A hand-off case asserts that one skill was loaded *after* another, so + * occurrence order is the observation — and collapsing repeats would make a + * run that loaded B, then A, then B again indistinguishable from one that never + * reached B from A (#2204). + * * @param {string} text One or more newline-delimited JSON events. A trailing * partial line is ignored, so this can be fed incrementally. - * @returns {{ invoked: Set, rest: string, result: string | null }} + * @returns {{ invoked: string[], rest: string, result: string | null }} */ export function collectSkillInvocations(text) { const lines = text.split("\n"); const rest = lines.pop() ?? ""; - const invoked = new Set(); + const invoked = []; let result = null; for (const line of lines) { if (!line.trim()) continue; @@ -159,7 +204,7 @@ export function collectSkillInvocations(text) { for (const block of evt.message?.content ?? []) { if (block?.type !== "tool_use" || block.name !== "Skill") continue; // Don't assume the input field's name — match on the whole payload. - invoked.add(JSON.stringify(block.input ?? {})); + invoked.push(JSON.stringify(block.input ?? {})); } } return { invoked, rest, result }; @@ -231,7 +276,7 @@ export function invokedSkillNames(payload) { * skills (Copilot). * * @param {string | null} expect Skill name, or null for a negative case. - * @param {Set} invoked + * @param {Iterable} invoked * @param {Set | null} [ours] Repo skill names. Null counts any skill. */ export function sampleHit(expect, invoked, ours = null) { @@ -242,6 +287,76 @@ export function sampleHit(expect, invoked, ours = null) { return names.includes(expect); } +/** + * Whether one sample satisfies a hand-off case. + * + * The chain has to appear as an ordered SUBSEQUENCE of what fired, not as a + * prefix and not as a contiguous run. Two reasons, both of which a stricter + * match gets wrong: the model is free to load an unrelated skill in between, + * and it may well load something before the chain's first link — neither + * changes the fact that A led to B, which is the only claim the case makes. + * + * Nothing is asserted about foreign skills here, unlike a negative case. A + * hand-off case names exactly what it wants and a contributor's own + * `~/.claude/skills` entry firing alongside it says nothing either way. + * + * @param {string[]} chain Ordered skill names, ending with the owning skill. + * @param {Iterable} invoked + * @returns {boolean} + */ +export function chainHit(chain, invoked) { + const names = [...invoked].flatMap(invokedSkillNames); + let want = 0; + for (const name of names) { + if (name === chain[want]) want++; + if (want === chain.length) return true; + } + return false; +} + +/** + * Score one sample against whichever kind of case it belongs to. + * + * @param {{ expect?: string | null, chain?: string[] }} c + * @param {Iterable} invoked + * @param {Set | null} ours + */ +export function caseHit(c, invoked, ours) { + return isChainCase(c) + ? chainHit(c.chain, invoked) + : sampleHit(c.expect, invoked, ours); +} + +/** + * Tools no eval run may use, first-move or hand-off. + * + * A skill may inject `!`-prefixed shell commands on load, and those run BEFORE + * its content reaches the model — so the deny list is what keeps a measurement + * from having side effects. It matters more for a hand-off case than for a + * first-move one: `--max-turns 1` was doing much of the containment by itself, + * and a 14-turn budget removes that (#2204). Hence the agentic and network + * tools here too — `Task` would spawn a subagent whose own tool policy this + * flag does not reach. + * + * The cost is stated rather than hidden: denying `Bash` also changes the path + * a run can take toward the second skill, since investigating a repo by hand + * often starts there. `Read`/`Glob`/`Grep` remain, which is enough to reach a + * hand-off, but a chained rate is a measurement under this policy and not a + * prediction of an unrestricted session. + */ +const DISALLOWED_TOOLS = [ + "Bash", + "Write", + "Edit", + "NotebookEdit", + "Task", + "Agent", + "SlashCommand", + "WebFetch", + "WebSearch", + "KillShell", +].join(","); + /** * Drive one fresh session and return the payloads the `Skill` tool was called * with. @@ -252,12 +367,17 @@ export function sampleHit(expect, invoked, ours = null) { * silently reported a plausible hit rate for runs that never happened (Copilot). * * @param {string} prompt - * @param {{ spawnFn?: typeof spawn, cwd?: string }} [opts] - * @returns {Promise>} + * @param {{ spawnFn?: typeof spawn, cwd?: string, maxTurns?: number }} [opts] + * @returns {Promise} Skill payloads, in the order they fired. */ export function runPrompt( prompt, - { spawnFn = spawn, cwd = ROOT, platform = process.platform } = {}, + { + spawnFn = spawn, + cwd = ROOT, + platform = process.platform, + maxTurns = 1, + } = {}, ) { return new Promise((resolve, reject) => { // The prompt goes in on STDIN, not in argv. `claude -p` with piped stdin @@ -273,11 +393,10 @@ export function runPrompt( "stream-json", "--verbose", "--max-turns", - "1", - // Keep the run read-only. A skill may inject `!`-prefixed shell commands - // on load, and those run BEFORE its content reaches the model. + String(maxTurns), + // Keep the run read-only, across every turn it is given. "--disallowedTools", - "Bash,Write,Edit,NotebookEdit", + DISALLOWED_TOOLS, ], { cwd, stdio: ["pipe", "pipe", "inherit"] }, platform, @@ -285,12 +404,12 @@ export function runPrompt( const p = spawnFn(command, args, options); let buf = ""; - const invoked = new Set(); + const invoked = []; let result = null; p.stdout.on("data", (chunk) => { const parsed = collectSkillInvocations(buf + chunk.toString()); buf = parsed.rest; - for (const payload of parsed.invoked) invoked.add(payload); + for (const payload of parsed.invoked) invoked.push(payload); if (parsed.result !== null) result = parsed.result; }); p.on("error", reject); @@ -339,28 +458,55 @@ async function main() { const jobs = cases.flatMap((c) => Array.from({ length: RUNS }, () => c)); const results = await pool(jobs, CONCURRENCY, async (c) => ({ c, - invoked: await runPrompt(c.prompt), + invoked: await runPrompt(c.prompt, { + maxTurns: isChainCase(c) ? CHAIN_MAX_TURNS : 1, + }), })); - let failed = 0; - for (const c of cases) { - const mine = results.filter((r) => r.c === c); - const passes = mine.filter((r) => - sampleHit(c.expect, r.invoked, ours), - ).length; - const rate = passes / mine.length; - const ok = rate >= THRESHOLD; - if (!ok) failed++; - const label = c.expect ?? "(no skill)"; - console.log( - `${ok ? "PASS" : "FAIL"} ${(rate * 100).toFixed(0).padStart(3)}% ${label.padEnd(20)} ${c.prompt}`, - ); - } + /** Score and print one group, returning how many of its cases fell short. */ + const report = (group, heading, threshold) => { + if (group.length === 0) return 0; + console.log(`\n${heading}`); + let failed = 0; + for (const c of group) { + const mine = results.filter((r) => r.c === c); + const passes = mine.filter((r) => caseHit(c, r.invoked, ours)).length; + const rate = passes / mine.length; + const ok = rate >= threshold; + if (!ok) failed++; + const label = isChainCase(c) + ? c.chain.join(" → ") + : (c.expect ?? "(no skill)"); + console.log( + `${ok ? "PASS" : "FAIL"} ${(rate * 100).toFixed(0).padStart(3)}% ${label.padEnd(26)} ${c.prompt}`, + ); + } + return failed; + }; - console.log( - `\n${cases.length - failed}/${cases.length} cases at or above ${THRESHOLD * 100}%.`, + const direct = cases.filter((c) => !isChainCase(c)); + const chained = cases.filter(isChainCase); + const directFailed = report(direct, "First move (1 turn)", THRESHOLD); + const chainedFailed = report( + chained, + `Hand-off (${CHAIN_MAX_TURNS} turns)`, + CHAIN_THRESHOLD, ); - process.exit(failed > 0 ? 1 : 0); + + // Reported as two numbers, never one. A hand-off rate is a second-hop load + // over many turns and a first-move rate is the model's opening move; summing + // them would produce a figure that describes neither, and a handful of + // hand-off cases would quietly move a headline everyone reads as trigger + // reliability (#2204). + console.log( + `\n${direct.length - directFailed}/${direct.length} first-move cases at or above ${THRESHOLD * 100}%.`, + ); + console.log( + chained.length === 0 + ? "No hand-off cases in this selection." + : `${chained.length - chainedFailed}/${chained.length} hand-off cases at or above ${CHAIN_THRESHOLD * 100}%.`, + ); + process.exit(directFailed + chainedFailed > 0 ? 1 : 0); } if ( diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 7386f927..438e43bc 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -15,6 +15,8 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { + caseHit, + chainHit, collectCases, collectSkillInvocations, runRejection, @@ -24,6 +26,9 @@ import { } from "./skill-eval.mjs"; import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs"; +/** The payloads a run records, in the order the skills fired. */ +const fired = (...names) => names.map((n) => JSON.stringify({ skill: n })); + const assistant = (...blocks) => JSON.stringify({ type: "assistant", message: { content: blocks } }); const skillUse = (name) => ({ @@ -36,8 +41,8 @@ test("collectSkillInvocations finds Skill tool_use payloads", () => { const { invoked } = collectSkillInvocations( assistant(skillUse("testing")) + "\n", ); - assert.equal(invoked.size, 1); - assert.ok([...invoked][0].includes("testing")); + assert.equal(invoked.length, 1); + assert.ok(invoked[0].includes("testing")); }); test("collectSkillInvocations ignores other tools and other event types", () => { @@ -46,23 +51,23 @@ test("collectSkillInvocations ignores other tools and other event types", () => "\n" + JSON.stringify({ type: "result", result: "Skill" }) + "\n"; - assert.equal(collectSkillInvocations(text).invoked.size, 0); + assert.equal(collectSkillInvocations(text).invoked.length, 0); }); test("collectSkillInvocations survives malformed and blank lines", () => { const text = "not json\n\n" + assistant(skillUse("local-dev")) + "\n"; const { invoked } = collectSkillInvocations(text); - assert.equal(invoked.size, 1); + assert.equal(invoked.length, 1); }); test("collectSkillInvocations holds back a trailing partial line", () => { const whole = assistant(skillUse("local-dev")); const first = collectSkillInvocations(whole.slice(0, 20)); - assert.equal(first.invoked.size, 0); + assert.equal(first.invoked.length, 0); assert.equal(first.rest, whole.slice(0, 20)); // Feeding the remainder back with the held-over prefix completes the event. const second = collectSkillInvocations(first.rest + whole.slice(20) + "\n"); - assert.equal(second.invoked.size, 1); + assert.equal(second.invoked.length, 1); }); test("collectSkillInvocations tolerates a tool_use with no input", () => { @@ -71,15 +76,90 @@ test("collectSkillInvocations tolerates a tool_use with no input", () => { }); test("sampleHit scores positive and negative cases", () => { - const fired = new Set(['{"skill":"testing"}']); - const none = new Set(); - assert.equal(sampleHit("testing", fired), true); - assert.equal(sampleHit("local-dev", fired), false); + const hit = fired("testing"); + const none = []; + assert.equal(sampleHit("testing", hit), true); + assert.equal(sampleHit("local-dev", hit), false); assert.equal(sampleHit(null, none), true); - assert.equal(sampleHit(null, fired), false); + assert.equal(sampleHit(null, hit), false); assert.equal(sampleHit("testing", none), false); }); +test("collectSkillInvocations preserves order and repeats", () => { + // A hand-off case asserts one skill was loaded AFTER another, so occurrence + // order is the observation. Deduplicating into a Set would make the B, A, B + // run below indistinguishable from one that never reached B from A. + const text = + [ + assistant(skillUse("test-servers")), + assistant(skillUse("testing")), + assistant(skillUse("test-servers")), + ].join("\n") + "\n"; + const { invoked } = collectSkillInvocations(text); + assert.deepEqual( + invoked.map((p) => JSON.parse(p).skill), + ["test-servers", "testing", "test-servers"], + ); +}); + +test("chainHit wants the links in order", () => { + assert.equal(chainHit(["testing", "test-servers"], fired("testing")), false); + assert.equal( + chainHit(["testing", "test-servers"], fired("testing", "test-servers")), + true, + ); + // The reverse hand-off is a different claim and must not score. + assert.equal( + chainHit(["testing", "test-servers"], fired("test-servers", "testing")), + false, + ); + assert.equal(chainHit(["testing", "test-servers"], []), false); +}); + +test("chainHit matches a subsequence, not a prefix or a contiguous run", () => { + // The model is free to load something before the chain starts, and something + // unrelated in between — neither changes the fact that A led to B. + assert.equal( + chainHit( + ["testing", "test-servers"], + fired("local-dev", "testing", "board-ops", "test-servers"), + ), + true, + ); + // And a repeat of the first link before it does not consume the match. + assert.equal( + chainHit( + ["testing", "test-servers"], + fired("test-servers", "testing", "test-servers"), + ), + true, + ); +}); + +test("caseHit routes each case shape to its own scorer", () => { + const ours = new Set(["testing", "test-servers"]); + const run = fired("testing", "test-servers"); + assert.equal( + caseHit({ chain: ["testing", "test-servers"] }, run, ours), + true, + ); + assert.equal( + caseHit({ chain: ["test-servers", "testing"] }, run, ours), + false, + ); + assert.equal(caseHit({ expect: "testing" }, run, ours), true); + assert.equal(caseHit({ expect: null }, run, ours), false); + // A chained case says nothing about foreign skills, unlike a negative one. + assert.equal( + caseHit( + { chain: ["testing", "test-servers"] }, + fired("testing", "my-personal-notes", "test-servers"), + ours, + ), + true, + ); +}); + test("invokedSkillNames matches structurally, not by substring", () => { // `{"skill":"not-testing"}` contains "testing" and must NOT count — a // substring match inflates the measured hit rate with invocations of a @@ -87,16 +167,10 @@ test("invokedSkillNames matches structurally, not by substring", () => { assert.deepEqual(invokedSkillNames('{"skill":"not-testing"}'), [ "not-testing", ]); - assert.equal( - sampleHit("testing", new Set(['{"skill":"not-testing"}'])), - false, - ); - assert.equal(sampleHit("testing", new Set(['{"skill":"testing"}'])), true); + assert.equal(sampleHit("testing", fired("not-testing")), false); + assert.equal(sampleHit("testing", fired("testing")), true); // The field name is not assumed, so any string value is a candidate. - assert.equal( - sampleHit("testing", new Set(['{"name":"testing","args":""}'])), - true, - ); + assert.equal(sampleHit("testing", ['{"name":"testing","args":""}']), true); }); test("invokedSkillNames tolerates payloads it cannot read", () => { @@ -180,6 +254,46 @@ test("runPrompt collects invocations across chunk boundaries", async () => { assert.equal(sampleHit("testing", invoked), true); }); +test("runPrompt gives a hand-off case a wider turn budget", () => { + // `--max-turns 1` is what makes a first-move case a first-move case; a chain + // that only ever gets one turn can never observe a second-hop load. + const seen = []; + for (const opts of [{}, { maxTurns: 14 }]) { + runPrompt("p", { + ...opts, + spawnFn: (_c, args) => { + seen.push(args[args.indexOf("--max-turns") + 1]); + const c = new EventEmitter(); + c.stdout = new EventEmitter(); + c.stdin = { end: () => {} }; + queueMicrotask(() => c.emit("close", 0)); + return c; + }, + }).catch(() => {}); + } + assert.deepEqual(seen, ["1", "14"]); +}); + +test("runPrompt keeps the run read-only across every turn", () => { + // A wider budget removes the containment `--max-turns 1` was doing on its + // own, so the deny list has to cover the agentic and network tools too — + // `Task` in particular, whose subagent this flag does not reach. + let denied; + runPrompt("p", { + spawnFn: (_c, args) => { + denied = args[args.indexOf("--disallowedTools") + 1].split(","); + const c = new EventEmitter(); + c.stdout = new EventEmitter(); + c.stdin = { end: () => {} }; + queueMicrotask(() => c.emit("close", 0)); + return c; + }, + }).catch(() => {}); + for (const tool of ["Bash", "Write", "Edit", "NotebookEdit", "Task"]) { + assert.ok(denied.includes(tool), `${tool} must be denied`); + } +}); + test("runPrompt rejects a run that produced no terminal result", async () => { await assert.rejects( runPrompt("p", { spawnFn: fakeSpawn({ code: 1 }) }), @@ -211,12 +325,12 @@ test("a negative case ignores skills that are not this repo's", () => { // a negative prompt says nothing about these skills — failing on it would be // a false failure about someone else's environment. const ours = new Set(["testing", "local-dev"]); - const foreign = new Set(['{"skill":"my-personal-notes"}']); - const mine = new Set(['{"skill":"testing"}']); + const foreign = fired("my-personal-notes"); + const mine = fired("testing"); assert.equal(sampleHit(null, foreign, ours), true); assert.equal(sampleHit(null, mine, ours), false); - assert.equal(sampleHit(null, new Set(), ours), true); + assert.equal(sampleHit(null, [], ours), true); // A positive case is unaffected: it names the skill it wants. assert.equal(sampleHit("testing", mine, ours), true); assert.equal(sampleHit("testing", foreign, ours), false); @@ -318,10 +432,7 @@ test("focused mode narrows the cases but not the repo's own skill set", () => { ["a+0", "a+1", "a+2", "a+3", "a+4", "a-"], ); // The consequence, stated as the assertion that matters: - assert.equal( - sampleHit(null, new Set(['{"skill":"beta"}']), focused.ours), - false, - ); + assert.equal(sampleHit(null, fired("beta"), focused.ours), false); rmSync(root, { recursive: true, force: true }); }); @@ -378,7 +489,7 @@ test("a name-only skill is not part of the repo's model-invoked set", () => { const { ours } = collectCases(undefined, root); assert.equal(ours.has("gamma"), false); // So its firing does not fail a negative case — it cannot fire on its own. - assert.equal(sampleHit(null, new Set(['{"skill":"gamma"}']), ours), true); + assert.equal(sampleHit(null, fired("gamma"), ours), true); rmSync(root, { recursive: true, force: true }); }); diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index 4371d81b..7704ec78 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -130,6 +130,36 @@ test("fails a skill that does not declare its invocation mode", () => { rmSync(dir, { recursive: true, force: true }); }); +test("a hand-off case is checked against the whole model-invoked set", () => { + // The ordering trap this pins: `zeta` sorts AFTER `beta`, so validating each + // file as it is read would reject a chain through `zeta` as unknown purely + // because of where the alphabet put it. + const withChain = (chain) => + JSON.stringify([ + ...JSON.parse(goodEvals("beta")), + { prompt: "reached the long way", chain }, + ]); + + const ok = fixture({ + beta: { skill: modelInvoked("beta"), evals: withChain(["zeta", "beta"]) }, + zeta: { skill: modelInvoked("zeta"), evals: goodEvals("zeta") }, + }); + const passed = run(ok); + assert.equal(passed.code, 0, passed.out); + rmSync(ok, { recursive: true, force: true }); + + // A link the model cannot invoke can never fire, so it would score a + // permanent 0% that reads as a description problem rather than a typo. + const bad = fixture({ + alpha: { skill: byName("alpha") }, + beta: { skill: modelInvoked("beta"), evals: withChain(["alpha", "beta"]) }, + }); + const failed = run(bad); + assert.equal(failed.code, 1); + assert.match(failed.out, /`alpha`, which is not a model-invoked skill/); + rmSync(bad, { recursive: true, force: true }); +}); + test("fails a model-invoked skill with no eval cases", () => { const dir = fixture({ beta: { skill: modelInvoked("beta") } }); const { code, out } = run(dir); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 4124302a..f1909bb3 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -254,6 +254,8 @@ function main(argv = process.argv.slice(2)) { } const parsed = []; + const evalFiles = []; + const modelInvokedDirs = new Set(); for (const dir of dirs) { const file = path.join(SKILLS_DIR, dir, "SKILL.md"); if (!existsSync(file)) { @@ -266,6 +268,7 @@ function main(argv = process.argv.slice(2)) { parsed.push(skill); if (skill.modelInvoked) { + modelInvokedDirs.add(dir); const evalsFile = path.join(SKILLS_DIR, dir, "evals", "evals.json"); if (!existsSync(evalsFile)) { failures.push( @@ -280,9 +283,21 @@ function main(argv = process.argv.slice(2)) { failures.push(`${dir}/evals/evals.json: not valid JSON — ${e.message}`); continue; } - for (const e of validateEvalCases(dir, cases)) { - failures.push(`${dir}/evals/evals.json: ${e}`); - } + evalFiles.push({ dir, cases }); + } + } + + // Validated after the loop, not inside it: a hand-off case names other + // skills, and checking those links needs the model-invoked set complete. + // Inside the loop the check would depend on directory order — `test-servers` + // sorts before `testing`, so a chain through `testing` would be rejected as + // unknown purely because of where the alphabet put it. + // Keyed on the DIRECTORY name, which is what a chain link names and what a + // skill is addressed by; the frontmatter `name` can be absent or disagree, + // and either would silently shrink the set a chain is checked against. + for (const { dir, cases } of evalFiles) { + for (const e of validateEvalCases(dir, cases, modelInvokedDirs)) { + failures.push(`${dir}/evals/evals.json: ${e}`); } }