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"); + }); +});