From 75940976f8afae853132035390d37181bcf8442f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 19:01:19 -0400 Subject: [PATCH 1/5] feat(skills): measure a skill reached from another skill (#2204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skills:eval` ran every case with `--max-turns 1`, so it measured one thing: whether a skill is the model's FIRST tool call. That is right for a skill a user reaches directly, and it leaves a whole class unmeasured — a skill reached from inside another skill's body. `testing` tells the model that picking a fixture is `/test-servers` and that it has to load it, and `test-servers` scores 5/5 while every one of those five cases asks for it by name. A skill only ever reachable through a hand-off would score a clean 100% with the hand-off silently never firing. An eval case is now one of two shapes, and exactly one: { "prompt": "…", "expect": "test-servers" } first move, 1 turn { "prompt": "…", "chain": ["testing", "test-servers"] } hand-off, 14 turns A chain must END with the skill whose file it lives in. The case exists to measure whether THIS skill is reachable, so the file that goes red is the one belonging to the skill that stopped being reached; anchoring on the first link would file a `test-servers` measurement under `testing`, where a `test-servers` description edit would never be seen. Links are checked against the repo's model-invoked set, which is why both `verify:skills` and `collectCases` now validate in a second pass — `test-servers` sorts before `testing`, so a per-directory check would reject a live chain as unknown purely because of where the alphabet put it. Scoring 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, and neither changes the claim that A led to B. `collectSkillInvocations` therefore returns an ordered array rather than a Set — a B, A, B run has to stay distinguishable from one that never reached B from A. The two rates are reported in separate columns and never summed. A hand-off is a second-hop load over many turns; a first-move rate is the model's opening move. `CHAIN_MAX_TURNS` (14) and `CHAIN_THRESHOLD` are their own knobs for the same reason, and `CHAIN_THRESHOLD` defaults to **0.5** rather than inheriting 0.8: see the measurement below. Read-only containment no longer leans on `--max-turns 1`, which was doing much of it by itself. The deny list gains the agentic and network tools — `Task` in particular, whose subagent the flag does not reach. Measured, `skills:eval -- test-servers`: First move (1 turn) 7/7 at 100%, unchanged Hand-off (14 turns) 67% and 33% at RUNS=3 So the hand-off is real and unreliable — which is the fact nothing could observe before this change, and the reason 0.5 rather than 0.8 is the default bar: at 0.8 both committed cases are red no matter how strongly the first skill points at the second, and the column stops carrying signal. A `["pr-flow", "test-servers"]` probe measured 0% and was dropped rather than kept: `pr-flow` says nothing about fixtures, so the case had no lever short of broadening a description onto another skill's ground. Both findings are written up in `docs/skill-authoring.md`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe Signed-off-by: cliffhall --- .claude/skills/test-servers/evals/evals.json | 14 ++ AGENTS.md | 14 ++ docs/skill-authoring.md | 111 +++++++++- scripts/lib/skill-manifest.mjs | 111 +++++++++- scripts/lib/skill-manifest.test.mjs | 112 +++++++++- scripts/skill-eval.mjs | 208 ++++++++++++++++--- scripts/skill-eval.test.mjs | 167 ++++++++++++--- scripts/verify-skills.main.test.mjs | 30 +++ scripts/verify-skills.mjs | 21 +- 9 files changed, 720 insertions(+), 68 deletions(-) diff --git a/.claude/skills/test-servers/evals/evals.json b/.claude/skills/test-servers/evals/evals.json index 1456dac66..895021058 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 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..f65a34619 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 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..e759961d7 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; + }; + 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, + ); + + // 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( - `\n${cases.length - failed}/${cases.length} cases at or above ${THRESHOLD * 100}%.`, + 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(failed > 0 ? 1 : 0); + process.exit(directFailed + chainedFailed > 0 ? 1 : 0); } if ( diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 7386f9273..438e43bcb 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 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}`); } } From f20bbac474895f45451e9f130cf5e6f96969aa5f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 19:33:51 -0400 Subject: [PATCH 2/5] feat(skills): address Copilot review round 1 on #2204 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all real. **The chain prompts carried the target skill's own trigger.** `test-servers` claims "a change needs a real server to exercise it", and both cases said "against a real/live server" — so the model could load `testing`, then pick `test-servers` from the ORIGINAL prompt, in that order, and score a hit that would have survived deleting the pointer from `testing` entirely. Rewritten to carry no server cue ("end to end"), and the measured rate fell from 100% / 67% to 33% / 33%. That is the size of the artefact, and 33% is what the pointer alone is worth. Both cases are now red against the 0.5 bar and stay that way: `skills:eval` is not a gate, the number IS the finding, and lowering the bar to turn the column green would discard the only signal this feature adds. Filed as #2247 with the trap written down so a fix cannot re-measure the artefact. **A deny list cannot bound a 14-turn run.** It only names tools known when it was written, and this checkout configures an HTTP `mcp-docs` server in `.mcp.json` while a contributor's own MCP servers and plugins add more. Now `--allowedTools Read,Glob,Grep,Skill` (what the harness actually needs) plus `--strict-mcp-config` with no `--mcp-config`, which drops every configured server. The deny list stays alongside, since a deny is unconditional while an allow list governs pre-approval. What remains outside all three is a contributor's plugin tools; `--bare` would remove those and skills with them, which would measure nothing — so that residual is stated rather than papered over. **The chain threshold compared inclusively.** "More often than not" is `> 0.5`, and `>=` passes exactly half whenever RUNS is even — 2/4 would report a result the criterion does not license. `passesThreshold` is now strict for chains and a floor for first moves, and a strict bound of 1.0 is rejected up front rather than failing every case while looking like a trigger problem. **The doc's probe command still denied only four tools**, contradicting the section's own promise that a probe and a scored case see the same policy. Updated, with a note that it must move with the harness, plus a hand-off probe form. **The reporting had no automated coverage** — the one thing that could silently merge the two measurements lived inside `main`. Extracted as `formatReport` and tested: both headings, per-group thresholds, the two separate summaries, the combined exit, and both single-kind selections. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe Signed-off-by: cliffhall --- .claude/skills/test-servers/evals/evals.json | 4 +- docs/skill-authoring.md | 61 +++++- scripts/skill-eval.mjs | 203 ++++++++++++++----- scripts/skill-eval.test.mjs | 111 ++++++++++ 4 files changed, 312 insertions(+), 67 deletions(-) diff --git a/.claude/skills/test-servers/evals/evals.json b/.claude/skills/test-servers/evals/evals.json index 895021058..28fecc3b4 100644 --- a/.claude/skills/test-servers/evals/evals.json +++ b/.claude/skills/test-servers/evals/evals.json @@ -20,14 +20,14 @@ "expect": "test-servers" }, { - "prompt": "Write an integration test that exercises tool listing against a real server.", + "prompt": "Write an integration test that exercises tool listing end to end.", "chain": [ "testing", "test-servers" ] }, { - "prompt": "Add end-to-end coverage for tool-list pagination against a live server.", + "prompt": "Add end-to-end coverage for the tool-list pagination path.", "chain": [ "testing", "test-servers" diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index f65a34619..fef908926 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -240,6 +240,19 @@ Five rules the shape enforces, each for a reason worth knowing: 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 @@ -257,14 +270,23 @@ 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. +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. + +**Both committed cases currently sit at 33% and are therefore red, and that 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 about a third of the time. +Strengthening it is its own change against its own issue; lowering the bar to +turn the column green would throw away the only signal this feature adds. ⚠️ **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 @@ -285,11 +307,30 @@ 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 \ + --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 ``` +⚠️ **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 \ + --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 @@ -309,7 +350,7 @@ 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%. +0/2 hand-off cases above 50%. ``` Narrowing the run never narrows what a **negative** case is scored against — a diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index e759961d7..ac61887d7 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -57,10 +57,16 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // 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 +// 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); @@ -327,16 +333,36 @@ export function caseHit(c, invoked, ours) { : sampleHit(c.expect, invoked, ours); } +/** + * The only tools an eval run needs: read the repo, and load a skill. + * + * Enumerating what is ALLOWED 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. + */ +const ALLOWED_TOOLS = ["Read", "Glob", "Grep", "Skill"]; + /** * 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. + * 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 @@ -355,7 +381,7 @@ const DISALLOWED_TOOLS = [ "WebFetch", "WebSearch", "KillShell", -].join(","); +]; /** * Drive one fresh session and return the payloads the `Skill` tool was called @@ -394,9 +420,14 @@ export function runPrompt( "--verbose", "--max-turns", String(maxTurns), - // Keep the run read-only, across every turn it is given. + // 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. + "--allowedTools", + ALLOWED_TOOLS.join(","), "--disallowedTools", - DISALLOWED_TOOLS, + DISALLOWED_TOOLS.join(","), + "--strict-mcp-config", ], { cwd, stdio: ["pipe", "pipe", "inherit"] }, platform, @@ -439,7 +470,106 @@ 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() { + if (CHAIN_THRESHOLD >= 1) { + // 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. + console.error( + `skills:eval — CHAIN_THRESHOLD must be below 1 (got ${CHAIN_THRESHOLD}); it is a strict lower bound.`, + ); + process.exit(1); + } if (probeClaudeVersion(parseClaudeVersion) === null) { console.error( "skills:eval — no usable `claude` CLI on PATH. This eval needs one.", @@ -463,50 +593,13 @@ async function main() { }), })); - /** 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; - }; - - 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, - ); - - // 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); + 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); } if ( diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 438e43bcb..6a1a5053c 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -17,6 +17,8 @@ import path from "node:path"; import { caseHit, chainHit, + formatReport, + passesThreshold, collectCases, collectSkillInvocations, runRejection, @@ -294,6 +296,115 @@ test("runPrompt keeps the run read-only across every turn", () => { } }); +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(() => {}); + assert.deepEqual(args[args.indexOf("--allowedTools") + 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("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 }) }), From 3ec125725a13b23c6b53ec44cb59e86a40732b3c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 20:03:11 -0400 Subject: [PATCH 3/5] feat(skills): address Copilot review round 2 on #2204 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both correct, and the first one means round 1's containment fix was only half a fix. **`--allowedTools` is not the availability filter.** It grants permission; it does not restrict which tools exist. So a tool a user's or a plugin's settings already permit stayed reachable across all 14 turns, and the bound still rested on the deny list being complete — the exact property round 1 set out to stop depending on. `--tools` is the real restriction (it selects from the built-in set), and both flags now carry the same four names: `--tools` to bound, `--allowedTools` so those four never need a permission prompt no headless run can answer. Verified against the real CLI — the full flag set runs to a clean `end_turn`. The test asserts both flags, and both probe snippets in `docs/skill-authoring.md` carry `--tools` with a ⚠️ saying which one does the bounding. **The threshold guard let `NaN` and negatives through.** `CHAIN_THRESHOLD=abc` is `NaN`, which fails every comparison and prints an `above NaN%` summary; a negative bar passes every chain unconditionally. Both turn an advertised env knob into a measurement that quietly means nothing. `CHAIN_THRESHOLD` must now be in `[0, 1)` — half-open because the chain bar is strict — and `THRESHOLD` in `[0, 1]`, inclusive because a first-move floor of 1.0 is meetable. The message names the offending value. A new case spawns the script for each bad value rather than asserting on the predicate, since these guards live in `main`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe Signed-off-by: cliffhall --- docs/skill-authoring.md | 9 +++++-- scripts/skill-eval.mjs | 36 ++++++++++++++++++++++++---- scripts/skill-eval.test.mjs | 47 ++++++++++++++++++++++++++++++++----- 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index fef908926..8bf29e76a 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -307,13 +307,18 @@ 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 \ - --allowedTools Read,Glob,Grep,Skill \ + --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 @@ -324,7 +329,7 @@ predicts nothing, which is the whole reason the two are meant to match ```sh printf '%s' "" \ | claude -p --output-format stream-json --verbose --max-turns 14 \ - --allowedTools Read,Glob,Grep,Skill \ + --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[]? diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index ac61887d7..a043f2ac2 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -336,12 +336,20 @@ export function caseHit(c, invoked, ours) { /** * The only tools an eval run needs: read the repo, and load a skill. * - * Enumerating what is ALLOWED rather than only what is denied is the load- + * 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"]; @@ -423,6 +431,8 @@ export function runPrompt( // 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", @@ -562,11 +572,27 @@ export function formatReport(cases, results, ours, opts) { } async function main() { - if (CHAIN_THRESHOLD >= 1) { - // 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. + // 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 — CHAIN_THRESHOLD must be below 1 (got ${CHAIN_THRESHOLD}); it is a strict lower bound.`, + `skills:eval — THRESHOLD must be a number in [0, 1] (got ${process.env.THRESHOLD ?? THRESHOLD}).`, ); process.exit(1); } diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 6a1a5053c..491a70983 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -11,6 +11,8 @@ 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"; @@ -31,6 +33,9 @@ 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 })); +/** 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) => ({ @@ -312,17 +317,47 @@ test("runPrompt bounds the run by what it needs, not only by what it forbids", ( return c; }, }).catch(() => {}); - assert.deepEqual(args[args.indexOf("--allowedTools") + 1].split(","), [ - "Read", - "Glob", - "Grep", - "Skill", - ]); + // `--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. From 313a580982f7fa3fe9123eced8d377511e75c041 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 20:28:37 -0400 Subject: [PATCH 4/5] feat(skills): address Copilot review round 3 on #2204 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One finding, and a sharp one: position in the stream is not causation. `collectSkillInvocations` flattened every `tool_use` block across every assistant event into one ordered array, and `chainHit` read position alone. But the model can emit several `Skill` blocks in a SINGLE assistant message, and it has not seen the first skill's body when it does — those are parallel guesses, not a hand-off. A run that emitted `testing` and `test-servers` together in one message scored as `A` led to `B`, and would have gone on scoring after the pointer in `testing`'s body was deleted: exactly the false pass this feature exists to rule out, arriving by a second route (Copilot). Each recorded invocation now carries the assistant event it came from — `{payload, turn}` — and `chainHit` requires every link after the first to land in a STRICTLY later turn. The turn count is threaded through `collectSkillInvocations`'s `turnOffset`/`nextTurn` so a stream read in chunks stays monotonic rather than restarting per read; without that, a chunk boundary would reset the numbering and reintroduce the same bug on long runs. The scan stays greedy, which is still correct under the constraint: taking the earliest occurrence of a link can only leave more room for the rest, so no later starting point succeeds where the greedy one fails. `sampleHit` is deliberately unaffected — a first-move case asks only whether a skill fired at all, so it reads names and ignores turns. Four new assertions in one case: a same-turn pair scores false; the same two skills a turn apart score true; a same-turn pair does not poison a later genuine hand-off; and only the chain's first link is unconstrained. Plus a `collectSkillInvocations` case pinning the turn tagging and the cross-chunk offset. `docs/skill-authoring.md` now lists this as the second way a chained case can false-pass, alongside a prompt carrying the target's own trigger. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe Signed-off-by: cliffhall --- docs/skill-authoring.md | 10 +++- scripts/skill-eval.mjs | 78 +++++++++++++++++++++++----- scripts/skill-eval.test.mjs | 100 +++++++++++++++++++++++++++++++++--- 3 files changed, 168 insertions(+), 20 deletions(-) diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 8bf29e76a..0bab8872b 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -216,7 +216,7 @@ 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: +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 @@ -232,6 +232,14 @@ Five rules the shape enforces, each for a reason worth knowing: 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. diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index a043f2ac2..748cdaba3 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -187,15 +187,27 @@ export function collectCases(only, skillsDir = SKILLS_DIR) { * 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: string[], 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 = []; let result = null; + let turn = turnOffset; for (const line of lines) { if (!line.trim()) continue; let evt; @@ -207,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.push(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); } /** @@ -281,12 +306,15 @@ 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 {Iterable} 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)); } @@ -302,19 +330,35 @@ export function sampleHit(expect, invoked, ours = null) { * 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} invoked + * @param {Iterable<{payload: string, turn: number}>} 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++; + 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; @@ -402,7 +446,8 @@ const DISALLOWED_TOOLS = [ * * @param {string} prompt * @param {{ spawnFn?: typeof spawn, cwd?: string, maxTurns?: number }} [opts] - * @returns {Promise} Skill payloads, in the order they fired. + * @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, @@ -447,10 +492,17 @@ export function runPrompt( let buf = ""; 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.push(payload); + turnOffset = parsed.nextTurn; + for (const entry of parsed.invoked) invoked.push(entry); if (parsed.result !== null) result = parsed.result; }); p.on("error", reject); diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 491a70983..97d4b5b5a 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -30,8 +30,16 @@ 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 })); +/** + * 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)); @@ -49,7 +57,8 @@ test("collectSkillInvocations finds Skill tool_use payloads", () => { assistant(skillUse("testing")) + "\n", ); assert.equal(invoked.length, 1); - assert.ok(invoked[0].includes("testing")); + assert.ok(invoked[0].payload.includes("testing")); + assert.equal(invoked[0].turn, 1); }); test("collectSkillInvocations ignores other tools and other event types", () => { @@ -79,7 +88,9 @@ test("collectSkillInvocations holds back a trailing partial line", () => { 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", () => { @@ -104,8 +115,85 @@ test("collectSkillInvocations preserves order and repeats", () => { ].join("\n") + "\n"; const { invoked } = collectSkillInvocations(text); assert.deepEqual( - invoked.map((p) => JSON.parse(p).skill), - ["test-servers", "testing", "test-servers"], + 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, ); }); From dfbd45993fae9eaa946306f699a06dadcf0bd92a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 20:33:34 -0400 Subject: [PATCH 5/5] docs(skills): record the hand-off measurement honestly after the turn rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-measured the two committed chain cases under the stricter turn-boundary scoring from 313a5809: 100% and 33% at RUNS=3, against 33% and 33% before. That rise is NOT an effect of the change and the doc now says so. The turn rule only ever rejects matches a flatter reading accepted, so it cannot raise a score — the two runs simply straddle a 67-point swing on the same prompt at RUNS=3, where one sample is worth 33 points. Reading it as an improvement would be exactly the mistake the eval exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe Signed-off-by: cliffhall --- docs/skill-authoring.md | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 0bab8872b..6e5158332 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -289,12 +289,21 @@ 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. -**Both committed cases currently sit at 33% and are therefore red, and that 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 about a third of the time. -Strengthening it is its own change against its own issue; lowering the bar to -turn the column green would throw away the only signal this feature adds. +**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 @@ -363,7 +372,7 @@ The summary is two lines, never one: ``` 7/7 first-move cases at or above 80%. -0/2 hand-off cases above 50%. +1/2 hand-off cases above 50%. ``` Narrowing the run never narrows what a **negative** case is scored against — a