diff --git a/.claude/skills/test-servers/evals/evals.json b/.claude/skills/test-servers/evals/evals.json index 1456dac66..28fecc3b4 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 end to end.", + "chain": [ + "testing", + "test-servers" + ] + }, + { + "prompt": "Add end-to-end coverage for the tool-list pagination path.", + "chain": [ + "testing", + "test-servers" + ] + }, { "prompt": "Sort this list alphabetically: banana, apple, cherry.", "expect": null diff --git a/AGENTS.md b/AGENTS.md index d5103dd71..cc9a5479c 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 39c3683ed..6e5158332 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,125 @@ 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. + +Six 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. +- **Every link after the first must land in a later assistant turn.** Position + in the stream is not causation: the model can emit several `tool_use` blocks + in one message, and it has not seen the first skill's body when it does — so + two `Skill` calls in the same turn are parallel guesses, not a hand-off, and + a flat index would score them as one (Copilot). This is the difference + between "B was loaded after A" and "A led to B", and it is the second way a + chained case can false-pass — the first being a prompt that carries the + target's own trigger, below. Only the chain's *first* link is unconstrained. +- **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. + +⚠️ **The prompt must not carry the TARGET skill's own trigger.** This is the +subtle way a chained case false-passes. `test-servers` claims the situation "a +change needs a real server to exercise it", so a prompt saying "…against a real +server" matches it directly: the model can pick `testing` first and then pick +`test-servers` from the *original prompt*, in that order, and the case scores a +hit that would survive deleting the pointer from `testing` entirely (Copilot). +Both committed cases said "against a real/live server" and were rewritten to +"end to end" for exactly this reason — and the measured rate **fell from 100% +and 67% to 33% and 33%**, which is the size of the effect this trap hides. +**Write the prompt so only the loaded first skill can introduce the second**, +and sanity-check it by asking whether the case would still pass if the pointer +were removed. + +⚠️ **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 — and it is compared +**strictly**. "More often than not" is `> 0.5`, and an inclusive compare would +pass 2/4 whenever `RUNS` is even, reporting a result the criterion does not +license (Copilot). A strict bound of `1.0` is therefore unreachable and the +harness rejects it up front rather than failing every case. + +At 0.8 a hand-off case would be red no matter how strongly the first skill +pointed at the second, and the column would stop carrying 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. + +**The committed cases have measured 33% / 33% on one `RUNS=3` run and 100% / +33% on another, and at least one of them being red is the intended state rather +than an oversight.** `skills:eval` is not a gate (see below), and the number is +the finding: `testing` points at `test-servers` in its first paragraph and the +model follows that pointer *sometimes*. Strengthening it is its own change +against its own issue (#2247); lowering the bar to turn the column green would +throw away the only signal this feature adds. + +⚠️ **Do not read a rise between two `RUNS=3` runs as an improvement.** One +sample is 33 points there, and the two runs above straddle a 67-point swing on +the same prompt with no change to the pointer. Note in particular that the +turn-boundary rule added later can only ever *lower* a chained score — it +rejects matches a flatter reading accepted — so a higher number after it is +noise by construction, not an effect. `RUNS=5` is the smallest honest setting +for a hand-off, and the cost is real: each sample is up to 14 turns. + +⚠️ **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 @@ -194,11 +324,35 @@ prompt fires at all, and only then spend a full run on its rate: # snippet also runs under bash. printf '%s' "" \ | claude -p --output-format stream-json --verbose --max-turns 1 \ - --disallowedTools Bash,Write,Edit,NotebookEdit \ + --tools Read,Glob,Grep,Skill --allowedTools Read,Glob,Grep,Skill \ + --disallowedTools Bash,Write,Edit,NotebookEdit,Task,Agent,SlashCommand,WebFetch,WebSearch,KillShell \ + --strict-mcp-config \ | jq -r 'select(.message.content?) | .message.content[]? | select(.type == "tool_use") | .name' | head -3 ``` +⚠️ **`--tools` is the restriction; `--allowedTools` only pre-approves.** +Dropping the first leaves the bound resting on the deny list alone, so a tool a +user's or a plugin's settings already permit stays reachable for all 14 turns +(Copilot). Keep both. + +⚠️ **These flags are a copy of the harness's, so they go stale.** Whenever +`runPrompt` in `scripts/skill-eval.mjs` changes its tool policy, change this +snippet in the same edit — a probe that may call a tool the eval forbids +predicts nothing, which is the whole reason the two are meant to match +(Copilot). To probe a **hand-off** instead, raise `--max-turns` to +`CHAIN_MAX_TURNS` and drop the `head -3`: + +```sh +printf '%s' "" \ + | claude -p --output-format stream-json --verbose --max-turns 14 \ + --tools Read,Glob,Grep,Skill --allowedTools Read,Glob,Grep,Skill \ + --disallowedTools Bash,Write,Edit,NotebookEdit,Task,Agent,SlashCommand,WebFetch,WebSearch,KillShell \ + --strict-mcp-config \ + | jq -r 'select(.message.content?) | .message.content[]? + | select(.type == "tool_use" and .name == "Skill") | .input.skill' +``` + **Probe a marginal case more than once.** A prompt that fires on a single probe can still measure 60% over five runs — one sample cannot distinguish "reliable" from "coin flip". Three probes is enough to tell a solid replacement from a @@ -211,6 +365,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 above 50%. ``` Narrowing the run never narrows what a **negative** case is scored against — a @@ -257,6 +419,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 fe8f61472..1d59707bb 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 7a00aae86..da6f4de0a 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 5325c1b3f..748cdaba3 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,35 @@ 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% (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. +// +// It is compared STRICTLY, unlike the first-move threshold. "More often than +// not" is `> 0.5`, and an inclusive compare passes exactly half the samples +// whenever RUNS is even — 2/4 would report a pass the stated criterion does not +// license (Copilot). A consequence worth knowing: a strict bound of 1.0 can +// never be met, so it is rejected below rather than silently failing every case. +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 +97,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 +115,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 +150,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,15 +181,33 @@ 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). + * + * Each entry also carries the **assistant event** it came from, and that + * boundary is what makes the order mean something. The model may emit several + * `tool_use` blocks in one message, and it cannot see the first skill's body + * until the message after — so two `Skill` calls in the SAME event are + * concurrent guesses, not a hand-off, however they happen to be ordered inside + * the array. Flattening the stream and reading position alone would score that + * as `A` leading to `B` (Copilot); `chainHit` requires a later turn instead. + * * @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 }} + * @param {number} [turnOffset] Assistant events already seen, so a stream fed + * in chunks keeps one monotonic turn count rather than restarting per chunk. + * @returns {{ invoked: {payload: string, turn: number}[], rest: string, + * result: string | null, nextTurn: number }} */ -export function collectSkillInvocations(text) { +export function collectSkillInvocations(text, turnOffset = 0) { const lines = text.split("\n"); const rest = lines.pop() ?? ""; - const invoked = new Set(); + const invoked = []; let result = null; + let turn = turnOffset; for (const line of lines) { if (!line.trim()) continue; let evt; @@ -156,13 +219,26 @@ export function collectSkillInvocations(text) { } if (evt?.type === "result") result = evt.subtype ?? null; if (evt?.type !== "assistant") continue; + // One assistant event is one turn: everything inside it was decided at + // once, before any of its results came back. + turn++; 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({ payload: JSON.stringify(block.input ?? {}), turn }); } } - return { invoked, rest, result }; + return { invoked, rest, result, nextTurn: turn }; +} + +/** + * The skill names one recorded invocation asked for. + * + * @param {{payload: string} | string} entry + * @returns {string[]} + */ +function entryNames(entry) { + return invokedSkillNames(typeof entry === "string" ? entry : entry.payload); } /** @@ -230,18 +306,135 @@ export function invokedSkillNames(payload) { * false failure about someone else's environment rather than about these * skills (Copilot). * + * Turn boundaries are irrelevant here — a first-move case asks only whether a + * skill fired at all — so this reads the names and ignores the rest. + * * @param {string | null} expect Skill name, or null for a negative case. - * @param {Set} invoked + * @param {Iterable<{payload: string} | string>} invoked * @param {Set | null} [ours] Repo skill names. Null counts any skill. */ export function sampleHit(expect, invoked, ours = null) { - const names = [...invoked].flatMap(invokedSkillNames); + const names = [...invoked].flatMap(entryNames); if (expect === null) { return ours === null ? names.length === 0 : !names.some((n) => ours.has(n)); } 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. + * + * Every link after the first must land in a **strictly later assistant turn** + * than the one before. Position in the stream is not causation: the model can + * emit several `tool_use` blocks in one message, and it has not seen the first + * skill's body when it does, so two `Skill` calls in the same turn are parallel + * guesses that a flat index would happily score as a hand-off (Copilot). This + * is the whole difference between "B was loaded after A" and "A led to B". + * + * The scan stays greedy, which is still correct under that constraint: taking + * the EARLIEST occurrence of a link can only leave more room for the rest, so + * no later starting point could succeed where the greedy one fails. + * + * 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<{payload: string, turn: number}>} invoked + * @returns {boolean} + */ +export function chainHit(chain, invoked) { + let want = 0; + let prevTurn = -Infinity; + for (const entry of invoked) { + if (!entryNames(entry).includes(chain[want])) continue; + // A link in the same turn as the previous one cannot have been caused by + // it — the model had not seen that skill's body yet. + if (want > 0 && !(entry.turn > prevTurn)) continue; + prevTurn = entry.turn; + 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); +} + +/** + * The only tools an eval run needs: read the repo, and load a skill. + * + * Enumerating what is AVAILABLE rather than only what is denied is the load- + * bearing half. A deny list cannot bound a 14-turn run, because it only names + * the tools known when it was written: this checkout configures an HTTP + * `mcp-docs` server in `.mcp.json`, and a contributor's own MCP servers and + * plugins add more tools that no list here has ever seen (Copilot). Naming the + * four the harness actually needs closes that by construction. + * + * The list goes to `--tools`, which selects from the built-in set, AND to + * `--allowedTools`, which pre-approves. The distinction matters and cost us a + * round: `--allowedTools` grants permission, it does not filter availability, + * so a tool a user's or a plugin's settings already permit would still have + * been reachable across those 14 turns (Copilot). `--tools` is the restriction; + * `--allowedTools` keeps the four from needing a prompt no headless run can + * answer. + */ +const ALLOWED_TOOLS = ["Read", "Glob", "Grep", "Skill"]; + +/** + * Tools no eval run may use, first-move or hand-off. + * + * Kept alongside the allow list rather than replaced by it: a deny is + * unconditional, while an allow list governs which tools are pre-approved, so + * the two together are stricter than either. A skill may inject `!`-prefixed + * shell commands on load, and those run BEFORE its content reaches the model — + * so this is what keeps a measurement from having side effects. It matters + * more for a hand-off case than 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 too — `Task` would spawn a + * subagent whose own tool policy neither flag reaches. + * + * MCP servers are dropped outright with `--strict-mcp-config` (and no + * `--mcp-config`) rather than named here, since their tool names are not + * knowable from this file. What remains outside all three mechanisms is a + * contributor's own plugin tools; `--bare` would remove those and skills with + * them, which would measure nothing. + * + * 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", +]; + /** * Drive one fresh session and return the payloads the `Skill` tool was called * with. @@ -252,12 +445,18 @@ 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<{payload: string, turn: number}[]>} Skill invocations, in + * the order they fired, each tagged with the assistant turn it came from. */ 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 +472,17 @@ 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: what the + // harness needs, minus what it must never do, minus every MCP server + // this checkout or the contributor happens to configure. + "--tools", + ALLOWED_TOOLS.join(","), + "--allowedTools", + ALLOWED_TOOLS.join(","), "--disallowedTools", - "Bash,Write,Edit,NotebookEdit", + DISALLOWED_TOOLS.join(","), + "--strict-mcp-config", ], { cwd, stdio: ["pipe", "pipe", "inherit"] }, platform, @@ -285,12 +490,19 @@ export function runPrompt( const p = spawnFn(command, args, options); let buf = ""; - const invoked = new Set(); + const invoked = []; let result = null; + // Carried across chunks so the turn count is monotonic over the whole + // stream rather than restarting at each read. + let turnOffset = 0; p.stdout.on("data", (chunk) => { - const parsed = collectSkillInvocations(buf + chunk.toString()); + const parsed = collectSkillInvocations( + buf + chunk.toString(), + turnOffset, + ); buf = parsed.rest; - for (const payload of parsed.invoked) invoked.add(payload); + turnOffset = parsed.nextTurn; + for (const entry of parsed.invoked) invoked.push(entry); if (parsed.result !== null) result = parsed.result; }); p.on("error", reject); @@ -320,7 +532,122 @@ async function pool(items, n, fn) { return out; } +/** + * Whether a measured rate clears its bar. + * + * The comparison differs by case kind, and the difference is the point. A + * first-move threshold is a floor to reach (`>=` 0.8 means four of five). A + * chain threshold states "the pointer is taken more often than not", which is + * strictly `> 0.5` — an inclusive compare would pass 2/4 whenever RUNS is even + * and report a result the stated criterion does not license (Copilot). + * + * @param {number} rate + * @param {number} threshold + * @param {boolean} strict + */ +export function passesThreshold(rate, threshold, strict) { + return strict ? rate > threshold : rate >= threshold; +} + +/** + * Render the whole report, and say how many cases fell short. + * + * Extracted and exported so the SEPARATION itself is testable. The acceptance + * criterion of #2204 is that a hand-off rate is never folded into the + * first-move headline, and until this was a function that claim had no + * automated coverage at all: the tests exercised the scorers and the turn + * budget while the reporting — the thing that could silently merge the two + * measurements — lived inside `main` where nothing could reach it (Copilot). + * + * @param {object[]} cases + * @param {{c: object, invoked: Iterable}[]} results One per sample. + * @param {Set | null} ours + * @param {{threshold: number, chainThreshold: number, chainMaxTurns: number}} opts + * @returns {{ lines: string[], failed: number }} + */ +export function formatReport(cases, results, ours, opts) { + const lines = []; + let failed = 0; + + const group = (members, heading, threshold, strict) => { + if (members.length === 0) return 0; + lines.push("", heading); + let short = 0; + for (const c of members) { + const mine = results.filter((r) => r.c === c); + const passes = mine.filter((r) => caseHit(c, r.invoked, ours)).length; + const rate = mine.length === 0 ? 0 : passes / mine.length; + const ok = mine.length > 0 && passesThreshold(rate, threshold, strict); + if (!ok) short++; + const label = isChainCase(c) + ? c.chain.join(" → ") + : (c.expect ?? "(no skill)"); + lines.push( + `${ok ? "PASS" : "FAIL"} ${(rate * 100).toFixed(0).padStart(3)}% ${label.padEnd(26)} ${c.prompt}`, + ); + } + return short; + }; + + const direct = cases.filter((c) => !isChainCase(c)); + const chained = cases.filter(isChainCase); + const directShort = group( + direct, + "First move (1 turn)", + opts.threshold, + false, + ); + const chainedShort = group( + chained, + `Hand-off (${opts.chainMaxTurns} turns)`, + opts.chainThreshold, + true, + ); + failed = directShort + chainedShort; + + // Two numbers, never one. A hand-off 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). + lines.push(""); + if (direct.length > 0) { + lines.push( + `${direct.length - directShort}/${direct.length} first-move cases at or above ${opts.threshold * 100}%.`, + ); + } + lines.push( + chained.length === 0 + ? "No hand-off cases in this selection." + : `${chained.length - chainedShort}/${chained.length} hand-off cases above ${opts.chainThreshold * 100}%.`, + ); + return { lines, failed }; +} + async function main() { + // The chain bar is strict, so 1.0 cannot be cleared by any run and would fail + // every hand-off case while looking like a trigger problem. NaN and negative + // values are rejected for the same reason from the other side: `Number("abc")` + // is NaN, which fails every comparison and prints an `above NaN%` summary, + // and a negative bar passes every chain unconditionally — both turn an + // advertised knob into a measurement that quietly means nothing (Copilot). + if ( + !Number.isFinite(CHAIN_THRESHOLD) || + CHAIN_THRESHOLD < 0 || + CHAIN_THRESHOLD >= 1 + ) { + console.error( + `skills:eval — CHAIN_THRESHOLD must be a number in [0, 1) (got ${process.env.CHAIN_THRESHOLD ?? CHAIN_THRESHOLD}); it is a strict lower bound.`, + ); + process.exit(1); + } + if (!Number.isFinite(THRESHOLD) || THRESHOLD < 0 || THRESHOLD > 1) { + // The first-move bar is inclusive, so 1.0 is meetable and allowed; the + // non-finite and negative cases fail the same way as above. + console.error( + `skills:eval — THRESHOLD must be a number in [0, 1] (got ${process.env.THRESHOLD ?? THRESHOLD}).`, + ); + process.exit(1); + } if (probeClaudeVersion(parseClaudeVersion) === null) { console.error( "skills:eval — no usable `claude` CLI on PATH. This eval needs one.", @@ -339,27 +666,17 @@ 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}`, - ); - } - - console.log( - `\n${cases.length - failed}/${cases.length} cases at or above ${THRESHOLD * 100}%.`, - ); + const { lines, failed } = formatReport(cases, results, ours, { + threshold: THRESHOLD, + chainThreshold: CHAIN_THRESHOLD, + chainMaxTurns: CHAIN_MAX_TURNS, + }); + for (const line of lines) console.log(line); process.exit(failed > 0 ? 1 : 0); } diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 7386f9273..97d4b5b5a 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -11,10 +11,16 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { EventEmitter } from "node:events"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { + caseHit, + chainHit, + formatReport, + passesThreshold, collectCases, collectSkillInvocations, runRejection, @@ -24,6 +30,20 @@ import { } from "./skill-eval.mjs"; import { MIN_POSITIVE_CASES } from "./lib/skill-manifest.mjs"; +/** + * What a run records, one skill per assistant turn — the shape a genuine + * hand-off has. + */ +const fired = (...names) => + names.map((n, i) => ({ payload: JSON.stringify({ skill: n }), turn: i + 1 })); + +/** Several skills emitted together in ONE assistant turn. */ +const firedTogether = (turn, ...names) => + names.map((n) => ({ payload: JSON.stringify({ skill: n }), turn })); + +/** The eval script itself, for the cases that must exercise `main`'s guards. */ +const SCRIPT_PATH = fileURLToPath(new URL("./skill-eval.mjs", import.meta.url)); + const assistant = (...blocks) => JSON.stringify({ type: "assistant", message: { content: blocks } }); const skillUse = (name) => ({ @@ -36,8 +56,9 @@ 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].payload.includes("testing")); + assert.equal(invoked[0].turn, 1); }); test("collectSkillInvocations ignores other tools and other event types", () => { @@ -46,40 +67,194 @@ 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", () => { const text = assistant({ type: "tool_use", name: "Skill" }) + "\n"; - assert.deepEqual([...collectSkillInvocations(text).invoked], ["{}"]); + assert.deepEqual(collectSkillInvocations(text).invoked, [ + { payload: "{}", turn: 1 }, + ]); }); 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((e) => [JSON.parse(e.payload).skill, e.turn]), + [ + ["test-servers", 1], + ["testing", 2], + ["test-servers", 3], + ], + ); +}); + +test("collectSkillInvocations tags each invocation with its assistant turn", () => { + // Two `Skill` blocks in ONE message share a turn: the model chose both before + // seeing either result, so nothing in that message can have caused anything + // else in it. The turn is what lets `chainHit` tell that apart from a + // hand-off; a flat index cannot. + const text = + [ + assistant(skillUse("testing"), skillUse("test-servers")), + assistant(skillUse("board-ops")), + ].join("\n") + "\n"; + const { invoked, nextTurn } = collectSkillInvocations(text); + assert.deepEqual( + invoked.map((e) => [JSON.parse(e.payload).skill, e.turn]), + [ + ["testing", 1], + ["test-servers", 1], + ["board-ops", 2], + ], + ); + // The count carries across chunks, so a stream read in pieces stays monotonic. + assert.equal(nextTurn, 2); + const more = collectSkillInvocations( + assistant(skillUse("local-dev")) + "\n", + nextTurn, + ); + assert.equal(more.invoked[0].turn, 3); +}); + +test("chainHit refuses two skills loaded in the same assistant turn", () => { + // The finding this pins: the model can emit several `tool_use` blocks in one + // message, and it has NOT seen the first skill's body when it does. So an + // [A, B] pair from one message is two parallel guesses, and scoring it as a + // hand-off would report a causal link that cannot exist — a case that would + // keep passing after the pointer in `testing` was deleted. + assert.equal( + chainHit( + ["testing", "test-servers"], + firedTogether(1, "testing", "test-servers"), + ), + false, + ); + // The same two skills one turn apart is the real thing. + assert.equal( + chainHit( + ["testing", "test-servers"], + [...firedTogether(1, "testing"), ...firedTogether(2, "test-servers")], + ), + true, + ); + // A same-turn pair does not poison a later genuine hand-off. + assert.equal( + chainHit( + ["testing", "test-servers"], + [ + ...firedTogether(1, "testing", "test-servers"), + ...firedTogether(2, "test-servers"), + ], + ), + true, + ); + // Only the FIRST link is unconstrained; every later one needs a new turn. + assert.equal( + chainHit( + ["local-dev", "testing", "test-servers"], + [ + ...firedTogether(1, "local-dev"), + ...firedTogether(2, "testing", "test-servers"), + ], + ), + false, + ); +}); + +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 +262,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 +349,185 @@ 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 bounds the run by what it needs, not only by what it forbids", () => { + // A deny list only names the tools known when it was written. This checkout + // configures an HTTP `mcp-docs` server in `.mcp.json`, and a contributor's + // own MCP servers and plugins add more that no list here has seen — over 14 + // turns those can reach the network or mutate state. + let args; + runPrompt("p", { + spawnFn: (_c, a) => { + args = a; + const c = new EventEmitter(); + c.stdout = new EventEmitter(); + c.stdin = { end: () => {} }; + queueMicrotask(() => c.emit("close", 0)); + return c; + }, + }).catch(() => {}); + // `--tools` is the availability filter and is what actually bounds the run. + // `--allowedTools` only pre-approves: a tool a user's or a plugin's settings + // already permit would still be reachable across 14 turns without this. + for (const flag of ["--tools", "--allowedTools"]) { + assert.deepEqual(args[args.indexOf(flag) + 1].split(","), [ + "Read", + "Glob", + "Grep", + "Skill", + ]); + } + // No `--mcp-config` accompanies it, so this drops every configured server. + assert.ok(args.includes("--strict-mcp-config")); + assert.ok(!args.includes("--mcp-config")); +}); + +test("a malformed threshold is rejected rather than silently measured", () => { + // `Number("abc")` is NaN, which fails every comparison and would print an + // `above NaN%` summary; a negative bar passes every chain unconditionally. + // Either turns an advertised env knob into a measurement that means nothing. + const run = (env) => + spawnSync(process.execPath, [SCRIPT_PATH], { + encoding: "utf8", + env: { ...process.env, ...env }, + }); + + for (const bad of ["abc", "-0.5", "1", "1.5"]) { + const { status, stderr } = run({ CHAIN_THRESHOLD: bad }); + assert.equal(status, 1, `CHAIN_THRESHOLD=${bad} must be rejected`); + assert.match(stderr, /CHAIN_THRESHOLD must be a number in \[0, 1\)/); + assert.ok(stderr.includes(bad), "the message names the offending value"); + } + // The first-move bar is inclusive, so 1 is legitimate there and only the + // nonsensical values are refused. + for (const bad of ["abc", "-1", "1.5"]) { + const { status, stderr } = run({ THRESHOLD: bad }); + assert.equal(status, 1, `THRESHOLD=${bad} must be rejected`); + assert.match(stderr, /THRESHOLD must be a number in \[0, 1\]/); + } +}); + +test("passesThreshold is a floor for a first move and strictly above for a chain", () => { + // "More often than not" is `> 0.5`. An inclusive compare passes 2/4 whenever + // RUNS is even, reporting a result the stated criterion does not license. + assert.equal(passesThreshold(0.5, 0.5, true), false); + assert.equal(passesThreshold(2 / 3, 0.5, true), true); + // A first-move threshold is a floor to REACH: 4/5 clears 0.8 exactly. + assert.equal(passesThreshold(0.8, 0.8, false), true); + assert.equal(passesThreshold(0.6, 0.8, false), false); +}); + +const OPTS = { threshold: 0.8, chainThreshold: 0.5, chainMaxTurns: 14 }; +/** `RUNS` samples of one case, `hits` of which fired the whole chain/skill. */ +const samples = (c, hits, runs) => + Array.from({ length: runs }, (_, i) => ({ + c, + invoked: i < hits ? fired(...(c.chain ?? [c.expect])) : [], + })); + +test("the report keeps the two measurements in separate columns", () => { + // The acceptance criterion of #2204: a hand-off rate is never folded into + // the first-move headline. Nothing covered this while it lived in `main`. + const direct = { prompt: "d", expect: "test-servers" }; + const chain = { prompt: "c", chain: ["testing", "test-servers"] }; + const { lines, failed } = formatReport( + [direct, chain], + [...samples(direct, 3, 3), ...samples(chain, 1, 3)], + new Set(["testing", "test-servers"]), + OPTS, + ); + const text = lines.join("\n"); + assert.match(text, /First move \(1 turn\)/); + assert.match(text, /Hand-off \(14 turns\)/); + assert.match(text, /1\/1 first-move cases at or above 80%\./); + assert.match(text, /0\/1 hand-off cases above 50%\./); + // One summary line per kind, and no line that merges them. + assert.equal(text.match(/cases (at or above|above)/g).length, 2); + assert.equal(failed, 1, "the chained case is short, the direct one is not"); +}); + +test("each group is scored against its own threshold", () => { + // 2/3 clears the chain bar strictly but would fail the first-move bar, so a + // single shared threshold would misreport whichever kind it was not tuned for. + const direct = { prompt: "d", expect: "test-servers" }; + const chain = { prompt: "c", chain: ["testing", "test-servers"] }; + const { lines, failed } = formatReport( + [direct, chain], + [...samples(direct, 2, 3), ...samples(chain, 2, 3)], + new Set(["testing", "test-servers"]), + OPTS, + ); + const text = lines.join("\n"); + assert.match(text, /FAIL\s+67%\s+test-servers/); + assert.match(text, /PASS\s+67%\s+testing → test-servers/); + assert.equal(failed, 1); +}); + +test("a single-kind selection reports only that kind, and says so", () => { + const direct = { prompt: "d", expect: "test-servers" }; + const only = formatReport( + [direct], + samples(direct, 3, 3), + new Set(["test-servers"]), + OPTS, + ); + assert.match(only.lines.join("\n"), /No hand-off cases in this selection\./); + assert.doesNotMatch(only.lines.join("\n"), /Hand-off \(14 turns\)/); + assert.equal(only.failed, 0); + + // And a chain-only selection prints no first-move headline or summary. + const chain = { prompt: "c", chain: ["testing", "test-servers"] }; + const chainOnly = formatReport( + [chain], + samples(chain, 3, 3), + new Set(["testing", "test-servers"]), + OPTS, + ); + const text = chainOnly.lines.join("\n"); + assert.doesNotMatch(text, /first-move cases/); + assert.match(text, /1\/1 hand-off cases above 50%\./); + assert.equal(chainOnly.failed, 0); +}); + test("runPrompt rejects a run that produced no terminal result", async () => { await assert.rejects( runPrompt("p", { spawnFn: fakeSpawn({ code: 1 }) }), @@ -211,12 +559,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 +666,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 +723,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 4371d81b7..7704ec785 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 4124302a0..f1909bb35 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}`); } }