chore: address Copilot review round 5
- verify-skills-cli: use a local CLI only when it matches the pin EXACTLY. A
floor let a maintainer on a newer CLI validate against a different schema than
CI's, so the same `local:gate` could disagree across machines — the failure a
pin exists to prevent.
- verify-skills-cli: extract `runValidator({probe, spawn, log, error})` and
cover the orchestration — validator accepts, validator rejects, spawn fails,
and the three unusable-probe shapes. On a machine with a working CLI an actual
run only ever walks the happy path.
- pr-flow: stop putting the `gh` token in curl's argv, where any local process
can read it off the process table. Feed the header through `curl --config -`.
- docs/quality-gate: three places had drifted — the CI tier row, the
`local:gate` chain, and the `test:scripts` inventory (which now names all four
suites this PR adds, not two).
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
d05e76d470
commit
75ab1b069f
5 changed files with 176 additions and 72 deletions
|
|
@ -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=<REPO_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/<pid>/cmdline` is.)
|
||||
|
||||
## 6. Open the PR
|
||||
|
||||
Target the base branch matching the work: **`v2/main`** for v2, **`v1/main`**
|
||||
|
|
|
|||
|
|
@ -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.**
|
||||
|
|
|
|||
|
|
@ -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 `// <path>` module banners, so it covers both shapes — a separate `<pkg>-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.
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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/);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue