From b8d3cfdc3dc34309defc72a5c4d4e1ed682c992b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 00:52:49 -0400 Subject: [PATCH] chore(scripts): parse the workflow instead of matching lines (#2146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, third review — and the second finding is really a verdict on the approach, so the fix is the approach: - `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. The value pattern required \S+, so the guard said nothing. It is \S* now, with its own message. - The hand-rolled opener could not see `run: | # explanation` or the flow mapping `env: { SMOKE_BROWSER: firefox }`. That is the third spelling it has missed in three rounds, each one a forbidden invocation that would have passed while test:scripts stayed green — worse than no guard, because it reports a coverage that is not there. YAML has too many spellings to recognize by indentation, so extractExecutableRegions now parses with the `yaml` package. It is already a root dependency, and this file is not in the published files allowlist, so nothing about the tarball changes. A parse error now throws rather than yielding no regions: a workflow the guard cannot read must not pass as a workflow with nothing in it. Verified by injecting a flow-mapping SMOKE_BROWSER override into the real main.yml — the suite fails, and goes green again when removed. Signed-off-by: Cliff Hall Signed-off-by: cliffhall --- AGENTS.md | 2 +- README.md | 2 +- scripts/lib/workflow-gate.mjs | 181 +++++++++++++++++------------ scripts/lib/workflow-gate.test.mjs | 47 +++++++- 4 files changed, 152 insertions(+), 80 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 59e4ec1f..5f3609bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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:`, or `npx playwright install --with-deps ` 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`. 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 reads only the positions that execute something — `run:` scalars (inline and block) and the values of an `env:` or `with:` mapping — 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. 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 reads only the positions that execute something — `run:` scalars (inline and block) and the values of an `env:` or `with:` mapping — 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. diff --git a/README.md b/README.md index 4d453083..603a9733 100644 --- a/README.md +++ b/README.md @@ -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 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 reads only the positions that actually execute something — `run:` scalars and the values of an `env:` or `with:` mapping — 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). 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 rather than matching lines, and reads only the positions that actually execute something — `run:` scalars and the values of an `env:` or `with:` mapping — 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 | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | diff --git a/scripts/lib/workflow-gate.mjs b/scripts/lib/workflow-gate.mjs index 3d7dd3fb..6613d904 100644 --- a/scripts/lib/workflow-gate.mjs +++ b/scripts/lib/workflow-gate.mjs @@ -49,14 +49,15 @@ * `process.env.CI` on its own, so it needs no guarding. * * ONLY EXECUTABLE POSITIONS ARE SCANNED — `run:` scalars (inline and block), - * and the values of an `env:` or `with:` mapping. Everything else in a workflow - * is metadata that runs nothing, so scanning it produces findings that are - * simply false: `name: Explain why smoke:web:firefox stays local` executes - * nothing, and failing the suite on it would contradict the "what it forbids, - * and nothing more" contract above (Copilot). Comments are skipped for the same - * reason and one more: the workflow's value is largely in its prose, and a - * guard that made the two tiers undocumentable in the file implementing one of - * them would be traded for the wrong thing. + * and the values of an `env:` or `with:` mapping, found by parsing the file + * rather than by matching lines. Everything else in a workflow is metadata that + * runs nothing, so scanning it produces findings that are simply false: + * `name: Explain why smoke:web:firefox stays local` executes nothing, and + * failing the suite on it would contradict the "what it forbids, and nothing + * more" contract above (Copilot). Comments fall out for the same reason — and + * the workflow's value is largely in its prose, so a guard that made the two + * tiers undocumentable in the file implementing one of them would be traded for + * the wrong thing. * * That is a real limit, stated rather than hidden: an execution vector outside * those three positions — a custom action that reads a script name from @@ -65,6 +66,7 @@ * ordinary step names, which is the faster way to get a guard deleted. */ +import { LineCounter, parseAllDocuments, visit } from "yaml"; import { SUPPORTED_BROWSERS, DEFAULT_BROWSER } from "./headless-browser.mjs"; /** Scripts whose `local:` prefix declares them local-only. */ @@ -110,74 +112,104 @@ const DYNAMIC_ENGINE_RE = new RegExp( "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; - -function isComment(line) { - return line.trimStart().startsWith("#"); -} - -function indentOf(line) { - return line.length - line.trimStart().length; -} - -/** `run:` with an inline command, optionally as the first key of a `- ` item. */ -const RUN_INLINE_RE = /^\s*(?:-\s+)?run:\s*(.*)$/; -/** A key that opens a block whose nested lines are executable: `run: |`, `env:`, `with:`. */ -const BLOCK_OPEN_RE = /^\s*(?:-\s+)?(run|env|with):\s*(?:[|>][-+]?\d*)?\s*$/; - -/** - * The executable scalars of a workflow, with the line each came from. - * - * Deliberately hand-rolled rather than YAML-parsed: the guard runs in - * `test:scripts`, which is `node --test` with no dependencies by design, and - * the three shapes it needs are simple enough to recognize by indentation. - * - * @param {string} text - * @returns {Array<{line: number, kind: string, text: string}>} - */ -export function extractExecutableRegions(text) { - const regions = []; - let blockKind = null; - let blockIndent = 0; - - text.split(/\r?\n/).forEach((line, index) => { - if (isComment(line)) return; - const lineNumber = index + 1; - - if (blockKind !== null) { - // A blank line neither ends a block scalar nor carries anything. - if (line.trim() === "") return; - if (indentOf(line) > blockIndent) { - regions.push({ line: lineNumber, kind: blockKind, text: line }); - return; - } - // Dedented out of the block — fall through and re-read this line as a key. - blockKind = null; - } - - const block = BLOCK_OPEN_RE.exec(line); - if (block) { - blockKind = block[1]; - blockIndent = indentOf(line); - return; - } - - const run = RUN_INLINE_RE.exec(line); - if (run && run[1].trim() !== "") { - regions.push({ line: lineNumber, kind: "run", text: run[1] }); - } - }); - - return regions; -} +// (`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 can still be quoted; a YAML value +// arrives already unquoted from the parser, so this is a no-op there. function stripQuotes(value) { const trimmed = value.trim().replace(/[,;]+$/, ""); const quoted = /^(["'])(.*)\1$/.exec(trimmed); return quoted ? quoted[2] : trimmed; } +/** The mapping keys whose values a workflow actually executes. */ +export const EXECUTABLE_KEYS = ["run", "env", "with"]; + +/** + * The executable scalars of a workflow, with the line each came from. + * + * Parsed with the `yaml` package rather than by hand. The first two rounds of + * this guard did hand-roll it, and both were wrong in the same direction — a + * regex that stopped at the first `}`, then an opener that could not see + * `run: | # explanation` or the flow mapping `env: { SMOKE_BROWSER: firefox }` + * (Copilot, twice). Each miss is a workflow that invokes a forbidden pass while + * `test:scripts` stays green, which is worse than no guard, because it reports + * a coverage that is not there. YAML has too many spellings of the same thing + * to recognize by indentation; a real parser knows all of them. + * + * `yaml` is already a root **dependency**, so this adds no install — and + * `test:scripts` staying dependency-free was always about not adding one, not + * about refusing what the repo has. This file is not in the published `files` + * allowlist, so it changes nothing about the tarball. + * + * A parse error THROWS rather than yielding no regions: a workflow the guard + * cannot read must not pass as a workflow with nothing in it. + * + * A multi-line `run:` block is one region carrying the whole script, reported + * at the line of its `run:` key rather than at the offending line inside it. + * The finding also prints what it matched, so that is still actionable. + * + * @param {string} text + * @param {string} [file] only for the parse-error message + * @returns {Array<{line: number, kind: string, text: string}>} + */ +export function extractExecutableRegions(text, file = "") { + const lineCounter = new LineCounter(); + const docs = parseAllDocuments(text, { lineCounter }); + const regions = []; + const lineOf = (node) => lineCounter.linePos(node.range[0]).line; + const isScalar = (node) => node?.range != null && !("items" in node); + + for (const doc of docs) { + if (doc.errors.length > 0) { + throw new Error( + `workflow-gate: could not parse ${file}: ${doc.errors[0].message}`, + ); + } + + visit(doc, { + Pair(_key, pair) { + const key = isScalar(pair.key) ? String(pair.key.value) : null; + if (key === null || !EXECUTABLE_KEYS.includes(key)) return; + + // `run:` — one scalar holding the command (inline or block). + if (isScalar(pair.value)) { + regions.push({ + line: lineOf(pair.value), + kind: key, + text: String(pair.value.value ?? ""), + }); + return; + } + + // `env:` / `with:` — a mapping, in either block or flow form. Each + // entry is rebuilt as `NAME: value` so the rules below read it the way + // they read a shell assignment, and so an entry with NO value (valid + // YAML, and an empty-string override) still reaches them. + visit(pair.value, { + Pair(_k, entry) { + if (!isScalar(entry.key)) return; + const value = isScalar(entry.value) + ? String(entry.value.value ?? "") + : ""; + regions.push({ + line: lineOf(entry.key), + kind: key, + text: `${String(entry.key.value)}: ${value}`, + }); + }, + }); + }, + }); + } + + return regions; +} + /** * Scan one workflow file's text for invocations that must stay local-only. * @@ -240,15 +272,20 @@ export function findWorkflowViolations(text, file = "") { for (const match of line.matchAll(BROWSER_ENV_RE)) { const value = stripQuotes(match[1]); 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 + // rather than falling back to the default. + const described = value === "" ? "an empty value" : `\`${value}\``; findings.push({ file, line: lineNumber, rule: VIOLATION.BROWSER_ENV, match: match[0].trim(), message: - `SMOKE_BROWSER must not be set to \`${value}\` in a workflow — only ` + - `\`${DEFAULT_BROWSER}\` runs in GitHub CI, and an expression cannot be ` + - `checked statically. Leave it unset; the default is \`${DEFAULT_BROWSER}\`.`, + `SMOKE_BROWSER 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}\`.`, }); } } diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index 8611f723..aafc64c0 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -144,6 +144,30 @@ describe("findWorkflowViolations", () => { text: " with:\n args: npm run local:gate\n", rules: [VIOLATION.LOCAL_SCRIPT], }, + { + // Copilot, third review: a hand-rolled opener could not see either of + // these, and each is a forbidden invocation that would have passed. + name: "flags a block scalar opened with a trailing comment", + text: " - run: | # why this step exists\n npm run local:gate\n", + rules: [VIOLATION.LOCAL_SCRIPT], + }, + { + name: "flags an env override written as a flow mapping", + text: " - env: { SMOKE_BROWSER: firefox }\n run: npm run smoke\n", + rules: [VIOLATION.BROWSER_ENV], + }, + { + // Present-but-empty is not unset: `resolveBrowserName` rejects it rather + // than falling back to chromium, so a workflow must not write it either. + name: "flags SMOKE_BROWSER set to nothing at all", + text: " env:\n SMOKE_BROWSER:\n", + rules: [VIOLATION.BROWSER_ENV], + }, + { + name: "flags an empty shell assignment too", + text: " - run: SMOKE_BROWSER= npm run smoke\n", + rules: [VIOLATION.BROWSER_ENV], + }, { // Copilot, second review: metadata executes nothing, so a finding there // is simply false — and the fastest way to get a guard deleted. @@ -157,8 +181,8 @@ describe("findWorkflowViolations", () => { rules: [], }, { - name: "does not read a bare mapping line outside env/with/run", - text: " SMOKE_BROWSER-ish-doc: firefox\n", + name: "does not read a similarly-named key as the override", + text: " env:\n SMOKE_BROWSER_DOC: firefox\n", rules: [], }, { @@ -212,13 +236,15 @@ describe("extractExecutableRegions", () => { // rest of the file as executable. const regions = extractExecutableRegions( [ - " env:", - " SMOKE_BROWSER: chromium", - " - name: smoke:web:firefox is local-only", - " - run: npm run smoke", + "steps:", + " - name: smoke:web:firefox is local-only", + " env:", + " SMOKE_BROWSER: chromium", + " run: npm run smoke", "", ].join("\n"), ); + // The step `name:` is absent, and the two executable values are not. assert.deepEqual( regions.map((r) => [r.kind, r.text.trim()]), [ @@ -227,6 +253,15 @@ describe("extractExecutableRegions", () => { ], ); }); + + it("throws on a workflow it cannot parse, rather than finding nothing", () => { + // A file that does not parse must not read as a file with nothing in it — + // that is the one failure mode where an unreadable workflow passes. + assert.throws( + () => extractExecutableRegions("jobs:\n - a\n b: c\n", "broken.yml"), + /could not parse broken\.yml/, + ); + }); }); describe(".github/workflows", () => {