From 51c394552e0703018d78e2ce284fa8c36112ffd8 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 14:46:13 +0000 Subject: [PATCH 1/6] Bust the Docker check-layer cache with a per-invocation CHECK_EPOCH (closes #26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit script/cibuild was a plain `docker build .`, and the Dockerfile does `COPY . .` followed by `RUN make check`. Docker invalidates a COPY layer only when the copied content changes, so on an unchanged tree the check layer was served from cache, the suite never ran, and the build still exited 0. Measured here: run 1 took 18.5s and ran the suite; run 2 on a byte-identical tree took 0.286s with `RUN make check` CACHED. script/cibuild and script/docker now assign a per-invocation nonce on its own line and pass it as --build-arg CHECK_EPOCH. The Dockerfile declares ARG CHECK_EPOCH, guards it with `[ -n "$CHECK_EPOCH" ] || exit 1`, and expands it into the check command. Post-fix, two consecutive runs both execute make check (17.4s / 8.1s) with `RUN script/bootstrap` still CACHED, so dependency layers are untouched and the build ceiling is not at risk. The guard is what makes a bare `docker build .` — the command REPO_POLICIES named verbatim — fail closed rather than reuse the empty and therefore stable cache key; verified failing in 0.455s. Holding the epoch constant restores the false green (run 2 fully CACHED), which pins the varying value as the operative mechanism rather than a coincidence. Because the guard references $CHECK_EPOCH it is itself value-keyed: BuildKit renders the epoch into that layer's description and re-runs the layer when the value changes. Each stage therefore has two independent invalidation points, the guard and the expansion, and the guard always precedes the check RUN. Both are kept and the prose now records this; the expansion remains defence in depth and is what puts the epoch in the build log. The false guarantee was org-canonical text in more than one document, so it is corrected everywhere it appeared rather than only where the issue first found it. REPO_POLICIES.md carried it in two places, and its Go multistage template had check steps in two stages; ARG is stage-scoped, so both stages get the treatment or the fleet inherits the half-fixed shape. CODE_STYLEGUIDE_GO.md restated the guarantee for the bare command this change makes fail closed. NEW_REPO_CHECKLIST.md specified the pre-fix script/cibuild verbatim, so every new repo would have been born with the false green, and EXISTING_REPO_CHECKLIST.md ended on a `docker build` acceptance item that the guard makes unsatisfiable by design — an agent working that checklist would have been led to delete the guard to tick the last box. Both checklists' Dockerfile criteria were also satisfiable by a Dockerfile whose check layers are still frozen, and now require the ARG and guard in every check-running stage. REPO_POLICIES.md's own Dockerfile criterion carried that same incomplete form; it is tightened by cross-reference to the CHECK_EPOCH rule rather than by duplicating the canonical block. The Go template's Key points gain a caveat that the cache-bust turns the `COPY --from=lint` no-op into a content-cache hit, so a repo using a file-dependency trick for stage ordering must re-prove that ordering on a warm cache after adopting it. That was re-proved in another repo in the org which uses the trick with a marker file, where the ordering held; the caveat states explicitly that it was not verified here, this repo being single-stage with no lint stage to order against. --- Dockerfile | 15 +++- README.md | 10 ++- TODO.md | 9 ++ prompts/CODE_STYLEGUIDE_GO.md | 15 +++- prompts/EXISTING_REPO_CHECKLIST.md | 18 ++-- prompts/NEW_REPO_CHECKLIST.md | 19 +++- prompts/REPO_POLICIES.md | 139 +++++++++++++++++++++++------ script/cibuild | 13 ++- script/docker | 13 ++- 9 files changed, 203 insertions(+), 48 deletions(-) diff --git a/Dockerfile b/Dockerfile index cb6489a..ac7232a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -12,4 +12,17 @@ COPY package.json yarn.lock ./ RUN script/bootstrap COPY . . -RUN make check + +# CHECK_EPOCH is a per-invocation nonce supplied by script/cibuild and +# script/docker. Without it an unchanged tree serves this layer from +# cache and the build reports a green it never ran. ARG is stage-scoped, +# so it must be redeclared in every stage that runs checks. The guard +# makes a bare `docker build .` fail loudly instead of silently reusing +# the empty (and therefore stable) cache key. Expand the value into the +# command so the cache miss does not depend on BuildKit's handling of an +# unreferenced ARG. Both the guard and the check RUN reference the value, +# so both are value-keyed: there are two independent invalidation points +# here, not one. Keep both. +ARG CHECK_EPOCH +RUN [ -n "$CHECK_EPOCH" ] || exit 1 +RUN echo "check epoch: ${CHECK_EPOCH}" && make check diff --git a/README.md b/README.md index 3991bfc..357f8df 100644 --- a/README.md +++ b/README.md @@ -123,9 +123,13 @@ alpine. We provide: - `script/check` — run all checks: `test`, `lint`, `fmt-check` (our own extension) - `script/docker` — build the Docker image, tagged via `script/projectname` - (byte-identical across repos) -- `script/cibuild` — cd to the repo root and `docker build .` (what CI runs; the - image build runs `script/check`) + (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/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 fac919c..9e3b80b 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,15 @@ fmt-check, and commit. # Completed Steps +- 2026-08-09: Fixed the false green in the canonical CI gate: `script/cibuild` + and `script/docker` now pass a per-invocation `CHECK_EPOCH` nonce, and the + `Dockerfile` (plus the Go multistage template in REPO_POLICIES.md, in both its + lint and builder stages) declares `ARG CHECK_EPOCH` with a guard that makes a + bare `docker build .` fail closed. Corrected the org-canonical text that + asserted a successful build implies all checks pass, across every document + carrying it: `REPO_POLICIES.md`, both repo checklists (which still told agents + to write the pre-fix `script/cibuild` and ended on an acceptance item the + guard makes unsatisfiable), and the Go styleguide. - 2026-08-07: Set the canonical `.golangci.yml` to the org-standard v2-schema config already deployed byte-identical across the org's Go repos (settings under `linters.settings` so thresholds like lll/funlen/cyclop/dupl actually diff --git a/prompts/CODE_STYLEGUIDE_GO.md b/prompts/CODE_STYLEGUIDE_GO.md index 043e8f1..d81ffff 100644 --- a/prompts/CODE_STYLEGUIDE_GO.md +++ b/prompts/CODE_STYLEGUIDE_GO.md @@ -1,6 +1,6 @@ --- title: Code Styleguide — Go -last_modified: 2026-03-18 +last_modified: 2026-08-09 --- 1. Try to hard wrap long lines at 77 characters or less. @@ -101,9 +101,16 @@ last_modified: 2026-03-18 `golangci-lint`. 1. Write a `Dockerfile` for every repo, even if it only runs the tests and - linting. `docker build .` should always make sure that the code is in an - able-to-be-compiled state, linted, and any tests run. The Docker build - should fail if linting doesn't pass. + 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 + [Repository Policies](https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md) + for the canonical form. 1. Every repo must have a `Makefile`. See [Repository Policies](https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md) diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 2f45550..69cb02b 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -1,6 +1,6 @@ --- title: Existing Repo Checklist -last_modified: 2026-07-06 +last_modified: 2026-08-09 --- Use this checklist when beginning work in a repo that may not yet conform to our @@ -29,10 +29,15 @@ with your task. if missing - [ ] `.editorconfig` exists — fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.editorconfig` -- [ ] `Dockerfile` and `.dockerignore` exist; Dockerfile runs `make check` as a - build step — fetch `.dockerignore` from - `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore` -- [ ] Gitea Actions workflow in `.gitea/workflows/` runs `docker build .` on +- [ ] `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. +- [ ] Gitea Actions workflow in `.gitea/workflows/` runs `script/cibuild` on push — reference `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitea/workflows/check.yml` - [ ] Language-specific config: @@ -104,5 +109,6 @@ with your task. # Final - [ ] `make check` passes -- [ ] `docker build` succeeds +- [ ] `script/cibuild` succeeds (a bare `docker build .` 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 2eb58ea..5b0bcfe 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -1,6 +1,6 @@ --- title: New Repo Checklist -last_modified: 2026-07-06 +last_modified: 2026-08-09 --- Use this checklist when creating a new repository from scratch. Follow the steps @@ -52,7 +52,12 @@ Template files can be fetched from: `https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md` - [ ] `Dockerfile` and `.dockerignore` — fetch `.dockerignore` from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore` - - All Dockerfiles must run `make check` as a build step + - 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. - Server: also builds and runs the application - Non-server: brings up dev environment and runs `make check` - Image pinned by sha256 hash with version/date comment @@ -90,8 +95,14 @@ are thin shims calling them. Model scripts: - [ ] `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) -- [ ] `script/cibuild` — cd to repo root, `docker build .` (what CI runs) + `script/projectname` (byte-identical across repos); assigns + `epoch="$(date +%s%N)$$"` on its own line and passes + `--build-arg CHECK_EPOCH="$epoch"` +- [ ] `script/cibuild` — cd to repo root, assign `epoch="$(date +%s%N)$$"` on + its own line, then run `docker build --build-arg CHECK_EPOCH="$epoch" .` + (what CI runs). The build arg is mandatory: see the `CHECK_EPOCH` rule in + `REPO_POLICIES.md` for why each element is load-bearing. A bare + `docker build .` fails closed by design. - [ ] `script/precommit` — called by the pre-commit hook; runs `script/check` - [ ] `script/install-precommit` — installs the pre-commit hook that runs `script/precommit` diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index 79d2fb7..82f6ae9 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -1,6 +1,6 @@ --- title: Repository Policies -last_modified: 2026-08-07 +last_modified: 2026-08-09 --- This document covers repository structure, tooling, and workflow standards. Code @@ -60,17 +60,19 @@ 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 `docker build .`; the Gitea workflow calls it. Four further - scripts are our own extensions to the standard: `script/check` runs - `script/test`, `script/lint`, and `script/fmt-check`; `script/precommit` is - what the git pre-commit hook runs, and it calls `script/check`; - `script/install-precommit` installs the git pre-commit hook (the `make hooks` - target shims to it); and `script/projectname` (literally that filename) simply - outputs the project's name. Scripts that need the name call - `script/projectname` — e.g. `script/docker` assembles its image tag from it — - so those scripts stay byte-identical across all repos. Repo-type-specific - pre-commit extras (e.g. `go mod tidy` verification in Go repos) belong in - `script/precommit`, not in the hook itself. Model scripts are at + repo root and runs `docker build --build-arg CHECK_EPOCH="$epoch" .`, where + `epoch` is a per-invocation nonce (see the `CHECK_EPOCH` rule below); the + Gitea workflow calls it. Four further scripts are our own extensions to the + standard: `script/check` runs `script/test`, `script/lint`, and + `script/fmt-check`; `script/precommit` is what the git pre-commit hook runs, + and it calls `script/check`; `script/install-precommit` installs the git + pre-commit hook (the `make hooks` target shims to it); and + `script/projectname` (literally that filename) simply outputs the project's + name. Scripts that need the name call `script/projectname` — e.g. + `script/docker` assembles its image tag from it — so those scripts stay + byte-identical across all repos. Repo-type-specific pre-commit extras (e.g. + `go mod tidy` verification in Go repos) belong in `script/precommit`, not in + the hook itself. Model scripts are at `https://git.eeqj.de/sneak/prompts/raw/branch/main/script/`. The README must document the provided scripts in an **Entrypoints** section (see the README requirements below). @@ -90,14 +92,70 @@ style conventions are in separate documents: 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. 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. + as a build step so the build fails if the branch is not green — which requires + `ARG CHECK_EPOCH` and its guard in every stage containing a check-running + `RUN`, per the `CHECK_EPOCH` rule below. Without them a Dockerfile satisfies + this criterion while its check layers are served from cache, so the build + cannot fail on a branch that is not green. For non-server repos, the + Dockerfile should bring up a development environment and run `make check`. For + server repos, `make check` should run as an early build stage before the final + image is assembled. Dockerfiles install development prerequisites by running + `script/bootstrap` rather than duplicating installs inline; COPY `script/` and + the dependency manifests (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, + etc.) before running it so the bootstrap layer stays cached until dependencies + change. + +- **Every 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`: + + ```dockerfile + ARG CHECK_EPOCH + RUN [ -n "$CHECK_EPOCH" ] || exit 1 + RUN echo "check epoch: ${CHECK_EPOCH}" && make check + ``` + + and in both `script/cibuild` and `script/docker`: + + ```sh + epoch="$(date +%s%N)$$" + docker build --build-arg CHECK_EPOCH="$epoch" . + ``` + + All four 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`. + - Expand the value into the command. This makes the cache miss contractual + rather than dependent on BuildKit's handling of an unreferenced `ARG`, and + it puts the epoch in the build log. The guard is itself value-keyed, for + the same reason: it references `$CHECK_EPOCH`, so BuildKit renders the + epoch into that layer's description (rendered as + `RUN [ -n "" ] || exit 1`) and re-runs it whenever the value + changes. Each stage therefore has two independent invalidation points, and + the guard always precedes the check `RUN`. Keep both: the expansion is + defence in depth, and it is what makes the epoch visible in the build + output. + - The `[ -n ... ]` guard is required: an unset `ARG` is empty, and empty is + a stable cache key, so without it a bare `docker build .` still produces + the false green. Failed steps are never cached, so the guard fails on + every such invocation, loudly. A bare `docker build .` failing is by + design. + - Assign `epoch=` on its own line, never inline in the `--build-arg` + argument: a failing command substitution inside an argument does not trip + `set -e`, so the inline form silently degrades to an empty constant. The + `$$` suffix is required because busybox `date` drops `%N` and exits 0, so + on an alpine host the epoch would degrade to second granularity and + concurrent invocations would collide. + + 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. - **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 @@ -119,7 +177,9 @@ style conventions are in separate documents: COPY go.mod go.sum ./ RUN go mod download COPY . . - RUN make fmt-check + ARG CHECK_EPOCH + RUN [ -n "$CHECK_EPOCH" ] || exit 1 + RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check RUN make lint # Build stage @@ -133,7 +193,9 @@ style conventions are in separate documents: COPY go.mod go.sum ./ RUN go mod download COPY . . - RUN make test + ARG CHECK_EPOCH + RUN [ -n "$CHECK_EPOCH" ] || exit 1 + RUN echo "check epoch: ${CHECK_EPOCH}" && make test ARG VERSION=dev RUN CGO_ENABLED=0 go build -trimpath \ @@ -154,6 +216,16 @@ style conventions are in separate documents: 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: @@ -165,11 +237,28 @@ style conventions are in separate documents: - 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. - Every repo should have a Gitea Actions workflow (`.gitea/workflows/`) that - runs `script/cibuild` (which runs `docker build .`) on push. Since the - Dockerfile already runs `make check`, a successful build implies all checks - pass. + runs `script/cibuild` (which runs + `docker build --build-arg CHECK_EPOCH="$epoch" .`) 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. - Use platform-standard formatters: `black` for Python, `prettier` for JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with diff --git a/script/cibuild b/script/cibuild index 75cc3e6..786533a 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,13 +1,20 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile runs script/check, so -# a successful build implies all checks pass. +# 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. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - docker build . + # 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. + epoch="$(date +%s%N)$$" + docker build --build-arg CHECK_EPOCH="$epoch" . } main "$@" diff --git a/script/docker b/script/docker index 9b9ea86..ac8897d 100755 --- a/script/docker +++ b/script/docker @@ -1,6 +1,9 @@ #!/bin/sh # script/docker: build the Docker image tagged with the project name. -# Identical in all repos; the tag comes from script/projectname. +# Identical in all repos; the tag comes from script/projectname. The +# Dockerfile's checks only actually run because CHECK_EPOCH is a fresh +# nonce on every invocation; without it a warm cache turns this into a +# green that proves nothing. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" @@ -8,7 +11,13 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" - docker build -t "$("$SCRIPT_DIR/projectname")" . + # 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. + epoch="$(date +%s%N)$$" + docker build --build-arg CHECK_EPOCH="$epoch" \ + -t "$("$SCRIPT_DIR/projectname")" . } main "$@" -- 2.49.1 From d173e69f85f92ec675dc0afddad7ce6e1e021f85 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 15:23:50 +0000 Subject: [PATCH 2/6] Make the pinned golangci-lint actually reach the host (closes #28) REPO_POLICIES.md now carries the canonical script/bootstrap snippet for Go repos alongside the .golangci.yml bullet, where the pinned linter version already lives. The guard it replaces, `if missing golangci-lint; then go install ...; fi`, tests PATH presence and never version, so on any already-provisioned machine the pin is inert and a version bump is a no-op. The Dockerfile installs unconditionally into a clean image, so CI and local then disagree about what the linter is: a local `make check` green while `make docker` rejects the same commit, and a container run surfacing findings the host run cannot see. Comparing versions alone is not enough. `go install` writes to GOBIN (or GOPATH/bin) while callers resolve through PATH, so a shadowing binary earlier in PATH lets the install succeed and change nothing a caller ever sees, while bootstrap prints success. The canonical form therefore compares the installed version against the pin, re-resolves through PATH after installing and asserts the pin, and treats any unparseable --version output as a mismatch so the failure direction is a redundant install rather than a skipped one. The comparison is exact over the whole version token. 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, reachable through the comparison meant to close it, and not caught by requiring the pin to be tagged, since a pre-release is tagged too. When the assert fails the diagnosis is derived from the resolved path rather than asserted: a path outside the install directory is shadowing and the operator is told to remove it or reorder PATH; a path inside it is not, and saying so would send them after a fault that does not exist; no resolution at all means the install directory is simply absent from PATH. A trailing slash on GOBIN is normalised away, since it would otherwise make the inside-the- directory test miss and misreport shadowing. The snippet ends with a call site, and both success paths print a confirmation naming the version. Two definitions with no invocation are a silent no-op with exactly the shape this change exists to close, and a success path that prints nothing is byte-identical to that no-op: same exit status, same empty output. The version helper ends in `|| true` so a --version that exits non-zero cannot kill the script through `set -e` under `set -o pipefail` before the diagnostic is printed, which the styleguide's bash form would otherwise do. The policy text states each of those as a requirement rather than leaving them implicit in the code, and records why the commit-pinned `go install` ref satisfies the hash-pinning rule: a commit hash is not a mutable tag, and the checksum database verifies the fetch, with no repo go.sum consulted, since `go install pkg@version` ignores the go.mod in the current directory or any parent. Whether a go.mod tool dependency should replace that is an open decision and is linked rather than settled here. The two version strings must be kept in sync and must match exactly what --version prints; tagged pins are preferred because the expected string is then derivable from the ref, rather than because the comparison cannot handle a pseudo-version. Verification runs the controls against the block as a consuming repo would adopt it, pasted into a script/bootstrap-shaped file and executed, rather than sourcing it and calling the function directly. The node and yarn handling described earlier in the document is untouched. --- TODO.md | 8 ++ prompts/REPO_POLICIES.md | 189 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+) diff --git a/TODO.md b/TODO.md index 9e3b80b..8d9557d 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,14 @@ fmt-check, and commit. # Completed Steps +- 2026-08-09: Made the pinned golangci-lint actually propagate: REPO_POLICIES.md + now carries the canonical `script/bootstrap` snippet for Go repos, which + installs when the installed version does not match the pin (the old + `if missing` guard tested PATH presence only, so pins were inert on any + provisioned machine and CI silently disagreed with local) and then re-resolves + the binary through `PATH` and fails loudly, naming the shadowing path, when + the install did not take effect — the failure mode the naive + compare-then-install fix leaves behind while reporting success. - 2026-08-09: Fixed the false green in the canonical CI gate: `script/cibuild` and `script/docker` now pass a per-invocation `CHECK_EPOCH` nonce, and the `Dockerfile` (plus the Go multistage template in REPO_POLICIES.md, in both its diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index 82f6ae9..a893c8d 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -354,6 +354,195 @@ style conventions are in separate documents: commit-pinned via `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5`. +- **`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. + + 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. + - **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. + - **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 + 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. + + 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). + + **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. + + 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. + - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD). -- 2.49.1 From fd78aeb0035440264fefce5d935ca657c55027fe Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 16:10:45 +0000 Subject: [PATCH 3/6] Keep secrets out of the Docker build context at every depth (closes #29) The canonical .dockerignore was three lines -- .git, node_modules, .DS_Store -- while the canonical Dockerfile does `COPY . .`, so a developer's local .env, *.pem or *.key was shipped into the build context and could land in an image layer. Nothing surfaced it because .gitignore covers those patterns, so the files are invisible to every git-based check. The obvious repair, copying .gitignore's secret patterns across, is worse than the gap it closes. .dockerignore does not use .gitignore semantics: Docker matches with moby/patternmatcher, which is filepath.Match semantics plus a `**` extension compiled to a regexp. `*` does not cross `/`, and a pattern without a leading `**/` is anchored at the build-context root. A file listing .env, *.pem and *.key therefore reads as solved, reviews as solved, and protects only the repository root, while config/.env and certs/server.key still ship. The three-line file at least invited scrutiny; the transplanted form manufactures confidence and stops anyone looking. So every depth-independent pattern here carries the `**/` prefix and only genuinely root-anchored entries stay unprefixed. `**/node_modules` fixes a defect the three-line file had today for any nested node_modules, independently of the secret exposure. Coverage is not limited to the three patterns the issue names, because the enumeration found more shapes reaching the image. `**/*.env` covers the prod.env / local.env convention, which the .env and .env.* spellings miss entirely; `.envrc` is a secrets file by direnv convention; *.p12 and *.pfx are bundles carrying private keys; and id_rsa, id_dsa, id_ecdsa and id_ed25519 are the extensionless SSH keys that were covered before only when someone happened to append a .key suffix. Matching is case-sensitive, so `**/*.key` does not match certs/SERVER.KEY, which is reachable on the case-insensitive filesystems most laptops use. Doubling each pattern with an ALL-CAPS twin is not the fix: measured against the planted set it still ships certs/Server.Key and certs/Ca.Pem while reading as though case were handled, which is this issue's failure mode restated in a new place. The matcher supports character ranges, so every secret name is written that way -- `**/*.[kK][eE][yY]`, `**/*.[pP][eE][mM]`, `**/.[eE][nN][vV][rR][cC]`, `**/[iI][dD]_[rR][sS][aA]` and the rest. The extensionless SSH keys and .envrc are folded for the same reason the extensions are: on the very filesystems that make SERVER.KEY reachable, direnv reads .ENVRC and ssh reads ID_RSA, so exempting them would have contradicted the rule that justifies the folding. Since `*` matches the empty string, `**/*.[eE][nN][vV]` already covers a bare .ENV and no separate literal .env entry is needed; the literal one is gone rather than left to imply that case is unhandled there. Deliberately not covered: bare `key` and `pem` filenames, which no tool produces and which collide with legitimate paths (a `**/key` pattern would delete an internal/key/ package directory from the context); `**/id_*`, which would match ordinary source such as id_generator.go and ID_MAP.go; and *.crt and *.cer, which are public certificates rather than secrets and are sometimes a legitimate build input. Each of those is planted as a positive control and verified present in the image after the change. The one predictable false positive is a committed env template: `**/*.env` excludes example.env. The remedy travels with the file rather than living in a review thread -- the header comment and the policy both say to re-include it with a negation, `!docs/example.env`, and never to delete the pattern, which would reopen the exposure for everything else it covers. The OS and editor patterns are included on their own merits rather than by mirroring .gitignore. None of them is ever a build input, and editor state in particular churns under a developer's hands, so each one is a source of `COPY . .` invalidation carrying no information about the source tree. Now that the checks are keyed on CHECK_EPOCH rather than on accidental context churn, there is no reason left to keep churn in the context. Language build artifacts are deliberately absent: they are per-repo, and the file's header comment tells consuming repos to add their own host-built binaries, which is the case that actually bites -- a host `make build` drops a multi-megabyte artifact into the context where .gitignore hides it from every git-based check. That comment gives the anchored form explicitly, `/myapp` rather than `**/myapp`, because the prefixed spelling also matches cmd/myapp/ and deletes the package directory. The header comment is the only part of this guidance a consuming repo actually receives, since it is vendored with the file. .gitignore is untouched. Its semantics are the inverse: an unanchored pattern already matches at any depth, so `**/`-prefixing it produces a file that is wrong in a way that looks careful. That asymmetry is why "derive one from the other" was the wrong instruction, and it is now written down in REPO_POLICIES.md in both directions, together with the case-sensitivity rule, the negation remedy, and the requirement to verify by enumerating the image rather than by reading the patterns. Every consuming repo inherits .dockerignore by copy, so the trap has to live where the next person looks, not only be fixed once here. Both repo checklists gain the same requirements. Verified by planting 130 secret files -- twenty-six name shapes, including every capitalisation of .env, .env.*, prod.env, .envrc, id_rsa, id_ed25519, ca.pem, server.key, bundle.p12 and bundle.pfx, at five depths from the context root to a/b/c -- alongside nine positive controls, then building a standalone probe image doing `COPY . .` and listing what actually landed inside it. The patterns as first written leaked 35 of the 130: every .ENVRC, .Envrc, ID_RSA, Id_Rsa, ID_ED25519, .ENV.PRODUCTION and .Env.Local, at all five depths. A lowercase-only control leaks 80, so the probe is not vacuous. After this change: zero, with all nine controls still present, including internal/ID_MAP.go and certs/CA.CRT. Transferred-context size is recorded but load-bearing on nothing: an earlier run reported 2.18kB transferred while 43 files, five of them secrets, were in the image. BuildKit transfers only the delta from the previous build, so the number describes the transfer and not the contents. Planted files were removed and their absence confirmed against the filesystem rather than against `git status`, which could not have seen them. `make docker` re-run after the change: the check layer executed rather than being served from cache, so the CHECK_EPOCH verification still holds under the altered build context. --- .dockerignore | 71 +++++++++++++++++++++++++++++- TODO.md | 15 +++++++ prompts/EXISTING_REPO_CHECKLIST.md | 12 +++++ prompts/NEW_REPO_CHECKLIST.md | 8 ++++ prompts/REPO_POLICIES.md | 59 ++++++++++++++++++++++++- 5 files changed, 162 insertions(+), 3 deletions(-) diff --git a/.dockerignore b/.dockerignore index 5414d56..d948f00 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,3 +1,70 @@ +# Docker matches this file with moby/patternmatcher: Go filepath.Match +# semantics plus a `**` extension, compiled to a regexp. Plain +# filepath.Match has no `**` at all. What follows from that: `*` does not +# cross `/`, and a pattern without a leading `**/` is anchored at the +# build-context root. Every depth-independent pattern therefore needs the +# `**/` prefix — without it `config/.env` and `certs/server.key` still +# ship while the file reads as solved. +# +# Root-anchored entries are for paths that occur exactly once, at the +# context root. A host-built binary is the usual case, and it must be +# written anchored: `/myapp`, never `**/myapp`. The prefixed form also +# matches `cmd/myapp/`, which deletes the package directory from the +# context. +# +# Matching is case-sensitive, so `**/*.key` does not match +# `certs/SERVER.KEY`, which is reachable on the case-insensitive +# filesystems most laptops use. Adding an ALL-CAPS twin per pattern is +# not the fix: it still misses `Server.Key` while reading as though case +# were handled. Character ranges cover every spelling in one line, so +# every secret name below is written that way — including the +# extensionless SSH keys and `.envrc`, because on those same +# case-insensitive filesystems direnv reads `.ENVRC` and ssh reads +# `ID_RSA`. +# +# `**/*.[eE][nN][vV]` also excludes a committed env template such as +# `example.env`. If the build genuinely needs one, re-include it with a +# negation after the pattern: `!docs/example.env`. +# +# Extend this file with the repo's own host-built artifacts (compiled +# binaries, test binaries, coverage output); those are per-repo and +# belong here because a host build otherwise drops them into the +# context. + +# Repository metadata: exactly one, at the context root. .git -node_modules -.DS_Store + +# Environment files. `*.env` covers both the bare `.env` name (`*` matches +# the empty string) and the `prod.env` convention. +**/*.[eE][nN][vV] +**/.[eE][nN][vV].* +**/.[eE][nN][vV][rR][cC] + +# Private keys and the bundles that carry them. Public certificates +# (*.crt, *.cer) are deliberately absent: they are not secrets and are +# sometimes a legitimate build input. +**/*.[pP][eE][mM] +**/*.[kK][eE][yY] +**/*.[pP]12 +**/*.[pP][fF][xX] +**/[iI][dD]_[rR][sS][aA] +**/[iI][dD]_[dD][sS][aA] +**/[iI][dD]_[eE][cC][dD][sS][aA] +**/[iI][dD]_[eE][dD]25519 + +# Dependencies: restored inside the image, never copied in. +**/node_modules + +# OS metadata. +**/.DS_Store +**/Thumbs.db + +# Editor state. Never a build input, and it churns under a developer's +# hands, so it invalidates COPY for reasons unrelated to the source. +**/*.swp +**/*.swo +**/*~ +**/*.bak +**/.idea +**/.vscode +**/*.sublime-* diff --git a/TODO.md b/TODO.md index 8d9557d..2826015 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,21 @@ fmt-check, and commit. # Completed Steps +- 2026-08-09: Closed the secret exposure in the canonical `.dockerignore`: a + developer's local `.env`, `*.pem` or `*.key` was reaching the Docker build + context under `COPY . .`, invisible to every git-based check because + `.gitignore` covers it. The patterns are written to `.dockerignore`'s own + `moby/patternmatcher` semantics — `**/`-prefixed so they hold at every depth, + which also fixes nested `node_modules` — rather than transplanted from + `.gitignore`, whose unprefixed form protects only the repository root while + reading as solved. Coverage extends past the `.env`/`.pem`/`.key` trio to the + `prod.env` convention, `.envrc`, PKCS#12 bundles and extensionless SSH keys, + every one of them case-folded with character ranges because matching is + case-sensitive and an ALL-CAPS twin per pattern still misses `Server.Key`. + `REPO_POLICIES.md` and both repo checklists now state that asymmetry and + require verification by enumerating the image rather than by reading the + patterns. Verified with a probe image before, against three naive forms + (unprefixed, lowercase-only, ALL-CAPS-doubled), and after. - 2026-08-09: Made the pinned golangci-lint actually propagate: REPO_POLICIES.md now carries the canonical `script/bootstrap` snippet for Go repos, which installs when the installed version does not match the pin (the old diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 69cb02b..c32a497 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -37,6 +37,18 @@ with your task. `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. +- [ ] `.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 + existing repo is where such a binary is likeliest to already be sitting in + the build context, invisible to git. +- [ ] Every depth-independent pattern in `.dockerignore` carries a `**/` prefix; + only genuinely root-anchored entries such as `.git` are unprefixed, and + `.gitignore`'s patterns have not been transplanted unmodified. + `.dockerignore` anchors an unprefixed pattern at the context root, so the + transplanted form leaves `config/.env` and `certs/server.key` in the build + context while reading as solved — see the `.dockerignore` rule in + `REPO_POLICIES.md`. - [ ] Gitea Actions workflow in `.gitea/workflows/` runs `script/cibuild` on push — reference `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitea/workflows/check.yml` diff --git a/prompts/NEW_REPO_CHECKLIST.md b/prompts/NEW_REPO_CHECKLIST.md index 5b0bcfe..7965c10 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -52,6 +52,14 @@ Template files can be fetched from: `https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md` - [ ] `Dockerfile` and `.dockerignore` — fetch `.dockerignore` from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore` + - Extend `.dockerignore` with the repo's own host-built artifacts, giving + every depth-independent pattern a `**/` prefix — but write a repo-root + binary anchored, `/myapp` and never `**/myapp`, which would also match + `cmd/myapp/` and delete the package directory. Do not transplant + `.gitignore`'s patterns: `.dockerignore` anchors an unprefixed pattern at + the context root, so the copied form leaves `config/.env` in the build + context while reading as solved. See the `.dockerignore` rule in + `REPO_POLICIES.md`. - 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 diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index a893c8d..edc7826 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -331,7 +331,64 @@ style conventions are in separate documents: editor files (`.swp`, `*~`), language build artifacts, and `node_modules/`. Fetch the standard `.gitignore` from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when setting up - a new repo. + a new repo. These patterns are written to `.gitignore`'s own semantics, in + which an unanchored pattern already matches at every depth. They are not a + `.dockerignore` and must not be transplanted into one unmodified — see the + next rule. + +- **`.dockerignore` does not use `.gitignore` semantics, and copying patterns + across unmodified leaves secrets in the build context.** Docker matches with + `moby/patternmatcher`: Go `filepath.Match` semantics plus a `**` extension, + compiled to a regexp — plain `filepath.Match` has no `**` at all. So `*` does + not cross `/`, and a pattern without a leading `**/` is anchored at the + build-context root. A `.dockerignore` listing `.env`, `*.pem` and `*.key` + therefore excludes only the copies at the repository root; `config/.env` and + `certs/server.key` still reach the context and can land in an image layer. + That file is more dangerous than a short one with no secret patterns at all, + because it reads as solved and stops anyone looking. Give every + depth-independent pattern the `**/` prefix — `**/node_modules`, + `**/.DS_Store`, and the secret patterns in the canonical file, which are + additionally case-folded per the rule below — and leave only genuinely + root-anchored entries such as `.git` unprefixed. The inverse move is equally + wrong: never apply `**/` to `.gitignore`, where it is redundant and produces a + file that is wrong in a way that looks careful. Each file is written to its + own semantics; neither is derived from the other. Fetch the standard + `.dockerignore` from + `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore` and extend + it with the repo's own host-built artifacts — a host `make build` that leaves + a compiled binary in the repo root puts that binary in the build context, + where `.gitignore` hides it from every git-based check. Write that binary + anchored, `/myapp` and never `**/myapp`: the prefixed form also matches + `cmd/myapp/` and deletes the package directory from the context. + +- **`.dockerignore` matching is case-sensitive, so cover capitalisation with + character classes rather than by doubling patterns.** `**/*.key` does not + match `certs/SERVER.KEY`, which is reachable on the case-insensitive + filesystems most laptops use. Adding an ALL-CAPS twin for each pattern is not + the fix: it still misses `Server.Key` and `Ca.Pem` while reading as though + case were handled — the same manufactured confidence as the root-anchored + form. The matcher supports character ranges, so one line covers every + spelling: `**/*.[kK][eE][yY]`, `**/*.[pP][eE][mM]`. Apply this to every secret + name, not only to extensions: the extensionless SSH keys and `.envrc` need it + for the same reason, since on the very filesystems that make `SERVER.KEY` + reachable, direnv reads `.ENVRC` and ssh reads `ID_RSA`. Note that `*` matches + the empty string, so `**/*.[eE][nN][vV]` already covers a bare `.ENV` and no + separate literal `.env` entry is needed. + +- **A pattern that also catches something the build needs is re-included with a + negation, not deleted.** The canonical `**/*.[eE][nN][vV]` excludes a + committed env template such as `example.env`; a repo whose build genuinely + reads one adds `!docs/example.env` after the pattern. Deleting the pattern + instead reopens the exposure for every other file it covers. + +- **Verify `.dockerignore` by enumerating the image, not by reading the + patterns.** Plant files at the root _and_ at least two directories deep, build + a probe image that does `COPY . .`, and list what actually landed + (`docker run --rm --entrypoint find IMAGE /app`). Reading the patterns and + agreeing they look right is exactly what lets the root-only form through. The + `transferring context` size is not a substitute: a nested secret is a few + bytes, and BuildKit transfers only the delta from the previous build, so the + reported size describes the transfer and not the contents of the image. - **No build artifacts in version control.** Code-derived data (compiled bundles, minified output, generated assets) must never be committed to the -- 2.49.1 From 3a218497b8a025c3c815dbf88a1e57273e526bb1 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 16:56:28 +0000 Subject: [PATCH 4/6] Keep in-repo agent scratch out of the build context and out of git (closes #27) The canonical .dockerignore and .gitignore both omitted the in-repo agent scratch directory. On this fleet that directory holds one worktree per in-flight agent -- an entire additional checkout of the repo each -- so under `COPY . .` all of it reached the build context and the image. Measured on this repo before the change: five planted scratch files, at every depth beneath the directory, all present inside a probe image built from the real context. Three consequences, only the first of which is about size. The context inflates by a multiple of the repo. Another session's unreviewed and sometimes uncommitted work is copied into a build artifact. And the directory is created and destroyed constantly by tooling, so it invalidates `COPY . .` for reasons that have nothing to do with this repo's content -- which is the accidental cache protection described at length in the issue thread, and the reason this change was sequenced behind the CHECK_EPOCH bust rather than landed alongside the rest of the .dockerignore work. The two entries are deliberately different shapes, because the two files have different semantics and neither is derived from the other. In .dockerignore the entry is anchored, `.claude`, with no `**/` prefix: the directory occurs exactly once, at the context root, and the prefixed form additionally matches any nested directory of that name. Measured rather than argued -- the `**/`-prefixed control was built and enumerated too, and it removes prompts/.claude/ from the context as well, which in a repo with a legitimately named nested directory would silently delete it from the build. In .gitignore the entry is unanchored, `.claude/`, because a .gitignore pattern already matches at every depth; `git check-ignore -v` confirms it covering both .claude/ and prompts/.claude/, so a `**/` prefix there would be redundant at best, and on an anchored pattern it would be actively wrong. Anchoring buys that at the cost of a residual exposure, and the vendored files now say so rather than only asserting the reason to anchor. "Occurs exactly once, at the context root" is a property of how agents are run, not of the tooling: the directory is created in the agent's working directory, so a monorepo running a per-service agent in services/api/ still ships services/api/.claude/ into the context and the image -- the exact exposure this change exists to close, left open in the repo shape where it is likeliest. The .dockerignore header block, the REPO_POLICIES.md bullet and both checklists state the gap and the remedy (anchored entries for the subdirectories that have one, or `**/.claude` once no legitimately named nested directory would be caught). Consuming repos receive the files and not the tracker, so a caveat that lives only in a PR body is not a caveat. It is not case-folded the way the neighbouring secret patterns are: tooling creates the directory in exactly one spelling, so a folded pattern would add no coverage. The earlier justification -- that a miss costs bloat rather than exposure -- is gone, because it contradicted this issue's own framing, in which the cost of a miss is unreviewed work in an image layer. The second half of this change is the consequence that ships broken silently. Excluding .git means `git describe` cannot run in any build stage, and it fails quietly there rather than erroring: `-X main.Version=` comes out empty, the binary reports no version, and the build still exits 0. The Go template in REPO_POLICIES.md had `ARG VERSION=dev` and never said where VERSION came from, which is precisely the gap a reader fills in with `git describe` inside the build. It now says: computed on the host, threaded in with `--build-arg VERSION=...`, shown as a complete command rather than as two rules each documenting half of one. script/docker and script/cibuild do it, with the same discipline the epoch already has -- assignment on its own line, because a failing command substitution inside an argument does not trip `set -e`, plus a non-empty fallback so a build from an export with no .git reports `unknown` rather than an empty string that reads as a successful version. That fallback is applied in exactly one place, and that place can actually execute. `git describe ... || true` leaves the value empty when it fails, and the `[ -n "$version" ]` line is what substitutes `unknown`. Folding the fallback into the substitution as `|| echo unknown` would have left the guard unreachable -- harmless in itself, but a guard that cannot fire is indistinguishable from one that works to every repo that copies it, and canonical text should not carry a check that is decorative. Measured firing in both sh and dash: not a git repository -> unknown, repository with no commits yet -> unknown, this repository -> the describe output. The checklist now carries the guard line as well, so the vendored guidance and the vendored script no longer disagree. The scripts pass VERSION unconditionally rather than growing a per-repo variant. This repo's Dockerfile declares no `ARG VERSION`, and BuildKit was measured accepting the unconsumed arg silently -- no warning, no cache effect, confirmed by the paired runs below in which the bootstrap layer still caches. The alternative, leaving it to each repo, reintroduces the trap: a repo that needs a version and finds no VERSION in its scripts writes `git describe` into the Dockerfile, which is the failure being closed. The same correction reaches the two Go documents that carry the GOLDFLAGS pattern, since a `$(shell git describe)` evaluated inside a build stage is exactly this empty version. Both are now `?=`, and their comments say precisely when that matters: where a build stage compiles by invoking make, `ARG VERSION` puts the value in the environment and `?=` defers to it, whereas the canonical Go template compiles with `go build` directly and uses the Makefile on the host only -- still `?=`, so that a repo which later moves its build behind make does not silently start shipping an empty version. Leaving them as `:=` would have left the corpus telling a reader one thing in the policy and the opposite in the styleguide. Both repo checklists gain the entries too. They are what an agent reads while writing these files, so they are where the wrong shape actually gets written: the .gitignore item is the one an existing repo never re-fetches, and it now names `.claude/` explicitly along with the warning not to prefix it. Verification, by enumerating a probe image rather than by reading the patterns. Standalone minimal Dockerfile held outside the context, `--no-cache` scoped to that one image, no prune of any kind. Before: all five planted scratch files in the image, 42 files total. After: zero, 37 files total, with README.md, script/check, prompts/NEW_REPO_CHECKLIST.md and a planted probe_src/app.md all still present as positive controls, so the exclusion is a real exclusion and not a COPY that stopped copying. Transferred context fell from 161.86kB to 68.82kB, recorded as corroboration only: BuildKit reports a delta, not a total, and an earlier run in this repo transferred 2.18kB while shipping 43 files. The CHECK_EPOCH verification was re-run under the changed context, because the context moved underneath the earlier measurement. Two consecutive script/cibuild runs on an unchanged tree: run 1 in 17.07s, run 2 in 5.73s, both executing the check layer with a distinct epoch and real prettier output from both lint and fmt-check. The `RUN script/bootstrap` layer is CACHED in run 2, which is the validity control -- it proves no concurrent prune landed between the runs and that no --no-cache path was taken, so the check layer executing is the bust working rather than a cold cache. Planted files were removed afterwards and their absence confirmed against the filesystem with `find`, not against `git status`, which cannot see them once .gitignore covers the directory -- the same blind spot that made the earlier secret exposure invisible. --- .dockerignore | 31 +++++- .gitignore | 6 ++ TODO.md | 26 +++++ prompts/CODE_STYLEGUIDE_GO.md | 15 ++- prompts/EXISTING_REPO_CHECKLIST.md | 31 +++++- prompts/GO_HTTP_SERVER_CONVENTIONS.md | 11 +- prompts/NEW_REPO_CHECKLIST.md | 49 +++++++-- prompts/REPO_POLICIES.md | 147 +++++++++++++++++++++----- script/cibuild | 15 ++- script/docker | 14 ++- 10 files changed, 297 insertions(+), 48 deletions(-) diff --git a/.dockerignore b/.dockerignore index d948f00..84c3383 100644 --- a/.dockerignore +++ b/.dockerignore @@ -10,7 +10,10 @@ # context root. A host-built binary is the usual case, and it must be # written anchored: `/myapp`, never `**/myapp`. The prefixed form also # matches `cmd/myapp/`, which deletes the package directory from the -# context. +# context. In-repo agent scratch is the other case, for the same reason +# — with the caveat recorded at that entry: anchoring is exact only +# where agents run at the repo root, and a repo where they do not must +# add its own entries. # # Matching is case-sensitive, so `**/*.key` does not match # `certs/SERVER.KEY`, which is reachable on the case-insensitive @@ -31,9 +34,33 @@ # belong here because a host build otherwise drops them into the # context. -# Repository metadata: exactly one, at the context root. +# Repository metadata: exactly one, at the context root. Excluding it +# means `git describe` cannot run in any build stage, and it fails +# quietly there rather than erroring, so a version embedded that way +# comes out empty. Compute the version on the host and pass it in with +# `--build-arg VERSION=...`; see the version rule in REPO_POLICIES.md. .git +# In-repo agent scratch: a directory holding a full additional checkout +# of the repo for each in-flight agent. Anchored because it occurs +# exactly once *where agents run at the repo root*, which is the +# convention this file assumes; the `**/` form would also match any +# nested directory of that name and delete it from the build. +# +# KNOWN GAP, and it is not hypothetical: the directory is created in the +# agent's working directory. If agents in this repo run in +# subdirectories — a monorepo with a per-service agent, say — then +# `services/api/.claude/` is NOT excluded by the line below and still +# reaches the build context and the image, which is the exposure this +# entry exists to close. A repo in that shape adds its own anchored +# entries (`/services/api/.claude`), or `**/.claude` after confirming no +# legitimately named nested directory would be caught. +# +# Not case-folded, unlike the secret patterns below: tooling creates +# this directory in exactly one spelling, so a folded pattern would add +# no coverage. +.claude + # Environment files. `*.env` covers both the bare `.env` name (`*` matches # the empty string) and the `prod.env` convention. **/*.[eE][nN][vV] diff --git a/.gitignore b/.gitignore index c5a0882..3558b76 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,12 @@ Thumbs.db .vscode/ *.sublime-* +# Agent scratch (worktrees of this repo, created and destroyed by +# in-flight tooling). Unanchored: .gitignore patterns already match at +# every depth, so no prefix is wanted here. This is not a .dockerignore +# entry and must not be given a `**/` prefix on the way into one. +.claude/ + # Node node_modules/ diff --git a/TODO.md b/TODO.md index 2826015..c598ba2 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,32 @@ fmt-check, and commit. # Completed Steps +- 2026-08-09: Kept in-repo agent scratch out of the Docker build context and out + of version control. `.claude/` holds one worktree — an entire additional + checkout of the repo — per in-flight agent, and under `COPY . .` all of it was + reaching the image: another session's unreviewed, sometimes uncommitted work, + inflating the context by a multiple of the repo and invalidating `COPY` for + reasons unrelated to the repo's own content. The `.dockerignore` entry is + root-anchored, because the directory occurs exactly once where agents run at + the repo root and the `**/` form additionally deletes any nested directory of + that name — with the residual gap that follows from anchoring (a monorepo + running agents in subdirectories still ships `services/api/.claude/`) stated + in the canonical `.dockerignore`, the policy and the existing-repo checklist, + since consuming repos receive the files rather than the tracker; the + `.gitignore` entry is unanchored, because `.gitignore` patterns already match + at every depth, and each file is written to its own semantics rather than + derived from the other. Also closed the consequence that ships broken + silently: excluding `.git` means `git describe` cannot run in any build stage + and yields an empty version without erroring, so `script/docker` and + `script/cibuild` now compute the version on the host and pass + `--build-arg VERSION`, and `REPO_POLICIES.md` states where `VERSION` comes + from instead of leaving the reader to fill the gap with `git describe` inside + the build. The two Go documents that carry the `GOLDFLAGS` pattern were + corrected in the same pass, from `:=` to `?=`, since a `$(shell git describe)` + evaluated inside a build stage is exactly the empty version this closes. + Verified by enumerating a probe image before, after, and against the + `**/`-prefixed form, with a positive control and the `CHECK_EPOCH` cache + verification re-run under the changed build context. - 2026-08-09: Closed the secret exposure in the canonical `.dockerignore`: a developer's local `.env`, `*.pem` or `*.key` was reaching the Docker build context under `COPY . .`, invisible to every git-based check because diff --git a/prompts/CODE_STYLEGUIDE_GO.md b/prompts/CODE_STYLEGUIDE_GO.md index d81ffff..be44685 100644 --- a/prompts/CODE_STYLEGUIDE_GO.md +++ b/prompts/CODE_STYLEGUIDE_GO.md @@ -49,7 +49,20 @@ last_modified: 2026-08-09 ``` ```make - VERSION := $(shell git describe --always --dirty) + # ?= rather than := because this `$(shell git describe ...)` is only + # correct on the host. `.dockerignore` excludes `.git`, so evaluated + # inside a build stage it expands to the empty string without failing + # and the binary reports no version at all. The version is computed on + # the host by `script/docker` / `script/cibuild` and passed with + # `--build-arg VERSION=...`. If this repo's Dockerfile compiles by + # invoking make (`RUN make build`), `ARG VERSION` in that stage puts the + # value in the environment and `?=` defers to it. The canonical Go + # template in REPO_POLICIES.md instead runs `go build` directly with + # `-ldflags "... -X main.Version=${VERSION}"`, so there this Makefile is + # a host-only path — but it is still `?=`, because a repo that later + # moves the build behind make must not silently start shipping an empty + # version. See the git-describe rule in REPO_POLICIES.md. + VERSION ?= $(shell git describe --always --dirty) BUILDARCH := $(shell uname -m) GOLDFLAGS += -X main.Version=$(VERSION) diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index c32a497..83421f9 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -24,9 +24,14 @@ with your task. - [ ] `LICENSE` file exists and matches the README - [ ] `REPO_POLICIES.md` exists and version date is current — fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/prompts/REPO_POLICIES.md` -- [ ] `.gitignore` is comprehensive (OS, editor, language artifacts, secrets) — - fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` - if missing +- [ ] `.gitignore` is comprehensive (OS, editor, agent scratch, language + artifacts, secrets) — fetch from + `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` if missing. + An existing repo usually has a hand-written one that is never re-fetched, + so check the entries rather than the file's presence: `.claude/` in + particular, unanchored, so agent worktrees cannot be committed by + accident. Do not give it a `**/` prefix — that is a `.dockerignore` form + and is wrong here. - [ ] `.editorconfig` exists — fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.editorconfig` - [ ] `Dockerfile` and `.dockerignore` exist (fetch `.dockerignore` from @@ -42,6 +47,26 @@ with your task. `/myapp`, never `**/myapp`, which would also match `cmd/myapp/`. An existing repo is where such a binary is likeliest to already be sitting in the build context, invisible to git. +- [ ] `.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. +- [ ] **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 + a per-service agent — it still reaches the build context and the image. An + existing repo is where such a layout already exists, so check it here + rather than assuming the canonical entry covers you: add anchored entries + for the subdirectories that have one (`/services/api/.claude`), or + `**/.claude` once you have confirmed no legitimately named nested + directory would be caught. +- [ ] If the repo embeds a version in a binary, that version is computed on the + host and passed with `--build-arg VERSION=...` by `script/docker` and + `script/cibuild`. No stage calls `git describe`: `.dockerignore` excludes + `.git`, so it yields an empty version without failing the build. A + tag-derived version additionally needs `fetch-depth: 0` on the CI checkout + step, which clones shallow and fetches no tags by default. - [ ] Every depth-independent pattern in `.dockerignore` carries a `**/` prefix; only genuinely root-anchored entries such as `.git` are unprefixed, and `.gitignore`'s patterns have not been transplanted unmodified. diff --git a/prompts/GO_HTTP_SERVER_CONVENTIONS.md b/prompts/GO_HTTP_SERVER_CONVENTIONS.md index 3a53b3c..06d44f0 100644 --- a/prompts/GO_HTTP_SERVER_CONVENTIONS.md +++ b/prompts/GO_HTTP_SERVER_CONVENTIONS.md @@ -1,6 +1,6 @@ --- title: Go HTTP Server Conventions -last_modified: 2026-02-22 +last_modified: 2026-08-09 --- This document defines the architectural patterns, design decisions, and @@ -991,7 +991,14 @@ func main() { Use ldflags to inject version information at build time: ```makefile -VERSION := $(shell git describe --tags --always) +# ?= rather than := because this `$(shell git describe ...)` is only correct +# on the host: `.dockerignore` excludes `.git`, so evaluated inside a build +# stage it expands to the empty string without failing and the binary reports +# no version. The version is computed on the host by `script/docker` / +# `script/cibuild` and passed with `--build-arg VERSION=...`; where the build +# stage invokes make, `ARG VERSION` puts it in the environment and `?=` defers +# to it. See the git-describe rule in REPO_POLICIES.md. +VERSION ?= $(shell git describe --tags --always) BUILDARCH := $(shell go env GOARCH) build: diff --git a/prompts/NEW_REPO_CHECKLIST.md b/prompts/NEW_REPO_CHECKLIST.md index 7965c10..bd6d7f0 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -35,7 +35,11 @@ Template files can be fetched from: - [ ] `.gitignore` — fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore`, extend for - language-specific artifacts + language-specific artifacts. Extensions are written to `.gitignore`'s own + semantics, where an unanchored pattern already matches at every depth: + never add a `**/` prefix here, which is a `.dockerignore` form. The + canonical file already carries `.claude/` so agent worktrees cannot be + committed by accident. - [ ] `.editorconfig` — fetch from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.editorconfig` - [ ] `Makefile` — fetch from @@ -59,7 +63,16 @@ Template files can be fetched from: `.gitignore`'s patterns: `.dockerignore` anchors an unprefixed pattern at the context root, so the copied form leaves `config/.env` in the build context while reading as solved. See the `.dockerignore` rule in - `REPO_POLICIES.md`. + `REPO_POLICIES.md`. The canonical file's `.claude` entry is anchored for + the same reason as a repo-root binary; leave it that way, but note it only + covers agents running at the repo root — if this repo will run them in + subdirectories, `services/api/.claude/` is not excluded and needs its own + anchored entry. + - If the image embeds a version in a binary, the version is computed on the + host and passed with `--build-arg VERSION=...`. `ARG VERSION=dev` is + 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 @@ -103,14 +116,30 @@ are thin shims calling them. Model scripts: - [ ] `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); assigns - `epoch="$(date +%s%N)$$"` on its own line and passes - `--build-arg CHECK_EPOCH="$epoch"` -- [ ] `script/cibuild` — cd to repo root, assign `epoch="$(date +%s%N)$$"` on - its own line, then run `docker build --build-arg CHECK_EPOCH="$epoch" .` - (what CI runs). The build arg is mandatory: see the `CHECK_EPOCH` rule in - `REPO_POLICIES.md` for why each element is load-bearing. A bare - `docker build .` fails closed by design. + `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: + + ```sh + epoch="$(date +%s%N)$$" + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + --build-arg VERSION="$version" \ + . + ``` + + (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. + - [ ] `script/precommit` — called by the pre-commit hook; runs `script/check` - [ ] `script/install-precommit` — installs the pre-commit hook that runs `script/precommit` diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index edc7826..19304f1 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -60,19 +60,21 @@ 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 `docker build --build-arg CHECK_EPOCH="$epoch" .`, where - `epoch` is a per-invocation nonce (see the `CHECK_EPOCH` rule below); the - Gitea workflow calls it. Four further scripts are our own extensions to the - standard: `script/check` runs `script/test`, `script/lint`, and - `script/fmt-check`; `script/precommit` is what the git pre-commit hook runs, - and it calls `script/check`; `script/install-precommit` installs the git - pre-commit hook (the `make hooks` target shims to it); and - `script/projectname` (literally that filename) simply outputs the project's - name. Scripts that need the name call `script/projectname` — e.g. - `script/docker` assembles its image tag from it — so those scripts stay - byte-identical across all repos. Repo-type-specific pre-commit extras (e.g. - `go mod tidy` verification in Go repos) belong in `script/precommit`, not in - the hook itself. Model scripts are at + repo root and 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 + (see the git-describe rule below); the Gitea workflow calls it. Four further + scripts are our own extensions to the standard: `script/check` runs + `script/test`, `script/lint`, and `script/fmt-check`; `script/precommit` is + what the git pre-commit hook runs, and it calls `script/check`; + `script/install-precommit` installs the git pre-commit hook (the `make hooks` + target shims to it); and `script/projectname` (literally that filename) simply + outputs the project's name. Scripts that need the name call + `script/projectname` — e.g. `script/docker` assembles its image tag from it — + so those scripts stay byte-identical across all repos. Repo-type-specific + pre-commit extras (e.g. `go mod tidy` verification in Go repos) belong in + `script/precommit`, not in the hook itself. Model scripts are at `https://git.eeqj.de/sneak/prompts/raw/branch/main/script/`. The README must document the provided scripts in an **Entrypoints** section (see the README requirements below). @@ -122,11 +124,18 @@ style conventions are in separate documents: ```sh epoch="$(date +%s%N)$$" - docker build --build-arg CHECK_EPOCH="$epoch" . + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + --build-arg VERSION="$version" \ + . ``` - All four elements are load-bearing; none is optional, and each guards a - failure mode that otherwise fails green: + 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: - `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`. @@ -197,6 +206,9 @@ style conventions are in separate documents: RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && 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 + # it yields an empty version without failing the build. ARG VERSION=dev RUN CGO_ENABLED=0 go build -trimpath \ -ldflags="-s -w -X main.Version=${VERSION}" \ @@ -247,18 +259,24 @@ style conventions are in separate documents: 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. + - `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 + build, not a source of truth. **No stage may call `git describe`**: + `.dockerignore` excludes `.git`, so it yields an empty version without + 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" .`) 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. + `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. - Use platform-standard formatters: `black` for Python, `prettier` for JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with @@ -328,8 +346,10 @@ style conventions are in separate documents: must be in `.gitignore`. No exceptions. - `.gitignore` should be comprehensive from the start: OS files (`.DS_Store`), - editor files (`.swp`, `*~`), language build artifacts, and `node_modules/`. - Fetch the standard `.gitignore` from + editor files (`.swp`, `*~`), in-repo agent scratch directories (`.claude/`, + which holds one worktree — an entire additional checkout of the repo — per + in-flight agent), language build artifacts, and `node_modules/`. Fetch the + standard `.gitignore` from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when setting up a new repo. These patterns are written to `.gitignore`'s own semantics, in which an unanchored pattern already matches at every depth. They are not a @@ -349,7 +369,8 @@ style conventions are in separate documents: depth-independent pattern the `**/` prefix — `**/node_modules`, `**/.DS_Store`, and the secret patterns in the canonical file, which are additionally case-folded per the rule below — and leave only genuinely - root-anchored entries such as `.git` unprefixed. The inverse move is equally + root-anchored entries unprefixed: `.git`, the in-repo agent scratch directory + `.claude`, and the repo's own host-built binary. The inverse move is equally wrong: never apply `**/` to `.gitignore`, where it is redundant and produces a file that is wrong in a way that looks careful. Each file is written to its own semantics; neither is derived from the other. Fetch the standard @@ -361,6 +382,33 @@ style conventions are in separate documents: anchored, `/myapp` and never `**/myapp`: the prefixed form also matches `cmd/myapp/` and deletes the package directory from the context. +- **In-repo agent scratch belongs in both files, written to each file's own + semantics.** `.claude/` holds one worktree per in-flight agent — an entire + additional checkout of the repo — so with `COPY . .` the build context + 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. + + **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 + a property of how agents are run and not of the tooling. Where agents run in + subdirectories — a monorepo with a per-service agent is the ordinary case — + `services/api/.claude/` is **not** excluded by the canonical entry and still + reaches the build context and the image, which is the exposure the entry + exists to close. A repo in that shape adds its own anchored entries + (`/services/api/.claude`), or `**/.claude` once it has confirmed no + legitimately named nested directory would be caught. This is stated in the + canonical `.dockerignore` itself, since that file is what consuming repos + receive. + - **`.dockerignore` matching is case-sensitive, so cover capitalisation with character classes rather than by doubling patterns.** `**/*.key` does not match `certs/SERVER.KEY`, which is reachable on the case-insensitive @@ -390,6 +438,49 @@ style conventions are in separate documents: bytes, and BuildKit transfers only the delta from the previous build, so the reported size describes the transfer and not the contents of the image. +- **Excluding `.git` means `git describe` cannot run inside any build stage, and + it fails quietly there.** The `GOLDFLAGS` version-embedding pattern assumes + `.git` is present; in a build stage there is no repository, so `git describe` + writes nothing to stdout and the `-X main.Version=` value comes out **empty** + rather than erroring. The binary then reports no version at all and the build + still exits 0. Compute the version **on the host** and thread it in as a build + arg. `script/docker` and `script/cibuild` do this, byte-identically across + repos: + + ```sh + # Assign on its own line: a failing command substitution inside an + # argument does not trip `set -e`, so the inline form degrades to an + # empty constant — the same silent-empty failure this rule is about. + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + --build-arg VERSION="$version" \ + . + ``` + + `--always` makes an untagged repo yield the abbreviated commit hash instead + of failing. `|| true` keeps a failing `git describe` from tripping `set -e` + and leaves the value empty, so the `[ -n "$version" ]` line is the single + place the fallback is applied — and it is a **live** check, not defence in + depth: it fires on a build from an export with no `.git`, and on a + repository with no commits yet. Do not fold the fallback into the + substitution as `|| echo unknown`; that makes the guard unreachable, and a + guard that cannot fire is indistinguishable from one that works to everyone + who copies it. The result is non-empty by construction either way, which is + the point: an empty version reads as a successful one, while `unknown` is + visibly wrong. The Dockerfile's side is `ARG VERSION=dev` in the stage that + compiles, declared there and not inherited, because `ARG` is stage-scoped + exactly as `CHECK_EPOCH` is. Passing `VERSION` to a repo whose Dockerfile + declares no such `ARG` is silently ignored by BuildKit and costs nothing, + which is why the scripts stay byte-identical rather than growing a per-repo + variant. + + One consequence for CI: the standard checkout action clones shallow and + fetches no tags, so `git describe --tags` there falls back to a bare commit + hash. A repo that embeds a tag-derived version must set `fetch-depth: 0` on + its checkout step; a repo that does not embed a version needs no change. + - **No build artifacts in version control.** Code-derived data (compiled bundles, minified output, generated assets) must never be committed to the repository if it can be avoided. The build process (e.g. Dockerfile, Makefile) diff --git a/script/cibuild b/script/cibuild index 786533a..4fbd751 100755 --- a/script/cibuild +++ b/script/cibuild @@ -14,7 +14,20 @@ main() { # nonce to an empty constant. `$$` is required because busybox `date` # drops %N without erroring. epoch="$(date +%s%N)$$" - docker build --build-arg CHECK_EPOCH="$epoch" . + # VERSION must be computed here, on the host: .dockerignore excludes + # .git, so `git describe` cannot run in any build stage and fails + # quietly there rather than erroring. Same own-line discipline as the + # epoch. `|| true` keeps a failing describe from tripping `set -e` + # and leaves the value empty; the guard below is then the single + # place the fallback is applied, and it does fire — on an export with + # no .git, or a repo with no commits yet. `unknown` is visibly wrong + # in a binary in a way that an empty version is not. + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + --build-arg VERSION="$version" \ + . } main "$@" diff --git a/script/docker b/script/docker index ac8897d..f9ebf9a 100755 --- a/script/docker +++ b/script/docker @@ -16,7 +16,19 @@ main() { # nonce to an empty constant. `$$` is required because busybox `date` # drops %N without erroring. epoch="$(date +%s%N)$$" - docker build --build-arg CHECK_EPOCH="$epoch" \ + # VERSION must be computed here, on the host: .dockerignore excludes + # .git, so `git describe` cannot run in any build stage and fails + # quietly there rather than erroring. Same own-line discipline as the + # epoch. `|| true` keeps a failing describe from tripping `set -e` + # and leaves the value empty; the guard below is then the single + # place the fallback is applied, and it does fire — on an export with + # no .git, or a repo with no commits yet. `unknown` is visibly wrong + # in a binary in a way that an empty version is not. + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + --build-arg VERSION="$version" \ -t "$("$SCRIPT_DIR/projectname")" . } -- 2.49.1 From 0620416869beb73bbd576d1ab7abd605d77e9952 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 17:35:48 +0000 Subject: [PATCH 5/6] Give golangci-lint per-checkout cache and lock state (closes #30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A golangci-lint result on a host running many concurrent workers does not reliably belong to the tree that asked for it. Two independent mechanisms, which have repeatedly been mistaken for one: The result cache is keyed 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 under the other tree's path. This produced a confirmed false green as well as the loud false reds. Moving workers from shared worktrees to their own clones does not address it — two clones collide exactly as two worktrees did — and removes only the foreign-path artefact that made the defect noticeable. The concurrency lock is $TMPDIR/golangci-lint.lock (pkg/commands/run.go, acquireFileLock), host-global and independent of GOLANGCI_LINT_CACHE, with a five-second acquire timeout, so it fails when the host is busiest. A private cache directory does not isolate it. Setting only the cache closes the contamination half and leaves runs failing red on a condition that is not a result at all. REPO_POLICIES.md now carries the canonical Go script/lint: both variables scoped into a .lint-cache/ directory inside the checkout, above any container-versus-host branch so every path reaching the linter gets them; --allow-serial-runners, which keeps the mutual-exclusion guard and queues rather than aborting, for the same-checkout overlap TMPDIR scoping cannot cover, with --allow-parallel-runners rejected because it deletes the guard; and a bounded retry that treats the lock error as VOID rather than as findings, exiting 75 on exhaustion so it is neither a pass nor a failure. Detection is on the stderr stream and never on exit status: findings go to stdout, so a finding quoting the lock message in source cannot be retried away, and the exit status is not a stable discriminator anyway. The interim void rule is recorded with the ../ clause that the original filter missed, and with its limit stated — it catches contamination that names foreign files, not contamination that suppresses findings. Both checklists gained the corresponding items, since a half-fix that sets only the cache reads as complete. GOCACHE was measured rather than assumed and does not need isolating: with the two variables scoped per checkout and GOCACHE shared at the host default, each checkout reported its own paths. Verified with the snippet extracted from the committed document and executed as a consuming repo would adopt it, each control paired against the pre-fix form: contamination reproduced on the pre-fix script and absent on the adopted one; a stub linter colliding twice then clearing, with the retry engaging and succeeding; exhaustion exiting 75 with a VOID message; a genuine finding whose text quotes the lock message reported as findings with no retry; and a real held lock failing the pre-fix script with exit 3 while the adopted script, inheriting the same environment, completed in one second. --- TODO.md | 29 +++ prompts/EXISTING_REPO_CHECKLIST.md | 7 + prompts/NEW_REPO_CHECKLIST.md | 11 + prompts/REPO_POLICIES.md | 345 +++++++++++++++++++++++++++++ 4 files changed, 392 insertions(+) diff --git a/TODO.md b/TODO.md index c598ba2..7965142 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,35 @@ fmt-check, and commit. # Completed Steps +- 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 + are separate defects and the second is the one that gets dropped: the result + cache is keyed on file content rather than location, so checkouts holding + identical files serve each other's findings under the other's path, while the + concurrency lock is `$TMPDIR/golangci-lint.lock` — host-global, independent of + the cache, and unaffected by isolating it. Moving workers from worktrees to + their own clones does not help either half; it only removes the foreign-path + artefact that made the defect visible. The lock error is retried rather than + surfaced, because it is not a result: it exits non-zero exactly as findings + do, and reporting it as findings sends a correct branch back for rework. + Detection is on the stderr stream and never on exit status, so a finding + quoting the lock message in source cannot be retried away, and exhaustion + exits 75 with a VOID message rather than passing or failing quietly. + `--allow-serial-runners` (which keeps the guard and queues) covers the + same-checkout overlap that `TMPDIR` scoping cannot; `--allow-parallel-runners` + is rejected outright. The stdout and stderr capture files are per invocation + rather than per checkout, because serialising the linter does not serialise + the shell's redirections: two runs in one checkout — the overlap the flag + exists to support — would otherwise truncate and read each other's output, + which is the same defect one layer above where it was fixed. Both checklists + gained the corresponding items, since a half-fix that sets only the cache + reads as complete. `GOCACHE` was measured and does not need isolating. + Verified with the snippet extracted from the committed document and executed + as a consuming repo would adopt it, against paired controls: contamination + reproduced on the pre-fix form and absent on the adopted one, retry engaged, + exhaustion loud, a genuine finding still reported, and a held host lock + failing the pre-fix script while leaving the adopted one untouched. - 2026-08-09: Kept in-repo agent scratch out of the Docker build context and out of version control. `.claude/` holds one worktree — an entire additional checkout of the repo — per in-flight agent, and under `COPY . .` all of it was diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 83421f9..08e6d0b 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -100,6 +100,13 @@ 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. - [ ] `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 diff --git a/prompts/NEW_REPO_CHECKLIST.md b/prompts/NEW_REPO_CHECKLIST.md index bd6d7f0..dcf8d45 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -109,6 +109,17 @@ are thin shims calling them. Model scripts: - [ ] `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/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 diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index 19304f1..dd870cc 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -691,6 +691,351 @@ style conventions are in separate documents: 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: + - 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. + + Do not record a verdict from a void run, and do not "fix" findings in files + the change does not touch — chasing phantom findings across untouched files + puts unrelated edits into a reviewed diff, which is more expensive than the + wasted rework. + + The `../` clause is the one that actually bites, and it is why a filter + keyed on `/tmp` or on absolute prefixes is not enough: golangci-lint reports + paths relative to its own resolved root rather than yours, and three of the + org's reported sightings had relative paths and would have passed such a + filter. Both clauses are needed and neither alone is sufficient — one + reproduction exited non-zero with the lock error and no foreign paths at + all, and another reported 34 well-formed findings, every one of them against + another checkout. + + **State the limit of these tests rather than treating them as a guarantee.** + They catch contamination that **names** foreign files. They cannot catch + contamination that **suppresses** findings through a poisoned entry for + colliding content, which has no wall-clock tell either — **no evidence of + 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. + - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD). -- 2.49.1 From cc6a5a00e74c219305e461f93092984afb616a9e Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 12:49:34 +0000 Subject: [PATCH 6/6] Run every lint in a container via Dockerfile.lint (closes #40) script/lint runs the linter directly when it is already inside a container and otherwise builds Dockerfile.lint, so the linter never runs on a developer host. That closes three host-only mechanisms: the result cache golangci-lint keys on file content rather than location, which produced a confirmed false green and findings reported against other checkouts; the host-global $TMPDIR/golangci-lint.lock, which fails a run in a way no caller can distinguish from findings; and host/container version skew, which hid thirteen findings on one repo. Detection is on LINT_IN_CONTAINER=1, set by every Dockerfile, and on nothing else. The two directions are not symmetric: a false negative inside a container attempts a nested docker build, finds no daemon and fails loudly, while a false positive on a host silently lints there, which is the defect this issue exists to kill. /.dockerenv is therefore rejected even as a fallback -- measured absent inside BuildKit RUN steps and present on any host that is itself a container, so it fails in both directions and one of them is the dangerous one. Nothing else changes shape. The Dockerfile still runs make check, script/check still runs test, lint and fmt-check, script/cibuild is still a single docker build with CHECK_EPOCH and VERSION, and the Go multistage lint stage and its COPY --from=lint ordering dependency survive with ENV LINT_IN_CONTAINER=1 added. Dockerfile.lint is the standalone developer-host path and carries the same CHECK_EPOCH guard, with the ARG below the dependency layer so only the lint re-runs. The script/bootstrap golangci-lint install and the per-checkout GOLANGCI_LINT_CACHE/TMPDIR wrapper are deleted as superseded. Neither has a caller left. A JS repo's yarn install stays: the rule is that no lint verdict may come from a host invocation, not that no linter binary may exist there, and in a repo whose formatter is its linter the formatter necessarily runs on the host. golangci-lint config verify is kept, on measurement. Under the pinned v2.12.2 a bogus top-level key and a bogus key under linters.settings.lll both pass `golangci-lint run` with exit 0 and `0 issues` while config verify exits 3 and names them; an unknown linter name fails run and passes config verify. It needs no network: every case reproduced byte-identically under `docker run --network none`, in a container where `getent hosts golangci-lint.run` exits 2. Comment blocks were cut hard across every file this unit touches. .dockerignore drops from 67 comment lines to 28, script/cibuild from 17 to 12, script/docker from 18 to 12, and prompts/REPO_POLICIES.md from 1182 lines to 907. What remains says why a line is load-bearing; the discovery narratives are gone. config verify lives in script/lint's native branch rather than in a Dockerfile, so every path that lints inherits it: the lint stage of the main image, which is what CI runs, as well as Dockerfile.lint. Putting it in one Dockerfile is how the other path silently loses it. --- .dockerignore | 87 +-- Dockerfile | 26 +- Dockerfile.lint | 27 + README.md | 4 +- TODO.md | 21 + prompts/CODE_STYLEGUIDE_GO.md | 8 +- prompts/EXISTING_REPO_CHECKLIST.md | 43 +- prompts/NEW_REPO_CHECKLIST.md | 44 +- prompts/REPO_POLICIES.md | 887 ++++++++++------------------- script/cibuild | 23 +- script/docker | 18 +- script/lint | 27 +- 12 files changed, 509 insertions(+), 706 deletions(-) create mode 100644 Dockerfile.lint diff --git a/.dockerignore b/.dockerignore index 84c3383..86b2d1a 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,75 +1,37 @@ -# Docker matches this file with moby/patternmatcher: Go filepath.Match -# semantics plus a `**` extension, compiled to a regexp. Plain -# filepath.Match has no `**` at all. What follows from that: `*` does not -# cross `/`, and a pattern without a leading `**/` is anchored at the -# build-context root. Every depth-independent pattern therefore needs the -# `**/` prefix — without it `config/.env` and `certs/server.key` still -# ship while the file reads as solved. +# .dockerignore does NOT use .gitignore semantics. Docker matches with +# moby/patternmatcher: filepath.Match plus `**`, so `*` does not cross +# `/` and an unprefixed pattern is anchored at the context root. Every +# depth-independent pattern therefore needs `**/`, or `config/.env` and +# `certs/server.key` still ship while the file reads as solved. Only +# genuinely root-anchored entries go unprefixed. Never transplant these +# into .gitignore, where `**/` is wrong. # -# Root-anchored entries are for paths that occur exactly once, at the -# context root. A host-built binary is the usual case, and it must be -# written anchored: `/myapp`, never `**/myapp`. The prefixed form also -# matches `cmd/myapp/`, which deletes the package directory from the -# context. In-repo agent scratch is the other case, for the same reason -# — with the caveat recorded at that entry: anchoring is exact only -# where agents run at the repo root, and a repo where they do not must -# add its own entries. +# Matching is case-sensitive, so secrets use character ranges rather +# than an ALL-CAPS twin, which would still miss `Server.Key`. # -# Matching is case-sensitive, so `**/*.key` does not match -# `certs/SERVER.KEY`, which is reachable on the case-insensitive -# filesystems most laptops use. Adding an ALL-CAPS twin per pattern is -# not the fix: it still misses `Server.Key` while reading as though case -# were handled. Character ranges cover every spelling in one line, so -# every secret name below is written that way — including the -# extensionless SSH keys and `.envrc`, because on those same -# case-insensitive filesystems direnv reads `.ENVRC` and ssh reads -# `ID_RSA`. -# -# `**/*.[eE][nN][vV]` also excludes a committed env template such as -# `example.env`. If the build genuinely needs one, re-include it with a -# negation after the pattern: `!docs/example.env`. -# -# Extend this file with the repo's own host-built artifacts (compiled -# binaries, test binaries, coverage output); those are per-repo and -# belong here because a host build otherwise drops them into the -# context. +# Extend with this repo's own host-built artifacts, written anchored: +# `/myapp`, never `**/myapp`, which also matches `cmd/myapp/` and +# deletes the package directory from the context. -# Repository metadata: exactly one, at the context root. Excluding it -# means `git describe` cannot run in any build stage, and it fails -# quietly there rather than erroring, so a version embedded that way -# comes out empty. Compute the version on the host and pass it in with -# `--build-arg VERSION=...`; see the version rule in REPO_POLICIES.md. +# Excluding .git means `git describe` cannot run in any build stage and +# fails quietly there; pass the version in with --build-arg VERSION. .git -# In-repo agent scratch: a directory holding a full additional checkout -# of the repo for each in-flight agent. Anchored because it occurs -# exactly once *where agents run at the repo root*, which is the -# convention this file assumes; the `**/` form would also match any -# nested directory of that name and delete it from the build. -# -# KNOWN GAP, and it is not hypothetical: the directory is created in the -# agent's working directory. If agents in this repo run in -# subdirectories — a monorepo with a per-service agent, say — then -# `services/api/.claude/` is NOT excluded by the line below and still -# reaches the build context and the image, which is the exposure this -# entry exists to close. A repo in that shape adds its own anchored -# entries (`/services/api/.claude`), or `**/.claude` after confirming no -# legitimately named nested directory would be caught. -# -# Not case-folded, unlike the secret patterns below: tooling creates -# this directory in exactly one spelling, so a folded pattern would add -# no coverage. +# Agent scratch: one full checkout of the repo per in-flight agent. +# Anchored because it occurs once where agents run at the repo root. +# KNOWN GAP: a repo running agents in subdirectories still ships +# `services/api/.claude/` and must add its own anchored entry. .claude -# Environment files. `*.env` covers both the bare `.env` name (`*` matches -# the empty string) and the `prod.env` convention. +# Environment files. `*.env` covers bare `.env` and the `prod.env` +# convention. Re-include a committed template with a negation if the +# build needs one: `!docs/example.env`. **/*.[eE][nN][vV] **/.[eE][nN][vV].* **/.[eE][nN][vV][rR][cC] -# Private keys and the bundles that carry them. Public certificates -# (*.crt, *.cer) are deliberately absent: they are not secrets and are -# sometimes a legitimate build input. +# Private keys and the bundles carrying them. Public certificates +# (*.crt, *.cer) are deliberately absent: they are legitimate inputs. **/*.[pP][eE][mM] **/*.[kK][eE][yY] **/*.[pP]12 @@ -86,8 +48,7 @@ **/.DS_Store **/Thumbs.db -# Editor state. Never a build input, and it churns under a developer's -# hands, so it invalidates COPY for reasons unrelated to the source. +# Editor state: never a build input, and it churns COPY. **/*.swp **/*.swo **/*~ diff --git a/Dockerfile b/Dockerfile index ac7232a..f9619d8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,26 +3,26 @@ FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e3 WORKDIR /app -# script/bootstrap installs all prerequisites (make via apk here; node -# and yarn are already in the base image, so those steps are skipped). -# Dependency manifests are copied first so the bootstrap layer is -# cached until they change. +# Makes script/lint run the linter directly rather than building +# Dockerfile.lint, which would need a docker daemon here. +ENV LINT_IN_CONTAINER=1 + +# script/bootstrap installs all prerequisites. Manifests are copied +# first so that layer stays cached until dependencies change. COPY script/ script/ COPY package.json yarn.lock ./ RUN script/bootstrap COPY . . -# CHECK_EPOCH is a per-invocation nonce supplied by script/cibuild and -# script/docker. Without it an unchanged tree serves this layer from +# CHECK_EPOCH is a per-invocation nonce from script/cibuild and +# script/docker; without it an unchanged tree serves this layer from # cache and the build reports a green it never ran. ARG is stage-scoped, -# so it must be redeclared in every stage that runs checks. The guard -# makes a bare `docker build .` fail loudly instead of silently reusing -# the empty (and therefore stable) cache key. Expand the value into the -# command so the cache miss does not depend on BuildKit's handling of an -# unreferenced ARG. Both the guard and the check RUN reference the value, -# so both are value-keyed: there are two independent invalidation points -# here, not one. Keep both. +# so declare it in every stage that runs checks. The guard fails a bare +# `docker build .`, which would otherwise reuse the empty (and therefore +# stable) cache key. The value is also expanded into the check command, +# so the cache miss does not depend on BuildKit's handling of an +# unreferenced ARG; keep both references. ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make check diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..7957ba9 --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,27 @@ +# Lint-only image, built by script/lint when it is not already inside a +# container. Linting is a build step, so a successful build is a clean +# lint, and nothing is bind-mounted, which matters when the daemon is +# remote. +# +# node 22-alpine, 2026-02-22 +FROM node@sha256:e4bf2a82ad0a4037d28035ae71529873c069b13eb0455466ae0bc13363826e34 + +WORKDIR /app + +# Makes script/lint run the linter directly instead of recursing into +# another docker build, which has no daemon here. +ENV LINT_IN_CONTAINER=1 + +COPY script/ script/ +COPY package.json yarn.lock ./ +RUN script/bootstrap + +COPY . . + +# ARG sits after the dependency layer so that layer stays cached and +# only the lint re-runs. The guard fails a bare `docker build +# -f Dockerfile.lint .`, which would otherwise reuse the empty (stable) +# cache key and report a lint it never ran. +ARG CHECK_EPOCH +RUN [ -n "$CHECK_EPOCH" ] || exit 1 +RUN echo "lint epoch: ${CHECK_EPOCH}" && make lint diff --git a/README.md b/README.md index 357f8df..15ba785 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,9 @@ 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 with prettier. Inside a container + (`LINT_IN_CONTAINER=1`, set by both Dockerfiles) it runs prettier directly; on + a host it builds `Dockerfile.lint` so the linter still runs in a container - `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 diff --git a/TODO.md b/TODO.md index 7965142..bf8d674 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,27 @@ fmt-check, and commit. # Completed Steps +- 2026-08-10: Moved every lint run into a container. `script/lint` now runs the + linter directly when `LINT_IN_CONTAINER=1` and otherwise builds + `Dockerfile.lint`, so the linter never runs on a developer host — closing the + content-keyed result cache that produced a confirmed false green, the + host-global `$TMPDIR/golangci-lint.lock`, and host/container version skew. + Detection is on that marker alone: a false negative inside a container fails + loudly on the missing daemon, while a false positive on a host would silently + restore host linting, so `/.dockerenv` is rejected outright — measured absent + inside BuildKit `RUN` steps and present on hosts that are themselves + containers. Everything else keeps its existing shape: `make check` still runs + in the image, `script/cibuild` is still one build, and the Go multistage lint + stage survives with `ENV LINT_IN_CONTAINER=1`. `Dockerfile.lint` carries the + same `CHECK_EPOCH` guard, with the `ARG` below the dependency layer so only + the lint re-runs. The `script/bootstrap` golangci-lint install and the + per-checkout cache/lock/`.lint-cache` wrapper are deleted as superseded; a JS + repo's `yarn install` stays, since the rule is about where a verdict comes + from, not about which binaries exist. `golangci-lint config verify` was kept + on measurement: a bogus config key passes `golangci-lint run` with `0 issues` + and fails `config verify`, and every case reproduced byte-identically under + `--network none`, so the schema is embedded and the line costs no network. + Comment blocks across the touched files were cut hard in the same pass. - 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..3b4a5a8 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,7 +111,11 @@ 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`, never by invoking the binary: the + linter always runs in a container, and `golangci-lint` is not installed on + the host by any repo. Invoked directly on a shared host it 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 diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 08e6d0b..59360d6 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 @@ -42,6 +42,15 @@ with your task. `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. +- [ ] **Every stage that runs checks sets `ENV LINT_IN_CONTAINER=1`** — the lint + stage and the build stage both. This is the item an existing repo most + often fails after adopting the containerised lint: without it + `script/lint` tries to build `Dockerfile.lint` from inside a build step, + where there is no daemon. +- [ ] `Dockerfile.lint` exists and `script/lint` builds it when not already in a + container — see the containerised-lint rule in `REPO_POLICIES.md`. Base + image pinned by sha256 with a version/date comment, `ARG CHECK_EPOCH` + **after** the dependency layer with the guard below it. - [ ] `.dockerignore` excludes the repo's own host-built artifacts (compiled binaries, test binaries, coverage output), written root-anchored — `/myapp`, never `**/myapp`, which would also match `cmd/myapp/`. An @@ -100,13 +109,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 detect-and-branch form, and no host + invocation anywhere in the repo can produce a lint **verdict** — grep for + the linter's own name across `script/`, the `Makefile` and CI config, not + just `script/lint`. A second path is likeliest here: a `make lint-fast`, + an older container-versus-host branch, or a CI step calling the binary + directly. **Expected hits that are not the defect**: `script/fmt`, and in + a repo whose formatter is also its linter, `script/fmt-check`. Everything + else the grep finds is a real second path and goes. +- [ ] Detection is on `LINT_IN_CONTAINER` alone. Reject any `/.dockerenv` or + cgroup heuristic: absent in BuildKit `RUN` steps, present on hosts that + are themselves containers, and a false positive lints on the host. +- [ ] `script/bootstrap` installs no golangci-lint. Delete the block, its + version and ref variables, and its call site. A JS repo's `yarn install` + stays — it brings a linter along with every other dependency, which is + fine as long as no verdict is taken from it. +- [ ] The per-checkout lint state is gone: no `GOLANGCI_LINT_CACHE` or `TMPDIR` + exports, no `--allow-serial-runners`, and `.lint-cache/` removed from + `.gitignore` and `.dockerignore`. - [ ] `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 +173,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 (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..9b8f15d 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 @@ -79,9 +79,21 @@ Template files can be fetched from: `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. + - Every stage that runs checks sets `ENV LINT_IN_CONTAINER=1`, so + `script/lint` runs the linter natively instead of trying to build + `Dockerfile.lint` where there is no daemon. + - Go repos: separate `lint` stage on the `golangci/golangci-lint` image, + with `COPY --from=lint /src/go.sum /dev/null` in the build stage to force + the ordering. Re-prove that ordering warm after adopting `CHECK_EPOCH`. - Server: also builds and runs the application - Non-server: brings up dev environment and runs `make check` - Image pinned by sha256 hash with version/date comment +- [ ] `Dockerfile.lint` — the lint-only image `script/lint` builds when it is + not already inside a container. Sets `ENV LINT_IN_CONTAINER=1`; same + `ARG CHECK_EPOCH` + guard + expanded-value discipline as above, with the + `ARG` **after** the dependency layer so only the lint re-runs. Base image + pinned by sha256 with a version/date comment. Copy from + `REPO_POLICIES.md`. - [ ] 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,18 +120,15 @@ 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` — runs the linter directly when + `LINT_IN_CONTAINER=1`, otherwise `epoch="$(date +%s%N)$$"` on its own line + then `docker build --build-arg CHECK_EPOCH="$epoch" -f Dockerfile.lint .`. + No lint verdict may come from a host invocation. Copy from + `REPO_POLICIES.md`. Detect on `LINT_IN_CONTAINER` only — never + `/.dockerenv`, which is absent in BuildKit `RUN` steps and present on + hosts that are themselves containers. Without the nonce this exits 0 on an + unchanged tree having linted nothing; without `-f Dockerfile.lint` it + builds the main image and lints nothing at all. - [ ] `script/fmt` / `make fmt` — formats code (writes) - [ ] `script/fmt-check` / `make fmt-check` — checks formatting (read-only) - [ ] `script/check` / `make check` — runs `test`, `lint`, `fmt-check`; must not @@ -149,7 +158,10 @@ are thin shims calling them. Model scripts: 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. + element is load-bearing. A bare `docker build .` fails closed by design, and + so does a bare `docker build -f Dockerfile.lint .`. The image runs + `make check`, which includes lint, so `script/cibuild` needs no separate + lint step. - [ ] `script/precommit` — called by the pre-commit hook; runs `script/check` - [ ] `script/install-precommit` — installs the pre-commit hook that runs @@ -161,7 +173,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 demonstrably executes - [ ] 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..424a877 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 @@ -94,32 +94,49 @@ style conventions are in separate documents: reading the Makefile. - Every repo should have a `Dockerfile`. All Dockerfiles must run `make check` - as a build step so the build fails if the branch is not green — which requires - `ARG CHECK_EPOCH` and its guard in every stage containing a check-running - `RUN`, per the `CHECK_EPOCH` rule below. Without them a Dockerfile satisfies - this criterion while its check layers are served from cache, so the build - cannot fail on a branch that is not green. For non-server repos, the - Dockerfile should bring up a development environment and run `make check`. For - server repos, `make check` should run as an early build stage before the final - image is assembled. Dockerfiles install development prerequisites by running - `script/bootstrap` rather than duplicating installs inline; COPY `script/` and - the dependency manifests (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, - etc.) before running it so the bootstrap layer stays cached until dependencies - change. + as a build step so the build fails if the branch is not green — the one + exception being `Dockerfile.lint`, which runs `make lint` alone because that + is its entire purpose — 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. + + **Every Dockerfile must also set `ENV LINT_IN_CONTAINER=1`**, above the + checks. `script/lint` builds `Dockerfile.lint` when it is not already in a + container; without the marker it would try that from inside a build step, + where there is no daemon. See the containerised-lint rule below. + + For non-server repos, the Dockerfile should bring up a development + environment and run `make check`. For server repos, `make check` should run + as an early build stage before the final image is assembled. Dockerfiles + install development prerequisites by running `script/bootstrap` rather than + duplicating installs inline; COPY `script/` and the dependency manifests + (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, etc.) before running it + so the bootstrap layer stays cached until dependencies change. - **Every 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 — `Dockerfile` and `Dockerfile.lint` alike; 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 + ENV LINT_IN_CONTAINER=1 ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make check ``` + `ENV LINT_IN_CONTAINER=1` belongs in every such stage too, and is the line + most often missed: without it `make check` reaches `script/lint`, which + tries to build `Dockerfile.lint` from inside a build step where there is no + daemon. See the containerised-lint rule below. + and in both `script/cibuild` and `script/docker`: ```sh @@ -132,6 +149,11 @@ style conventions are in separate documents: . ``` + `script/lint` needs the same nonce but is **not** this command: it builds a + different file with `-f Dockerfile.lint` and passes no version. Copy its + form from the containerised-lint rule below, not this block — a + `docker build` with no `-f` builds the main image and lints nothing. + 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; @@ -162,27 +184,189 @@ style conventions are in separate documents: concurrent invocations would collide. 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. + `go mod download` and `script/bootstrap` cached, so it does not push against + the five-minute Docker build ceiling. Blanket `--no-cache` is not an + acceptable substitute: it also busts the dependency layer, so every run + reinstalls dependencies over the network instead of only the first and those + after a manifest change. Never reach for `docker builder prune` — the build + cache is shared with every other build on the host. + +- **Every lint run happens in a container.** `script/lint` runs the linter + directly when it is already inside one, and otherwise builds `Dockerfile.lint` + so that it is. Either way the linter never runs on a developer host, where its + answer is not trustworthy: + - **Confirmed false green.** golangci-lint keys cached results on file + **content, not location**, so a second checkout of the same commit serves + its findings. One implementer reported `0 issues` on a branch genuinely + red with a `goconst` finding. Own-clones-instead-of-worktrees does not + help; two clones are byte-identical exactly as two worktrees were. + - **False reds**: findings reported against other checkouts and against + worktrees already deleted; in one case 399 issues returned to a clean + clone that genuinely lints 0. + - **Lock contention indistinguishable from findings.** golangci-lint flocks + `$TMPDIR/golangci-lint.lock` (`pkg/commands/run.go`, `acquireFileLock()`), + host-global and independent of `GOLANGCI_LINT_CACHE`, 5-second timeout. It + prints `parallel golangci-lint is running`, analyzes nothing, exits + non-zero. Not fixed by per-cache isolation — measured. + - **Version skew**: a host linter differing from the pinned one, with the + container surfacing thirteen findings the host missed. + + A container has its own cache, its own `TMPDIR` and a binary pinned by + digest, so none of it is reachable. This supersedes the per-checkout + `GOLANGCI_LINT_CACHE`/`TMPDIR` wrapper, which existed only to make a host + run trustworthy; delete it on adoption. + + The canonical `script/lint`, whose executable lines are the same in every + repo apart from the native lint command: + + ```sh + #!/bin/sh + # script/lint: run the linter. Inside a container, run it directly; on a + # host, build Dockerfile.lint so it runs in one anyway. + # + # LINT_IN_CONTAINER is set by this repo's Dockerfiles and is the ONLY + # accepted signal. Do not add a /.dockerenv fallback: it is absent inside + # BuildKit RUN steps and present on hosts that are themselves containers, + # so it both misses and false-positives — and a false positive silently + # restores host linting. + set -eu + + ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" + + main() { + cd "$ROOT" + + if [ "${LINT_IN_CONTAINER:-}" = "1" ]; then + # config verify lives here, not in a Dockerfile, so every path + # that lints inherits it — the lint stage of the main image as + # well as Dockerfile.lint. Duplicating it into each Dockerfile + # is how one of them silently loses it. + golangci-lint config verify --config .golangci.yml + exec golangci-lint run --config .golangci.yml ./... + fi + + # Own line, and `$$` because busybox `date` drops %N silently. + # Without a fresh nonce the lint layer is cached and this exits 0 + # having linted nothing. + epoch="$(date +%s%N)$$" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + -f Dockerfile.lint \ + . + } + + main "$@" + ``` + + and `Dockerfile.lint`, the standalone path for a developer host: + + ```dockerfile + # Lint-only image, built by script/lint when not already in a container. + # golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07 + FROM golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 + + WORKDIR /src + ENV LINT_IN_CONTAINER=1 + + COPY go.mod go.sum ./ + RUN go mod download + + COPY . . + + # ARG after the dependency layer so only the lint re-runs. + ARG CHECK_EPOCH + RUN [ -n "$CHECK_EPOCH" ] || exit 1 + RUN echo "lint epoch: ${CHECK_EPOCH}" && make lint + ``` + + Load-bearing properties: + - **Detection rests on `LINT_IN_CONTAINER=1` and nothing else.** Every + Dockerfile in the repo sets it; a host does not. The asymmetry is the + whole design: a **false negative** inside a container tries a nested + `docker build`, finds no daemon and fails loudly, while a **false + positive** on a host silently lints there — the exact defect this rule + exists to kill. So the signal must be one only our own images can produce. + `/.dockerenv` is not such a signal and must not be used, even as a + fallback: measured, it is **absent** inside BuildKit `RUN` steps and + **present** on any host that is itself a container, which is the common + case for CI runners and agent sandboxes. It fails in both directions, and + one of them is the dangerous one. + - **`CHECK_EPOCH`, not `--no-cache`.** `docker build -f Dockerfile.lint .` + on an unchanged tree returns a sub-second cached success having linted + nothing. The `ARG` goes **after** the dependency layer so only the lint + re-runs; `--no-cache` would also reinstall dependencies on every lint. + - **Non-Go repos get the same pattern around their own linter** — `eslint`, + `ruff`, `prettier`, `shellcheck`. Only the base image and the native lint + command change. + - **Keep `golangci-lint config verify`, put it in `script/lint`, and it + costs no network.** It goes in the native branch, not in a Dockerfile, so + the lint stage of the main image inherits it along with `Dockerfile.lint`; + putting it in one Dockerfile leaves the other path unverified. The two + commands catch disjoint classes, measured under the pinned v2.12.2: a + bogus top-level key and a bogus key under `linters.settings.lll` both pass + `golangci-lint run` with **exit 0 and `0 issues`** while `config verify` + exits 3 and names them; an invalid value type fails both; an unknown + linter name fails `run` and passes `config verify`. So `run` alone + silently ignores an unknown key — the mode where a threshold reads as + configured and is not applied. It needs no network: every case reproduced + byte-identically under `docker run --network none`, in a container where + `getent hosts golangci-lint.run` exits 2. The schema is embedded in the + pinned binary. Re-run that control when bumping the pin. + - **A failed `script/lint` that names no finding is not a lint result.** On + the host path `docker build` exits 1 both for findings and for a build + that never got there (daemon down, image unpullable, disk full). BuildKit + names the failing step; read it, fix the environment, re-run. Do not + record a verdict from a run that did not lint. + + **Scope: this rule is about linters, and a formatter is not one.** + `script/fmt` writes your working tree, so it can only run on the host, and + `script/fmt-check` is its read-only twin. In a repo whose formatter **is** + its linter (prettier over markdown; this repo), `script/bootstrap` therefore + installs the linter on the host as an ordinary dependency and + `script/fmt-check` runs it there. That is accepted: the version is pinned in + `package.json` and installed into the repo's own `node_modules`, so there is + no shared content-keyed cache, no host-global lock and nothing to skew + against. What is forbidden is taking a **lint verdict** from it — + `script/lint` stays the only source of one. A repo auditing itself will see + those hits and should leave them; anything else the grep finds is a real + second path to the linter and goes. + + **What a consuming repo does to adopt this**, in order: + 1. Add `Dockerfile.lint`. + 2. Replace `script/lint` with the form above, with its own native lint + command. + 3. Add `ENV LINT_IN_CONTAINER=1` to **every** stage of every Dockerfile that + runs checks — the lint stage and the build stage both. + 4. Delete any golangci-lint install from `script/bootstrap`, with its + version and ref variables and its call site. No lint verdict comes from + the host any more, so it can only reintroduce version skew. A JS repo's + `yarn install` stays. + 5. Delete the per-checkout lint state: `GOLANGCI_LINT_CACHE` and `TMPDIR` + exports, `--allow-serial-runners`, the retry/VOID wrapper, and + `.lint-cache/` from both `.gitignore` and `.dockerignore`. + 6. Verify by running `make lint` twice on an unchanged tree: the lint layer + must be `DONE` both times, never `CACHED`. Then plant a violation, + confirm it fails naming the finding, revert. A bare + `docker build -f Dockerfile.lint .` must fail on the guard. + + `script/check`, `script/cibuild`, `script/docker` and the `Dockerfile` are + unchanged by this: `make check` still runs inside the image, and + `script/lint` there takes the native path. - **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. - - The standard pattern for a Go repo Dockerfile is: + on the `golangci/golangci-lint` image (pinned by hash), so lint failures + surface in seconds rather than after a full compile. The build stage declares + an explicit dependency on it via `COPY --from=lint /src/go.sum /dev/null`, + which forces BuildKit — which runs stages in parallel by default — to finish + linting first. The canonical Go repo `Dockerfile`: ```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 WORKDIR /src + ENV LINT_IN_CONTAINER=1 COPY go.mod go.sum ./ RUN go mod download COPY . . @@ -195,6 +379,7 @@ style conventions are in separate documents: # golang:1.x-alpine, YYYY-MM-DD FROM golang@sha256:... AS builder WORKDIR /src + ENV LINT_IN_CONTAINER=1 # Force BuildKit to run the lint stage before proceeding COPY --from=lint /src/go.sum /dev/null @@ -221,50 +406,36 @@ 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. + - The lint stage uses the `golangci/golangci-lint` image directly (it has + both Go and the linter), so nothing needs installing. `make lint` there + runs `script/lint`, which sees `LINT_IN_CONTAINER=1` and invokes + `golangci-lint` natively instead of building `Dockerfile.lint`. Without + that `ENV` the stage would attempt a nested build and fail. + - `COPY --from=lint /src/go.sum /dev/null` is a no-op copy that exists only + to create the stage dependency; without it 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 + The cache-bust turns the no-op `COPY` into a content-cache hit, so an + ordering guarantee established cold does not automatically carry over. It + was re-proved in another org repo using the same trick and held, but that + result does not transfer by assumption — re-check it warm. + - If the project uses `//go:embed` referencing build artifacts, the lint + stage must create placeholders so the directives resolve: + `RUN mkdir -p web/dist && touch web/dist/index.html`. + - If linting needs CGO or system libraries (e.g. `vips-dev`), `apk add` them + in the lint stage. + - Tests run in the build stage, not the lint stage: they may need 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. - - `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 - build, not a source of truth. **No stage may call `git describe`**: - `.dockerignore` excludes `.git`, so it yields an empty version without - failing. See the git-describe rule further down. + frozen at its last cached result. In each stage the guard sits immediately + below the `ARG` and the value is expanded into the first check `RUN`. + Later `RUN`s in the same stage need no expansion; their parent layer is + already busted. + - `ARG VERSION=dev` is declared in the build stage and supplied by + `script/docker` and `script/cibuild`. **No stage may call + `git describe`**: `.dockerignore` excludes `.git`, so it yields an empty + version without failing. See the git-describe rule further down. - Every repo should have a Gitea Actions workflow (`.gitea/workflows/`) that runs `script/cibuild` (which runs @@ -274,9 +445,9 @@ style conventions are in separate documents: 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. + always go through `script/cibuild` or `script/docker`. Never accept a pass as + evidence without confirming it ran: a sub-second wall time, or `CACHED` on the + check layer, means nothing was executed. - Use platform-standard formatters: `black` for Python, `prettier` for JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with @@ -498,516 +669,83 @@ 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: golangci-lint is not installed on the host by any + repo. Bumping the version means changing that one digest. -- **`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 golangci-lint.** This supersedes the + pinned host install that used to be canonical here. `script/lint` never runs + it on the host — it either builds `Dockerfile.lint` or is already in a + container that ships the binary — so a host install has no caller, and its + only remaining effect is to put a second, independently-versioned linter where + somebody eventually runs it by hand and believes the result. Delete the block, + its version and ref variables, and its call site. - 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: + This is not a ban on host dependency installs generally. A JS or docs repo's + `script/bootstrap` runs `yarn install`, which brings its linter along with + every other dependency; that is unavoidable and fine. The rule is about a + **dedicated** linter install, and about where a verdict may come from. - ```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: the per-checkout `GOLANGCI_LINT_CACHE`/`TMPDIR` wrapper for + `script/lint`.** It existed only to make a host lint run trustworthy, and the + containerised-lint rule above removes the host run. Delete the wrapper, the + `--allow-serial-runners` flag, and `.lint-cache/` from both `.gitignore` and + `.dockerignore`. Two of its conclusions outlive it: **`GOCACHE` does not need + isolating** (measured — content-addressed, no foreign paths in its entries, no + global lock), and **verifying lint plumbing requires paired controls** run + against the artifact as a consuming repo would adopt it, since a control that + passes against the broken form proves nothing. - **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. - - 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 +771,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/cibuild b/script/cibuild index 4fbd751..c75ed5a 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,27 +1,22 @@ #!/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. +# invocation: without it Docker serves the check layer from cache and the +# build exits 0 without running the suite. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - # 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. + # Both assignments 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. `$$` because busybox `date` + # drops %N without erroring. VERSION is computed here because + # .dockerignore excludes .git, so `git describe` in a build stage + # yields an empty version without failing; the guard below is the + # single place the fallback is applied. epoch="$(date +%s%N)$$" - # VERSION must be computed here, on the host: .dockerignore excludes - # .git, so `git describe` cannot run in any build stage and fails - # quietly there rather than erroring. Same own-line discipline as the - # epoch. `|| true` keeps a failing describe from tripping `set -e` - # and leaves the value empty; the guard below is then the single - # place the fallback is applied, and it does fire — on an export with - # no .git, or a repo with no commits yet. `unknown` is visibly wrong - # in a binary in a way that an empty version is not. version="$(git describe --tags --always --dirty 2>/dev/null || true)" [ -n "$version" ] || version="unknown" docker build \ diff --git a/script/docker b/script/docker index f9ebf9a..e52f2e1 100755 --- a/script/docker +++ b/script/docker @@ -11,19 +11,13 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" - # 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. + # Both assignments 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. `$$` because busybox `date` + # drops %N without erroring. VERSION is computed here because + # .dockerignore excludes .git, so `git describe` in a build stage + # yields an empty version without failing. epoch="$(date +%s%N)$$" - # VERSION must be computed here, on the host: .dockerignore excludes - # .git, so `git describe` cannot run in any build stage and fails - # quietly there rather than erroring. Same own-line discipline as the - # epoch. `|| true` keeps a failing describe from tripping `set -e` - # and leaves the value empty; the guard below is then the single - # place the fallback is applied, and it does fire — on an export with - # no .git, or a repo with no commits yet. `unknown` is visibly wrong - # in a binary in a way that an empty version is not. version="$(git describe --tags --always --dirty 2>/dev/null || true)" [ -n "$version" ] || version="unknown" docker build \ diff --git a/script/lint b/script/lint index c489634..b6d3db8 100755 --- a/script/lint +++ b/script/lint @@ -1,13 +1,34 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run the linter. Inside a container, run it directly; +# on a host, build Dockerfile.lint so it runs in one anyway. The linter +# is never run on a developer host, where a shared result cache, a +# host-global lock and a stale toolchain make its answer untrustworthy. +# +# LINT_IN_CONTAINER is set by this repo's Dockerfiles and is the ONLY +# accepted signal. Do not add a /.dockerenv fallback: it is absent +# inside BuildKit RUN steps and present on hosts that are themselves +# containers, so it both misses and false-positives — and a false +# positive silently restores host linting. 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 + + if [ "${LINT_IN_CONTAINER:-}" = "1" ]; then + exec yarn run prettier --check '**/*.md' \ + --tab-width 4 --prose-wrap always + fi + + # Own line, and `$$` because busybox `date` drops %N silently. + # Without a fresh nonce the lint layer is cached and this exits 0 + # having linted nothing. + epoch="$(date +%s%N)$$" + docker build \ + --build-arg CHECK_EPOCH="$epoch" \ + -f Dockerfile.lint \ + . } main "$@" -- 2.49.1