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..b9ede1d 100644 --- a/README.md +++ b/README.md @@ -117,19 +117,25 @@ 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 + linter runs in a container, always: it is never installed on the host and + never invoked there. 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 - `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 + `docker build --build-arg CHECK_EPOCH="$epoch" .` (what CI runs). 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. A bare `docker build .` 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..56c27ba 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,53 @@ fmt-check, and commit. # Completed Steps +- 2026-08-10: Closed three gaps the containerised-lint rule left between the + canonical text and the first repos to implement it. `.dockerignore` excluding + the agent scratch directory is now stated as a correctness precondition of + that rule rather than a context-size measure: the lint image lints whatever + `COPY . .` copies, and toolchains discover files by walking the tree instead + of reading `.gitignore`, so a nested worktree puts the foreign-tree false reds + back inside the container — `sneak/quak` measured the same discovery mechanism + taking a test count from 210 to 1050. The cache-bust arg is fixed at + `CHECK_EPOCH` in `Dockerfile.lint` as well, because a per-file name is + invisible to the grep that proves every build is busted, making a renamed + guard indistinguishable from a missing one. And the formatting check is now + required to run in exactly one of the two images, with either placement + allowed: splitting lint out of the `Dockerfile` is precisely when `fmt-check` + gets dropped from both, and running the formatter beside the linters is the + better shape where it is the same pinned dependency. +- 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. 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..d199899 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,24 @@ 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 is never installed on the host. + 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. -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..d0d140e 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,31 @@ 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`. The + arg is named `CHECK_EPOCH` in this file too — a repo that calls it + `LINT_EPOCH` here is missed by the grep that checks every build is + cache-busted. +- [ ] The formatting check runs in exactly one of the two images — either + `script/fmt-check` in the `Dockerfile` or the formatter beside the linters + in `Dockerfile.lint`, whichever puts it on the pinned toolchain. Neither + image running it is the failure to look for here, since moving lint out of + the `Dockerfile` is exactly when it gets dropped. - [ ] `.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 @@ -50,8 +69,11 @@ with your task. - [ ] `.dockerignore` excludes `.claude`, root-anchored and with no `**/` prefix. Agent worktrees are entire checkouts of the repo, so they inflate the context by a multiple of it and can copy another session's unreviewed - work into an image layer. Confirm by enumerating the image, not by reading - the file — `.gitignore` hides these from `git status` too. + work into an image layer — and `Dockerfile.lint` then lints that checkout + as though it were this one, because toolchains discover files by walking + the tree and never read `.gitignore` (`sneak/quak`: 210 discovered tests + became 1050). Confirm by enumerating the image, not by reading the file — + `.gitignore` hides these from `git status` too. - [ ] **Do agents in this repo run anywhere other than the repo root?** The scratch directory is created in the agent's working directory, so the canonical anchored entry misses `services/api/.claude/` in a monorepo with @@ -100,13 +122,24 @@ 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. No host + linter invocation survives anywhere in the repo — grep for the linter's + own name in `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. +- [ ] `script/bootstrap` installs no linter. Delete the golangci-lint install + 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. +- [ ] 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 +186,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..7c46a9e 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,36 @@ 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. The arg keeps the name `CHECK_EPOCH` in + this file as well, so one grep covers both builds. + - The formatting check runs in exactly one of the two images: either + `script/fmt-check` in the `Dockerfile`, or the formatter beside the + linters in `Dockerfile.lint` where that is the same pinned dependency. + Never neither, never both. + - `.dockerignore` must exclude the agent scratch directory before this image + is trusted: it lints whatever is in the build context, and toolchains walk + the tree rather than reading `.gitignore`, so an agent worktree that + reaches the context is linted as though it were the repo. - [ ] 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 +129,26 @@ 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 .`. The + linter is never installed on the host and never invoked there. Copy the + canonical script from `REPO_POLICIES.md`; it is byte-identical across + repos. Without the nonce this script exits 0 on an unchanged tree having + linted nothing. - [ ] `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 +160,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 +182,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..287c294 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,34 +94,51 @@ 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 +- Every repo should have a `Dockerfile`. It must run the repo's checks as build + steps 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. + cannot fail on a branch that is not green. + + **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. Of the two, only `script/test` is fixed + here: a repo may run its formatter in `Dockerfile.lint` beside the linters + instead, and some should — see the containerised-lint rule below. It must + then run in that file and not in this one, and never in neither. + + 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`: + and in `script/lint`, `script/cibuild` and `script/docker`: ```sh epoch="$(date +%s%N)$$" @@ -134,8 +152,10 @@ style conventions are in separate documents: 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; - none is optional, and each guards a failure mode that otherwise fails green: + document half a command. `script/lint` passes only `CHECK_EPOCH`, since no + version is embedded in a lint image. All four `CHECK_EPOCH` elements are + load-bearing; none is optional, and each guards a failure mode that + otherwise fails green: - `ARG` is stage-scoped, so a single declaration leaves the other check stages frozen while the fix reviews as complete. Declare it in every stage that runs checks, immediately above the first such `RUN`. @@ -164,47 +184,232 @@ 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, which makes linting network-dependent and pushes a lint that + should take seconds toward the build ceiling. 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 linter is never installed on the host and never invoked there. + 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`, identical in every repo: + + ```sh + #!/bin/sh + # script/lint: run the linter. The linter is never installed on the host + # and never invoked there — it 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. + 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, which makes every lint network-dependent. + - **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, and lints + with the linter from `node_modules`, which is also how it gets the version + pinned in `package.json` rather than whatever is on the host. + - **The lint container lints whatever is in the build context, so + `.dockerignore` is part of this rule and not merely hygiene.** `COPY . .` + copies an agent scratch worktree — an entire second checkout of the repo — + into the lint image unless `.dockerignore` excludes it, and language + toolchains discover files by walking the tree rather than by reading + `.gitignore`, so `./...`, `eslint .` and `prettier --check .` all descend + into it. `sneak/quak` measured this on the same discovery mechanism in its + test runner: a nested `.claude/` worktree took the discovered test count + from 210 to 1050 (https://git.eeqj.de/sneak/quak/issues/30). Left in the + context it re-creates _inside_ the container the foreign-tree false reds + that moving lint into a container was adopted to end, and it does so in + the convincing form — the findings are real, they simply belong to another + checkout. See the `.dockerignore` rules below, and verify by enumerating + the image rather than by reading the patterns. + - **The build arg is named `CHECK_EPOCH` in `Dockerfile.lint` too**, not + `LINT_EPOCH` or any other per-file name, and `script/lint` passes it under + that name. Both files guard the same failure under the same contract, and + the single name is what lets a reviewer grep a repo for `CHECK_EPOCH` and + see every cache-bust it has. Rename it in one file and that grep silently + misses it, so a renamed guard and an absent guard read identically without + opening both Dockerfiles. + - **The formatting check runs in exactly one of the two images, and either + one is allowed.** The canonical `Dockerfile` above runs `script/fmt-check` + because that is where the non-lint checks live. A repo may instead run its + formatter in `Dockerfile.lint` beside the linters, which is the better + shape wherever the formatter is the same pinned dependency as the linter + (`prettier` out of `node_modules`, say), because it takes the last host + toolchain off the checked path for the same reason the linter came off it. + What is not allowed is running it in neither image, or in both. Whichever + image runs it carries the epoch guard, and `script/check` still runs all + three targets on the developer's side either way. + - **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 `script/bootstrap` or anywhere + else.** A host install is now dead weight whose only remaining effect is + to reintroduce the version skew above. + - **`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`. + + **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}" && make fmt-check + RUN make 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 +426,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 +445,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 @@ -388,14 +571,22 @@ style conventions are in separate documents: inflates by a multiple of the repo, and another session's unreviewed, sometimes uncommitted work can be copied into an image layer. The directory is also created and destroyed constantly, so it invalidates `COPY . .` for - reasons that have nothing to do with the repo's own content. In `.gitignore` - the entry is `.claude/`, unanchored, which already matches at every depth. In - `.dockerignore` it is `.claude`, anchored and with **no** `**/` prefix: the - directory occurs exactly once **where agents run at the repo root**, and the - prefixed form would also match any nested directory of that name and delete it - from the build. It is not case-folded the way the secret patterns are, because - tooling creates it in exactly one spelling, so a folded pattern would add no - coverage. + reasons that have nothing to do with the repo's own content. **And because + `Dockerfile.lint` and `Dockerfile` run their tooling over the copied context, + a worktree that reaches it is linted and tested as though it were the repo.** + Nothing else stops that: language toolchains discover files by walking the + tree and do not read `.gitignore`, which is how `sneak/quak` saw a nested + `.claude/` worktree take its discovered test count from 210 to 1050 + (https://git.eeqj.de/sneak/quak/issues/30). This entry is therefore a + correctness precondition of the containerised-lint rule above and not a size + optimisation — without it the foreign-tree false reds that rule exists to end + simply move inside the container. In `.gitignore` the entry is `.claude/`, + unanchored, which already matches at every depth. In `.dockerignore` it is + `.claude`, anchored and with **no** `**/` prefix: the directory occurs exactly + once **where agents run at the repo root**, and the prefixed form would also + match any nested directory of that name and delete it from the build. It is + not case-folded the way the secret patterns are, because tooling creates it in + exactly one spelling, so a folded pattern would add no coverage. **Known gap that comes with the anchored form.** The directory is created in the agent's working directory, so the "exactly once, at the root" premise is @@ -498,516 +689,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 +806,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/lint b/script/lint index c489634..858a336 100755 --- a/script/lint +++ b/script/lint @@ -1,13 +1,27 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run the linter. The linter is never installed on the host +# and never invoked there — it 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. 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 "$@"