From 35858dab66e6d3ba806a6544393c6f888b59d58e Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 12:49:34 +0000 Subject: [PATCH] Run every lint in a container via Dockerfile.lint (closes #40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The linter is no longer installed on the host and no longer invoked there. script/lint is now `docker build -f Dockerfile.lint .` and nothing else, with the linter running as a build step, so a successful build of that file is a clean lint — and it works unchanged where the docker daemon is remote and bind mounts are impossible. That removes three host-only failure mechanisms rather than mitigating them: the result cache keyed on file content rather than location, which produced a confirmed false green and a string of findings reported against other checkouts; the host-global $TMPDIR/golangci-lint.lock, which fails a run with `parallel golangci-lint is running` in a way no caller can distinguish from findings; and host/container version skew, which hid thirteen findings on one repo. A container per run has its own cache, its own lock and a binary pinned by digest. Resolving the recursion this creates. script/lint is a docker build, so a Dockerfile that runs `make check` would nest a build inside a build step where there is no daemon. Fixed by direction, not detection: the main Dockerfile runs script/test and script/fmt-check individually, with a comment saying why `make check` must not come back, and script/cibuild runs script/lint first for fail-fast feedback. script/check still runs all three, so developers and the pre-commit hook are unaffected. Dockerfile.lint carries the same CHECK_EPOCH guard as the main image, with the ARG placed below the dependency layer so only the lint steps re-run. Blanket --no-cache was rejected: it re-runs the dependency install on every lint and makes linting network-dependent. golangci-lint config verify is kept, on measurement rather than preference. Under the pinned v2.12.2, a bogus top-level key and a bogus key nested under linters.settings.lll both pass `golangci-lint run` with exit 0 and `0 issues` while config verify exits 3 and names them; an unknown linter name fails run and passes config verify. The two catch disjoint classes, and `run` alone silently ignores the class where a threshold reads as configured and is not applied. The concern that config verify fetches its JSON schema over live HTTPS does not hold for this version: every case reproduced byte-identically under `docker run --network none`, in a container where `getent hosts golangci-lint.run` exits 2. The schema is embedded in the pinned binary. Two canonical forms are superseded and deleted rather than left standing beside the new one, because consuming repos read these documents literally and two contradictory canonical script/lint forms is worse than either. The script/bootstrap golangci-lint install landed for https://git.eeqj.de/sneak/prompts/issues/28 is removed: nothing invokes a host linter now, so it can only reintroduce the skew it was written to close. Its version-enforcement principle — compare version not presence, re-resolve through PATH after installing, let a mis-parse fall through to reinstall, and call it — stays documented for any other pinned host tool. The per-checkout GOLANGCI_LINT_CACHE/TMPDIR wrapper is removed with it; its entire subject was making a host run trustworthy. Adopting repos delete .lint-cache/ from .gitignore and .dockerignore too. The Go multistage lint stage and its COPY --from=lint ordering trick go the same way: that stage ran `make lint`, which is now a docker build. Corrected everywhere the claim that a successful docker build implies lint passed — REPO_POLICIES.md, both repo checklists, the Go styleguide and the README. The guarantee now belongs to script/cibuild, which runs both container builds; a bare `docker build .` never lints at all. Verified in this repo, not only documented: two consecutive script/lint runs on a byte-identical tree both executed prettier (4.556s and 3.738s, lint layers DONE with a fresh epoch printed, dependency layers CACHED as intended); a planted violation failed the build naming the file, and reverting it went green; a bare `docker build -f Dockerfile.lint .` failed on the guard; make check, script/docker and script/cibuild all green with the check layers demonstrably executing; and the main image build completed without attempting a nested build. Rework, from independent review of this commit. The canonical text is the deliverable here, so a false sentence is a fleet-wide defect: the Dockerfile rule still said the build "fails if the branch is not green", which stopped being true when lint left that image, and the earlier sweep grepped one phrasing rather than the claim. Re-swept on the claim itself — green/red-branch wording, build-fails wording, entailment verbs near build/lint/check, and "linted" asserted as covered — across prompts/, README.md, TODO.md, both Dockerfiles and every script. Two absolutes are narrowed to what is actually true, because seventeen repos adopt this literally. The rule is that no lint VERDICT may come from a host invocation, not that the binary never exists on the host: a JS repo's `yarn install` puts its linter in node_modules on the host unavoidably, and in a repo whose formatter is its linter — this one — `script/fmt-check` runs the same command that Dockerfile.lint runs. That gap is now stated with its bound (the version is pinned in the repo's own node_modules, so no shared cache, no host lock, nothing to skew) and the audit grep keeps its reach, gaining a note on which two hits are expected rather than being weakened. script/lint conflates "found issues" with "could not run": docker build exits 1 for both. The exit-75 VOID machinery is deliberately not restored, and the reasoning is now recorded where a reader looking for it lands. The dangerous direction is already closed, since a build that cannot run fails closed and can never read as clean; BuildKit already names the failing step, where the old lock error went to stderr while findings went to stdout and was easy to lose; and the failure is not transient, so the retry that justified the old machinery would be wrong here. Rebuilding the distinction would mean per-invocation capture files and traps again plus matching on BuildKit's message format, which is not a stable interface, and a mis-match in the "treat as infrastructure" direction would be the false green this rule exists to prevent. What survives is binding as a reading rule: a run that did not reach the lint step is not a verdict. Also corrected: script/lint was listed above a CHECK_EPOCH snippet that does a bare `docker build .` with no -f, which would have built the main image and linted nothing; the canonical Go Dockerfile template used `make fmt-check` / `make test` where every prose rule in the same document says script/, one Makefile edit away from re-entering the recursion; README omitted the mandatory VERSION build arg; script/docker did not say lint had left its image, a comment that propagates fleet-wide; the "byte-identical across repos" claim for script/lint is narrowed to its executable lines; and "--no-cache makes linting network-dependent" is softened to the measured comparative claim. --- Dockerfile | 11 +- Dockerfile.lint | 41 ++ README.md | 27 +- TODO.md | 42 ++ prompts/CODE_STYLEGUIDE_GO.md | 32 +- prompts/EXISTING_REPO_CHECKLIST.md | 66 +- prompts/NEW_REPO_CHECKLIST.md | 77 ++- prompts/REPO_POLICIES.md | 1001 ++++++++++++---------------- script/check | 7 + script/cibuild | 17 +- script/docker | 6 + script/lint | 26 +- 12 files changed, 689 insertions(+), 664 deletions(-) create mode 100644 Dockerfile.lint diff --git a/Dockerfile b/Dockerfile index ac7232a..afdb9f8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -25,4 +25,13 @@ COPY . . # here, not one. Keep both. ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 -RUN echo "check epoch: ${CHECK_EPOCH}" && make check + +# The individual non-lint checks, NOT `make check`. Lint is deliberately +# absent here: `script/lint` is itself a `docker build` (of +# Dockerfile.lint), so running `make check` in this image would attempt +# a docker build inside a build step, where there is no daemon. Putting +# `make check` back reintroduces exactly that recursion. Lint is not +# skipped — script/cibuild runs script/lint first, in its own container, +# before this build starts. +RUN echo "check epoch: ${CHECK_EPOCH}" && script/test +RUN script/fmt-check diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..b045aba --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,41 @@ +# Lint-only image. `script/lint` builds this file and nothing else: the +# linter runs as a build step, so a successful build IS a clean lint. +# Building rather than bind-mounting is what makes it work where the +# docker daemon is remote and bind mounts are impossible. +# +# The linter is invoked directly below rather than through `make lint`. +# That is not a style choice: `script/lint` IS this build, so calling it +# from inside would recurse into a docker build with no daemon. +# +# This repo's linter is prettier over markdown. A Go repo's version of +# this file differs only in the base image and the two lint commands; +# see the containerised-lint rule in prompts/REPO_POLICIES.md. +# +# node 22-alpine, 2026-02-22 +FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e34 + +WORKDIR /app + +# Dependency layer first, and deliberately above the ARG below, so it +# stays cached and only the lint steps re-run on every invocation. +# Without that ordering the cache-bust would reinstall dependencies on +# every lint and make linting network-dependent. +COPY script/ script/ +COPY package.json yarn.lock ./ +RUN script/bootstrap + +COPY . . + +# CHECK_EPOCH is a per-invocation nonce supplied by script/lint. Without +# it an unchanged tree serves the lint layer from cache and the build +# reports a lint it never ran — a green that proves nothing, which is +# the whole failure mode this file exists to avoid reintroducing. The +# guard makes a bare `docker build -f Dockerfile.lint .` fail loudly +# instead of silently reusing the empty (and therefore stable) cache +# key. The value is expanded into the lint command as well, so the cache +# miss does not depend on BuildKit's handling of an unreferenced ARG and +# the epoch is visible in the build log. Keep both references. +ARG CHECK_EPOCH +RUN [ -n "$CHECK_EPOCH" ] || exit 1 +RUN echo "lint epoch: ${CHECK_EPOCH}" && \ + yarn run prettier --check '**/*.md' --tab-width 4 --prose-wrap always diff --git a/README.md b/README.md index 357f8df..f3b15e0 100644 --- a/README.md +++ b/README.md @@ -117,19 +117,32 @@ alpine. We provide: - `script/projectname` — output the project name (our own extension); used by `script/docker` for the image tag - `script/test` — run the test suite (no tests defined here) -- `script/lint` — lint the markdown files with prettier +- `script/lint` — lint the markdown files, by building `Dockerfile.lint`. The + lint verdict comes only from the container; nothing on the host produces one. + (The prettier in `node_modules` that `script/fmt` and `script/fmt-check` use + is the same binary, which is why this says "verdict" rather than "never on the + host" — see the scope note in `prompts/REPO_POLICIES.md`.) Linting happens as + a build step, so a successful build is a clean lint, and the same + per-invocation `CHECK_EPOCH` nonce used elsewhere is what stops Docker serving + that lint from cache on an unchanged tree. A failure that names no finding — + daemon down, image unpullable — is not a lint result: read which build step + failed, fix that, and re-run - `script/fmt` — format all markdown files with prettier (writes) - `script/fmt-check` — check formatting (read-only) - `script/check` — run all checks: `test`, `lint`, `fmt-check` (our own - extension) + extension). Needs a docker daemon, since `script/lint` is a container build - `script/docker` — build the Docker image, tagged via `script/projectname` (byte-identical across repos); passes the same `CHECK_EPOCH` nonce as `script/cibuild` -- `script/cibuild` — cd to the repo root, assign `epoch="$(date +%s%N)$$"`, then - `docker build --build-arg CHECK_EPOCH="$epoch" .` (what CI runs; the image - build runs `script/check`, and the per-invocation `CHECK_EPOCH` nonce is what - stops Docker serving that check from cache on an unchanged tree — a bare - `docker build .` fails closed on purpose) +- `script/cibuild` — cd to the repo root, run `script/lint` first, then assign + `epoch="$(date +%s%N)$$"` and `version="$(git describe ...)"` on their own + lines and + `docker build --build-arg CHECK_EPOCH="$epoch" --build-arg VERSION="$version" .` + (what CI runs; both build args are mandatory). Two container builds: the lint + image, then the main image, which runs `script/test` and `script/fmt-check` + but deliberately not `make check` — that would nest a docker build inside a + build step. So `script/cibuild` is what proves the branch green; a bare + `docker build .` never lints, and fails closed on purpose - `script/precommit` — run by the git pre-commit hook (our own extension); calls `script/check` - `script/install-precommit` — installs the git pre-commit hook (our own diff --git a/TODO.md b/TODO.md index 7965142..64b6863 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,48 @@ fmt-check, and commit. # Completed Steps +- 2026-08-10: Moved every lint run into a container, on the owner's ruling, and + made this repo do it rather than merely document it. `script/lint` is now + `docker build -f Dockerfile.lint .` and nothing else; the linter is never + installed on the host and never invoked there, so a run cannot inherit another + checkout's content-keyed result cache, the host-global + `$TMPDIR/golangci-lint.lock`, or a host toolchain that differs from the pinned + one — the three mechanisms behind a confirmed false green, a string of + findings reported against other agents' checkouts, and a container that saw + thirteen findings the host missed. Linting runs as a build step, so a + successful build is a clean lint, which also works where the docker daemon is + remote and bind mounts are impossible. The recursion this creates is resolved + by direction rather than by detection: the main `Dockerfile` runs the + individual non-lint checks instead of `make check`, and `script/cibuild` runs + `script/lint` first, so no build ever nests a build. `Dockerfile.lint` carries + the same `CHECK_EPOCH` guard as the main image, with the `ARG` below the + dependency layer so only the lint steps re-run — blanket `--no-cache` was + rejected because it makes every lint reinstall its dependencies over the + network. Two canonical forms were superseded rather than left standing beside + the new one, since consuming repos read this document literally: the + `script/bootstrap` golangci-lint install (nothing runs a host linter now, so + it can only reintroduce skew; the version-enforcement principle stays + documented for other pinned host tools) and the per-checkout + cache/lock/`.lint-cache` wrapper (its whole subject was making a host run + trustworthy). The Go multistage lint stage goes with them: it ran `make lint`, + which is now a docker build. `golangci-lint config verify` was kept on + measurement, not preference — a bogus config key passes `golangci-lint run` + with `0 issues` and fails `config verify`, and every case reproduced + byte-identically under `docker run --network none`, so the schema is embedded + in the pinned binary and the line costs no network. Two positions are stated + rather than left as gaps, because canon that omits them gets re-derived + wrongly: `docker build` returns 1 both for findings and for a build that never + reached the lint step, and the exit-75 VOID machinery is deliberately not + restored — the dangerous direction is closed since an unrunnable lint fails + closed, BuildKit already names the failing step, and the failure is not + transient, so what survives is the reading rule that a run which did not lint + is not a verdict. And the rule is about linters: `script/fmt` and + `script/fmt-check` run on the host by necessity, which in a repo whose + formatter is its linter — this one — means that exact command does run there, + recorded as a known and bounded gap rather than papered over. Verified with + two consecutive runs on an unchanged tree both executing the linter, a planted + violation caught and reverted, the bare-build guard firing, and the main image + building without attempting a nested build. - 2026-08-09: Made a golangci-lint result belong to the tree that asked for it. REPO_POLICIES.md now carries the canonical Go `script/lint`, which gives the linter per-checkout `GOLANGCI_LINT_CACHE` and per-checkout `TMPDIR`. The two diff --git a/prompts/CODE_STYLEGUIDE_GO.md b/prompts/CODE_STYLEGUIDE_GO.md index be44685..40c95ba 100644 --- a/prompts/CODE_STYLEGUIDE_GO.md +++ b/prompts/CODE_STYLEGUIDE_GO.md @@ -1,6 +1,6 @@ --- title: Code Styleguide — Go -last_modified: 2026-08-09 +last_modified: 2026-08-10 --- 1. Try to hard wrap long lines at 77 characters or less. @@ -111,17 +111,27 @@ last_modified: 2026-08-09 1. For anything beyond a simple script or tool, or anything that is going to run in any sort of "production" anywhere, make sure it passes - `golangci-lint`. + `golangci-lint`. Run it with `make lint`, which builds `Dockerfile.lint`: + the linter runs in a container, always, and `golangci-lint` is not installed + on the host at all — no repo's `script/bootstrap` installs it any more. A + `golangci-lint` invoked directly on a shared host reads a result cache keyed + on file content rather than location and a host-global lock, so its answer + may belong to another checkout entirely, and a `make lint` is the only + verdict worth recording. A failure that names no finding is not a verdict + either: read which build step failed before concluding anything. -1. Write a `Dockerfile` for every repo, even if it only runs the tests and - linting. `script/cibuild` and `script/docker` should always make sure that - the code is in an able-to-be-compiled state, linted, and any tests run, and - the build should fail if linting doesn't pass. That guarantee holds only - because those scripts pass a per-invocation `CHECK_EPOCH` build arg that - busts the check layers out of the Docker cache; without it an unchanged tree - serves those layers from cache and the build reports a green it never ran. A - bare `docker build .` fails closed by design, on the `[ -n "$CHECK_EPOCH" ]` - guard — always go through `script/cibuild` or `script/docker`. See +1. Write a `Dockerfile` for every repo, even if it only runs the tests. It runs + the non-lint checks; linting lives in `Dockerfile.lint` and is run by + `script/cibuild` before the main build, because `script/lint` is itself a + `docker build` and cannot run inside one. So `script/cibuild` is what + guarantees the code is in an able-to-be-compiled state, linted, and tested — + **a successful `docker build .` on its own does not, because it never + lints.** That guarantee holds only because each build passes a + per-invocation `CHECK_EPOCH` build arg that busts its check layers out of + the Docker cache; without it an unchanged tree serves those layers from + cache and the build reports a green it never ran. A bare `docker build .` + fails closed by design, on the `[ -n "$CHECK_EPOCH" ]` guard — always go + through `script/cibuild`, `script/docker` or `script/lint`. See [Repository Policies](https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md) for the canonical form. diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 08e6d0b..38b31be 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -1,6 +1,6 @@ --- title: Existing Repo Checklist -last_modified: 2026-08-09 +last_modified: 2026-08-10 --- Use this checklist when beginning work in a repo that may not yet conform to our @@ -36,12 +36,23 @@ with your task. `https://git.eeqj.de/sneak/prompts/raw/branch/main/.editorconfig` - [ ] `Dockerfile` and `.dockerignore` exist (fetch `.dockerignore` from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore`); - Dockerfile runs `make check` as a build step, and every stage containing a - check-running `RUN` declares `ARG CHECK_EPOCH` with the - `RUN [ -n "$CHECK_EPOCH" ] || exit 1` guard immediately below it — see the - `CHECK_EPOCH` rule in `REPO_POLICIES.md`. Without them the check layer is - served from cache on an unchanged tree and the build reports a green it - never ran. + Dockerfile runs the **non-lint** checks as build steps (`script/test`, + `script/fmt-check`), and every stage containing a check-running `RUN` + declares `ARG CHECK_EPOCH` with the `RUN [ -n "$CHECK_EPOCH" ] || exit 1` + guard immediately below it — see the `CHECK_EPOCH` rule in + `REPO_POLICIES.md`. Without them the check layer is served from cache on + an unchanged tree and the build reports a green it never ran. +- [ ] The `Dockerfile` no longer runs `make check`, and no longer has a `lint` + stage or a `COPY --from=lint ... /dev/null` ordering line. This is the + item an existing repo most often fails: `script/lint` is now a + `docker build`, so both of those nest a docker build inside a build step. + Delete the stage; `script/cibuild` running `script/lint` first is what + replaces its fail-fast purpose. +- [ ] `Dockerfile.lint` exists and `script/lint` builds it — see the + containerised-lint rule in `REPO_POLICIES.md` for the canonical file. Its + base image is pinned by sha256 with a version/date comment, it carries + `ARG CHECK_EPOCH` **after** the dependency layer with the guard below it, + and it invokes the linter directly rather than through `make lint`. - [ ] `.dockerignore` excludes the repo's own host-built artifacts (compiled binaries, test binaries, coverage output), written root-anchored — `/myapp`, never `**/myapp`, which would also match `cmd/myapp/`. An @@ -100,13 +111,33 @@ with your task. `script/install-precommit`, shimmed by `make hooks`) runs it - [ ] README has an **Entrypoints** section documenting the `script/` entrypoints and linking the standard -- [ ] Go: `script/lint` isolates golangci-lint per checkout — - `GOLANGCI_LINT_CACHE` and `TMPDIR` both exported into `.lint-cache/` - (which is in `.gitignore` and `.dockerignore`), `--allow-serial-runners` - passed, and the lock error retried rather than reported as findings. Copy - the canonical block from `REPO_POLICIES.md`. Setting only the cache is the - common half-fix and leaves `parallel golangci-lint is running` failing - runs red. +- [ ] `script/lint` is the canonical container build and nothing else, and no + host invocation anywhere in the repo can produce a lint **verdict** — grep + for the linter's own name across `script/`, the `Makefile` and CI config, + not just in `script/lint`. An existing repo is where a second path to the + linter is likeliest to exist: a `make lint-fast`, a container-versus-host + branch, or a CI step that calls the binary directly. **Two hits are + expected and are not the defect**, so triage rather than delete: any + `script/fmt` (a formatter must run on the host — that is its job), and, in + a repo whose formatter is also its linter, `script/fmt-check`, whose + command will be the same string as the one in `Dockerfile.lint`. See the + scope note in the containerised-lint rule. Everything else the grep finds + is a real second path and goes. +- [ ] `script/bootstrap` installs no linter **for the purpose of linting**. + Delete a dedicated install — the golangci-lint block, its version and ref + variables, and its call site: nothing invokes a host linter any more, so + all it can still do is put a differently versioned binary where somebody + runs it by hand and believes the result. A JS repo's `yarn install` stays; + it brings a linter along with every other dependency, which is unavoidable + and harmless as long as no verdict is taken from it. +- [ ] The per-checkout lint state is gone: no `GOLANGCI_LINT_CACHE` or `TMPDIR` + exports, no `--allow-serial-runners`, and `.lint-cache/` removed from + `.gitignore` and `.dockerignore`. A container has its own cache and its + own lock, so keeping the wrapper leaves two contradictory `script/lint` + forms in the fleet. +- [ ] `script/cibuild` runs `script/lint` before the main `docker build`. + Without that line CI never lints at all, because the main image + deliberately does not. - [ ] `make check` does not modify any files in the repo - [ ] `make test` has a 30-second timeout - [ ] `make test` runs real tests, not a no-op (at minimum, import/compile @@ -153,6 +184,9 @@ with your task. # Final - [ ] `make check` passes -- [ ] `script/cibuild` succeeds (a bare `docker build .` fails closed by design, - on the `CHECK_EPOCH` guard) +- [ ] `make lint` runs twice on an unchanged tree with the lint layer `DONE` + both times, never `CACHED` and never sub-second +- [ ] `script/cibuild` succeeds and runs both container builds (a bare + `docker build .` or `docker build -f Dockerfile.lint .` fails closed by + design, on the `CHECK_EPOCH` guard) - [ ] Commit and merge fixes before starting your actual task diff --git a/prompts/NEW_REPO_CHECKLIST.md b/prompts/NEW_REPO_CHECKLIST.md index dcf8d45..58df5e9 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -1,6 +1,6 @@ --- title: New Repo Checklist -last_modified: 2026-08-09 +last_modified: 2026-08-10 --- Use this checklist when creating a new repository from scratch. Follow the steps @@ -73,15 +73,27 @@ Template files can be fetched from: declared in the stage that compiles, and **no stage calls `git describe`** — `.dockerignore` excludes `.git`, so it yields an empty version without failing the build. - - All Dockerfiles must run `make check` as a build step, and every stage - containing a check-running `RUN` must declare `ARG CHECK_EPOCH` with the - `RUN [ -n "$CHECK_EPOCH" ] || exit 1` guard immediately below it — see the - `CHECK_EPOCH` rule in `REPO_POLICIES.md`. Without them the check layer is - served from cache on an unchanged tree and the build reports a green it - never ran. + - The `Dockerfile` runs the **non-lint** checks as build steps — + `script/test` and `script/fmt-check`, never `make check`. `script/lint` is + a `docker build` of `Dockerfile.lint`, so `make check` here nests a build + inside a build step, where there is no daemon. Put a comment above those + `RUN` lines saying so. Every stage containing a check-running `RUN` must + declare `ARG CHECK_EPOCH` with the `RUN [ -n "$CHECK_EPOCH" ] || exit 1` + guard immediately below it — see the `CHECK_EPOCH` rule in + `REPO_POLICIES.md`. Without them the check layer is served from cache on + an unchanged tree and the build reports a green it never ran. - Server: also builds and runs the application - - Non-server: brings up dev environment and runs `make check` + - Non-server: brings up dev environment and runs those checks - Image pinned by sha256 hash with version/date comment +- [ ] `Dockerfile.lint` — the lint-only image that `script/lint` builds. Same + `ARG CHECK_EPOCH` + guard + expanded-value discipline as above, with the + `ARG` placed **after** the dependency layer so only the lint steps re-run. + Base image pinned by sha256 with a version/date comment. Go repos use + `golangci/golangci-lint` and run both `golangci-lint config verify` and + `golangci-lint run`; other repos use the same pattern around their own + linter (eslint, ruff, prettier). Copy the canonical file from + `REPO_POLICIES.md`. The linter is invoked directly there, never via + `make lint`, which would recurse. - [ ] Gitea Actions workflow at `.gitea/workflows/check.yml` that runs `script/cibuild` on push — reference `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitea/workflows/check.yml` @@ -108,29 +120,27 @@ are thin shims calling them. Model scripts: then `install-precommit`, plus repo-specific init - [ ] `script/test` / `make test` — runs real tests, not a no-op (30-second timeout) -- [ ] `script/lint` / `make lint` — runs linter - - [ ] Go: exports `GOLANGCI_LINT_CACHE` **and** `TMPDIR` into a - `.lint-cache/` directory inside the checkout, above any - container-versus-host branch so every path that reaches the linter - gets them; passes `--allow-serial-runners` (never - `--allow-parallel-runners`); retries on - `parallel golangci-lint is running` detected on **stderr** and exits - 75 with a VOID message on exhaustion. Copy the canonical block from - `REPO_POLICIES.md` rather than writing your own: a version that sets - only the cache leaves the false-red half live, and one that detects - the collision by exit status can retry a real finding away. - - [ ] Go: `.lint-cache/` is in both `.gitignore` and `.dockerignore` +- [ ] `script/lint` / `make lint` — builds `Dockerfile.lint` and nothing else: + `epoch="$(date +%s%N)$$"` on its own line, then + `docker build --build-arg CHECK_EPOCH="$epoch" -f Dockerfile.lint .`. No + lint verdict may come from a host invocation. Copy the canonical script + from `REPO_POLICIES.md`; its executable lines are identical across repos. + Without the nonce this script exits 0 on an unchanged tree having linted + nothing, and without `-f Dockerfile.lint` it builds the main image and + lints nothing at all. - [ ] `script/fmt` / `make fmt` — formats code (writes) - [ ] `script/fmt-check` / `make fmt-check` — checks formatting (read-only) - [ ] `script/check` / `make check` — runs `test`, `lint`, `fmt-check`; must not - modify files + modify files. It needs a docker daemon, because `script/lint` is a + container build, and it must never be called from inside a build stage - [ ] `script/projectname` — outputs the project name (used by `script/docker` for the image tag) - [ ] `script/docker` / `make docker` — builds Docker image, tagged via `script/projectname` (byte-identical across repos); carries the same three version lines as `script/cibuild` below, and passes `--build-arg CHECK_EPOCH="$epoch"` and `--build-arg VERSION="$version"` -- [ ] `script/cibuild` — cd to repo root, then, each on its own line: +- [ ] `script/cibuild` — cd to repo root, run `script/lint` **first** for + fail-fast feedback, then, each on its own line: ```sh epoch="$(date +%s%N)$$" @@ -142,14 +152,17 @@ are thin shims calling them. Model scripts: . ``` - (what CI runs). Both build args are mandatory, and both assignments must be - on their own line: a failing command substitution inside an argument does - not trip `set -e`, so the inline form degrades silently to an empty - constant. The `[ -n "$version" ]` line is a live check that fires on an - export with no `.git` and on a repo with no commits — keep it, and do not - collapse it into `|| echo unknown`, which makes it unreachable. See the - `CHECK_EPOCH` and git-describe rules in `REPO_POLICIES.md` for why each - element is load-bearing. A bare `docker build .` fails closed by design. + (what CI runs). The `script/lint` call is not optional: the main image does + not lint, so without it CI never lints. Both build args are mandatory, and + both assignments must be on their own line: a failing command substitution + inside an argument does not trip `set -e`, so the inline form degrades + silently to an empty constant. The `[ -n "$version" ]` line is a live check + that fires on an export with no `.git` and on a repo with no commits — keep + it, and do not collapse it into `|| echo unknown`, which makes it + unreachable. See the `CHECK_EPOCH` and git-describe rules in + `REPO_POLICIES.md` for why each element is load-bearing. A bare + `docker build .` fails closed by design, and so does a bare + `docker build -f Dockerfile.lint .`. - [ ] `script/precommit` — called by the pre-commit hook; runs `script/check` - [ ] `script/install-precommit` — installs the pre-commit hook that runs @@ -161,7 +174,11 @@ are thin shims calling them. Model scripts: # 4. Verify - [ ] `make check` passes +- [ ] `make lint` demonstrably runs the linter rather than returning a cached + build: run it twice on an unchanged tree and confirm the lint layer says + `DONE`, never `CACHED`, both times - [ ] `make docker` succeeds +- [ ] `script/cibuild` succeeds and runs both container builds - [ ] No secrets in repo - [ ] No mutable image/package references - [ ] No unnecessary files in repo root diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index dd870cc..db6e30e 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -1,6 +1,6 @@ --- title: Repository Policies -last_modified: 2026-08-09 +last_modified: 2026-08-10 --- This document covers repository structure, tooling, and workflow standards. Code @@ -60,7 +60,8 @@ style conventions are in separate documents: prerequisite since nvm requires bash. yarn is then pinned via `corepack prepare yarn@ --activate`. Never install "latest" or "lts"; always exact versions. `script/cibuild` runs the CI build: it changes to the - repo root and runs + repo root, runs `script/lint` first for fail-fast feedback (that is itself a + container build — see the containerised-lint rule below), and then runs `docker build --build-arg CHECK_EPOCH="$epoch" --build-arg VERSION="$version" .`, where `epoch` is a per-invocation nonce (see the `CHECK_EPOCH` rule below) and `version` is computed on the host because `.git` is not in the build context @@ -93,31 +94,50 @@ style conventions are in separate documents: contributor should be able to understand the entire development workflow by reading the Makefile. -- Every repo should have a `Dockerfile`. All Dockerfiles must run `make check` - as a build step so the build fails if the branch is not green — which requires - `ARG CHECK_EPOCH` and its guard in every stage containing a check-running - `RUN`, per the `CHECK_EPOCH` rule below. Without them a Dockerfile satisfies - this criterion while its check layers are served from cache, so the build - cannot fail on a branch that is not green. For non-server repos, the - Dockerfile should bring up a development environment and run `make check`. For - server repos, `make check` should run as an early build stage before the final - image is assembled. Dockerfiles install development prerequisites by running - `script/bootstrap` rather than duplicating installs inline; COPY `script/` and - the dependency manifests (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, - etc.) before running it so the bootstrap layer stays cached until dependencies - change. +- Every repo should have a `Dockerfile`. It must run the repo's **non-lint** + checks as build steps, so the build fails on a branch those checks reject — + which requires `ARG CHECK_EPOCH` and its guard in every stage containing a + check-running `RUN`, per the `CHECK_EPOCH` rule below. Without them a + Dockerfile satisfies this criterion while its check layers are served from + cache, so it cannot fail on a branch those checks would have rejected. + + **A green `docker build .` does not mean the branch is green**, because this + file does not lint. Only `script/cibuild` carries that meaning: it runs + `script/lint` and then this build. Do not restate the older, stronger claim + anywhere — it was true only while lint ran inside this image. + + **It runs the individual non-lint checks — `script/test` and + `script/fmt-check` — and never `make check`.** `script/lint` is itself a + `docker build` (of `Dockerfile.lint`, per the containerised-lint rule + below), so a `RUN make check` in this file attempts a docker build inside a + build step, where there is no daemon. Lint is not skipped by this: it runs + in its own container, and `script/cibuild` runs it first. Put a comment to + that effect directly above those `RUN` lines, because `make check` is what + the next person will reach for. + + For non-server repos, the Dockerfile should bring up a development + environment and run those checks. For server repos, they should run as an + early build stage before the final image is assembled. Dockerfiles install + development prerequisites by running `script/bootstrap` rather than + duplicating installs inline; COPY `script/` and the dependency manifests + (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, etc.) before running it + so the bootstrap layer stays cached until dependencies change. - **Every check-running `RUN` must be cache-busted with `CHECK_EPOCH`.** Docker invalidates a `COPY` layer only when the copied content changes, so on an - unchanged tree the `RUN make check` layer is served from cache, the suite - never runs, and the build still exits 0. A sub-second `docker build` reporting - success is a cache hit, not a result. The canonical form, in **every** stage - containing a check-running `RUN`: + unchanged tree the check layer is served from cache, the suite never runs, and + the build still exits 0. A sub-second `docker build` reporting success is a + cache hit, not a result. This applies to **every** file that runs checks in a + build step, which since linting moved into its own container means + `Dockerfile` and `Dockerfile.lint` both — a `Dockerfile.lint` without the + cache-bust is a lint that never ran, reported as a pass. The canonical form, + in **every** stage containing a check-running `RUN`, placed **after** the + dependency-install layer so that layer stays cached: ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 - RUN echo "check epoch: ${CHECK_EPOCH}" && make check + RUN echo "check epoch: ${CHECK_EPOCH}" && ``` and in both `script/cibuild` and `script/docker`: @@ -132,6 +152,12 @@ style conventions are in separate documents: . ``` + `script/lint` needs the same nonce but is **not** this command: it builds a + different file and passes no version. Do not copy the block above into it — + a `docker build` with no `-f Dockerfile.lint` builds the main image instead, + which is a lint that silently lints nothing. Its canonical form is in the + containerised-lint rule below; copy that one. + The `VERSION` lines are there for a different reason, covered by the git-describe rule below; they are shown here so the two rules do not each document half a command. All four `CHECK_EPOCH` elements are load-bearing; @@ -164,47 +190,279 @@ style conventions are in separate documents: This invalidates the check layers and everything after them while leaving `go mod download`, `script/bootstrap`, and the pinned toolchain install cached, so it does not push against the five-minute Docker build ceiling. - Blanket `--no-cache` also works but is wasteful and can blow that ceiling. + Blanket `--no-cache` is **not** an acceptable substitute, on `Dockerfile` or + on `Dockerfile.lint`: it re-runs `go mod download` / `yarn install` on every + invocation, so a lint that should take seconds pays a dependency install + each time and reaches the network on **every** run rather than only on the + first and after a manifest change. It is a difference of degree, not an + absolute — a container build is never fully offline-independent — but it is + the difference between a lint that usually needs nothing and one that always + does. Never reach for `docker builder prune` to achieve the same end: the + build cache is shared with every other build on the host, including other + people's. -- **Dockerfiles must use a separate lint stage for fail-fast feedback.** Go - repos use a multistage build where linting runs in an independent stage based - on the `golangci/golangci-lint` image (pinned by hash). This stage runs - `make fmt-check` and `make lint` before the full build begins. The build stage - then declares an explicit dependency on the lint stage via - `COPY --from=lint /src/go.sum /dev/null`, which forces BuildKit to complete - linting before proceeding to compilation and tests. This ensures lint failures - surface in seconds rather than minutes, without blocking on dependency - download or compilation in the build stage. +- **Every lint run happens in a container, and `script/lint` is that container + build.** The precise claim, because it is the one that has to survive contact + with a repo whose formatter and linter are the same binary: **no lint verdict + may come from a host invocation.** No `script/`, no `Makefile` target and no + CI step may produce a lint result by running a linter on the host. That is + stronger than it sounds and weaker than "the binary is never on the host" — + see the scope note at the end of this rule, which says exactly which host + invocations remain legitimate and why. Every repo carries a `Dockerfile.lint` + next to its `Dockerfile`; the linter runs as a **build step**, so a successful + build _is_ a clean lint. Building rather than bind-mounting is deliberate: it + is what makes the pattern work unchanged where the docker daemon is remote and + bind mounts are impossible. Docker is assumed available in every environment. + Discarding the linter's cache on every run is the point of this rule, not a + cost it pays. - The standard pattern for a Go repo Dockerfile is: + This closes a family of defects, every one of them an artefact of running + the linter on a shared host, and every one of them observed rather than + hypothesised: + - **A confirmed false green.** An implementer reported `0 issues` on a + branch that was genuinely red with a `goconst` finding. golangci-lint keys + cached results on file **content, not location**, so a second checkout of + the same commit holds byte-identical files and serves its result. Note + what content-keying implies: moving agents from worktrees into their own + clones does **not** help, because two clones are byte-identical exactly as + two worktrees were. It removes the foreign-path symptom and leaves the + mechanism live, which makes the defect quieter rather than rarer. + - **False reds**, repeatedly: findings reported against `../wt82-lint/...`, + against another agent's checkout, and against a worktree that had already + been deleted; in one case 399 issues returned to a clean clone that + genuinely lints 0. + - **Lock contention that cannot be distinguished from findings.** + golangci-lint flocks `$TMPDIR/golangci-lint.lock` (`pkg/commands/run.go`, + `acquireFileLock()`) — host-global, keyed on the temp directory, entirely + independent of `GOLANGCI_LINT_CACHE`, with a 5-second acquire timeout, so + it fails precisely when the host is busiest. On failure it prints + `parallel golangci-lint is running`, analyzes nothing, and exits non-zero. + **Proven not fixed by per-cache isolation**: two concurrent runs with + entirely separate cache directories still collided. + - **Version skew.** A host linter differing from the pinned one, with the + container surfacing thirteen findings the host missed on one repo, and a + local `make check` green against a `make docker` that rejected the same + commit with six `goconst` findings. + + A container per run has its own cache, its own `TMPDIR` and therefore its + own lock, and a binary pinned by digest, so none of the above is reachable. + That is also why the per-checkout `GOLANGCI_LINT_CACHE`/`TMPDIR` wrapper + that used to be canonical here is **gone rather than kept alongside this**: + its entire subject was making a host run trustworthy, and there are no host + runs. Consuming repos delete it when they adopt this; see the adoption list + at the end of this rule. + + The canonical `Dockerfile.lint` for a Go repo: ```dockerfile - # Lint stage — fast feedback on formatting and lint issues - # golangci/golangci-lint:v2.x.x, YYYY-MM-DD - FROM golangci/golangci-lint@sha256:... AS lint + # Lint-only image. `script/lint` builds this file and nothing else: the + # linter runs as a build step, so a successful build IS a clean lint. + # + # The linter is invoked directly below rather than through `make lint`. + # That is not a style choice: `script/lint` IS this build, so calling it + # from inside would recurse into a docker build with no daemon. + # + # golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07 + FROM golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 + WORKDIR /src + + # Dependency layer first, and deliberately above the ARG below, so it + # stays cached and only the lint steps re-run on every invocation. COPY go.mod go.sum ./ RUN go mod download + COPY . . + ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 - RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check - RUN make lint + RUN echo "lint epoch: ${CHECK_EPOCH}" && \ + golangci-lint config verify --config .golangci.yml + RUN golangci-lint run --config .golangci.yml ./... + ``` + and the canonical `script/lint`, whose executable lines are identical in + every repo (only the comment wording is a repo's own): + + ```sh + #!/bin/sh + # script/lint: run the linter. This is the ONLY source of a lint verdict + # in this repo — the linter runs in a container, one way, everywhere, so + # a run cannot inherit another checkout's cache, another process's lock, + # or a host toolchain that differs from the pinned one. + # + # A failure here that names no finding is NOT a lint result: docker + # build exits 1 both for findings and for a build that never reached + # the lint step. BuildKit names the failing step; read it, fix the + # environment, and re-run. + set -eu + + ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" + + main() { + cd "$ROOT" + # Own line: a failing command substitution inside an argument does + # not trip `set -e`, and `$$` is required because busybox `date` + # drops %N without erroring. Without a fresh nonce the lint layer is + # served from cache and this script exits 0 having linted nothing. + epoch="$(date +%s%N)$$" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + -f Dockerfile.lint \ + . + } + + main "$@" + ``` + + Load-bearing properties: + - **`CHECK_EPOCH`, not `--no-cache`.** `docker build -f Dockerfile.lint .` + on an unchanged tree returns a sub-second cached success having linted + nothing — the same false green the `CHECK_EPOCH` rule above exists to + close, arriving through a new file. The `ARG` goes **after** the + dependency layer so `go mod download` / `yarn install` stay cached and + only the lint steps re-run. Blanket `--no-cache` also busts the dependency + layer, so every lint reaches the network instead of only the first one. + - **Non-Go repos get the same pattern around their own linter** — `eslint`, + `ruff`, `prettier`, `shellcheck` — because the ruling is every lint run, + not every Go lint run. Only the base image and the lint commands change; + the `WORKDIR`, dependency layer, `ARG CHECK_EPOCH`, guard and + expanded-value `RUN` are identical. A JS or docs repo bases on its pinned + node image, runs `script/bootstrap` as the dependency layer **inside the + image**, and lints with the linter from that image's `node_modules`. Note + what this does not claim: a developer also runs `script/bootstrap` on the + host, so a copy of that linter exists there too. What the rule forbids is + taking a **verdict** from it. + - **Keep `golangci-lint config verify`, and it costs no network.** The two + commands catch **disjoint** classes of defect, measured under the pinned + v2.12.2 against a config carrying one planted defect at a time: a bogus + top-level key and a bogus key nested under `linters.settings.lll` both + pass `golangci-lint run` with **exit 0 and `0 issues`** while + `config verify` exits 3 and names the key; an invalid value type fails + both; an unknown linter name fails `run` and passes `config verify`. So + `run` alone silently ignores an unknown key, which is exactly the mode + where a threshold reads as configured and is not applied. The earlier + caution that `config verify` resolves its JSON schema over a live HTTPS + fetch does **not** hold for this pinned version: every case above was + re-run under `docker run --network none` and produced byte-identical + diagnostics and exit statuses, in a container where + `getent hosts golangci-lint.run` exits 2. The schema is embedded in the + pinned binary. Re-run that control when bumping the pin rather than + treating the result as permanent. + - **No repo installs a linter on the host _in order to lint_ — remove any + such install from `script/bootstrap`.** A dedicated linter install (the + `go install` of golangci-lint is the case that existed) is now dead weight + whose only remaining effect is to reintroduce the version skew above. This + does not forbid a host dependency install that happens to bring a linter + along with everything else, which is unavoidable in a JS repo and is fine + as long as no verdict is taken from it. + - **A failed `script/lint` that names no finding is not a lint result.** The + exit status alone cannot tell you which happened: `docker build` returns 1 + both when the lint step fails on findings and when the build never got + that far — daemon unreachable, base image unpullable, disk full, + dependency layer failing. The per-checkout wrapper this rule replaced drew + that line explicitly, exiting 75 for a run that analysed nothing, and that + machinery is deliberately **not** restored. Three reasons, and they are a + position rather than an omission: + - **The dangerous direction is already closed.** The wrapper's exit 75 + existed because a lock collision could be read as a _result_ on a run + that analysed nothing. A build that cannot run fails **closed**: it + can never read as clean. What is lost is diagnosability, not safety. + - **The failure is self-describing, where the lock collision was not.** + BuildKit prints the failing step verbatim — + `ERROR: failed to solve: process "/bin/sh -c "` for + a genuine finding, against a named earlier step or a daemon error for + anything else. The discriminator is already in the output and needs no + code. The lock message, by contrast, went to stderr while findings + went to stdout and was easy to lose. + - **It is not transient, so retrying is wrong.** The lock collision + cleared on a retry, which is what made an automatic retry worth + building. A dead daemon or a full disk does not, and a retry loop over + a failing dependency fetch hides a real reproducibility problem. + Rebuilding the distinction in the script would also mean reintroducing + per-invocation capture files and traps to get the exit status out from + under a pipe, plus matching on BuildKit's message format, which is not + a stable interface — and a mis-match in the "treat it as + infrastructure" direction would be the false green this whole rule + exists to prevent. + + The substance of the VOID rule survives as a reading rule, and it is + binding: **do not record a lint verdict from a run that did not reach + the lint step, and do not "fix" anything on the strength of one.** Read + which step failed, fix the environment, and re-run. + + - **`script/check` still runs `test`, `lint` and `fmt-check`**, so a + developer and the pre-commit hook get all three. It therefore requires a + docker daemon, and it must never be invoked from inside a build stage — + see the `Dockerfile` rule above. + - If the project uses `//go:embed` directives referencing build artifacts + (e.g. a web frontend compiled elsewhere), `Dockerfile.lint` must create + placeholder files so the directives resolve: + `RUN mkdir -p web/dist && touch web/dist/index.html`. It must not depend + on the real build output; it exists to fail fast. + - If linting requires CGO or system libraries (e.g. `vips-dev`), install + them in `Dockerfile.lint`. + + **Scope: this rule is about linters, and a formatter is not one.** + `script/fmt` writes to your working tree, so it can only run on the host; + `script/fmt-check` is its read-only twin and runs on the host too, as well + as inside the main image. Neither is a lint run and neither is in scope. + + **The awkward case, stated rather than left for an adopter to trip over: in + a repo whose formatter _is_ its linter** — prettier over a markdown or docs + repo is the standard shape, and this repo is one — the command in + `Dockerfile.lint` and the command in `script/fmt-check` are the same string, + so that exact command does still run on the host. That is a real gap in the + absolute reading and it is accepted for two reasons: the mechanisms this + rule exists to close do not reach it (prettier's version is pinned in + `package.json` and installed into the repo's own `node_modules`, so there is + no shared content-keyed cache, no host-global lock, and no version to skew + against), and CI's verdict is containerised regardless, because + `script/fmt-check` also runs inside the main image build. What is **not** + acceptable is taking a lint verdict from the host copy: `script/lint` stays + the only source of one. If a repo's linter and formatter ever diverge in + version or configuration, this gap becomes a real defect and the check-mode + invocation has to move into the container too. + + **What a consuming repo does to adopt this**, in order: add + `Dockerfile.lint`; replace `script/lint` with the build above; delete the + `lint` stage from its `Dockerfile` along with the + `COPY --from=lint ... /dev/null` ordering line; change that `Dockerfile`'s + `RUN make check` to `script/test` and `script/fmt-check` with the comment + explaining why; add `script/lint` as the first step of `script/cibuild`; + delete any golangci-lint install from `script/bootstrap`; and delete the + `.lint-cache/` entries from `.gitignore` and `.dockerignore` together with + the per-checkout cache/lock wrapper they served. + + **The separate lint _stage_ is superseded by this and must not survive + alongside it.** It ran `make lint`, which is now a docker build, so keeping + it is not a stylistic preference but a recursion. Its purpose — fail-fast + feedback before the slow build — is served by `script/cibuild` running + `script/lint` first, and its `COPY --from=lint /src/go.sum /dev/null` + ordering trick, along with the warm-cache re-proof that trick required, is + no longer needed because the ordering is now sequential in the shell. + +- **The canonical Go repo `Dockerfile`**, which builds and tests but does not + lint: + + ```dockerfile # Build stage # golang:1.x-alpine, YYYY-MM-DD FROM golang@sha256:... AS builder WORKDIR /src - - # Force BuildKit to run the lint stage before proceeding - COPY --from=lint /src/go.sum /dev/null - COPY go.mod go.sum ./ RUN go mod download COPY . . + ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 - RUN echo "check epoch: ${CHECK_EPOCH}" && make test + + # The individual non-lint checks, NOT `make check`: script/lint is a + # docker build (Dockerfile.lint), so `make check` here would nest a + # build inside a build step, where there is no daemon. Lint is not + # skipped — script/cibuild runs it first, in its own container. + RUN echo "check epoch: ${CHECK_EPOCH}" && script/fmt-check + RUN script/test # VERSION comes from the host via --build-arg; see the git-describe rule # below. Never run `git describe` here: .dockerignore excludes .git, so @@ -221,44 +479,17 @@ style conventions are in separate documents: ``` Key points: - - The lint stage uses the `golangci/golangci-lint` image directly (it - includes both Go and the linter), so there is no need to install the - linter separately. - - `COPY --from=lint /src/go.sum /dev/null` is a no-op file copy that creates - a stage dependency. BuildKit runs stages in parallel by default; without - this line, the build stage would not wait for lint to finish and a lint - failure might not fail the overall build. - - **Re-prove that ordering on a warm cache after adopting `CHECK_EPOCH`.** - The cache-bust turns this no-op `COPY` into a content-cache hit, so an - ordering guarantee established on a cold cache does not automatically - carry over; it has to be re-checked warm. This was re-proved in another - repo in the org that uses the same file-dependency trick (there with a - marker file in place of `go.sum`), and the ordering held. It has **not** - been verified in this repo, which is single-stage and has no lint stage to - order against. Any repo relying on a file-dependency trick for stage - ordering should re-check it warm after adopting the bust rather than - assuming this result transfers. - - If the project uses `//go:embed` directives that reference build artifacts - (e.g. a web frontend compiled in a separate stage), the lint stage must - create placeholder files so the embed directives resolve. Example: - `RUN mkdir -p web/dist && touch web/dist/index.html web/dist/style.css`. - The lint stage should not depend on the actual build output — it exists to - fail fast. - - If the project requires CGO or system libraries for linting (e.g. - `vips-dev`), install them in the lint stage with `apk add`. - - The build stage runs `make test` after compilation setup. Tests run in the - build stage, not the lint stage, because they may require compiled - artifacts or heavier dependencies. - - `ARG CHECK_EPOCH` appears in **both** stages, because `ARG` is - stage-scoped: declaring it only in the lint stage leaves `make test` - frozen at the last cached result. In each stage the guard sits immediately - below the `ARG` so a bare `docker build .` fails instead of reusing the - empty cache key, and the value is expanded into the first check `RUN` so - the cache miss does not rely on BuildKit's unreferenced-`ARG` handling. - Both of those lines reference `$CHECK_EPOCH`, so both are value-keyed: - each stage is invalidated at two independent points. The later `RUN`s in - the same stage need no expansion of their own: they are already - invalidated by their busted parent layer. + - Tests run in the build stage because they may require compiled artifacts + or heavier dependencies. + - `ARG CHECK_EPOCH` must be declared in **every** stage containing a + check-running `RUN`, because `ARG` is stage-scoped: declaring it in one + stage leaves the others frozen at their last cached result while the fix + reviews as complete. In each such stage the guard sits immediately below + the `ARG`, and the value is expanded into the first check `RUN` so the + cache miss does not rely on BuildKit's unreferenced-`ARG` handling. Both + lines reference `$CHECK_EPOCH`, so each stage has two independent + invalidation points. Later `RUN`s in the same stage need no expansion of + their own: their parent layer is already busted. - `ARG VERSION=dev` is declared in the build stage, and its value is supplied on the host by `script/docker` and `script/cibuild` via `--build-arg VERSION=...`. The `dev` default is a placeholder for a local @@ -267,16 +498,21 @@ style conventions are in separate documents: failing. See the git-describe rule further down. - Every repo should have a Gitea Actions workflow (`.gitea/workflows/`) that - runs `script/cibuild` (which runs - `docker build --build-arg CHECK_EPOCH="$epoch" --build-arg VERSION="$version" .`) - on push. The Dockerfile runs `make check`, so a successful build implies all - checks pass — but that implication holds **only** because of the `CHECK_EPOCH` - cache-bust described above. Without it, an unchanged tree serves the check - layer from cache and the build reports a green it never earned. A bare - `docker build .` fails closed by design, on the `[ -n "$CHECK_EPOCH" ]` guard; - always go through `script/cibuild` or `script/docker`. Never accept a - `script/cibuild` pass as evidence without confirming it ran: a sub-second wall - time, or `CACHED` on the check layer, means nothing was executed. + runs `script/cibuild` on push. `script/cibuild` runs **two** container builds: + `script/lint` (`Dockerfile.lint`) first, then + `docker build --build-arg CHECK_EPOCH="$epoch" --build-arg VERSION="$version" .` + for the main image, which runs the non-lint checks. A successful + `script/cibuild` therefore implies all checks pass; **a successful + `docker build .` on its own does not, because it never lints.** That is the + one claim to be careful with when reading these files: the guarantee belongs + to `script/cibuild`, not to any single Dockerfile. Both halves of it hold only + because each build passes its own `CHECK_EPOCH` nonce — without it an + unchanged tree serves the layers from cache and the build reports a green it + never earned. A bare `docker build .` or `docker build -f Dockerfile.lint .` + fails closed by design, on the `[ -n "$CHECK_EPOCH" ]` guard; always go + through `script/cibuild`, `script/docker` or `script/lint`. Never accept a + pass as evidence without confirming it ran: a sub-second wall time, or + `CACHED` on a check or lint layer, means nothing was executed. - Use platform-standard formatters: `black` for Python, `prettier` for JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with @@ -498,516 +734,98 @@ style conventions are in separate documents: - `.golangci.yml` is standardized and must _NEVER_ be modified by an agent, only manually by the user. Fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`. The - canonical golangci-lint version is v2.12.2 (released 2026-05-06), installed - commit-pinned via - `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5`. + canonical golangci-lint version is v2.12.2 (released 2026-05-06), pinned as + the image digest in `Dockerfile.lint` + (`golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240`, + which reports + `golangci-lint has version 2.12.2 built with go1.26.2 from c0d3ddc9`). That + digest is the only pin there is: the linter is not installed on the host, in + `script/bootstrap` or anywhere else. Bumping the version means changing that + one digest, and it propagates to every consumer of the image with no host + state able to disagree with it. -- **`script/bootstrap` in Go repos must install the pinned golangci-lint - whenever the installed version does not match the pin — not merely when the - binary is absent — and must then verify the install took effect by - re-resolving the binary through `PATH`.** The presence test - `if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi` is wrong: - it tests `PATH` presence and never version, so on any already-provisioned - machine the pin is inert and a version bump is a no-op. Meanwhile the - Dockerfile installs unconditionally into a clean image, so CI and local - silently disagree about what the linter even is. Observed consequences: a - local `make check` green while `make docker` rejected the same commit with six - `goconst` findings, and a container linter surfacing thirteen findings the - host run missed. A stale host linter does not merely fail to prove the tree is - clean — it hides findings only the container can see. This is a deliberate - departure from the node handling described above, which uses whatever node is - installed: the linter version is the specific thing being held equal between - host and container, so for it, presence is not enough. +- **`script/bootstrap` must not install a linter at all.** This supersedes the + pinned-golangci-lint install that used to be canonical here. Nothing runs a + linter on the host any more — `script/lint` is a container build — so a host + install has no caller left, and its only remaining effect is to put a second, + independently-versioned linter on the machine where somebody will eventually + run it by hand and believe the result. The version-skew failures that install + was written to close (a local `make check` green while `make docker` rejected + the same commit with six `goconst` findings; a container linter surfacing + thirteen findings the host run missed) are closed more completely by having + exactly one linter, pinned by image digest, that no host state can shadow. + Repos adopting the containerised lint delete the install block, its version + and ref variables, and its call site from `script/bootstrap`. - Comparing versions is necessary but **not sufficient**, because the obvious - fix also fails green. `go install` writes to `GOBIN` (or `GOPATH/bin`) while - callers resolve `golangci-lint` through `PATH`. If a different binary - shadows it earlier in `PATH`, the install genuinely succeeds and changes - nothing any caller will ever see: bootstrap prints success and the next - `make lint` still runs the stale linter. That is worse than no fix, because - it converts a known-stale toolchain into one everyone believes is pinned. - The canonical form, placed in `script/bootstrap` after Go itself is present: - - ```sh - # golangci-lint v2.12.2, 2026-05-06. GOLANGCI_LINT_VERSION must be exactly - # what `golangci-lint --version` prints for this ref; update both together. - GOLANGCI_LINT_VERSION="2.12.2" - GOLANGCI_LINT_REF="github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5" - - # The version golangci-lint reports, resolved the way callers resolve it. - # Prints nothing when the binary is absent, exits non-zero, or prints - # something unparseable: all of those must read as "does not match". - # The capture is the whole version token, not just its numeric prefix. - # Stopping at the first `-` would make 2.12.2-rc1 compare equal to 2.12.2 - # and skip the install, which is the defect this whole rule exists to close. - # The trailing `|| true` is required, not tidiness. Under `set -o pipefail` - # a non-zero --version would otherwise propagate out of the pipeline and - # kill the script through `set -e` before the diagnostic below is printed. - golangci_lint_version() { - command -v golangci-lint >/dev/null 2>&1 || return 0 - golangci-lint --version 2>/dev/null | head -n 1 | - sed -n 's/.*has version v\{0,1\}\([0-9][^ ]*\).*/\1/p' || true - } - - ensure_golangci_lint() { - if [ "$(golangci_lint_version)" = "$GOLANGCI_LINT_VERSION" ]; then - echo "bootstrap: golangci-lint $GOLANGCI_LINT_VERSION already installed" - return 0 - fi - echo "bootstrap: installing golangci-lint $GOLANGCI_LINT_VERSION" - go install "$GOLANGCI_LINT_REF" - - # go install writes to GOBIN (or GOPATH/bin); callers resolve through - # PATH. Re-resolve through PATH and assert the install took effect. - # `hash -r` is load-bearing: without it a shell that already resolved - # a stale golangci-lint answers from its own lookup cache, and this - # check false-fails with the shadowing message below. - hash -r 2>/dev/null || true - gcl_got="$(golangci_lint_version)" - if [ "$gcl_got" = "$GOLANGCI_LINT_VERSION" ]; then - echo "bootstrap: golangci-lint $GOLANGCI_LINT_VERSION installed," \ - "and PATH resolves it" - return 0 - fi - gcl_bin="$(go env GOBIN)" - [ -n "$gcl_bin" ] || gcl_bin="$(go env GOPATH)/bin" - # Strip a trailing slash: GOBIN=/x/ would otherwise make the - # "$gcl_bin"/* test below miss and misreport shadowing. - while :; do - case "$gcl_bin" in - */) gcl_bin="${gcl_bin%/}" ;; - *) break ;; - esac - done - gcl_found="$(command -v golangci-lint 2>/dev/null || true)" - echo "bootstrap: installed golangci-lint $GOLANGCI_LINT_VERSION into" \ - "$gcl_bin, but that is not what callers will get." >&2 - case "$gcl_found" in - "") - echo "bootstrap: PATH resolves no golangci-lint at all." \ - "Add $gcl_bin to PATH, then re-run bootstrap." >&2 - ;; - "$gcl_bin"/*) - echo "bootstrap: PATH resolves $gcl_found, inside that same" \ - "directory, reporting version ${gcl_got:-unparseable}." \ - "Nothing is shadowing it, so the install itself did not" \ - "produce the pinned version: check that" \ - "GOLANGCI_LINT_VERSION matches GOLANGCI_LINT_REF." >&2 - ;; - *) - echo "bootstrap: PATH resolves $gcl_found instead, reporting" \ - "version ${gcl_got:-unparseable}. Remove that binary or" \ - "put $gcl_bin earlier in PATH, then re-run bootstrap." >&2 - ;; - esac - exit 1 - } - - # The definitions above are inert on their own; the call site is part of - # the canonical form. In a script/bootstrap that follows the "define all - # functions, then call main" convention, this line belongs inside main() - # next to the other ensure_* steps. - ensure_golangci_lint - ``` - - Four properties are load-bearing; each guards a failure mode that otherwise - fails green: - - **Compare the installed version against the pin**, never test presence. - This is what makes a version bump propagate to machines that already have - some golangci-lint. Compare the **whole** version token, exactly: a parser - that stops at the first `-` reports `2.12.2` for a host running - `2.12.2-rc1`, which compares equal to a `2.12.2` pin and skips the install - — the original defect, reintroduced through the comparison meant to fix - it. + **The version-enforcement principle it established still applies to any + other tool a repo pins and installs on the host**, and it is the part worth + keeping, because each of its four properties guards a failure that otherwise + reports success: + - **Compare the installed version against the pin, never test presence.** A + `if missing ; then install; fi` guard tests `PATH` presence and + never version, so on any already-provisioned machine the pin is inert and + a version bump is a silent no-op. Compare the **whole** version token, + exactly: a parser that stops at the first `-` reports `2.12.2` for a host + running `2.12.2-rc1` and skips the install — the original defect, + reintroduced through the comparison meant to fix it. - **After installing, re-resolve the binary the way callers resolve it** — - through `PATH`, not the path `go install` wrote to — and assert - `--version` reports the pin. When it does not, fail non-zero and name the - path `command -v` actually found, the version it reports, and the - directory the install wrote to. That is a condition a human has to fix by - hand, so bootstrap must not print success in it. Use `hash -r` first so - the shell does not answer from its own lookup cache. Diagnose the cause - from the resolved path rather than asserting one: only a path **outside** - the install directory is shadowing. When the resolved path is inside it, - nothing is shadowing and telling the operator to delete that binary or - reorder `PATH` sends them after a fault that does not exist. + through `PATH`, not the directory the installer wrote to — and assert the + reported version is the pin. An installer that writes to `GOBIN` while a + different binary shadows it earlier in `PATH` genuinely succeeds and + changes nothing any caller sees, which is worse than no fix: it converts a + known-stale tool into one everyone believes is pinned. Run `hash -r` first + so the shell does not answer from its own lookup cache, and when the + assertion fails, name the path `command -v` found, the version it reports, + and the directory the install wrote to. Diagnose from the resolved path + rather than asserting a cause: only a path **outside** the install + directory is shadowing. - **A mis-parse must fall through to reinstall, never to a false match.** - Absent binary, non-zero exit, empty output, and unrecognised output all - yield an empty string, which compares unequal to the pin. The failure + Absent binary, non-zero exit, empty output and unrecognised output should + all yield an empty string, which compares unequal to the pin. The failure direction is always a redundant install, never a skipped one. - - **Call it, and say so on success.** Two function definitions with no call - site are a silent no-op that reproduces the original defect exactly: exit - 0, nothing installed, no output, stale linter still resolved. A success - path that prints nothing is byte-identical to that no-op — same exit - status, same empty output — so both success branches must print a - confirmation naming the version. In a change about undetectable no-ops, - "it printed nothing and exited 0" must not be the healthy signal. + - **Call it, and say so on success.** A function defined and never called is + a silent no-op indistinguishable from success: exit 0, nothing installed, + no output. Both success branches must print a line naming the version. + + Verifying such logic requires a negative control in an environment where a + shadowing binary exists earlier in `PATH` than the install target — without + it the control passes against the naive compare-then-install form too and + proves nothing — plus a mis-parse control that feeds unparseable `--version` + output and confirms a reinstall. Run those controls against the block as a + consuming repo would adopt it: pasted into a `script/bootstrap`-shaped file + that is then executed, never by sourcing it and invoking the function + yourself. Driving the function directly tests something the artifact does + not do, and it is exactly how a missing call site passes every control while + the adopted snippet does nothing. Keep it POSIX sh: no bashisms, no arrays, no `[[`, no `grep -P`. - **On the hash-pinning rule.** `@c0d3ddc9cf3faa61a4e378e879ece580256d76e5` is - a commit hash, not a server-mutable version tag, and the go command verifies - the fetched module against the checksum database — the mechanism the - hash-pinning rule at the top of this document already names as acceptable - for Go modules. Note that `go install pkg@version` runs in module-aware mode - ignoring the `go.mod` in the current directory or any parent, so no repo - `go.sum` is consulted for this install; the checksum database is what - verifies it. The linter is a bootstrap prerequisite rather than part of any - repo's module graph, which is why the canonical form installs it by - commit-pinned ref instead of declaring it in `go.mod`. Whether a `go.mod` - tool dependency — which would pin the hash in a committed, reviewable file - instead — should replace this is an open decision, tracked at - [prompts#37](https://git.eeqj.de/sneak/prompts/issues/37). +- **SUPERSEDED, and deleted rather than kept: the per-checkout + `GOLANGCI_LINT_CACHE`/`TMPDIR` wrapper for `script/lint`.** Every line of it + was about making a linter run on a shared host trustworthy — a private result + cache so a byte-identical checkout could not serve its findings, a private + `TMPDIR` so the host-global lock could not collide, retry and VOID handling so + a lock collision was never reported as findings. The containerised-lint rule + above removes the host run itself, so there is nothing left for that wrapper + to isolate, and a repo carrying both would carry two contradictory canonical + `script/lint` forms. Its findings are not lost: they are the evidence for + containerising, and they are recorded in that rule. Repos that adopted it + delete the wrapper, the `.lint-cache/` entries from `.gitignore` and + `.dockerignore`, and the `--allow-serial-runners` flag with them. - **Keep `GOLANGCI_LINT_VERSION` and the ref in sync.** The ref is a hash and - carries no readable version, so the expected version is a separate string, - and it must be exactly what `--version` prints for that ref — the comparison - is an exact match on the whole version token. When the pinned commit carries - a release tag the go command resolves the hash to that tag, so the string is - simply the release number, `2.12.2` here. When it does not, the go command - falls back to a pseudo-version and the binary reports something like - `2.12.3-0.20260506110758-c0d3ddc9cf3f`; that compares exactly like any other - string, so it works, but it cannot be known without building the binary once - and reading `--version` off it. Prefer pins on tagged releases for that - reason — the expected string is then derivable from the ref — not because - the comparison cannot handle the alternative. + Two of its conclusions are kept because they outlive it. **`GOCACHE` does + not need isolating**, measured rather than assumed: it is content-addressed, + its entries are compiled artifacts rather than diagnostics carrying a + foreign tree's paths, and it has no equivalent global lock — the whole fleet + compiles concurrently against one `GOCACHE` all day without a contention + error. And **verifying any change to lint plumbing requires paired + controls**: a control that passes against the broken form proves nothing, + and it must be run against the artifact as a consuming repo would adopt it — + the file executed, not the functions sourced and driven by hand. - Because the comparison covers the whole token, a pre-release is never - confused with its release: a host carrying `2.12.2-rc1` against a `2.12.2` - pin compares unequal and gets reinstalled. This matters more than it looks, - because a pre-release tag is still a tag, so a rule requiring merely that - the pin be tagged would not catch it. - - **Verifying a change to this logic requires a negative control run in an - environment where a shadowing binary exists earlier in `PATH` than the - install target.** Without that, the control passes against the naive - compare-then-install form as well and therefore proves nothing. Also check - the mis-parse direction by feeding it unparseable `--version` output and - confirming it reinstalls rather than reporting a match. - - **Run those controls against the block as a consuming repo would adopt it** - — pasted into a `script/bootstrap`-shaped file that is then executed — not - by sourcing it and invoking the function yourself. Driving the function - directly tests something the artifact does not do, and it is exactly how a - missing call site passes every control while the adopted snippet does - nothing. - -- **`script/lint` in Go repos must give golangci-lint per-checkout cache and - lock state, and must never report a lock collision as a lint result.** - golangci-lint shares two pieces of state across every process on the host, and - they are separate mechanisms with separate fixes. Isolating one and stopping - leaves the other fully live while reading as a fix. This is independent of the - pinned-install rule above and does not replace it: that one makes the host run - the right linter, this one makes the run's result belong to your own tree. - - **Mechanism 1, the result cache — produces false greens as well as false - reds.** golangci-lint keys cached results on file **content, not location**, - so two checkouts of the same commit hold byte-identical files, share cache - entries, and one tree's findings are served for the other — reported at the - _other_ tree's path. Observed across the org: 399 issues attributed to a - `/tmp` worktree that no longer existed, returned from a clean clone that - genuinely lints 0 issues; ten findings against a deleted worktree; findings - reported against `../wt82-lint/...`; and, in the dangerous direction, an - implementer reporting "lint 0 issues" on a branch that was genuinely red. - Note what content-keying implies: **moving agents from worktrees to their - own clones does not help.** Two clones of a repo are byte-identical exactly - as two worktrees were. What own-clones removes is the deleted-worktree path - artefact — the loud, obviously wrong symptom — while leaving the mechanism - live, which makes the defect quieter rather than rarer. - - **Mechanism 2, the concurrency lock — and it does not live in the cache - directory.** From `pkg/commands/run.go`, `acquireFileLock()`: - - ```go - lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock") - ``` - - That is `$TMPDIR/golangci-lint.lock` — host-global, keyed on the temp - directory, entirely independent of `GOLANGCI_LINT_CACHE`. It is an `flock` - retried every second under a **5-second total timeout**, so it fails - precisely when the host is busiest. On failure the run emits - `parallel golangci-lint is running` and analyzes nothing. **A private cache - directory does not prevent this**; that was established by controlled test, - with two concurrent runs under separate cache directories sharing no mounted - path, one of which still collided. Anyone who sets only - `GOLANGCI_LINT_CACHE` has closed the contamination half and left the - false-red half untouched. - - The canonical form for a Go repo's `script/lint`: - - ```sh - #!/bin/sh - # script/lint: run the linter. - set -eu - - ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" - - # Per-checkout golangci-lint state. Both variables are required and they - # fix different defects; neither is redundant with the other. - # - # GOLANGCI_LINT_CACHE: the result cache is keyed on file content, not - # location, so checkouts holding identical files serve each other's - # findings. A per-REPO cache directory does NOT fix this — every checkout - # of that repo still collides — so the path must be inside the invoking - # checkout. - # - # TMPDIR: golangci-lint flocks $TMPDIR/golangci-lint.lock - # (pkg/commands/run.go, acquireFileLock: filepath.Join(os.TempDir(), - # "golangci-lint.lock")). That lock is host-global and independent of - # GOLANGCI_LINT_CACHE, with a 5s acquire timeout. Scoping TMPDIR into the - # checkout is the only thing here that isolates it. Do not delete this as - # redundant with the cache variable; it is not. - # - # The leading dot in .lint-cache is load-bearing: the go tool skips - # dot-prefixed directories when expanding ./..., so the linter never reads - # its own cache and temp files back as source. Do not rename it. - LINT_STATE="$ROOT/.lint-cache" - GOLANGCI_LINT_CACHE="$LINT_STATE/cache" - TMPDIR="$LINT_STATE/tmp" - export GOLANGCI_LINT_CACHE TMPDIR - mkdir -p "$GOLANGCI_LINT_CACHE" "$TMPDIR" - - # Capture files, per INVOCATION and not per checkout. Two runs in the same - # checkout would otherwise redirect into one pair of fixed paths, opened - # O_TRUNC before the linter even starts, and each would print and scan the - # other's output — a run reporting a result that is not its own, which is - # the whole defect this bullet exists to close, one layer up from where it - # was closed. That case is not hypothetical here: two runs in the same - # checkout is exactly what --allow-serial-runners below exists to support, - # and serialising the linter does not serialise the shell's redirections - # or the grep and cat that read them. mktemp rather than $$: two - # containerised runs over one bind-mounted checkout are in separate PID - # namespaces and can both be PID 7, which puts the collision back. - LINT_OUT="$(mktemp "$LINT_STATE/run.out.XXXXXX")" - LINT_ERR="$(mktemp "$LINT_STATE/run.err.XXXXXX")" - - # Print whatever the linter had written before the interruption, then exit - # 128+signal. The exit is what makes this handler TERMINATING, and that is - # the point: a signal-trap handler that returns RESUMES the script. With - # the capture files already deleted, execution would fall into the grep - # below against a missing file, take the not-a-collision branch, and report - # the FINDINGS exit status with empty output for a run that was killed — - # after deleting the findings it was about to print. That is why cleanup - # is on EXIT only. Killing a run is not hypothetical here: it is the stated - # mitigation for the unbounded wait --allow-serial-runners can produce, and - # it is what Ctrl-C on a make check does. - # - # All three writes are best-effort, and the `|| :` on each is load-bearing - # rather than defensive habit. Under set -e a failed write aborts the - # function BEFORE exit "$1", and the shell then exits 1 — the findings - # status, on a run that analysed nothing. SIGHUP is precisely the case - # where writing fails: once the controlling terminal is gone the writes - # return EIO. Guarding only the two cats is not enough, because with the - # capture files empty the cats write nothing and succeed, and the echo is - # what fails. - lint_interrupted() { - if [ -f "$LINT_ERR" ]; then cat "$LINT_ERR" >&2 || :; fi - if [ -f "$LINT_OUT" ]; then cat "$LINT_OUT" || :; fi - echo "lint: interrupted by a signal, so nothing was completed." \ - "This is NOT a lint result." >&2 || : - exit "$1" - } - - # The EXIT trap still fires on the way out of a terminating handler, so - # cleanup happens exactly once on every path. `|| :` because a failing rm - # — an unwritable state directory is enough — would otherwise change the - # exit status of an otherwise clean run under set -e. - trap 'rm -f "$LINT_OUT" "$LINT_ERR" || :' EXIT - trap 'lint_interrupted 129' HUP - trap 'lint_interrupted 130' INT - trap 'lint_interrupted 143' TERM - - # Backstop for a caller that reached the linter without the environment - # above. With it set, this should never fire. - LINT_MAX_ATTEMPTS=5 - # EX_TEMPFAIL. Distinct from 1 (findings) and 3 (linter error) so a void - # run is never counted as either. - LINT_VOID_EXIT=75 - - golangci_lint_run() { - attempt=1 - delay=2 - while :; do - rc=0 - # --allow-serial-runners KEEPS the mutual-exclusion guard and makes - # an overlapping run queue on the lock instead of aborting after - # 5s. It is NOT --allow-parallel-runners, which removes the guard - # entirely; never use that one. This is what covers two runs inside - # the SAME checkout, which TMPDIR scoping cannot — script/precommit - # overlapping a make check is the realistic trigger. - golangci-lint run --allow-serial-runners "$@" \ - >"$LINT_OUT" 2>"$LINT_ERR" || rc=$? - - # Detect the lock collision on the STDERR STREAM, never on the exit - # status. Findings are written to stdout and golangci-lint reports - # this failure only on stderr, so a finding that quotes the string - # from source cannot be mistaken for a collision and retried away — - # that direction would be a false green. The exit status is not a - # usable discriminator: the collision exits 3 (exitcodes.Failure), - # which does separate it from findings at 1, but run.go returns it - # as a plain error that Execute maps to Failure like every other - # error at that level, so 3 cannot separate a collision from a - # genuine linter failure — an unknown linter name, an unknown flag, - # malformed config YAML all exit 3 too. (An unparseable Go source - # file does not: that is reported as typecheck issues and exits 1.) - # Retrying on 3 would retry real failures into a void. - if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then - cat "$LINT_ERR" >&2 - cat "$LINT_OUT" - return "$rc" - fi - - if [ "$attempt" -ge "$LINT_MAX_ATTEMPTS" ]; then - cat "$LINT_ERR" >&2 - echo "lint: VOID after $LINT_MAX_ATTEMPTS attempts:" \ - "golangci-lint never acquired its lock, so nothing was" \ - "analyzed. This is NOT a lint result and no verdict may" \ - "be recorded from it. Re-run it." >&2 - return "$LINT_VOID_EXIT" - fi - echo "lint: lock held by another golangci-lint; attempt" \ - "$attempt of $LINT_MAX_ATTEMPTS, retrying in ${delay}s" >&2 - sleep "$delay" - attempt=$((attempt + 1)) - delay=$((delay * 2)) - done - } - - main() { - cd "$ROOT" - golangci_lint_run ./... - } - - main "$@" - ``` - - Load-bearing properties, each guarding a mode that otherwise reports a - verdict it did not earn: - - **Both variables, per checkout.** Cache alone leaves the false reds; - `TMPDIR` alone leaves the contamination that produced a confirmed false - green. A per-repo path for either is not isolation on a host where every - worker holds its own copy of the same repo. - - **Set them on every path that reaches the linter.** The export block goes - _above_ any container-versus-host branch, and a native escape hatch must - call `golangci_lint_run` rather than `exec golangci-lint` directly. One - repo in the org had exactly one such path with no cache environment at - all, inheriting the fleet-wide default, so the fix on the other path was - worth nothing there. - - **The lock collision is not a result, and must never be reported as one.** - Retry it, and on exhaustion exit a status that is neither the findings - status nor success, with a message that says VOID. Swallowing it into a - success is the worst available outcome; reporting it as findings sends a - correct branch back for rework against findings that do not exist. - - **Detect the collision by the message on stderr, not by exit status, and - never retry a genuine finding.** Distinguishing "exited non-zero because - of the lock" from "exited non-zero because of findings" is the whole crux, - and getting it wrong in the direction of treating findings as a lock error - retries a real failure into a void — or, if a later implementation decided - to treat exhaustion as success, into a green. - - **`--allow-serial-runners`, never `--allow-parallel-runners`.** The first - keeps the guard and queues; the second deletes it and lets two runs - corrupt shared state. With the flag set, an overlapping run inside the - same checkout waits rather than failing, which is what is actually wanted. - The honest cost is that it waits without bound, so a stale process holding - the lock hangs the run instead of failing it; the contending set is - bounded to the same checkout, and an eventual result is preferable to a - fabricated one. - - **Capture stdout and stderr to per-INVOCATION paths, and clean them up.** - Two fixed paths under the checkout are one pair for every run in it, and - the redirections truncate them before the linter starts, so two - overlapping runs print and scan each other's output. - `--allow-serial-runners` does not prevent this — it serialises the linter, - not the shell — and the overlap it exists to support is precisely - `script/precommit` against a `make check` in one checkout. The observed - shapes are a run printing the other's `0 issues.` while its own linter - found something, and a lock error erased before `grep` reads it, so the - retry never fires and the void run returns as a result. Both are a run - reporting a result that is not its own, which is this bullet's entire - subject reintroduced one layer above where it was fixed. Use `mktemp` - under the state directory rather than `$$`: two containerised runs over - one bind-mounted checkout sit in separate PID namespaces and can hold the - same low PID, which puts the collision back on exactly the fleet's - arrangement. `$$` is acceptable only where `mktemp` is unavailable and - that arrangement is ruled out. - - **Clean up on `EXIT` only, and give each signal a TERMINATING handler.** A - signal-trap handler that does not exit **resumes** the script: with the - capture files already deleted, the run falls into the `grep` against a - missing file, takes the not-a-collision branch, and reports the - **findings** exit status with empty output for a run that was killed — - having deleted the findings it was about to print. Measured: `TERM`, `INT` - and `HUP` all returning 1 with empty stdout, against 143, 130 and 129 for - the same block without the handler. Exit `128+signal` instead, and let the - `EXIT` trap do the cleanup on the way out. The handler also makes a - **best-effort** attempt to print what the linter had already written — - best-effort because under `SIGHUP` the terminal is typically gone and - every write returns `EIO`, in which case nothing is printed and only the - status carries the message. Each of those writes needs its own `|| :`: - under `set -e` a failed write aborts the handler before it reaches `exit`, - and the shell then exits 1, which is the findings status on a run that - analysed nothing. Guarding only the `cat`s is not enough — with the - capture files empty they write nothing and succeed, and the `echo` is what - fails. Put `|| :` on the `rm` in the `EXIT` trap for the same reason: an - unwritable state directory makes it fail, and a failing `EXIT` trap under - `set -e` turns an otherwise clean run into exit 1, measured in both `dash` - and `bash`. - - **A signal must reach the LINTER, not just the wrapper.** POSIX defers a - trap until the running foreground command completes, so - `kill -TERM ` does nothing at all while `golangci-lint` is - running — measured still alive three seconds later, where the same block - without a handler dies immediately at 143. Ctrl-C is unaffected because - the terminal signals the whole process group. This matters exactly where - the handler is supposed to help: the unbounded `--allow-serial-runners` - wait, where the process holding things up is the linter itself. Kill the - group (`kill -- -`) or use Ctrl-C. - - **Known, accepted gap:** a signal arriving between the `mktemp` calls and - the `trap ... EXIT` line leaves the two capture files behind. It is - closable, and cheaply — initialise both variables to the empty string and - move all four `trap` lines above the `mktemp` calls, with nothing - rewritten afterwards. It is accepted anyway because of what the gap costs, - not because of what closing it costs: two stray files in a gitignored - directory, never an incorrect result. Reconsider it on that trade-off if - the balance ever changes. - - Adopting repos must add `.lint-cache/` to both `.gitignore` and - `.dockerignore`. The second matters as much as the first: the directory - reaches tens of megabytes, and without the exclusion it enters the build - context and invalidates `COPY . .` for reasons unrelated to the repo's - content. - - **`GOCACHE` does not need isolating, and this was measured rather than - assumed.** With `GOLANGCI_LINT_CACHE` and `TMPDIR` per checkout and - `GOCACHE` left at the host default and shared, two checkouts of identical - content each reported their own paths and neither reported the other's. The - Go build cache is content-addressed and its entries are compiled artifacts - rather than diagnostics carrying a foreign tree's paths, and it has no - equivalent global lock — the whole fleet compiles concurrently against one - `GOCACHE` all day without a contention error. Isolating it would cost a full - cold compile per checkout for no measured benefit. The earlier hypothesis - that Go build-cache contention might explain the lock error is superseded: - the lock is located in source at `$TMPDIR/golangci-lint.lock`. - - **Verifying a change to this logic requires a negative control, and the - control must be built out of checkouts with identical content.** Create two - checkouts of the same tree containing a deliberate lint finding, run the - linter in the second so it populates the cache, then run it in the first and - confirm the finding is reported at the first checkout's own path and never - at the second's. Run the same control against the unisolated form and - confirm the contamination appears there — a control that passes against the - broken implementation proves nothing. Note specifically that a control built - from checkouts whose **content differs** passes against the unisolated form - too, because differing content does not collide in a content-keyed cache, so - it is not a test of anything. For the lock half, hold - `$TMPDIR/golangci-lint.lock` with `flock` and confirm the run queues rather - than aborting, that a caller never sees the collision as findings, and that - exhaustion fails loudly and distinguishably. - - **Run those controls against the block as a consuming repo would adopt it** - — pasted into a `script/lint`-shaped file that is then executed, not sourced - with the functions driven by hand. The same warning as for the bootstrap - block above, for the same reason. - -- **Interim rule for reading a golangci-lint result on a shared host, until - every repo has adopted the isolation above.** A lint run is **VOID** unless - both hold: +- **Interim rule for reading a lint result produced on the host, in a repo that + has not yet adopted the containerised lint above.** A lint run is **VOID** + unless both hold: - the output contains no `parallel golangci-lint is running`, and - no reported file path begins with `../`, and none is an absolute path outside the tree the run was launched from. @@ -1033,8 +851,9 @@ style conventions are in separate documents: that mode has been observed, and nobody should go chasing it**; the point is the reach of the tests, not a claim that the mode exists. They are a filter for the loud mode, not a proof of soundness — which is the whole argument - for fixing this in the tooling instead of documenting a discipline that - depends on every agent remembering to apply it. + for containerising the linter instead of documenting a discipline that + depends on every agent remembering to apply it. Adopt the rule above and + this one stops applying to the repo entirely. - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD). diff --git a/script/check b/script/check index 3e1778c..b4085ec 100755 --- a/script/check +++ b/script/check @@ -1,6 +1,13 @@ #!/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/lint is a docker build (see Dockerfile.lint), so this script +# requires a docker daemon. That is deliberate: it is the only way a +# developer and the pre-commit hook get the same linter CI gets. It also +# means this script must never be run from inside a build stage — see +# the comment in Dockerfile, which runs the individual non-lint checks +# for exactly that reason. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" diff --git a/script/cibuild b/script/cibuild index 4fbd751..c94c16f 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,14 +1,21 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile runs script/check, but -# that only proves anything because CHECK_EPOCH is a fresh nonce on every -# invocation: without it Docker serves the check layer from cache on an -# unchanged tree and the build exits 0 without running the suite. +# script/cibuild: run the CI build. Two container builds, in order: +# script/lint (Dockerfile.lint) and then the main image, which runs the +# non-lint checks. Both only prove anything because each passes its own +# fresh CHECK_EPOCH nonce: without it Docker serves the check layers +# from cache on an unchanged tree and the build exits 0 without running +# anything. set -eu -ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" +ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" + # Lint first, for fail-fast feedback: it is its own container build + # and computes its own CHECK_EPOCH. It runs here rather than inside + # the main image because a docker build cannot run a docker build. + "$SCRIPT_DIR/lint" # Assign on its own line: a failing command substitution inside an # argument does not trip `set -e`, which would silently degrade the # nonce to an empty constant. `$$` is required because busybox `date` diff --git a/script/docker b/script/docker index f9ebf9a..0a08060 100755 --- a/script/docker +++ b/script/docker @@ -4,6 +4,12 @@ # Dockerfile's checks only actually run because CHECK_EPOCH is a fresh # nonce on every invocation; without it a warm cache turns this into a # green that proves nothing. +# +# Those checks are the NON-LINT ones. Linting left this image: it runs +# in its own container, built by script/lint, because script/lint is a +# docker build and cannot run inside one. So a green here does not mean +# the branch is green — script/cibuild, which runs script/lint first, is +# what means that. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" diff --git a/script/lint b/script/lint index c489634..2af27c6 100755 --- a/script/lint +++ b/script/lint @@ -1,13 +1,33 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run the linter. This is the ONLY source of a lint verdict +# in this repo — the linter runs in a container, one way, everywhere, so +# a run cannot inherit another checkout's cache, another process's lock, +# or a host toolchain that differs from the pinned one. Linting happens +# as a build step (see Dockerfile.lint), so a successful build is a +# clean lint, and it works where the docker daemon is remote and bind +# mounts are impossible. +# +# A failure here that names no finding is NOT a lint result: docker +# build exits 1 both for findings and for a build that never reached the +# lint step (daemon down, image unpullable, disk full). BuildKit names +# the failing step; read it, fix the environment, and re-run. Do not +# record a verdict from a run that did not lint. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - echo "Linting markdown files..." - yarn run prettier --check '**/*.md' --tab-width 4 --prose-wrap always + # Assign on its own line: a failing command substitution inside an + # argument does not trip `set -e`, which would silently degrade the + # nonce to an empty constant. `$$` is required because busybox `date` + # drops %N without erroring. Without a fresh nonce the lint layer is + # served from cache and this script exits 0 having linted nothing. + epoch="$(date +%s%N)$$" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + -f Dockerfile.lint \ + . } main "$@"