chore: address Copilot review round 4
- verify:skills:cli — the authoritative validator is now a GUARANTEED step in `local:gate` and CI, not a best-effort hand-off. It resolves the CLI itself (an installed one at 2.1.233+, else a pinned `npx -y`), so both tiers run the same script and cannot drift. `verify:skills` keeps skipping when the CLI is absent, which is right for `validate`: fast, offline, no Claude Code required. - skill-manifest: the 1,536-char listing-entry cap covers `description` + `when_to_use` together; checking the description alone passed an over-cap entry. `when_to_use` is now parsed, validated and counted, with boundary tests at 1,536/1,537. - verify-skills: report the budget as *this repo's contribution*. The real listing budget is shared with bundled skills and the contributor's own ~/.claude/skills, none of which is visible from here, so the number was claiming more than it measured. - skill-eval: a negative case now means "no skill OF THIS REPO'S fired". It failed on any Skill invocation, so a contributor with an unrelated global skill got a false failure about their environment rather than these skills. - docs/architecture + local-dev: the Node backend runner gets its `@inspector/core` alias from `clients/web/tsup.runner.config.ts`, not from Vite. The old text sent runner work to the wrong config. - local-dev: the tarball does not ship "only each client's build/" — the allowlist also carries `clients/web/dist`, `clients/web/static` and the install helper. States the invariant that actually matters instead. - issue-triage: the test for needing triage is "no board card". Adding "and no milestone" would skip the approved-but-unboarded case the same section then handles. Also corrected the `$M` note: the lookup is `--state all`, so closed issues DO have entries — `isopen` is about the invariants, not the lookup. - issue-create: replaced "all five" with a per-version table. Steps 3 and 6 (milestone, Priority) do not exist for v1, so the old list opened with a rule the same skill later tells you to violate. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
7e0bb8b58d
commit
d05e76d470
18 changed files with 330 additions and 51 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
12
.github/workflows/main.yml
vendored
12
.github/workflows/main.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
10
AGENTS.md
10
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.**
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
||||
|
|
|
|||
|
|
@ -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 `// <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`; 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`.
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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];
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string>} invoked
|
||||
* @param {Set<string> | 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++;
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
|
|
|||
101
scripts/verify-skills-cli.mjs
Normal file
101
scripts/verify-skills-cli.mjs
Normal file
|
|
@ -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();
|
||||
}
|
||||
52
scripts/verify-skills-cli.test.mjs
Normal file
52
scripts/verify-skills-cli.test.mjs
Normal file
|
|
@ -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/);
|
||||
});
|
||||
|
|
@ -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 = {};
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in a new issue