diff --git a/.claude/skills/issue-create/SKILL.md b/.claude/skills/issue-create/SKILL.md index b4c7b219..d56ddbd5 100644 --- a/.claude/skills/issue-create/SKILL.md +++ b/.claude/skills/issue-create/SKILL.md @@ -102,8 +102,22 @@ The priority rubric is in `/issue-triage`. ## Note on issues that arrive from elsewhere -An issue opened by hand in the GitHub UI — by an outside reporter *or* by a -maintainer — arrives with no label, no milestone, and no card. That is normal on -arrival, not a defect to fix the moment it lands: it comes into the system -through `/issue-triage` instead, and starts in **Incoming** with no milestone, -because nobody has approved it. +An issue opened through the GitHub UI — by an outside reporter *or* by a +maintainer — arrives with **no milestone and no card**, and that is normal on +arrival rather than a defect to fix the moment it lands: it comes into the +system through `/issue-triage` instead, and starts in **Incoming**, because +nobody has approved it. + +Its **labels are partly set already**, which the triage pass has to account for: +blank issues are disabled, so it came through a form in +[`.github/ISSUE_TEMPLATE/`](../../../.github/ISSUE_TEMPLATE) (Copilot). + +| Form | Applies | Still missing | +| --- | --- | --- | +| Bug report | `bug` | the version label — the form carries a version-line **dropdown** a maintainer reads at triage, because a `labels:` list is static and GitHub cannot map an answer to a label | +| Feature request | `enhancement`, `v2` | nothing — v1 takes security fixes only and cannot receive a feature, so `v2` is correct by construction | + +So a bug report needs its version label applied from the dropdown answer, and a +feature request usually needs no label work at all. ⚠️ GitHub serves the chooser +from the **default branch**, so a form edited on `v2/main` does not take effect +until the next milestone merge. diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ae060bc9..9abb42eb 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -52,6 +52,21 @@ jobs: # jobs, but that's a larger restructure and deliberately out of scope. run: npm run validate + - name: Validate the skills with the authoritative CLI (#2163) + # `npm run validate` above already ran `verify:skills`, whose own parser + # reads each SKILL.md the way Claude Code does. This is the second + # opinion: `claude plugin validate` is the authoritative schema, and the + # guard skips it whenever the CLI is absent — which, without this step, + # would mean always, in CI. Installing it here is what makes the + # acceptance criterion true rather than aspirational. + # + # Pinned rather than @latest: an unpinned validator can start failing a + # PR that changed nothing, which is how a gate loses its credibility. + # It needs no authentication — verified against a clean HOME. + run: | + npm install -g @anthropic-ai/claude-code@2.1.250 + claude plugin validate .claude/skills + - name: Verify the browser-externalized-builtin build gate (#1769) # Runs a real `vite build` with a Node built-in forced into the browser # graph and asserts the build FAILS via the #1769 gate. The unit tests diff --git a/AGENTS.md b/AGENTS.md index 3461d9a2..05eee3c9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,7 +149,12 @@ that from happening: **malformed YAML**, which loads the body with an *empty* description, so `/skill-name` still works and a manual spot check passes while the skill can never auto-fire again. An unquoted colon in a description is enough. It also - runs `claude plugin validate` when that CLI is on PATH. + runs `claude plugin validate` — the authoritative schema — when that CLI is on + PATH **and new enough to have the subcommand** (2.1.233+; below that it exits + nonzero as an unknown command, which must not fail the gate for someone who + simply has not upgraded). Because the guard skips it when absent, CI installs + a pinned CLI and runs it as its own step, so "authoritative validation runs in + CI" is true rather than aspirational. ⚠️ Frontmatter is only read when the opening `---` is the file's **first line** — no BOM, no leading blank line. 2. **Every skill declares its invocation mode explicitly.** diff --git a/docs/quality-gate.md b/docs/quality-gate.md index c4569c63..c9699c40 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -35,7 +35,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run verify:skills` | Guards `.claude/skills` ([#2163](https://github.com/modelcontextprotocol/inspector/issues/2163)). A skill's body loads only when it is invoked, so one that stops being reachable loses behavior **silently** — and the way that happens is frontmatter Claude Code cannot parse, which loads the body with an *empty* description: `/skill-name` still works, so a manual spot check passes, while nothing is left for the model to match against. This parses each `SKILL.md` the same way, requires an explicit invocation mode on every skill and committed eval cases (positives **and** negatives) on every model-invoked one, and fails when the skill listing exceeds its recorded character budget. Also runs `claude plugin validate` when that CLI is on PATH — but does not depend on it, since it is absent in CI. Runs in `validate`. | +| `npm run verify:skills` | Guards `.claude/skills` ([#2163](https://github.com/modelcontextprotocol/inspector/issues/2163)). A skill's body loads only when it is invoked, so one that stops being reachable loses behavior **silently** — and the way that happens is frontmatter Claude Code cannot parse, which loads the body with an *empty* description: `/skill-name` still works, so a manual spot check passes, while nothing is left for the model to match against. This parses each `SKILL.md` the same way, requires an explicit invocation mode on every skill and committed eval cases (positives **and** negatives) on every model-invoked one, and fails when the skill listing exceeds its recorded character budget. Also runs `claude plugin validate` — the authoritative schema — when that CLI is present and at least 2.1.233 (below that the subcommand does not exist and exits nonzero, which must not fail the gate for an un-upgraded contributor). Runs in `validate`; CI installs a pinned CLI and runs the authoritative validator as its own step. | | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Two more arrive with the skills (#2163): `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description — and `skill-eval.test.mjs` covers the trigger eval's stream parsing and its process-outcome handling, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 8306355e..407cd060 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -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, rest: string }} + * @returns {{ invoked: Set, 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); diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index b11fcca1..9f39febc 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -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`/, ); }); diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs new file mode 100644 index 00000000..2ed7fc80 --- /dev/null +++ b/scripts/verify-skills.main.test.mjs @@ -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} 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 }); +}); diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 13871ddc..9dc0726a 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -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