From ca6828916f797029afe7db6b7a6d399859ff8f53 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 01:07:41 -0400 Subject: [PATCH] chore: address Copilot review round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .claude/skills/board-ops/SKILL.md | 12 +++- .claude/skills/issue-triage/SKILL.md | 10 +++- .claude/skills/pr-flow/SKILL.md | 7 ++- scripts/lib/skill-manifest.mjs | 27 +++++++++ scripts/lib/skill-manifest.test.mjs | 21 +++++++ scripts/skill-eval.mjs | 51 ++++++++++++++--- scripts/skill-eval.test.mjs | 83 +++++++++++++++++++++++++++- scripts/verify-skills.mjs | 27 +++++++-- 8 files changed, 221 insertions(+), 17 deletions(-) diff --git a/.claude/skills/board-ops/SKILL.md b/.claude/skills/board-ops/SKILL.md index 5fa4114d..903d303d 100644 --- a/.claude/skills/board-ops/SKILL.md +++ b/.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==) | .id') + --jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector" + and .content.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==) | .id') + --jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector" + and .content.number==) | .id') gh project item-delete 28 --owner modelcontextprotocol --id "$ITEM_ID" ``` diff --git a/.claude/skills/issue-triage/SKILL.md b/.claude/skills/issue-triage/SKILL.md index d0f51001..ac3bfac7 100644 --- a/.claude/skills/issue-triage/SKILL.md +++ b/.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. diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 0fa2b439..121a97fe 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.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 diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index 3036e0a9..bcb2052b 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/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; +} diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index cb2b302a..7b6b156c 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/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); +}); diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 971e8587..8306355e 100755 --- a/scripts/skill-eval.mjs +++ b/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>} + */ +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; diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 966b17cf..b11fcca1 100644 --- a/scripts/skill-eval.test.mjs +++ b/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/, + ); +}); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 2389175a..13871ddc 100755 --- a/scripts/verify-skills.mjs +++ b/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; }