chore: address Copilot review round 2

- verify-skills: version-gate the optional `claude plugin validate`. Probing
  only for the executable failed the MANDATORY gate for anyone on a CLI older
  than 2.1.233, which has no such subcommand and exits nonzero on it. Skips
  below the floor; a failure now means a supported validator rejected the skills.
- skill-eval: match the invoked skill structurally. A substring test counted
  `{"skill":"not-testing"}` as a hit for `testing` and inflated the rate.
- skill-eval: `runOnce` → exported `runPrompt` with an injectable `spawnFn`, so
  the nonzero-exit branch is actually pinned. Reverting it now fails a test
  (verified by mutation) rather than leaving the suite green.
- board-ops: filter the item lookups by repository. Issue numbers are repo-local
  and these are org boards — #11 really does carry a `servers` card — so an
  unfiltered `select` can move or delete the wrong card, or pass two ids.
- issue-triage: the audit's `own()` filter dropped draft cards, which carry no
  `.content.repository`, so the "non-Issue on a board" check reported 0 while
  the invariant was being violated. Verified against a fixture.
- pr-flow: cut the branch from the base it will target. The blanket "cut from
  v2/main" was wrong for v1, whose history is unrelated.

Signed-off-by: cliffhall <cliff@futurescale.com>

.claude/skills/board-ops/SKILL.md

@@ -128,9 +128,16 @@ above the board's item count (~265 as of 2026-08-01) — past it `item-list`
 truncates **silently**, `select` matches nothing, and `item-edit --id ""` fails
 with an opaque node-resolution error rather than saying the limit was too low.
 
+⚠️ **Filter by repository, not by number alone.** These are **org** projects and
+issue numbers are **repo-local**, so an unfiltered `select` can match another
+repo's issue that happens to share the number — board #11 really does carry a
+`modelcontextprotocol/servers` card — and then moves or deletes the wrong card,
+or passes two ids at once (Copilot).
+
 ```sh
 ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \
-  --jq '.items[] | select(.content.number==<ISSUE_NUMBER>) | .id')
+  --jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector"
+                          and .content.number==<ISSUE_NUMBER>) | .id')
 # e.g. Status → In Review, when its PR opens
 gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" \
   --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id 159c8a02
@@ -144,7 +151,8 @@ not parked in Done:
 
 ```sh
 ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \
-  --jq '.items[] | select(.content.number==<ISSUE_NUMBER>) | .id')
+  --jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector"
+                          and .content.number==<ISSUE_NUMBER>) | .id')
 gh project item-delete 28 --owner modelcontextprotocol --id "$ITEM_ID"
 ```
 

.claude/skills/issue-triage/SKILL.md

@@ -219,7 +219,10 @@ for P in 28 11; do gh project item-list $P --owner modelcontextprotocol \
 jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b11.json" --arg R "$R" '
   ($o[0] | map({key:(.number|tostring), value:{st:.state, sr:(.stateReason // ""),
                 lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $M
-  | def own($s): [$s[].items[] | select(.content.repository==$R)];
+  | def own($s): [$s[].items[]
+        # A DRAFT card has no `.content.repository`, so filtering on equality
+        # alone drops the very items the "non-Issue" check exists to find.
+        | select((.content.repository // null) == null or .content.repository==$R)];
     def I($n): ($M[($n|tostring)] // null);
     def ms($n): (I($n).ms // null);
     def lab($n): (I($n).lab // []);
@@ -268,6 +271,11 @@ Two things the queries must account for, both learned the hard way:
   one*, so a predicate that only asks "is any version label present" passes an
   issue carrying **both** `v1` and `v2` — which belongs to two lines at once
   and shows up in both filtered queries. Same for the five type labels (Copilot).
+- **Keep drafts inside `own`.** A draft card carries no `.content.repository`,
+  so a plain equality filter removes it before the "non-Issue" check can see it —
+  and that check then reports `0` while the invariant it states (no drafts) is
+  being violated (Copilot). The filter admits an item with no repository and
+  excludes only cards that name a *different* one.
 - **Only open issues have a `$I` entry.** A closed issue still has a card
   (correctly, in `Done`), so a check that treats "no milestone found" as a
   violation must gate on `isopen(.n)` or it flags every closed card.

.claude/skills/pr-flow/SKILL.md

@@ -38,8 +38,11 @@ Not `fix/oauth-resource-metadata`. This keeps the two lines legible in
 `git branch -a` and in the PR list once v1 and v2 branches coexist on the same
 remote, and it matches the base branches themselves (`v2/main`, `v1/main`).
 
-**Cut the branch from `v2/main`**, never from a milestone-merge branch — the
-latter carries release-only commits that will show up in your PR's diff.
+**Cut the branch from the base it will target** — `v2/main` for v2 work,
+`v1/main` for v1. The two lines have unrelated histories, so a `v1/fix/…`
+branch cut from `v2/main` arrives at `v1/main` carrying the whole v2 tree
+(Copilot). And never cut from a milestone-merge branch — it carries release-only
+commits that will show up in your PR's diff.
 
 Working in a git worktree is fine and often preferable. ⚠️ **A worktree needs a
 real `npm install`, not a symlinked `node_modules`** — a symlink passes

scripts/lib/skill-manifest.mjs

@@ -206,3 +206,30 @@ export function validateEvalCases(skillName, cases) {
   }
   return errors;
 }
+
+/** First Claude Code release with `claude plugin validate`. */
+export const PLUGIN_VALIDATE_MIN_VERSION = [2, 1, 233];
+
+/**
+ * Read a semver triple out of `claude --version` output ("2.1.250 (Claude Code)").
+ *
+ * @param {string} text
+ * @returns {number[] | null}
+ */
+export function parseClaudeVersion(text) {
+  const m = /(\d+)\.(\d+)\.(\d+)/.exec(text);
+  return m === null ? null : [Number(m[1]), Number(m[2]), Number(m[3])];
+}
+
+/**
+ * @param {number[]} a
+ * @param {number[]} b
+ * @returns {number} negative if a < b, 0 if equal, positive if a > b.
+ */
+export function compareVersions(a, b) {
+  for (let i = 0; i < Math.max(a.length, b.length); i++) {
+    const diff = (a[i] ?? 0) - (b[i] ?? 0);
+    if (diff !== 0) return diff;
+  }
+  return 0;
+}

scripts/lib/skill-manifest.test.mjs

@@ -11,6 +11,9 @@ import {
   parseSkill,
   validateEvalCases,
   listingCost,
+  parseClaudeVersion,
+  compareVersions,
+  PLUGIN_VALIDATE_MIN_VERSION,
 } from "./skill-manifest.mjs";
 
 const fm = (body) => `---\n${body}\n---\n\n# Body\n`;
@@ -198,3 +201,21 @@ test("validateEvalCases rejects malformed cases", () => {
     /expect. must be a skill name or null/,
   );
 });
+
+test("parseClaudeVersion reads the CLI's version banner", () => {
+  assert.deepEqual(parseClaudeVersion("2.1.250 (Claude Code)"), [2, 1, 250]);
+  assert.deepEqual(parseClaudeVersion("claude 10.0.4\n"), [10, 0, 4]);
+  assert.equal(parseClaudeVersion("no version here"), null);
+});
+
+test("compareVersions orders the plugin-validate floor correctly", () => {
+  // An older CLI has no `plugin validate` and exits nonzero on it as an unknown
+  // command; failing the mandatory gate for that would be wrong, so the probe
+  // must skip below the floor rather than run and fail.
+  assert.ok(compareVersions([2, 1, 232], PLUGIN_VALIDATE_MIN_VERSION) < 0);
+  assert.equal(compareVersions([2, 1, 233], PLUGIN_VALIDATE_MIN_VERSION), 0);
+  assert.ok(compareVersions([2, 1, 250], PLUGIN_VALIDATE_MIN_VERSION) > 0);
+  assert.ok(compareVersions([2, 2, 0], PLUGIN_VALIDATE_MIN_VERSION) > 0);
+  assert.ok(compareVersions([3], PLUGIN_VALIDATE_MIN_VERSION) > 0);
+  assert.ok(compareVersions([1, 9, 999], PLUGIN_VALIDATE_MIN_VERSION) < 0);
+});

scripts/skill-eval.mjs

@@ -99,6 +99,29 @@ export function collectSkillInvocations(text) {
   return { invoked, rest };
 }
 
+/**
+ * The skill names a `Skill` tool_use payload actually asked for.
+ *
+ * Structural rather than substring: matching `"testing"` against the raw JSON
+ * counts `{"skill":"not-testing"}` as a hit and inflates the measured rate
+ * (Copilot). The field name is still not assumed — every string value in the
+ * payload is a candidate, compared by equality.
+ *
+ * @param {string} payload JSON text as recorded by `collectSkillInvocations`.
+ * @returns {string[]}
+ */
+export function invokedSkillNames(payload) {
+  let parsed;
+  try {
+    parsed = JSON.parse(payload);
+  } catch {
+    return [];
+  }
+  if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed))
+    return [];
+  return Object.values(parsed).filter((v) => typeof v === "string");
+}
+
 /**
  * Whether one sample satisfies a case.
  *
@@ -107,13 +130,27 @@ export function collectSkillInvocations(text) {
  */
 export function sampleHit(expect, invoked) {
   if (expect === null) return invoked.size === 0;
-  return [...invoked].some((payload) => payload.includes(expect));
+  return [...invoked].some((payload) =>
+    invokedSkillNames(payload).includes(expect),
+  );
 }
 
-/** Returns the payloads the Skill tool was called with in one fresh session. */
-function runOnce(prompt) {
+/**
+ * Drive one fresh session and return the payloads the `Skill` tool was called
+ * with.
+ *
+ * `spawnFn` is injectable so the stream handling and — more importantly — the
+ * exit handling are testable. Without a seam here, changing the nonzero branch
+ * back to resolving an empty set would leave every test green while the eval
+ * silently reported a plausible hit rate for runs that never happened (Copilot).
+ *
+ * @param {string} prompt
+ * @param {{ spawnFn?: typeof spawn, cwd?: string }} [opts]
+ * @returns {Promise<Set<string>>}
+ */
+export function runPrompt(prompt, { spawnFn = spawn, cwd = ROOT } = {}) {
   return new Promise((resolve, reject) => {
-    const p = spawn(
+    const p = spawnFn(
       "claude",
       [
         "-p",
@@ -128,7 +165,7 @@ function runOnce(prompt) {
         "--disallowedTools",
         "Bash,Write,Edit,NotebookEdit",
       ],
-      { cwd: ROOT, stdio: ["ignore", "pipe", "inherit"] },
+      { cwd, stdio: ["ignore", "pipe", "inherit"] },
     );
 
     let buf = "";
@@ -143,7 +180,7 @@ function runOnce(prompt) {
       // A CLI that failed to run observed NOTHING. Resolving it as an empty set
       // would silently pass every negative case and read as a trigger miss on
       // every positive one, so an auth error or a rate limit would come back as
-      // a plausible-looking hit rate (Copilot).
+      // a plausible-looking hit rate.
       if (code !== 0) {
         reject(new Error(`\`claude -p\` exited ${code} for prompt: ${prompt}`));
         return;
@@ -187,7 +224,7 @@ async function main() {
   const jobs = cases.flatMap((c) => Array.from({ length: RUNS }, () => c));
   const results = await pool(jobs, CONCURRENCY, async (c) => ({
     c,
-    invoked: await runOnce(c.prompt),
+    invoked: await runPrompt(c.prompt),
   }));
 
   let failed = 0;

scripts/skill-eval.test.mjs

@@ -10,7 +10,13 @@
 
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { collectSkillInvocations, sampleHit } from "./skill-eval.mjs";
+import { EventEmitter } from "node:events";
+import {
+  collectSkillInvocations,
+  invokedSkillNames,
+  runPrompt,
+  sampleHit,
+} from "./skill-eval.mjs";
 
 const assistant = (...blocks) =>
   JSON.stringify({ type: "assistant", message: { content: blocks } });
@@ -67,3 +73,78 @@ test("sampleHit scores positive and negative cases", () => {
   assert.equal(sampleHit(null, fired), false);
   assert.equal(sampleHit("testing", none), false);
 });
+
+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
+  // different skill.
+  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);
+  // The field name is not assumed, so any string value is a candidate.
+  assert.equal(
+    sampleHit("testing", new Set(['{"name":"testing","args":""}'])),
+    true,
+  );
+});
+
+test("invokedSkillNames tolerates payloads it cannot read", () => {
+  for (const payload of ["not json", "[1,2]", "null", '"testing"']) {
+    assert.deepEqual(invokedSkillNames(payload), []);
+  }
+});
+
+/** Minimal stand-in for a ChildProcess: emits stdout chunks, then closes. */
+function fakeSpawn({ chunks = [], code = 0, error = null }) {
+  return () => {
+    const child = new EventEmitter();
+    child.stdout = new EventEmitter();
+    queueMicrotask(() => {
+      if (error) {
+        child.emit("error", error);
+        return;
+      }
+      for (const c of chunks) child.stdout.emit("data", Buffer.from(c));
+      child.emit("close", code);
+    });
+    return child;
+  };
+}
+
+test("runPrompt collects invocations across chunk boundaries", async () => {
+  const whole =
+    JSON.stringify({
+      type: "assistant",
+      message: {
+        content: [
+          { type: "tool_use", name: "Skill", input: { skill: "testing" } },
+        ],
+      },
+    }) + "\n";
+  const invoked = await runPrompt("p", {
+    spawnFn: fakeSpawn({ chunks: [whole.slice(0, 30), whole.slice(30)] }),
+  });
+  assert.equal(sampleHit("testing", invoked), true);
+});
+
+test("runPrompt rejects a nonzero exit rather than reporting an empty observation", async () => {
+  // The regression that matters: resolving here would pass every negative case
+  // and read as a trigger miss on every positive one, so a rate-limited run
+  // would report a plausible hit rate.
+  await assert.rejects(
+    runPrompt("p", { spawnFn: fakeSpawn({ code: 1 }) }),
+    /exited 1 for prompt: p/,
+  );
+});
+
+test("runPrompt propagates a spawn error", async () => {
+  await assert.rejects(
+    runPrompt("p", { spawnFn: fakeSpawn({ error: new Error("ENOENT") }) }),
+    /ENOENT/,
+  );
+});

scripts/verify-skills.mjs

@@ -31,6 +31,9 @@ import { spawnSync } from "node:child_process";
 import path from "node:path";
 import { fileURLToPath } from "node:url";
 import {
+  compareVersions,
+  parseClaudeVersion,
+  PLUGIN_VALIDATE_MIN_VERSION,
   parseSkill,
   validateEvalCases,
   listingCost,
@@ -121,11 +124,27 @@ function main() {
       `(${modelInvoked.join(", ")}); listing ${cost}/${LISTING_BUDGET} chars.`,
   );
 
-  // The authoritative parse, when it is available.
-  const probe = spawnSync("claude", ["--version"], { stdio: "ignore" });
-  if (probe.error) {
+  // The authoritative parse, when it is available — strictly a bonus. An older
+  // CLI does not have `plugin validate` and exits nonzero on it as an unknown
+  // command, which would fail the MANDATORY gate for a contributor whose only
+  // sin is not having upgraded (Copilot). So gate on the version that
+  // introduced it and skip below it, reserving a failure for a validator that
+  // actually rejected the skills.
+  const probe = spawnSync("claude", ["--version"], { encoding: "utf8" });
+  if (probe.error || probe.status !== 0) {
     console.log(
-      "verify:skills — `claude` CLI not on PATH; skipped `claude plugin validate`.",
+      "verify:skills — `claude` CLI not usable here; skipped `claude plugin validate`.",
+    );
+    return;
+  }
+  const version = parseClaudeVersion(probe.stdout ?? "");
+  if (
+    version === null ||
+    compareVersions(version, PLUGIN_VALIDATE_MIN_VERSION) < 0
+  ) {
+    console.log(
+      `verify:skills — \`claude\` ${version?.join(".") ?? "(unreadable version)"} predates ` +
+        `${PLUGIN_VALIDATE_MIN_VERSION.join(".")}; skipped \`claude plugin validate\`.`,
     );
     return;
   }