Uh oh!
There was an error while loading. Please reload this page.
chore: extract AGENTS.md recipes into agent skills - #2189
Conversation
Closes#2163 `AGENTS.md` was one 1,223-line contract loaded in full on every turn, whether the task was a one-line CSS fix or a release cut. This splits it: rules stay resident, procedures move to `.claude/skills/`. - Ten skills under `.claude/skills/`, seven `disable-model-invocation: true` (invoked by name only, so they cost no listing context and have no trigger question) and three model-invoked with committed eval cases. - `AGENTS.md` 1,223 → 380 lines: rules, a skills index, and a project tree reduced to one line per entry. The removed recipes are not duplicated. - `README.md` 811 → 105 lines, split into `docs/{architecture,quality-gate, test-servers,publishing,docker}.md`, and dropped from `CLAUDE.md`'s auto-load. - `.github/copilot-instructions.md` and its mirroring rule removed — Copilot reads `AGENTS.md`. - `npm run verify:skills` (in `validate`, so in `local:gate` and CI) parses each SKILL.md the way Claude Code does, requires an explicit invocation mode and eval cases for model-invoked skills, and budgets the listing. - `npm run skills:eval` runs the committed trigger cases headless. Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟡 Changes recommended
The skill guard, evaluator, board workflows, and extracted documentation contain correctness and consistency issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extracts procedural guidance from resident instructions into scoped agent skills, supported by validation tooling and reorganized documentation.
Changes:
- Adds ten agent skills with trigger evaluations.
- Adds skill validation/evaluation scripts to the development workflow.
- Splits the root documentation into focused guides.
File summaries
| File | Description |
|---|---|
AGENTS.md | Retains rules and indexes skills. |
README.md | Condenses project and documentation overview. |
CLAUDE.md | Stops automatically loading the README. |
.gitignore | Allows committed skills while ignoring local state. |
.github/copilot-instructions.md | Removes duplicated review guidance. |
package.json | Adds skill validation and evaluation commands. |
scripts/verify-skills.mjs | Validates committed skill manifests and evaluations. |
scripts/skill-eval.mjs | Measures model-triggered skill invocation. |
scripts/lib/skill-manifest.mjs | Parses and validates skill metadata. |
scripts/lib/skill-manifest.test.mjs | Tests skill manifest rules. |
docs/architecture.md | Documents shared architecture. |
docs/quality-gate.md | Extracts quality-gate reference material. |
docs/test-servers.md | Extracts the test-server catalogue. |
docs/publishing.md | Extracts packaging and release documentation. |
docs/docker.md | Extracts container guidance. |
clients/launcher/README.md | Redirects publishing documentation. |
.claude/skills/board-ops/SKILL.md | Adds project-board procedures. |
.claude/skills/issue-create/SKILL.md | Adds issue-creation procedures. |
.claude/skills/issue-triage/SKILL.md | Adds triage and audit procedures. |
.claude/skills/local-dev/SKILL.md | Adds local-development guidance. |
.claude/skills/local-dev/evals/evals.json | Adds local-development trigger cases. |
.claude/skills/pre-push-gate/SKILL.md | Adds gate-diagnosis procedures. |
.claude/skills/pr-flow/SKILL.md | Adds pull-request workflow procedures. |
.claude/skills/project-structure/SKILL.md | Adds repository-placement guidance. |
.claude/skills/project-structure/evals/evals.json | Adds structure trigger cases. |
.claude/skills/release/SKILL.md | Adds release procedures. |
.claude/skills/test-servers/SKILL.md | Adds test-server execution guidance. |
.claude/skills/testing/SKILL.md | Adds testing procedures. |
.claude/skills/testing/evals/evals.json | Adds testing trigger cases. |
Review details
Suppressed comments (2)
.claude/skills/issue-triage/SKILL.md:244
- Likewise, this only detects issues with no type label; an issue carrying multiple type labels passes even though
AGENTS.mdrequires exactly one. Count labels from the five-type set and flag counts other than one.
"open, no type label": [$o[0][] | select(.state=="OPEN")
| select(([.labels[].name] | index("bug") or index("enhancement")
or index("documentation") or index("chore")
or index("question"))|not) | .number],
docs/quality-gate.md:38
- This supposedly comprehensive
test:scriptsreference omits the newly addedskill-manifest.test.mjssuite. Add it to the parser-test list so the extracted quality-gate documentation remains accurate.
| `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. 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. |
- Files reviewed: 26/29 changed files
- Comments generated: 13
- Review effort level: Balanced
| }; | ||
| } | ||
| const rest = text.slice(FRONTMATTER_FENCE.length + 1); | ||
| const end = rest.indexOf("\n" + FRONTMATTER_FENCE); |
| } | ||
| }); | ||
| p.on("error", reject); | ||
| p.on("close", () => resolve(invoked)); |
| ## Showcase configs | ||
| Each config below is a ready-made server for exercising one feature by hand. Load one with `--config`, and unless noted, connect with **Protocol Era = Modern**. |
| ⚠️ **Every milestone is a v2 release bucket.** There is no v1 bucket, so a `v1` | ||
| issue cannot satisfy the milestone rule — don't drop it in a v2.x one. Leave it | ||
| unmilestoned and say so. |
| "open, no version label":[$o[0][] | select(.state=="OPEN") | ||
| | select(([.labels[].name]|index("v1") or index("v2"))|not) | .number], |
| # From the repo root, with a built launcher: | ||
| node clients/launcher/build/index.js --web | ||
| # then in the Inspector, add the server the config announces |
| ```sh | ||
| ITEM_ID=$(gh project item-add 28 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') | ||
| # Status → Todo | ||
| gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" \ | ||
| --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id fbdaf21e | ||
| # Priority → Medium (score it with the rubric in /issue-triage; don't eyeball it) | ||
| gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" \ | ||
| --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iJE4 --single-select-option-id da944a9c | ||
| ``` |
| **On merge of a v2 PR, manually close its issue and move the board item to | ||
| Done** (`259d6aab`), since auto-close won't fire on `v2/main`. |
| // NOT part of `validate`, `local:gate`, or CI: it needs the `claude` CLI and | ||
| // real model calls, so it is metered and non-deterministic. Run it when adding | ||
| // a skill or editing a model-invoked skill's description. |
| function runOnce(prompt) { | ||
| return new Promise((resolve, reject) => { | ||
| const p = spawn( |
- skill-manifest: the closing `---` fence must occupy a whole line. `---oops` was accepted as a terminator, silently truncating the frontmatter and passing validation — the exact failure the guard exists to catch. Regression cases added (prefix, trailing whitespace, EOF, CRLF). - skill-eval: reject a nonzero `claude -p` exit instead of resolving it as an empty observation. An auth error or rate limit was scoring as "no skill invoked", which passes every negative case and reads as a trigger miss on every positive one. - skill-eval: extract `collectSkillInvocations` / `sampleHit` behind a `main()` guard and cover them in `skill-eval.test.mjs` — the stream parsing and the process-outcome handling are unreachable from an eval run's happy path. - docs/test-servers: annotate the era on every catalogue row. Six legacy rows carried no annotation under a "unless noted, Modern" default, so the doc sent readers to the wrong era for the feature they were trying to see. - docs/quality-gate: `validate` runs four guards, not three; add the `verify:skills` row and the two new `test:scripts` suites; fix the `#publishing` fragment left dangling by the split. - issue-create / pr-flow: delegate the card steps to `/board-ops` rather than restating its option IDs, which are explicitly unstable. - issue-triage: the audit counted presence, not cardinality — an issue carrying both `v1` and `v2` passed a check that claims "exactly one". Same for the five type labels. Also scope the two milestone checks to board #28. - AGENTS.md: codify the v1 milestone exception. Every milestone is a v2 release bucket, so a v1 issue has none to take and the Incoming-milestone invariant cannot apply to board #11. - clients/launcher/README: its pointer promised an explanation of why the cli/tui manifests ship, which the extracted guide does not make. Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 29, 2026
Copilot review round 1 — responsesAll 13 comments read; 12 acted on, 1 declined with reasoning. Pushed in a51e62d. Mirroring at PR level because inline replies go hidden once the fix is pushed. Fixed
**
Declined, with reasoning
Keeping it out, and the issue supports the split rather than contradicting it. Its acceptance criteria list The substantive reason is that a trigger eval cannot be a gate. It costs metered model calls on every push, it is non-deterministic by construction (the measurement is a hit rate over samples), and it fails on a rate limit or an expired token — so as a gate it would be red for reasons unrelated to the diff, and the first response to that is to stop trusting it. You are right that Two findings from the eval worth flagging
|
`verify:skills` checks that a skill is well-formed and that its cases exist; only `skills:eval` observes whether it actually fires, and that cannot be gated. Say so in the script header and in AGENTS.md rather than leaving the split to be inferred. Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟡 Changes recommended
Board procedures, v1 branching, evaluator matching, and validator compatibility contain unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
scripts/skill-eval.mjs:149
- The nonzero-exit behavior is still not covered:
skill-eval.test.mjsimports onlycollectSkillInvocationsandsampleHit, so changing this branch back to resolve an empty set would leave every test green despite the test header and quality-gate documentation saying this failure is pinned. Expose an injectable process/completion seam and assert that a nonzero close rejects.
if (code !== 0) {
reject(new Error(`\`claude -p\` exited ${code} for prompt: ${prompt}`));
return;
.claude/skills/board-ops/SKILL.md:147
- This delete lookup has the same repository-local issue-number ambiguity as the move recipe. On an organization board containing another repository's issue with the same number, it can delete the wrong card or pass multiple IDs; include the repository in the selector.
ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \
--jq '.items[] | select(.content.number==<ISSUE_NUMBER>) | .id')
- Files reviewed: 27/30 changed files
- Comments generated: 5
- Review effort level: Balanced
| export function sampleHit(expect, invoked) { | ||
| if (expect === null) return invoked.size === 0; | ||
| return [...invoked].some((payload) => payload.includes(expect)); | ||
| } |
| ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \ | ||
| --jq '.items[] | select(.content.number==<ISSUE_NUMBER>) | .id') |
| **Cut the branch from `v2/main`**, never from a milestone-merge branch — the | ||
| latter carries release-only commits that will show up in your PR's diff. |
| jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b11.json" --arg R "$R" ' | ||
| ($o[0] | map({key:(.number|tostring), value:{st:.state, sr:(.stateReason // ""), | ||
| lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $M | ||
| | def own($s): [$s[].items[] | select(.content.repository==$R)]; |
| // The authoritative parse, when it is available. | ||
| const probe = spawnSync("claude", ["--version"], { stdio: "ignore" }); | ||
| if (probe.error) { |
- verify-skills: version-gate the optional `claude plugin validate`. Probing
only for the executable failed the MANDATORY gate for anyone on a CLI older
than 2.1.233, which has no such subcommand and exits nonzero on it. Skips
below the floor; a failure now means a supported validator rejected the skills.
- skill-eval: match the invoked skill structurally. A substring test counted
`{"skill":"not-testing"}` as a hit for `testing` and inflated the rate.
- skill-eval: `runOnce` → exported `runPrompt` with an injectable `spawnFn`, so
the nonzero-exit branch is actually pinned. Reverting it now fails a test
(verified by mutation) rather than leaving the suite green.
- board-ops: filter the item lookups by repository. Issue numbers are repo-local
and these are org boards — #11 really does carry a `servers` card — so an
unfiltered `select` can move or delete the wrong card, or pass two ids.
- issue-triage: the audit's `own()` filter dropped draft cards, which carry no
`.content.repository`, so the "non-Issue on a board" check reported 0 while
the invariant was being violated. Verified against a fixture.
- pr-flow: cut the branch from the base it will target. The blanket "cut from
v2/main" was wrong for v1, whose history is unrelated.
Signed-off-by: cliffhall <cliff@futurescale.com>cliffhall
commented
Aug 29, 2026
Copilot review round 2 — responsesAll five new findings (including both suppressed) are real and are fixed in ca68289.
Suppressed,
|
There was a problem hiding this comment.
🟡 Changes recommended
The evaluator can silently omit broken skills, and the required authoritative CI validation is currently skipped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/skill-eval.mjs:59
- Malformed model-invoked skills and missing eval files are silently skipped here. Running
npm run skills:evaldirectly can therefore execute the remaining cases and exit successfully while omitting the very skill that was just broken. Fail collection on these conditions (and on invalid cases), or make this command run the verifier before evaluating.
if (skill.errors.length > 0 || !skill.modelInvoked) continue;
const evalsFile = path.join(SKILLS_DIR, dir, "evals", "evals.json");
if (!existsSync(evalsFile)) continue;
.claude/skills/issue-create/SKILL.md:109
- This contradicts the repository's issue forms: blank issues are disabled, the bug form applies
bug, and the feature form appliesenhancementplusv2(.github/ISSUE_TEMPLATE/*.yml). Describing all incoming issues as unlabeled can make the triage procedure reason from the wrong initial state.
An issue opened by hand in the GitHub UI — by an outside reporter *or* by a
maintainer — arrives with no label, no milestone, and no card. That is normal on
- Files reviewed: 27/30 changed files
- Comments generated: 2
- Review effort level: Balanced
| .sort(); | ||
| } | ||
| function main() { |
| 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`.", | ||
| ); | ||
| return; |
- CI now installs a pinned `claude` CLI and runs `claude plugin validate` as its own step. The guard skips the authoritative validator when the CLI is absent, which without this meant "always, in CI" — so the acceptance criterion was aspirational. Verified it needs no authentication (clean HOME). - skill-eval: classify a run from its terminal `result` event, not its exit code. With `--max-turns 1` a run in which a skill FIRES necessarily hits the limit and the CLI exits 1 (`error_max_turns`, `num_turns: 2`), so last round's blanket nonzero rejection threw away exactly the observations the eval counts. A run with no terminal `result`, or an unusable subtype, still rejects. - skill-eval: fail collection on a skill that does not parse, a model-invoked skill with no eval file, unreadable JSON, or invalid cases. Skipping them let `skills:eval` exit 0 while silently omitting the skill just broken. - verify-skills: `main()` takes an optional skills directory, and `verify-skills.main.test.mjs` drives it over fixtures — missing SKILL.md, malformed frontmatter, undeclared invocation mode, missing/invalid evals, budget overflow (and that name-only skills do not count toward it), and multi-offender reporting. Matches the sibling guards' *.main.test.mjs precedent; without it the gate could stop enforcing while the parser tests stayed green. - issue-create: the note claimed an issue arrives unlabeled, contradicting .github/ISSUE_TEMPLATE — blank issues are disabled, the bug form applies `bug`, the feature form applies `enhancement` + `v2`. Now a table of what each form sets and what triage still has to add. Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 29, 2026
Copilot review round 3 — responsesAll four (two inline, two suppressed) fixed in 7e0bb8b.
Suppressed, Suppressed, One correction to a round-2 fix, found while testing this roundLast round I implemented "reject nonzero exits" literally. That was wrong for this CLI, and the eval stopped working. With The concern behind the comment is still right, so it is now implemented at the correct layer: |
There was a problem hiding this comment.
🟡 Changes recommended
Skill-validation guarantees remain incomplete, and several extracted procedures contain contradictory or inaccurate guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
Previously missed (5) — in code that hasn't changed since the last review.
scripts/lib/skill-manifest.mjs:104
- The linked reliability criterion defines the 1,536-character cap over
description + when_to_use, but this checksdescriptionalone andparseSkilldoes not retainwhen_to_use. A future skill can therefore pass the local guard with an over-cap listing entry. Parse that field and validate the combined length, with a boundary test.
} else if (description.length > DESCRIPTION_CAP) {
docs/architecture.md:12
- The Node backend runner does not use Vite's alias; its build defines
@inspector/coreinclients/web/tsup.runner.config.ts:55-57. Pointing both surfaces atvite.config.tsmakes the architecture guide direct maintainers to the wrong configuration.
- **Web:** the same alias in `clients/web/vite.config.ts` for the browser app and the Node backend runner.
.claude/skills/local-dev/SKILL.md:33
- The tarball does not ship only each client's
build/: the rootfilesallowlist also includesclients/web/dist,clients/web/static, and the install helper. Describe the relevant invariant—consumers use prebuilt artifacts and do not run the client-install cascade—without contradicting the packaging guide.
This issue also appears on line 73 of the same file.
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.
.claude/skills/issue-triage/SKILL.md:16
- The actual sweep below selects every unboarded issue, including the explicitly handled case that already has a milestone. Defining the test as “no card and no milestone” would cause that approved-but-unboarded case to be skipped; the criterion should be absence of a board card, with milestone presence deciding Todo versus Incoming.
This issue also appears on line 279 of the same file.
**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
scripts/verify-skills.mjs:115
listingCost(parsed)counts only repository skills, but the 4,000-character listing budget is shared with bundled and~/.claude/skillsentries, as the linked reliability requirements note. This guard can therefore pass while the real listing is already truncating entries, and it permits repo skills alone to consume the entire shared budget. Reserve a measured baseline for non-repo entries or measure the complete listing with the pinned CLI; at minimum the reported value must be identified as only the repo contribution.
scripts/verify-skills.mjs:144
- This lets
local:gatepass without running the authoritative validator whenever the CLI is absent, while issue #2163 explicitly requiresclaude plugin validateto run in both the local gate and CI. Make the pinned validator a guaranteed local-gate step, or fail here with installation remediation instead of treating the missing validator as success.
.claude/skills/local-dev/SKILL.md:74 - The Node backend runner is built by tsup and gets this alias from
clients/web/tsup.runner.config.ts:55-57, not from Vite. The current instruction sends dependency/debugging work to the wrong config.
- **Web** — the same alias in `clients/web/vite.config.ts`, for both the browser
app and the Node backend runner.
.claude/skills/issue-create/SKILL.md:10
- The “all five” invariant cannot hold for v1: this skill later says v1 must have neither a milestone nor Priority, matching AGENTS.md. Define completion in terms of fields applicable to the version and mark both v2-only items in this list, otherwise the procedure starts with a rule it tells users to violate later.
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
.claude/skills/issue-triage/SKILL.md:281
$Iis built fromgh issue list --state all, so closed issues do have entries in$M. This explanation describes an older open-only query and obscures whyisopenis required; update it to explain that open-only invariants must exclude valid closed/Done cards.
- **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.
- Files reviewed: 29/32 changed files
- Comments generated: 1
- Review effort level: Balanced
| export function sampleHit(expect, invoked) { | ||
| if (expect === null) return invoked.size === 0; | ||
| return [...invoked].some((payload) => | ||
| invokedSkillNames(payload).includes(expect), |
- 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>
cliffhall
commented
Aug 29, 2026
Copilot review round 4 — responsesAll ten (one inline, nine suppressed) fixed in d05e76d. The two that changed the design
Correctness fixes
|
There was a problem hiding this comment.
🟡 Changes recommended
Validator reproducibility, credential exposure, missing orchestration coverage, and documentation drift remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 31/34 changed files
- Comments generated: 7
- Review effort level: Balanced
| if ( | ||
| version !== null && | ||
| compareVersions(version, PLUGIN_VALIDATE_MIN_VERSION) >= 0 | ||
| ) { |
| function main() { | ||
| const probe = spawnSync("claude", ["--version"], { encoding: "utf8" }); |
| curl -sX POST -H "Authorization: token $(gh auth token)" \ | ||
| --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" |
Uh oh!
There was an error while loading. Please reload this page.
| | `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`. | ||
| | `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. | |
| # jobs, but that's a larger restructure and deliberately out of scope. | ||
| run: npm run validate | ||
| - name: Validate the skills with the authoritative CLI (#2163) |
| | `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. | |
- 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>cliffhall
commented
Aug 29, 2026
Copilot review round 5 — responsesAll seven fixed in 75ab1b0, plus the PR description updated.
|
cliffhall
commented
Aug 29, 2026
Copilot review round 15 — responsesBoth points fixed in 3905cee (the round reported 0 new inline comments).
I noted in the source why exact equality is right here rather than in general: the commands this guard checks take no arguments, so a command that did would need prefix matching with an argument boundary instead. That is the assumption a future editor would otherwise have to rediscover. "Resident guidance contradicts the shipped manifests." Checked this by diffing Both now describe what the skill actually carries — the reasoning behind the placement rules, with the rules staying in |
There was a problem hiding this comment.
🔵 Needs a closer look
Prerelease CLI versions currently satisfy the stable exact-version pin.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/lib/skill-manifest.mjs:262
- The “exact pin” check drops SemVer suffixes: an installed
2.1.250-beta.1is parsed as[2, 1, 250]and therefore compares equal to the stable2.1.250pin. That can run a different validator schema locally while the logs claim it matches exactly. Parse the complete version banner (rejecting prerelease/build suffixes for this stable pin) and add a prerelease test case.
const m = /(\d+)\.(\d+)\.(\d+)/.exec(text);
- Files reviewed: 35/38 changed files
- Comments generated: 0 new
- Review effort level: Balanced
`parseClaudeVersion` dropped SemVer suffixes, so an installed `2.1.250-beta.1`
parsed as `[2, 1, 250]` and compared EQUAL to the stable pin — a different
validator schema running locally while the log claimed an exact match, which is
the cross-machine disagreement the pin exists to prevent.
It now returns `{ parts, prerelease }`, because the two callers want different
things: `isPinnedVersion` rejects any prerelease (build metadata still matches,
per SemVer precedence), while the eval's availability probe only needs some
usable CLI. `formatClaudeVersion` keeps the suffix in the messages, so a
prerelease is reported as what it is rather than as the pin.
Signed-off-by: cliffhall <cliff@futurescale.com>cliffhall
commented
Aug 29, 2026
Copilot review round 16 — responsesThe suppressed finding is fixed in c26995b (the round reported 0 new inline comments).
Build metadata still matches (
|
There was a problem hiding this comment.
🟡 Changes recommended
The CI wiring guard can pass even when the workflow no longer runs on pushes, and several updated JSDoc contracts remain stale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/verify-skills.mjs:98
ciRunsUnconditionallynever checks the workflow-levelontrigger. A workflow containing this step but changed toon: releasewould still returntrue, leaving the wiring guard green even though pushes and PR updates never run the validator. Validate that the workflow is triggered for pushes and add a release-only workflow fixture.
scripts/verify-skills.main.test.mjs:298- The positive fixture has no
on: pushtrigger, so it cannot verify the function's stated “on every push” contract. Add the trigger here and a negative case showing that the same step under a release-only workflow is rejected; that test should fail until the workflow-level check is implemented.
scripts/verify-skills-cli.mjs:85
- The injected probe now returns the parsed version object rather than
number[], andplatformis also accepted by the implementation but absent here. Keeping this annotation stale makes valid test harnesses appear type-invalid.
- Files reviewed: 35/38 changed files
- Comments generated: 2
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
| * @param {(text: string) => number[] | null} parseVersion | ||
| * @param {{ spawn?: typeof spawnSync, platform?: string }} [io] | ||
| * @returns {number[] | null} |
- ciRunsUnconditionally never looked at the workflow-level `on:` trigger, so a
workflow switched to `on: release` still contained the step and reported as
wired while no PR ever ran the validator. It now requires `push` or
`pull_request`, handling all three shapes `on:` takes. The positive fixtures
had no trigger at all, so they could not assert the "on every push" half of
the contract; they carry one now, alongside a release-only negative. Verified
by mutation.
- Stale JSDoc from the round-16 version change: `probeClaudeVersion` is now
generic in its parser's result rather than hard-coding `number[]`,
`validatorCommand` documents `{ parts, prerelease }`, and `runValidator`
documents the `platform` option it already accepted. Annotations that lie make
valid call sites look type-invalid.
Signed-off-by: cliffhall <cliff@futurescale.com>cliffhall
commented
Aug 29, 2026
Copilot review round 17 — responsesAll three fixed in 1203246.
It now requires
Stale JSDoc after the round-16 version change ( Worth conceding the pattern: an annotation that lies is worse than none, because it makes a correct call site look type-invalid — and I changed the parser's return type in round 16 without walking its consumers' docs. That is the same drift this PR keeps finding in prose, in a place I was not looking for it. |
There was a problem hiding this comment.
🟡 Changes recommended
The release recipe fails on fresh clones, and board snapshots can expose private project data through accidental commits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.claude/skills/release/SKILL.md:38
- This fails or reads the wrong starting version on a fresh/default clone: only
mainis checked out locally, sopackage.jsonis read frommainand the localv2/mainref may not exist. Fetch first, create the bump branch fromorigin/v2/main, and only then read the version.
# The branch name carries the version you are bumping TO, so read it first.
node -p "require('./package.json').version" # what is on v2/main now
git checkout -b v2/chore/<ISSUE>-bump-<X-Y-Z> v2/main
- Files reviewed: 35/38 changed files
- Comments generated: 1
- Review effort level: Balanced
| ```sh | ||
| gh project item-list 28 --owner modelcontextprotocol --format json --limit 600 > board-snapshot.json |
- board-ops: write board dumps to a temp dir, not the repo root. The boards are private, so a snapshot is a full export of item IDs and every card's Status and Priority — and the recipe left three such files in the worktree, one `git add -A` from being published in a PR. All three now go to `$BOARD_TMP`, and the snapshot step prints the path, since recovery needs it. - release: branch from `origin/v2/main` after fetching, and read the version once you are on it. A default clone has only `main` checked out, so the local ref may not exist and `package.json` was `main`'s — the released version rather than the one being bumped from. Added the one-liner for reading it before branching, since the branch name needs it. Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 29, 2026
Copilot review round 18 — responsesBoth fixed in d29bbbb.
All three now go to I chose a temp dir over
|
There was a problem hiding this comment.
🟡 Changes recommended
The local-gate wiring check can be satisfied by a non-executed textual command reference.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 35/38 changed files
- Comments generated: 1
- Review effort level: Balanced
| "the root `validate` no longer runs `verify:format-coverage` (a sibling guard). Restore it.", | ||
| ); | ||
| } | ||
| if (!reachableScripts(rootScripts, "local:gate").has("verify:skills:cli")) { |
The `local:gate` half of the wiring vouch used `reachableScripts`, which extracts any `npm run …` substring — so `echo npm run verify:skills:cli && npm run coverage` satisfied it while the validator never ran. Same over-match the CI half had in round 15, from the other side. `scriptChainRuns` walks the script graph instead, splitting each body on the separators that begin a new command and requiring an exact invocation of the target. A segment naming some other script is followed with or without trailing flags, since `npm run coverage --silent` still runs it, and `seen` makes a cyclic graph terminate. Regression cases for the mention, a longer script name, and the cycle; verified by mutation. Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 29, 2026
Copilot review round 19 — responsesFixed in b91b746.
Regression cases cover the mention, the longer name, the absent case, and the cycle, plus a
|
There was a problem hiding this comment.
🟡 Changes recommended
The gate-integrity checks can report success when validation is skipped or its failures are masked.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
scripts/verify-skills.mjs:182
- This accepts a step even when its failure cannot gate CI. For example,
run: npm run verify:skills:cli || truepassesrunsCommand, and a step withcontinue-on-error: truealso passes this loop, but either lets rejected skills leave the workflow green. Reject failure-masking shell forms andcontinue-on-errorso the asserted authoritative gate remains mandatory.
- Files reviewed: 35/38 changed files
- Comments generated: 2
- Review effort level: Balanced
| for (const sibling of [ | ||
| "verify:typecheck-coverage", | ||
| "verify:dep-lockstep", | ||
| "verify:skills", |
| for (const segment of body.split(/[\n;|&]+/).map((s) => s.trim())) { | ||
| if (segment === `npm run ${target}`) return true; | ||
| const tokens = segment.split(/\s+/); | ||
| if (tokens[0] === "npm" && tokens[1] === "run" && tokens[2]) { | ||
| queue.push(tokens[2]); |
All three findings are the same shape: a check that accepts a command which
exists but cannot gate.
- Splitting on `|` and `&` conflated `||`, so both failure-masking forms counted:
`npm run X || true` swallows a rejection and `true || npm run X` never runs
the validator. Both `runsCommand` and the chain walk now split on `\n`, `;`
and `&&` only, which refuses each without special-casing either.
- A CI step (or job) carrying `continue-on-error` asserts nothing, since a
rejected skill leaves the workflow green. Both are now skipped, treating a
string `"true"` and an unevaluable `${{ }}` expression as masking on the same
conservative principle as the `if:` checks; an explicit `false` still counts.
- `rootReachesScript` was substring-based, so a `validate` reading `echo npm run
verify:skills` satisfied the sibling vouch while the guard never executed —
the cycle protecting nothing. It now uses the exact chain walk, which is
therefore shared in `lib/npm-scripts.mjs` and fixes the same hole in all four
vouches, not just this PR's. `reachableScripts` keeps its substring behavior
for the harvesting callers, where over-inclusion is safe.
The chain walk also follows npm's implicit pre/post hooks, with a test — a guard
invoked from a `prevalidate` is genuinely run, and reporting it missing would be
a false failure introduced by the fix.
Signed-off-by: cliffhall <cliff@futurescale.com>cliffhall
commented
Aug 29, 2026
Copilot review round 20 — responsesAll three fixed in a2ddece. All three are the same shape — a check that accepts a command which exists but cannot gate — so they are worth reading together.
Both the shell matcher and the script-chain walk now split on Suppressed,
Rather than narrow one call site, the exact walk moved into Two things I added while there, both to avoid trading one false result for another: the walk follows npm's implicit pre/post hooks, since a guard invoked from a |
cliffhall
commented
Aug 29, 2026
Review loop statusCopilot's 21st review came back CI is green on the current head:
What the review changedThe 20 rounds produced ~35 findings. The ones that changed behavior rather than prose:
Two are worth flagging because they were my errors that a later round caught: round 2's "reject nonzero exits" broke the eval outright (a firing skill hits Ready for maintainer review. |
Closes#2163
AGENTS.mdwas a single 1,223-line contract loaded in full on every turn — the issue/board workflow, the priority rubric, the release procedure, the Mantine conventions — whether the task was a one-line CSS fix or a release cut. This splits it along the line the issue drew: rules stay resident inAGENTS.md; procedures move to.claude/skills/.Nothing is duplicated. Every recipe that moved was removed from where it was.
The skills
Ten, under
.claude/skills/. Seven aredisable-model-invocation: true— procedures with side effects, invoked by name — so there is no trigger question for them at all and their descriptions never enter the listing budget.issue-create/issue-createissue-triage/issue-triageboard-opsgh projectrecipes + the field/option IDs for #28 and #11, the option-deletion hazard/board-opspr-flow/pr-flowpre-push-gatelocal:gateand diagnosing a failing stage/pre-push-gaterelease/releasetest-servers/test-serverslocal-devtestingproject-structureuser-invocable: falseWhat shrank
AGENTS.mdREADME.mdREADME.mdis split intodocs/{architecture,quality-gate,test-servers,publishing,docker}.mdand is no longer auto-loaded byCLAUDE.md. The project tree inAGENTS.mdis one line per entry — the thirty-line commentary onapp-origin-controller.tsand friends is gone, with a note that those files carry their own header comments and the source is the place to read them..github/copilot-instructions.mdand the rule to mirror review-relevant changes into it are deleted. Copilot readsAGENTS.md.Reliability
The issue's central concern:
AGENTS.mdis unconditional, a skill is not, and a skill that stops firing loses behavior silently. Four things address that.npm run verify:skillsruns insidevalidate, so it is inlocal:gateand in GitHub CI. It parses eachSKILL.mdthe way Claude Code does and fails on anything that would strip the metadata — the whole-class failure being malformed frontmatter YAML, which loads the body with an empty description, so/skill-namestill works and a manual spot check passes while the skill can never auto-fire again. It also requires an explicit invocation mode on every skill, requires eval cases (positives and negatives) for model-invoked ones, and budgets the listing. It runsclaude plugin validatetoo when that CLI is on PATH — but the check does not depend on it, because the CLI is absent in CI and a guard that skips itself wherever it actually runs is worse than one that reimplements the check. Its pure parsers are unit-tested viatest:scripts(13 cases).That guard earned its keep during this PR: tuning the
testingdescription introduceddescription: Testing procedures for this repo: which npm command…, and the unquoted colon was caught immediately.Listing budget: 1,173 / 4,000 characters, printed on every run and failed when exceeded. I used a computed measure rather than a
/doctorreading because it is enforceable and regresses visibly when a skill is added; the acceptance criterion asked for the number to be recorded, and it is now recorded somewhere that checks itself.npm run skills:evalruns the committed trigger cases headless (claude -p … --output-format stream-json), N samples per case, asserting on aSkilltool_use. It is deliberately not in the gate — it needs the CLI and real model calls.Two measured findings, both recorded in AGENTS.md
pathsis not a free win. The issue recommended it fortestingas "far closer to deterministic than description matching". It does gate loading to matching files — but measured against that skill's own cases, adding it roughly halved the rate at which the same skill fired from a conversational prompt (0–50% withpaths, 33–100% without). It also cannot be measured at all: a prompt-only eval can never exercise a path trigger, so shipping it means shipping an untestable claim. I dropped it and wrote down why, so it does not get helpfully re-added.A case whose answer is already in
AGENTS.mdis not a trigger case. Two earlylocal-devcases sat at 0% because the model answered them correctly and directly from the resident rules — which is the design working, not a miss. Likewise a prompt naming a concrete file ("how does the@inspector/corealias resolve?") invites aRead, which is a better answer than a skill. Good cases are "how do I / where does this go" procedure questions.Current hit rates at
RUNS=3:project-structure5/5,local-dev5/6,testing3/5, negatives 100% across the board. The two below threshold are borderline-by-nature rather than broken, and are committed as-is so the next tuning pass has them.Gate
npm run local:gatepasses end to end (GATE_EXIT=0), including the Firefox engine pass and Storybook.npm run test:scriptsis 305/305. All relative Markdown links across the repo were verified to resolve.Deliberate calls worth reviewing
.gitignorenow un-ignores.claude/skills/(.claude/*+!.claude/skills/), so the skills ship with the checkout while Claude Code's local state stays out.AGENTS.mdas six bullets, with the reasoning inlocal-dev. The issue's table assigned them to the skill, but they are exactly what a reviewer cites against a diff, and the issue's own constraint says rules stay resident. The skill carries the war stories.npm run verify:skills:cliis a step inlocal:gateand in CI. It uses an installedclaudeonly when it matchesPINNED_CLI_VERSIONexactly, and otherwise runsnpx -y @anthropic-ai/claude-code@2.1.250— so the gate can reach the network, and CI installs that package on every push. That is a deliberate supply-chain trade for making "authoritative validation runs in CI" true rather than aspirational; the pin is what bounds it, and the version is a one-line change.verify:skills(insidevalidate) stays offline and best-effort, so the fast inner loop needs no CLI and no network.