sneak/quak
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Run all linting in Docker via Dockerfile.lint (closes #30) #31

Open
clawbot wants to merge 3 commits from next into main
pull from: next
merge into: sneak:main
Conversation 8 Commits 3 Files Changed 14 +925 -70
14 changed files with 925 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
Dockerfile
View File

@@ -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

35
Dockerfile.lint Normal file
View File

@@ -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 .

105
README.md
View File

@@ -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`.

40
TODO.md
View File

@@ -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`

1
package.json
View File

@@ -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 ."
},

18
script/check
View File

@@ -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 "$@"

21
script/cibuild
View File

@@ -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)" .
}

3
script/docker
View File

@@ -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)"

17
script/lint
View File

@@ -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 "$@"

18
script/precommit
View File

@@ -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 "$@"

17
test/packaging/build-context.test.ts
View File

@@ -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 `<dockerfile>.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,
);
},
);
});

6
test/packaging/entrypoints.test.ts
View File

@@ -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.
//

184
test/packaging/lint-docker.test.ts Normal file
View File

@@ -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<string, string>;
};
expect(pkg.scripts.lint).toBeUndefined();
});
});

503
test/packaging/lint-once.test.ts Normal file
View File

@@ -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/<name>"` and `script/<name>` into other scripts, `make <target>`
// through the Makefile shims, `yarn run <name>` through the `package.json`
// scripts, and `docker build -f <file>` 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 <target>` 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<string, string[]> => {
const recipes = new Map<string, string[]>();
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<string, string> => {
const pkg = JSON.parse(read("package.json")) as {
scripts?: Record<string, string>;
};
return pkg.scripts ?? {};
};
const scripts = packageScripts();
// Node keys: `script/<name>`, `docker:<Dockerfile>`, `make:<target>`,
// `yarn:<package.json script>`, `workflow:<CI workflow file>`.
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<string>;
}
// 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<string>() };
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<string, string>;
};
// 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",
]);
});
});