chore: address Copilot review round 3
- CI now installs a pinned `claude` CLI and runs `claude plugin validate` as its own step. The guard skips the authoritative validator when the CLI is absent, which without this meant "always, in CI" — so the acceptance criterion was aspirational. Verified it needs no authentication (clean HOME). - skill-eval: classify a run from its terminal `result` event, not its exit code. With `--max-turns 1` a run in which a skill FIRES necessarily hits the limit and the CLI exits 1 (`error_max_turns`, `num_turns: 2`), so last round's blanket nonzero rejection threw away exactly the observations the eval counts. A run with no terminal `result`, or an unusable subtype, still rejects. - skill-eval: fail collection on a skill that does not parse, a model-invoked skill with no eval file, unreadable JSON, or invalid cases. Skipping them let `skills:eval` exit 0 while silently omitting the skill just broken. - verify-skills: `main()` takes an optional skills directory, and `verify-skills.main.test.mjs` drives it over fixtures — missing SKILL.md, malformed frontmatter, undeclared invocation mode, missing/invalid evals, budget overflow (and that name-only skills do not count toward it), and multi-offender reporting. Matches the sibling guards' *.main.test.mjs precedent; without it the gate could stop enforcing while the parser tests stayed green. - issue-create: the note claimed an issue arrives unlabeled, contradicting .github/ISSUE_TEMPLATE — blank issues are disabled, the bug form applies `bug`, the feature form applies `enhancement` + `v2`. Now a table of what each form sets and what triage still has to add. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
ca6828916f
commit
7e0bb8b58d
4 changed files with 357 additions and 32 deletions
|
|
@ -19,10 +19,10 @@
|
|||
// that people stop trusting the gate. Run it when adding a skill or editing a
|
||||
// model-invoked skill's description.
|
||||
//
|
||||
// The cost of that split, stated plainly: `verify:skills` cannot detect a
|
||||
// well-formed skill whose description simply never matches anything. Nothing in
|
||||
// the gate can. Closing that would need a deterministic trigger oracle, which
|
||||
// does not exist.
|
||||
// The cost of that split, stated plainly: neither `verify:skills` nor the
|
||||
// authoritative `claude plugin validate` CI step can detect a well-formed skill
|
||||
// whose description simply never matches anything. Both check structure.
|
||||
// Closing that would need a deterministic trigger oracle, which does not exist.
|
||||
//
|
||||
// Usage:
|
||||
// npm run skills:eval # every model-invoked skill's cases
|
||||
|
|
@ -33,7 +33,7 @@ import { spawn, spawnSync } from "node:child_process";
|
|||
import { readFileSync, existsSync, readdirSync, statSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { parseSkill } from "./lib/skill-manifest.mjs";
|
||||
import { parseSkill, validateEvalCases } from "./lib/skill-manifest.mjs";
|
||||
|
||||
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
|
||||
const SKILLS_DIR = path.join(ROOT, ".claude", "skills");
|
||||
|
|
@ -54,11 +54,35 @@ function collectCases(only) {
|
|||
)
|
||||
continue;
|
||||
const skill = parseSkill(dir, readFileSync(skillFile, "utf8"));
|
||||
if (skill.errors.length > 0 || !skill.modelInvoked) continue;
|
||||
// Skipping a BROKEN skill here would let `skills:eval` run the remaining
|
||||
// cases and exit 0 while omitting the very skill that was just broken —
|
||||
// reporting a green measurement of a set that quietly shrank (Copilot).
|
||||
// Only a well-formed, deliberately name-only skill is skipped.
|
||||
if (skill.errors.length > 0) {
|
||||
throw new Error(
|
||||
`${dir}/SKILL.md does not parse (${skill.errors[0]}). Run \`npm run verify:skills\`.`,
|
||||
);
|
||||
}
|
||||
if (!skill.modelInvoked) continue;
|
||||
const evalsFile = path.join(SKILLS_DIR, dir, "evals", "evals.json");
|
||||
if (!existsSync(evalsFile)) continue;
|
||||
for (const c of JSON.parse(readFileSync(evalsFile, "utf8")))
|
||||
cases.push({ ...c, from: dir });
|
||||
if (!existsSync(evalsFile)) {
|
||||
throw new Error(
|
||||
`${dir} is model-invoked but has no evals/evals.json. Run \`npm run verify:skills\`.`,
|
||||
);
|
||||
}
|
||||
let parsed;
|
||||
try {
|
||||
parsed = JSON.parse(readFileSync(evalsFile, "utf8"));
|
||||
} catch (e) {
|
||||
throw new Error(
|
||||
`${dir}/evals/evals.json is not valid JSON — ${e.message}`,
|
||||
);
|
||||
}
|
||||
const invalid = validateEvalCases(dir, parsed);
|
||||
if (invalid.length > 0) {
|
||||
throw new Error(`${dir}/evals/evals.json: ${invalid.join("; ")}`);
|
||||
}
|
||||
for (const c of parsed) cases.push({ ...c, from: dir });
|
||||
}
|
||||
return cases;
|
||||
}
|
||||
|
|
@ -74,12 +98,13 @@ function collectCases(only) {
|
|||
*
|
||||
* @param {string} text One or more newline-delimited JSON events. A trailing
|
||||
* partial line is ignored, so this can be fed incrementally.
|
||||
* @returns {{ invoked: Set<string>, rest: string }}
|
||||
* @returns {{ invoked: Set<string>, rest: string, result: string | null }}
|
||||
*/
|
||||
export function collectSkillInvocations(text) {
|
||||
const lines = text.split("\n");
|
||||
const rest = lines.pop() ?? "";
|
||||
const invoked = new Set();
|
||||
let result = null;
|
||||
for (const line of lines) {
|
||||
if (!line.trim()) continue;
|
||||
let evt;
|
||||
|
|
@ -89,6 +114,7 @@ export function collectSkillInvocations(text) {
|
|||
// A malformed line is noise from the CLI, not an observation.
|
||||
continue;
|
||||
}
|
||||
if (evt?.type === "result") result = evt.subtype ?? null;
|
||||
if (evt?.type !== "assistant") continue;
|
||||
for (const block of evt.message?.content ?? []) {
|
||||
if (block?.type !== "tool_use" || block.name !== "Skill") continue;
|
||||
|
|
@ -96,7 +122,40 @@ export function collectSkillInvocations(text) {
|
|||
invoked.add(JSON.stringify(block.input ?? {}));
|
||||
}
|
||||
}
|
||||
return { invoked, rest };
|
||||
return { invoked, rest, result };
|
||||
}
|
||||
|
||||
/**
|
||||
* Terminal `result` subtypes that mean the session ran to a real conclusion.
|
||||
*
|
||||
* `error_max_turns` is a SUCCESSFUL observation here, not a failure: with
|
||||
* `--max-turns 1`, a run in which the model invokes a skill necessarily hits
|
||||
* the limit and the CLI exits 1 — so treating a nonzero exit as failure would
|
||||
* reject exactly the runs the eval is trying to count. Verified against the
|
||||
* CLI: a firing prompt ends `{subtype: "error_max_turns", num_turns: 2}`.
|
||||
*/
|
||||
const CONCLUSIVE_RESULTS = new Set(["success", "error_max_turns"]);
|
||||
|
||||
/**
|
||||
* Whether a finished run produced a usable observation.
|
||||
*
|
||||
* The failure that matters is the opposite one: an auth error, a rate limit or
|
||||
* a missing CLI observes NOTHING, and counting that as "no skill invoked"
|
||||
* passes every negative case and reads as a trigger miss on every positive one,
|
||||
* so a run that never happened comes back as a plausible hit rate (Copilot).
|
||||
* Classifying on the terminal event rather than the exit code separates the two.
|
||||
*
|
||||
* @param {{ result: string | null, code: number | null }} outcome
|
||||
* @returns {string | null} A reason to reject, or null if the run is usable.
|
||||
*/
|
||||
export function runRejection({ result, code }) {
|
||||
if (result === null) {
|
||||
return `produced no terminal \`result\` event (exit ${code})`;
|
||||
}
|
||||
if (!CONCLUSIVE_RESULTS.has(result)) {
|
||||
return `ended \`${result}\` (exit ${code})`;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -170,19 +229,18 @@ export function runPrompt(prompt, { spawnFn = spawn, cwd = ROOT } = {}) {
|
|||
|
||||
let buf = "";
|
||||
const invoked = new Set();
|
||||
let result = null;
|
||||
p.stdout.on("data", (chunk) => {
|
||||
const parsed = collectSkillInvocations(buf + chunk.toString());
|
||||
buf = parsed.rest;
|
||||
for (const payload of parsed.invoked) invoked.add(payload);
|
||||
if (parsed.result !== null) result = parsed.result;
|
||||
});
|
||||
p.on("error", reject);
|
||||
p.on("close", (code) => {
|
||||
// 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.
|
||||
if (code !== 0) {
|
||||
reject(new Error(`\`claude -p\` exited ${code} for prompt: ${prompt}`));
|
||||
const rejection = runRejection({ result, code });
|
||||
if (rejection !== null) {
|
||||
reject(new Error(`\`claude -p\` ${rejection} for prompt: ${prompt}`));
|
||||
return;
|
||||
}
|
||||
resolve(invoked);
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ import assert from "node:assert/strict";
|
|||
import { EventEmitter } from "node:events";
|
||||
import {
|
||||
collectSkillInvocations,
|
||||
runRejection,
|
||||
invokedSkillNames,
|
||||
runPrompt,
|
||||
sampleHit,
|
||||
|
|
@ -116,6 +117,38 @@ function fakeSpawn({ chunks = [], code = 0, error = null }) {
|
|||
};
|
||||
}
|
||||
|
||||
const resultEvent = (subtype) =>
|
||||
JSON.stringify({ type: "result", subtype }) + "\n";
|
||||
|
||||
test("collectSkillInvocations reports the terminal result subtype", () => {
|
||||
assert.equal(
|
||||
collectSkillInvocations(resultEvent("success")).result,
|
||||
"success",
|
||||
);
|
||||
assert.equal(collectSkillInvocations("").result, null);
|
||||
});
|
||||
|
||||
test("runRejection accepts a run that hit the turn limit", () => {
|
||||
// With `--max-turns 1`, a run in which a skill FIRES necessarily hits the
|
||||
// limit and the CLI exits 1. Rejecting on the exit code alone would throw away
|
||||
// exactly the observations the eval exists to count — verified against the
|
||||
// real CLI, which ends such a run `error_max_turns` with `num_turns: 2`.
|
||||
assert.equal(runRejection({ result: "error_max_turns", code: 1 }), null);
|
||||
assert.equal(runRejection({ result: "success", code: 0 }), null);
|
||||
});
|
||||
|
||||
test("runRejection rejects a run that observed nothing", () => {
|
||||
// An auth failure, a rate limit, or a CLI that never started. Counting these
|
||||
// as "no skill invoked" passes every negative case and reads as a trigger
|
||||
// miss on every positive one.
|
||||
assert.match(runRejection({ result: null, code: 1 }) ?? "", /no terminal/);
|
||||
assert.match(runRejection({ result: null, code: 0 }) ?? "", /no terminal/);
|
||||
assert.match(
|
||||
runRejection({ result: "error_during_execution", code: 1 }) ?? "",
|
||||
/ended `error_during_execution`/,
|
||||
);
|
||||
});
|
||||
|
||||
test("runPrompt collects invocations across chunk boundaries", async () => {
|
||||
const whole =
|
||||
JSON.stringify({
|
||||
|
|
@ -127,18 +160,34 @@ test("runPrompt collects invocations across chunk boundaries", async () => {
|
|||
},
|
||||
}) + "\n";
|
||||
const invoked = await runPrompt("p", {
|
||||
spawnFn: fakeSpawn({ chunks: [whole.slice(0, 30), whole.slice(30)] }),
|
||||
spawnFn: fakeSpawn({
|
||||
chunks: [
|
||||
whole.slice(0, 30),
|
||||
whole.slice(30),
|
||||
resultEvent("error_max_turns"),
|
||||
],
|
||||
code: 1,
|
||||
}),
|
||||
});
|
||||
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.
|
||||
test("runPrompt rejects a run that produced no terminal result", async () => {
|
||||
await assert.rejects(
|
||||
runPrompt("p", { spawnFn: fakeSpawn({ code: 1 }) }),
|
||||
/exited 1 for prompt: p/,
|
||||
/no terminal `result` event \(exit 1\) for prompt: p/,
|
||||
);
|
||||
});
|
||||
|
||||
test("runPrompt rejects a run that ended in an unusable state", async () => {
|
||||
await assert.rejects(
|
||||
runPrompt("p", {
|
||||
spawnFn: fakeSpawn({
|
||||
chunks: [resultEvent("error_during_execution")],
|
||||
code: 1,
|
||||
}),
|
||||
}),
|
||||
/ended `error_during_execution`/,
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
177
scripts/verify-skills.main.test.mjs
Normal file
177
scripts/verify-skills.main.test.mjs
Normal file
|
|
@ -0,0 +1,177 @@
|
|||
// Fixture-level tests for `verify-skills`'s orchestration (#2163, Copilot).
|
||||
//
|
||||
// `skill-manifest.test.mjs` covers the parsers; this covers `main()` — that the
|
||||
// guard actually *reports* what those parsers find, and exits nonzero when it
|
||||
// does. Without it, the mandatory gate could stop enforcing its checks while
|
||||
// every parser test stayed green, which is the same "a gate that stops gating"
|
||||
// failure the sibling guards' `*.main.test.mjs` suites exist to prevent.
|
||||
//
|
||||
// Each case builds a throwaway skills directory and runs the real script
|
||||
// against it via the directory argument. Run via `npm run test:scripts`.
|
||||
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const SCRIPT = path.join(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"verify-skills.mjs",
|
||||
);
|
||||
|
||||
/**
|
||||
* Build a skills directory.
|
||||
*
|
||||
* @param {Record<string, {skill?: string, evals?: string}>} skills
|
||||
*/
|
||||
function fixture(skills) {
|
||||
const root = mkdtempSync(path.join(tmpdir(), "verify-skills-"));
|
||||
for (const [name, { skill, evals }] of Object.entries(skills)) {
|
||||
const dir = path.join(root, name);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
if (skill !== undefined) writeFileSync(path.join(dir, "SKILL.md"), skill);
|
||||
if (evals !== undefined) {
|
||||
mkdirSync(path.join(dir, "evals"), { recursive: true });
|
||||
writeFileSync(path.join(dir, "evals", "evals.json"), evals);
|
||||
}
|
||||
}
|
||||
return root;
|
||||
}
|
||||
|
||||
function run(dir) {
|
||||
const res = spawnSync(process.execPath, [SCRIPT, dir], { encoding: "utf8" });
|
||||
return { code: res.status, out: (res.stdout ?? "") + (res.stderr ?? "") };
|
||||
}
|
||||
|
||||
const byName = (name, extra = "") =>
|
||||
`---\nname: ${name}\ndescription: A description.\ndisable-model-invocation: true\n${extra}---\n\nBody\n`;
|
||||
|
||||
const modelInvoked = (name) =>
|
||||
`---\nname: ${name}\ndescription: A description.\ndisable-model-invocation: false\n---\n\nBody\n`;
|
||||
|
||||
const goodEvals = (name) =>
|
||||
JSON.stringify([
|
||||
{ prompt: "fires", expect: name },
|
||||
{ prompt: "does not", expect: null },
|
||||
]);
|
||||
|
||||
test("passes a well-formed directory", () => {
|
||||
const dir = fixture({
|
||||
alpha: { skill: byName("alpha") },
|
||||
beta: { skill: modelInvoked("beta"), evals: goodEvals("beta") },
|
||||
});
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 0, out);
|
||||
assert.match(out, /2 skills, 1 model-invoked \(beta\)/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails an empty or missing directory", () => {
|
||||
const empty = mkdtempSync(path.join(tmpdir(), "verify-skills-empty-"));
|
||||
assert.match(run(empty).out, /no skills found/);
|
||||
assert.equal(run(empty).code, 1);
|
||||
assert.equal(run(path.join(empty, "nope")).code, 1);
|
||||
rmSync(empty, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails a skill directory with no SKILL.md", () => {
|
||||
const dir = fixture({ alpha: {} });
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /alpha: no SKILL\.md/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails frontmatter that would load with empty metadata", () => {
|
||||
// The failure the guard exists for: `/alpha` still works, so a spot check
|
||||
// passes, while the description the model matches against is gone.
|
||||
const dir = fixture({
|
||||
alpha: {
|
||||
skill:
|
||||
"---\nname: alpha\ndescription: Use this: always\ndisable-model-invocation: true\n---\n",
|
||||
},
|
||||
});
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /not valid YAML/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails a skill that does not declare its invocation mode", () => {
|
||||
const dir = fixture({
|
||||
alpha: { skill: "---\nname: alpha\ndescription: d\n---\n" },
|
||||
});
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /must be declared explicitly/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails a model-invoked skill with no eval cases", () => {
|
||||
const dir = fixture({ beta: { skill: modelInvoked("beta") } });
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /need committed eval cases/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("fails eval cases that are unreadable, all-positive, or all-negative", () => {
|
||||
for (const [evals, pattern] of [
|
||||
["{ not json", /not valid JSON/],
|
||||
[JSON.stringify([{ prompt: "a", expect: "beta" }]), /no negative case/],
|
||||
[JSON.stringify([{ prompt: "a", expect: null }]), /no positive case/],
|
||||
[JSON.stringify([{ expect: null }]), /prompt/],
|
||||
]) {
|
||||
const dir = fixture({ beta: { skill: modelInvoked("beta"), evals } });
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1, out);
|
||||
assert.match(out, pattern);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test("fails when the skill listing exceeds its budget", () => {
|
||||
// Only model-invoked skills occupy the listing, so the same text behind
|
||||
// `disable-model-invocation: true` must NOT trip it.
|
||||
const long = "d".repeat(1500);
|
||||
const many = {};
|
||||
for (let i = 0; i < 4; i++) {
|
||||
many[`skill-${i}`] = {
|
||||
skill: `---\nname: skill-${i}\ndescription: ${long}\ndisable-model-invocation: false\n---\n`,
|
||||
evals: goodEvals(`skill-${i}`),
|
||||
};
|
||||
}
|
||||
const over = fixture(many);
|
||||
const { code, out } = run(over);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /over the recorded budget/);
|
||||
rmSync(over, { recursive: true, force: true });
|
||||
|
||||
const quiet = {};
|
||||
for (let i = 0; i < 4; i++) {
|
||||
quiet[`skill-${i}`] = {
|
||||
skill: `---\nname: skill-${i}\ndescription: ${long}\ndisable-model-invocation: true\n---\n`,
|
||||
};
|
||||
}
|
||||
const under = fixture(quiet);
|
||||
assert.equal(run(under).code, 0);
|
||||
rmSync(under, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("reports every offender in one pass rather than dying on the first", () => {
|
||||
const dir = fixture({
|
||||
alpha: {},
|
||||
beta: { skill: modelInvoked("beta") },
|
||||
gamma: {
|
||||
skill:
|
||||
"---\nname: wrong-name\ndescription: d\ndisable-model-invocation: true\n---\n",
|
||||
},
|
||||
});
|
||||
const { code, out } = run(dir);
|
||||
assert.equal(code, 1);
|
||||
assert.match(out, /3 problem\(s\)/);
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
|
@ -41,7 +41,7 @@ import {
|
|||
} from "./lib/skill-manifest.mjs";
|
||||
|
||||
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
|
||||
const SKILLS_DIR = path.join(ROOT, ".claude", "skills");
|
||||
const DEFAULT_SKILLS_DIR = path.join(ROOT, ".claude", "skills");
|
||||
|
||||
/** Directory names under `.claude/skills` that contain a SKILL.md. */
|
||||
export function skillDirs(
|
||||
|
|
@ -54,7 +54,12 @@ export function skillDirs(
|
|||
.sort();
|
||||
}
|
||||
|
||||
function main() {
|
||||
function main(argv = process.argv.slice(2)) {
|
||||
// An explicit directory is the seam the fixture tests drive; without it this
|
||||
// guard could only ever be exercised against the repo's own (green) skills,
|
||||
// so it could stop enforcing anything while every unit test stayed green.
|
||||
const override = argv[0];
|
||||
const SKILLS_DIR = override ? path.resolve(override) : DEFAULT_SKILLS_DIR;
|
||||
const failures = [];
|
||||
|
||||
if (!existsSync(SKILLS_DIR)) {
|
||||
|
|
@ -124,6 +129,8 @@ function main() {
|
|||
`(${modelInvoked.join(", ")}); listing ${cost}/${LISTING_BUDGET} chars.`,
|
||||
);
|
||||
|
||||
if (override) return;
|
||||
|
||||
// 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
|
||||
|
|
|
|||
Loading…
Reference in a new issue