From 3ec125725a13b23c6b53ec44cb59e86a40732b3c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 20:03:11 -0400 Subject: [PATCH] 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 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.