diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index fef90892..8bf29e76 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 ac61887d..a043f2ac 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 below 1 (got ${CHAIN_THRESHOLD}); it is a strict lower bound.`, + `skills:eval — CHAIN_THRESHOLD must be a number in [0, 1) (got ${process.env.CHAIN_THRESHOLD ?? CHAIN_THRESHOLD}); it is a strict lower bound.`, + ); + process.exit(1); + } + if (!Number.isFinite(THRESHOLD) || THRESHOLD < 0 || THRESHOLD > 1) { + // The first-move bar is inclusive, so 1.0 is meetable and allowed; the + // non-finite and negative cases fail the same way as above. + console.error( + `skills:eval — THRESHOLD must be a number in [0, 1] (got ${process.env.THRESHOLD ?? THRESHOLD}).`, ); process.exit(1); } diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 6a1a5053..491a7098 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.