From 043aabbd274be677f1beee36383f53d79f569f25 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 14:47:51 +0000 Subject: [PATCH] Isolate the lint cache per worktree and context-gate the native path (closes #99) Closes #80. script/lint decided whether it could skip the pinned image by asking what version was on PATH rather than where it was running, and cache isolation is part of that same question. Both issues are that one defect. The cache was one directory per repo, shared by every worktree on the host. Two checkouts of this repo have identical Go file contents, so their cache keys collide and golangci-lint replays the stored analysis, including the paths recorded when it was produced. The loud direction of that failure - a clean tree failed by a dirty sibling - is the harmless one. The silent direction, a dirty tree passed by a clean sibling, is another way for a gate here to report a green it did not earn. The cache is now keyed on a digest of the worktree path, so a collision is not possible, and it stays persistent per worktree: a warm run is still seconds. Each cache records the worktree it belongs to and is collected when that worktree is gone, so throwaway worktrees do not accumulate caches; the tree lives under XDG_CACHE_HOME and is disposable by definition. Collection makes the tree writable first, since the Go module cache inside it is deliberately read-only, and never lets a failure to tidy up fail the lint: the first cut did exactly that, and a gate failing for a housekeeping reason is its own bug. script/lint-audit is the backstop, and runs on every lint: it rejects output citing any file that is not in the tree being linted, so a result built out of another checkout's analysis is a hard error instead of a silent pass. It runs on clean output too, because that is the case nobody investigates. It never certifies that a run passed - it does not look at whether there were findings - so it cannot itself become a gate that reports a green. golangci-lint's "parallel golangci-lint is running" refusal is now a bounded retry rather than a verdict. It is not a lint result, and exiting non-zero on it is indistinguishable to a caller from real findings; issue #88 measured that a private cache does not remove the contention. Exhausting the retries fails with a message that says the tree was never analysed. The native path now requires VAULTIK_LINT_IN_CONTAINER=1, which only the Dockerfile's lint stage sets, in addition to matching the pin. A developer's locally installed 2.12.2 is a different build reached by a different code path and no longer bypasses the digest pin. /.dockerenv was considered and rejected as the signal: 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. Inside the container a version mismatch is now a hard error rather than a fall-through, since there is no daemon there to fall through to. Version detection uses `golangci-lint version --short`, the interface meant for it, keeping the banner scrape only as a fallback. script/bootstrap no longer prints "bootstrap complete" on a machine that cannot run the gate. Docker missing, or present with an unreachable daemon, is a hard failure naming exactly what breaks. Installing docker from bootstrap was rejected: it needs root, a daemon, and on macOS a GUI cask, so the attempt would itself fail in the common case and trade one false success for a second failure mode. TODO.md's claim that `make check` became "as trustworthy as script/cibuild" is corrected to what README.md already said: only the lint leg is equivalent, while tests and gofmt still run against the host toolchain. README.md's requirements section gains docker and sqlite3. Verified by reproduction, not inspection: two concurrent lints from two worktrees of differing cleanliness each reported only their own findings with no lock error; a real run made to report paths outside its tree exits 1; a matching linter shimmed onto PATH is never invoked while the pinned image runs; a PATH without docker makes bootstrap fail. script/ cibuild exits 0 with the lint layer executing in the pinned image, which is what proves the in-container path still works. --- Dockerfile | 9 ++ README.md | 6 ++ TODO.md | 44 +++++++- script/bootstrap | 58 +++++++++- script/lint | 267 +++++++++++++++++++++++++++++++++++++++++----- script/lint-audit | 113 ++++++++++++++++++++ 6 files changed, 461 insertions(+), 36 deletions(-) create mode 100755 script/lint-audit diff --git a/Dockerfile b/Dockerfile index 013babe..f6fe069 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,6 +10,15 @@ FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f01691 RUN apk add --no-cache make build-base +# 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 + WORKDIR /src # Copy go mod files first for better layer caching diff --git a/README.md b/README.md index 1033685..5a0b16e 100644 --- a/README.md +++ b/README.md @@ -563,6 +563,12 @@ regardless of color setting (emoji are not color). ## requirements * 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. +* `sqlite3` CLI, which the test suite shells out to * S3-compatible object storage (or local filesystem, or rclone remote) ## development workflow diff --git a/TODO.md b/TODO.md index b8ed086..39e5708 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,43 @@ Define remaining scope for a first tagged release and cut v0.1.0. # Completed Steps +- 2026-08-09: Isolated the lint cache per worktree and context-gated the + native lint path (issues #99, #80). One defect seen twice: + `script/lint` decided whether it could skip the pinned image by asking + what version was on `PATH` rather than where it was running, and cache + isolation is part of that same question. The cache was one directory + per repo, shared by every worktree on the host, so two checkouts with + identical Go file contents collided and golangci-lint replayed the + stored analysis — paths and all. The loud direction of that failure + (a clean tree failed by a dirty sibling) is the harmless one; the + silent direction, a dirty tree **passed** by a clean sibling, is a + sixth way for a gate here to report a green it did not earn. The cache + is now keyed on a digest of the worktree path, and every run is + audited by the new `script/lint-audit`, which rejects output citing any + file that is not in the tree being linted — a backstop that runs on + clean output too, because that is the case nobody investigates. Caches + record the worktree they belong to and are collected when it + disappears, so throwaway worktrees do not accumulate them; the whole + tree lives under `XDG_CACHE_HOME` and is disposable. The + `parallel golangci-lint is running` refusal is now a bounded retry + rather than a verdict: it is not a lint result, and exiting non-zero + on it is indistinguishable to a caller from real findings (#88 showed + a private cache does not remove that contention). The native path now + requires `VAULTIK_LINT_IN_CONTAINER=1`, set only by the `Dockerfile` + lint stage, in addition to matching the pin, so a developer's locally + installed 2.12.2 no longer bypasses the digest pin; `/.dockerenv` was + rejected as the signal because `dockerd` creates it for `docker run` + and it is not reliably present during a BuildKit `docker build`, which + is the case the exception exists for. Version detection uses + `golangci-lint version --short` with the old banner scrape kept only + as a fallback. `script/bootstrap` no longer prints `bootstrap + complete` on a machine that cannot run the gate: a missing docker, or + one whose daemon is unreachable, is a hard failure naming exactly what + breaks. Verification was by reproduction rather than inspection — two + concurrent lints from two worktrees of differing cleanliness, a real + run made to report an outside path, a matching linter shimmed onto + `PATH`, and a `PATH` with docker removed — and is recorded on the pull + request. - 2026-08-09: Closed the fifth false-green mechanism (issues #93, #69). `script/test` omitted `-count=1`, so Go's test result cache could satisfy the gate outright: a second back-to-back `make test` printed @@ -198,8 +235,11 @@ Define remaining scope for a first tagged release and cut v0.1.0. exactly the pinned one (which is how the lint stage runs it inside the container); anything else goes through Docker, and a missing or unreachable Docker daemon is a hard error rather than a silent - fallback. `make check` is therefore now as trustworthy as - `script/cibuild`. + fallback. Only the **lint** leg of `make check` became equivalent to + `script/cibuild`; its tests and `gofmt` still run on the host against + the host toolchain, as `README.md` states. An earlier version of this + entry claimed `make check` was "as trustworthy as `script/cibuild`" + outright, which overstated it; corrected under issue #80. - 2026-08-09: Finished the lint remediation under the canonical `.golangci.yml` (issue #61, which also unblocks issue #59). The remaining findings were fixed behavior-preservingly: `wsl_v5` diff --git a/script/bootstrap b/script/bootstrap index c41f677..aa6b2c2 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -48,6 +48,52 @@ missing() { ! command -v "$1" >/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. +# +# 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 +# fail in the common case - trading one false success for a second +# failure mode. Naming exactly what breaks is more useful. +# Prints the problem and returns 0 when docker cannot be used; returns +# 1 (and prints nothing) when it can. +docker_problem() { + if missing docker; then + echo "docker is not installed" + return 0 + fi + if ! docker info >/dev/null 2>&1; then + echo "the docker daemon is not reachable" + return 0 + fi + return 1 +} + +require_docker() { + reason="$(docker_problem)" || return 0 + cat >&2 <&2 - echo "bootstrap: the pinned linter (see the Dockerfile lint stage)" >&2 - fi + # 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. # sqlite3 CLI: the test suite shells out to it (VACUUM). if missing sqlite3; then pkg_install sqlite sqlite3 sqlite sqlite; fi go mod download + # Last, so that everything installable is installed before the one + # thing this script cannot install decides the outcome. + require_docker + echo "bootstrap complete" } diff --git a/script/lint b/script/lint index 3819049..321b5b0 100755 --- a/script/lint +++ b/script/lint @@ -8,12 +8,24 @@ # needs editing. # # Normally that means running the pinned image with docker. The one -# exception is a golangci-lint on PATH whose version is exactly equal to -# the pin: that is the same linter, so it is run directly. This is what -# happens inside the lint container itself (Dockerfile runs `make lint`, -# and there is no docker daemon in there). A PATH binary at any other -# version is never used - that silent substitution is the bug this -# script exists to prevent. +# 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. +# +# 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. # # Extra arguments are passed through to `golangci-lint run`, before # `./...` (see script/lint-fix). @@ -22,6 +34,15 @@ 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... @@ -36,8 +57,24 @@ pinned_version() { } # 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++) { @@ -49,6 +86,13 @@ installed_version() { }' } +# 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" ] +} + require_docker() { image="$1" if ! command -v docker >/dev/null 2>&1; then @@ -57,9 +101,10 @@ lint: docker is required to run the pinned linter. pinned image: $image -Install docker, or install golangci-lint $(pinned_version) on PATH. -Linting with any other version is not supported: it is what lets a -local run pass while CI fails. +Install docker. Linting with any other golangci-lint is not supported: +it is what lets a local run pass while CI fails. An installed +golangci-lint on PATH is not used, whatever its version; only the lint +stage of the Dockerfile itself runs the linter natively. EOF exit 1 fi @@ -70,30 +115,102 @@ run. pinned image: $image -Start the daemon (and check DOCKER_HOST / your group membership), or -install golangci-lint $(pinned_version) on PATH. This script will not -fall back to a different linter version. +Start the daemon (and check DOCKER_HOST / your group membership). This +script will not fall back to a different linter version or to an +unpinned binary on PATH. EOF exit 1 fi } -# Caches for the containerized linter. 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. -cache_root() { +# Where the per-worktree caches live. +cache_home() { echo "${XDG_CACHE_HOME:-${HOME:-/tmp}/.cache}/vaultik-lint" } -run_in_docker() { - image="$1" - shift - require_docker "$image" +# A short, stable digest of this worktree's path. +path_digest() { + if command -v sha256sum >/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 +} - cache="$(cache_root)" +# 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" +} - exec docker run --rm \ +# 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 \ @@ -101,26 +218,118 @@ run_in_docker() { --env GOMODCACHE=/cache/go-mod \ --env GOLANGCI_LINT_CACHE=/cache/golangci-lint \ --volume "$ROOT:/src" \ - --volume "$cache:/cache" \ + --volume "$CACHE:/cache" \ --workdir /src \ - "$image" \ + "$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 +} + main() { cd "$ROOT" - image="$(lint_image)" - if [ -z "$image" ]; then + IMAGE="$(lint_image)" + if [ -z "$IMAGE" ]; then echo "lint: no lint stage found in $DOCKERFILE" >&2 exit 1 fi - if [ "$(installed_version)" = "$(pinned_version)" ]; then - exec golangci-lint run "$@" ./... + 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 - run_in_docker "$image" "$@" + 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 <