chore(scripts): treat an expression-built script name as forbidden too (#2146)
Copilot, first review:
- The guard read only literal names, so `npm run smoke:web:${{ matrix.browser }}`
and `npm run local:${{ matrix.task }}` walked past it — the one form in which
a forbidden engine reaches CI without anyone typing its name. It already
refused an expression-valued SMOKE_BROWSER for that exact reason; this applies
the reasoning consistently. Its limit is now stated: a fully opaque
`npm run ${{ matrix.script }}` names no family and is not detectable.
- The module header claimed the gate adds "the non-Chromium engine passes";
it adds Firefox only, and WebKit belongs to neither tier.
- eslint.config.js:33 still said `npm run ci` — the last stale reference.
- AGENTS.md read "everything from `smoke` rightward is `local:gate` only",
which is two claims wrong: smoke and Storybook run in GitHub CI too, and
`pack:verify` is in neither tier.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
0b64460097
commit
b18b975128
6 changed files with 51 additions and 13 deletions
2
.github/copilot-instructions.md
vendored
2
.github/copilot-instructions.md
vendored
|
|
@ -102,7 +102,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
|
|||
|
||||
## Gates and PR hygiene
|
||||
|
||||
- `npm run format` before committing; **`npm run local:gate` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`; the Firefox step is local-only and not mirrored in GitHub CI). The gate is named `local:` rather than `ci` on purpose and has **no alias** (#2146) — `npm run ci` is gone, and a workflow that invokes a `local:*` script, a non-Chromium engine pass, `smoke:web:engine`, or sets `SMOKE_BROWSER` to a non-Chromium value fails `test:scripts` via `scripts/lib/workflow-gate.mjs`. Flag any diff that adds one of those to `.github/workflows/**`, or that reintroduces a `ci`/`ci:*` root script. `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. The canonical CI-vs-local table is in the root README ("Two tiers: GitHub CI and the local gate").
|
||||
- `npm run format` before committing; **`npm run local:gate` before pushing** (`validate` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`; the Firefox step is local-only and not mirrored in GitHub CI). The gate is named `local:` rather than `ci` on purpose and has **no alias** (#2146) — `npm run ci` is gone, and a workflow that invokes a `local:*` script, a non-Chromium engine pass, `smoke:web:engine`, or sets `SMOKE_BROWSER` to a non-Chromium value fails `test:scripts` via `scripts/lib/workflow-gate.mjs` — as does naming either family through a workflow expression (`smoke:web:${{ matrix.browser }}`). Flag any diff that adds one of those to `.github/workflows/**`, or that reintroduces a `ci`/`ci:*` root script. `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. The canonical CI-vs-local table is in the root README ("Two tiers: GitHub CI and the local gate").
|
||||
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
|
||||
- **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`; `pack:verify`'s `--web` child sets its own `MCP_CATALOG_PATH` for the same reason (#2003 — its App deep link persists a server row). Anything that boots the web backend and then *navigates* it needs that isolation, not just the scripts named `smoke:*`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged.
|
||||
- **A smoke must assert the thing survives, not that it started.** `smoke:tui` runs the Ink TUI under a pseudoterminal (`scripts/lib/pty.mjs`) and requires it to still be running `SMOKE_TUI_SURVIVE_MS` after its first frame (`scripts/lib/render-smoke.mjs`) — because it previously settled OK on first paint while the TUI was exiting 1 ~40ms later with `Raw mode is not supported`, and won that race on every machine (#2147). Flag anything that collapses the survival wait back into "resolve on the marker", and any new smoke whose only assertion is that a process printed something once. A smoke walks only its own happy path, so the branch that matters — started, then died — belongs in a `scripts/lib/*.test.mjs` driven against a stub (as with `announced-child.mjs` #2000 and `ensure-test-servers.mjs` #2111), not in the smoke.
|
||||
|
|
|
|||
|
|
@ -929,7 +929,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
|
|||
- Run TUI tests with `npm run test` from `clients/tui/`
|
||||
- Run launcher tests with `npm run test` from `clients/launcher/`
|
||||
- Run the root tooling's own tests with `npm run test:scripts` from the root (this is also what runs the #2146 workflow guard against `.github/workflows/**`) — `node --test "scripts/**/*.test.mjs"`, node's built-in runner (the root has no vitest harness by design). A new `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`; keep the filename `*.test.mjs`, since `node --test` silently **skips** a file its glob misses and still exits 0 (`verify:typecheck-coverage` guards against exactly that).
|
||||
- **The test tiers, shallowest first:** unit (`test`, per client) → web integration (`test:integration`, real transports/servers) → out-of-process (`clients/cli/__tests__/e2e.test.ts`, spawns the built binary) → smokes through the built launcher (`npm run smoke`) → Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts` (it's part of cli's `test`), but **not** web's integration project, which runs inside the `coverage` gate. Everything from `smoke` rightward is `npm run local:gate` only, and is described under [Mandatory pre-push gate](#mandatory-pre-push-gate).
|
||||
- **The test tiers, shallowest first:** unit (`test`, per client) → web integration (`test:integration`, real transports/servers) → out-of-process (`clients/cli/__tests__/e2e.test.ts`, spawns the built binary) → smokes through the built launcher (`npm run smoke`) → Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts` (it's part of cli's `test`), but **not** web's integration project, which runs inside the `coverage` gate. Everything from `smoke` rightward is outside `validate` and is described under [Mandatory pre-push gate](#mandatory-pre-push-gate) — note that being outside `validate` is not the same as being local-only: `smoke` and the Storybook tests run in GitHub CI too, `pack:verify` is in neither tier (local/release only), and the genuinely gate-only steps are the two named in the [README's two-tier table](./README.md#two-tiers-github-ci-and-the-local-gate).
|
||||
- The repo root has no aggregate `test` script — each client self-validates, so run `npm run validate` from the root (all clients, fast) or `cd clients/<name> && npm run validate` (one client). Each client still exposes its own `test` / `test:coverage` for quick iteration.
|
||||
- **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want to reproduce the gate locally before pushing. **CI runs `coverage`** on every push (#1550): the per-file ≥90 gate is CI-enforced, so a PR that drops any file below 90 on lines/statements/functions/branches fails the job. It runs in its **own job, in parallel with `build`** (#2159) — `coverage` consumes nothing `validate` produces, since each client's `test:coverage` builds whatever it needs itself, and running the two serially was 84% of a ~17m wall clock. Both publish jobs therefore gate on `needs: [build, coverage]`; leaving either off would let a release publish with the coverage gate red. Because web's `test:coverage` already runs the integration project, CI has no separate `test:integration` step — the integration paths are exercised inside the coverage gate.
|
||||
- Each client's `test:coverage` enforces a **uniform per-file gate of ≥ 90 on all four dimensions** — lines, statements, functions, and branches — across `clients/web`, `clients/cli`, `clients/tui`, and `clients/launcher` (CI enforces this gate). This is the result of a codebase-wide audit: the branch floor was first lifted 50 → 70 for web (#1271), then the whole gate raised to 90 with real tests added for every outlier. Genuinely-unreachable branches are **not** waved through by lowering the gate — they are annotated at the source with a justified `/* v8 ignore … -- <reason> */` comment. Acceptable reasons are happy-dom-inherent paths (Mantine portal mount points, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards (e.g. a `?? fallback` for a value the types guarantee non-null, or a `Select.onChange` receiving a value outside the allowed list). New code must clear 90 on every dimension; reach for a justified `v8 ignore` only when a branch is genuinely impossible to exercise. The web coverage `include` (in `clients/web/vite.config.ts`) covers the shared `core/` runtime consumed by the browser — `core/mcp`, `core/react`, `core/auth`, `core/storage`, `core/logging`, `core/node`, **`core/json`, and `core/client`** (the last two folded in by #1689). When adding a `core/json/*` or `core/client/*` module, its tests live under `clients/web/src/test/core/…` and are gated the same ≥90 way.
|
||||
|
|
@ -1001,7 +1001,7 @@ Everything about the selection lives in **`scripts/lib/headless-browser.mjs`**
|
|||
- **An unrecognized `SMOKE_BROWSER` is an error, never a fallback.** Falling back to Chromium would report a green Chromium run under a job labelled `webkit` — coverage claimed but not run, which is worse than none.
|
||||
- **A launch failure names the engine that failed** and a remedy that works **from where the caller actually is**: `npm run smoke:web:<engine>`, or `npx playwright install --with-deps <engine>` run *from `clients/web`*. Naming `chromium` while WebKit was missing would send the reader to install a browser they already have — and naming a bare root-level `npx playwright` would send them to a version that is not the pinned one, which can install a browser revision the pinned Playwright still cannot launch.
|
||||
- **Firefox is gated locally, not in CI, and that asymmetry is deliberate.** A GitHub Actions job was trialled and removed. It was cheap — ~2 minutes, parallel with the 15-minute `build` job, so zero added wall-clock — but across a dozen runs Firefox never once disagreed with Chromium, so it spent runner minutes on every push, from every branch, carrying a real flake surface (`playwright install --with-deps` runs `apt-get update`, which fails whenever a third-party repo in the runner image breaks) to re-confirm a result already in hand. Moving it into `npm run local:gate` keeps the check where a human is about to push a change they can still reason about, and pays for it once rather than on every push. **Don't re-add the CI job on the argument that cross-engine coverage is good in principle** — that argument was accepted, and the pre-push gate is what serves it. Re-add it on evidence: a cross-engine regression that reached `v2/main` because someone skipped the gate.
|
||||
- **A guard enforces the split, so it cannot be undone by accident (#2146).** `scripts/lib/workflow-gate.mjs` is run by `npm run test:scripts` over every file under `.github/workflows/`, and fails the suite on: an invocation of a `local:*` script (the prefix *means* local-only, so a future sibling is covered without editing the guard); an invocation of a non-Chromium engine pass, derived from `SUPPORTED_BROWSERS` so a fourth engine extends the guard rather than opening a hole in it; an invocation of `smoke:web:engine`, whose engine comes from the environment and so cannot be read off the workflow at all; or `SMOKE_BROWSER` set to anything but a literal `chromium` — the back door that would redirect an otherwise innocent `npm run smoke`. It deliberately does **not** forbid `npm run smoke`, `smoke:web:chromium`, or `smoke:tui`: those belong in CI and are there today, and `smoke:tui` self-skips under `process.env.CI` on its own. Lines that are comments are not scanned — a comment invokes nothing, and a guard that made the workflow undocumentable would be traded for the wrong thing.
|
||||
- **A guard enforces the split, so it cannot be undone by accident (#2146).** `scripts/lib/workflow-gate.mjs` is run by `npm run test:scripts` over every file under `.github/workflows/`, and fails the suite on: an invocation of a `local:*` script (the prefix *means* local-only, so a future sibling is covered without editing the guard); an invocation of a non-Chromium engine pass, derived from `SUPPORTED_BROWSERS` so a fourth engine extends the guard rather than opening a hole in it; an invocation of `smoke:web:engine`, whose engine comes from the environment and so cannot be read off the workflow at all; or `SMOKE_BROWSER` set to anything but a literal `chromium` — the back door that would redirect an otherwise innocent `npm run smoke`. A name built from a workflow **expression** counts as forbidden too (`smoke:web:${{ matrix.browser }}`, `local:${{ matrix.task }}`): a matrix the guard cannot read may well contain `firefox`, so an unreadable name is not waved through. Its limit is worth knowing — a fully opaque `npm run ${{ matrix.script }}` names no family at all and is not detectable. It deliberately does **not** forbid `npm run smoke`, `smoke:web:chromium`, or `smoke:tui`: those belong in CI and are there today, and `smoke:tui` self-skips under `process.env.CI` on its own. Lines that are comments are not scanned — a comment invokes nothing, and a guard that made the workflow undocumentable would be traded for the wrong thing.
|
||||
- **This makes `npm run local:gate` a strict superset of GitHub CI rather than a mirror of it.** That was already the direction (`smoke:tui` self-skips on CI and runs only locally); Firefox is the second such step. The invariant that still holds, and the one that matters, is the useful direction: **passing `npm run local:gate` locally means CI's gates will pass.**
|
||||
- **`pack:verify` stays pinned to Chromium**, passing it explicitly rather than reading `SMOKE_BROWSER`. It is a _packaging_ check; the engine question belongs where the sandbox is under test, and pinning it also means it can't be pointed at an engine its npm script never installed.
|
||||
- **WebKit fails the two App smokes**, for reasons nobody has identified — the failure stage even varies between runs. The investigation was dropped as not worth the effort once it became clear it does not reproduce in real Safari, and that an isolated repro of the suspected mechanism did not reproduce it under Playwright's WebKit either. Treat a WebKit failure as unexplained rather than as a defect until someone has actually looked.
|
||||
|
|
|
|||
|
|
@ -502,7 +502,7 @@ So the direction that matters holds: **passing `npm run local:gate` means CI's g
|
|||
|
||||
**The gate is deliberately not named `ci`** ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)). That name collided with `npm ci` — a built-in that clean-installs from the lockfile, does **not** run this script, and is one keystroke away — and it invited the cross-engine passes into GitHub CI by looking like the thing a workflow ought to call. There is **no back-compat alias**: `npm run ci` now fails with npm's missing-script error, which prints the available scripts.
|
||||
|
||||
That is the readable half, and prose rots. The enforced half is `scripts/lib/workflow-gate.mjs`, run by `npm run test:scripts` over every file under `.github/workflows/`. It fails the suite if a workflow invokes a `local:*` script, a non-Chromium engine pass (`smoke:web:firefox`, `smoke:web:webkit`), or `smoke:web:engine` (whose engine comes from the environment and so cannot be read off the workflow at all), or sets `SMOKE_BROWSER` to anything but a literal `chromium`. It deliberately does **not** forbid `npm run smoke`, `smoke:web:chromium`, or `smoke:tui` — those belong in CI and are there today. Comments are not scanned, so the workflow can still explain itself.
|
||||
That is the readable half, and prose rots. The enforced half is `scripts/lib/workflow-gate.mjs`, run by `npm run test:scripts` over every file under `.github/workflows/`. It fails the suite if a workflow invokes a `local:*` script, a non-Chromium engine pass (`smoke:web:firefox`, `smoke:web:webkit`), or `smoke:web:engine` (whose engine comes from the environment and so cannot be read off the workflow at all), or sets `SMOKE_BROWSER` to anything but a literal `chromium`. It treats a name built from a workflow **expression** the same way — `smoke:web:${{ matrix.browser }}`, `local:${{ matrix.task }}` — since a matrix the guard cannot read may well contain `firefox`. It deliberately does **not** forbid `npm run smoke`, `smoke:web:chromium`, or `smoke:tui` — those belong in CI and are there today. Comments are not scanned, so the workflow can still explain itself.
|
||||
|
||||
| Script | What it does |
|
||||
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
|
|
|
|||
|
|
@ -30,7 +30,7 @@ const typeAware = {
|
|||
// an awaited one minus four characters, and the unhandled rejection it
|
||||
// produces surfaces in a different test, in a different file, as a stack
|
||||
// pointing at SDK internals — which is how #1947 came to fail the whole
|
||||
// `npm run ci` chain from two un-held `callTool` promises.
|
||||
// `npm run local:gate` chain from two un-held `callTool` promises.
|
||||
"@typescript-eslint/no-floating-promises": "error",
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -8,11 +8,11 @@
|
|||
* `smoke:launcher`, `smoke:cli`, `smoke:tui`, `smoke:web` and
|
||||
* `smoke:web:chromium` — all of which BELONG there.
|
||||
* - **`npm run local:gate`**, the pre-push gate, is a strict superset: it adds
|
||||
* the non-Chromium engine passes, and `smoke:tui` really runs there rather
|
||||
* than self-skipping.
|
||||
* the **Firefox** engine pass, and `smoke:tui` really runs there rather than
|
||||
* self-skipping. WebKit is on demand and belongs to neither tier.
|
||||
*
|
||||
* The narrow thing that must never reach CI is the **non-Chromium engine
|
||||
* passes**. `smoke:web:firefox` was placed in the local gate on purpose (#2086,
|
||||
* The narrow thing that must never reach CI is a **non-Chromium engine pass**.
|
||||
* `smoke:web:firefox` was placed in the local gate on purpose (#2086,
|
||||
* #2133) after a CI job was trialled and removed for never once disagreeing
|
||||
* with Chromium, and `smoke:web:webkit` fails two of the three smokes outright
|
||||
* for reasons nobody has identified. Confidence in the cross-engine runs is not
|
||||
|
|
@ -33,9 +33,16 @@
|
|||
* 3. Invoking `smoke:web:engine`, whose engine comes from the environment and
|
||||
* therefore cannot be read off the workflow at all.
|
||||
* 4. Setting `SMOKE_BROWSER` to anything but a literal `chromium` — the back
|
||||
* door that would redirect an otherwise innocent `npm run smoke`. An
|
||||
* expression (`${{ matrix.browser }}`) is flagged for the same reason as
|
||||
* `smoke:web:engine`: the guard cannot tell what it resolves to.
|
||||
* door that would redirect an otherwise innocent `npm run smoke`.
|
||||
* 5. Naming either family through a workflow **expression** —
|
||||
* `npm run smoke:web:${{ matrix.browser }}`, `npm run local:${{ … }}`, or
|
||||
* `SMOKE_BROWSER: ${{ matrix.browser }}`. This is rule 3's reason applied
|
||||
* consistently: a matrix whose values the guard cannot read is a matrix
|
||||
* that may well contain `firefox`, so an unreadable name is treated as a
|
||||
* forbidden one rather than waved through (Copilot). Note the limit — a
|
||||
* fully opaque `npm run ${{ matrix.script }}` names no family at all and
|
||||
* is not detectable here; the guard covers the constructions that spell
|
||||
* out a forbidden one.
|
||||
*
|
||||
* WHAT IT MUST NOT FORBID: `npm run smoke`, `smoke:web:chromium`, `smoke:tui`.
|
||||
* Those belong in CI and are there today — `smoke:tui` self-skips under
|
||||
|
|
@ -71,7 +78,13 @@ export const VIOLATION = {
|
|||
BROWSER_ENV: "smoke-browser-override",
|
||||
};
|
||||
|
||||
const LOCAL_SCRIPT_RE = /\blocal:[a-z0-9][a-z0-9:-]*/gi;
|
||||
// The name may be spelled out (`local:gate`) or built from an expression
|
||||
// (`local:${{ matrix.task }}`) — both are `local:` scripts, and neither may be
|
||||
// invoked from a workflow.
|
||||
const LOCAL_SCRIPT_RE = /\blocal:(?:[a-z0-9][a-z0-9:-]*|\$\{\{[^}]*\}\})/gi;
|
||||
// Same reasoning one level down: `smoke:web:` followed by an expression could
|
||||
// resolve to any engine, so it is read as a forbidden one.
|
||||
const DYNAMIC_ENGINE_RE = /\bsmoke:web:\$\{\{[^}]*\}\}/gi;
|
||||
// The value may be quoted (`SMOKE_BROWSER: "firefox"`), assigned
|
||||
// (`SMOKE_BROWSER=firefox`), or an expression — capture whatever follows.
|
||||
const BROWSER_ENV_RE = /\bSMOKE_BROWSER\s*[:=]\s*(\S+)/gi;
|
||||
|
|
@ -134,6 +147,19 @@ export function findWorkflowViolations(text, file = "<workflow>") {
|
|||
}
|
||||
}
|
||||
|
||||
for (const match of line.matchAll(DYNAMIC_ENGINE_RE)) {
|
||||
findings.push({
|
||||
file,
|
||||
line: lineNumber,
|
||||
rule: VIOLATION.ENGINE_SCRIPT,
|
||||
match: match[0],
|
||||
message:
|
||||
`\`${match[0]}\` builds an engine-pass name from an expression, so the guard cannot ` +
|
||||
`tell which engine it runs — and a matrix that a workflow can vary may well contain ` +
|
||||
`\`firefox\`. Name \`smoke:web:${DEFAULT_BROWSER}\` outright, or just run \`npm run smoke\`.`,
|
||||
});
|
||||
}
|
||||
|
||||
for (const match of line.matchAll(BROWSER_ENV_RE)) {
|
||||
const value = stripQuotes(match[1]);
|
||||
if (value === DEFAULT_BROWSER) continue;
|
||||
|
|
|
|||
|
|
@ -53,6 +53,18 @@ describe("findWorkflowViolations", () => {
|
|||
text: " - run: npm run smoke:web:engine\n",
|
||||
rules: [VIOLATION.ENGINE_SCRIPT],
|
||||
},
|
||||
{
|
||||
// Copilot, first review: the matrix form is exactly how a forbidden
|
||||
// engine reaches CI without anyone typing its name.
|
||||
name: "flags an engine pass whose name is built from an expression",
|
||||
text: " - run: npm run smoke:web:${{ matrix.browser }}\n",
|
||||
rules: [VIOLATION.ENGINE_SCRIPT],
|
||||
},
|
||||
{
|
||||
name: "flags a local: script whose name is built from an expression",
|
||||
text: " - run: npm run local:${{ matrix.task }}\n",
|
||||
rules: [VIOLATION.LOCAL_SCRIPT],
|
||||
},
|
||||
{
|
||||
name: "flags SMOKE_BROWSER pointed at another engine",
|
||||
text: " env:\n SMOKE_BROWSER: firefox\n",
|
||||
|
|
|
|||
Loading…
Reference in a new issue