diff --git a/Dockerfile b/Dockerfile index 0caabf2..a194ffd 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,36 +1,27 @@ -# Lint stage — fast feedback on formatting and lint issues -# node 22.22.0 on Alpine 3.23.3 (node:22-alpine), 2026-08-09 -FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e34 AS lint -WORKDIR /app -COPY script/ script/ -COPY package.json yarn.lock ./ -RUN script/bootstrap -COPY . . -RUN make fmt-check -RUN make lint - -# Check stage — the full suite and the build +# Test and build image: the suite, then the compile. +# +# Linting deliberately does not happen here. `script/lint` is a build of +# Dockerfile.lint, and `script/check` calls `script/lint`, so running +# `make check` in this image would mean running `docker build` inside a +# container. Lint runs exactly once, in Dockerfile.lint; script/cibuild +# builds that first and this second. # node 22.22.0 on Alpine 3.23.3 (node:22-alpine), 2026-08-09 FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e34 AS check WORKDIR /app -# Force BuildKit to run the lint stage before proceeding. Without this the -# two stages run in parallel and a lint failure can lose the race. -COPY --from=lint /app/yarn.lock /dev/null - COPY script/ script/ COPY package.json yarn.lock ./ RUN script/bootstrap COPY . . -# CHECK_EPOCH is a cache buster: without it Docker serves `make check` from +# CHECK_EPOCH is a cache buster: without it Docker serves the test layer from # cache on an unchanged tree, the suite never executes, and the build still # exits 0. The guard makes an absent argument a hard failure — an unset ARG # is the empty string, which is a perfectly stable cache key, so a plain # `docker build .` would otherwise still get the false green. Fail closed. ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 -RUN make check +RUN make test ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..0ee2a9a --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,35 @@ +# Lint image: every lint run happens here, and nowhere else. The repo is +# COPYed into a digest-pinned image and the linters run as build steps, so a +# successful build IS a clean lint. `script/lint` does nothing but build this +# file, which also works where the docker daemon is remote and bind mounts are +# impossible. Nothing that runs inside a container may call `script/lint`: +# that is why Dockerfile no longer runs `make check`. +# node 22.22.0 on Alpine 3.23.3 (node:22-alpine), 2026-08-09 +FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e34 AS lint +WORKDIR /app + +# Manifests before sources, so the dependency install layer stays cached +# until package.json or yarn.lock changes. script/bootstrap ends in +# `yarn install --frozen-lockfile`; the lint steps below are deliberately +# not cached. +COPY script/ script/ +COPY package.json yarn.lock ./ +RUN script/bootstrap + +COPY . . + +# LINT_EPOCH is a cache buster, with the same fail-closed contract as +# CHECK_EPOCH in Dockerfile. No lint cache is wanted: on an unchanged tree +# Docker serves the linter layers in well under a second, having linted +# nothing, and the build still exits 0. The guard makes an absent argument a +# hard failure — an unset ARG is the empty string, which is a perfectly +# stable cache key, so a plain `docker build -f Dockerfile.lint .` would +# otherwise get exactly that false green. Every layer below this one is a +# child of the guard, so a fresh epoch forces all of them to execute. +ARG LINT_EPOCH +RUN [ -n "$LINT_EPOCH" ] || exit 1 + +# The linters are invoked directly rather than through `make lint`, because +# `make lint` is the build of this file. +RUN yarn run eslint . +RUN yarn run prettier --check . diff --git a/README.md b/README.md index 8df251c..0d1d7d9 100644 --- a/README.md +++ b/README.md @@ -86,31 +86,70 @@ alpine. We provide: files the compiler wrote, and make the CLI executable (our own extension) - `script/test` — run the test suite (vitest, hard-capped at 30s where `timeout` is available, verbose rerun on failure) -- `script/lint` — run eslint and a prettier check +- `script/lint` — run eslint and a prettier check, by building + `Dockerfile.lint`; requires docker (see Linting below) - `script/fmt` — format all files with prettier (writes) -- `script/fmt-check` — check formatting (read-only) -- `script/check` — run all checks: `test`, `lint`, `fmt-check` (our own - extension) -- `script/docker` — build the Docker image, tagged via `script/projectname` -- `script/cibuild` — cd to the repo root and run the image build (what CI runs; - the build runs `make fmt-check` and `make lint` in a first stage, then - `make check` and `make build` in a second) +- `script/fmt-check` — check formatting on the host (read-only); standalone, and + not called by `script/check` or `script/precommit`, because `script/lint` + already checks formatting in the container (see Linting below) +- `script/check` — run all checks: `test`, `lint` (our own extension) +- `script/docker` — build the test and build image, tagged via + `script/projectname` +- `script/cibuild` — cd to the repo root and build both images (what CI runs): + `script/lint` first, then the `Dockerfile` image, which runs `make test` and + `make build` - `script/precommit` — run by the git pre-commit hook (our own extension); runs - `script/lint` and `script/fmt-check` but deliberately not the tests, so the - TDD red-phase commit can land + `script/lint`, which checks both lint and formatting, but deliberately not the + tests, so the TDD red-phase commit can land - `script/install-precommit` — installs the git pre-commit hook (our own extension); `make hooks` shims to it `make hooks` installs the pre-commit hook that runs `script/precommit`. -Both `script/docker` and `script/cibuild` pass -`--build-arg CHECK_EPOCH="$(date +%s)"`. The Dockerfile refuses to build without -it. This is deliberate: on an unchanged tree Docker would otherwise serve the -`make check` layer from cache, so the suite would never run and the build would -still exit 0. A changing epoch invalidates the check and build layers on every -invocation while leaving the dependency layers below them cached, and the -missing-argument guard means a bare `docker build .` fails loudly instead of -quietly reporting a green it did not earn. +### Linting + +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 +`script/fmt-check` as well, which would run prettier a second time over the same +tree for the same verdict — and the weaker of the two, since the host's prettier +is whatever the working tree has installed. So `make check` and the pre-commit +hook both still fail on a badly formatted tree, and prettier runs exactly once +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. 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, 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. + +### Build epochs + +`script/lint` passes `--build-arg LINT_EPOCH="$(date +%s)"`, and `script/docker` +and `script/cibuild` pass `--build-arg CHECK_EPOCH="$(date +%s)"`. Both +Dockerfiles refuse to build without their argument. This is deliberate: on an +unchanged tree Docker would otherwise serve the linter and test layers from +cache, so nothing would run and the build would still exit 0 — a lint build over +an untouched tree returns success in well under a second, having linted nothing. +A changing epoch invalidates every layer below the guard on every invocation +while leaving the dependency layers above them cached, and the missing-argument +guard means a bare `docker build .` fails loudly instead of quietly reporting a +green it did not earn: an unset build argument is the empty string, which is a +perfectly stable cache key. ## Rationale @@ -145,8 +184,10 @@ All work on quak is test-driven. No exceptions. 3. Subsequent commits add the implementation and any refactors needed to make the tests pass. 4. A feature branch can only be merged into `main` when `make check` is green. - `main` is always green. The Dockerfile runs `make check` and `make build`, so - neither a red branch nor one that does not compile can pass CI. + `main` is always green. CI runs `script/cibuild`, which lints via + `Dockerfile.lint` and then runs `make test` and `make build` in the + `Dockerfile` image, so neither a red branch nor one that does not compile can + pass CI. 5. Tests are the canonical API documentation for this library. Every test file is commented thoroughly enough that a reader who has never seen quak can learn how to use it from the tests alone. Comments explain why a behavior @@ -160,11 +201,11 @@ All work on quak is test-driven. No exceptions. history must still show tests landing before (or with) the matching implementation. 8. The pre-commit hook installed by `make hooks` runs `script/precommit`, which - runs the lint and format checks but not the full `make check`. This is - deliberate so the TDD red-phase commit (failing tests, no implementation yet) - can land. The full `make check` runs as part of the image build, which is - what CI executes via `script/cibuild`, so a red branch still cannot reach - `main`. + runs `script/lint` — eslint and the prettier check, in the container — but + not the tests, and so not the full `make check`. This is deliberate so the + TDD red-phase commit (failing tests, no implementation yet) can land. The + suite runs as part of the image build, which is what CI executes via + `script/cibuild`, so a red branch still cannot reach `main`. ## Design @@ -191,7 +232,8 @@ quak/ quak.ts CLI entrypoint (commander.js) test/ unit + integration tests (vitest) Makefile - Dockerfile + Dockerfile test suite and compile + Dockerfile.lint eslint and prettier, as build steps package.json tsconfig.json ``` @@ -478,9 +520,14 @@ documents: implementation. Tests are the canonical API documentation and must be commented thoroughly. `main` is always green. -- **Required checks before every commit:** `make lint` (eslint + prettier check) - and `make fmt-check` must pass. The pre-commit hook enforces this. - `make check` (which also runs tests) must pass before merging to `main`. +- **Required checks before every commit:** `make lint` must pass — that is + eslint plus the prettier check, and it builds `Dockerfile.lint`, so it needs + docker. The pre-commit hook enforces exactly that. `make check` (which also + 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; 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/TODO.md b/TODO.md index 5671aff..efc43fb 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,46 @@ Update the README API reference section to match the current implementation. # Completed Steps +- 2026-08-10: Made `lint-once.test.ts` enforce what its header claims. It walked + `make check` only, so it never read `Dockerfile` — the image CI builds through + `script/cibuild` — and a second `prettier --check .` could be added there with + the suite staying green. The walk now also starts at + `.gitea/workflows/check.yml` and follows its `run:` steps, so the graph under + test is the one CI executes rather than the one someone assumed it executes. + The lockfile assertion was a substring check against the whole of + `script/bootstrap`, which has two install sites and so reported the branch the + containers never take; the two branches are now resolved separately and every + `yarn install` in each is required to be `--frozen-lockfile`. Prettier is + counted per occurrence instead of per line, so two invocations chained with + `&&` no longer read as one, and edges are followed on counted lines instead of + being skipped. Every way for the walk to reach nothing — an unknown target, an + unknown script, a missing file, a node with no commands, an unknown node kind + — is a thrown error rather than a quiet zero. Every assertion in the file was + mutation-tested individually. +- 2026-08-10: Stopped `make check` running `prettier --check .` twice. Since + linting moved into Docker, the duplicate was one container pass and one host + pass of the same check: `script/lint` builds `Dockerfile.lint`, which runs + prettier as a build step, and `script/check` then called `script/fmt-check` as + well. The host call is gone from `script/check` and from `script/precommit`; + the container keeps checking formatting, because a successful + `Dockerfile.lint` build is what CI treats as proof of a clean tree, and it is + also what still fails the pre-commit hook on a badly formatted tree. + `script/fmt-check` survives as a standalone entrypoint, whose verdict cannot + drift from the container's. A test walks the invocation graph from each + entrypoint — through the Makefile shims, the `script/` calls and the + `docker build` — and asserts the prettier count, so the duplication cannot + come back unnoticed. +- 2026-08-10: Moved all linting into Docker. `script/lint` builds a new root + `Dockerfile.lint`, which copies the repo into the digest-pinned node image and + runs eslint and prettier as build steps, so a successful build is a clean + lint; no host lint path remains and `yarn lint` is gone from `package.json`. A + fail-closed `LINT_EPOCH` guard stops Docker serving the linter layers from + cache, which is how a lint build returns success in under a second having + linted nothing. The lint stage inside `Dockerfile` and its `COPY --from=lint` + ordering hack are gone: that image now runs `make test` and `make build` only, + because `script/check` calls `script/lint` and running it in a container would + mean docker inside docker. `script/cibuild` builds the lint image first, then + the test and build image. - 2026-08-09: Made `make docker` green and policy-conformant. Multi-stage Dockerfile: a lint stage runs `make fmt-check` and `make lint`, and the check stage takes a `COPY --from=lint` dependency on it before running `make check` diff --git a/package.json b/package.json index 746ba14..423fad4 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,6 @@ "build": "script/build", "quak": "node ./dist/bin/quak.js", "test": "vitest run", - "lint": "eslint .", "fmt": "prettier --write .", "fmt-check": "prettier --check ." }, diff --git a/script/check b/script/check index 3e1778c..2f37914 100755 --- a/script/check +++ b/script/check @@ -1,6 +1,19 @@ #!/bin/sh -# script/check: run all checks (test, lint, fmt-check). Our own -# extension to scripts-to-rule-them-all. Must not modify any files. +# script/check: run all checks (test, lint). Our own extension to +# scripts-to-rule-them-all. Must not modify any files. +# +# The formatting check is part of lint, not a step of its own: +# script/lint builds Dockerfile.lint, which runs eslint AND +# `prettier --check .` as build steps. Calling script/fmt-check here as +# well would run prettier a second time over the same tree for the same +# verdict — the weaker of the two, since the host toolchain is whatever +# the working tree happens to have installed while the container's is +# digest-pinned. script/fmt-check remains a standalone entrypoint for +# asking the formatting question by itself. +# +# script/lint builds Dockerfile.lint, so this script requires docker and +# must never be run from inside a container: that is why the Dockerfile +# image runs script/test and script/build rather than this. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" @@ -8,7 +21,6 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" main() { "$SCRIPT_DIR/test" "$SCRIPT_DIR/lint" - "$SCRIPT_DIR/fmt-check" } main "$@" diff --git a/script/cibuild b/script/cibuild index 51224da..dd1f7b5 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,16 +1,23 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile runs script/check and -# script/build, and CHECK_EPOCH differs on every invocation, so those two -# layers cannot be served from Docker's cache: a green build here means -# the checks ran now, not that a previous run was remembered. The layers -# below the epoch (bootstrap, yarn install) are unaffected and stay -# cached. A build that omits the argument fails by design. +# script/cibuild: run the CI build, which is both images in a defined order. +# +# First script/lint, which builds Dockerfile.lint and is the one and only +# place linting happens — it goes first so a lint failure is reported before +# the slower suite runs. Then the Dockerfile image, which runs script/test +# and script/build. CHECK_EPOCH and LINT_EPOCH differ on every invocation, so +# neither the linters nor the suite can be served from Docker's cache: a +# green build here means the checks ran now, not that a previous run was +# remembered. The layers below the epochs (bootstrap, yarn install) are +# unaffected and stay cached. A build that omits the arguments fails by +# design. set -eu -ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" +ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" + "$SCRIPT_DIR/lint" docker build --build-arg CHECK_EPOCH="$(date +%s)" . } diff --git a/script/docker b/script/docker index 0bd2a83..c691f2c 100755 --- a/script/docker +++ b/script/docker @@ -3,7 +3,8 @@ # Identical in all repos; the tag comes from script/projectname. # CHECK_EPOCH is passed for the same reason script/cibuild passes it: the # Dockerfile refuses to build without it, so that no path to an image can -# quietly serve the check and build layers from cache. +# quietly serve the test and build layers from cache. This builds the test +# and build image only; linting is a separate image, built by script/lint. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" diff --git a/script/lint b/script/lint index 3ede02b..4a602f2 100755 --- a/script/lint +++ b/script/lint @@ -1,13 +1,24 @@ #!/bin/sh -# script/lint: run the linter (eslint plus a prettier check). +# script/lint: run the linters. eslint and prettier are never run against +# the working tree from here: linting runs via docker only, one way, +# everywhere — script/lint builds Dockerfile.lint, which COPYs the repo into +# the pinned node image and runs the linters as build steps. That works even +# when the docker daemon is remote and bind mounts are impossible. +# +# LINT_EPOCH is passed on every invocation because no lint cache is wanted: +# on an unchanged tree Docker would otherwise serve the linter layers, having +# linted nothing, and still exit 0. Dockerfile.lint refuses to build without +# the argument, so no path to a lint result can quietly come from cache. +# +# Nothing that runs inside a container may call this script; see the header +# of Dockerfile. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - yarn run eslint . - yarn run prettier --check . + docker build --build-arg LINT_EPOCH="$(date +%s)" -f Dockerfile.lint . } main "$@" diff --git a/script/precommit b/script/precommit index 07b9e7e..489e72d 100755 --- a/script/precommit +++ b/script/precommit @@ -2,17 +2,25 @@ # script/precommit: run by the git pre-commit hook; fails the commit if # checks fail. Our own extension to scripts-to-rule-them-all. # -# Runs lint and fmt-check but deliberately NOT the tests, so the TDD -# red-phase commit (failing tests, no implementation yet) can land. CI -# runs make check via docker build, which catches any branch that -# ships red. +# Runs lint but deliberately NOT the tests, so the TDD red-phase commit +# (failing tests, no implementation yet) can land. CI runs +# script/cibuild, which builds both images and so catches any branch +# that ships red. +# +# The formatting check is still enforced here, because script/lint is a +# build of Dockerfile.lint and that runs `prettier --check .` as a build +# step: a badly formatted tree fails this hook, and therefore the +# commit. Calling script/fmt-check as well would only run prettier a +# second time over the same tree for the same verdict. +# +# script/lint is a docker build (Dockerfile.lint); docker is required to +# commit, which is the point of linting one way, everywhere. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" main() { "$SCRIPT_DIR/lint" - "$SCRIPT_DIR/fmt-check" } main "$@" diff --git a/test/packaging/build-context.test.ts b/test/packaging/build-context.test.ts index 9ffa5a1..056fb57 100644 --- a/test/packaging/build-context.test.ts +++ b/test/packaging/build-context.test.ts @@ -12,7 +12,7 @@ // // Neither shows up as a build failure, so they are asserted here. import { describe, expect, it } from "vitest"; -import { readFileSync } from "node:fs"; +import { existsSync, readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { join } from "node:path"; @@ -46,4 +46,19 @@ describe(".dockerignore", () => { it("leaves .gitignore in the build context for prettier", () => { expect(dockerignore).not.toContain(".gitignore"); }); + + // Both images are built from this same context, and the lint image runs + // eslint and prettier across it. BuildKit lets a `.dockerignore` + // shadow the root one for a single build; such a file would silently give + // the lint build a different, unreviewed context — and eslint's flat config + // does not ignore dot-directories, so a stray `.claude/` worktree would be + // linted. + it.each(["Dockerfile", "Dockerfile.lint"])( + "is not shadowed by a per-Dockerfile ignore file for %s", + (name) => { + expect(existsSync(join(repoRoot, `${name}.dockerignore`))).toBe( + false, + ); + }, + ); }); diff --git a/test/packaging/entrypoints.test.ts b/test/packaging/entrypoints.test.ts index 3a09f64..58149c5 100644 --- a/test/packaging/entrypoints.test.ts +++ b/test/packaging/entrypoints.test.ts @@ -1,7 +1,9 @@ // The package manifest promises three files that only exist after a build: // `main`, `types`, and the `quak` binary. Nothing in the test suite used to -// look at them, and `make check` runs test, lint and fmt-check but never the -// build, so `tsconfig.json` and `package.json` were free to drift apart. They +// look at them, and `make check` runs the suite and the lint container but +// never the build, so `tsconfig.json` and `package.json` were free to drift +// apart. (The formatting check is part of the lint container, not a step of +// its own; `test/packaging/lint-once.test.ts` is what holds that shape.) They // did: `rootDir` was `./src` while `include` also pulled in `bin/**/*`, which // is TS6059, and no build had succeeded for as long as that was true. // diff --git a/test/packaging/lint-docker.test.ts b/test/packaging/lint-docker.test.ts new file mode 100644 index 0000000..fa561f2 --- /dev/null +++ b/test/packaging/lint-docker.test.ts @@ -0,0 +1,184 @@ +// Linting runs in Docker, one way, everywhere: `script/lint` builds +// `Dockerfile.lint`, which COPYs the repo into a digest-pinned image and runs +// eslint and prettier as build steps, so a successful build IS a clean lint. +// +// Three things can quietly undo that, and none of them shows up as a build +// failure, which is why they are asserted here: +// +// 1. Recursion. `script/check` calls `script/lint`, and `script/lint` is now a +// `docker build`. Anything that runs `make check` inside a container is +// therefore asking for Docker inside Docker, and CI breaks. The image built +// from `Dockerfile` runs the suite and the compile only; lint happens once, +// in `Dockerfile.lint`. +// 2. Cache. A lint build over an unchanged tree returns success in well under a +// second having linted nothing. The `LINT_EPOCH` guard is what forces the +// linter layers to execute, and it has to fail closed: an unset build +// argument is the empty string, which is a perfectly stable cache key, so an +// invocation that omits it must be rejected rather than served a cached +// green. +// 3. A host lint path surviving alongside the container one, which would let a +// lint result come from an unpinned local toolchain. +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { join } from "node:path"; + +const repoRoot = fileURLToPath(new URL("../../", import.meta.url)); + +const read = (name: string): string => + readFileSync(join(repoRoot, name), "utf-8"); + +// The executable lines of a shell script or Dockerfile: comments carry the +// reasoning and frequently name the very commands these tests forbid, so they +// would otherwise trigger every assertion below. +const instructions = (name: string): string[] => + read(name) + .split("\n") + .map((line) => line.trim()) + .filter((line) => line !== "" && !line.startsWith("#")); + +const lintScript = instructions("script/lint"); +const dockerfileLint = instructions("Dockerfile.lint"); +const dockerfile = instructions("Dockerfile"); +const cibuild = instructions("script/cibuild"); + +const has = (lines: string[], pattern: RegExp): boolean => + lines.some((line) => pattern.test(line)); + +describe("script/lint", () => { + it("lints by building Dockerfile.lint", () => { + expect(has(lintScript, /docker build .*-f Dockerfile\.lint/)).toBe( + true, + ); + }); + + // The whole point of the ruling: no invocation of a linter against the + // working tree survives, so a lint verdict can only come from the pinned + // image. + it("runs no linter on the host", () => { + expect(has(lintScript, /eslint|prettier/)).toBe(false); + }); + + // Without a fresh epoch the build is served from cache in under a second, + // having linted nothing, and still exits 0. + it("passes a fresh LINT_EPOCH on every run", () => { + expect( + has(lintScript, /--build-arg LINT_EPOCH="\$\(date \+%s\)"/), + ).toBe(true); + }); +}); + +describe("Dockerfile.lint", () => { + // Tag references are server-mutable, so they are remote code execution. + it("pins its base image by digest", () => { + expect(has(dockerfileLint, /^FROM \S+@sha256:[0-9a-f]{64}/)).toBe(true); + }); + + it("runs eslint as a build step", () => { + expect(has(dockerfileLint, /^RUN .*eslint \./)).toBe(true); + }); + + it("runs prettier as a build step", () => { + expect(has(dockerfileLint, /^RUN .*prettier --check \./)).toBe(true); + }); + + // An unset ARG is the empty string, and an empty string is a perfectly + // stable cache key. Rejecting it is what stops a bare + // `docker build -f Dockerfile.lint .` from reporting a green it did not + // earn. + it("refuses to build without LINT_EPOCH", () => { + expect(has(dockerfileLint, /^ARG LINT_EPOCH$/)).toBe(true); + expect( + has(dockerfileLint, /^RUN \[ -n "\$LINT_EPOCH" \] \|\| exit 1$/), + ).toBe(true); + }); + + // The guard only forces execution of the layers below it, so both linters + // have to sit after it. Layer order is the mechanism, not a style choice. + it("puts both linters below the epoch guard", () => { + const guard = dockerfileLint.findIndex((line) => + /^RUN \[ -n "\$LINT_EPOCH" \]/.test(line), + ); + const linters = dockerfileLint + .map((line, index) => ({ line, index })) + .filter(({ line }) => /^RUN .*(eslint|prettier)/.test(line)); + + expect(linters.length).toBeGreaterThan(0); + for (const { line, index } of linters) { + expect( + index, + `${line} must run below the LINT_EPOCH guard`, + ).toBeGreaterThan(guard); + } + }); + + // Dependency installation is the slow layer and has nothing to do with the + // sources, so it caches separately: manifests first, sources afterwards. + it("copies the manifests before the sources", () => { + const manifests = dockerfileLint.findIndex((line) => + /^COPY package\.json yarn\.lock/.test(line), + ); + const sources = dockerfileLint.findIndex((line) => + /^COPY \. \.$/.test(line), + ); + + expect(manifests).toBeGreaterThanOrEqual(0); + expect(sources).toBeGreaterThan(manifests); + }); + + // script/lint is a docker build; a lint step that shelled out to it would + // recurse. + it("does not call script/lint or make lint", () => { + expect(has(dockerfileLint, /make lint|script\/lint/)).toBe(false); + }); +}); + +describe("Dockerfile", () => { + // `make check` runs script/lint, which is a docker build, so an image that + // ran it would need a Docker daemon inside the container. + it("does not run make check, make lint or script/lint", () => { + expect( + has(dockerfile, /make check|make lint|script\/(check|lint)/), + ).toBe(false); + }); + + // The replaced lint stage took a `COPY --from=lint` dependency to order + // itself before the check stage. Dockerfile.lint is that stage now, and + // two definitions of how to lint is one too many. + it("has no lint stage", () => { + expect(has(dockerfile, /AS lint\b|--from=lint\b/)).toBe(false); + }); + + it("still runs the suite and the build under the epoch guard", () => { + expect(has(dockerfile, /^RUN make test$/)).toBe(true); + expect(has(dockerfile, /^RUN make build$/)).toBe(true); + expect( + has(dockerfile, /^RUN \[ -n "\$CHECK_EPOCH" \] \|\| exit 1$/), + ).toBe(true); + }); +}); + +describe("script/cibuild", () => { + // CI has to get both verdicts. Lint goes first so the fast failure is + // reported before the suite runs. + it("builds the lint image before the test and build image", () => { + const lint = cibuild.findIndex((line) => /\/lint"/.test(line)); + const check = cibuild.findIndex((line) => + /docker build .*CHECK_EPOCH/.test(line), + ); + + expect(lint).toBeGreaterThanOrEqual(0); + expect(check).toBeGreaterThan(lint); + }); +}); + +describe("package.json", () => { + // `yarn lint` was a second, unpinned way to get a lint verdict, from + // whatever eslint the working tree happened to have installed. + it("exposes no host lint script", () => { + const pkg = JSON.parse(read("package.json")) as { + scripts: Record; + }; + expect(pkg.scripts.lint).toBeUndefined(); + }); +}); diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts new file mode 100644 index 0000000..16fbad1 --- /dev/null +++ b/test/packaging/lint-once.test.ts @@ -0,0 +1,503 @@ +// `make check` used to run `prettier --check .` twice: once inside the lint +// container (`script/lint` builds `Dockerfile.lint`, which runs eslint and +// prettier as build steps) and once again on the host, because `script/check` +// also called `script/fmt-check`. Two passes, one verdict, and the host one is +// the weaker of the two — its prettier is whatever the working tree happens to +// have installed, while the container's is digest-pinned and installed under +// `--frozen-lockfile`. +// +// The fix was to delete the host call from `script/check` and `script/precommit`. +// Nothing about that fix is self-enforcing: anyone can wire `script/fmt-check` +// back in, or add a prettier step to a Dockerfile, and every build stays green +// while quietly doing the work twice again. So the count is asserted here +// rather than promised in a comment. +// +// The assertion is a static walk of the invocation graph, not a string match +// 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 ` +// 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. +// The CI walk starts at the workflow file rather than at a hand-picked script, +// so "the path CI executes" is read out of the repo instead of assumed; it +// reaches `script/cibuild`, and through it the `Dockerfile` image that `make +// check` never touches. Walking only `make check` is how a duplicate prettier +// pass in `Dockerfile` stayed invisible. +// +// 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 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"; +import { join } from "node:path"; + +const repoRoot = fileURLToPath(new URL("../../", import.meta.url)); + +const read = (name: string): string => + readFileSync(join(repoRoot, name), "utf-8"); + +// A backslash at end of line continues the command; the resolver has to see the +// whole invocation, since the interesting flags (`-f Dockerfile.lint`) can sit +// on the continuation. +const joinContinuations = (text: string): string[] => { + const joined: string[] = []; + for (const raw of text.split("\n")) { + const line = raw.trim(); + const previous = joined[joined.length - 1]; + if (previous !== undefined && previous.endsWith("\\")) { + joined[joined.length - 1] = + `${previous.slice(0, -1).trim()} ${line}`; + } else { + joined.push(line); + } + } + return joined; +}; + +// Comments are stripped everywhere. The headers of these scripts explain the +// duplication this test exists to prevent, and therefore name `prettier` and +// `script/fmt-check` repeatedly; counting them would make the test assert the +// prose instead of the behaviour. +const executable = (text: string): string[] => + joinContinuations(text).filter( + (line) => line !== "" && !line.startsWith("#"), + ); + +// Every occurrence, not "does this line mention prettier": a line that reads +// `yarn run prettier --check . && yarn run prettier --check src` is two passes +// over the same tree, which is exactly the bug this file exists to catch, and +// counting it as one would hide it. `.prettierrc` and `.prettierignore` are not +// invocations and do not match, because `\b` requires a non-word character +// after the name. +const countPrettier = (line: string): number => + (line.match(/\bprettier\b/g) ?? []).length; + +// 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. +const makeRecipes = (): Map => { + const recipes = new Map(); + let current: string | null = null; + for (const raw of read("Makefile").split("\n")) { + if (raw.startsWith("\t")) { + if (current !== null) { + recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, "")); + } + continue; + } + 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 = makeRecipes(); + +const packageScripts = (): Record => { + const pkg = JSON.parse(read("package.json")) as { + scripts?: Record; + }; + return pkg.scripts ?? {}; +}; + +const scripts = packageScripts(); + +// 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 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 + // 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)) + .map((line) => line.replace(/^-?\s*run:\s*/, "")); + } + if (node.startsWith("make:")) { + const target = node.slice("make:".length); + const recipe = recipes.get(target); + // A renamed or deleted target must be a loud failure: silently walking + // an empty recipe would report zero prettier invocations, which reads + // like the tidiest possible result. + if (recipe === undefined) { + throw new Error(`no such Makefile target: ${target}`); + } + return recipe; + } + if (node.startsWith("yarn:")) { + const name = node.slice("yarn:".length); + const script = scripts[name]; + if (script === undefined) { + throw new Error(`no such package.json script: ${name}`); + } + return [script]; + } + throw new Error(`unresolvable node: ${node}`); +}; + +// 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. +const commandsOf = (node: string): string[] => { + const commands = resolve(node); + if (commands.length === 0) { + throw new Error(`node resolved to no commands: ${node}`); + } + return commands; +}; + +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( + /(?:\$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. + 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. + 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. + if (/\bdocker\s+build\b/.test(line)) { + const file = /\s-f\s+(\S+)/.exec(line); + edges.push(`docker:${file === null ? "Dockerfile" : file[1]}`); + } + + return edges; +}; + +interface Walk { + prettier: number; + reached: Set; +} + +// Repeated invocations must count repeatedly — running the same script twice is +// exactly the bug — so nodes are not deduplicated. The path stack is only there +// to turn a cycle into a loud failure instead of a hang. +// +// Counting and edge-following both happen for every line: a line that invokes +// prettier can also invoke something else, and skipping the edges of counted +// lines silently truncated the graph. +const walk = (node: string, path: string[] = [], into?: Walk): Walk => { + const result = into ?? { prettier: 0, reached: new Set() }; + if (path.includes(node)) { + throw new Error(`invocation cycle: ${[...path, node].join(" -> ")}`); + } + result.reached.add(node); + + for (const line of commandsOf(node)) { + result.prettier += countPrettier(line); + for (const edge of edgesOf(line)) { + walk(edge, [...path, node], result); + } + } + return result; +}; + +describe("prettier runs exactly once per make check", () => { + const check = walk("make:check"); + + // The headline assertion, and the one the issue is about. + it("invokes prettier once for the whole of make check", () => { + expect(check.prettier).toBe(1); + }); + + // Guards against the count being 1 (or 0) because the walk never got + // anywhere. `make check` has to reach the suite, the lint script, and the + // Dockerfile whose build IS the lint verdict. + it.each(["script/check", "script/test", "script/lint", "Dockerfile.lint"])( + "reaches %s while counting", + (node) => { + const key = node.startsWith("script/") ? node : `docker:${node}`; + expect([...check.reached]).toContain(key); + }, + ); + + // The one that survives is the container's, not the host's: that is the + // authoritative verdict, since a successful Dockerfile.lint build is what + // CI treats as proof of a clean tree. + it("keeps the surviving invocation inside the lint container", () => { + expect(walk("docker:Dockerfile.lint").prettier).toBe(1); + }); + + it("does not reach the host formatting check from make check", () => { + expect([...check.reached]).not.toContain("script/fmt-check"); + }); +}); + +describe("prettier runs exactly once per CI build", () => { + // Rooted at the workflow file, so this is the graph CI executes rather than + // the graph someone believed CI executes. `make check` cannot stand in for + // it: CI runs script/cibuild, which builds Dockerfile as well as + // Dockerfile.lint, and nothing under `make check` ever reads Dockerfile. + const ci = walk("workflow:.gitea/workflows/check.yml"); + + it("invokes prettier once for the whole CI build", () => { + expect(ci.prettier).toBe(1); + }); + + // script/cibuild is here because the workflow is asserted to run it; + // Dockerfile is here because it is the half of the CI graph that the + // `make check` walk cannot see. + it.each([ + "script/cibuild", + "script/lint", + "docker:Dockerfile.lint", + "docker:Dockerfile", + ])("reaches %s while counting", (node) => { + expect([...ci.reached]).toContain(node); + }); + + // The test and build image must not lint: linting is Dockerfile.lint's job, + // and a prettier step added here would be a second pass over the same tree + // for the same verdict — on the one path where it matters most. + it("keeps prettier out of the test and build image", () => { + expect(walk("docker:Dockerfile").prettier).toBe(0); + }); +}); + +describe("the standalone entrypoints still do what their names say", () => { + // REPO_POLICIES.md requires both `make lint` and `make fmt-check` to exist + // and mean something. Dropping fmt-check from script/check must not turn it + // into a target nobody can use, and must not leave `make check` passing + // because both halves became no-ops. + it("still checks formatting under make fmt-check", () => { + expect(walk("make:fmt-check").prettier).toBe(1); + }); + + it("still checks formatting under make lint", () => { + expect(walk("make:lint").prettier).toBe(1); + }); +}); + +describe("script/precommit", () => { + // Same duplication as script/check, same fix. The hook still catches a + // badly formatted tree before the commit lands, because script/lint is the + // container prettier run — that is the whole reason the host call could go. + it("checks formatting exactly once", () => { + expect(walk("script/precommit").prettier).toBe(1); + }); + + it("gets that check from the lint container", () => { + expect([...walk("script/precommit").reached]).toContain( + "docker:Dockerfile.lint", + ); + }); +}); + +// script/bootstrap installs the dependencies, and it has two install sites: one +// for the case where yarn has to be reached through nvm, and one for the case +// where yarn is already on PATH. A substring check against the whole file +// cannot tell them apart, so it reports the first and says nothing about the +// second — which is the one the containers take, because the pinned node image +// ships yarn. Both are resolved separately here. +const installBranches = (): { withoutYarn: string[]; withYarn: string[] } => { + const lines = executable(read("script/bootstrap")); + const open = lines.findIndex((line) => + /^install_js_deps\s*\(\)/.test(line), + ); + if (open === -1) { + throw new Error("script/bootstrap: no install_js_deps function"); + } + const close = lines.indexOf("}", open); + const body = lines.slice(open + 1, close === -1 ? undefined : close); + 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) { + throw new Error( + "script/bootstrap: install_js_deps is not the expected " + + "if missing yarn / else / fi shape", + ); + } + return { + withoutYarn: body.slice(guard + 1, otherwise), + withYarn: body.slice(otherwise + 1, end), + }; +}; + +// Every `yarn install` in the given lines, with its flags, so an unpinned +// install cannot hide next to a pinned one. +const yarnInstalls = (lines: string[]): string[] => + lines.flatMap((line) => + [...line.matchAll(/\byarn install\b[^"'&|;]*/g)].map((match) => + match[0].trim(), + ), + ); + +describe("host and container prettier cannot disagree", () => { + // With the host pass gone from `make check`, `make fmt-check` is the only + // host-side formatting check left, and the container is the gate. The two + // must keep producing the same verdict on the same tree, or a developer + // running `make fmt-check` gets a green that CI then rejects. + // + // Three things make them agree, and all three are load-bearing: + it("pins the same prettier for both", () => { + const pkg = JSON.parse(read("package.json")) as { + devDependencies: Record; + }; + // An exact version, not a range: `^3.8.1` would let the container and + // the host resolve different builds with different formatting. + expect(pkg.devDependencies.prettier).toMatch(/^\d+\.\d+\.\d+$/); + }); + + it("installs from the lockfile on the branch the container takes", () => { + // Both images are FROM a node image, which ships yarn, so `missing + // yarn` is false and this is the branch that runs in the container. + const installs = yarnInstalls(installBranches().withYarn); + expect(installs).not.toHaveLength(0); + for (const install of installs) { + expect(install).toContain("--frozen-lockfile"); + } + }); + + it("installs from the lockfile on the nvm branch too", () => { + // Not the container's branch, but it is the one a developer without + // yarn on PATH gets, and their prettier has to match the container's. + const installs = yarnInstalls(installBranches().withoutYarn); + expect(installs).not.toHaveLength(0); + for (const install of installs) { + expect(install).toContain("--frozen-lockfile"); + } + }); + + it("runs script/bootstrap inside the lint container", () => { + // Without this the lockfile assertions above would be about a script + // the container never executes. + expect([...walk("docker:Dockerfile.lint").reached]).toContain( + "script/bootstrap", + ); + }); + + it("keeps .gitignore in the build context", () => { + // Prettier 3 reads .gitignore as a default ignore file, so excluding it + // from the context would change which files the container checks. + const dockerignore = read(".dockerignore") + .split("\n") + .map((line) => line.trim()); + expect(dockerignore).not.toContain(".gitignore"); + }); +}); + +describe("the walk cannot pass vacuously", () => { + // An earlier draft of this file computed a Makefile target as + // `node.slice("make:")` — a string where a number belongs, which coerces to + // NaN and made every target resolve to nothing. The count went to zero and + // an assertion of "not twice" would have been satisfied by a walk that had + // read nothing at all. Every way of reaching nothing is therefore an + // error here, and the ways are tested rather than assumed. + it("reports zero for a subgraph that does not run prettier", () => { + expect(walk("make:clean").prettier).toBe(0); + }); + + it("refuses a Makefile target that does not exist", () => { + expect(() => walk("make:no-such-target")).toThrow( + /no such Makefile target/, + ); + }); + + it("refuses a package.json script that does not exist", () => { + expect(() => walk("yarn:no-such-script")).toThrow( + /no such package.json script/, + ); + }); + + it("refuses a script that does not exist", () => { + expect(() => walk("script/no-such-script")).toThrow(/ENOENT/); + }); + + it("refuses a node that resolves to no commands", () => { + // .dockerignore has no RUN steps, standing in for a Dockerfile whose + // steps a restructure moved somewhere the resolver cannot see. + expect(() => walk("docker:.dockerignore")).toThrow( + /resolved to no commands/, + ); + }); + + it("refuses a node kind it does not understand", () => { + expect(() => walk("nonsense")).toThrow(/unresolvable node/); + }); + + it("refuses to walk in circles", () => { + expect(() => walk("make:check", ["script/check"])).toThrow( + /invocation cycle/, + ); + }); +}); + +describe("the resolver reads what the shell would run", () => { + // Counting per line is how `yarn run prettier --check . && yarn run + // prettier --check src` read as a single invocation. + it("counts every prettier invocation on a line", () => { + expect( + countPrettier( + "yarn run prettier --check . && yarn run prettier --check src", + ), + ).toBe(2); + }); + + it("does not count the config files as invocations", () => { + expect(countPrettier("COPY .prettierrc .prettierignore ./")).toBe(0); + }); + + // The counting `continue` also dropped every edge that shared a line with a + // prettier call, so a whole subtree could be hidden behind one `&&`. + it("still follows the edges of a line that invokes prettier", () => { + expect( + edgesOf('yarn run prettier --check . && "$SCRIPT_DIR/lint"'), + ).toContain("script/lint"); + }); + + it("resolves every spelling of a script call to one node", () => { + expect( + edgesOf('"$SCRIPT_DIR/lint" "${SCRIPT_DIR}/test" script/fmt'), + ).toEqual(["script/lint", "script/test", "script/fmt"]); + }); + + it("follows a bare docker build to Dockerfile and -f to its file", () => { + expect(edgesOf("docker build .")).toContain("docker:Dockerfile"); + expect(edgesOf("docker build -f Dockerfile.lint .")).toContain( + "docker:Dockerfile.lint", + ); + }); + + it("reads the run steps of the CI workflow and not its uses steps", () => { + expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([ + "script/cibuild", + ]); + }); +});