From d257f8f65895647c7152ac193c14165f4f5b7c4a Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 12:54:46 +0000 Subject: [PATCH] Lint in a container as a build step, via Dockerfile.lint (closes #113) Every lint run now happens inside its own container, invoked through script/lint, and linting is a build step rather than a container command: a successful build of the new root Dockerfile.lint IS a clean lint. That shape also works where the docker daemon is remote and bind mounts are impossible. Its FROM line -- golangci/golangci-lint:v2.12.2, pinned by digest -- is now the only pin of the linter version in this repo. A container per run has its own lint cache and its own golangci-lint lock, both discarded with it, so neither cross-worktree contamination nor lock contention exists any more. The machinery that defended against them is therefore gone: the per-worktree cache directories, the lock-retry loop, and script/lint-audit, which existed to catch findings replayed from a cache that no longer exists. So is the host lint path in its entirety -- the native escape hatch, its version detection, and VAULTIK_LINT_IN_CONTAINER in both script/lint and the Dockerfile. Nothing lints on the host, at any version. A cached build lints nothing, so the CHECK_EPOCH mechanism the product Dockerfile already used is what makes a green mean something: ARG CHECK_EPOCH with no default, placed below the module layers so dependency caching survives, a `RUN [ -n "$CHECK_EPOCH" ] || exit 1` guard so a build that withholds the arg fails instead of replaying, and the value expanded into the lint command itself. script/lint computes `epoch="$(date +%s%N)$$"` as a bare assignment on its own line, because inline in the argument a failing substitution does not abort under `set -eu` and yields a constant empty epoch -- which is exactly the false green being prevented. The product Dockerfile loses its lint stage rather than gaining a second linter pin. That stage ran `make lint`, which is now `docker build`: docker-in-docker inside a BuildKit step with no daemon. Calling golangci-lint directly there instead would have meant two independently bumpable digests for one tool. `make fmt-check` moves beside `make test` in the builder stage, and script/cibuild now builds Dockerfile.lint and then Dockerfile, each with its own fresh epoch, failing on either. Consequence, stated in comments rather than left to be discovered: script/docker builds the product image only and no longer lints; script/check and script/cibuild are the gates. `golangci-lint config verify` runs as its own epoch-keyed layer, above the lint. It is not belt-and-braces: `golangci-lint run` rejects a config it cannot PARSE but silently IGNORES an unknown top-level KEY. Renaming .golangci.yml's `linters:` to `linterz:` -- one character -- discards `default: all`, the disable list and every threshold, leaves only the small default linter set running, and exits 0 reporting `0 issues.` on a tree the real config fails with an lll finding, in a run whose lint layer demonstrably executed. That is a set-but- ineffective config falling back to defaults instead of failing loudly, sitting in the gate's own configuration. `config verify` catches it and does so with the network genuinely off at this pin: under `docker run --network none` against the pinned digest it exits 0 on this repo's config and exits 3 on the `linterz:` variant. It is keyed on CHECK_EPOCH like the lint itself, because a cached validation validates nothing. script/lint-fix is kept, reimplemented as a bind-mounted docker run against the image parsed out of Dockerfile.lint -- a build step cannot write fixes back to the worktree -- and its header states outright that it is a developer convenience, never a gate, and needs a local daemon. cmd/vaultik/lintdocker_test.go parses both Dockerfiles and both scripts and fails if any part of the mechanism is dropped: the digest pin, the defaultless ARG below `go mod download`, the emptiness guard, the expansion of the epoch into each check command, the bare per-invocation epoch assignment in both scripts, cibuild building both files, the config verification running before the lint, and -- structurally, not by searching for one retired variable name -- that no script invokes golangci-lint except through docker. Every one of those losses is silent: the build still exits 0 and nothing is checked, which is why they are asserted rather than trusted. The scanner behind the last of those has its own test, because a structural check that goes blind passes on every tree, including a broken one. script/lint takes no arguments now, and says so instead of dropping them: a build step has no command line to pass linter flags to. --- Dockerfile | 72 ++--- Dockerfile.lint | 104 +++++++ Makefile | 8 +- README.md | 88 +++--- TODO.md | 60 ++++ cmd/vaultik/lintdocker_test.go | 507 +++++++++++++++++++++++++++++++++ cmd/vaultik/makefile_test.go | 22 +- script/bootstrap | 33 ++- script/cibuild | 43 ++- script/docker | 7 + script/lint | 355 +++++------------------ script/lint-audit | 113 -------- script/lint-fix | 50 +++- 13 files changed, 921 insertions(+), 541 deletions(-) create mode 100644 Dockerfile.lint create mode 100644 cmd/vaultik/lintdocker_test.go delete mode 100755 script/lint-audit diff --git a/Dockerfile b/Dockerfile index f6fe069..2ba39e3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,23 +1,29 @@ -# Lint stage +# This file has no lint stage, deliberately. # -# This FROM line is the single source of truth for the linter version: -# script/lint parses the image reference out of it and runs that exact -# image, so a local `make lint` and CI use the same linter. Bump the -# linter here (tag AND digest) and nowhere else. +# Linting lives in Dockerfile.lint, built by script/lint, and +# script/cibuild builds both. A lint stage here would have to either +# shell out to `make lint` -- which is now `docker build`, so +# docker-in-docker inside a BuildKit step with no daemon -- or call +# golangci-lint directly, which would mean a second, independently +# bumpable digest pin for the linter alongside the one in +# Dockerfile.lint. Two pins for one tool is the drift that +# https://git.eeqj.de/sneak/vaultik/issues/78 was filed over. See +# https://git.eeqj.de/sneak/vaultik/issues/113 for the ruling. # -# golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 -FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 AS lint +# Consequence, stated rather than left to be discovered: script/docker +# builds this file only and therefore does not lint. `make fmt-check` +# and `make test` still run here, so what a green build of this file +# means is "formatted, tested, and it compiles" -- the lint verdict +# comes from script/lint or script/cibuild. -RUN apk add --no-cache make build-base +# Build stage +# golang:1.26.1-alpine, 2026-03-17 +FROM golang:1.26.1-alpine@sha256:2389ebfa5b7f43eeafbd6be0c3700cc46690ef842ad962f6c5bd6be49ed82039 AS builder -# The context signal for script/lint's native path. This stage runs -# `make lint` with no docker daemon available, so it is the one place -# that must run the golangci-lint on PATH directly. script/lint takes -# that path only when this is set AND the version matches the pin above; -# version equality alone would also admit a developer's locally -# installed copy on a host, bypassing the digest pin (issue #80). -# Nothing outside this stage sets it. -ENV VAULTIK_LINT_IN_CONTAINER=1 +ARG VERSION=dev + +# Install build dependencies for CGO (mattn/go-sqlite3) and sqlite3 CLI (tests) +RUN apk add --no-cache make build-base sqlite WORKDIR /src @@ -28,7 +34,7 @@ RUN go mod download # Copy source code COPY . . -# Run formatting check and linter. +# Run the format check and the tests. # # CHECK_EPOCH must stay immediately above these RUNs. These layers are # keyed on its value, so they are cache-eligible only for a value @@ -47,45 +53,15 @@ COPY . . # runs the checks and every one after it on an unchanged tree replays # these layers from cache, executes nothing, and still exits 0. Failed # steps are never cached, so the guard fails on EVERY invocation rather -# than once -- a bare `docker build .` is now a loud error, not a quiet +# than once -- a bare `docker build .` is a loud error, not a quiet # green. Do not give CHECK_EPOCH a default value; a default would # satisfy the guard with a constant and restore the hole. # -# ARG scope is per-stage, so the builder stage declares its own. # Everything above this line (apk, go.mod, `go mod download`) is # deliberately outside the busted range and keeps caching. ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check -RUN echo "check epoch: ${CHECK_EPOCH}" && make lint - -# Build stage -# golang:1.26.1-alpine, 2026-03-17 -FROM golang:1.26.1-alpine@sha256:2389ebfa5b7f43eeafbd6be0c3700cc46690ef842ad962f6c5bd6be49ed82039 AS builder - -# Depend on lint stage passing -COPY --from=lint /src/go.sum /dev/null - -ARG VERSION=dev - -# Install build dependencies for CGO (mattn/go-sqlite3) and sqlite3 CLI (tests) -RUN apk add --no-cache make build-base sqlite - -WORKDIR /src - -# Copy go mod files first for better layer caching -COPY go.mod go.sum ./ -RUN go mod download - -# Copy source code -COPY . . - -# Run tests. See the CHECK_EPOCH comment in the lint stage for the -# mechanism; ARG scope is per-stage, so this stage needs its own -# declaration, its own guard, and its own expansion, and they must stay -# immediately above the check RUN. -ARG CHECK_EPOCH -RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make test # Build (pure Go, no CGO required since we use modernc.org/sqlite) diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..e0413a1 --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,104 @@ +# Lint image. +# +# Every lint run in this repo happens inside this image, invoked through +# script/lint, and linting is a BUILD STEP rather than a container +# command: a successful build of this file IS a clean lint. That shape +# also works where the docker daemon is remote and bind mounts are +# impossible, which `docker run` against a mounted worktree does not. +# +# This FROM line is the single source of truth for the linter version in +# this repo. Nothing else pins golangci-lint: the product Dockerfile has +# no lint stage, deliberately, so there is no second digest to bump and +# no pair of pins that can drift apart. Bump the tag AND the digest here +# and nowhere else. +# +# Note for readers coming from REPO_POLICIES.md: that document still +# describes the older pattern, a lint stage inside the product +# Dockerfile wired up with `COPY --from=lint /src/go.sum /dev/null`. +# That pattern is superseded here by the owner's ruling recorded in +# https://git.eeqj.de/sneak/vaultik/issues/113 -- lint runs in its own +# image, per run, with its own cache and its own lock, which is what +# makes concurrent runs on one host safe. The policy text is org-wide +# and is being amended separately; this file is what this repo does. +# +# golangci/golangci-lint:v2.12.2, 2026-08-10 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 + +WORKDIR /src + +# Copy the dependency manifests first so the module download layer stays +# cached until they change. Everything above the ARG below is cacheable +# on purpose; a cold module download on every lint would make the inner +# loop unusable and buys nothing, because it is not what the gate is +# asserting. +COPY go.mod go.sum ./ +RUN go mod download + +COPY . . + +# Force the check layers to execute on every invocation. +# +# CHECK_EPOCH must stay immediately above the RUNs below. Those layers +# are keyed on its value, so they are cache-eligible only for a value +# already built against this same tree; script/lint and script/cibuild +# each pass a fresh value on every invocation, which is what makes their +# green mean the linter really ran. Without it, `docker build -f +# Dockerfile.lint .` on an unchanged tree exits 0 in well under a second +# having linted nothing. +# +# The value is expanded into each check command itself rather than left +# to a bare declaration, so the cache miss does not depend on BuildKit's +# unreferenced-ARG handling staying as it is. It also puts the epoch in +# the build log, where a reader can see the layer was keyed fresh. +# +# The guard is what makes a build that omits --build-arg fail instead of +# lie. An unset ARG is an empty string, and an empty string is a +# perfectly stable cache key: without the guard the first such build +# lints and every one after it on an unchanged tree replays this layer, +# executes nothing, and still exits 0. Failed steps are never cached, so +# the guard fails on EVERY invocation rather than once. Do not give +# CHECK_EPOCH a default value; a default would satisfy the guard with a +# constant and restore the hole. +ARG CHECK_EPOCH +RUN [ -n "$CHECK_EPOCH" ] || exit 1 + +# Validate .golangci.yml before linting with it. +# +# This is not belt-and-braces; it closes a hole that `golangci-lint run` +# leaves wide open. `run` rejects YAML it cannot PARSE, but it silently +# IGNORES an unknown top-level KEY. Renaming `linters:` to `linterz:` -- +# one character -- discards `default: all`, the whole disable list and +# every threshold, leaves only golangci-lint's small default linter set +# running, and exits 0 reporting `0 issues.` on a tree the real config +# fails. Demonstrated on this repo at this pin, recorded on +# https://git.eeqj.de/sneak/vaultik/pulls/114: with a planted +# over-length line, `script/lint` exits 1 naming the `lll` finding with +# `linters:` and exits 0 with `linterz:`. A set-but-ineffective config +# quietly falling back to defaults is precisely the false-green class +# this gate exists to eliminate, so it must not sit in the gate's own +# configuration. +# +# `config verify` catches it, and it does so OFFLINE at this pinned +# version -- verified, not assumed. Under `docker run --network none` +# against the pinned digest it exits 0 on this repo's config and exits 3 +# on the `linterz:` variant with `additional properties 'linterz' not +# allowed`. An earlier revision of this file asserted the opposite, that +# the schema is fetched over live HTTPS from an unpinned URL, and used +# that to justify omitting this line. That claim was false at v2.12.2; +# the schema is embedded. If a future bump reintroduces a network fetch +# the failure is loud and this comment is where to record it. +# +# It is keyed on CHECK_EPOCH, like the lint run below, so it executes on +# every invocation. Content-addressing alone would arguably be enough -- +# .golangci.yml arrives through `COPY . .`, so a cache hit here implies +# a byte-identical config was validated when the layer really ran. That +# argument is exactly the one that would also excuse caching the lint +# layer, and this repo has ruled it insufficient: a cached check layer +# checks nothing, and the cost of being wrong is silent. Forcing it costs +# milliseconds and puts the epoch in the log, where a reader can see that +# this validation ran rather than being replayed. +RUN echo "check epoch: ${CHECK_EPOCH}" && \ + golangci-lint config verify --config .golangci.yml + +RUN echo "check epoch: ${CHECK_EPOCH}" && \ + golangci-lint run --config .golangci.yml ./... diff --git a/Makefile b/Makefile index 45e4b5c..17f714a 100644 --- a/Makefile +++ b/Makefile @@ -87,10 +87,10 @@ clean: go clean # Install dependencies. The linter is deliberately not installed here: -# script/lint runs the digest-pinned golangci-lint image declared by the -# Dockerfile's lint stage, which is the single source of truth for the -# linter version. A second, separately pinned copy on PATH could drift -# from it and make a local `make lint` disagree with CI. +# script/lint lints by building Dockerfile.lint, whose FROM line is the +# single source of truth for the linter version. A second, separately +# pinned copy on PATH could drift from it and make a local `make lint` +# disagree with CI. deps: go mod download diff --git a/README.md b/README.md index cb7edda..89e4697 100644 --- a/README.md +++ b/README.md @@ -598,10 +598,11 @@ regardless of color setting (emoji are not color). * Go 1.26 or later * Docker, with a reachable daemon, to lint, check, or commit: - `script/lint` runs the digest-pinned `golangci-lint` image declared by - the `Dockerfile` lint stage, and `make check` and the pre-commit hook - both run it. A `golangci-lint` installed on `PATH` is not a substitute - and is never used on a host, whatever its version. + `script/lint` lints by building `Dockerfile.lint`, which runs the + digest-pinned `golangci-lint` image as a build step, and `make check` + and the pre-commit hook both run it. A `golangci-lint` installed on + `PATH` is not a substitute and is never used on a host, whatever its + version. * `sqlite3` CLI, which the test suite shells out to * S3-compatible object storage (or local filesystem, or rclone remote) @@ -671,46 +672,69 @@ them. We provide: diverges from the 30s `REPO_POLICIES.md` mandates; the reasoning is in the comment in the script, and issue #101 proposes amending the policy text. -* `script/lint` — run `golangci-lint run ./...` at the exact version CI - uses, by running the digest-pinned `golangci-lint` image declared by - the `Dockerfile` lint stage (requires Docker; it fails loudly rather - than falling back to a differently versioned `golangci-lint` on - `PATH`). That `FROM` line is the single source of truth for the linter - version — bump it there and nowhere else. +* `script/lint` — lint by building `Dockerfile.lint`, which runs + `golangci-lint run --config .golangci.yml ./...` as a build step + inside the digest-pinned `golangci-lint` image, so a successful build + *is* a clean lint. Nothing lints on the host, at any version, ever; + the script requires Docker and fails loudly rather than falling back + to a `golangci-lint` on `PATH`. That `FROM` line is the single source + of truth for the linter version — bump it there and nowhere else. + + It takes no arguments, because a build step has no command line to + pass flags to, and it passes a fresh `--build-arg CHECK_EPOCH` on + every invocation so the lint layer cannot be replayed from cache (see + `script/cibuild` below for what that mechanism defends against). To + watch the linter execute, run it as + `BUILDKIT_PROGRESS=plain script/lint` and check that the lint layer + says `RUN … golangci-lint` rather than `CACHED`. + + One container per run means one lint cache and one `golangci-lint` + lock per run, both private to it and discarded with it, so concurrent + runs on one host cannot contaminate or block each other. * `script/lint-fix` — apply the linter's autofixes (rewrites files), - using the same pinned linter + using the same pinned image, parsed out of `Dockerfile.lint`. It + cannot be a build step, because fixes have to land in the worktree, so + it bind-mounts the tree into a `docker run` and therefore needs a + *local* daemon. It is a developer convenience and never a gate: no + gate reads its exit status. Run `make lint` afterwards to find out + whether the tree is clean. * `script/fmt` — format all code (writes) * `script/fmt-check` — check formatting (read-only) * `script/check` — run `script/test`, `script/lint`, and - `script/fmt-check`. This is authoritative *because* `script/lint` uses - the pinned linter: a local `make check` and CI cannot disagree about - lint findings. + `script/fmt-check`. This is authoritative *because* `script/lint` + builds `Dockerfile.lint`: a local `make check` and CI cannot disagree + about lint findings. * `script/docker` — build the Docker image tagged via `script/projectname`. Passes a fresh `--build-arg CHECK_EPOCH` for the same reason `script/cibuild` does, so a local image build cannot be - green on checks it replayed from cache. -* `script/cibuild` — CI entrypoint: `docker build` (the `Dockerfile` - runs `make fmt-check` and `make lint` in its lint stage and `make - test` in its builder stage). This is the full CI-equivalent gate — it - runs the checks in the same containers CI does, from a clean copy of - the tree, so it also catches anything that depends on host state. It - passes a fresh `--build-arg CHECK_EPOCH`, unique per invocation, which - the `Dockerfile` declares immediately above the check `RUN`s in both - stages and expands into each check command. Those layers are keyed on + green on checks it replayed from cache. It builds the *product* image + only, and the product `Dockerfile` has no lint stage, so it does not + lint: a green here means formatted, tested, and it compiles. +* `script/cibuild` — CI entrypoint, and the full gate. Two builds, in + order: `Dockerfile.lint` (the linter, as a build step) and then + `Dockerfile` (`make fmt-check` and `make test` in its builder stage, + then the product image). Either failing fails the script. It runs the + checks in the same containers CI does, from a clean copy of the tree, + so it also catches anything that depends on host state. + + It passes a fresh `--build-arg CHECK_EPOCH` to each build, unique per + invocation, which both files declare immediately above their check + `RUN`s and expand into each check command. Those layers are keyed on that value, so a new value re-runs them even on a byte-identical tree, and a green from this script means the checks executed. Dependency and module layers sit above the `ARG` and still cache, so a build is not cold. - A build that supplies no `CHECK_EPOCH` — a bare `docker build .` — - fails rather than lying. An unset `ARG` is an empty string and an - empty string is a stable cache key, so without a guard such a build - would serve all three check layers from cache, execute nothing, and - still exit 0. Each check stage therefore asserts the value is - non-empty before running anything, and because failed steps are never - cached that assertion fires on every invocation rather than once. Use - `script/cibuild` (or `script/docker`, which passes the same arg); a - bare `docker build .` is now a loud error. + A build that supplies no `CHECK_EPOCH` — a bare `docker build .` or + `docker build -f Dockerfile.lint .` — fails rather than lying. An + unset `ARG` is an empty string and an empty string is a stable cache + key, so without a guard such a build would serve every check layer + from cache, execute nothing, and still exit 0. Each file therefore + asserts the value is non-empty before running anything, and because + failed steps are never cached that assertion fires on every + invocation rather than once. Use `script/lint`, `script/docker` or + `script/cibuild`, which pass the arg; a bare `docker build` is a loud + error. * `script/precommit` — pre-commit gate: `go mod tidy` + `go fmt` (must not change files), then `script/check` * `script/install-precommit` — install the git pre-commit hook that diff --git a/TODO.md b/TODO.md index 85aad68..e05ed56 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,66 @@ release" is exactly the contradiction # Completed Steps +- 2026-08-10: Moved every lint run into its own container, as a build + step ([issue #113](https://git.eeqj.de/sneak/vaultik/issues/113)). + New root `Dockerfile.lint`, built by `script/lint`, runs + `golangci-lint run --config .golangci.yml ./...` as a `RUN` + instruction in the digest-pinned `golangci/golangci-lint` image: a + successful build of that file *is* a clean lint, and it works even + where the daemon is remote and bind mounts are impossible. That + `FROM` line is now the only pin of the linter version in the repo. + + This supersedes the per-worktree cache isolation landed for + [issue #99](https://git.eeqj.de/sneak/vaultik/issues/99). Isolation + fixed cross-worktree contamination but not lock contention — two + concurrent runs with entirely separate cache directories still + collided. A container per run has its own cache and its own lock, so + the whole class is gone, and with it the per-worktree cache + machinery, the lock-retry loop, and `script/lint-audit`, which + existed to catch replayed findings from a cache that no longer + exists. The host lint path went too: no escape hatch, no + `VAULTIK_LINT_IN_CONTAINER`, no version detection. Nothing lints on + the host at any version. + + A cached build lints nothing, so the same `CHECK_EPOCH` mechanism the + product `Dockerfile` already used is what makes the green mean + something: `ARG CHECK_EPOCH` with no default below the module layers, + a `RUN [ -n "$CHECK_EPOCH" ] || exit 1` guard, the value expanded + into each check command, and a fresh `$(date +%s%N)$$` per invocation + computed as a bare assignment. `cmd/vaultik/lintdocker_test.go` + parses both Dockerfiles and both scripts and fails if any part of + that is dropped, because every way of losing it is silent. Its + host-lint assertion is structural — no script runs `golangci-lint` + except through `docker` — rather than a search for the one retired + variable name, which nothing could ever reintroduce. + + The product `Dockerfile` lost its lint stage rather than gaining a + second linter pin: `make lint` is now `docker build`, so the stage + would have been docker-in-docker with no daemon, and calling + `golangci-lint` directly there would have restored the two-pins drift + of [issue #78](https://git.eeqj.de/sneak/vaultik/issues/78). + `make fmt-check` moved beside `make test` in the builder stage, and + `script/cibuild` now builds `Dockerfile.lint` and then `Dockerfile`, + each with its own fresh epoch. Consequence, stated rather than left + to be found: `script/docker` builds the product image only and no + longer lints; the gates are `script/check` and `script/cibuild`. + + `golangci-lint config verify` runs as its own epoch-keyed layer, + above the lint. `golangci-lint run` rejects a config it cannot parse + but silently ignores an unknown top-level *key*: renaming `linters:` + to `linterz:` discarded `default: all` and every threshold and still + exited 0 on a tree the real config fails. `config verify` catches + that, and it does so with the network off at this pin — checked under + `docker run --network none`, not assumed. An earlier revision omitted + it on the claim that it fetches its schema over live HTTPS; that + claim was false at v2.12.2. + + `script/lint-fix` is kept, reimplemented as a + bind-mounted `docker run` against the image parsed out of + `Dockerfile.lint` — it cannot be a build step, because fixes have to + land in the worktree — and marked in its header as a developer + convenience that no gate reads. + - 2026-08-09: Finished the `--json` stdout contract and gave `make build` a rule ([issue #108](https://git.eeqj.de/sneak/vaultik/issues/108), [issue #110](https://git.eeqj.de/sneak/vaultik/issues/110)). Two diff --git a/cmd/vaultik/lintdocker_test.go b/cmd/vaultik/lintdocker_test.go new file mode 100644 index 0000000..3be8078 --- /dev/null +++ b/cmd/vaultik/lintdocker_test.go @@ -0,0 +1,507 @@ +package main_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This file guards the shape of the lint gate. Every property asserted +// here is one whose loss is SILENT: the build still exits 0, the gate +// still looks green, and nothing was linted or tested. +// +// The gate is a build step. script/lint builds Dockerfile.lint, which +// runs golangci-lint as a RUN instruction, so a successful build is a +// clean lint. BuildKit will happily replay that RUN from cache on an +// unchanged tree in well under a second, which is why the check layers +// are keyed on a CHECK_EPOCH build arg that the calling script +// regenerates per invocation, and why an empty value is a hard error +// rather than a stable cache key. +// +// These are parses rather than invocations. Shelling out to docker from +// the test suite would nest a build inside `make test`, which itself +// runs inside a build in CI. The one property a parse cannot establish +// -- that a real finding actually fails the build -- is verified by +// hand against a deliberately broken tree, recorded on the pull +// request. + +// The files under guard, relative to the repository root. +const ( + lintDockerfile = "Dockerfile.lint" + productDockerfile = "Dockerfile" + lintScript = "script/lint" + cibuildScript = "script/cibuild" +) + +// linterBinary is the linter's command name. Every occurrence of it in +// executable shell in this repo must be inside a docker invocation; see +// TestNoHostLintPathRemains. +const linterBinary = "golangci-lint" + +// checkEpochARG is the declaration, with no default value. A default +// would satisfy the non-empty guard with a constant, and a constant is +// a stable cache key: the checks would be replayed from cache forever +// after the first build. +const checkEpochARG = "ARG CHECK_EPOCH" + +// checkEpochGuard is what turns a build that omits --build-arg into a +// loud failure instead of a quiet green. Failed steps are never cached, +// so it fires on every such invocation rather than once. +const checkEpochGuard = `RUN [ -n "$CHECK_EPOCH" ] || exit 1` + +// freshEpoch is the epoch computation the calling scripts must use, as +// a bare assignment on its own line. Inline in an argument, a failing +// `date` would not abort under `set -eu`; CHECK_EPOCH would become the +// empty string, and the guard above would be the only thing standing +// between that and a permanently cached green. `$$` is required because +// `date +%s` is second-granular and busybox silently drops `%N`, so +// without the pid two concurrent runs in one second can collide. +const freshEpoch = `epoch="$(date +%s%N)$$"` + +// TestLintDockerfilePinsTheLinterByDigest fails if the lint image stops +// being pinned. An unpinned tag makes the gate's verdict depend on +// whatever the registry currently serves under that name. +func TestLintDockerfilePinsTheLinterByDigest(t *testing.T) { + t.Parallel() + + from := "" + + for _, instruction := range instructions(t, lintDockerfile) { + if strings.HasPrefix(instruction, "FROM ") { + from = instruction + + break + } + } + + require.NotEmpty(t, from, "%s declares no FROM", lintDockerfile) + assert.Contains(t, from, "golangci/golangci-lint", + "the lint image must be the golangci-lint image") + assert.Contains(t, from, "@sha256:", + "the lint image must be pinned by digest, not by tag alone") +} + +// TestLintDockerfileCannotBeCachedGreen pins the whole cache-busting +// mechanism in the file that lints: the declaration with no default, +// the non-empty guard, and the value expanded into the lint command +// itself rather than merely declared. +func TestLintDockerfileCannotBeCachedGreen(t *testing.T) { + t.Parallel() + + found := instructions(t, lintDockerfile) + + argAt := indexOf(found, checkEpochARG) + require.GreaterOrEqual(t, argAt, 0, + "%s must declare `%s` with no default value", + lintDockerfile, checkEpochARG) + + assert.GreaterOrEqual(t, indexOf(found, checkEpochGuard), argAt, + "%s must guard against an empty CHECK_EPOCH with `%s`", + lintDockerfile, checkEpochGuard) + + assertEpochExpandedInto(t, found[argAt:], "golangci-lint run") + + // Dependency layers must stay above the ARG, or every lint run + // re-downloads the module cache and the inner loop becomes + // unusable. + download := indexOf(found, "RUN go mod download") + require.GreaterOrEqual(t, download, 0, + "%s must download modules in their own layer", lintDockerfile) + assert.Less(t, download, argAt, + "`%s` must come after `go mod download` so dependency layers"+ + " still cache", checkEpochARG) +} + +// TestLintDockerfileVerifiesTheLinterConfig guards the validation of +// .golangci.yml itself. `golangci-lint run` rejects a config it cannot +// parse but silently IGNORES an unknown top-level key, so renaming +// `linters:` to `linterz:` discards `default: all` and every threshold +// and still exits 0 reporting no issues. `config verify` is what turns +// that into a failure, and it has to run BEFORE the lint, or the lint +// spends a minute reporting a verdict from a config already known to be +// wrong. +func TestLintDockerfileVerifiesTheLinterConfig(t *testing.T) { + t.Parallel() + + found := instructions(t, lintDockerfile) + verify := linterBinary + " config verify" + + verifyAt := indexContaining(found, verify) + require.GreaterOrEqual(t, verifyAt, 0, + "%s must run `%s --config .golangci.yml`: without it a typo'd"+ + " top-level key in .golangci.yml is silently ignored and the"+ + " gate passes with only the default linter set", lintDockerfile, + verify) + + runAt := indexContaining(found, linterBinary+" run") + require.GreaterOrEqual(t, runAt, 0, "%s must lint", lintDockerfile) + assert.Less(t, verifyAt, runAt, + "%s must verify the config before linting with it", lintDockerfile) + + // Keyed on the epoch like every other check layer, so it executes + // per invocation rather than being replayed. A cached validation + // validates nothing. + assertEpochExpandedInto(t, found, verify) +} + +// TestProductDockerfileCannotBeCachedGreen holds the same line for the +// checks that remain in the product image build. +func TestProductDockerfileCannotBeCachedGreen(t *testing.T) { + t.Parallel() + + found := instructions(t, productDockerfile) + + argAt := indexOf(found, checkEpochARG) + require.GreaterOrEqual(t, argAt, 0, + "%s must declare `%s` with no default value", + productDockerfile, checkEpochARG) + + assert.GreaterOrEqual(t, indexOf(found, checkEpochGuard), argAt, + "%s must guard against an empty CHECK_EPOCH", productDockerfile) + + assertEpochExpandedInto(t, found[argAt:], "make fmt-check") + assertEpochExpandedInto(t, found[argAt:], "make test") +} + +// TestProductDockerfileDoesNotLint records the split deliberately: the +// linter lives in Dockerfile.lint and nowhere else, so there is exactly +// one digest pinning it. A lint stage reintroduced here would either be +// docker-in-docker (`make lint` is now `docker build`) or a second, +// independently bumpable pin. +func TestProductDockerfileDoesNotLint(t *testing.T) { + t.Parallel() + + contents := readRepoFile(t, productDockerfile) + + for _, forbidden := range []string{"golangci", "make lint"} { + assert.NotContains(t, instructionText(contents), forbidden, + "%s must not lint: the linter is pinned once, in %s", + productDockerfile, lintDockerfile) + } +} + +// TestLintScriptBuildsTheLintDockerfileWithAFreshEpoch is the other +// half of the mechanism. The Dockerfile's guard only rejects an EMPTY +// epoch; a constant non-empty one would satisfy it and still be served +// from cache forever. +func TestLintScriptBuildsTheLintDockerfileWithAFreshEpoch(t *testing.T) { + t.Parallel() + + script := readRepoFile(t, lintScript) + + assertBareEpochAssignment(t, script, lintScript) + assert.Contains(t, script, `--build-arg CHECK_EPOCH="$epoch"`, + "%s must pass the fresh epoch to the build", lintScript) + assert.Contains(t, script, lintDockerfile, + "%s must build %s", lintScript, lintDockerfile) +} + +// TestCibuildBuildsBothDockerfilesWithFreshEpochs guards the CI gate: +// dropping either build silently removes a whole class of check from +// CI while leaving it green. +func TestCibuildBuildsBothDockerfilesWithFreshEpochs(t *testing.T) { + t.Parallel() + + script := readRepoFile(t, cibuildScript) + + assertBareEpochAssignment(t, script, cibuildScript) + assert.Equal(t, 2, strings.Count(script, freshEpoch), + "%s must compute a fresh epoch for each of its two builds", + cibuildScript) + assert.Equal(t, 2, + strings.Count(script, `--build-arg CHECK_EPOCH="$epoch"`), + "%s must pass a fresh epoch to both builds", cibuildScript) + assert.Contains(t, script, "-f Dockerfile.lint", + "%s must build %s", cibuildScript, lintDockerfile) +} + +// TestNoHostLintPathRemains fails if any escape hatch to a host linter +// comes back. The owner's ruling is that every lint run happens inside +// a container; a PATH binary that happens to match the pinned version +// is a different build reached by a different code path, and admitting +// it is what lets a local pass disagree with CI. +// +// This asserts the PROPERTY -- no script invokes the linter except +// through docker -- rather than the absence of any particular variable +// name. An earlier version of this test looked only for the literal +// VAULTIK_LINT_IN_CONTAINER, the name of the hatch that was removed +// alongside it, so nothing could ever trip it again: a hatch under any +// other name left it passing. A structural test that passes on a broken +// tree is worse than no test, because it is what a later reader trusts +// instead of re-deriving the invariant. +// +// script/lint-fix is not exempted. It is the one script that runs the +// linter as a container rather than as a build step, but it still runs +// it in one, so the same property holds of it. +func TestNoHostLintPathRemains(t *testing.T) { + t.Parallel() + + root := repoRoot(t) + + entries, err := os.ReadDir(filepath.Join(root, "script")) + require.NoError(t, err) + require.NotEmpty(t, entries, "no scripts found to scan") + + for _, entry := range entries { + if entry.IsDir() { + continue + } + + name := filepath.Join("script", entry.Name()) + for _, line := range shellCode(readRepoFile(t, name)) { + assertLinterIsContainerised(t, name, line) + } + } +} + +// assertLinterIsContainerised fails if the line runs the linter without +// handing it to docker first. Position matters: docker has to come +// before the binary, or the line is running the host linter and merely +// mentioning docker afterwards. +func assertLinterIsContainerised(t *testing.T, name, line string) { + t.Helper() + + at := strings.Index(line, linterBinary) + if at < 0 { + return + } + + docker := strings.Index(line, "docker") + + assert.True(t, docker >= 0 && docker < at, + "%s runs %s on the host; every lint run happens in a container"+ + " (line: %s)", name, linterBinary, line) +} + +// TestShellCodeSeesCodeAndNotProse keeps the scanner above honest. It +// has to ignore comments and here-document bodies, because script/lint +// and script/bootstrap both NAME golangci-lint in prose -- in comments, +// and in the error text they print -- precisely to say that the host +// binary is never used. A scanner that went blind, by over-eager +// stripping or by failing to join continuation lines, would make +// TestNoHostLintPathRemains pass on everything. +func TestShellCodeSeesCodeAndNotProse(t *testing.T) { + t.Parallel() + + script := strings.Join([]string{ + "#!/bin/sh", + "# a comment naming golangci-lint", + "cat >&2 <&2 </dev/null 2>&1 } -# Docker is a hard requirement, not a nice-to-have: script/lint runs the -# digest-pinned golangci-lint image from the Dockerfile's lint stage, and -# script/check and script/precommit both run script/lint. A bootstrap -# that prints "bootstrap complete" on a machine where `make check` cannot -# run is a false success, so this fails instead. +# Docker is a hard requirement, not a nice-to-have: script/lint lints by +# building Dockerfile.lint, whose digest-pinned golangci-lint image is +# the only place the linter runs, and script/check and script/precommit +# both run script/lint. A bootstrap that prints "bootstrap complete" on a +# machine where `make check` cannot run is a false success, so this fails +# instead. # # Installing docker from here was considered and rejected: it needs root, # a running daemon, and on macOS a GUI cask, so an attempt would itself @@ -79,13 +80,15 @@ bootstrap: FAILED - $reason. Docker is required to develop this repo. Without it these do not work: - script/lint runs the digest-pinned golangci-lint image declared - by the Dockerfile's lint stage, which is the single - source of truth for the linter version + script/lint builds Dockerfile.lint, which runs the linter as a + build step in a digest-pinned golangci-lint image. + That FROM line is the single source of truth for the + linter version script/check runs script/lint script/precommit runs script/check, so commits are blocked by the pre-commit hook installed by script/setup - script/cibuild builds the Dockerfile, which is what CI runs + script/cibuild builds Dockerfile.lint and Dockerfile, which is what + CI runs Install docker (and start the daemon, checking DOCKER_HOST and your group membership), then re-run script/bootstrap. golangci-lint on PATH @@ -104,12 +107,12 @@ main() { # Go toolchain if missing go; then pkg_install go golang go go; fi - # golangci-lint is deliberately NOT installed: script/lint runs the - # digest-pinned golangci-lint image from the Dockerfile's lint stage, - # so whatever a package manager happens to ship would only be a - # shadow of the pinned version that could drift from CI. script/lint - # will not use a PATH binary on a host at any version, so installing - # one here would buy nothing. + # golangci-lint is deliberately NOT installed: script/lint lints by + # building Dockerfile.lint, whose digest-pinned image is the only + # place the linter runs, so whatever a package manager happens to + # ship would only be a shadow of the pinned version that could drift + # from CI. Nothing on the host is ever used as a linter, at any + # version, so installing one here would buy nothing. # sqlite3 CLI: the test suite shells out to it (VACUUM). if missing sqlite3; then pkg_install sqlite sqlite3 sqlite sqlite; fi diff --git a/script/cibuild b/script/cibuild index 371fe59..e3bb238 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,22 +1,31 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile does not run -# script/check; it runs `make fmt-check` and `make lint` in its lint -# stage and `make test` in its builder stage. A successful build -# implies those three passed, provided they actually ran -- which is -# what the CHECK_EPOCH below is for. -# Generic: needs no adaptation. The Gitea workflow runs this on push. +# script/cibuild: run the CI build. This is the full gate, and it is two +# builds, in this order: +# +# Dockerfile.lint the linter, as a build step (a clean build IS a +# clean lint) +# Dockerfile `make fmt-check` and `make test` in the builder +# stage, then the product image +# +# Either one failing fails this script. Note what follows from the +# split: script/docker builds only the product image and so no longer +# lints -- this script and script/check (which runs script/lint) are the +# things that decide whether the tree is clean. +# +# Generic apart from the two Dockerfiles: the Gitea workflow runs this +# on push. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - # The Dockerfile's check layers are keyed on CHECK_EPOCH, so a - # fresh value here is what forces them to re-run: without it an + # Both Dockerfiles key their check layers on CHECK_EPOCH, so a fresh + # value is what forces those layers to re-run: without it an # unchanged tree replays them from cache, the checks never execute, - # and the build still exits 0. The ARG sits immediately above the - # check RUNs, so dependency and module layers still cache. The - # Dockerfile also refuses to build at all when CHECK_EPOCH is empty, + # and the build still exits 0. Each ARG sits immediately above the + # check RUNs, so dependency and module layers still cache. Both + # Dockerfiles also refuse to build at all when CHECK_EPOCH is empty, # so a missing value fails loudly here rather than passing quietly. # # The value must be unique per invocation, not per second. `date +%s` @@ -35,6 +44,18 @@ main() { # script exists to prevent -- so the guard would disarm itself and # still exit 0. As a bare assignment, `set -e` catches a failing # `date` and no build starts. + # + # A separate value per build, because they are separate builds: one + # `date` shared between them would still be fresh, but reusing it + # invites the two to be collapsed into a single value that is + # computed somewhere else and passed in. + epoch="$(date +%s%N)$$" + # cacheonly for the lint build: its verdict is the exit status and + # the image is never run, so exporting it is pure cost. See + # script/lint. + docker build --output=type=cacheonly \ + --build-arg CHECK_EPOCH="$epoch" -f Dockerfile.lint . + epoch="$(date +%s%N)$$" docker build --build-arg CHECK_EPOCH="$epoch" . } diff --git a/script/docker b/script/docker index f592630..3fb027b 100755 --- a/script/docker +++ b/script/docker @@ -2,6 +2,13 @@ # script/docker: build the Docker image tagged with the project name. # Identical in all repos; the tag comes from script/projectname. # Generic: needs no adaptation. +# +# This builds the PRODUCT image only, and the product Dockerfile has no +# lint stage: linting lives in Dockerfile.lint and is run by +# script/lint. So a green here means `make fmt-check` and `make test` +# passed and the image built -- it says nothing about lint. The gates +# are script/check (which runs script/lint) and script/cibuild (which +# builds both files). set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" diff --git a/script/lint b/script/lint index 321b5b0..5499014 100755 --- a/script/lint +++ b/script/lint @@ -1,110 +1,42 @@ #!/bin/sh # script/lint: run the linter. # -# The linter always runs at the version pinned by the Dockerfile's lint -# stage, so a local run and a CI run of the same tree cannot disagree. -# That FROM line (image tag plus digest) is the single source of truth -# for the linter version in this repo: bump it there and nothing else -# needs editing. +# The linter runs inside the image built by Dockerfile.lint, and it runs +# there as a BUILD STEP: a successful build of that file IS a clean +# lint. Nothing lints on the host, at any version, ever. That FROM line +# is the single source of truth for the linter version in this repo, so +# a local run and a CI run of the same tree cannot disagree. # -# Normally that means running the pinned image with docker. The one -# exception is running INSIDE that image: the Dockerfile's lint stage -# runs `make lint`, and there is no docker daemon in there. That stage -# sets VAULTIK_LINT_IN_CONTAINER=1, and only when that variable is set -# is a golangci-lint on PATH used directly - and then only if its -# version is exactly the pin. Version equality alone is deliberately NOT -# enough: it also matches a developer's locally installed copy of the -# same version, which is a different build with a different Go -# toolchain, reached by a different code path, and it would bypass the -# digest pin this script exists to enforce. /.dockerenv was considered -# as the context signal and rejected: dockerd creates it for `docker -# run`, but it is not reliably present during a BuildKit `docker build`, -# which is exactly the case the exception exists for. +# One container per run means one lint cache and one golangci-lint lock +# per run, both private to that run and thrown away with it. That is +# what makes concurrent runs on a shared host safe, and it is why this +# script no longer carries per-worktree cache directories, a lock-retry +# loop, or an output audit: there is no shared state left for them to +# defend (issue https://git.eeqj.de/sneak/vaultik/issues/113). # -# The linter's output is checked before it is believed: every run is -# audited by script/lint-audit for findings that cannot belong to this -# tree, and a run refused by golangci-lint's cross-process lock is -# retried rather than reported as a verdict. See the lock-retry loop in -# main and the header of script/lint-audit. +# To watch the linter execute, set BUILDKIT_PROGRESS=plain, which docker +# honours directly: # -# Extra arguments are passed through to `golangci-lint run`, before -# `./...` (see script/lint-fix). +# BUILDKIT_PROGRESS=plain script/lint +# +# The check layers -- `golangci-lint config verify` and then +# `golangci-lint run` -- must appear as executing rather than CACHED on +# every run; see the CHECK_EPOCH comment in Dockerfile.lint. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -DOCKERFILE="$ROOT/Dockerfile" - -# golangci-lint takes a cross-process lock and refuses to start while -# another instance holds it. That refusal is not a lint result, and -# exiting non-zero on it is indistinguishable to a caller from real -# findings - so it is retried rather than reported. Bounded, because a -# lock that is never released must fail rather than hang. -LOCK_MESSAGE="parallel golangci-lint is running" -LOCK_ATTEMPTS=6 -LOCK_SLEEP=15 - -# The image reference of the Dockerfile's lint stage, tag and digest -# included, e.g. -# golangci/golangci-lint:v2.12.2-alpine@sha256:91b2... -lint_image() { - awk '$1 == "FROM" && $3 == "AS" && $4 == "lint" { print $2; exit }' \ - "$DOCKERFILE" -} - -# The bare version that image reference pins, e.g. 2.12.2 -pinned_version() { - lint_image | sed -e 's/@.*//' -e 's/.*://' -e 's/^v//' -e 's/-.*//' -} - -# The version of the golangci-lint on PATH, if any, e.g. 2.12.2 -# -# `version --short` prints the bare version and is the interface meant -# for this (checked against 2.10.1 and 2.12.2). The banner scrape below -# it is a fallback for a release where --short is absent or silent; the -# banner's exact wording is not a stable interface, which is why it is -# no longer the primary parse. -installed_version() { - command -v golangci-lint >/dev/null 2>&1 || return 0 - - short="$(golangci-lint version --short 2>/dev/null | - tr -d '[:space:]' | sed -e 's/^v//')" - case "$short" in - *[0-9].[0-9]*.[0-9]*) - echo "$short" - return 0 - ;; - esac - - golangci-lint version 2>/dev/null | awk ' - { - for (i = 1; i <= NF; i++) { - if ($i ~ /^[0-9]+\.[0-9]+\.[0-9]+$/) { - print $i - exit - } - } - }' -} - -# True inside the Dockerfile's lint stage, which sets this. Nothing else -# sets it: setting it by hand on a host is an explicit, visible decision -# to lint with an unpinned binary, not something reached by accident. -in_lint_container() { - [ "${VAULTIK_LINT_IN_CONTAINER:-}" = "1" ] -} +DOCKERFILE="$ROOT/Dockerfile.lint" require_docker() { - image="$1" if ! command -v docker >/dev/null 2>&1; then cat >&2 <&2 </dev/null 2>&1; then - printf '%s' "$ROOT" | sha256sum | cut -c1-12 - elif command -v shasum >/dev/null 2>&1; then - printf '%s' "$ROOT" | shasum -a 256 | cut -c1-12 - else - printf '%s' "$ROOT" | cksum | tr -cd '0-9' | cut -c1-12 - fi -} - -# Caches for the containerized linter, private to THIS worktree. -# -# Keeping them out of the repo and persisting them between runs is what -# keeps the inner loop fast: a warm run costs about the same as a native -# one plus container startup. Keeping them keyed on the worktree path is -# what keeps them correct. One shared cache for the whole repo was the -# defect in issue #99: two worktrees of this repo have identical file -# contents, so their cache keys collide, and golangci-lint replays the -# stored results - including the file paths recorded when they were -# produced. That silently reports one worktree's findings, or one -# worktree's clean bill of health, for another. -cache_dir() { - slug="$(printf '%s' "$(basename "$ROOT")" | tr -c 'A-Za-z0-9._-' '-')" - echo "$(cache_home)/$slug-$(path_digest)" -} - -# One cache per worktree means throwaway worktrees would otherwise leave -# caches behind forever. Each cache records the worktree it belongs to, -# and any cache whose worktree no longer exists is collected here, so -# growth is bounded by the number of worktrees that actually exist. The -# whole tree also sits under XDG_CACHE_HOME (~/.cache by default), so it -# is disposable by definition: `rm -rf "${XDG_CACHE_HOME:-~/.cache}/vaultik-lint"` -# costs nothing but the next run's cold cache. -prune_dead_caches() { - home="$(cache_home)" - if [ ! -d "$home" ]; then - return 0 - fi - for dir in "$home"/*; do - if [ ! -f "$dir/worktree" ]; then - continue - fi - owner="$(cat "$dir/worktree")" - if [ -z "$owner" ]; then - continue - fi - if [ ! -d "$owner" ]; then - # The Go module cache inside is deliberately read-only, and - # rm(1) cannot unlink a file out of a directory it may not - # write, so the tree has to be made writable first. And - # failing to tidy up is a housekeeping problem, never a - # reason to fail a lint: without the fallback below, `set - # -e` turns a stale cache that will not delete into a lint - # error, which is a gate failing for a reason that has - # nothing to do with the code. (Observed, not theorised.) - chmod -R u+w "$dir" 2>/dev/null || true - if ! rm -rf "$dir" 2>/dev/null; then - # Restore the marker on a partial removal: an - # unmarked leftover would be skipped by every future - # run and never collected. - mkdir -p "$dir" 2>/dev/null || true - echo "$owner" >"$dir/worktree" 2>/dev/null || true - echo "lint: could not remove stale cache $dir" >&2 - fi - fi - done -} - -prepare_cache() { - cache="$1" - mkdir -p "$cache/go-build" "$cache/go-mod" "$cache/golangci-lint" - echo "$ROOT" >"$cache/worktree" -} - -# Run the linter, wherever it is that this script is allowed to run it. -run_linter() { - if in_lint_container; then - golangci-lint run "$@" ./... - return $? - fi - - docker run --rm \ - --user "$(id -u):$(id -g)" \ - --env HOME=/tmp \ - --env GOFLAGS=-buildvcs=false \ - --env GOCACHE=/cache/go-build \ - --env GOMODCACHE=/cache/go-mod \ - --env GOLANGCI_LINT_CACHE=/cache/golangci-lint \ - --volume "$ROOT:/src" \ - --volume "$CACHE:/cache" \ - --workdir /src \ - "$IMAGE" \ - golangci-lint run "$@" ./... -} - -# Run the linter, streaming its combined output while also capturing it, -# and hand back its exit status. The output has to be inspected before -# it is believed, which is why this script no longer just execs the -# linter. `tee` would swallow the status, so it is smuggled out through -# a file: there is no pipefail in POSIX sh. -run_capture() { - capture="$1" - shift - rm -f "$capture.status" - { - rc=0 - # `set -e` is in force inside this subshell too, so the status - # has to be caught here: an unguarded non-zero exit (which is - # what "the linter found something" looks like) would abort the - # subshell before the status was ever written. - run_linter "$@" 2>&1 || rc=$? - echo "$rc" >"$capture.status" - } | tee "$capture" - - if [ ! -s "$capture.status" ]; then - echo "lint: the linter did not report an exit status" >&2 - exit 1 - fi - read -r captured_status <"$capture.status" - rm -f "$capture.status" - return "$captured_status" -} - -# Reject output that cannot describe this tree. See script/lint-audit -# for what that means and why: in short, a finding citing a file that is -# not here means the result being reported was produced somewhere else, -# and a PASS built out of another checkout's analysis is silent (issue -# #99). The audit therefore runs on clean output as well. -audit_output() { - capture="$1" - if ! "$ROOT/script/lint-audit" "$capture"; then - if [ -n "$CACHE" ]; then - echo " this tree's lint cache: $CACHE" >&2 - fi - exit 1 - fi +script/lint takes no arguments. The linter runs as a build step, so +there is no command line to pass flags to; anything accepted here would +have to be silently dropped. To apply autofixes, use script/lint-fix, +which runs the same pinned image as a container for exactly this +reason. +EOF + exit 2 } main() { + [ "$#" -eq 0 ] || usage + cd "$ROOT" + require_docker - IMAGE="$(lint_image)" - if [ -z "$IMAGE" ]; then - echo "lint: no lint stage found in $DOCKERFILE" >&2 - exit 1 - fi + # A fresh epoch per invocation is what forces the check layers to + # execute; the layers above the ARG in Dockerfile.lint still cache, + # so a run is not cold. The value must be unique per invocation, not + # per second: `date +%s` is second-granular, so two concurrent + # invocations in the same second would get identical epochs and the + # later one could be served from cache -- the false green in + # miniature. `%N` alone does not fix it either, because busybox + # silently drops %N, exits 0, and hands back second granularity with + # no warning. `$$` is what makes this correct regardless, since + # concurrent invocations have different pids. + # + # Assign it on its own line rather than inline in the argument. + # Under `set -eu` a command substitution that fails inside an + # argument does NOT abort the script: CHECK_EPOCH would become an + # empty string, an empty string is a constant, and a constant epoch + # is exactly the cached-lint false green this guards against. As a + # bare assignment, `set -e` catches a failing `date` and no build + # starts. + epoch="$(date +%s%N)$$" - CACHE="" - if in_lint_container; then - # No docker daemon in here, so there is no fallback: a mismatch - # is a hard error rather than a quiet substitution. - installed="$(installed_version)" - pinned="$(pinned_version)" - if [ -z "$installed" ] || [ "$installed" != "$pinned" ]; then - cat >&2 <} -EOF - exit 1 - fi - else - require_docker "$IMAGE" - prune_dead_caches - CACHE="$(cache_dir)" - prepare_cache "$CACHE" - fi - - capture="$(mktemp "${TMPDIR:-/tmp}/vaultik-lint.XXXXXX")" - trap 'rm -f "$capture" "$capture.status"' EXIT HUP INT TERM - - attempt=1 - while :; do - status=0 - run_capture "$capture" "$@" || status=$? - - if grep -Fq "$LOCK_MESSAGE" "$capture"; then - if [ "$attempt" -lt "$LOCK_ATTEMPTS" ]; then - echo "lint: another golangci-lint holds the lock;" \ - "retrying in ${LOCK_SLEEP}s" \ - "(attempt $attempt of $LOCK_ATTEMPTS)" >&2 - sleep "$LOCK_SLEEP" - attempt=$((attempt + 1)) - continue - fi - cat >&2 < -# -# Exits 0 when every finding cites a file in this tree, 1 when any does -# not. It NEVER certifies that a lint run passed - it has no idea -# whether the run found issues, and does not look. It only rejects -# output that is impossible for this tree, which is a different and much -# weaker claim. Do not use it as a gate; use script/lint. -# -# Why this exists (issue #99): golangci-lint caches analysis results, -# and a cache shared between two checkouts of this repo can serve one -# checkout's stored findings for another, file paths included. The -# failure is symmetric and only one direction is loud - a clean tree -# failed by a dirty sibling gets investigated, while a dirty tree passed -# by a clean sibling is silent. This turns the silent direction into a -# hard error, which is why it runs on clean output too. -# -# The primary fix is that script/lint now keys its cache on the worktree -# path so the collision cannot happen. This is the backstop, because a -# backstop that only runs when we already believe things are fine is -# worth more than one more assumption. -set -eu - -ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" - -# Where script/lint bind-mounts the tree inside the pinned image. A -# containerized run that prints absolute paths (`--path-mode abs`) -# prints them under this, so they are this tree's files under another -# name. Note the consequence, and why the cache key rather than this -# check is the real fix: two containerized runs of different checkouts -# both call themselves /src, so contamination between two container -# runs is not distinguishable by path alone. -CONTAINER_ROOT="/src" - -usage() { - echo "usage: $(basename "$0") " >&2 - exit 2 -} - -# Every path cited by a finding that is not a file in this tree. -# -# The linter runs with the tree root as its working directory, so a -# legitimate finding cites either a relative path that resolves inside -# the tree or an absolute path under the root. A path that escapes -# (absolute and elsewhere, or with a `..` component) or that names a -# file which is not here describes something this run did not analyse. -foreign_paths() { - capture="$1" - awk -F: '$1 ~ /\.go$/ && $2 ~ /^[0-9]+$/ { print $1 }' "$capture" | - sort -u | - while IFS= read -r path; do - case "$path" in - "$ROOT"/*) - path="${path#"$ROOT"/}" - ;; - "$CONTAINER_ROOT"/*) - path="${path#"$CONTAINER_ROOT"/}" - ;; - /*) - printf '%s\n' "$path" - continue - ;; - ../* | */../*) - printf '%s\n' "$path" - continue - ;; - esac - if [ ! -e "$ROOT/$path" ]; then - printf '%s\n' "$path" - fi - done -} - -main() { - [ "$#" -eq 1 ] || usage - capture="$1" - if [ ! -f "$capture" ]; then - echo "lint-audit: no such capture file: $capture" >&2 - exit 2 - fi - - foreign="$(foreign_paths "$capture")" - if [ -z "$foreign" ]; then - exit 0 - fi - - cat >&2 <&2 - cat >&2 <&2 + exit 1 + fi + + # Run as the invoking user so the rewritten files stay owned by + # them. HOME is set because the Go and golangci-lint caches default + # under it and that user has no home inside the container; those + # caches are per-container and discarded with it. + docker run --rm \ + --user "$(id -u):$(id -g)" \ + --env HOME=/tmp \ + --env GOFLAGS=-buildvcs=false \ + --volume "$ROOT:/src" \ + --workdir /src \ + "$image" \ + golangci-lint run --config .golangci.yml --fix "$@" ./... } main "$@"