fix: address Copilot review round 6
- `reachableScripts` now follows every npm spelling of `run` — the canonical `run-script` and its `run`/`rum`/`urn` aliases — with a unit case, so `npm run-script validate:web` appended to `local:gate` fails the whole-gate check instead of slipping past it. - docs/quality-gate.md `verify:skills` row, AGENTS.md skills rule 1 and the #2085 narration now describe the actual chain (`validate:guards`, reached by both `validate` and `local:validate`) and the qualified promise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
9cff77e6d9
commit
61fcada059
4 changed files with 28 additions and 6 deletions
|
|
@ -217,8 +217,8 @@ Skills are conditional — a skill's body loads only when it is invoked — so a
|
|||
skill that stops being reachable loses behavior **silently**. Four rules keep
|
||||
that from happening:
|
||||
|
||||
1. **`npm run verify:skills` must pass.** It runs inside `validate` (and so in
|
||||
`local:gate` and in CI). It parses each `SKILL.md`'s frontmatter the way Claude
|
||||
1. **`npm run verify:skills` must pass.** It runs inside `validate:guards`,
|
||||
which `validate` (CI) and `local:validate` (the gate) both run first. It parses each `SKILL.md`'s frontmatter the way Claude
|
||||
Code does and fails on anything that would strip the metadata — most importantly
|
||||
**malformed YAML**, which loads the body with an _empty_ description, so
|
||||
`/skill-name` still works and a manual spot check passes while the skill can
|
||||
|
|
@ -509,7 +509,7 @@ The two coverage guards do **not** catch this, and adding a third is not the fix
|
|||
|
||||
**Every `lint` script runs with `--max-warnings 0`, so a warning fails `validate` exactly as an error does (#2085).** All six scopes carry the flag — each of `clients/{web,cli,tui,launcher}`'s `eslint .`, plus the root's `lint:core` and `lint:shared`.
|
||||
|
||||
This exists because the gate's promise — that passing `npm run local:gate` locally means CI's gates pass — was kept while a real bug walked through it. `react-hooks/exhaustive-deps` ships at `warn` in the recommended set, and two `useCallback`s in `App.tsx` omitted a non-stable `refresh` from their dependency arrays; ESLint printed the right message on both lines on every run, nothing consumed it, and the stale closure was caught only by a review round on #2076. It is the same argument [Build output is never a gate target](#build-output-is-never-a-gate-target) makes from the other direction: a channel nobody fails on is one people learn to skim.
|
||||
This exists because the gate's promise — that a green `npm run local:gate` means every check CI applies has already passed — was kept while a real bug walked through it. `react-hooks/exhaustive-deps` ships at `warn` in the recommended set, and two `useCallback`s in `App.tsx` omitted a non-stable `refresh` from their dependency arrays; ESLint printed the right message on both lines on every run, nothing consumed it, and the stale closure was caught only by a review round on #2076. It is the same argument [Build output is never a gate target](#build-output-is-never-a-gate-target) makes from the other direction: a channel nobody fails on is one people learn to skim.
|
||||
|
||||
Two consequences worth stating:
|
||||
|
||||
|
|
|
|||
|
|
@ -37,7 +37,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor
|
|||
| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. |
|
||||
| `npm run verify:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// <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 the installed CLI is **exactly** the pinned version, and otherwise says so and moves on. It runs inside `validate`, which `local:gate` executes *before* the pinned step, so accepting any other local CLI would let it reject skills that CI accepts and exit the gate before the reproducible check ever ran. Runs in `validate`; because that hand-off is best-effort, the authoritative validator also gets a guaranteed step of its own — see `verify:skills:cli` below. |
|
||||
| `npm run verify:skills` | Guards `.claude/skills` ([#2163](https://github.com/modelcontextprotocol/inspector/issues/2163)). A skill's body loads only when it is invoked, so one that stops being reachable loses behavior **silently** — and the way that happens is frontmatter Claude Code cannot parse, which loads the body with an *empty* description: `/skill-name` still works, so a manual spot check passes, while nothing is left for the model to match against. This parses each `SKILL.md` the same way, requires an explicit invocation mode on every skill and committed eval cases (positives **and** negatives) on every model-invoked one, and fails when the skill listing exceeds its recorded character budget. Also runs `claude plugin validate` — the authoritative schema — when the installed CLI is **exactly** the pinned version, and otherwise says so and moves on. It runs inside `validate:guards`, which both `validate` (CI) and `local:validate` (the gate's first stage) execute *before* the pinned step, so accepting any other local CLI would let it reject skills that CI accepts and exit the gate before the reproducible check ever ran. Runs in `validate`; because that hand-off is best-effort, the authoritative validator also gets a guaranteed step of its own — see `verify:skills:cli` below. |
|
||||
| `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. Needs no authentication. Not in `validate` — it can reach the network. |
|
||||
| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. 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`. |
|
||||
|
|
|
|||
|
|
@ -25,14 +25,17 @@ export function tokenize(command) {
|
|||
* <name>` references within a single manifest's `scripts`, plus npm's implicit
|
||||
* `pre<name>`/`post<name>` lifecycle hooks (npm runs those around `<name>`
|
||||
* without an explicit `npm run`, so a gate moved into e.g. `prevalidate` is
|
||||
* still reached). A gate harvested from a script that nothing reachable from
|
||||
* still reached). Every spelling npm accepts for `run` is followed —
|
||||
* `run-script` is the canonical command and `run`, `rum` and `urn` are its
|
||||
* aliases — so a chain written as `npm run-script validate:web` reaches the
|
||||
* same scripts as `npm run validate:web` (Copilot, #2341). A gate harvested from a script that nothing reachable from
|
||||
* `entry` invokes gates nothing, so callers restrict to this set to assert "CI
|
||||
* actually runs this", not merely "the script exists".
|
||||
*/
|
||||
export function reachableScripts(scripts, entry = "validate") {
|
||||
const reached = new Set();
|
||||
const queue = [entry];
|
||||
const runRef = /npm run ([\w:-]+)/g;
|
||||
const runRef = /npm (?:run-script|run|rum|urn) ([\w:-]+)/g;
|
||||
while (queue.length > 0) {
|
||||
const name = queue.shift();
|
||||
if (reached.has(name)) continue;
|
||||
|
|
|
|||
|
|
@ -48,6 +48,25 @@ test("reachableScripts: follows `npm run` refs and pre/post hooks (r10)", () =>
|
|||
assert.ok(!reached.has("unrelated"));
|
||||
});
|
||||
|
||||
test("reachableScripts: follows every npm spelling of `run` (#2341)", () => {
|
||||
// `run-script` is the canonical command; `run`, `rum` and `urn` are its
|
||||
// aliases. A chain written in any of them reaches the same scripts, so a
|
||||
// guard that only followed `npm run` would let `npm run-script validate:web`
|
||||
// reintroduce what it exists to forbid (Copilot).
|
||||
const scripts = {
|
||||
gate: "npm run-script a && npm rum b && npm urn c && npm run d",
|
||||
a: "noop",
|
||||
b: "noop",
|
||||
c: "noop",
|
||||
d: "noop",
|
||||
e: "noop",
|
||||
};
|
||||
const reached = reachableScripts(scripts, "gate");
|
||||
for (const name of ["a", "b", "c", "d"])
|
||||
assert.ok(reached.has(name), `npm spelling reaching ${name}`);
|
||||
assert.ok(!reached.has("e"));
|
||||
});
|
||||
|
||||
test("reachableScripts: a `prevalidate`-hosted typecheck is reachable (r10)", () => {
|
||||
const scripts = {
|
||||
validate: "npm run build && npm run test",
|
||||
|
|
|
|||
Loading…
Reference in a new issue