Skip to content

Commit 2ead041

Browse files
committed
chore(scripts): scan the shell command template, and flag a multi-line env override (#2146)
Copilot, ninth and tenth reviews: - A custom `shell:` is a COMMAND TEMPLATE that Actions runs around every script (`{0}` is the script path), so `shell: npm run local:gate && bash {0}` invoked the gate while the `run:` value beside it looked harmless. Step `shell`, and `defaults.run.shell` at both workflow and job level, are command regions now. An ordinary `bash`/`pwsh`/`cmd` matches no rule, so naming them costs nothing. - GitHub's multiline environment-file syntax (`SMOKE_BROWSER<<EOF`, value, delimiter) bypassed the assignment patterns. It is reported as an override whose value the guard cannot READ rather than parsed: the value can arrive through any number of shapes — a brace group redirected to $GITHUB_ENV, a heredoc nested in another — and a parser handling some of them would be a coverage claim that holds only for the shapes someone thought of. Nobody spells `chromium` that way, and if they did, `SMOKE_BROWSER=chromium` is the fix. Signed-off-by: Cliff Hall <cliff@futurescale.com> Signed-off-by: cliffhall <cliff@futurescale.com>
1 parent bc7abae commit 2ead041

4 files changed

Lines changed: 102 additions & 8 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1003,7 +1003,7 @@ Everything about the selection lives in **`scripts/lib/headless-browser.mjs`**
10031003
- **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.
10041004
- **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.
10051005
- **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.
1006-
- **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 (matched case-insensitively, since Windows environment variables are), or an assignment in a `run:` — so a script printing the variable's name is not read as setting it; and the `run:` half covers POSIX, cmd, the `>> $GITHUB_ENV` append and PowerShell (the default shell on a Windows runner), not just `NAME=value`. A name the guard cannot **read** counts as forbidden too — a workflow expression (`smoke:web:${{ matrix.browser }}`) or a shell variable the script expands itself (`smoke:web:$ENGINE`, `%ENGINE%`, `local:$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.
1006+
- **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 (matched case-insensitively, since Windows environment variables are), or an assignment in a `run:` — so a script printing the variable's name is not read as setting it; and the `run:` half covers POSIX, cmd, the `>> $GITHUB_ENV` append and PowerShell (the default shell on a Windows runner), not just `NAME=value`. A name the guard cannot **read** counts as forbidden too — a workflow expression (`smoke:web:${{ matrix.browser }}`) or a shell variable the script expands itself (`smoke:web:$ENGINE`, `%ENGINE%`, `local:$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`/`defaults.run.shell`, job `env`/`container.env`/`with`/`defaults.run.shell`, step `run`/`shell`/`env`/`with` (a custom `shell:` is a command template Actions runs *around* the script), 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.
10071007
- **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.**
10081008
- **`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.
10091009
- **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.

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,7 @@ So the direction that matters holds: **passing `npm run local:gate` means CI's g
502502

503503
**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.
504504

505-
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 `SMOKE_BROWSER` (case-insensitively, since Windows environment variables are), or an assignment in a `run:` script (POSIX, cmd, a `>> $GITHUB_ENV` append, or PowerShell — the default shell on a Windows runner) — so a script that merely *prints* the variable's name is not read as setting it. It treats any name it cannot **read** the same way — a workflow expression (`smoke:web:${{ matrix.browser }}`) or a shell variable the script expands itself (`smoke:web:$ENGINE`, `%ENGINE%`) — since a value the workflow can vary may well be `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.
505+
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 `SMOKE_BROWSER` (case-insensitively, since Windows environment variables are), or an assignment in a `run:` script (POSIX, cmd, a `>> $GITHUB_ENV` append, or PowerShell — the default shell on a Windows runner) — so a script that merely *prints* the variable's name is not read as setting it. It treats any name it cannot **read** the same way — a workflow expression (`smoke:web:${{ matrix.browser }}`) or a shell variable the script expands itself (`smoke:web:$ENGINE`, `%ENGINE%`) — since a value the workflow can vary may well be `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`/`defaults.run.shell`, job `env`/`container.env`/`with`/`defaults.run.shell`, step `run`/`shell`/`env`/`with` — a custom `shell:` counts, since Actions runs it *around* the script — 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.
506506

507507
| Script | What it does |
508508
| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |

‎scripts/lib/workflow-gate.mjs‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,17 @@ const BROWSER_ASSIGNMENTS = [
184184
),
185185
value: 1,
186186
},
187+
// GitHub's MULTILINE environment-file syntax: `SMOKE_BROWSER<<EOF`, the value
188+
// on following lines, then the delimiter. Reported as unreadable rather than
189+
// parsed (Copilot): the value can arrive through any number of shapes — a
190+
// brace group redirected to $GITHUB_ENV, a heredoc nested in another heredoc
191+
// — and a parser that handled some of them would be a coverage claim that
192+
// holds only for the shapes someone thought of. Nobody spells `chromium` this
193+
// way; if they did, `SMOKE_BROWSER=chromium` is the fix.
194+
{
195+
re: new RegExp(String.raw`\b${BROWSER_ENV_VAR}\s*<<[^\n]*`, "gi"),
196+
unreadable: true,
197+
},
187198
// .NET, reachable from PowerShell: SetEnvironmentVariable("NAME", "value").
188199
// The value arm accepts an UNQUOTED second argument as well — `$engine`, or a
189200
// `${{ }}` expression — because that spelling sets the environment just the
@@ -225,8 +236,8 @@ function stripQuotes(value) {
225236
* allowlist, so it changes nothing about the tarball.
226237
*
227238
* The walk is STRUCTURAL — it descends the schema's executable paths
228-
* (workflow `env`, job `env`/`container.env`/`with`, step `run`/`env`/`with`)
229-
* rather than
239+
* (workflow `env`/`defaults.run.shell`, job `env`/`container.env`/`with`/
240+
* `defaults.run.shell`, step `run`/`shell`/`env`/`with`) rather than
230241
* matching pairs by key name anywhere in the tree. Key-name matching looks
231242
* equivalent and is not: workflow input ids are user-defined, so an input
232243
* legitimately named `env` puts its own `description` and `default` in front of
@@ -305,9 +316,20 @@ export function extractExecutableRegions(text, file = "<workflow>") {
305316

306317
const at = (node, key) => (isMap(node) ? node.get(key, true) : undefined);
307318

319+
/**
320+
* `defaults.run.shell` — a custom `shell:` is a COMMAND TEMPLATE that
321+
* Actions runs around every script (`{0}` is the script path), so
322+
* `shell: npm run local:gate && bash {0}` invokes the gate while the `run:`
323+
* value beside it looks harmless (Copilot). An ordinary `bash`/`pwsh`/`cmd`
324+
* matches none of the rules, so naming it here costs nothing.
325+
*/
326+
const addDefaultShell = (node) =>
327+
addCommand(at(deref(at(deref(node), "run")), "shell"));
328+
308329
const root = deref(doc.contents);
309330
if (!isMap(root)) continue;
310331
addMapping("env", at(root, "env"));
332+
addDefaultShell(at(root, "defaults"));
311333

312334
const jobs = deref(at(root, "jobs"));
313335
if (!isMap(jobs)) continue;
@@ -322,13 +344,15 @@ export function extractExecutableRegions(text, file = "<workflow>") {
322344
// job-level `env` would (Copilot). `container:` may also be a bare image
323345
// string, which `addMapping` ignores.
324346
addMapping("env", at(deref(at(job, "container")), "env"));
347+
addDefaultShell(at(job, "defaults"));
325348

326349
const steps = deref(at(job, "steps"));
327350
if (!isSeq(steps)) continue;
328351
for (const stepNode of steps.items) {
329352
const step = deref(stepNode);
330353
if (!isMap(step)) continue;
331354
addCommand(at(step, "run"));
355+
addCommand(at(step, "shell"));
332356
addMapping("env", at(step, "env"));
333357
addMapping("with", at(step, "with"));
334358
}
@@ -418,9 +442,10 @@ export function findWorkflowViolations(text, file = "<workflow>") {
418442
? [{ value: region.value, match: region.text.trim() }]
419443
: []
420444
: kind === "run"
421-
? BROWSER_ASSIGNMENTS.flatMap(({ re, value }) =>
445+
? BROWSER_ASSIGNMENTS.flatMap(({ re, value, unreadable }) =>
422446
[...line.matchAll(re)].map((m) => ({
423-
value: m[value],
447+
value: unreadable ? null : m[value],
448+
unreadable,
424449
match: m[0].trim(),
425450
})),
426451
)
@@ -429,12 +454,17 @@ export function findWorkflowViolations(text, file = "<workflow>") {
429454
[];
430455

431456
for (const override of overrides) {
432-
const value = stripQuotes(override.value);
457+
const value = override.unreadable ? null : stripQuotes(override.value);
433458
if (value === DEFAULT_BROWSER) continue;
434459
// Setting it to nothing is not the same as leaving it out: the variable
435460
// is then present and empty, which `resolveBrowserName` rejects outright
436461
// rather than falling back to the default.
437-
const described = value === "" ? "an empty value" : `\`${value}\``;
462+
const described =
463+
value === null
464+
? "a multi-line value this guard cannot read"
465+
: value === ""
466+
? "an empty value"
467+
: `\`${value}\``;
438468
findings.push({
439469
file,
440470
line: lineNumber,

0 commit comments

Comments
 (0)