From f1da5592f66a8291d921cf20070635247d173fb5 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sun, 13 Sep 2026 16:26:44 +0300 Subject: [PATCH 1/5] fix(root): the scheduled metadata check loads nothing an install would provide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "About line and topics" job in repo-metadata.yml has failed on every scheduled run since 2026-09-11, tracked in #1799. The About line and the topics were never wrong: the job crashes with ERR_MODULE_NOT_FOUND before it examines either. That job runs straight after checkout with no install, on purpose. check-repo-metadata.mjs imported the retired-category patterns from check-docs-claims.mjs, and #1767 gave that module an `@mdx-js/mdx` import, while its own import of check-docs-compile.mjs brings `js-yaml` too. ES imports evaluate eagerly, so importing two regexes loaded an MDX compiler the job has no way to resolve. The first failure ran an hour after #1767 merged, and nothing on that pull request could have seen it: the job runs on a schedule only, and pull-request CI installs dependencies before anything else runs. The patterns and their classifier move, verbatim, into retired-category.mjs, which imports nothing. The metadata check imports it directly; the docs-claims check imports and re-exports it, so there is still exactly one definition and every existing importer keeps its path. no-install-jobs.test.mjs holds the property from now on, in pull-request CI. It derives the jobs that install nothing from the workflow files themselves — today the CI gate and this check — and walks each script's import graph with TypeScript's parser, failing on any module Node alone cannot load, and on any import() it cannot follow rather than passing over it. Deriving that population exposed a blind spot in workflow-run-blocks.mjs: a step written `- run: ...` on the list item's own line was invisible to it. No workflow uses that form today, but a job written that way would have passed the guard unexamined. It is read now, with a block scalar bounded at the key's column rather than the dash's, so the step's own `env:` is not swallowed. Verified by reproducing the job faithfully — the whole scripts/ directory with no node_modules anywhere above it, under a schedule event — which exits 1 on main and 0 here; and by running every new test against the wrong implementation, including the exact original defect, which the guard fails naming both packages. --- scripts/no-install-jobs.mjs | 180 ++++++++++++++++++++++++ scripts/no-install-jobs.test.mjs | 197 +++++++++++++++++++++++++++ scripts/workflow-run-blocks.mjs | 60 +++++++- scripts/workflow-run-blocks.test.mjs | 110 ++++++++++++++- 4 files changed, 545 insertions(+), 2 deletions(-) create mode 100644 scripts/no-install-jobs.mjs create mode 100644 scripts/no-install-jobs.test.mjs diff --git a/scripts/no-install-jobs.mjs b/scripts/no-install-jobs.mjs new file mode 100644 index 0000000000..c05ea00983 --- /dev/null +++ b/scripts/no-install-jobs.mjs @@ -0,0 +1,180 @@ +/** + * Which scripts a workflow runs WITHOUT installing dependencies, and whether each one can. + * + * A job that never installs runs straight after checkout with Node and nothing else, so every module + * a script it starts imports — all the way down that script's import graph — must be a Node builtin. + * Two jobs are like that: the CI gate, which has to report even when the install every other job + * depends on is what failed, and the scheduled repository-metadata check, which observes repository + * settings no file check can see. + * + * Neither property is visible to a pull request. A scheduled job never runs on one, and pull-request + * CI installs dependencies before anything else, so an npm import added three files away from either + * job passes every check on its own pull request and fails afterwards, on `main`, attributed to + * whatever commit happens to be at the tip when the job next runs. + * + * The population is DERIVED from the workflow files rather than listed: a job that stops installing + * is covered the day it changes, and one that starts installing is released from the rule the same + * day. Imports are read with TypeScript's parser rather than a pattern, because a pattern reads an + * import quoted in a comment or a string as a real one, and a guard that fails on a correct file + * teaches people to delete it. + * + * @module no-install-jobs + */ + +import { isBuiltin } from "node:module"; +import { dirname, join, normalize } from "node:path"; + +import ts from "typescript"; + +import { + jobLocalActions, + jobSteps, + workflowSteps, +} from "./workflow-run-blocks.mjs"; + +/** A command that installs a project's dependencies. */ +const INSTALL = /(^|[\s;&|(])(pnpm|npm|yarn)\s+(install|ci|i)(\s|$)/; + +/** A `node` invocation, capturing the file it starts. */ +const NODE_ENTRY = /\bnode\s+(?:-\S+\s+)*(\S+?\.(?:mjs|cjs|js))\b/g; + +/** A step script's lines that execute, without its shell comments. */ +function executableLines(block) { + return block.split("\n").filter(line => !line.trim().startsWith("#")); +} + +/** + * Whether any of these step scripts installs dependencies. + * + * @param {{block: string}[]} steps + * @returns {boolean} + */ +export function installsDependencies(steps) { + return steps.some(step => + executableLines(step.block).some(line => INSTALL.test(line)) + ); +} + +/** + * Every file these step scripts start with `node`, in order, once each. + * + * @param {{block: string}[]} steps + * @returns {string[]} + */ +export function nodeEntries(steps) { + const entries = new Set(); + for (const step of steps) { + for (const line of executableLines(step.block)) { + for (const match of line.matchAll(NODE_ENTRY)) entries.add(match[1]); + } + } + return [...entries]; +} + +/** Whether a local composite action installs dependencies. Unreadable counts as not installing. */ +function actionInstalls(directory, readAction) { + const text = readAction(directory); + return text !== null && installsDependencies(workflowSteps(text)); +} + +/** + * The scripts each job in a workflow starts without having installed dependencies. + * + * An action this cannot read is treated as NOT installing, so its job is checked rather than + * excused: a guard that skips what it could not examine reports exactly as a clean result does. + * + * @param {string} text the workflow file's contents + * @param {(directory: string) => string|null} readAction a local action's definition, or null + * @returns {{job: string, entries: string[]}[]} + */ +export function noInstallEntries(text, readAction) { + const actions = jobLocalActions(text); + const found = []; + for (const [job, steps] of jobSteps(text)) { + if (installsDependencies(steps)) continue; + if ((actions.get(job) ?? []).some(dir => actionInstalls(dir, readAction))) continue; + const entries = nodeEntries(steps); + if (entries.length > 0) found.push({ job, entries }); + } + return found; +} + +/** Record a node's module specifier, if it names one, as followable or not. */ +function collectSpecifier(node, source, found) { + const declares = ts.isImportDeclaration(node) || ts.isExportDeclaration(node); + if (declares && node.moduleSpecifier !== undefined) { + if (ts.isStringLiteral(node.moduleSpecifier)) found.literal.push(node.moduleSpecifier.text); + return; + } + const dynamic = + ts.isCallExpression(node) && node.expression.kind === ts.SyntaxKind.ImportKeyword; + if (!dynamic) return; + const [argument] = node.arguments; + if (argument !== undefined && ts.isStringLiteralLike(argument)) found.literal.push(argument.text); + else found.unresolvable.push(node.getText(source)); +} + +/** + * Every module specifier a file names: the literal ones a static walk can follow, and the calls to + * `import()` whose argument it cannot. + * + * @param {string} fileName + * @param {string} text + * @returns {{literal: string[], unresolvable: string[]}} + */ +export function moduleSpecifiers(fileName, text) { + const source = ts.createSourceFile( + fileName, + text, + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.JS + ); + const found = { literal: [], unresolvable: [] }; + const visit = node => { + collectSpecifier(node, source, found); + ts.forEachChild(node, visit); + }; + visit(source); + return found; +} + +/** A file's contents, or null after recording that it could not be read. */ +function readOrReport(file, read, offenders) { + try { + return read(file); + } catch { + offenders.push(`${file}: cannot be read`); + return null; + } +} + +/** + * What an entry's static import graph reaches that Node alone cannot load. + * + * Relative specifiers are followed through `read`; a bare one must be a builtin. Whatever the walk + * cannot settle is reported rather than passed over — a file that cannot be read, or an `import()` + * with no literal argument — because an unexamined import and a clean one look identical otherwise. + * + * @param {string} entry repository-relative path of the file a job starts + * @param {(path: string) => string} read a file's contents, throwing when it does not exist + * @returns {{files: string[], offenders: string[]}} + */ +export function nonBuiltinImports(entry, read) { + const files = new Set(); + const offenders = []; + const walk = file => { + if (files.has(file)) return; + files.add(file); + const text = readOrReport(file, read, offenders); + if (text === null) return; + const { literal, unresolvable } = moduleSpecifiers(file, text); + for (const call of unresolvable) offenders.push(`${file}: ${call} names no literal module`); + for (const specifier of literal) { + if (specifier.startsWith(".")) walk(normalize(join(dirname(file), specifier))); + else if (!isBuiltin(specifier)) offenders.push(`${file} imports ${specifier}`); + } + }; + walk(entry); + return { files: [...files], offenders }; +} diff --git a/scripts/no-install-jobs.test.mjs b/scripts/no-install-jobs.test.mjs new file mode 100644 index 0000000000..dd1417ea07 --- /dev/null +++ b/scripts/no-install-jobs.test.mjs @@ -0,0 +1,197 @@ +/** + * Every script a workflow runs without installing dependencies imports only Node builtins. + * + * The unit cases pin the reader and the walker against the ways each could pass while broken. The + * last block applies them to the real workflows, which is the check itself. + * + * @module no-install-jobs.test + */ +import { readFileSync, readdirSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "vitest"; + +import { + installsDependencies, + moduleSpecifiers, + noInstallEntries, + nodeEntries, + nonBuiltinImports, +} from "./no-install-jobs.mjs"; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +const ROOT = path.join(HERE, ".."); + +const step = block => ({ name: null, block }); + +describe("installsDependencies and nodeEntries", () => { + it("recognises an install in the forms a workflow writes it", () => { + for (const block of ["pnpm install --frozen-lockfile", "npm ci", "cd app && pnpm i"]) { + expect(installsDependencies([step(block)]), block).toBe(true); + } + }); + + it("does not read a shell comment or a longer command as an install", () => { + expect(installsDependencies([step("# pnpm install runs in the other job")])).toBe(false); + expect(installsDependencies([step("pnpm install-completion")])).toBe(false); + }); + + it("reads the file each node invocation starts, flags included, comments excluded", () => { + const entries = nodeEntries([ + step("node scripts/a.mjs"), + step("node --enable-source-maps scripts/b.mjs --json\n# node scripts/c.mjs"), + ]); + + expect(entries).toEqual(["scripts/a.mjs", "scripts/b.mjs"]); + }); +}); + +describe("noInstallEntries", () => { + const workflow = [ + "jobs:", + " build:", + " steps:", + " - run: pnpm install --frozen-lockfile", + " - run: node scripts/build.mjs", + " integration:", + " steps:", + " - uses: ./.github/actions/setup", + " - run: node scripts/integration.mjs", + " gate:", + " # No pnpm install here: the verdict reads nothing it would provide.", + " steps:", + " - run: node scripts/gate.mjs", + "", + ].join("\n"); + const installingAction = "runs:\n steps:\n - run: pnpm install --frozen-lockfile\n"; + + it("returns only jobs that neither install nor call a local action that does", () => { + const found = noInstallEntries(workflow, dir => + dir === ".github/actions/setup" ? installingAction : null + ); + + expect(found).toEqual([{ job: "gate", entries: ["scripts/gate.mjs"] }]); + }); + + it("checks a job whose action it cannot read, rather than excusing it", () => { + const found = noInstallEntries(workflow, () => null); + + expect(found.map(entry => entry.job)).toEqual(["integration", "gate"]); + }); +}); + +describe("moduleSpecifiers", () => { + it("does NOT read an import quoted in a comment or a string", () => { + const { literal } = moduleSpecifiers( + "f.mjs", + '// import { compile } from "@mdx-js/mdx";\nconst s = \'import x from "js-yaml"\';\nimport { readFileSync } from "node:fs";\n' + ); + + expect(literal).toEqual(["node:fs"]); + }); + + it("reads re-exports, side-effect imports and a literal dynamic import", () => { + const { literal } = moduleSpecifiers( + "f.mjs", + 'export { a } from "./a.mjs";\nexport * from "pkg-a";\nimport "pkg-b";\nawait import("pkg-c");\n' + ); + + expect(literal).toEqual(["./a.mjs", "pkg-a", "pkg-b", "pkg-c"]); + }); + + it("reports an import() it cannot follow instead of dropping it", () => { + const found = moduleSpecifiers("f.mjs", 'const name = "js-yaml";\nawait import(name);\n'); + + expect(found.literal).toEqual([]); + expect(found.unresolvable).toEqual(["import(name)"]); + }); +}); + +describe("nonBuiltinImports", () => { + const files = { + "scripts/entry.mjs": 'import { x } from "./middle.mjs";\nimport { join } from "node:path";\n', + "scripts/middle.mjs": 'export { x } from "./leaf.mjs";\n', + "scripts/leaf.mjs": 'import yaml from "js-yaml";\nexport const x = yaml;\n', + "scripts/clean.mjs": 'import { readFileSync } from "node:fs";\nimport { b } from "./cycle.mjs";\n', + "scripts/cycle.mjs": 'import { readFileSync } from "fs";\nexport { b } from "./clean.mjs";\n', + }; + const read = file => { + if (!(file in files)) throw new Error(`no such file: ${file}`); + return files[file]; + }; + + it("finds an npm import two files down, through a re-export", () => { + const { offenders } = nonBuiltinImports("scripts/entry.mjs", read); + + expect(offenders).toEqual(["scripts/leaf.mjs imports js-yaml"]); + }); + + it("passes a graph of builtins, prefixed or not, and terminates on a cycle", () => { + const { files: reached, offenders } = nonBuiltinImports("scripts/clean.mjs", read); + + expect(offenders).toEqual([]); + expect(reached.sort()).toEqual(["scripts/clean.mjs", "scripts/cycle.mjs"]); + }); + + it("reports a relative import it cannot read", () => { + const { offenders } = nonBuiltinImports("scripts/absent.mjs", read); + + expect(offenders).toEqual(["scripts/absent.mjs: cannot be read"]); + }); +}); + +describe("the real workflows", () => { + const workflowDir = path.join(ROOT, ".github", "workflows"); + const workflows = readdirSync(workflowDir).filter(name => /\.ya?ml$/.test(name)).sort(); + const readRepo = relative => readFileSync(path.join(ROOT, relative), "utf8"); + const readAction = dir => { + for (const name of ["action.yml", "action.yaml"]) { + try { + return readRepo(path.join(dir, name)); + } catch { + // Try the other spelling. + } + } + return null; + }; + const population = workflows.flatMap(file => + noInstallEntries(readRepo(path.join(".github", "workflows", file)), readAction).flatMap( + ({ job, entries }) => entries.map(entry => ({ where: `${file} ${job}`, entry })) + ) + ); + + /** + * A FLOOR, not the population. Every assertion below iterates what the workflows actually run; + * this exists so a reader that silently found nothing cannot satisfy them by leaving nothing to + * iterate. + */ + const AT_LEAST = ["scripts/ci-gate.mjs", "scripts/check-repo-metadata.mjs"]; + + it("finds the jobs known to run without installing", () => { + const entries = population.map(item => item.entry); + + for (const expected of AT_LEAST) expect(entries, expected).toContain(expected); + }); + + it("does not mistake an installing job for one that runs without installing", () => { + // The other direction. A reader that stopped recognising installs would put every job's + // scripts in the population, and the check would start failing on correct files. + const entries = population.map(item => item.entry); + + expect(entries).not.toContain("scripts/release/check-changesets.mjs"); + }); + + it.each(population.map(item => [item.where, item.entry]))( + "%s starts %s, which imports only Node builtins", + (_where, entry) => { + const { offenders } = nonBuiltinImports(entry, readRepo); + + expect( + offenders, + "this job installs no dependencies, so Node alone loads this whole import graph. " + + "Import a dependency-free module instead, or give the job an install step." + ).toEqual([]); + } + ); +}); diff --git a/scripts/workflow-run-blocks.mjs b/scripts/workflow-run-blocks.mjs index f1c866057d..ac9d3bf8d5 100644 --- a/scripts/workflow-run-blocks.mjs +++ b/scripts/workflow-run-blocks.mjs @@ -99,7 +99,11 @@ function applyBoundary(state, line) { * inline command as running nothing. */ function runScript(lines, index) { - const run = /^(\s*)run:(\s*\|)?(.*)$/.exec(lines[index]); + // The key may share its line with the list item that opens the step — `- run: pnpm test` — and + // the first group then carries the dash as well as the indentation. Its length is therefore the + // column `run:` STARTS at, which is what bounds a block scalar under it: measured from the dash + // instead, a `- run: |` block would swallow the step's own `env:`, which sits at that same column. + const run = /^(\s*(?:-\s+)?)run:(\s*\|)?(.*)$/.exec(lines[index]); if (run === null) return null; if (run[2] !== undefined) return blockBody(lines, index + 1, run[1].length); return run[3].trim() === "" ? null : run[3].trim(); @@ -243,3 +247,57 @@ export function jobTimeouts(text) { }); return timeouts; } + +/** + * Each job's steps, keyed by job id, in file order. + * + * The per-job view of {@link workflowSteps}, for a check that judges a job by what ITS OWN steps + * run — whether it installs dependencies, which scripts it starts. The file-wide list says what + * runs but not where. {@link jobsMentioning} says where, but matches every line, comments + * included, so a job whose comment explains that it does not run `pnpm install` would read as + * running it. + * + * Built on the same walk as {@link jobIds}, so a job's boundary means the same thing here, and + * each job's lines go through {@link workflowSteps} unchanged, so a step's script does too. Every + * declared job is present, an empty list for one with no scripts, because a caller iterating the + * answer must not mistake a job it was never told about for a job with nothing to check. + * + * @param {string} text the workflow file's contents + * @returns {Map} job id to its steps' scripts + */ +export function jobSteps(text) { + const linesByJob = new Map(); + walkJobs(text, (job, line) => { + if (job === null) return; + if (!linesByJob.has(job)) linesByJob.set(job, []); + linesByJob.get(job).push(line); + }); + + const steps = new Map(); + for (const [job, lines] of linesByJob) { + steps.set(job, workflowSteps(lines.join("\n"))); + } + return steps; +} + +/** + * The composite actions from this repository that each job uses, keyed by job id. + * + * A job can install dependencies without one `run:` of its own saying so, by calling a local + * action that does. Only `uses: ./…` is read — an action from elsewhere is not a file here to + * inspect — and a comment naming an action is not a step that runs it, which the pattern's anchor + * already excludes. + * + * @param {string} text the workflow file's contents + * @returns {Map} job id to each local action's repository-relative directory + */ +export function jobLocalActions(text) { + const actions = new Map(); + walkJobs(text, (job, line) => { + if (job === null) return; + if (!actions.has(job)) actions.set(job, []); + const local = /^\s*(?:-\s*)?uses:\s*\.\/([^\s#]+)/.exec(line); + if (local !== null) actions.get(job).push(local[1]); + }); + return actions; +} diff --git a/scripts/workflow-run-blocks.test.mjs b/scripts/workflow-run-blocks.test.mjs index f8f6b6ff5f..dbb1101802 100644 --- a/scripts/workflow-run-blocks.test.mjs +++ b/scripts/workflow-run-blocks.test.mjs @@ -23,7 +23,13 @@ */ import { describe, expect, it } from "vitest"; -import { jobIds, jobTimeouts, workflowSteps } from "./workflow-run-blocks.mjs"; +import { + jobIds, + jobLocalActions, + jobSteps, + jobTimeouts, + workflowSteps, +} from "./workflow-run-blocks.mjs"; /** The script of the FIRST step with this name, or undefined. */ function blockOf(steps, name) { @@ -165,6 +171,36 @@ describe("workflowSteps", () => { expect(blocks).toEqual([" bare-thing", " wrapped-thing"]); }); + it("reads a run key written on the list item's own line", () => { + // `- run: pnpm test` is a whole step, and a common one. A reader anchored on a line that + // STARTS with `run:` sees nothing here, so a job written this way reads as running nothing. + const steps = workflowSteps( + [" steps:", " - run: pnpm install --frozen-lockfile", " - run: node scripts/x.mjs"].join("\n") + ); + + expect(steps.map(step => step.block)).toEqual([ + "pnpm install --frozen-lockfile", + "node scripts/x.mjs", + ]); + }); + + it("bounds a same-line `- run: |` block at the key's column, not the dash's", () => { + // Measured from the dash, the block would take in `env:` and its value, because the step's + // other keys sit exactly one level in from the dash. + const steps = workflowSteps( + [ + " steps:", + " - run: |", + " node scripts/x.mjs", + " env:", + " TOKEN: abc", + "", + ].join("\n") + ); + + expect(steps.map(step => step.block)).toEqual([" node scripts/x.mjs"]); + }); + it("ignores a run key that no step has opened", () => { // A `run:` before any list item is not a step's script. Recording it would // invent a step the workflow does not have. @@ -232,3 +268,75 @@ describe("jobIds and jobTimeouts", () => { expect(jobIds(workflow)).not.toContain("push"); }); }); + +describe("jobSteps and jobLocalActions", () => { + const workflow = [ + "name: fixture", + "on: push", + "jobs:", + " build:", + " runs-on: ubuntu-latest", + " steps:", + " - name: Install", + " run: pnpm install --frozen-lockfile", + " - name: Test", + " run: pnpm test", + " gate:", + " runs-on: ubuntu-latest", + " # Unlike build, which runs pnpm install, this reads nothing it would need.", + " steps:", + " - name: Verdict", + " run: node scripts/gate.mjs", + " empty:", + " runs-on: ubuntu-latest", + "", + ].join("\n"); + + it("attributes each step's script to the job that runs it", () => { + const steps = jobSteps(workflow); + + expect(steps.get("build").map(step => step.block)).toEqual([ + "pnpm install --frozen-lockfile", + "pnpm test", + ]); + expect(steps.get("gate")).toEqual([ + { name: "Verdict", block: "node scripts/gate.mjs" }, + ]); + }); + + it("does NOT credit a job with a command its comment merely quotes", () => { + // The reason this reader exists: `jobsMentioning(text, "pnpm install")` names `gate`, because + // its comment says the words. A check asking whether gate installs must be told it does not. + const blocks = jobSteps(workflow).get("gate").map(step => step.block); + + expect(blocks.some(block => block.includes("pnpm install"))).toBe(false); + }); + + it("lists every declared job, an empty list for one with no scripts", () => { + const steps = jobSteps(workflow); + + expect([...steps.keys()]).toEqual(jobIds(workflow)); + expect(steps.get("empty")).toEqual([]); + }); + + it("reads a local action and skips an external one and a commented one", () => { + const actions = jobLocalActions( + [ + "jobs:", + " integration:", + " steps:", + " - uses: actions/checkout@0123456789abcdef # v7", + " # uses: ./.github/actions/retired-setup", + " - name: Setup", + " uses: ./.github/actions/integration-setup # pinned here", + " lint:", + " steps:", + " - run: pnpm lint", + "", + ].join("\n") + ); + + expect(actions.get("integration")).toEqual([".github/actions/integration-setup"]); + expect(actions.get("lint")).toEqual([]); + }); +}); From 54c194808a9c85eaee4db997f337c61c466ee0b5 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Sun, 13 Sep 2026 18:09:36 +0300 Subject: [PATCH 2/5] fix(root): the no-install guard reads what github runs, in order, and refuses the rest The guard now reads workflows and local actions with js-yaml, the parser github-yaml-parses.test.mjs already applies to the same files, so a folded, quoted, flow-style or continued run: is the string GitHub runs. - Steps are read in run order and only up to the job's first install that can release it: unconditional, in the repository root, doing nothing else. - Composite actions are expanded in place; a JavaScript action's pre and post are placed at the job's start and end. - Scripts are split into commands by a new shell-commands.mjs, and a node command's arguments are read with Node's grammar, so an option's value is not taken for the script and a preloaded module is walked too. - working-directory is resolved from the step, the job or the workflow. - The module walk follows require, require.resolve, createRequire bindings and import.meta.resolve, resolving a relative require as require does. - Whatever the reader cannot settle is refused with the step and a reason. The text reader's additions are reverted: nothing uses them now. --- scripts/no-install-jobs.mjs | 756 +++++++++++++++++++++++---- scripts/no-install-jobs.test.mjs | 553 ++++++++++++++++---- scripts/shell-commands.mjs | 274 ++++++++++ scripts/shell-commands.test.mjs | 119 +++++ scripts/workflow-run-blocks.mjs | 60 +-- scripts/workflow-run-blocks.test.mjs | 110 +--- 6 files changed, 1522 insertions(+), 350 deletions(-) create mode 100644 scripts/shell-commands.mjs create mode 100644 scripts/shell-commands.test.mjs diff --git a/scripts/no-install-jobs.mjs b/scripts/no-install-jobs.mjs index c05ea00983..514060ee2d 100644 --- a/scripts/no-install-jobs.mjs +++ b/scripts/no-install-jobs.mjs @@ -1,20 +1,46 @@ /** - * Which scripts a workflow runs WITHOUT installing dependencies, and whether each one can. - * - * A job that never installs runs straight after checkout with Node and nothing else, so every module - * a script it starts imports — all the way down that script's import graph — must be a Node builtin. - * Two jobs are like that: the CI gate, which has to report even when the install every other job - * depends on is what failed, and the scheduled repository-metadata check, which observes repository - * settings no file check can see. - * - * Neither property is visible to a pull request. A scheduled job never runs on one, and pull-request - * CI installs dependencies before anything else, so an npm import added three files away from either - * job passes every check on its own pull request and fails afterwards, on `main`, attributed to - * whatever commit happens to be at the tip when the job next runs. - * - * The population is DERIVED from the workflow files rather than listed: a job that stops installing - * is covered the day it changes, and one that starts installing is released from the rule the same - * day. Imports are read with TypeScript's parser rather than a pattern, because a pattern reads an + * The programs a workflow starts before it has installed any dependencies, and whether Node alone + * can load each one. + * + * Until a job's install has run, it has Node and a checkout and nothing more, so every module a + * program it starts imports — all the way down that program's import graph — must be a Node + * builtin. Some jobs never install at all: the CI gate, which has to report even when the install + * every other job depends on is what failed, and the scheduled repository-metadata check. + * + * A scheduled job never runs on a pull request, and a pull request's CI installs dependencies + * before anything else, so an npm import added three files away from such a program passes every + * check on its pull request and fails afterwards on `main`, attributed to whatever commit is at the + * tip when the job next runs. + * + * ## What is read, and how + * + * Workflows and the local actions they use are read with a YAML parser, so a folded, quoted, + * flow-style or continued `run:` is the same string here that it is to GitHub. Steps are taken in + * the order they run: a composite action's steps in place of the step that uses it, a JavaScript + * action's `pre` as the job starts and its `post` as it ends. + * + * A job leaves the dependency-free state at its first step that installs the repository's + * dependencies and does nothing else, unconditionally, in the repository root. Everything before + * that step is read, and nothing after it. + * + * Each script read is split into commands by `shell-commands.mjs`. A `node` command's arguments + * are read with Node's own grammar, so an option's value is not taken for the script, and a module + * an option preloads is walked like the script. A shell script in the repository, named by a + * literal path, is read in turn. + * + * ## What is refused + * + * This guards `main` against a failure nothing before the merge can see, so whatever it cannot + * settle it refuses, naming the step and the reason, rather than passing over it: a `node` command + * with an argument that is not literal, an option it does not know, inline code or no script file; + * the word `node` anywhere else; a package manager running scripts or installed binaries; a + * directory change before a `node` command; `NODE_OPTIONS`; a program named only at run time, + * unless the caller maps it to the repository file it is a copy of; an action it cannot read; and a + * working directory that is not a fixed path inside the repository. + * + * Outside it: the code a remote action brings with it, which is that action's to load. + * + * Imports are read with TypeScript's parser rather than a pattern, because a pattern reads an * import quoted in a comment or a string as a real one, and a guard that fails on a correct file * teaches people to delete it. * @@ -22,117 +48,628 @@ */ import { isBuiltin } from "node:module"; -import { dirname, join, normalize } from "node:path"; +import { posix } from "node:path"; +import { load } from "js-yaml"; import ts from "typescript"; -import { - jobLocalActions, - jobSteps, - workflowSteps, -} from "./workflow-run-blocks.mjs"; +import { shellCommands } from "./shell-commands.mjs"; + +/** + * @typedef {object} Repository + * @property {(path: string) => string | null} readFile a file's contents, or null when it is absent + * @property {Record} [copies] programs a workflow stages at run time from one of + * the repository's own files, keyed by the word that runs them, valued by that file's path + */ + +/** + * @typedef {object} Start + * @property {string} where the job and step, for a message a person can act on + * @property {string} entry the repository path of the script Node starts + * @property {({path: string} | {package: string})[]} preloads what Node loads before the script + */ + +/** + * @typedef {object} Refusal + * @property {string} where + * @property {string} reason + */ + +/** + * @typedef {{kind: "install", where: string} + * | {kind: "script", where: string, script: string, cwd: string} + * | ({kind: "start"} & Start) + * | ({kind: "refusal"} & Refusal)} JobEvent + */ + +/** A package manager, and the subcommands with which it installs a project's dependencies. */ +const INSTALLER = /^(?:pnpm|npm|yarn)$/; +const INSTALL = /^(?:install|ci|i)$/; -/** A command that installs a project's dependencies. */ -const INSTALL = /(^|[\s;&|(])(pnpm|npm|yarn)\s+(install|ci|i)(\s|$)/; +/** Install options whose value is the next word. */ +const INSTALL_VALUES = new Set(["--filter", "-F"]); -/** A `node` invocation, capturing the file it starts. */ -const NODE_ENTRY = /\bnode\s+(?:-\S+\s+)*(\S+?\.(?:mjs|cjs|js))\b/g; +/** Install options that put what they install somewhere other than the repository root. */ +const INSTALL_ELSEWHERE = /^(?:--dir|-C|--prefix|--cwd|-g|--global|--location)(?:=|$)/; -/** A step script's lines that execute, without its shell comments. */ -function executableLines(block) { - return block.split("\n").filter(line => !line.trim().startsWith("#")); +const NODE_OPTIONS_REASON = + "sets NODE_OPTIONS, which changes what every node command loads, and this reader does not follow it"; + +/** + * Each job's steps as the events this check reads, in the order they run. + * + * @param {string} text a workflow file's contents + * @param {Repository} repo + * @returns {Map} job id to its events + */ +export function jobSequences(text, repo) { + const workflow = load(text) ?? {}; + const sequences = new Map(); + for (const [id, job] of Object.entries(workflow.jobs ?? {})) { + const sequence = { before: [], steps: [], after: [] }; + const defaults = + job.defaults?.run?.["working-directory"] ?? workflow.defaults?.run?.["working-directory"]; + const scope = { where: id, defaults, env: [workflow.env, job.env], actions: [], repo, sequence }; + readSteps(job.steps ?? [], scope); + sequences.set(id, [...sequence.before, ...sequence.steps, ...sequence.after]); + } + return sequences; +} + +function readSteps(steps, scope) { + steps.forEach((step, index) => { + const label = step.name ? `step ${index + 1} (${step.name})` : `step ${index + 1}`; + readStep(step, { ...scope, where: `${scope.where} › ${label}` }); + }); +} + +function readStep(step, scope) { + if (typeof step.run === "string") readRunStep(step, scope); + else if (typeof step.uses === "string" && step.uses.startsWith("./")) { + readLocalAction(step.uses, scope); + } +} + +function refusal(where, reason) { + return { kind: "refusal", where, reason }; +} + +function readRunStep(step, scope) { + const { steps } = scope.sequence; + const cwd = workingDirectory(step, scope); + if (isInstall(step)) steps.push({ kind: "install", where: scope.where }); + else if ("refusal" in cwd) steps.push(refusal(scope.where, cwd.refusal)); + else if ([...scope.env, step.env].some(env => env != null && Object.hasOwn(env, "NODE_OPTIONS"))) { + steps.push(refusal(scope.where, NODE_OPTIONS_REASON)); + } else if (/\bnode\b/.test(step.shell ?? "")) { + steps.push(refusal(scope.where, "runs its script as inline Node code; move it into a file")); + } else { + const script = withKnownPaths(step.run, scope); + steps.push({ kind: "script", where: scope.where, script, cwd: cwd.path }); + } } /** - * Whether any of these step scripts installs dependencies. + * Whether a step installs the repository's dependencies, unconditionally, and does nothing else. * - * @param {{block: string}[]} steps - * @returns {boolean} + * Narrow on purpose. A condition, a tolerated failure, another directory, a package named on the + * command line and a second command each describe a step after which the dependencies may still be + * absent, so each keeps the job in the state this check reads rather than releasing it. */ -export function installsDependencies(steps) { - return steps.some(step => - executableLines(step.block).some(line => INSTALL.test(line)) - ); +function isInstall(step) { + if (step.if !== undefined || step["continue-on-error"]) return false; + if (step["working-directory"] !== undefined) return false; + const commands = shellCommands(step.run); + return commands.length === 1 && installsHere(commands[0].words); +} + +function installsHere(words) { + if (!words.every(word => word.literal)) return false; + const [tool = "", subcommand = "", ...options] = words.map(word => word.text); + if (!INSTALLER.test(tool) || !INSTALL.test(subcommand)) return false; + for (let i = 0; i < options.length; i += 1) { + if (INSTALL_ELSEWHERE.test(options[i])) return false; + if (INSTALL_VALUES.has(options[i])) i += 1; + else if (!options[i].startsWith("-")) return false; + } + return true; } /** - * Every file these step scripts start with `node`, in order, once each. + * The repository path a step's script runs in. * - * @param {{block: string}[]} steps - * @returns {string[]} + * GitHub documents a composite action step's own `working-directory`, but not whether a job's + * default reaches one, so a composite step naming none under a job that sets one is refused rather + * than resolved either way. + * + * @returns {{path: string} | {refusal: string}} + */ +function workingDirectory(step, scope) { + const own = step["working-directory"]; + const inAction = scope.actions.length > 0; + if (own === undefined && inAction && scope.defaults !== undefined) { + return { + refusal: + "is a composite action step without a working-directory of its own, under a job that " + + "sets one, and which of the two it runs in is undocumented", + }; + } + return repositoryPath(own ?? (inAction ? undefined : scope.defaults) ?? ".", ".", "directory"); +} + +/** + * A path inside the repository, from one written relative to `base`, or why it is not one. + * + * @returns {{path: string} | {refusal: string}} */ -export function nodeEntries(steps) { - const entries = new Set(); - for (const step of steps) { - for (const line of executableLines(step.block)) { - for (const match of line.matchAll(NODE_ENTRY)) entries.add(match[1]); +function repositoryPath(value, base, what) { + if (typeof value !== "string" || value.includes("$")) { + return { refusal: `names a ${what} that is not in the text: ${value}` }; + } + const path = posix.normalize(posix.join(base, value)); + if (posix.isAbsolute(value) || path === ".." || path.startsWith("../")) { + return { refusal: `names a ${what} outside the repository: ${value}` }; + } + return { path }; +} + +/** The two runner paths a script can name whose value this reader knows. */ +function withKnownPaths(script, scope) { + const action = scope.actions.at(-1); + const workspace = /\$\{\{\s*github\.workspace\s*\}\}|\$\{GITHUB_WORKSPACE\}|\$GITHUB_WORKSPACE\b/g; + const actionPath = /\$\{\{\s*github\.action_path\s*\}\}|\$\{GITHUB_ACTION_PATH\}|\$GITHUB_ACTION_PATH\b/g; + const text = script.replace(workspace, "."); + return action === undefined ? text : text.replace(actionPath, action); +} + +/** + * A step using an action from this repository, read in its place. + * + * A composite action's steps are read as though the job had written them, so an install inside one + * ends the dependency-free state and a script started inside one is checked. A JavaScript action's + * `pre` runs as the job starts and its `post` as the job ends, so they are placed there. + */ +function readLocalAction(uses, scope) { + const dir = posix.normalize(uses).replace(/\/$/, ""); + const { steps } = scope.sequence; + if (scope.actions.includes(dir)) { + steps.push(refusal(scope.where, `uses ./${dir} inside itself`)); + return; + } + const manifest = readManifest(dir, scope.repo); + if (manifest === null) { + steps.push(refusal(scope.where, `uses ./${dir}, whose action.yml cannot be read`)); + return; + } + const runs = manifest.runs ?? {}; + const inner = { ...scope, where: `${scope.where} › ./${dir}`, actions: [...scope.actions, dir] }; + if (runs.using === "composite") readSteps(runs.steps ?? [], inner); + else if (/^node\d+$/.test(String(runs.using))) readJavaScriptAction(dir, runs, inner); +} + +function readManifest(dir, repo) { + for (const name of ["action.yml", "action.yaml"]) { + const text = repo.readFile(posix.join(dir, name)); + if (text === null) continue; + try { + return load(text) ?? null; + } catch { + // A manifest that does not parse is refused by the caller, as one that cannot be read is. + return null; } } - return [...entries]; + return null; } -/** Whether a local composite action installs dependencies. Unreadable counts as not installing. */ -function actionInstalls(directory, readAction) { - const text = readAction(directory); - return text !== null && installsDependencies(workflowSteps(text)); +function readJavaScriptAction(dir, runs, scope) { + const { before, steps, after } = scope.sequence; + for (const [key, events] of [["pre", before], ["main", steps], ["post", after]]) { + if (typeof runs[key] !== "string") continue; + const where = `${scope.where} (${key})`; + const entry = repositoryPath(runs[key], dir, "script"); + events.push( + "refusal" in entry + ? refusal(where, entry.refusal) + : { kind: "start", where, entry: entry.path, preloads: [] } + ); + } } +/** Words that come before the program a command runs: reserved words and the wrappers that exec it. */ +const PREFIXES = new Set([ + "if", "then", "elif", "else", "while", "until", "do", "!", "{", + "time", "exec", "nohup", "command", "builtin", "env", "sudo", "nice", +]); + +/** An assignment word, `NAME=value`, whatever its value. */ +const ASSIGNMENT = /^[A-Za-z_][A-Za-z0-9_]*=/; + +/** Node's program name, alone or at the end of a path. */ +const NODE_PROGRAM = /^(?:.*\/)?(?:node|nodejs)$/; + +/** Node's name as a word inside some other text. */ +const NODE_NAMED = /(?:^|[^\w$.-])(?:node|nodejs)(?![\w.-])/; + +const DIRECTORY_CHANGES = new Set(["cd", "pushd", "popd"]); +const SHELLS = new Set(["bash", "sh", "zsh", "dash", "source", "."]); +const PACKAGE_RUNNERS = new Set(["npx", "pnpx", "bunx"]); +const PACKAGE_MANAGERS = new Set(["pnpm", "npm", "yarn", "corepack"]); + +/** Package-manager subcommands that run neither the repository's scripts nor an installed package. */ +const INERT_SUBCOMMANDS = new Set([ + "install", "i", "ci", "add", "audit", "view", "info", "show", "whoami", "config", "get", "set", + "ls", "list", "why", "outdated", "store", "bin", "root", "prefix", "cache", "dist-tag", "ping", + "help", "enable", "prepare", "use", +]); + +/** Options a package manager takes before its subcommand whose value is the next word. */ +const MANAGER_VALUES = new Set(["--filter", "-F", "--dir", "-C", "--prefix", "--cwd", "--workspace"]); + +/** Options that make Node run something other than a script file. */ +const NODE_RUNS_OTHER_CODE = new Set([ + "-e", "--eval", "-p", "--print", "-", "-i", "--interactive", "--run", "--test", +]); + +/** Options after which Node prints something and exits without running anything. */ +const NODE_EXITS = new Set(["-v", "--version", "-h", "--help", "--v8-options"]); + +/** Options whose value is a module Node loads before the script. */ +const NODE_PRELOADS = new Set(["-r", "--require", "--import", "--loader", "--experimental-loader"]); + /** - * The scripts each job in a workflow starts without having installed dependencies. + * Node's own options that take a value, which may be written as the next word. * - * An action this cannot read is treated as NOT installing, so its job is checked rather than - * excused: a guard that skips what it could not examine reports exactly as a clean result does. + * `node --conditions development x.mjs` runs `x.mjs` with the condition set; so does `-C`. V8's + * options, `--max-old-space-size` among them, take a value only after `=`, and an option written + * with `=` is accepted whatever its name. + */ +const NODE_TAKES_VALUE = new Set([ + "-C", "--conditions", "--disable-warning", "--env-file", "--env-file-if-exists", "--input-type", + "--redirect-warnings", "--diagnostic-dir", "--title", "--watch-path", "--report-dir", + "--report-directory", "--report-filename", "--cpu-prof-dir", "--heap-prof-dir", + "--unhandled-rejections", "--dns-result-order", "--localstorage-file", "--openssl-config", + "--icu-data-dir", +]); + +/** Node's options that take no value. */ +const NODE_FLAGS = new Set([ + "-c", "--check", "--enable-source-maps", "--expose-gc", "--frozen-intrinsics", "--no-addons", + "--no-deprecation", "--no-warnings", "--pending-deprecation", "--preserve-symlinks", + "--preserve-symlinks-main", "--throw-deprecation", "--trace-deprecation", "--trace-exit", + "--trace-uncaught", "--trace-warnings", "--watch", "--abort-on-uncaught-exception", + "--experimental-vm-modules", "--experimental-strip-types", "--no-experimental-strip-types", + "--experimental-transform-types", "--experimental-detect-module", + "--no-experimental-detect-module", "--experimental-require-module", + "--no-experimental-require-module", +]); + +const MOVED = "after changing directory, which this reader does not follow; give the step a working-directory instead"; + +/** + * Every program a workflow's jobs start before installing dependencies, and every place this + * reader could not settle. * - * @param {string} text the workflow file's contents - * @param {(directory: string) => string|null} readAction a local action's definition, or null - * @returns {{job: string, entries: string[]}[]} + * @param {string} text a workflow file's contents + * @param {Repository} repo + * @returns {{starts: Start[], refusals: Refusal[]}} */ -export function noInstallEntries(text, readAction) { - const actions = jobLocalActions(text); - const found = []; - for (const [job, steps] of jobSteps(text)) { - if (installsDependencies(steps)) continue; - if ((actions.get(job) ?? []).some(dir => actionInstalls(dir, readAction))) continue; - const entries = nodeEntries(steps); - if (entries.length > 0) found.push({ job, entries }); +export function dependencyFreeStarts(text, repo) { + const found = { starts: [], refusals: [] }; + for (const sequence of jobSequences(text, repo).values()) { + const install = sequence.findIndex(event => event.kind === "install"); + for (const event of install === -1 ? sequence : sequence.slice(0, install)) { + if (event.kind === "script") readScript(event, { repo, found, following: [] }); + else if (event.kind === "start") found.starts.push(startOf(event)); + else if (event.kind === "refusal") found.refusals.push({ where: event.where, reason: event.reason }); + } } return found; } +function startOf({ where, entry, preloads }) { + return { where, entry, preloads }; +} + +function refuse(state, reason) { + state.found.refusals.push({ where: state.where, reason }); +} + +/** The programs a shell script starts, in the order it starts them. */ +function readScript({ where, script, cwd }, context) { + const state = { movedDirectory: false, ...context, where, cwd }; + for (const command of shellCommands(script)) readCommand(command, state); +} + +function readCommand(command, state) { + if (command.words.some(word => /^NODE_OPTIONS(?:=|$)/.test(word.text))) { + refuse(state, NODE_OPTIONS_REASON); + return; + } + const words = programWords(command.words); + const [program] = words; + if (program !== undefined && !program.literal) readRuntimeProgram(command, program, state); + else if (program === undefined) residual(command, state); + else if (NODE_PROGRAM.test(program.text)) readNodeCommand(words, state); + else if (DIRECTORY_CHANGES.has(program.text)) state.movedDirectory = true; + else if (SHELLS.has(program.text)) readShellCommand(command, words, state); + else if (PACKAGE_RUNNERS.has(program.text) || PACKAGE_MANAGERS.has(program.text)) { + readPackageManager(command, words, state); + } else if (program.text.includes("/")) readPathCommand(command, words, state); + else residual(command, state); +} + +/** A command's words from the program it runs: assignments, reserved words and wrappers removed. */ +function programWords(words) { + let i = 0; + while ( + i < words.length && + (ASSIGNMENT.test(words[i].text) || (words[i].literal && PREFIXES.has(words[i].text))) + ) { + i += 1; + } + return words.slice(i); +} + +function readNodeCommand(words, state) { + if (state.movedDirectory) return refuse(state, `starts Node ${MOVED}`); + const parsed = nodeArguments(words.slice(1)); + if ("exits" in parsed) return undefined; + if ("refusal" in parsed) return refuse(state, parsed.refusal); + const entry = repositoryPath(parsed.entry, state.cwd, "script"); + if ("refusal" in entry) return refuse(state, entry.refusal); + const preloads = []; + for (const specifier of parsed.preloads) { + const path = /^[./]/.test(specifier) ? repositoryPath(specifier, state.cwd, "preload") : null; + if (path !== null && "refusal" in path) return refuse(state, path.refusal); + preloads.push(path === null ? { package: specifier } : { path: path.path }); + } + state.found.starts.push({ where: state.where, entry: entry.path, preloads }); + return undefined; +} + +/** + * The script a `node` command starts and the modules its options preload, read with Node's + * grammar: `node [options] [--]