diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 121a97fe..9af16125 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -107,16 +107,26 @@ committed — so attach them to the PR body from there rather than referencing a in-repo path. Name them for what they show (`tools-tab-before.png`), not `Screenshot 2026-07-31 at 14.02.11.png`. -To host them, upload to GitHub's attachment endpoint with your `gh` token; the -parameters go in the **query string** with the raw bytes as the body (a JSON -body fails with a misleading "Invalid name for request"): +To host them, upload to GitHub's attachment endpoint with your `gh` token. Two +mechanics, both of which bite: + +- The parameters go in the **query string**, with the raw bytes as the body. A + JSON body fails with a misleading "Invalid name for request". +- ⚠️ **Do not put the token in argv.** `-H "Authorization: token $(gh auth + token)"` puts your credential in curl's command line, where any local user or + process can read it off the process table while the upload runs (Copilot). + Feed it through `--config -` instead: curl reads its options from stdin, so + the token never becomes an argument. ```sh -curl -sX POST -H "Authorization: token $(gh auth token)" \ - --data-binary @pr-screenshots/tools-tab-after.png \ +printf 'header = "Authorization: token %s"\n' "$(gh auth token)" | curl -sS --config - \ + -X POST --data-binary @pr-screenshots/tools-tab-after.png \ "https://uploads.github.com/user-attachments/assets?repository_id=&name=tools-tab-after.png&content_type=image/png" ``` +(The token is still in the shell's environment and in `printf`'s *stdin*, which +is not world-readable the way `/proc//cmdline` is.) + ## 6. Open the PR Target the base branch matching the work: **`v2/main`** for v2, **`v1/main`** diff --git a/AGENTS.md b/AGENTS.md index 1dfea7ef..dd317894 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -156,9 +156,12 @@ that from happening: 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. + It resolves the CLI rather than hoping for one: an installed CLI **only when + it matches the pin exactly**, otherwise the pinned package via `npx -y`. + Exact rather than a floor, because a newer local CLI is a *different* schema + from CI's — accepting it would let the same `local:gate` disagree across + machines, which is what a pin exists to prevent. Both tiers run the same + script, so they cannot drift either. ⚠️ 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 33a3220b..018c0144 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -11,7 +11,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | Tier | How it runs | What it covers | | --- | --- | --- | -| **GitHub CI** (`.github/workflows/main.yml`) | Automatically, on every push | `npm install`, then `validate`, `verify:build-gate`, `verify:bundle-externals`, `smoke` (which includes `smoke:web:chromium`), `test:storybook` — plus `coverage` in a parallel job ([#2159](https://github.com/modelcontextprotocol/inspector/issues/2159)) | +| **GitHub CI** (`.github/workflows/main.yml`) | Automatically, on every push | `npm install`, then `validate`, `verify:skills:cli`, `verify:build-gate`, `verify:bundle-externals`, `smoke` (which includes `smoke:web:chromium`), `test:storybook` — plus `coverage` in a parallel job ([#2159](https://github.com/modelcontextprotocol/inspector/issues/2159)) | | **The local gate** (`npm run local:gate`) | By hand, before you push | Every check above (the install is yours to run), **plus** the Firefox engine pass (`smoke:web:firefox`), and `smoke:tui` for real rather than self-skipped | The local gate is a **strict superset**, not a mirror. Two of its steps have no GitHub CI counterpart, each for its own reason: @@ -36,11 +36,11 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `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`; 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: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 **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. 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. Four 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. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, 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. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. 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`. -| `npm run local:gate` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | +| `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. diff --git a/scripts/verify-skills-cli.mjs b/scripts/verify-skills-cli.mjs index 5e788ea5..d01f86e0 100644 --- a/scripts/verify-skills-cli.mjs +++ b/scripts/verify-skills-cli.mjs @@ -9,10 +9,12 @@ // "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 does NOT skip: an already-installed CLI is used only when it matches the +// pin EXACTLY, and otherwise the pinned package 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 — +// and exact rather than a floor because a newer local CLI is a DIFFERENT schema +// from CI's, which would let the same `local:gate` disagree across machines. // // It needs no authentication — verified against a scrubbed environment and a // clean HOME. @@ -24,15 +26,22 @@ import { compareVersions, parseClaudeVersion, PINNED_CLI_VERSION, - PLUGIN_VALIDATE_MIN_VERSION, } from "./lib/skill-manifest.mjs"; +/** `PINNED_CLI_VERSION` as a comparable triple. */ +const PINNED_VERSION_PARTS = parseClaudeVersion(PINNED_CLI_VERSION) ?? []; + 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`. + * The argv that runs the pinned validator. + * + * A local CLI is used only when it matches `PINNED_CLI_VERSION` **exactly**. + * Accepting anything at or above the floor would defeat the pin: a maintainer on + * a newer CLI would validate against a different schema than CI's, so the same + * `local:gate` could disagree across machines — which is the failure a pin + * exists to prevent (Copilot). Everyone else runs the pinned package. * * @param {{ version: number[] | null }} local * @returns {{ command: string, args: string[], via: string }} @@ -40,14 +49,18 @@ const SKILLS_DIR = ".claude/skills"; export function validatorCommand({ version }) { if ( version !== null && - compareVersions(version, PLUGIN_VALIDATE_MIN_VERSION) >= 0 + compareVersions(version, PINNED_VERSION_PARTS) === 0 ) { return { command: "claude", args: ["plugin", "validate", SKILLS_DIR], - via: `local claude ${version.join(".")}`, + via: `local claude ${version.join(".")} (matches the pin)`, }; } + const why = + version === null + ? "no local CLI" + : `local CLI is ${version.join(".")}, not the pinned ${PINNED_CLI_VERSION}`; return { command: "npx", args: [ @@ -57,45 +70,64 @@ export function validatorCommand({ version }) { "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)`, + via: `npx @anthropic-ai/claude-code@${PINNED_CLI_VERSION} (${why})`, }; } -function main() { - const probe = spawnSync("claude", ["--version"], { encoding: "utf8" }); +/** + * Run the authoritative validator. + * + * The probe and the spawn are injected so the orchestration itself is tested: + * on a machine with a working CLI, an actual run only ever walks the happy + * path, so a regression in the probe, the spawn-error branch, or the + * propagation of a rejected validator would leave `test:scripts` green while + * this gate quietly stopped gating (Copilot). + * + * @param {{ probe?: () => {error?: Error, status?: number|null, stdout?: string}, + * spawn?: (cmd: string, args: string[], opts: object) => {error?: Error, status?: number|null}, + * log?: (msg: string) => void, error?: (msg: string) => void }} [io] + * @returns {number} Process exit code. + */ +export function runValidator(io = {}) { + const { + probe = () => spawnSync("claude", ["--version"], { encoding: "utf8" }), + spawn = (cmd, args, opts) => spawnSync(cmd, args, opts), + log = console.log, + error = console.error, + } = io; + + const probed = probe(); const version = - probe.error || probe.status !== 0 + probed.error || probed.status !== 0 ? null - : parseClaudeVersion(probe.stdout ?? ""); + : parseClaudeVersion(probed.stdout ?? ""); const { command, args, via } = validatorCommand({ version }); - console.log(`verify:skills:cli — validating ${SKILLS_DIR} via ${via}…`); + log(`verify:skills:cli — validating ${SKILLS_DIR} via ${via}…`); - const res = spawnSync(command, args, { + const res = spawn(command, args, { cwd: ROOT, stdio: "inherit", shell: process.platform === "win32", }); if (res.error) { - console.error( + error( `\nverify:skills:cli — could not run \`${command}\`: ${res.error.message}`, ); - process.exit(1); + return 1; } if (res.status !== 0) { - console.error( + error( "\nverify:skills:cli — `claude plugin validate` rejected the skills.", ); - process.exit(1); + return 1; } + return 0; } if ( process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url) ) { - main(); + process.exit(runValidator()); } diff --git a/scripts/verify-skills-cli.test.mjs b/scripts/verify-skills-cli.test.mjs index f218fc5b..ba48caef 100644 --- a/scripts/verify-skills-cli.test.mjs +++ b/scripts/verify-skills-cli.test.mjs @@ -1,52 +1,111 @@ -// Tests for the authoritative-validator step's CLI resolution (#2163, Copilot). +// Tests for the guaranteed authoritative-validator step (#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. +// Two things are covered, and neither is reachable from an ordinary run on a +// machine that has a working, pinned CLI: +// +// 1. Which validator gets run. A local CLI is used only when it matches the +// pin EXACTLY — accepting anything newer would mean a maintainer validates +// against a different schema than CI, so the same `local:gate` could +// disagree across machines, which is the failure a pin exists to prevent. +// 2. The orchestration around it. A regression in the probe, the spawn-error +// branch, or the propagation of a rejected validator would otherwise leave +// `test:scripts` green while this gate stopped gating. 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"; +import { validatorCommand, runValidator } from "./verify-skills-cli.mjs"; +import { PINNED_CLI_VERSION } from "./lib/skill-manifest.mjs"; -test("uses an installed CLI that has the subcommand", () => { - const cmd = validatorCommand({ version: [2, 1, 250] }); +const pinned = PINNED_CLI_VERSION.split(".").map(Number); +const npxArgs = [ + "-y", + `@anthropic-ai/claude-code@${PINNED_CLI_VERSION}`, + "plugin", + "validate", + ".claude/skills", +]; + +test("uses a local CLI only when it matches the pin exactly", () => { + const cmd = validatorCommand({ version: pinned }); assert.equal(cmd.command, "claude"); assert.deepEqual(cmd.args, ["plugin", "validate", ".claude/skills"]); - assert.match(cmd.via, /local claude 2\.1\.250/); + assert.match(cmd.via, /matches the pin/); }); -test("uses the installed CLI exactly at the floor", () => { - assert.equal( - validatorCommand({ version: PLUGIN_VALIDATE_MIN_VERSION }).command, - "claude", - ); +test("falls back to the pinned package for any other local version", () => { + // Newer as well as older: a newer CLI is a DIFFERENT schema from CI's, which + // is exactly the cross-machine disagreement the pin exists to prevent. + for (const version of [ + [pinned[0], pinned[1], pinned[2] + 1], + [pinned[0], pinned[1], pinned[2] - 1], + [pinned[0] + 1, 0, 0], + [2, 1, 232], + ]) { + const cmd = validatorCommand({ version }); + assert.equal(cmd.command, "npx", `expected npx for ${version.join(".")}`); + assert.deepEqual(cmd.args, npxArgs); + assert.match(cmd.via, /not the pinned/); + } }); -test("falls back to a pinned npx when no CLI is present", () => { +test("falls back to the pinned package 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.deepEqual(cmd.args, npxArgs); 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/); +/** Collects what `runValidator` was asked to spawn. */ +function harness({ probe, spawnResult }) { + const spawned = []; + const logs = []; + const errors = []; + const code = runValidator({ + probe: () => probe, + spawn: (command, args, opts) => { + spawned.push({ command, args, opts }); + return spawnResult; + }, + log: (m) => logs.push(m), + error: (m) => errors.push(m), + }); + return { code, spawned, logs, errors }; +} + +const okProbe = { status: 0, stdout: `${PINNED_CLI_VERSION} (Claude Code)\n` }; + +test("runs the local CLI and succeeds when the validator accepts", () => { + const r = harness({ probe: okProbe, spawnResult: { status: 0 } }); + assert.equal(r.code, 0); + assert.equal(r.spawned[0].command, "claude"); + assert.equal(r.errors.length, 0); + assert.match(r.logs.join(), /validating \.claude\/skills via local claude/); +}); + +test("treats an unusable probe as no CLI at all", () => { + // A `claude` that fails `--version` cannot be trusted to have the subcommand. + for (const probe of [ + { error: new Error("ENOENT") }, + { status: 1, stdout: "" }, + { status: 0, stdout: "not a version" }, + ]) { + const r = harness({ probe, spawnResult: { status: 0 } }); + assert.equal(r.code, 0); + assert.equal(r.spawned[0].command, "npx"); + } +}); + +test("fails when the validator rejects the skills", () => { + const r = harness({ probe: okProbe, spawnResult: { status: 1 } }); + assert.equal(r.code, 1); + assert.match(r.errors.join(), /rejected the skills/); +}); + +test("fails when the validator cannot be spawned at all", () => { + const r = harness({ + probe: okProbe, + spawnResult: { error: new Error("spawn npx ENOENT") }, + }); + assert.equal(r.code, 1); + assert.match(r.errors.join(), /could not run `claude`: spawn npx ENOENT/); });