diff --git a/AGENTS.md b/AGENTS.md index 5f3609bd..a698d4ee 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`, 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. +- **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. - **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 603a9733..e19004fe 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` (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. +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. | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | diff --git a/scripts/lib/workflow-gate.mjs b/scripts/lib/workflow-gate.mjs index 6613d904..76bc4848 100644 --- a/scripts/lib/workflow-gate.mjs +++ b/scripts/lib/workflow-gate.mjs @@ -66,7 +66,14 @@ * ordinary step names, which is the faster way to get a guard deleted. */ -import { LineCounter, parseAllDocuments, visit } from "yaml"; +import { + LineCounter, + isAlias, + isMap, + isScalar, + isSeq, + parseAllDocuments, +} from "yaml"; import { SUPPORTED_BROWSERS, DEFAULT_BROWSER } from "./headless-browser.mjs"; /** Scripts whose `local:` prefix declares them local-only. */ @@ -126,9 +133,6 @@ function stripQuotes(value) { 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. * @@ -146,6 +150,22 @@ export const EXECUTABLE_KEYS = ["run", "env", "with"]; * about refusing what the repo has. This file is not in the published `files` * 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 + * 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 + * the rules, and a doc string mentioning `SMOKE_BROWSER: firefox` fails the + * gate (Copilot). A guard that fails on documentation of itself does not + * survive contact with a contributor. + * + * ALIASES ARE RESOLVED. An alias node carries no `.value`, so reading one as a + * scalar yields the empty string — and `run: *command`, with the command + * anchored in a metadata scalar the walk never visits, would invoke anything at + * all while the guard saw nothing (Copilot). Findings are reported at the line + * where the alias is USED, not where its anchor was defined, since that is the + * line to change. + * * 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. * @@ -161,8 +181,6 @@ 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) { @@ -171,40 +189,68 @@ export function extractExecutableRegions(text, file = "") { ); } - visit(doc, { - Pair(_key, pair) { - const key = isScalar(pair.key) ? String(pair.key.value) : null; - if (key === null || !EXECUTABLE_KEYS.includes(key)) return; + const lineOf = (node) => lineCounter.linePos(node.range[0]).line; + const deref = (node) => (isAlias(node) ? node.resolve(doc) : node); - // `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; - } + /** A `run:` command — one scalar, reported where it is written or aliased. */ + const addCommand = (node) => { + if (node == null) return; + const resolved = deref(node); + if (!isScalar(resolved)) return; + regions.push({ + line: lineOf(node), + kind: "run", + text: String(resolved.value ?? ""), + }); + }; - // `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}`, - }); - }, + /** + * An `env:` or `with:` mapping. Each entry is rebuilt as `NAME: value` so + * the rules 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. + */ + const addMapping = (kind, node) => { + if (node == null) return; + const map = deref(node); + if (!isMap(map)) return; + for (const entry of map.items) { + if (!isScalar(entry.key)) continue; + const value = deref(entry.value); + const text = isScalar(value) ? String(value.value ?? "") : ""; + regions.push({ + line: lineOf(entry.key), + kind, + text: `${String(entry.key.value)}: ${text}`, }); - }, - }); + } + }; + + const at = (node, key) => (isMap(node) ? node.get(key, true) : undefined); + + const root = deref(doc.contents); + if (!isMap(root)) continue; + addMapping("env", at(root, "env")); + + const jobs = deref(at(root, "jobs")); + if (!isMap(jobs)) continue; + for (const jobEntry of jobs.items) { + const job = deref(jobEntry.value); + if (!isMap(job)) continue; + addMapping("env", at(job, "env")); + // A job-level `with:` is the input block of a reusable-workflow call. + addMapping("with", at(job, "with")); + + const steps = deref(at(job, "steps")); + if (!isSeq(steps)) continue; + for (const stepNode of steps.items) { + const step = deref(stepNode); + if (!isMap(step)) continue; + addCommand(at(step, "run")); + addMapping("env", at(step, "env")); + addMapping("with", at(step, "with")); + } + } } return regions; @@ -223,9 +269,17 @@ export function extractExecutableRegions(text, file = "") { export function findWorkflowViolations(text, file = "") { const findings = []; - for (const { line: lineNumber, text: line } of extractExecutableRegions( + for (const { line: lineNumber, text: region } of extractExecutableRegions( text, + file, )) { + // 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 + .split("\n") + .filter((l) => !l.trimStart().startsWith("#")) + .join("\n"); for (const match of line.matchAll(LOCAL_SCRIPT_RE)) { findings.push({ file, diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index aafc64c0..70569819 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -27,78 +27,182 @@ import { const repoRoot = join(import.meta.dirname, "..", ".."); const workflowDir = join(repoRoot, ".github", "workflows"); +/** + * A minimal but SCHEMA-VALID workflow around some step YAML. + * + * The fixtures have to be real workflows now that the walk is structural + * rather than key-name based: a bare `- run: …` fragment sits in no job and no + * step, so it is correctly invisible, and a table built out of fragments would + * pass while asserting nothing. + */ +const workflow = (steps) => + [ + "on: push", + "jobs:", + " build:", + " runs-on: ubuntu-latest", + " steps:", + steps, + ].join("\n"); + describe("findWorkflowViolations", () => { const cases = [ { name: "flags the pre-push gate itself", - text: " - run: npm run local:gate\n", + text: workflow(" - run: npm run local:gate"), rules: [VIOLATION.LOCAL_SCRIPT], }, { name: "flags any other local: script, without being edited for it", - text: " - run: npm run local:storybook\n", + text: workflow(" - run: npm run local:storybook"), rules: [VIOLATION.LOCAL_SCRIPT], }, { name: "flags the Firefox engine pass", - text: " - run: npm run smoke:web:firefox\n", + text: workflow(" - run: npm run smoke:web:firefox"), rules: [VIOLATION.ENGINE_SCRIPT], }, { name: "flags the WebKit engine pass", - text: " - run: npm run smoke:web:webkit\n", + text: workflow(" - run: npm run smoke:web:webkit"), rules: [VIOLATION.ENGINE_SCRIPT], }, { name: "flags the env-driven runner, whose engine cannot be read statically", - text: " - run: npm run smoke:web:engine\n", + text: workflow(" - run: npm run smoke:web:engine"), 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", + text: workflow(" - run: npm run smoke:web:${{ matrix.browser }}"), rules: [VIOLATION.ENGINE_SCRIPT], }, { name: "flags a local: script whose name is built from an expression", - text: " - run: npm run local:${{ matrix.task }}\n", + text: workflow(" - run: npm run local:${{ matrix.task }}"), + rules: [VIOLATION.LOCAL_SCRIPT], + }, + { + // Copilot, second review: an expression body can itself contain braces, + // so a brace-balanced regex stops at the first `}` and misses the one + // case worth catching. The rule keys off the `${{` prefix instead. + name: "flags an expression whose own body contains braces", + text: workflow( + " - run: npm run smoke:web:${{ format('{0}', matrix.browser) }}", + ), + rules: [VIOLATION.ENGINE_SCRIPT], + }, + { + name: "flags a command inside a block scalar", + text: workflow(" - run: |\n npm run local:gate"), + 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: workflow( + " - run: | # why this step exists\n npm run local:gate", + ), + rules: [VIOLATION.LOCAL_SCRIPT], + }, + { + // Copilot, fourth review: an alias carries no `.value`, so reading one as + // a scalar sees an empty string — with the command anchored somewhere the + // walk never visits, this invoked the gate while the guard saw nothing. + name: "flags a command reached through a YAML alias", + text: workflow( + [ + " - name: &cmd npm run local:gate", + " run: echo documenting the gate", + " - run: *cmd", + ].join("\n"), + ), + rules: [VIOLATION.LOCAL_SCRIPT], + }, + { + name: "flags a command passed as an action input", + text: workflow(" - with:\n args: npm run local:gate"), rules: [VIOLATION.LOCAL_SCRIPT], }, { name: "flags SMOKE_BROWSER pointed at another engine", - text: " env:\n SMOKE_BROWSER: firefox\n", + text: workflow( + " - env:\n SMOKE_BROWSER: firefox\n run: npm run smoke", + ), rules: [VIOLATION.BROWSER_ENV], }, { name: "flags a quoted SMOKE_BROWSER value", - text: ' env:\n SMOKE_BROWSER: "webkit"\n', + text: workflow( + ' - env:\n SMOKE_BROWSER: "webkit"\n run: npm run smoke', + ), rules: [VIOLATION.BROWSER_ENV], }, { name: "flags a shell-assigned SMOKE_BROWSER", - text: " - run: SMOKE_BROWSER=firefox npm run smoke\n", + text: workflow(" - run: SMOKE_BROWSER=firefox npm run smoke"), rules: [VIOLATION.BROWSER_ENV], }, { name: "flags an expression, which cannot be checked statically", - text: " env:\n SMOKE_BROWSER: ${{ matrix.browser }}\n", + text: workflow( + " - env:\n SMOKE_BROWSER: ${{ matrix.browser }}\n run: npm run smoke", + ), + rules: [VIOLATION.BROWSER_ENV], + }, + { + // Copilot, third review: the flow spelling of the same mapping. + name: "flags an env override written as a flow mapping", + text: workflow( + " - env: { SMOKE_BROWSER: firefox }\n run: npm run smoke", + ), + 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: workflow( + " - env:\n SMOKE_BROWSER:\n run: npm run smoke", + ), + rules: [VIOLATION.BROWSER_ENV], + }, + { + name: "flags an empty shell assignment too", + text: workflow(" - run: SMOKE_BROWSER= npm run smoke"), + rules: [VIOLATION.BROWSER_ENV], + }, + { + name: "flags a workflow-level env override", + text: [ + "on: push", + "env:", + " SMOKE_BROWSER: firefox", + "jobs:", + " build:", + " steps:", + " - run: npm run smoke", + ].join("\n"), rules: [VIOLATION.BROWSER_ENV], }, { name: "allows SMOKE_BROWSER set explicitly to chromium", - text: " env:\n SMOKE_BROWSER: chromium\n", + text: workflow( + " - env:\n SMOKE_BROWSER: chromium\n run: npm run smoke", + ), rules: [], }, { name: "allows the smoke suite, which belongs in CI", - text: " - run: npm run smoke\n", + text: workflow(" - run: npm run smoke"), rules: [], }, { name: "allows the Chromium engine pass, which belongs in CI", - text: " - run: npm run smoke:web:chromium\n", + text: workflow(" - run: npm run smoke:web:chromium"), rules: [], }, { @@ -106,118 +210,119 @@ describe("findWorkflowViolations", () => { // deliberately outside ENGINE_SMOKES — it is not an engine pass, and the // guard must not read `smoke:web:` as forbidden by itself. name: "allows the Chromium-only tab smoke", - text: " - run: npm run smoke:web:tabs\n", + text: workflow(" - run: npm run smoke:web:tabs"), rules: [], }, { name: "allows smoke:tui, which self-skips under CI on its own", - text: " - run: npm run smoke:tui\n", + text: workflow(" - run: npm run smoke:tui"), rules: [], }, { name: "allows the rest of the CI chain", - text: [ - " - run: npm run validate", - " - run: npm run coverage", - " - run: npm run verify:build-gate", - " - run: npm run verify:bundle-externals", - " - run: npm run test:storybook", - "", - ].join("\n"), + text: workflow( + [ + " - run: npm run validate", + " - run: npm run coverage", + " - run: npm run verify:build-gate", + " - run: npm run verify:bundle-externals", + " - run: npm run test:storybook", + ].join("\n"), + ), rules: [], }, - { - // Copilot, second review: an expression body can itself contain braces, - // so a brace-balanced regex stops at the first `}` and misses the one - // case worth catching. The rule keys off the `${{` prefix instead. - name: "flags an expression whose own body contains braces", - text: " - run: npm run smoke:web:${{ format('{0}', matrix.browser) }}\n", - rules: [VIOLATION.ENGINE_SCRIPT], - }, - { - name: "flags a command inside a block scalar, not just an inline run", - text: " - run: |\n npm run local:gate\n", - rules: [VIOLATION.LOCAL_SCRIPT], - }, - { - name: "flags a command passed as an action input", - 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. name: "does not read a step name as an invocation", - text: " - name: Explain why smoke:web:firefox stays local\n", + text: workflow( + " - name: Explain why smoke:web:firefox stays local\n run: npm run smoke", + ), rules: [], }, { name: "does not read an `if:` condition as one either", - text: " if: github.event_name == 'push'\n", + text: workflow( + " - if: github.event_name == 'push'\n run: npm run smoke", + ), + rules: [], + }, + { + // Copilot, fourth review: workflow input ids are user-defined, so an + // input legitimately named `env` would have its own docs scanned by a + // key-name match — failing the gate on documentation of the gate. + name: "does not scan a workflow input that happens to be named env", + text: [ + "on:", + " workflow_dispatch:", + " inputs:", + " env:", + " description: why smoke:web:firefox stays local", + " default: npm run local:gate", + "jobs:", + " build:", + " steps:", + " - run: npm run smoke", + ].join("\n"), rules: [], }, { name: "does not read a similarly-named key as the override", - text: " env:\n SMOKE_BROWSER_DOC: firefox\n", + text: workflow( + " - env:\n SMOKE_BROWSER_DOC: firefox\n run: npm run smoke", + ), rules: [], }, { - name: "does not read a comment as an invocation", - text: " # smoke:web:firefox runs in npm run local:gate, never here\n", + name: "does not read a YAML comment as an invocation", + text: workflow( + " # smoke:web:firefox runs in npm run local:gate, never here\n - run: npm run smoke", + ), rules: [], }, { + // Copilot, third review: this fixture used to be a YAML comment, which + // the parser strips — so it asserted nothing. Inside a `run:` block the + // comment text really does survive into the scalar, which is where the + // "a comment invokes nothing" rule has to be applied. name: "does not read a shell comment inside a run block as one either", - text: " # SMOKE_BROWSER=webkit is local-only\n", + text: workflow( + " - run: |\n # SMOKE_BROWSER=webkit is local-only\n npm run smoke", + ), rules: [], }, ]; for (const { name, text, rules } of cases) { it(name, () => { - const found = findWorkflowViolations(text, "case.yml").map((f) => f.rule); + const found = findWorkflowViolations(text, "case.yml"); assert.deepEqual( - found, + found.map((f) => f.rule), rules, - formatWorkflowViolations(findWorkflowViolations(text, "case.yml")), + formatWorkflowViolations(found), ); }); } it("reports the line a finding came from", () => { const [finding] = findWorkflowViolations( - "jobs:\n build:\n steps:\n - run: npm run local:gate\n", + workflow(" - run: npm run local:gate"), "main.yml", ); - assert.equal(finding.line, 4); + assert.equal(finding.line, 6); assert.equal(finding.file, "main.yml"); assert.equal(finding.match, "local:gate"); }); + it("names the file in a parse error, rather than a placeholder", () => { + // The filename is passed in for diagnostics; forgetting to forward it left + // a malformed real workflow reported as `` (Copilot). + assert.throws( + () => findWorkflowViolations("jobs:\n - a\n b: c\n", "main.yml"), + /could not parse main\.yml/, + ); + }); + it("derives the forbidden engines rather than listing them by hand", () => { // Adding a fourth engine to SUPPORTED_BROWSERS must extend the guard, not // open a hole in it — which is why this reads the derived constant. @@ -230,26 +335,26 @@ describe("findWorkflowViolations", () => { }); describe("extractExecutableRegions", () => { - it("ends a block when the next line dedents, and re-reads that line", () => { - // The table cases cannot isolate this: a block that never closed would - // still produce the right findings for them, and would then read the whole - // rest of the file as executable. + it("descends the executable paths and skips the metadata beside them", () => { const regions = extractExecutableRegions( [ - "steps:", - " - name: smoke:web:firefox is local-only", - " env:", - " SMOKE_BROWSER: chromium", - " run: npm run smoke", - "", + "on: push", + "jobs:", + " build:", + " 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. + // Order follows the walk (a step's `run` before its `env`), not the file. assert.deepEqual( regions.map((r) => [r.kind, r.text.trim()]), [ - ["env", "SMOKE_BROWSER: chromium"], ["run", "npm run smoke"], + ["env", "SMOKE_BROWSER: chromium"], ], ); });