From c3c8a33e2ca1ac04513222fc0ca4efec437267dc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 02:15:02 -0400 Subject: [PATCH] chore: address Copilot review round 6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - verify-skills: the best-effort `plugin validate` hand-off now also requires the EXACT pinned CLI. `local:gate` runs `validate` before the pinned `verify:skills:cli` step, so a newer local CLI could reject skills that CI's pinned validator accepts — and the gate would exit there, never reaching the reproducible check. One schema everywhere, or none. - Dropped `PLUGIN_VALIDATE_MIN_VERSION`: with both validators on the exact pin it was read only by its own test, and a constant nothing enforces is a claim nothing enforces. Its test now pins what actually matters — that equality is distinguishable from newer as well as older. - The CI step's comment described the old floor behavior. - issue-create: the duplicate check searched only open issues, so a completed or rejected match was invisible to a procedure that says never to duplicate. Searches all states and reports state/stateReason. Signed-off-by: cliffhall --- .claude/skills/issue-create/SKILL.md | 9 +++++++-- .github/workflows/main.yml | 9 ++++++--- AGENTS.md | 6 ++---- docs/quality-gate.md | 2 +- scripts/lib/skill-manifest.mjs | 3 --- scripts/lib/skill-manifest.test.mjs | 24 +++++++++++++----------- scripts/verify-skills.mjs | 28 +++++++++++++++++----------- 7 files changed, 46 insertions(+), 35 deletions(-) diff --git a/.claude/skills/issue-create/SKILL.md b/.claude/skills/issue-create/SKILL.md index 8a7de0b2..2d2b43db 100644 --- a/.claude/skills/issue-create/SKILL.md +++ b/.claude/skills/issue-create/SKILL.md @@ -33,9 +33,14 @@ item is a real GitHub issue. ## 0. Check the board first +Search **all** states, not just open. A closed issue is still a duplicate — it +may have been completed, rejected, or filed and superseded — and refiling it +loses that history. When one matches, the call is whether to reopen it or file +genuinely new work; either way you need to have seen it (Copilot). + ```sh -gh issue list --repo modelcontextprotocol/inspector --state open --limit 1000 \ - --search "" --json number,title,labels,milestone +gh issue list --repo modelcontextprotocol/inspector --state all --limit 1000 \ + --search "" --json number,title,state,stateReason,labels,milestone ``` ## 1. Pick the labels diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 1f526c57..d86cce57 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -60,9 +60,12 @@ jobs: # would mean always, in CI. Installing it here is what makes the # acceptance criterion true rather than aspirational. # - # 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 + # The script resolves the CLI itself — an installed one ONLY when it + # matches the pin exactly, otherwise the pinned package via `npx -y` — + # so this is the same command `npm run local:gate` runs, and the two + # cannot drift. Exact rather than a floor: a newer local CLI is a + # DIFFERENT schema, which is how a "reproducible" gate starts disagreeing + # across machines. 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 diff --git a/AGENTS.md b/AGENTS.md index dd317894..074d5576 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,10 +149,8 @@ that from happening: **malformed YAML**, which loads the body with an *empty* description, so `/skill-name` still works and a manual spot check passes while the skill can never auto-fire again. An unquoted colon in a description is enough. It also - runs `claude plugin validate` — 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). + runs `claude plugin validate` — the authoritative schema — when the installed + CLI is **exactly** the pinned version, and otherwise says so and moves on. 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. diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 018c0144..67d8ee8c 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -35,7 +35,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. Part of `npm run local:gate` and of GitHub CI. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run verify:skills` | Guards `.claude/skills` ([#2163](https://github.com/modelcontextprotocol/inspector/issues/2163)). A skill's body loads only when it is invoked, so one that stops being reachable loses behavior **silently** — and the way that happens is frontmatter Claude Code cannot parse, which loads the body with an *empty* description: `/skill-name` still works, so a manual spot check passes, while nothing is left for the model to match against. This parses each `SKILL.md` the same way, requires an explicit invocation mode on every skill and committed eval cases (positives **and** negatives) on every model-invoked one, and fails when the skill listing exceeds its recorded character budget. Also runs `claude plugin validate` — 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` | 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 the installed CLI is **exactly** the pinned version, and otherwise says so and moves on. It runs inside `validate`, which `local:gate` executes *before* the pinned step, so accepting any other local CLI would let it reject skills that CI accepts and exit the gate before the reproducible check ever ran. 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 **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`. | diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index e4ca17fa..a5932330 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -237,9 +237,6 @@ export function validateEvalCases(skillName, cases) { */ 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]; - /** * Read a semver triple out of `claude --version` output ("2.1.250 (Claude Code)"). * diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 69803a58..0661d0a8 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -13,7 +13,7 @@ import { listingCost, parseClaudeVersion, compareVersions, - PLUGIN_VALIDATE_MIN_VERSION, + PINNED_CLI_VERSION, } from "./skill-manifest.mjs"; const fm = (body) => `---\n${body}\n---\n\n# Body\n`; @@ -208,16 +208,18 @@ test("parseClaudeVersion reads the CLI's version banner", () => { assert.equal(parseClaudeVersion("no version here"), null); }); -test("compareVersions orders the plugin-validate floor correctly", () => { - // An older CLI has no `plugin validate` and exits nonzero on it as an unknown - // command; failing the mandatory gate for that would be wrong, so the probe - // must skip below the floor rather than run and fail. - assert.ok(compareVersions([2, 1, 232], PLUGIN_VALIDATE_MIN_VERSION) < 0); - assert.equal(compareVersions([2, 1, 233], PLUGIN_VALIDATE_MIN_VERSION), 0); - assert.ok(compareVersions([2, 1, 250], PLUGIN_VALIDATE_MIN_VERSION) > 0); - assert.ok(compareVersions([2, 2, 0], PLUGIN_VALIDATE_MIN_VERSION) > 0); - assert.ok(compareVersions([3], PLUGIN_VALIDATE_MIN_VERSION) > 0); - assert.ok(compareVersions([1, 9, 999], PLUGIN_VALIDATE_MIN_VERSION) < 0); +test("compareVersions can decide exact equality with the pin", () => { + // Both validators require the EXACT pinned version, so what matters is that + // "equal" is distinguishable from newer as well as older — a floor would let a + // newer local CLI validate against a different schema than CI's. + const pin = parseClaudeVersion(PINNED_CLI_VERSION); + assert.equal(compareVersions(pin, pin), 0); + assert.ok(compareVersions([pin[0], pin[1], pin[2] - 1], pin) < 0); + assert.ok(compareVersions([pin[0], pin[1], pin[2] + 1], pin) > 0); + assert.ok(compareVersions([pin[0] + 1, 0, 0], pin) > 0); + // Shorter and longer triples still order sanely. + assert.ok(compareVersions([pin[0]], pin) < 0); + assert.equal(compareVersions([...pin, 0], pin), 0); }); test("the listing-entry cap covers description AND when_to_use together", () => { diff --git a/scripts/verify-skills.mjs b/scripts/verify-skills.mjs index 31ce1aa6..3e189db4 100755 --- a/scripts/verify-skills.mjs +++ b/scripts/verify-skills.mjs @@ -33,7 +33,7 @@ import { fileURLToPath } from "node:url"; import { compareVersions, parseClaudeVersion, - PLUGIN_VALIDATE_MIN_VERSION, + PINNED_CLI_VERSION, parseSkill, validateEvalCases, listingCost, @@ -43,6 +43,9 @@ import { const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const DEFAULT_SKILLS_DIR = path.join(ROOT, ".claude", "skills"); +/** `PINNED_CLI_VERSION` as a comparable triple. */ +const PINNED_VERSION_PARTS = parseClaudeVersion(PINNED_CLI_VERSION) ?? []; + /** Directory names under `.claude/skills` that contain a SKILL.md. */ export function skillDirs( dir, @@ -134,27 +137,30 @@ function main(argv = process.argv.slice(2)) { if (override) return; - // The authoritative parse, when it is available — strictly a bonus. An older - // CLI does not have `plugin validate` and exits nonzero on it as an unknown - // command, which would fail the MANDATORY gate for a contributor whose only - // sin is not having upgraded (Copilot). So gate on the version that - // introduced it and skip below it, reserving a failure for a validator that - // actually rejected the skills. + // The authoritative parse, when it is available — strictly a bonus, and only + // from the EXACT pinned CLI. + // + // `local:gate` runs `validate` (and therefore this) before the pinned + // `verify:skills:cli` step, so a local CLI on any other version could reject + // skills that CI's pinned validator accepts — and the gate would exit here, + // never reaching the reproducible step (Copilot). Accepting merely "new enough" + // would reintroduce exactly the cross-machine schema drift the pin removes. + // Anything else is left to `verify:skills:cli`, which fetches the pin. const probe = spawnSync("claude", ["--version"], { encoding: "utf8" }); if (probe.error || probe.status !== 0) { console.log( - "verify:skills — `claude` CLI not usable here; skipped `claude plugin validate`.", + "verify:skills — no usable `claude` CLI here; `verify:skills:cli` runs the pinned validator.", ); return; } const version = parseClaudeVersion(probe.stdout ?? ""); if ( version === null || - compareVersions(version, PLUGIN_VALIDATE_MIN_VERSION) < 0 + compareVersions(version, PINNED_VERSION_PARTS) !== 0 ) { console.log( - `verify:skills — \`claude\` ${version?.join(".") ?? "(unreadable version)"} predates ` + - `${PLUGIN_VALIDATE_MIN_VERSION.join(".")}; skipped \`claude plugin validate\`.`, + `verify:skills — local \`claude\` is ${version?.join(".") ?? "(unreadable)"}, not the pinned ` + + `${PINNED_CLI_VERSION}; \`verify:skills:cli\` runs the pinned validator.`, ); return; }