From f20bbac474895f45451e9f130cf5e6f96969aa5f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 19:33:51 -0400 Subject: [PATCH] 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 89502105..28fecc3b 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 f65a3461..fef90892 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 e759961d..ac61887d 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 438e43bc..6a1a5053 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 }) }),