From be593765224e3210657865d6f34cf05a54e1f86f Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 15:23:50 +0000 Subject: [PATCH] Make the pinned golangci-lint actually reach the host (closes #28) REPO_POLICIES.md now carries the canonical script/bootstrap snippet for Go repos alongside the .golangci.yml bullet, where the pinned linter version already lives. The guard it replaces, `if missing golangci-lint; then go install ...; fi`, tests PATH presence and never version, so on any already-provisioned machine the pin is inert and a version bump is a no-op. The Dockerfile installs unconditionally into a clean image, so CI and local then disagree about what the linter is: a local `make check` green while `make docker` rejects the same commit, and a container run surfacing findings the host run cannot see. Comparing versions alone is not enough. `go install` writes to GOBIN (or GOPATH/bin) while callers resolve through PATH, so a shadowing binary earlier in PATH lets the install succeed and change nothing a caller ever sees, while bootstrap prints success. The canonical form therefore compares the installed version against the pin, re-resolves through PATH after installing and asserts the pin, and treats any unparseable --version output as a mismatch so the failure direction is a redundant install rather than a skipped one. When the assert fails the diagnosis is derived from the resolved path rather than asserted: a path outside the install directory is shadowing and the operator is told to remove it or reorder PATH; a path inside it is not, and saying so would send them after a fault that does not exist; no resolution at all means the install directory is simply absent from PATH. The snippet ends with a call site. Two function definitions and no invocation are a silent no-op with exactly the shape this change exists to close, and the adoption instructions say to paste the block verbatim. The version helper ends in `|| true` so a --version that exits non-zero cannot kill the script through `set -e` under `set -o pipefail` before the diagnostic is printed, which the styleguide's bash form would otherwise do. The policy text states each of those as a requirement rather than leaving them implicit in the code, records why the commit-pinned `go install` ref satisfies the hash-pinning rule (a commit hash is not a mutable tag, and the checksum database verifies the fetch; no repo go.sum is consulted, since `go install pkg@version` ignores the go.mod in the current directory or any parent), and requires the pin to be a tagged release commit: an untagged pin yields a pseudo-version that the parser truncates at the first `-`, so no value of GOLANGCI_LINT_VERSION derived from what --version prints can ever match. Verification requires two things the previous round lacked: a negative control against a shadowing binary, and running the controls against the block as a consuming repo would adopt it, pasted into a script/bootstrap-shaped file and executed, rather than sourcing it and calling the function directly. The node and yarn handling described earlier in the document is untouched. --- TODO.md | 8 ++ prompts/REPO_POLICIES.md | 160 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+) diff --git a/TODO.md b/TODO.md index 9e3b80b..8d9557d 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,14 @@ fmt-check, and commit. # Completed Steps +- 2026-08-09: Made the pinned golangci-lint actually propagate: REPO_POLICIES.md + now carries the canonical `script/bootstrap` snippet for Go repos, which + installs when the installed version does not match the pin (the old + `if missing` guard tested PATH presence only, so pins were inert on any + provisioned machine and CI silently disagreed with local) and then re-resolves + the binary through `PATH` and fails loudly, naming the shadowing path, when + the install did not take effect — the failure mode the naive + compare-then-install fix leaves behind while reporting success. - 2026-08-09: Fixed the false green in the canonical CI gate: `script/cibuild` and `script/docker` now pass a per-invocation `CHECK_EPOCH` nonce, and the `Dockerfile` (plus the Go multistage template in REPO_POLICIES.md, in both its diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index 82f6ae9..38e9472 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -354,6 +354,166 @@ style conventions are in separate documents: commit-pinned via `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5`. +- **`script/bootstrap` in Go repos must install the pinned golangci-lint + whenever the installed version does not match the pin — not merely when the + binary is absent — and must then verify the install took effect by + re-resolving the binary through `PATH`.** The presence test + `if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi` is wrong: + it tests `PATH` presence and never version, so on any already-provisioned + machine the pin is inert and a version bump is a no-op. Meanwhile the + Dockerfile installs unconditionally into a clean image, so CI and local + silently disagree about what the linter even is. Observed consequences: a + local `make check` green while `make docker` rejected the same commit with six + `goconst` findings, and a container linter surfacing thirteen findings the + host run missed. A stale host linter does not merely fail to prove the tree is + clean — it hides findings only the container can see. This is a deliberate + departure from the node handling described above, which uses whatever node is + installed: the linter version is the specific thing being held equal between + host and container, so for it, presence is not enough. + + Comparing versions is necessary but **not sufficient**, because the obvious + fix also fails green. `go install` writes to `GOBIN` (or `GOPATH/bin`) while + callers resolve `golangci-lint` through `PATH`. If a different binary + shadows it earlier in `PATH`, the install genuinely succeeds and changes + nothing any caller will ever see: bootstrap prints success and the next + `make lint` still runs the stale linter. That is worse than no fix, because + it converts a known-stale toolchain into one everyone believes is pinned. + The canonical form, placed in `script/bootstrap` after Go itself is present: + + ```sh + # golangci-lint v2.12.2, 2026-05-06. The ref must resolve to a TAGGED + # release; see the note on untagged pins below. + GOLANGCI_LINT_VERSION="2.12.2" + GOLANGCI_LINT_REF="github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5" + + # The version golangci-lint reports, resolved the way callers resolve it. + # Prints nothing when the binary is absent, exits non-zero, or prints + # something unparseable: all of those must read as "does not match". + # The trailing `|| true` is required, not tidiness. Under `set -o pipefail` + # a non-zero --version would otherwise propagate out of the pipeline and + # kill the script through `set -e` before the diagnostic below is printed. + golangci_lint_version() { + command -v golangci-lint >/dev/null 2>&1 || return 0 + golangci-lint --version 2>/dev/null | head -n 1 | + sed -n 's/.*has version v\{0,1\}\([0-9][0-9.]*\).*/\1/p' || true + } + + ensure_golangci_lint() { + if [ "$(golangci_lint_version)" = "$GOLANGCI_LINT_VERSION" ]; then + return 0 + fi + echo "bootstrap: installing golangci-lint $GOLANGCI_LINT_VERSION" + go install "$GOLANGCI_LINT_REF" + + # go install writes to GOBIN (or GOPATH/bin); callers resolve through + # PATH. Re-resolve through PATH and assert the install took effect. + # `hash -r` is load-bearing: without it a shell that already resolved + # a stale golangci-lint answers from its own lookup cache, and this + # check false-fails with the shadowing message below. + hash -r 2>/dev/null || true + gcl_got="$(golangci_lint_version)" + if [ "$gcl_got" = "$GOLANGCI_LINT_VERSION" ]; then + return 0 + fi + gcl_bin="$(go env GOBIN)" + [ -n "$gcl_bin" ] || gcl_bin="$(go env GOPATH)/bin" + gcl_found="$(command -v golangci-lint 2>/dev/null || true)" + echo "bootstrap: installed golangci-lint $GOLANGCI_LINT_VERSION into" \ + "$gcl_bin, but that is not what callers will get." >&2 + case "$gcl_found" in + "") + echo "bootstrap: PATH resolves no golangci-lint at all." \ + "Add $gcl_bin to PATH, then re-run bootstrap." >&2 + ;; + "$gcl_bin"/*) + echo "bootstrap: PATH resolves $gcl_found, inside that same" \ + "directory, reporting version ${gcl_got:-unparseable}." \ + "Nothing is shadowing it, so the install itself did not" \ + "produce the pinned version: check that" \ + "GOLANGCI_LINT_VERSION matches GOLANGCI_LINT_REF." >&2 + ;; + *) + echo "bootstrap: PATH resolves $gcl_found instead, reporting" \ + "version ${gcl_got:-unparseable}. Remove that binary or" \ + "put $gcl_bin earlier in PATH, then re-run bootstrap." >&2 + ;; + esac + exit 1 + } + + # The definitions above are inert on their own; the call site is part of + # the canonical form. In a script/bootstrap that follows the "define all + # functions, then call main" convention, this line belongs inside main() + # next to the other ensure_* steps. + ensure_golangci_lint + ``` + + Four properties are load-bearing; each guards a failure mode that otherwise + fails green: + - **Compare the installed version against the pin**, never test presence. + This is what makes a version bump propagate to machines that already have + some golangci-lint. + - **After installing, re-resolve the binary the way callers resolve it** — + through `PATH`, not the path `go install` wrote to — and assert + `--version` reports the pin. When it does not, fail non-zero and name the + path `command -v` actually found, the version it reports, and the + directory the install wrote to. That is a condition a human has to fix by + hand, so bootstrap must not print success in it. Use `hash -r` first so + the shell does not answer from its own lookup cache. Diagnose the cause + from the resolved path rather than asserting one: only a path **outside** + the install directory is shadowing. When the resolved path is inside it, + nothing is shadowing and telling the operator to delete that binary or + reorder `PATH` sends them after a fault that does not exist. + - **A mis-parse must fall through to reinstall, never to a false match.** + Absent binary, non-zero exit, empty output, and unrecognised output all + yield an empty string, which compares unequal to the pin. The failure + direction is always a redundant install, never a skipped one. + - **Call it.** Two function definitions with no call site are a silent no-op + that reproduces the original defect exactly: exit 0, nothing installed, no + output, stale linter still resolved. + + Keep it POSIX sh: no bashisms, no arrays, no `[[`, no `grep -P`. + + **On the hash-pinning rule.** `@c0d3ddc9cf3faa61a4e378e879ece580256d76e5` is + a commit hash, not a server-mutable version tag, and the go command verifies + the fetched module against the checksum database — the mechanism the + hash-pinning rule at the top of this document already names as acceptable + for Go modules. Note that `go install pkg@version` runs in module-aware mode + ignoring the `go.mod` in the current directory or any parent, so no repo + `go.sum` is consulted for this install; the checksum database is what + verifies it. The linter is a bootstrap prerequisite rather than part of any + repo's module graph, which is why the canonical form installs it by + commit-pinned ref instead of declaring it in `go.mod`. + + **The pin must be a tagged release commit.** `GOLANGCI_LINT_VERSION` is a + separate string because the ref is a hash and carries no readable version, + so the two must be updated together. It works out to a clean `2.12.2` only + because that commit is the `v2.12.2` tag commit: the go command resolves a + commit query to a semver tag on that commit when one exists, so the built + binary reports `2.12.2`. Pin to an **untagged** commit and the go command + falls back to a pseudo-version — the binary then reports something like + `2.12.3-0.20260506110758-c0d3ddc9cf3f`, and this comparison cannot express + it: the parser above stops at the first `-`, so the value to match would + have to be the truncated numeric prefix `2.12.3`, which is also the number + of a real future release. Setting `GOLANGCI_LINT_VERSION` to the full string + `--version` prints can never match, and bootstrap would then reinstall on + every run and exit 1 blaming a shadowing binary that does not exist. Keep + pins on tagged releases. + + **Verifying a change to this logic requires a negative control run in an + environment where a shadowing binary exists earlier in `PATH` than the + install target.** Without that, the control passes against the naive + compare-then-install form as well and therefore proves nothing. Also check + the mis-parse direction by feeding it unparseable `--version` output and + confirming it reinstalls rather than reporting a match. + + **Run those controls against the block as a consuming repo would adopt it** + — pasted into a `script/bootstrap`-shaped file that is then executed — not + by sourcing it and invoking the function yourself. Driving the function + directly tests something the artifact does not do, and it is exactly how a + missing call site passes every control while the adopted snippet does + nothing. + - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD).