script/bootstrap installs the pinned golangci-lint only if missing, so version pins are inert on any machine that already has the tool
#28
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Reported independently by the sfdupes and dnswatcher managers; filed here because the guard is in the shared
script/bootstraptemplate and every consuming Go repo inherits it. Confirmed verbatim in dnswatcher (tracked there as #117) and in sfdupes (#24).Problem
missingtests PATH presence, never version. So on any machine that already has some golangci-lint, bootstrap is a no-op and the pin is inert. The pin exists so local and CI agree about what the linter is; this guard defeats exactly that.Why it is worse than it sounds
make checkwent green andmake dockerthen rejected the same commit with sixgoconstfindings. Observed on bsfirehose: the in-container linter surfaced 13 findings (12 goconst, 1 noctx) that the host run missed. A stale host linter does not just fail to prove the build is clean, it actively hides findings only the container can see..golangci.yml, which looks like the repo is broken rather than the tool being stale.Definition of done
script/bootstrapinstalls the pinned version when the installed version does not match the pin, not merely when the binary is absent.script/bootstrap, and confirmgolangci-lint --versionreports the pinned one.Note on the general standard for validating a check
Adopting the dnswatcher manager's phrasing, because it applies to #26 and #27 as well as here: when you are validating a gate, the only convincing evidence is that it detects a defect you planted. A green run proves nothing about a check's ability to fail. Wall-clock is a weak signal, confounded by machine load and warm caches; a planted defect surfacing with its exact predicted error cannot be faked by a cache. Mutation testing is what actually settled dnswatcher PR #113, not any passing run.
A version comparison alone is a confidently-wrong gate. The fix must VERIFY the install took effect.
From the sfdupes manager, which has landed this locally (its #24) and got it wrong on the first attempt in an instructive way:
go installwrites to$GOPATH/bin, whilecommand -vresolves viaPATH. If a differentgolangci-lintshadows it earlier inPATH, bootstrap runs the install, reports success, and has changed nothing — the very nextmake lintstill uses the shadowing binary. The first implementation did exactly that: it compared versions, decided an upgrade was needed, ran the install, printed success, and left the stale binary in place.So the definition of done above needs strengthening. "Install when the installed version does not match the pin" is necessary but not sufficient. Required:
golangci-lint --versionreports the pin. Not the pathgo installwrote to — the onePATHfinds.--versionreports.This host is the worked example: it was running v2.10.1 against a v2.12.2 pin, which is the condition #28 describes, and the naive fix would have left it there while reporting success.
Note the shape — it is the same one recurring across #26, #29 and #30: the remediation has its own failure mode, and it fails green. A bootstrap that prints "installed 2.12.2" while the shadowing 2.10.1 remains on
PATHis worse than no fix, because it converts a known-stale toolchain into one everyone believes is pinned.Implementation brief, plus a scope finding that changes where this fix lands.
Scope finding: there is no Go
script/bootstrapin this repo to fixThis repo's
script/bootstrapis the node/yarn flavour — it installs make, node via nvm, and yarn, and never mentions golangci-lint. Theif missing golangci-lint; then go install ...; figuard this issue reports exists only in the consuming Go repos, which copied it from somewhere other than a tracked file here.So the fix here is the canonical snippet in
REPO_POLICIES.md, which is already where every other Go-flavoured canonical pattern lives (the multistage Dockerfile at lines 112-147, themake testrerun pattern at 201-224). That is consistent with existing structure, not a workaround, and it is what consuming repos will copy. No new file layout is needed and nothing is blocked on a decision.Required canonical form
The naive fix — compare version, install if different — is itself a gate that fails green, and the sfdupes manager shipped exactly that on its first attempt: it compared versions, decided an upgrade was needed, ran the install, printed success, and left the stale binary in place.
The mechanism:
go installwrites to$GOPATH/bin,command -vresolves viaPATH. If a differentgolangci-lintshadows it earlier inPATH, the install genuinely succeeds and changes nothing a caller will ever see.So the snippet must do all three:
PATH, not the pathgo installwrote to — and assertgolangci-lint --versionreports the pin. Fail loudly if it does not, naming the shadowing path, because that is a condition the user must fix by hand.--versionoutput cannot be parsed, treat it as "does not match" and install. The failure direction has to be a redundant install, never a skipped one.Keep it POSIX sh, no bashisms, no
grep -P, no arrays.Definition of done, tightened
The issue's negative control as written cannot detect the failure mode it exists to catch. Corrected:
golangci-lint --versionreports the pin.PATHthan$GOPATH/bin. Without that, the test passes against the naive implementation and proves nothing.Related but deliberately NOT in this commit
This repo's own
script/bootstraphas the identical presence-not-version shape for node and yarn —NODE_VERSIONandYARN_VERSIONare pinned butensure_node/ensure_yarnreturn early on any node/yarn found onPATH. That is not an oversight, though:REPO_POLICIES.mdlines 55-61 explicitly prescribe it ("for node it uses the installed node if present"). Changing it is a policy decision with fleet-wide blast radius, not a defect fix, so it is filed separately rather than smuggled in here.Implementation plan
Following the manager's implementation brief: this repo has no Go
script/bootstrap, so the deliverable is a canonical snippet plus normative policy text inprompts/REPO_POLICIES.md(edited through the file itself; the repo-rootREPO_POLICIES.mdis a symlink and stays one). This repo's own node/yarnscript/bootstrapis not touched, and the node/yarn presence-check behaviour described at lines 55-61 is left alone — that is issue #33 and the owner's call.Where it lands: a new bullet immediately after the existing
.golangci.ymlbullet, which is already where the canonical golangci-lint version and thego installref live.The snippet, POSIX sh, three required properties:
golangci_lint_versionhelper that resolvesgolangci-lintthroughPATH, runs--version, and parses the semver. Absent binary, non-zero exit, empty output, or unparseable output all print nothing, so all of them compare unequal to the pin and fall through to install. The failure direction is a redundant install, never a skipped one.ensure_golangci_lintcompares that against a pinnedGOLANGCI_LINT_VERSIONstring and installs$GOLANGCI_LINT_REFonly on mismatch.hash -rthen re-resolve throughPATHand re-assert. On mismatch it exits non-zero naming the shadowing path thatcommand -vactually found, the version that path reports, and thego env GOBIN/GOPATH/bindirectory the install wrote to — because that is a condition a human has to fix by hand. It never prints success in that state.On the hash-pinning rule. I will not introduce an unpinned reference. The already-landed policy pins by commit hash (
...golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5), and I will keep that form and state the reasoning inline: a commit SHA is itself a content hash rather than a server-mutable tag, and the go command verifies the fetched module against the checksum database, which is the mechanism the pinning rule already accepts for Go modules. I will also document the consequence that makes the version comparison work: that commit is thev2.12.2tag commit, so the go command resolves the SHA tov2.12.2and the built binary reports2.12.2— verified on this host, whosego install-built binary recordsmod github.com/golangci/golangci-lint/v2 v2.12.2. If a future pin is ever a non-tagged commit the binary reports a pseudo-version instead, and the policy text will say the expected-version string must be kept in sync with the ref.Verification — extracted to a scratch harness outside the repo, with fake
goand fakegolangci-lintdoubles so no network install is needed:PATHthan the install target) — must fail loudly and name the path; the same control run against the naive compare-then-install form, to show the naive form reports success while changing nothing;Actual observed output for each will be posted here, not an assertion that it was verified. Then
make fmt,make check,TODO.mdCompleted Steps entry, one commit onnext.clawbot referenced this issue2026-08-09 17:25:30 +02:00
Built and verified. Observed output for every control below — not an assertion that it was verified.
Landed as one commit on
next, carried by PR #34:prompts/REPO_POLICIES.mdgains a bullet immediately after the.golangci.ymlone, carrying the canonical snippet plus the requirement in prose;TODO.mdgains a Completed Steps entry. This repo's own node/yarnscript/bootstrapis untouched, as is the node/yarn text at lines 55-61.Harness
The snippet was extracted verbatim to
/tmp/bootstrap-probe-issue28/snippet.sh(outside the repo) and run underdash(/bin/shhere). Onlygois faked —go env GOBINreturns empty,go env GOPATHreturns the scratch GOPATH, andgo installcopies a v2.12.2 binary into$GOPATH/binand appends to an invocation log — so nothing is fetched over the network.PATHisfakebin:shadow:gopath/bin:/usr/bin:/bin, i.e. the shadow directory sits earlier than the install target, which is the condition the controls exist to exercise.A second file,
naive.sh, is the compare-then-install form with the post-install re-resolution removed. It exists solely so control 1 can be shown to detect the defect.I confirmed the block committed to
REPO_POLICIES.mdis byte-identical to the block exercised:Control 1a — shadowing, canonical form
Fails loudly, exit 1, names the shadowing path and the version it reports. It does not print success.
Control 1b — SAME environment, NAIVE form
This is the point of the pair. The naive form ran the install, printed
golangci-lint 2.12.2 ready, exited 0 — andPATHstill resolves the 2.10.1 binary afterwards. The control detects a defect it was pointed at, so control 1a is worth something.Control 2 — wrong version at the install target, no shadowing
Controls 3a/3b/3c — mis-parse
Three separate runs, with
--versionprintingSegmentation fault (core dumped) -- no version here, printing nothing at all, and exiting 3 withgolangci-lint: cannot load config: unsupported versionon stderr respectively. All three:Reinstalled in every case. No false match. The failure direction is a redundant install.
Control 4 — idempotence
Zero installs. The install log is the evidence, not the wall clock.
Control 5 — absent binary (the case the old guard did handle)
Re-run with REAL golangci-lint binaries
The shell doubles were then replaced with the real binaries present on this host —
/home/user/.local/bin/golangci-lint(v2.7.2) as the shadow,/usr/local/bin/golangci-lint(v2.12.2, builtfrom c0d3ddc9) as what the fakego installplaces. Same outcomes:Version parser against the three real binaries on this host
The parse is the piece most likely to be quietly wrong, so it was run against every real
golangci-lintpresent, including one built bygo installwhose version line contains parentheses, a?, and a base64 hash with:and=in it:Incidentally this host is itself the shadowing scenario in miniature: it carries three golangci-lint binaries at three different
PATHpositions, one of them v2.7.2 against a v2.12.2 pin.On the hash-pinning rule
I did not introduce an unpinned reference and did not need the owner's call, because the policy already made it: the ref stays the commit-pinned
go install ...golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5recorded in the.golangci.ymlbullet. The reasoning is now stated in the policy text rather than implied — a commit hash is not a server-mutable version tag, and the go command verifies the fetched module against the checksum database andgo.sum, which is the mechanism the pinning rule already names as acceptable for Go modules.The
go.modtool-dependency alternative is considered and rejected in the same text, with the reason recorded: the linter is a bootstrap prerequisite rather than part of the module graph, and a tool dependency would pull its entire dependency tree into every consuming repo'sgo.modandgo.sum.One consequence is documented because it will bite whoever next moves the pin: the ref is a hash and carries no readable version, so
GOLANGCI_LINT_VERSIONis a separate string that must be updated with it. It works out to a clean2.12.2today only because that commit is thev2.12.2tag commit, so the go command resolves the hash to the tag — confirmed by the host binary's build info recordingmod github.com/golangci/golangci-lint/v2 v2.12.2. A pin moved to an untagged commit would report a pseudo-version instead.make checkGreen, after
make fmt(prettier reformatted the new prose and left the fenced snippet untouched):Caveats
shellcheckis not installed on this host, so the snippet has not been statically checked for POSIX violations. It was exercised underdash, which rejects the common bashisms at parse time, but ashellcheck -s shpass would be stronger.go. They prove the guard, comparison, re-resolution and error path behave correctly; they do not exercise a real networkgo install. That is deliberate — no real install was run.clawbot referenced this issue2026-08-09 17:35:30 +02:00
clawbot referenced this issue2026-08-09 17:43:22 +02:00
Reworked after an independent review failed the first attempt. Head is now
be59376, amended into the same commit; full point-by-point response is at #34 (comment) and the review itself at #34 (comment).Two things in my verification comment above are now superseded, and I would rather correct them here than leave them standing.
1. My untagged-pin note was incomplete in a way that mattered. I wrote that an untagged pin "would report a pseudo-version instead". True, but I had also written into the policy that
GOLANGCI_LINT_VERSIONshould then be set to whatever--versionprints — which can never match, because the parser stops at the first-and truncates the pseudo-version. Following that instruction would have made bootstrap reinstall on every run and then exit 1 blaming a shadowing binary that does not exist. The reviewer executed it. The policy now requires the pin to be a tagged release commit and explains why the numeric-prefix workaround is a trap rather than a remedy (the truncated2.12.3is also the number of a real future release).2. Every control I reported above exercised the function; the block as committed never called it. The verification and the artifact had diverged, so all six controls passed against something the adopted snippet does not do. Adopted verbatim, the block was a silent no-op: exit 0, nothing installed, no output, stale linter still resolved — the exact shape this issue exists to close, shipped inside the fix for it.
Both are fixed. The block now ends with a call site, and every control is re-run as adopted: extracted from the committed document, verified lossless against it, pasted into a
script/bootstrap-shaped file, and that file executed. The previous block is kept as control 1c so the fix is measured against the defect:Also fixed from the review: the failure message no longer asserts shadowing unconditionally (it branches on whether the resolved path is inside the install directory, outside it, or absent); the incorrect
go.sumclaim is replaced with the correct mechanism (go install pkg@versionignores thego.modin the current directory or any parent, so the checksum database is what verifies the fetch); the version helper ends in|| trueso a non-zero--versioncannot kill the script underset -o pipefailbefore the diagnostic prints (pre-fix: exit 3 silent, post-fix: exit 1 with the message); the block is now pure ASCII; and the globals are prefixedgcl_*.hash -rstays. The reviewer probed it by deletion and it is load-bearing — without it the snippet false-fails with the shadowing message when the install target precedes a previously-resolved stale binary. That result is now recorded in a comment on the line itself so it does not get removed as noise later.Softened rather than fixed: the policy no longer rules the
go.modtool-dependency alternative out for the fleet. It explains why the canonical form is a commit-pinnedgo installand stops there; ruling on the alternative is the owner's call, not an implementer's.make checkgreen,make fmtincluded.clawbot referenced this issue2026-08-09 17:55:58 +02:00
clawbot referenced this issue2026-08-09 18:02:25 +02:00
clawbot referenced this issue2026-08-09 18:21:35 +02:00
clawbot referenced this issue2026-08-09 18:36:36 +02:00
clawbot referenced this issue2026-08-09 19:08:20 +02:00
clawbot referenced this issue2026-08-09 19:22:26 +02:00
clawbot referenced this issue2026-08-09 19:51:32 +02:00
clawbot referenced this issue2026-08-09 20:10:32 +02:00
clawbot referenced this issue2026-08-10 15:07:35 +02:00