From d1d6cdd4f082f7e2f7d21989b1fe4469f3b227cc Mon Sep 17 00:00:00 2001 From: clawbot Date: Sat, 5 Sep 2026 09:33:23 +0000 Subject: [PATCH] Revert the 16 commits pushed to next on 2026-09-04 by an agent outside the managed fleet sneak, 2026-09-05: "inference instance stopped. undo its rogue work." The reverted commits stay in history; nothing else on next is touched. Model: fable-5-1 --- README.md | 38 +- test/packaging/lint-once.test.ts | 760 ++----------------------------- 2 files changed, 45 insertions(+), 753 deletions(-) diff --git a/README.md b/README.md index d2a132f..0d1d7d9 100644 --- a/README.md +++ b/README.md @@ -108,14 +108,11 @@ alpine. We provide: ### Linting -Linting runs in a container, one way, in every path that produces a lint verdict -for `make check` or the pre-commit hook. `script/lint` builds `Dockerfile.lint`, -which copies the repo into a digest-pinned node image and runs eslint and -prettier as build steps, so a successful build is a clean lint. No lint path -reachable from `script/check` or `script/precommit` runs on the host: every lint -verdict those two produce comes from the container. Docker is therefore required -to lint, and the container route also works where the docker daemon is remote -and bind mounts are impossible. +Linting runs in a container, one way, everywhere. `script/lint` builds +`Dockerfile.lint`, which copies the repo into a digest-pinned node image and +runs eslint and prettier as build steps, so a successful build is a clean lint. +There is no host lint path: docker is required to lint, and that also works +where the docker daemon is remote and bind mounts are impossible. The formatting check is part of that, not a step beside it. `script/check` and `script/precommit` therefore call `script/lint` and stop; neither calls @@ -127,20 +124,15 @@ in each. `test/packaging/lint-once.test.ts` asserts that count by walking the invocation graph, so a second pass cannot creep back in unnoticed. `script/fmt-check` remains as a standalone entrypoint for asking the formatting -question on its own, without docker and without the rest of lint. It is not the -last formatting path on the host — `script/fmt` writes with the host prettier, -and `package.json` exposes `fmt` and `fmt-check` as direct prettier calls — it -is the one of them that the check graph could plausibly have reached, and -nothing reaches it: neither `script/check` nor `script/precommit` calls it, so -it never contributes to their verdicts. Its own verdict cannot drift from the -container's: prettier is pinned to an exact version, installed from `yarn.lock` -under `--frozen-lockfile` in both places, and reads `.gitignore` as its default -ignore file — which is why `.dockerignore` deliberately keeps `.gitignore` in -the build context. +question on its own, without docker and without the rest of lint. Its verdict +cannot drift from the container's: prettier is pinned to an exact version, +installed from `yarn.lock` under `--frozen-lockfile` in both places, and reads +`.gitignore` as its default ignore file — which is why `.dockerignore` +deliberately keeps `.gitignore` in the build context. -Lint happens in exactly one place in that graph, which constrains the rest of -the build. `script/check` calls `script/lint`, so `make check` cannot run inside -a container without asking for docker inside docker. The image built from +Lint happens in exactly one place, which constrains the rest of the build. +`script/check` calls `script/lint`, so `make check` cannot run inside a +container without asking for docker inside docker. The image built from `Dockerfile` therefore runs `make test` and `make build` and does not lint; `script/cibuild` builds `Dockerfile.lint` first and that image second, so CI gets both verdicts. @@ -534,8 +526,8 @@ documents: runs the tests) must pass before merging to `main`. `make fmt-check` is available for a host-side formatting check on its own, but it is not a separate requirement: `make lint` already covers it, and running both would - check formatting twice. Never invoke eslint or prettier directly; every lint - verdict `make check` and the pre-commit hook produce comes from the container. + check formatting twice. Never invoke eslint or prettier directly; linting runs + in the container only. - **Formatting:** prettier with 4-space indents and `proseWrap: always` for markdown. Use `make fmt` to format. Use `yarn` not `npm`. diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts index 2813716..16fbad1 100644 --- a/test/packaging/lint-once.test.ts +++ b/test/packaging/lint-once.test.ts @@ -16,56 +16,10 @@ // against one file. Starting from an entrypoint, it follows every edge the repo // actually uses to reach another command — `run:` steps in the CI workflow, // `"$SCRIPT_DIR/"` and `script/` into other scripts, `make ` -// and `$(MAKE) ` through the Makefile shims, the prerequisites of a -// Makefile target, `yarn run ` through the `package.json` scripts, and -// `docker build` into the `RUN` steps of the Dockerfile it names, heredoc -// bodies included — and counts the prettier invocations it finds. -// -// What that covers is worth stating exactly rather than as "anywhere", because -// a comment claiming more than the code delivers is the same defect as an -// assertion that cannot fail — and an earlier draft of this header did claim -// it, while `check: fmt-check` and `@$(MAKE) fmt-check` both walked straight -// past it. Command names are read as `[a-z][a-z0-9_-]*` for Makefile targets -// and `script/` files, and `[A-Za-z][A-Za-z0-9_:-]*` for `package.json` -// scripts. A command reached by some other means — `sh -c`, a shell alias, an -// `include`d makefile, a name outside those charsets, a generated file — is -// not followed. Recipe lines reached through a make conditional are followed, -// without evaluating the condition, and so is a recipe written on the target -// line after a `;`; a recipe built by an `include`d makefile, a pattern rule, -// or a target name outside that charset is not. A variable given a literal -// value on one line — `VAR := ...`, `VAR = ...`, `VAR ?= ...` — is substituted -// wherever the Makefile spells it `$(VAR)` or `${VAR}`, because this Makefile -// writes its recipes that way (`@$(YARN) tsc --watch`); the substitution is -// one pass over literals, so a value that itself names another variable, a -// value built by a make function (`$(shell ...)`, `$(addprefix ...)`), and the -// body of a `define`/`endef` block are not expanded and not followed. A -// build is recognised as `docker build`, as its three management-command -// spellings (`docker buildx build`, `docker image build`, `docker builder -// build`), and as any of those reached through global flags standing between -// the command and the subcommand (`docker --context ci build`, `docker -H -// tcp://h:2375 buildx build`), a flag's separate argument being stepped over; -// `docker compose build` is deliberately not, since it builds services out of -// a compose file this walk does not read, and neither is any other non-flag -// word in that position. Matching only a literal `docker build` was worse than a -// wrong file: `docker buildx build -f .` emitted no edge at all, not -// even the default one. A recognised build resolves to the file named by -// `-f`, `-f=`, `-f` with the value attached to the flag, `--file` or -// `--file=`, wherever in that invocation the flag sits, and to `Dockerfile` -// when it names none; a Dockerfile chosen some other way — a bundled short -// flag cluster (`-qf `), a value this walk cannot expand to a literal — -// resolves to the default rather than to the real file. Each build on a line -// is followed separately, and the flag search for one is bounded to the slice -// running from that command to the next `&&`, `||`, `;` or `|`, so a second -// build on the same line is not swallowed by the first and a later command's -// `-f` is not read as the build's. That bounding is a split on those four -// separators, not a shell parse: a separator appearing inside quotes or a -// command substitution still ends the slice, so a flag standing after one — -// `docker build --build-arg MSG="a;b" -f Dockerfile.lint .` — is not read. -// Inside a Dockerfile the `RUN` keyword is matched case-insensitively and may -// be followed by any whitespace, because Docker executes `run ...` and a -// tab-separated `RUN\t...` exactly as it executes `RUN ...`; an instruction -// indented from the left margin is read too. Within those edges, a prettier -// call is caught wherever it is added. +// through the Makefile shims, `yarn run ` through the `package.json` +// scripts, and `docker build -f ` into that Dockerfile's `RUN` steps — and +// counts the prettier invocations it finds. A prettier call added anywhere in +// that graph is therefore caught, wherever it is added. // // Two entrypoints are walked, because they cover different graphs: `make check` // is what a developer runs, and `.gitea/workflows/check.yml` is what CI runs. @@ -78,9 +32,8 @@ // Undercounting is the failure mode that would make this test worthless. Three // things guard against it: the walk is asserted to have reached the nodes that // matter, an unresolvable or empty node is a thrown error rather than a quiet -// zero, and both the counting and the edge-following are per occurrence rather -// than per line, so two prettier calls or two `docker build`s chained with `&&` -// on one line cannot read as one. +// zero, and prettier is counted per occurrence rather than per line, so two +// invocations chained with `&&` cannot read as one. import { describe, expect, it } from "vitest"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; @@ -130,189 +83,26 @@ const countPrettier = (line: string): number => // Makefile targets are thin shims (`check:` / tab / `@script/check`), so a // `make ` edge has to resolve through them to keep "per `make check`" // meaning what it says. Recipe lines are the tab-indented ones. -// -// The prerequisite list is read as well, and it is not decoration: make runs a -// target's prerequisites before its recipe, so `check: fmt-check` invokes -// fmt-check every bit as much as a `@script/fmt-check` line in the recipe -// would. An earlier version of this parser captured the target name and threw -// the rest of the line away, which made `check: fmt-check` — a one-token edit, -// and the most natural way for someone to wire the host formatting check back -// into `make check` — a second prettier pass that this file scored as one. The -// shipped Makefile already relies on prerequisites (`install: build-bin`), so -// this is the file's own house style rather than a contrived evasion. -interface MakeTarget { - prerequisites: string[]; - recipe: string[]; -} - -// Names are lowercase-with-dashes here, plus digits and underscores. The -// leading character is deliberately not uppercase: `YARN := yarn run` is a -// variable assignment, not a rule, and `(?!=)` alone would not say so on a -// line spelled `YARN: yarn run`. -const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*"; - -// Conditional directives are not target lines, and they are not recipe lines -// either — they are parse-time structure wrapped around the recipe that -// encloses them. Treating one as an ordinary non-tab line ends the current -// recipe, and every tab-indented line after it is discarded, so -// -// check: -// \t@script/check -// ifeq (1,1) -// \t@script/fmt-check -// endif -// -// reads as a recipe of one command while `make -n check` prints two. The -// condition is deliberately not evaluated: that would mean evaluating make -// variables, and the conservative reading — every branch is reachable, so -// every branch's lines are recipe lines — is the one that cannot lose an -// invocation. Counting a command from a branch make would skip is a false -// alarm someone fixes; missing one is the failure this file exists to prevent. -const MAKE_CONDITIONAL = /^\s*(?:ifeq|ifneq|ifdef|ifndef|else|endif)\b/; - -// `@$(FMT)` is one token away from being a second prettier pass, and until the -// variable is resolved it reads as a line that invokes nothing: `make -n check` -// printed both `script/check` and `script/fmt-check` while this file scored -// one. `$(MAKE)` was already special-cased below, which is this same rule -// half-applied to a single name; this is the general form of it. -// -// Deliberately only the simple case: a name assigned a literal on one line, -// optionally through an `export`/`override` prefix. `:=` and `=` differ in -// when make expands them, which does not change the single value a literal can -// take, so those two are read the same way and the last one wins. `?=` differs -// in whether it assigns at all — it is skipped when the name already has a -// value — so among assignments to one name the first `?=` wins, and reading it -// as last-wins would resolve a reference to the string make discards. What is -// not read is anything needing evaluation — a value containing another -// `$(...)`, a make function, or a `define` body — -// because expanding those means implementing make, and a half-implementation -// that resolves a variable to the wrong string would count invocations that do -// not happen. An unresolved reference is left standing verbatim instead, which -// is the same dead end as any other unfollowed edge rather than a wrong answer. -const MAKE_ASSIGNMENT = new RegExp( - `^(?:(?:export|override)\\s+)*([A-Za-z_][A-Za-z0-9_]*)\\s*(:=|\\?=|=)\\s*(.*)$`, -); -const MAKE_VARIABLE_REFERENCE = /\$[({]([A-Za-z_][A-Za-z0-9_]*)[)}]/g; - -// `define`/`endef` bodies are skipped rather than parsed: the body is a -// multi-line value, so reading its lines as assignments would take whatever -// `=` they happen to contain and call it a variable. -const makeVariables = (text: string): Map => { - const variables = new Map(); - let inDefine = false; - for (const raw of text.split("\n")) { - if (/^\s*endef\b/.test(raw)) { - inDefine = false; - continue; - } - if (/^\s*define\b/.test(raw)) { - inDefine = true; - continue; - } - if (inDefine || raw.startsWith("\t")) continue; - const assignment = MAKE_ASSIGNMENT.exec(raw.replace(/#.*$/, "").trim()); - if (assignment === null) continue; - const [, name, operator, value] = assignment; - if (name === undefined || operator === undefined || value === undefined) - continue; - // `?=` assigns only when the name has no value yet, so the first one - // wins where `:=` and `=` let the last one win. Overwriting here would - // resolve the reference to a string make never uses. - if (operator === "?=" && variables.has(name)) continue; - // A value that is itself a reference is the recursive case, and is not - // resolved. Recording it would hand the substitution below a string it - // cannot finish expanding. - if (/\$[({]/.test(value)) continue; - variables.set(name, value.trim()); - } - return variables; -}; - -// One pass, and only over names that were assigned a literal: the result is -// never re-scanned for further references, so this cannot recurse or diverge. -const expandMakeVariables = ( - line: string, - variables: Map, -): string => - line.replace( - MAKE_VARIABLE_REFERENCE, - (reference, name: string) => variables.get(name) ?? reference, - ); - -const parseMakefile = (text: string): Map => { - const recipes = new Map(); - // Collected in a pass of their own because make reads the whole file - // before it runs anything: a recipe may spell a variable that the Makefile - // assigns further down, and a single pass would leave that one unexpanded - // purely because of where its author put it. - const variables = makeVariables(text); +const makeRecipes = (): Map => { + const recipes = new Map(); let current: string | null = null; - for (const line of text.split("\n")) { - // Expanded before parsing rather than only on recipe lines, so a - // prerequisite written `check: $(FMT)` resolves the same way a recipe - // line does. Assignment lines are expanded too and are unaffected: a - // literal value has nothing to substitute. - const raw = expandMakeVariables(line, variables); + for (const raw of read("Makefile").split("\n")) { if (raw.startsWith("\t")) { if (current !== null) { - recipes - .get(current) - ?.recipe.push(raw.trim().replace(/^[@-]+/, "")); + recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, "")); } continue; } - if (MAKE_CONDITIONAL.test(raw)) continue; - const target = new RegExp(`^(${MAKE_TARGET_NAME})\\s*:(?!=)(.*)$`).exec( - raw, - ); - current = target === null ? null : (target[1] ?? null); - if (target === null || current === null) continue; - // `target: prereqs ; command` puts the first recipe line on the target - // line itself. Read as prerequisites the whole of it, `;` and the - // command split into tokens that name no target and are filtered out, - // and the invocation disappears — `check: ; @script/fmt-check` was a - // second prettier pass this parser scored as none. - const rest = target[2] ?? ""; - const semicolon = rest.indexOf(";"); - const inlineRecipe = - semicolon === -1 ? "" : rest.slice(semicolon + 1).trim(); - // Trailing `# comment` is not a prerequisite; neither is the empty - // string a split leaves behind. - const prerequisites = ( - semicolon === -1 ? rest : rest.slice(0, semicolon) - ) - .replace(/#.*$/, "") - .trim() - .split(/\s+/) - .filter((name) => name !== ""); - const existing = recipes.get(current); - if (existing === undefined) { - recipes.set(current, { prerequisites, recipe: [] }); - } else { - existing.prerequisites.push(...prerequisites); - } - if (inlineRecipe !== "") { - recipes - .get(current) - ?.recipe.push(inlineRecipe.replace(/^[@-]+/, "")); + const target = /^([a-z][a-z-]*)\s*:(?!=)/.exec(raw); + current = target === null ? null : target[1]; + if (current !== null && !recipes.has(current)) { + recipes.set(current, []); } } return recipes; }; -const recipes = parseMakefile(read("Makefile")); - -// Only prerequisites that name a target of this Makefile are followed: the -// rest are filenames, or expansions of variables this parser does not -// evaluate, and neither is an invocation of anything it could read. -const prerequisitesOf = (node: string): string[] => { - if (!node.startsWith("make:")) return []; - const target = recipes.get(node.slice("make:".length)); - if (target === undefined) return []; - return target.prerequisites - .filter((name) => recipes.has(name)) - .map((name) => `make:${name}`); -}; +const recipes = makeRecipes(); const packageScripts = (): Record => { const pkg = JSON.parse(read("package.json")) as { @@ -323,63 +113,19 @@ const packageScripts = (): Record => { const scripts = packageScripts(); -// A `RUN < { - const commands: string[] = []; - let terminator: string | null = null; - for (const line of executable(text)) { - if (terminator !== null) { - if (line === terminator) terminator = null; - else commands.push(line); - continue; - } - const run = DOCKER_RUN.exec(line); - if (run === null) continue; - const command = run[1] as string; - terminator = HEREDOC_OPEN.exec(command)?.[2] ?? null; - commands.push(command); - } - if (terminator !== null) { - throw new Error(`unterminated heredoc: ${terminator}`); - } - return commands; -}; - // Node keys: `script/`, `docker:`, `make:`, // `yarn:`, `workflow:`. const resolve = (node: string): string[] => { if (node.startsWith("script/")) return executable(read(node)); if (node.startsWith("docker:")) { - return dockerRunCommands(read(node.slice("docker:".length))); + return executable(read(node.slice("docker:".length))) + .filter((line) => line.startsWith("RUN ")) + .map((line) => line.slice("RUN ".length)); } // The `run:` steps of a workflow, in file order. `uses:` steps are actions, // not commands, and have no edges into this repo's graph. A `run: |` block - // resolves to the bare `|`, which reaches nothing — so the count is exactly - // what such a step does NOT move, and it is the pinned resolved list, not - // the count, that turns it red. See the exact-equality test at the bottom - // of this file. + // would resolve to the bare `|`, which reaches nothing and therefore fails + // the count rather than passing quietly. if (node.startsWith("workflow:")) { return executable(read(node.slice("workflow:".length))) .filter((line) => /^-?\s*run:\s*\S/.test(line)) @@ -394,7 +140,7 @@ const resolve = (node: string): string[] => { if (recipe === undefined) { throw new Error(`no such Makefile target: ${target}`); } - return recipe.recipe; + return recipe; } if (node.startsWith("yarn:")) { const name = node.slice("yarn:".length); @@ -410,128 +156,42 @@ const resolve = (node: string): string[] => { // Same reasoning as the missing-target error, applied to every node kind: a // node that resolves to no commands contributes zero prettier invocations and // zero edges, which is indistinguishable from a clean result. Fail instead. -// -// A Makefile target with prerequisites and an empty recipe is the one case -// that is genuinely not vacuous — it runs its prerequisites — so it is not -// caught here. const commandsOf = (node: string): string[] => { const commands = resolve(node); - if (commands.length === 0 && prerequisitesOf(node).length === 0) { + if (commands.length === 0) { throw new Error(`node resolved to no commands: ${node}`); } return commands; }; -// Which invocation shapes read as a build, and which do not. -// -// RECOGNISED: `docker build`, the three management-command spellings of the -// same thing — `docker buildx build`, `docker image build`, `docker builder -// build` — and any of those reached through global flags standing between the -// command and the subcommand: `docker --context ci build`, `docker -H -// tcp://host:2375 build`, `docker --debug buildx build`. A global flag's -// separate argument is stepped over (`--context ci`), and an `=`-joined one is -// a single token (`--context=ci`). All of these read `-f` the same way and -// build the Dockerfile it names, so all of them are edges. -// -// `docker buildx build` was the concrete miss: `\bdocker\s+build\b` cannot -// reach across `buildx`, so `docker buildx build -f Dockerfile.extra .` in -// script/cibuild emitted no edge at all — not even the default `Dockerfile` -// one, which the old file-flag bug at least still produced — and a tree -// running prettier twice reported green. -// -// NOT RECOGNISED, deliberately: `docker compose build`, which builds services -// named in a compose file this parser does not read, so resolving it against a -// Dockerfile path would be a wrong answer rather than a missing one. Only -// `-`-prefixed tokens (plus their arguments) and those three literal -// subcommand words are stepped over, so no other non-flag word between -// `docker` and `build` matches, and `docker run -f build` is not a build. -const DOCKER_BUILD = - /\bdocker(?:\s+-{1,2}[A-Za-z][\w-]*(?:=\S+)?(?:\s+[^-\s]\S*)?)*(?:\s+(?:buildx|image|builder))?\s+build\b/g; - const edgesOf = (line: string): string[] => { const edges: string[] = []; // `"$SCRIPT_DIR/lint"`, `"$ROOT/script/lint"` and a bare `script/lint` are // all the same edge. for (const match of line.matchAll( - new RegExp( - `(?:\\$SCRIPT_DIR|\\$\\{SCRIPT_DIR\\}|script)/(${MAKE_TARGET_NAME})`, - "g", - ), + /(?:\$SCRIPT_DIR|\$\{SCRIPT_DIR\}|script)\/([a-z][a-z-]*)/g, )) { edges.push(`script/${match[1]}`); } // Only real targets: `pkg_install gnumake make make make` in // script/bootstrap is a package name, not an invocation of this Makefile. - // - // `$(MAKE)` and `${MAKE}` count as `make`. Recursive make is spelled that - // way by convention rather than as a literal `make`, and this Makefile - // already expands variables into recipes (`@$(YARN) tsc --watch`), so a - // case-sensitive literal-only match left `@$(MAKE) fmt-check` — the other - // one-token way to put the host prettier pass back into `make check` — - // unfollowed. Valueless flags between the command and the target (`make - // -n check`, `make -j4 check`) are skipped. A flag that takes a separate - // argument is not: `make -C sub build` runs `sub/Makefile`'s target, not - // this one's, and resolving it against these recipes would be wrong - // rather than merely incomplete. - for (const match of line.matchAll( - new RegExp( - `(?:\\$\\(MAKE\\)|\\$\\{MAKE\\}|\\bmake)(?:\\s+-\\S+)*\\s+(${MAKE_TARGET_NAME})`, - "g", - ), - )) { + for (const match of line.matchAll(/\bmake\s+([a-z][a-z-]*)/g)) { if (recipes.has(match[1] ?? "")) edges.push(`make:${match[1]}`); } // Same rule for yarn: `yarn run prettier` is the linter itself (counted, // not followed), `yarn run fmt-check` would be a package.json script that // runs it indirectly. - // - // The name charset is wider than the Makefile's because `package.json` - // script names conventionally carry colons, digits and underscores - // (`lint:fmt`, `test:e2e`). Under the narrower charset `yarn run lint:fmt` - // read as `yarn run lint`, which is not a script, so it produced no edge - // and no count. - for (const match of line.matchAll( - /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g, - )) { + for (const match of line.matchAll(/\byarn(?:\s+run)?\s+([a-z][a-z-]*)/g)) { if ((match[1] ?? "") in scripts) edges.push(`yarn:${match[1]}`); } // The container lint pass lives behind a `docker build`; without following // it the count would miss the one invocation that is supposed to survive. - // - // Every spelling of the file flag is read, not just `-f `. Docker - // accepts `--file `, `--file=`, `-f=` and the value - // attached to the short flag as one token (`-f`) for the same thing, - // and matching `-f ` alone resolved all of them to the default - // `Dockerfile` edge — so `docker build --file=Dockerfile.lint .` in a - // recipe was followed into the wrong file, counted nothing, and reported - // green. The flag is searched for anywhere in the invocation, as `-f` - // already was, because a real build line wraps it in `--build-arg` and - // other flags. A leading `\s` is required so that a longer flag ending in - // the same letters cannot supply the match, and the attached form is - // allowed only for the short flag, so that `--force-rm` — a long flag that - // merely starts with the same letters — still does not read as one. - // - // Every `docker build` on the line produces an edge, and each one looks for - // its file flag only within its own invocation — the slice from the command - // to the next `&&`, `||`, `;` or `|`. One `test` and one `exec` over the - // whole line got both halves of that wrong: `docker build -f Dockerfile.lint - // . && docker build -f Dockerfile.extra .` followed the first file and - // dropped the second, so a tree running prettier twice reported green, and a - // bare `docker build . && cp -f Dockerfile.lint /tmp/x` read `cp`'s `-f` as - // the build's, counting an invocation that never happens and losing the - // default `Dockerfile` edge. The bounding is a split on those four - // separators and nothing more: a separator inside quotes or a `$(...)` - // substitution ends the slice anyway, and a newline-separated command list - // is already one line per invocation by the time it gets here. - for (const match of line.matchAll(DOCKER_BUILD)) { - const invocation = line - .slice(match.index) - .split(/&&|\|\||;|\|/)[0] as string; - const file = /\s(?:--file[=\s]+|-f=?\s*)(\S+)/.exec(invocation); + if (/\bdocker\s+build\b/.test(line)) { + const file = /\s-f\s+(\S+)/.exec(line); edges.push(`docker:${file === null ? "Dockerfile" : file[1]}`); } @@ -557,11 +217,6 @@ const walk = (node: string, path: string[] = [], into?: Walk): Walk => { } result.reached.add(node); - // Before the recipe, exactly as make does. - for (const edge of prerequisitesOf(node)) { - walk(edge, [...path, node], result); - } - for (const line of commandsOf(node)) { result.prettier += countPrettier(line); for (const edge of edgesOf(line)) { @@ -678,13 +333,9 @@ const installBranches = (): { withoutYarn: string[]; withYarn: string[] } => { } const close = lines.indexOf("}", open); const body = lines.slice(open + 1, close === -1 ? undefined : close); - // The guard has to be a plain positive `missing yarn` test at the front of - // the condition. `if ! missing yarn ...` is the same shape to a looser - // regex but swaps which branch is which, and since the two branches are - // asserted separately below, that would silently relabel them — a failing - // test would then name the wrong branch. Anything else throws the named - // error below, which is the loud failure this file prefers. - const guard = body.findIndex((line) => /^if\s+missing yarn\b/.test(line)); + const guard = body.findIndex((line) => + /^if\b.*\bmissing yarn\b/.test(line), + ); const otherwise = body.indexOf("else", guard); const end = body.indexOf("fi", otherwise); if (guard === -1 || otherwise === -1 || end === -1) { @@ -837,244 +488,6 @@ describe("the resolver reads what the shell would run", () => { ).toEqual(["script/lint", "script/test", "script/fmt"]); }); - // The two edges the previous version of this file could not see, each - // pinned directly as well as through the graph. Both were reproduced as - // Makefile mutations that gave `make check` two prettier passes with the - // whole suite green. - it("follows a Makefile prerequisite as an invocation", () => { - // Not hypothetical: this is the shipped Makefile's own `install`. - expect(prerequisitesOf("make:install")).toEqual(["make:build-bin"]); - expect(prerequisitesOf("make:check")).toEqual([]); - }); - - it("treats $(MAKE) and ${MAKE} as make", () => { - expect(edgesOf("$(MAKE) fmt-check")).toContain("make:fmt-check"); - expect(edgesOf("${MAKE} fmt-check")).toContain("make:fmt-check"); - expect(edgesOf("make fmt-check")).toContain("make:fmt-check"); - expect(edgesOf("make -n fmt-check")).toContain("make:fmt-check"); - }); - - // Two more constructs that gave `make check` a second prettier pass with - // the whole suite green, each reproduced as a Makefile mutation before - // being pinned here. - it("keeps recipe lines inside a make conditional", () => { - const recipes = parseMakefile( - [ - "check:", - "\t@script/check", - "ifeq (1,1)", - "\t@script/fmt-check", - "endif", - ].join("\n"), - ); - // What `make -n check` prints, in order. - expect(recipes.get("check")?.recipe).toEqual([ - "script/check", - "script/fmt-check", - ]); - }); - - it("reads a recipe written on the target line after a semicolon", () => { - const recipes = parseMakefile( - ["check: ; @script/fmt-check", "\t@script/check"].join("\n"), - ); - expect(recipes.get("check")?.recipe).toEqual([ - "script/fmt-check", - "script/check", - ]); - // And the `;` and the command are not mistaken for prerequisites. - expect(recipes.get("check")?.prerequisites).toEqual([]); - expect( - parseMakefile("check: lint ; @script/fmt-check").get("check"), - ).toEqual({ - prerequisites: ["lint"], - recipe: ["script/fmt-check"], - }); - }); - - // The fifth construct that gave `make check` two prettier passes with the - // whole suite green: `FMT := script/fmt-check` and a recipe of `@$(FMT)`. - // The shipped Makefile writes recipe lines this way already - // (`@$(YARN) tsc --watch`), so an unexpanded variable was a gap in the - // house style rather than in an exotic corner of make. - it("expands a variable spelled into a recipe line", () => { - const recipes = parseMakefile( - [ - "FMT := script/fmt-check", - "check:", - "\t@$(FMT)", - "\t@${FMT}", - "\t@script/check", - ].join("\n"), - ); - // What `make -n check` prints, in order. - expect(recipes.get("check")?.recipe).toEqual([ - "script/fmt-check", - "script/fmt-check", - "script/check", - ]); - }); - - it("expands a variable spelled into a prerequisite list", () => { - expect( - parseMakefile("FMT = fmt-check\ncheck: $(FMT)").get("check") - ?.prerequisites, - ).toEqual(["fmt-check"]); - }); - - // A recipe may name a variable the Makefile assigns further down. - it("expands a variable assigned after the recipe that uses it", () => { - const recipes = parseMakefile( - ["check:", "\t@$(FMT)", "FMT ?= script/fmt-check"].join("\n"), - ); - expect(recipes.get("check")?.recipe).toEqual(["script/fmt-check"]); - }); - - // `export FMT := script/fmt-check` is an assignment make honours and the - // pattern anchored at the name, so the whole line read as neither an - // assignment nor a target and `@$(FMT)` stood unresolved: `make -n check` - // printed `script/check` and `script/fmt-check` while this file scored - // one. `override` reaches the same place by the same route. - it("expands a variable assigned through an export or override prefix", () => { - expect( - parseMakefile( - [ - "export FMT := script/fmt-check", - "check:", - "\t@$(FMT)", - "\t@script/check", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check", "script/check"]); - expect( - parseMakefile( - ["override FMT = script/fmt-check", "check:", "\t@$(FMT)"].join( - "\n", - ), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check"]); - // `export` on its own line names a variable without assigning one, and - // is not read as an assignment of the empty string. - expect( - parseMakefile( - [ - "FMT := script/fmt-check", - "export FMT", - "check:", - "\t@$(FMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check"]); - }); - - // `?=` assigns only when the name has no value yet, so among assignments - // to one name the first `?=` wins where `:=` and `=` let the last one win. - // Reading every operator as last-wins resolved `@$(FMT)` to the value make - // discards: the file counted an invocation that never happens and missed - // the one that does, with the suite green either way. - it("keeps the first value when a later assignment is conditional", () => { - expect( - parseMakefile( - [ - "FMT := script/fmt-check", - "FMT ?= script/build", - "check:", - "\t@$(FMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check"]); - // A `?=` that is itself the first assignment does assign, and a `?=` - // after it does not. - expect( - parseMakefile( - [ - "FMT ?= script/fmt-check", - "FMT ?= script/build", - "check:", - "\t@$(FMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check"]); - // `:=` and `=` stay last-wins, which is what make does. - expect( - parseMakefile( - [ - "FMT := script/build", - "FMT := script/fmt-check", - "check:", - "\t@$(FMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["script/fmt-check"]); - }); - - // The edges of the expansion, asserted so the header's not-followed list - // is the code's behaviour rather than a claim about it. Each of these - // leaves the reference standing verbatim, which reaches nothing — the same - // dead end as any other unfollowed edge, and not a wrong resolution. - it("leaves a value it cannot resolve to a literal unexpanded", () => { - // A make function: resolving it would mean running it. - expect( - parseMakefile( - [ - "FMT := $(shell echo script/fmt-check)", - "check:", - "\t@$(FMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["$(FMT)"]); - // A `define` body is a multi-line value, not a single-line assignment. - expect( - parseMakefile( - [ - "define RUNFMT", - "@script/fmt-check", - "endef", - "check:", - "\t$(RUNFMT)", - ].join("\n"), - ).get("check")?.recipe, - ).toEqual(["$(RUNFMT)"]); - // An undefined name is not silently emptied. - expect( - parseMakefile(["check:", "\t@$(NOPE)"].join("\n")).get("check") - ?.recipe, - ).toEqual(["$(NOPE)"]); - }); - - it("reads a package.json script name that contains a colon", () => { - // The lookup against `scripts` is what turns this into an edge, and no - // script in this repo is colon-named, so the charset is asserted on - // the extraction itself. - expect( - [ - ..."yarn run lint:fmt".matchAll( - /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g, - ), - ].map((match) => match[1]), - ).toEqual(["lint:fmt"]); - }); - - it("reads the body of a BuildKit heredoc RUN step", () => { - expect( - dockerRunCommands( - [ - "FROM scratch", - "RUN < { - expect(() => - dockerRunCommands("FROM scratch\nRUN < { expect(edgesOf("docker build .")).toContain("docker:Dockerfile"); expect(edgesOf("docker build -f Dockerfile.lint .")).toContain( @@ -1082,119 +495,6 @@ describe("the resolver reads what the shell would run", () => { ); }); - // `-f` is not the only spelling of the flag. `--file=Dockerfile.lint` and - // `--file Dockerfile.lint` mean exactly what `-f Dockerfile.lint` means, - // and matching only `-f` sent both to the default `Dockerfile` edge - // instead. That is the false-green shape this whole file exists to - // prevent: a second prettier pass added as - // `docker build --file=Dockerfile.lint .` was followed into the wrong - // Dockerfile, counted nothing, and left every build green. The long form - // is an ordinary thing for a human to write, so it is followed rather than - // merely documented as unfollowed. - // - // The equality rather than containment matters: resolving to the right - // file is only half of it, the default edge must not also be emitted. - it("follows --file in all three spellings as it does -f", () => { - expect(edgesOf("docker build --file=Dockerfile.lint .")).toEqual([ - "docker:Dockerfile.lint", - ]); - expect(edgesOf("docker build --file Dockerfile.lint .")).toEqual([ - "docker:Dockerfile.lint", - ]); - expect(edgesOf("docker build -f=Dockerfile.lint .")).toEqual([ - "docker:Dockerfile.lint", - ]); - }); - - // The value may be attached to the short flag with no separator at all: - // `-fDockerfile.lint` is a single token, and the flag parser reads it as - // `-f` naming that file, exactly as `-f Dockerfile.lint` does. Requiring a - // separator sent it to the default `Dockerfile` edge instead — the same - // false green as the long spellings above, and one the header promised was - // followed: a second prettier pass added as `docker build - // -fDockerfile.lint .` was resolved into the wrong file, counted nothing, - // and left the build green. `-fFILE` is an ordinary thing for a human to - // write, so it is followed rather than merely documented as unfollowed. - it("follows a short file flag with its value attached", () => { - expect(edgesOf("docker build -fDockerfile.lint .")).toEqual([ - "docker:Dockerfile.lint", - ]); - }); - - // The flag is found anywhere in the invocation rather than at a fixed - // position, matching how `-f` was already read, since a real build line - // carries `--build-arg` and friends around it. - it("finds the file flag after other flags and build args", () => { - expect( - edgesOf( - 'docker build --build-arg LINT_EPOCH="1" --file=Dockerfile.lint --progress=plain .', - ), - ).toEqual(["docker:Dockerfile.lint"]); - }); - - // A different flag that merely begins with the same letters is not the - // file flag: `--force-rm` must not read as one, or its next token would be - // resolved as a Dockerfile and thrown on as unreadable. - it("does not mistake --force-rm for the file flag", () => { - expect(edgesOf("docker build --force-rm .")).toEqual([ - "docker:Dockerfile", - ]); - }); - - // The header says a bundled short flag cluster resolves to the default - // rather than to the file it names, which is a limitation and so has to be - // pinned: an unpinned limitation is how the header starts overclaiming - // again. If someone teaches the resolver to split clusters, this test is - // the one that tells them to correct the header too. - it("resolves a bundled -qf cluster to the default Dockerfile", () => { - expect(edgesOf("docker build -qf Dockerfile.lint .")).toEqual([ - "docker:Dockerfile", - ]); - }); - - // The false green this file exists to prevent, in the one shape it still - // had: `script/lint` chaining a second `docker build` after the lint image - // built prettier twice and counted once, because a single `test` and a - // single `exec` over the line produced exactly one edge and resolved it to - // the first file named. That is the same failure as the original duplicate - // pass — two invocations, one verdict — reached through the walker instead - // of through the repo. - it("follows both docker builds when one line chains two", () => { - expect( - edgesOf( - 'docker build --build-arg LINT_EPOCH="$(date +%s)" -f Dockerfile.lint . && docker build -f Dockerfile.extra .', - ), - ).toEqual(["docker:Dockerfile.lint", "docker:Dockerfile.extra"]); - }); - - // The other half: a flag search over the whole line reads a *later* - // command's `-f` as the build's own. A bare `docker build .` followed by - // `cp -f` resolved to `Dockerfile.lint`, which both counts an invocation - // that never happens and drops the `Dockerfile` edge — the half of the CI - // graph the second entrypoint exists to cover. Each separator that bounds - // an invocation is pinned, since the bounding is a split rather than a - // shell parse and dropping one of them would be silent. - it("does not read a later command's -f as the build's", () => { - expect( - edgesOf("docker build . && cp -f Dockerfile.lint /tmp/x"), - ).toEqual(["docker:Dockerfile"]); - expect( - edgesOf("docker build . || cp -f Dockerfile.lint /tmp/x"), - ).toEqual(["docker:Dockerfile"]); - expect( - edgesOf("docker build . ; cp -f Dockerfile.lint /tmp/x"), - ).toEqual(["docker:Dockerfile"]); - expect(edgesOf("docker build . | tee -f Dockerfile.lint")).toEqual([ - "docker:Dockerfile", - ]); - }); - - // This exact-equality assertion is also the block-scalar guard, which is - // not obvious from its name: a `run: |` step resolves to the bare `|`, - // which reaches nothing, so the prettier count would stay 1 no matter what - // the block contains. Pinning the resolved list is what makes such a step - // red. The cost is that any legitimate second `run:` step — a cache step, - // an `echo` — fails this test for a reason unrelated to prettier. it("reads the run steps of the CI workflow and not its uses steps", () => { expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([ "script/cibuild",