From a73f0abbe8147753e66611c2af90128bd3b25ece Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 13:05:03 +0000 Subject: [PATCH] Check formatting once per make check, in the container (closes #29) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit script/check ran script/test, script/lint and script/fmt-check. Since linting moved into Docker, script/lint is a build of Dockerfile.lint, which runs `prettier --check .` as a build step — so make check checked formatting twice over the same tree: once in the container and once on the host. script/precommit had the same pair. Drop the script/fmt-check call from both. The container keeps the check, because a successful Dockerfile.lint build is what CI treats as proof of a clean tree, and it is the stronger of the two verdicts: its prettier is digest-pinned and installed under --frozen-lockfile, while the host's is whatever the working tree happens to have. The pre-commit hook is unchanged in what it catches — script/lint still fails a badly formatted tree, and therefore the commit. script/fmt-check survives as a standalone entrypoint, as REPO_POLICIES.md requires, for asking the formatting question by itself without docker. Its verdict cannot drift from the container's: prettier is pinned to an exact version, installed from yarn.lock in both places, and reads .gitignore as its default ignore file, which is why .dockerignore keeps .gitignore in the build context. The count is asserted rather than promised. test/packaging/lint-once.test.ts walks the invocation graph from each entrypoint — through the Makefile shims, the script/ calls, the package.json scripts and the docker build into Dockerfile.lint's RUN steps — and counts prettier invocations: one per make check, one per script/precommit, and one each for make lint and make fmt-check alone, so neither can become a no-op that satisfies the count trivially. The walk also asserts which nodes it reached, so a restructure that defeats the resolver fails the test instead of quietly counting zero. Observed: 2 prettier invocations per make check before, 1 after. --- README.md | 49 ++++-- TODO.md | 13 ++ script/check | 14 +- script/precommit | 15 +- test/packaging/lint-once.test.ts | 276 +++++++++++++++++++++++++++++++ 5 files changed, 345 insertions(+), 22 deletions(-) create mode 100644 test/packaging/lint-once.test.ts diff --git a/README.md b/README.md index 89d5533..0d1d7d9 100644 --- a/README.md +++ b/README.md @@ -89,17 +89,18 @@ alpine. We provide: - `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/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 @@ -113,6 +114,22 @@ 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 @@ -184,10 +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 suite 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 @@ -502,11 +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, - which builds `Dockerfile.lint` and therefore needs docker) and - `make fmt-check` must pass. The pre-commit hook enforces this. `make check` - (which also runs tests) must pass before merging to `main`. Never invoke - eslint or prettier directly; linting runs in the container only. +- **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 78b257f..0a73af7 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,19 @@ Update the README API reference section to match the current implementation. # Completed Steps +- 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 diff --git a/script/check b/script/check index 74baf3c..2f37914 100755 --- a/script/check +++ b/script/check @@ -1,6 +1,15 @@ #!/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 @@ -12,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/precommit b/script/precommit index 9344d32..489e72d 100755 --- a/script/precommit +++ b/script/precommit @@ -2,10 +2,16 @@ # 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 script/cibuild, which builds both images and so 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. @@ -15,7 +21,6 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" main() { "$SCRIPT_DIR/lint" - "$SCRIPT_DIR/fmt-check" } main "$@" diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts new file mode 100644 index 0000000..527fb3e --- /dev/null +++ b/test/packaging/lint-once.test.ts @@ -0,0 +1,276 @@ +// `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 — `"$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. +// +// Undercounting is the failure mode that would make this test worthless, so the +// walk is also asserted to have reached the nodes that matter: if a restructure +// defeats the resolver, the reachability assertions fail rather than the count +// silently dropping to zero and "passing". +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("#"), + ); + +// 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:`. +const commandsOf = (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)); + } + 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:")) { + return [scripts[node.slice("yarn:".length)] ?? ""]; + } + throw new Error(`unresolvable node: ${node}`); +}; + +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 + // below, 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. +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)) { + if (/\bprettier\b/.test(line)) { + result.prettier += 1; + continue; + } + 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("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", + ); + }); +}); + +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 it from the lockfile in the container", () => { + // script/bootstrap is what Dockerfile.lint runs to install deps. + expect(read("script/bootstrap")).toContain( + "yarn install --frozen-lockfile", + ); + }); + + 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"); + }); +});