feat(skills): address Copilot review round 2 on #2204

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ABdTnqyBodw6mHVVkGehqe
Signed-off-by: cliffhall <cliff@futurescale.com>

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' "<prompt>" \
   | 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' "<prompt>" \
   | 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[]?

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);
   }

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.