diff --git a/.claude/skills/issue-create/SKILL.md b/.claude/skills/issue-create/SKILL.md index d56ddbd5..8a7de0b2 100644 --- a/.claude/skills/issue-create/SKILL.md +++ b/.claude/skills/issue-create/SKILL.md @@ -6,16 +6,22 @@ disable-model-invocation: true # Creating an issue -An issue **you** create is not "created" until all five of these are true. A -label is a repo tag, the milestone is a release bucket, and the board is a -separate org project — `--label v2` does **not** add a board card, and adding a -card does **not** set a Status or a Priority. +An issue **you** create is not "created" until every step below that applies to +its version is done. A label is a repo tag, the milestone is a release bucket, +and the board is a separate org project — `--label v2` does **not** add a board +card, and adding a card does **not** set a Status or a Priority. -1. **Version label** — exactly one of `v1` / `v2` -2. **Type label** — exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question` -3. **Milestone** -4. **A card on the board for that version** — `v2` → #28, `v1` → #11 -5. **Status _and_ Priority set on that card** (Priority is v2-only) +| # | Step | v2 | v1 | +| --- | --- | --- | --- | +| 1 | **Version label** — exactly one of `v1` / `v2` | ✅ | ✅ | +| 2 | **Type label** — exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question` | ✅ | ✅ | +| 3 | **Milestone** | ✅ | ❌ — every milestone is a v2 release bucket, so there is none to take | +| 4 | **A card on the board for that version** — `v2` → #28, `v1` → #11 | ✅ | ✅ | +| 5 | **Status** on that card | ✅ | ✅ | +| 6 | **Priority** on that card | ✅ | ❌ — board #11 has no such field | + +The two ❌ rows are not oversights to fix later: the field does not exist for +that line, and `AGENTS.md` states both exceptions (Copilot). Set the labels and milestone at **create time**, never by backfilling: an unlabeled issue belongs to no version line and appears in no version-filtered diff --git a/.claude/skills/issue-triage/SKILL.md b/.claude/skills/issue-triage/SKILL.md index ac3bfac7..f1d73916 100644 --- a/.claude/skills/issue-triage/SKILL.md +++ b/.claude/skills/issue-triage/SKILL.md @@ -6,9 +6,11 @@ disable-model-invocation: true # Triaging issues -**An issue needs triage when it arrives with no board card and no milestone — -regardless of who filed it.** That is the whole test, and it is deliberately -about *state*, not authorship. An outside reporter has no board access, so their +**An issue needs triage when it has no board card — regardless of who filed +it.** That is the whole test, and it is deliberately about *state*, not +authorship. The milestone is not part of the test; it decides *where the card +lands* (milestoned → **Todo**, unmilestoned → **Incoming**), so an +approved-but-unboarded issue is still swept in rather than skipped (Copilot). An outside reporter has no board access, so their issue necessarily lands this way — but so does a maintainer's, whenever they open one by hand instead of through the `/issue-create` flow. Write access makes the board *reachable*, not automatic. What puts an issue in Todo is somebody @@ -276,9 +278,12 @@ Two things the queries must account for, both learned the hard way: 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. +- **`$M` holds closed issues too** — the lookup is built from + `gh issue list --state all`, which it has to be, because the last check reads + closed issues' state reasons. So `isopen` is not there to cope with a missing + entry; it is there because the *invariants* are about open work. A closed + issue legitimately sits in `Done` with whatever milestone it had, and a check + that did not gate on `isopen` would report every one of them (Copilot). **Do not "fix" a `Done` card that is missing a milestone.** Cards predating a rule are not defects to backfill in bulk — the audit exists to stop *new* drift, diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index bc164851..56ecffcf 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -29,8 +29,11 @@ need: the root `postinstall` (`scripts/install-clients.mjs`) cascades the root to re-sync every client. The cascade is dev-only — it exits early when the package is installed under -`node_modules`, and the published tarball ships only each client's `build/`. Set -`INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it. +`node_modules`, and a consumer never needs it because the tarball ships +**prebuilt** artifacts (each client's `build/`, plus `clients/web/dist` and +`clients/web/static`) rather than sources to compile. Set +`INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it. The `files` allowlist itself is +[`docs/publishing.md`](../../../docs/publishing.md). ⚠️ **In a git worktree, do a real `npm install`.** A symlinked `node_modules` passes lint, tests and coverage, then fails every Storybook story file on Vite's @@ -70,8 +73,11 @@ build-time alias: - **CLI / TUI** — `esbuildOptions.alias` in `tsup.config.ts` maps `@inspector/core` → the repo `core/` directory, with `noExternal: [/^@inspector\/core/]` inlining it. -- **Web** — the same alias in `clients/web/vite.config.ts`, for both the browser - app and the Node backend runner. +- **Web** — **two** configs, because the client is two builds: + `clients/web/vite.config.ts` for the browser app, and + `clients/web/tsup.runner.config.ts` for the Node backend runner, which + defines the alias itself. Dependency or debugging work on the runner belongs + in the tsup config, not the Vite one (Copilot). ## Where a dependency goes diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 31a69bea..e24e7efe 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -23,7 +23,7 @@ npm run format # auto-fix: core/, scripts/, the shared surface, every clien npm run local:gate # several minutes ``` -`local:gate` is `validate` → `coverage` → `verify:build-gate` → +`local:gate` is `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. It is a **strict superset** of GitHub CI (which additionally runs `npm install`, @@ -62,6 +62,13 @@ A tracked `.ts`/`.tsx`/`.mts`/`.cts` lands in no tsconfig project. Usually a new top-level file in a client whose build config roots at `./src`; add it to that client's `tsconfig.test.json` (or the src config's `include`). +### `verify:skills` / `verify:skills:cli` + +A `.claude/skills` manifest does not parse, declares no invocation mode, or a +model-invoked skill is missing its eval cases. `verify:skills:cli` is the +authoritative validator and fetches a pinned CLI over the network if you have +none installed — so it is also the one stage that will fail offline. + ### `verify:dep-lockstep` A dependency reaching one `tsc` program from two installs resolves to two diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9abb42eb..1f526c57 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -60,12 +60,12 @@ jobs: # 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 + # The script resolves the CLI itself — an installed one when it is new + # enough, otherwise a PINNED one via `npx -y` — so this is the same + # command `npm run local:gate` runs, and the two cannot drift. Pinned + # rather than @latest: an unpinned validator can start failing a PR that + # changed nothing. It needs no authentication — verified with a clean HOME. + run: npm run verify:skills:cli - name: Verify the browser-externalized-builtin build gate (#1769) # Runs a real `vite build` with a Node built-in forced into the browser diff --git a/AGENTS.md b/AGENTS.md index 05eee3c9..1dfea7ef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,9 +152,13 @@ that from happening: 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. + simply has not upgraded). + That best-effort hand-off keeps `validate` fast and offline, but it also + means it usually does not run — so the authoritative check gets a **guaranteed + step of its own**, `npm run verify:skills:cli`, in `local:gate` and in CI. + It resolves the CLI rather than hoping for one: an installed CLI when it is + new enough, otherwise a **pinned** one via `npx -y`. Both tiers run the same + script, so they cannot drift. ⚠️ 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/README.md b/README.md index 51573c55..b7671ed6 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ npm run coverage # the per-file ≥90% gate (lines/statements/functions/bran npm run local:gate # MANDATORY before pushing — a strict superset of GitHub CI ``` -`npm run local:gate` is `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. See [Testing and the quality gate](./docs/quality-gate.md) for what each stage covers and why two of them are local-only, and [`AGENTS.md`](./AGENTS.md) for the testing rules themselves. +`npm run local:gate` is `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. See [Testing and the quality gate](./docs/quality-gate.md) for what each stage covers and why two of them are local-only, and [`AGENTS.md`](./AGENTS.md) for the testing rules themselves. ## Contributing — `AGENTS.md`, `CLAUDE.md`, and the skills diff --git a/docs/architecture.md b/docs/architecture.md index 0aa2b00a..e04b7775 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -9,7 +9,7 @@ `core/` intentionally has **no `package.json`** — it is not published on its own. Each client bundles it in via a `@inspector/core` alias: - **CLI / TUI:** `esbuildOptions.alias` in their `tsup.config.ts` maps `@inspector/core` → the repo `core/` directory, and `noExternal: [/^@inspector\/core/]` inlines it into the bundle. -- **Web:** the same alias in `clients/web/vite.config.ts` for the browser app and the Node backend runner. +- **Web:** two configs, because the client is two builds — `clients/web/vite.config.ts` for the browser app, and `clients/web/tsup.runner.config.ts` for the Node backend runner, which defines the alias itself. Publishing `core/` as its own package (e.g. for third parties to build on) is deliberately deferred — see issue [#1636](https://github.com/modelcontextprotocol/inspector/issues/1636). diff --git a/docs/quality-gate.md b/docs/quality-gate.md index c9699c40..33a3220b 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -35,7 +35,8 @@ 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` — 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 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`; because that hand-off is best-effort, the authoritative validator also gets a guaranteed step of its own — see `verify:skills:cli` below. | +| `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one when it is new enough (2.1.233+), otherwise a **pinned** `npx -y @anthropic-ai/claude-code@…`. Pinned because a validator that moves on its own can start failing a PR that changed nothing. Needs no authentication. Not in `validate` — it can reach the network. | | `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/package.json b/package.json index ad1e99d9..1b5c58f6 100644 --- a/package.json +++ b/package.json @@ -38,12 +38,13 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "local:gate": "npm run validate && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", + "local:gate": "npm run validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", "local:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", "verify:bundle-externals": "node scripts/verify-bundle-externals.mjs", "verify:typecheck-coverage": "node scripts/verify-typecheck-coverage.mjs", "verify:skills": "node scripts/verify-skills.mjs", + "verify:skills:cli": "node scripts/verify-skills-cli.mjs", "test:scripts": "node --test \"scripts/**/*.test.mjs\"", "validate": "npm run verify:format-coverage && npm run verify:skills && npm run verify:typecheck-coverage && npm run verify:dep-lockstep && npm run test:scripts && npm run validate:core && npm run validate:web && npm run validate:cli && npm run validate:tui && npm run validate:launcher", "verify:format-coverage": "node scripts/verify-format-coverage.mjs", diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index bcb2052b..e4ca17fa 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -13,7 +13,11 @@ import { parse as parseYaml } from "yaml"; -/** Combined per-entry cap Claude Code applies to a skill listing entry. */ +/** + * Per-entry cap Claude Code applies to a skill listing entry. It covers + * `description` **and** `when_to_use` together, so checking `description` alone + * would pass an over-cap entry (Copilot). + */ export const DESCRIPTION_CAP = 1536; /** @@ -101,9 +105,18 @@ export function parseSkill(dirName, text) { const description = meta.description; if (typeof description !== "string" || description.trim().length === 0) { errors.push("`description` is required and must be a non-empty string"); - } else if (description.length > DESCRIPTION_CAP) { + } + + const whenToUse = meta["when_to_use"]; + if ("when_to_use" in meta && typeof whenToUse !== "string") { + errors.push("`when_to_use` must be a string"); + } + const entryLength = + (typeof description === "string" ? description.length : 0) + + (typeof whenToUse === "string" ? whenToUse.length : 0); + if (entryLength > DESCRIPTION_CAP) { errors.push( - `\`description\` is ${description.length} chars; Claude Code caps a listing entry at ${DESCRIPTION_CAP}`, + `\`description\` + \`when_to_use\` is ${entryLength} chars; Claude Code caps a listing entry at ${DESCRIPTION_CAP}`, ); } @@ -141,6 +154,7 @@ export function parseSkill(dirName, text) { return { name: typeof name === "string" ? name : undefined, description: typeof description === "string" ? description : undefined, + whenToUse: typeof whenToUse === "string" ? whenToUse : undefined, modelInvoked, userInvocable: meta["user-invocable"] !== false, paths: Array.isArray(meta.paths) ? meta.paths : undefined, @@ -152,13 +166,22 @@ export function parseSkill(dirName, text) { * Cost of the skill listing Claude Code loads into context, counting only the * skills that appear in it. * - * @param {Array<{ name?: string, description?: string, modelInvoked?: boolean }>} skills + * The number is this repo's CONTRIBUTION to the listing, not the whole listing: + * bundled skills and a contributor's own `~/.claude/skills` share the same + * budget, and none of those is visible from here (Copilot). Keeping the repo's + * share well under the cap is what leaves room for them. + * + * @param {Array<{ name?: string, description?: string, whenToUse?: string, modelInvoked?: boolean }>} skills */ export function listingCost(skills) { return skills .filter((s) => s.modelInvoked) .reduce( - (n, s) => n + (s.name?.length ?? 0) + (s.description?.length ?? 0), + (n, s) => + n + + (s.name?.length ?? 0) + + (s.description?.length ?? 0) + + (s.whenToUse?.length ?? 0), 0, ); } @@ -207,6 +230,13 @@ export function validateEvalCases(skillName, cases) { return errors; } +/** + * Claude Code version the authoritative validator is pinned to when it has to + * be fetched. Pinned rather than @latest: a validator that moves on its own can + * start failing a PR that changed nothing. + */ +export const PINNED_CLI_VERSION = "2.1.250"; + /** First Claude Code release with `claude plugin validate`. */ export const PLUGIN_VALIDATE_MIN_VERSION = [2, 1, 233]; diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 7b6b156c..69803a58 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -219,3 +219,36 @@ test("compareVersions orders the plugin-validate floor correctly", () => { assert.ok(compareVersions([3], PLUGIN_VALIDATE_MIN_VERSION) > 0); assert.ok(compareVersions([1, 9, 999], PLUGIN_VALIDATE_MIN_VERSION) < 0); }); + +test("the listing-entry cap covers description AND when_to_use together", () => { + // The cap is over the combined entry, so checking `description` alone would + // pass an over-cap listing entry. + const under = fm( + `name: x\ndescription: ${"a".repeat(1000)}\nwhen_to_use: ${"b".repeat(536)}\ndisable-model-invocation: true`, + ); + assert.deepEqual(parseSkill("x", under).errors, []); + + const over = fm( + `name: x\ndescription: ${"a".repeat(1000)}\nwhen_to_use: ${"b".repeat(537)}\ndisable-model-invocation: true`, + ); + assert.match(parseSkill("x", over).errors.join(), /caps a listing entry/); + + assert.match( + parseSkill( + "x", + fm( + "name: x\ndescription: d\nwhen_to_use: 7\ndisable-model-invocation: true", + ), + ).errors.join(), + /when_to_use. must be a string/, + ); +}); + +test("listingCost counts when_to_use too", () => { + assert.equal( + listingCost([ + { name: "aa", description: "bbb", whenToUse: "cc", modelInvoked: true }, + ]), + 7, + ); +}); diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 407cd060..8ad3de7c 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -45,6 +45,7 @@ const CONCURRENCY = Number(process.env.CONCURRENCY ?? 4); /** Collect the committed cases for every model-invoked skill (optionally one). */ function collectCases(only) { const cases = []; + const ours = new Set(); for (const dir of readdirSync(SKILLS_DIR).sort()) { if (only && dir !== only) continue; const skillFile = path.join(SKILLS_DIR, dir, "SKILL.md"); @@ -64,6 +65,7 @@ function collectCases(only) { ); } if (!skill.modelInvoked) continue; + ours.add(dir); const evalsFile = path.join(SKILLS_DIR, dir, "evals", "evals.json"); if (!existsSync(evalsFile)) { throw new Error( @@ -84,7 +86,7 @@ function collectCases(only) { } for (const c of parsed) cases.push({ ...c, from: dir }); } - return cases; + return { cases, ours }; } /** @@ -184,14 +186,22 @@ export function invokedSkillNames(payload) { /** * Whether one sample satisfies a case. * + * A negative case asserts that no skill **of this repo's** fired. Asserting + * that nothing fired at all would fail for a contributor who happens to have an + * unrelated skill in `~/.claude/skills`, or when a bundled skill matches — a + * false failure about someone else's environment rather than about these + * skills (Copilot). + * * @param {string | null} expect Skill name, or null for a negative case. * @param {Set} invoked + * @param {Set | null} [ours] Repo skill names. Null counts any skill. */ -export function sampleHit(expect, invoked) { - if (expect === null) return invoked.size === 0; - return [...invoked].some((payload) => - invokedSkillNames(payload).includes(expect), - ); +export function sampleHit(expect, invoked, ours = null) { + const names = [...invoked].flatMap(invokedSkillNames); + if (expect === null) { + return ours === null ? names.length === 0 : !names.some((n) => ours.has(n)); + } + return names.includes(expect); } /** @@ -273,7 +283,7 @@ async function main() { process.exit(1); } - const cases = collectCases(process.argv[2]); + const { cases, ours } = collectCases(process.argv[2]); if (cases.length === 0) { console.error("skills:eval — no cases found."); process.exit(1); @@ -288,7 +298,9 @@ async function main() { let failed = 0; for (const c of cases) { const mine = results.filter((r) => r.c === c); - const passes = mine.filter((r) => sampleHit(c.expect, r.invoked)).length; + const passes = mine.filter((r) => + sampleHit(c.expect, r.invoked, ours), + ).length; const rate = passes / mine.length; const ok = rate >= THRESHOLD; if (!ok) failed++; diff --git a/scripts/skill-eval.test.mjs b/scripts/skill-eval.test.mjs index 9f39febc..9933b939 100644 --- a/scripts/skill-eval.test.mjs +++ b/scripts/skill-eval.test.mjs @@ -197,3 +197,21 @@ test("runPrompt propagates a spawn error", async () => { /ENOENT/, ); }); + +test("a negative case ignores skills that are not this repo's", () => { + // A contributor's own `~/.claude/skills` entry, or a bundled skill, firing on + // a negative prompt says nothing about these skills — failing on it would be + // a false failure about someone else's environment. + const ours = new Set(["testing", "local-dev"]); + const foreign = new Set(['{"skill":"my-personal-notes"}']); + const mine = new Set(['{"skill":"testing"}']); + + assert.equal(sampleHit(null, foreign, ours), true); + assert.equal(sampleHit(null, mine, ours), false); + assert.equal(sampleHit(null, new Set(), ours), true); + // A positive case is unaffected: it names the skill it wants. + assert.equal(sampleHit("testing", mine, ours), true); + assert.equal(sampleHit("testing", foreign, ours), false); + // With no repo set, any invocation still fails a negative case. + assert.equal(sampleHit(null, foreign), false); +}); diff --git a/scripts/verify-skills-cli.mjs b/scripts/verify-skills-cli.mjs new file mode 100644 index 00000000..5e788ea5 --- /dev/null +++ b/scripts/verify-skills-cli.mjs @@ -0,0 +1,101 @@ +#!/usr/bin/env node +// Runs `claude plugin validate` — the AUTHORITATIVE skill schema — as a +// guaranteed step, resolving the CLI rather than hoping it is installed (#2163). +// +// `verify:skills` (inside `validate`) reimplements the parse and treats a +// missing CLI as a skip, deliberately: `npm run validate` is the fast inner +// loop, it must work offline, and a Node contributor should not need Claude Code +// installed to run it. But "skips when absent" plus "usually absent" adds up to +// "never runs", which made the acceptance criterion aspirational (Copilot). +// +// So the authoritative check gets its own step, in `local:gate` and in CI, and +// it does NOT skip: an already-installed CLI new enough to have the subcommand +// is used as-is, and otherwise a PINNED one is fetched with `npx -y`. Pinned +// rather than @latest because a validator that moves on its own can start +// failing a PR that changed nothing, which is how a gate loses its credibility. +// +// It needs no authentication — verified against a scrubbed environment and a +// clean HOME. + +import { spawnSync } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { + compareVersions, + parseClaudeVersion, + PINNED_CLI_VERSION, + PLUGIN_VALIDATE_MIN_VERSION, +} from "./lib/skill-manifest.mjs"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const SKILLS_DIR = ".claude/skills"; + +/** + * The argv that runs a usable validator: the local CLI when it is new enough, + * otherwise the pinned one through `npx`. + * + * @param {{ version: number[] | null }} local + * @returns {{ command: string, args: string[], via: string }} + */ +export function validatorCommand({ version }) { + if ( + version !== null && + compareVersions(version, PLUGIN_VALIDATE_MIN_VERSION) >= 0 + ) { + return { + command: "claude", + args: ["plugin", "validate", SKILLS_DIR], + via: `local claude ${version.join(".")}`, + }; + } + return { + command: "npx", + args: [ + "-y", + `@anthropic-ai/claude-code@${PINNED_CLI_VERSION}`, + "plugin", + "validate", + SKILLS_DIR, + ], + via: + version === null + ? `npx @anthropic-ai/claude-code@${PINNED_CLI_VERSION} (no local CLI)` + : `npx @anthropic-ai/claude-code@${PINNED_CLI_VERSION} (local ${version.join(".")} predates the subcommand)`, + }; +} + +function main() { + const probe = spawnSync("claude", ["--version"], { encoding: "utf8" }); + const version = + probe.error || probe.status !== 0 + ? null + : parseClaudeVersion(probe.stdout ?? ""); + + const { command, args, via } = validatorCommand({ version }); + console.log(`verify:skills:cli — validating ${SKILLS_DIR} via ${via}…`); + + const res = spawnSync(command, args, { + cwd: ROOT, + stdio: "inherit", + shell: process.platform === "win32", + }); + if (res.error) { + console.error( + `\nverify:skills:cli — could not run \`${command}\`: ${res.error.message}`, + ); + process.exit(1); + } + if (res.status !== 0) { + console.error( + "\nverify:skills:cli — `claude plugin validate` rejected the skills.", + ); + process.exit(1); + } +} + +if ( + process.argv[1] && + path.resolve(process.argv[1]) === fileURLToPath(import.meta.url) +) { + main(); +} diff --git a/scripts/verify-skills-cli.test.mjs b/scripts/verify-skills-cli.test.mjs new file mode 100644 index 00000000..f218fc5b --- /dev/null +++ b/scripts/verify-skills-cli.test.mjs @@ -0,0 +1,52 @@ +// Tests for the authoritative-validator step's CLI resolution (#2163, Copilot). +// +// The point of the step is that it does NOT skip: `verify:skills` treats a +// missing CLI as a skip, and "skips when absent" plus "usually absent" adds up +// to "never runs", which is what made the acceptance criterion aspirational. +// So the branch that matters is the one that falls back to a pinned `npx` +// rather than giving up — and on a machine with a current CLI installed, that +// branch is never taken by an actual run. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { validatorCommand } from "./verify-skills-cli.mjs"; +import { + PINNED_CLI_VERSION, + PLUGIN_VALIDATE_MIN_VERSION, +} from "./lib/skill-manifest.mjs"; + +test("uses an installed CLI that has the subcommand", () => { + const cmd = validatorCommand({ version: [2, 1, 250] }); + assert.equal(cmd.command, "claude"); + assert.deepEqual(cmd.args, ["plugin", "validate", ".claude/skills"]); + assert.match(cmd.via, /local claude 2\.1\.250/); +}); + +test("uses the installed CLI exactly at the floor", () => { + assert.equal( + validatorCommand({ version: PLUGIN_VALIDATE_MIN_VERSION }).command, + "claude", + ); +}); + +test("falls back to a pinned npx when no CLI is present", () => { + const cmd = validatorCommand({ version: null }); + assert.equal(cmd.command, "npx"); + assert.deepEqual(cmd.args, [ + "-y", + `@anthropic-ai/claude-code@${PINNED_CLI_VERSION}`, + "plugin", + "validate", + ".claude/skills", + ]); + assert.match(cmd.via, /no local CLI/); +}); + +test("falls back when the installed CLI predates the subcommand", () => { + // An older binary answers `--version` fine and then exits nonzero on + // `plugin validate` as an unknown command, so running it would fail the gate + // for someone whose only sin is not having upgraded. + const cmd = validatorCommand({ version: [2, 1, 232] }); + assert.equal(cmd.command, "npx"); + assert.match(cmd.via, /predates the subcommand/); +}); diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index 2ed7fc80..0bbf2c61 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -147,7 +147,7 @@ test("fails when the skill listing exceeds its budget", () => { const over = fixture(many); const { code, out } = run(over); assert.equal(code, 1); - assert.match(out, /over the recorded budget/); + assert.match(out, /over its recorded budget/); rmSync(over, { recursive: true, force: true }); const quiet = {}; diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 9dc0726a..31ce1aa6 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -111,8 +111,11 @@ function main(argv = process.argv.slice(2)) { const cost = listingCost(parsed); if (cost > LISTING_BUDGET) { failures.push( - `the skill listing costs ${cost} chars, over the recorded budget of ${LISTING_BUDGET}. ` + - "Tighten a description, or raise the budget deliberately in scripts/lib/skill-manifest.mjs.", + `this repo contributes ${cost} chars to the skill listing, over its recorded ` + + `budget of ${LISTING_BUDGET}. Tighten a description, or raise the budget ` + + "deliberately in scripts/lib/skill-manifest.mjs — but note the real listing " + + "budget is SHARED with bundled skills and the contributor's own " + + "~/.claude/skills, none of which is visible from here.", ); } @@ -126,7 +129,7 @@ function main(argv = process.argv.slice(2)) { const modelInvoked = parsed.filter((s) => s.modelInvoked).map((s) => s.name); console.log( `verify:skills — OK: ${parsed.length} skills, ${modelInvoked.length} model-invoked ` + - `(${modelInvoked.join(", ")}); listing ${cost}/${LISTING_BUDGET} chars.`, + `(${modelInvoked.join(", ")}); this repo's listing share ${cost}/${LISTING_BUDGET} chars.`, ); if (override) return;