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 <