modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

chore(scripts): make the browser rule kind-aware, cover container.env, pin the name (#2146)

Browse source 
Copilot, fifth review:

- The SMOKE_BROWSER rule was a blanket text match across every region, so a
  script that merely PRINTS the variable — `echo "SMOKE_BROWSER: firefox"` —
  or a `with:` input mentioning it was reported as an override. It is
  kind-aware now: an `env:` entry counts only when SMOKE_BROWSER is literally
  its key (recognized structurally, not by regex), a `run:` script only on a
  shell assignment, and a `with:` value not at all — whatever it names is
  already covered by the script rules. A shell assignment inside an `echo` is
  indistinguishable without parsing the shell, and inside `run:` erring toward
  the finding is the safe direction; that is stated in the header.

- `jobs.<id>.container.env` is job-wide too — every step in the container sees
  it — so an override there redirected `npm run smoke` with the guard silent.
  Now traversed, and a `container:` given as a bare image string is ignored
  rather than tripping over.

- Nothing asserted the FIRST acceptance criterion: reintroducing a `ci` or
  `ci:*` root script left test:scripts green. Asserted directly over the
  manifest's keys now, and checked to actually bite by reviving `ci` and
  watching the suite fail.

Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-27 01:25:51 -04:00
commit 6d2806de75
4 changed files with 125 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

2
AGENTS.md
View file

@ -1003,7 +1003,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`, and one that includes setting it to *nothing*, since present-and-empty is rejected rather than defaulted. 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. It **parses** the workflow (via the root-declared `yaml` package) and descends the schema's executable paths — workflow `env`, job `env`/`with`, step `run`/`env`/`with`, with YAML aliases resolved — rather than matching pairs by key name anywhere in the tree, which would scan a workflow input legitimately named `env` and fail the gate on documentation of the gate. So it reads only what executes — so ordinary metadata such as a step `name:` explaining the rule is not reported as an invocation of it (Copilot); comments are skipped for the same reason, and because a guard that made the workflow undocumentable would be traded for the wrong thing. That scoping is a stated limit rather than a hidden one: an execution vector outside those three positions is not seen.
- **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`, and one that includes setting it to *nothing*, since present-and-empty is rejected rather than defaulted, and `container.env` as much as a job's own `env`. That rule is kind-aware — an `env:` key, or a shell assignment in a `run:` — so a script printing the variable's name is not read as setting it. 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. It **parses** the workflow (via the root-declared `yaml` package) and descends the schema's executable paths — workflow `env`, job `env`/`container.env`/`with`, step `run`/`env`/`with`, with YAML aliases resolved — rather than matching pairs by key name anywhere in the tree, which would scan a workflow input legitimately named `env` and fail the gate on documentation of the gate. So it reads only what executes — so ordinary metadata such as a step `name:` explaining the rule is not reported as an invocation of it (Copilot); comments are skipped for the same reason, and because a guard that made the workflow undocumentable would be traded for the wrong thing. That scoping is a stated limit rather than a hidden one: an execution vector outside those three positions is not seen.
- **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.

2
README.md
View file

@ -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` (including setting it to *nothing*, which is present-and-empty rather than unset, and is rejected rather than defaulted). 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. It **parses** the workflow and descends the schema's executable paths — workflow `env`, job `env`/`with`, step `run`/`env`/`with`, with aliases resolved — rather than matching text or key names, so it reads only what actually executes — so a step *name* explaining the rule is not mistaken for an invocation of it, and the workflow can still document 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` (including setting it to *nothing*, which is present-and-empty rather than unset, and is rejected rather than defaulted). That last rule respects **where** it is written — an `env:` entry whose key is literally `SMOKE_BROWSER`, or a shell assignment in a `run:` script — so a script that merely *prints* the variable's name is not read as setting it. 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. It **parses** the workflow and descends the schema's executable paths — workflow `env`, job `env`/`container.env`/`with`, step `run`/`env`/`with`, with aliases resolved — rather than matching text or key names, so it reads only what actually executes — so a step *name* explaining the rule is not mistaken for an invocation of it, and the workflow can still document itself.
| Script | What it does |
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |

69
scripts/lib/workflow-gate.mjs
View file

@ -33,7 +33,14 @@
* 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`.
* door that would redirect an otherwise innocent `npm run smoke`. This one
* is KIND-AWARE: an `env:` entry counts only when `SMOKE_BROWSER` is
* literally its key, and a `run:` script only on a shell assignment
* (`SMOKE_BROWSER=…`). A blanket text match rejected `echo
* "SMOKE_BROWSER: firefox"`, which sets nothing (Copilot). A shell
* assignment inside an `echo` is indistinguishable without parsing the
* shell, and inside a `run:` block erring toward the finding is the safe
* direction.
* 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
@ -74,7 +81,11 @@ import {
isSeq,
parseAllDocuments,
} from "yaml";
import { SUPPORTED_BROWSERS, DEFAULT_BROWSER } from "./headless-browser.mjs";
import {
BROWSER_ENV_VAR,
SUPPORTED_BROWSERS,
DEFAULT_BROWSER,
} from "./headless-browser.mjs";
/** Scripts whose `local:` prefix declares them local-only. */
export const LOCAL_SCRIPT_PREFIX = "local:";
@ -118,12 +129,13 @@ const DYNAMIC_ENGINE_RE = new RegExp(
String.raw`\bsmoke:web:${EXPRESSION}`,
"gi",
);
// The value may be quoted (`SMOKE_BROWSER: "firefox"`), assigned
// (`SMOKE_BROWSER=firefox`), an expression, or ABSENT — `SMOKE_BROWSER:` with
// no value is valid YAML that sets the variable to the empty string, which
// `resolveBrowserName` rejects as present-but-empty rather than treating as
// unset (Copilot). So the value is `\S*`, and an empty capture is a finding.
const BROWSER_ENV_RE = /\bSMOKE_BROWSER\s*[:=]\s*(\S*)/gi;
// A shell assignment inside a `run:` script. The YAML `env:` spelling is not
// matched by text at all — it is recognized structurally, by key, so that a
// script merely PRINTING `SMOKE_BROWSER: firefox` is not read as setting it
// (Copilot). `\S*` rather than `\S+` because `SMOKE_BROWSER=` sets the empty
// string, which `resolveBrowserName` rejects as present-but-empty rather than
// treating as unset.
const BROWSER_SHELL_RE = new RegExp(`\\b${BROWSER_ENV_VAR}=(\\S*)`, "g");
// A shell assignment inside a `run:` script can still be quoted; a YAML value
// arrives already unquoted from the parser, so this is a no-op there.
@ -151,7 +163,8 @@ function stripQuotes(value) {
* allowlist, so it changes nothing about the tarball.
*
* The walk is STRUCTURAL — it descends the schema's executable paths
* (workflow `env`, job `env`/`with`, step `run`/`env`/`with`) rather than
* (workflow `env`, job `env`/`container.env`/`with`, step `run`/`env`/`with`)
* rather than
* matching pairs by key name anywhere in the tree. Key-name matching looks
* equivalent and is not: workflow input ids are user-defined, so an input
* legitimately named `env` puts its own `description` and `default` in front of
@ -221,6 +234,8 @@ export function extractExecutableRegions(text, file = "<workflow>") {
regions.push({
line: lineOf(entry.key),
kind,
name: String(entry.key.value),
value: text,
text: `${String(entry.key.value)}: ${text}`,
});
}
@ -240,6 +255,11 @@ export function extractExecutableRegions(text, file = "<workflow>") {
addMapping("env", at(job, "env"));
// A job-level `with:` is the input block of a reusable-workflow call.
addMapping("with", at(job, "with"));
// `container.env` is job-wide too: every step running in the container
// sees it, so an override there redirects `npm run smoke` exactly as a
// job-level `env` would (Copilot). `container:` may also be a bare image
// string, which `addMapping` ignores.
addMapping("env", at(deref(at(job, "container")), "env"));
const steps = deref(at(job, "steps"));
if (!isSeq(steps)) continue;
@ -269,14 +289,12 @@ export function extractExecutableRegions(text, file = "<workflow>") {
export function findWorkflowViolations(text, file = "<workflow>") {
const findings = [];
for (const { line: lineNumber, text: region } of extractExecutableRegions(
text,
file,
)) {
for (const region of extractExecutableRegions(text, file)) {
const { line: lineNumber, kind } = region;
// Inside a `run:` block the parser hands back the shell comments too, so
// the "a comment invokes nothing" rule has to be applied here now that it
// no longer falls out of skipping YAML comment lines (Copilot).
const line = region
const line = region.text
.split("\n")
.filter((l) => !l.trimStart().startsWith("#"))
.join("\n");
@ -323,8 +341,23 @@ export function findWorkflowViolations(text, file = "<workflow>") {
});
}
for (const match of line.matchAll(BROWSER_ENV_RE)) {
const value = stripQuotes(match[1]);
// The browser rule is the one that must respect WHICH position it is in.
const overrides =
kind === "env"
? region.name === BROWSER_ENV_VAR
? [{ value: region.value, match: region.text.trim() }]
: []
: kind === "run"
? [...line.matchAll(BROWSER_SHELL_RE)].map((m) => ({
value: m[1],
match: m[0].trim(),
}))
: // A `with:` value is an action input, not an environment. Whatever
// it names is covered by the script rules above.
[];
for (const override of overrides) {
const value = stripQuotes(override.value);
if (value === DEFAULT_BROWSER) continue;
// Setting it to nothing is not the same as leaving it out: the variable
// is then present and empty, which `resolveBrowserName` rejects outright
@ -334,9 +367,9 @@ export function findWorkflowViolations(text, file = "<workflow>") {
file,
line: lineNumber,
rule: VIOLATION.BROWSER_ENV,
match: match[0].trim(),
match: override.match,
message:
`SMOKE_BROWSER must not be set to ${described} in a workflow — only ` +
`${BROWSER_ENV_VAR} must not be set to ${described} in a workflow — only ` +
`\`${DEFAULT_BROWSER}\` runs in GitHub CI, an expression cannot be ` +
`checked statically, and an empty value is rejected rather than ` +
`defaulted. Leave it unset; the default is \`${DEFAULT_BROWSER}\`.`,

72
scripts/lib/workflow-gate.test.mjs
View file

@ -188,6 +188,52 @@ describe("findWorkflowViolations", () => {
].join("\n"),
rules: [VIOLATION.BROWSER_ENV],
},
{
// Copilot, fifth review: `container.env` is job-wide too — every step in
// the container sees it, so it redirects `npm run smoke` exactly as a
// job-level `env` would.
name: "flags a SMOKE_BROWSER override under container.env",
text: [
"on: push",
"jobs:",
" build:",
" container:",
" image: node:22",
" env:",
" SMOKE_BROWSER: firefox",
" steps:",
" - run: npm run smoke",
].join("\n"),
rules: [VIOLATION.BROWSER_ENV],
},
{
name: "does not trip over a container given as a bare image string",
text: [
"on: push",
"jobs:",
" build:",
" container: node:22",
" steps:",
" - run: npm run smoke",
].join("\n"),
rules: [],
},
{
// Copilot, fifth review: a blanket text match rejected a script that
// merely PRINTS the variable name, which sets nothing.
name: "does not read a script printing the variable as setting it",
text: workflow(
' - run: |\n echo "SMOKE_BROWSER: firefox"\n npm run smoke',
),
rules: [],
},
{
name: "does not read an action input mentioning it as setting it",
text: workflow(
' - with:\n message: "SMOKE_BROWSER: firefox"',
),
rules: [],
},
{
name: "allows SMOKE_BROWSER set explicitly to chromium",
text: workflow(
@ -334,6 +380,32 @@ describe("findWorkflowViolations", () => {
});
});
describe("the gate's name", () => {
const scripts = JSON.parse(
readFileSync(join(repoRoot, "package.json"), "utf8"),
).scripts;
it("is `local:gate`, and runs the local-only Storybook step", () => {
assert.ok(scripts["local:gate"], "the pre-push gate must exist");
assert.match(scripts["local:gate"], /local:storybook/);
});
it("has no `ci` alias, not even a deprecated one", () => {
// The rename is only worth anything if the old name is GONE (#2146): an
// alias preserves exactly the association being scrubbed, and leaves
// something copy-pasteable for a workflow author. Nothing else notices a
// reintroduced `ci`, so assert it here (Copilot).
const revived = Object.keys(scripts).filter(
(name) => name === "ci" || name.startsWith("ci:"),
);
assert.deepEqual(
revived,
[],
`no root script may be named \`ci\` or \`ci:*\` — found ${revived.join(", ")}`,
);
});
});
describe("extractExecutableRegions", () => {
it("descends the executable paths and skips the metadata beside them", () => {
const regions = extractExecutableRegions(