Check the golangci-lint version in bootstrap, not just presence (closes #24) #34
Reference in New Issue
Block a user
Delete Branch "bootstrap-version-check"
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?
Closes #24.
script/bootstrapinstalled the pinned linter only when the command wasabsent (
if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi), so on any host that already had somegolangci-lintthe pin wasnever consulted and a version bump was inert forever. This host was a
live reproduction: v2.10.1 installed against a v2.12.2 pin,
make checkgreen,
make dockerrejecting the same commit.What changed
GOLANGCI_LINT_VERSION(
2.12.2, no leadingv, the way the binary reports it), withGOLANGCI_LINT_MODULEandGOLANGCI_LINT_REFderived from it. Afuture bump edits one string; the install ref and the comparison value
cannot drift apart (DoD 2).
golangci_lint_version()helper: returns early with no output ifcommand -v golangci-lintfails, otherwise parsesgolangci-lint --versionwithawk, taking the field after the literal wordversionand stripping an optional leadingv. That covers bothoutput shapes seen in practice — this host's
golangci-lint has version 2.10.1 built with go1.26.5 from (unknown, modified: ?, mod sum: "...") on (unknown)and the pinned image's version-plus-commitline.
not the pin — older, newer, absent, or unparseable — is reinstalled,
and the mismatch is printed before installing so the upgrade is
visible in the bootstrap output.
git,makeandgokeep theirpresence-only checks and now carry a comment saying that is
deliberate — they come from the host package manager, the repo pins no
system toolchain versions,
go.modgoverns the language version, andreproducible builds are the digest-pinned Docker images' job. Nothing
was newly pinned.
POSIX
shthroughout; no bashisms (verified underdash, see below).Verification
1. Wrong-version host upgrades. Before:
golangci-lint has version 2.10.1 built with go1.26.5 .... Runningscript/bootstrap:After:
golangci-lint has version 2.12.2 built with go1.26.5 ...(DoD 4, first half).
2. Second run is a no-op. Re-running prints only
bootstrap complete— no mismatch line, no install — and the version is unchanged(DoD 4, second half).
3.
make lintnow uses the pinned version.golangci-lint --versionreports 2.12.2, the same version the Dockerfile lint stagepins (
golangci/golangci-lint:v2.12.2@sha256:5cceeef0...).make lint:0 issues.(DoD 5).4.
make checkgreen with the correct linter:v2.12.2 surfaced no findings that v2.10.1 was hiding on
main—nothing to report as a follow-up on that front.
5.
make dockergreen, and the build really ran. Guarding againstthe cached-no-op failure mode of #32: the build log (captured with
BUILDKIT_PROGRESS=plain) contains exactly threeCACHEDsteps, all ofthem
WORKDIR /src. Every gate step executed with real output and realwall-clock time:
The
COPY . .layer changed with this commit, which is what invalidatedthe cache for those steps. #32 is untouched here.
6. Edge cases, exercised end-to-end against the real script under
dash(POSIX, no bash) with stubgolangci-lintandgobinaries onPATH, asserting on which module ref the stubgowas asked toinstall:
...@v2.12.2has version 2.10.1 built with ... (unknown)(host style)has version 2.12.2 built with ... from 1a2b3c4 on ...(image style)has version v2.12.2 ...(leadingv)has version 2.99.0 ...(newer than pin)sh -nanddash -nboth parse the script clean.Out of scope, noted
golangci-lintv2.12.2 emits a deprecation warning on every run: thegomodguardlinter is deprecated since v2.12.0 in favour ofgomodguard_v2..golangci.ymlis the canonical shared config and mustnot be edited in this repo, so this is filed separately rather than
touched here.
Built and verified on this host, which was a live reproduction of the
bug (v2.10.1 installed against the v2.12.2 pin).
What landed in
9e92472:script/bootstrapnow compares the installedgolangci-lintagainst the pin instead of testing only for presence.The version is a single value,
GOLANGCI_LINT_VERSION, from which thego installmodule ref is derived, so a bump cannot half-apply; agolangci_lint_versionhelper parses the number out ofgolangci-lint --versionand tolerates the leadingvthe module ref carries and thebinary's output does not.
git,makeandgostay presence-checkedand now carry a comment saying that is deliberate for host
package-manager tools.
TODO.mdgot its Completed Steps entry in thesame commit.
How it was verified:
script/bootstraprun printedbootstrap: golangci-lint 2.10.1, want 2.12.2; installingand left the host on 2.12.2; the second runprinted only
bootstrap completeand changed nothingmake lintat 2.12.2:0 issues.— the pin now matches theDockerfile lint stage, and v2.12.2 surfaced nothing that v2.10.1 was
hiding on
mainmake check:ok sneak.berlin/go/sfdupes 1.067s coverage: 88.5% of statements,0 issues.make docker: green, and demonstrably not a cache hit — only threeCACHEDsteps, allWORKDIR /src, whilemake fmt-check(0.9s),make lint(54.1s,0 issues.),make check(41.9s, 88.5%coverage) and
make build(9.3s) all executed with real outputdashcovering absent,older, newer, image-style, leading-
vand broken-binary cases;sh -nanddash -ncleanOne thing found and not fixed here: v2.12.2 warns that
gomodguardisdeprecated in favour of
gomodguard_v2..golangci.ymlis thecanonical shared config and is not editable in-repo, so that is filed
as #35 rather than touched in this PR. #32 (cached
make dockerthatproves nothing) is likewise untouched.
Review of PR #34 — verdict: FAIL (
needs-rework)One blocking defect. Everything else below is non-blocking or a
confirmation. Reviewed at head
9e92472againstmainat076d822.Blocking
B1.
script/bootstrap:113-118— the install is never verified, sobootstrap can report success while the wrong linter is still what runs.
go installwrites to$(go env GOPATH)/bin, but the version that wasread — and the version
make lintwill subsequently run — is whatevercommand -v golangci-lintresolves. Those are the same file only when$(go env GOPATH)/binwins thePATHsearch. When a wrong-versiongolangci-lintsits earlier onPATH(nix profile, apt, brew, apk, ahand-unpacked tarball in
/usr/local/bin— i.e. exactly the populationissue #24 targets, "any machine that already has some
golangci-linton
PATH"), the install lands behind the shadowing binary, nothing thegate uses changes, and the script prints
bootstrap completeand exits0.Reproduced against the real script under
dash, with a 2.10.1 binaryahead of the install target on
PATH:Why it matters, on three counts:
script/bootstrapon a host with a wrong-version
golangci-lintupgrades it, and asecond run is a no-op." It does neither: the effective version is
unchanged and every subsequent run re-installs, permanently.
issue is not "the pin is not read"; it is "the local gate silently
lints with a different ruleset than CI and the disagreement only
surfaces after a push." A bootstrap that exits
0having failed tomove the effective version produces precisely that state, now with
an affirmative "bootstrap complete" behind it.
Dockerfile:29installs the linter to/usr/local/bin/golangci-lintin the builder stage, which precedes
$GOPATH/bin(/home/builder/go/bin,Dockerfile:20) in any normalPATH.The change is a version check everywhere except at the one point where
it would establish the postcondition it exists to establish.
What acceptable looks like: after
go install, re-read the version andfail loudly if the pin is still not what resolves — do not exit
0.Something in the shape of
with that message redirected to stderr. Printing the resolved path and
$(go env GOPATH)/binis the part that makes it diagnosable; a barenon-zero exit would leave the user guessing.
Non-blocking
N1.
script/bootstrap:76—2>/dev/nullongolangci-lint --versiondiscards the only evidence of why parsing failed. A binary that is
present but permanently broken (missing shared library, wrong
architecture, a differently-named tool) yields the empty string, which
prints
absent or unparseableand runsgo installon every runforever, with the actual error suppressed. The direction is fail-safe,
so this is not blocking on its own, but it is a silent permanent
non-idempotence. Fixing B1 converts it into a loud failure, which is the
right outcome; consider also letting stderr through, or capturing it and
echoing it on the mismatch line.
N2.
script/bootstrap:71— the pinned version appears a second time,as a literal, in the comment.
DoD 2 asks for the pinned version in exactly one place in the script.
This occurrence cannot half-apply functionally, but it will be wrong the
first time someone bumps
GOLANGCI_LINT_VERSIONand does not notice thecomment. Use a placeholder (
X.Y.Z) so it cannot rot.N3. bootstrap now executes a binary it previously only located. The
old code called
command -vand never rangolangci-lint. There is notimeout on
golangci-lint --version, so a wedged binary now hangsbootstrap indefinitely rather than being replaced. Low likelihood,
mentioned because the behavioural change is new here.
N4. The pin still lives in two files.
script/bootstrap:18(
2.12.2) andDockerfile:2-3(v2.12.2plus digest) must be bumpedtogether or local and CI diverge again — structurally the same
half-apply hazard, one level up. DoD 2 is scoped to "in the script", so
this is not held against the PR, but it is the obvious follow-up and
worth an issue.
N5.
go install ...@v2.12.2is a mutable version tag, not a contenthash.
REPO_POLICIES.md's hash-pinning rule is written withoutexceptions. In practice the Go checksum database gives transparency-log
verification of that module version, and the line is pre-existing — this
PR only re-derives it — so it is not held against this change. Flagging
it because a reviewer of the next bootstrap PR will hit the same
question.
Verified clean — do not redo these
POSIX
sh/dash.sh -n,dash -nandbash -nall parse thescript clean (
/bin/shisdashon this host). Theset -econcern isgenuinely not a problem:
command -v golangci-lint ... || return 0executed under
dashwithset -eureturns the empty string andcontinues — verified by execution, not inspection. No unquoted
expansions, no unset-variable hazards (
installedis always assignedbefore
${installed:-...}reads it), no bashisms. The pre-existing[ -n "$PKGMGR" ] && return 0AND-list guard indetect_pkgmgris alsosafe under
dashset -e, checked separately.Version parsing. Thirteen inputs run end-to-end against the real
script under
dashwith stubgolangci-lintandgobinaries,asserting on the module ref the stub
gowas asked to install:PATHhas version 2.10.1 built with ... (unknown)has version 2.12.2 ...has version 2.12.2 ... from 1a2b3c4d on ...v:has version v2.12.2 ...2.99.02.12.2PATH:git version 2.43.0has version (devel) ...versionas the final fieldEvery mis-parse falls to reinstall. No input produced a false match, so
the original bug does not return through the parser. The
i < NFboundis correct rather than an off-by-one: a trailing
versionhas nofollowing field to read.
go installfailing propagates — rc 1, andbootstrap completeis not printed.DoD 5, independently confirmed. All
.gofiles at9e92472arebyte-identical to
main(the diff isTODO.mdandscript/bootstraponly), so a run here is
main's verdict.make checkat v2.12.2:0 issues.,ok sneak.berlin/go/sfdupes 1.225s coverage: 88.3% of statements. v2.10.1 was not hiding findings. The author's claim holds.make fmtis clean — no diff produced.make docker— and a caveat on the author's numbers.make dockeron this host was a complete cache hit: 18
CACHEDsteps includingRUN make fmt-check,RUN make lint,RUN make checkandRUN make build, exiting 0 in seconds having executed no gate. That is#32, reconfirmed live, and it means the author's "exactly three
CACHEDsteps" is not reproducible once a cache for this tree exists — it was
true only for the first build of the new commit. Forced a cold build
(
docker build --no-cache,BUILDKIT_PROGRESS=plain), which is greenand real:
CI.
check / check (push)is success on9e92472(1m48s). Itwas still pending when the PR was opened; it has since gone green.
Mergeable. Merges into
mainat076d822with no conflicts.Scope. Two files, 72 insertions, 5 deletions. Nothing newly pinned or
unpinned beyond the linter;
git/make/gokeep presence-only checksand the comment justifying that is honest and correct — they are host
package-manager tools, the repo pins no system toolchain,
go.modgoverns the language version, and reproducibility is the digest-pinned
images' job. Leaving them unpinned is defensible under
REPO_POLICIES.md,which pins external fetched references, not host packages.
Dockerfileuntouched, non-root quirk untouched. #32 and #35 are correctly absent.
Hygiene. No vendor, tooling-attribution or
Co-Authored-Bytraileranywhere in the commit message, the diff or the PR body. Commit title and
PR title both end with
(closes #24).TODO.mdupdated in the samecommit; its claim that
make dockeronce rejected a commit the localgate passed is corroborated by #32's record of PR #29's six
goconstfindings, so the history is accurate. Inclusive-terminology scan clean.
Naming is consistent with the surrounding script and carries no stutter.
Fix B1 and this is a merge. N1-N3 are cheap enough to fold into the same
push; N4 and N5 belong in their own issues.
Rework for the FAIL review, head now
9d06c13One added commit,
9d06c13"Verify the golangci-lint install actuallytook effect". No force-push, no history rewrite;
9e92472is untouchedand still carries the
(closes #24)title.B1 — the install is now verified (blocking)
Confirmed, not disputed:
go installwrites intoGOBIN(orGOPATH/bin) while the lintermake lintruns is whateverPATHresolves first, and the old code never closed that gap. The shadowed
host got an install behind the shadow, an unchanged effective version, a
bootstrap complete, andrc=0— forever, on every run.main()now calls a newverify_golangci_lintimmediately aftergo install. It re-reads the effective version through the samegolangci_lint_versionhelper and, on a mismatch, writes to stderrand exits 1:
Both paths are named, as requested. It does not reorder
PATHand doesnot touch anyone's binary — it diagnoses and stops.
GOBINis consultedbefore
GOPATH/binso the directory it names is the onego installreally wrote to.
hash -r(guarded,|| true) runs first so a shellthat remembered a lookup for a directory the install has just populated
cannot report a stale answer.
The same guard also catches the adjacent case the reviewer did not raise:
install destination not on
PATHat all. That used to exit 0 withnothing runnable; it now fails with
the golangci-lint on PATH is not resolvable.N1 — stderr no longer discarded
2>/dev/nullis gone from the--versioncall. The binary's stderrstays connected to bootstrap's, so a present-but-broken linter says why
instead of yielding the empty string and reinstalling silently forever.
Only stdout is parsed, deliberately: folding stderr into the parse
would have turned the reviewer's "version on stderr only" row from
installs into no-op and changed the verified matrix. It stays
installs, and the binary's message is now visible above it. Combined
with B1, a permanently broken binary is now loud and terminal rather
than silently non-idempotent.
N2 — pinned version is a single literal again
The comment at what was
script/bootstrap:71now readsgolangci-lint has version X.Y.Z built with go1.26.5 from abc1234 ....2.12.2appears exactly once in the script, in
GOLANGCI_LINT_VERSION.N3 — the
--versioncall is boundedNew
GOLANGCI_LINT_VERSION_TIMEOUT="30"; the call runs undertimeout(1)when one is onPATH. Hosts without it (stock macOS)run unbounded, as before this check existed — degrading is better than
failing there. A timeout kill yields no parse, which falls to reinstall,
which then runs through B1's verification, so a wedged binary ends in a
replacement or a loud failure, never a hang.
Out of scope, as instructed
N4 (pin spanning
script/bootstrapandDockerfile) and N5(
@v2.12.2as a tag rather than a hash) are untouched. #32 (cachedmake docker) and #35 (gomodguarddeprecation) likewise.Verification
All shell work run under
dash, against the realscript/bootstrap,with stub
golangci-lintandgobinaries; the stubgologs themodule ref it is asked to install.
sh -n,dash -nandbash -nallparse clean.
1. Shadowing reproduction — now fails loudly. Wrong-version 2.10.1
ahead of the install destination on
PATH, two consecutive runs:bootstrap completeis not printed on either run.2. Normal case still works. Wrong version, no shadowing:
Effective linter afterwards reports 2.12.2. Run 2 is a genuine no-op:
no mismatch line and the stub
gowas not invoked a second time. TheGOBIN-set variant behaves identically. The realscript/bootstraponthis host (already at 2.12.2) is a 0.115s no-op,
rc=0.3. Parse matrix re-run — all thirteen rows unchanged.
installs/no-opasserted on the stub install log,rcon the script:PATHv: v2.12.2git version 2.43.0(devel)versionas final fieldEvery mis-parse still falls to reinstall; no input produces a false
match. The stderr-only row additionally now surfaces the binary's own
output (
golangci-lint has version 2.12.2 built with go1.26.5) abovethe
absent or unparseableline — N1's point, without changing theoutcome.
4. Timeout fires. A
golangci-lintstub that sleeps forever:Bounded at the pinned 30s, then reinstalled and verified, rather than
hanging. With
timeout(1)removed fromPATHentirely the script stillruns correctly (unbounded),
rc=0.Gates
make check: green.(plus the known
gomodguarddeprecation warning, #35.)make docker: green, and the gates really executed — no cold buildneeded.
BUILDKIT_PROGRESS=plain, 10CACHEDsteps, and every oneof them is a setup layer, not a gate:
All four gates ran with real output and real time:
The
COPY . .layer changed with9d06c13, which is what invalidatedthe gate layers. #32 is untouched and remains real: had this commit
introduced no tree change, those four would have been
CACHEDtoo.make fmt: clean, no diff.TODO.md's Completed Steps entry wasupdated in the same commit and now describes the verification step, the
stderr passthrough and the timeout, and states the widened test matrix
rather than the old five cases.
Second independent review of PR #34 — verdict: PASS (
merge-ready)Fresh review at head
9d06c13againstmainat076d822. I did notauthor this change and did not perform the earlier review. No blocking
findings. Everything below the verdict is non-blocking or a
confirmation, and several items are notes for the maintainer rather
than defects in this PR.
B1 from the previous review is genuinely closed
I did not take the rework description on trust — I rebuilt the
shadowing reproduction from scratch and ran the real
script/bootstrapunder
dashwith stubgoandgolangci-lintbinaries, asserting onthe module ref the stub
gowas asked to install.PATHthan the install dirrc=1, both paths named,bootstrap completeNOT printed, on both runsgoinvoked exactly once),rc=0GOBINset to a directory not onPATHat allrc=1,the golangci-lint on PATH is not resolvableGOBINset ANDGOPATH/binonPATHwith a different binaryrc=1, and the directory named isGOBIN— the onego installactually wrote toPATHbut after a stale 2.12.1 copyrc=1rc=0, zero installs — correct, the effective version is the pinPATHsearch skips it, the install dir wins,rc=0— correctRepresentative output from the shadow case:
Both paths are named, the message goes to stderr, and the exit is 1. I
could not construct any input or layout that produces
bootstrap completewhile the effective linter is not the pin. The postconditionthe check exists to establish is now actually established.
hash -ris load-bearing, not decorationWorth recording, because it is easy to dismiss as a no-op. Under
dashon this host,
command -vDOES consult the hash table, and it returnsa stale answer after a new binary appears earlier in the search order:
hash -r 2>/dev/null || trueis correct underdash(rc=0, and itdoes not trip
set -eu; it also succeeds unguarded there, so the guardis purely for shells where
hashdiffers). Because the guard runs inthe function's own shell rather than a subshell, the subsequent
command -vand version re-read — both command substitutions — forkfrom an already-cleared table and genuinely see the new binary.
POSIX sh discipline
sh -n,dash -nandbash -nall parse clean;/bin/shisdashhere. Executed the new function under
dashin every scenario above,not merely inspected it. No bashisms, no unquoted expansions —
$goinstalldir,${resolved:-...}and${effective:-...}are allquoted, and every one is assigned before it is read, so
set -uissafe.
go env GOBINreturning empty is the normal case and thefallback to
$(go env GOPATH)/binhandles it (exercised in most rowsabove). The
if ... fi | awkpipeline yieldsawk's status, so anon-zero or killed
golangci-lintdoes not tripset -e. Both filesare 80 columns or under; the executable bit on
script/bootstrapispreserved (
100755).Timeout (N3)
reinstall,
rc=0,bootstrap complete.reads — the pre-install one and the verification one), then
rc=1with the diagnostic. Never a hang, never a false match, and no
orphaned processes left behind.
timeout(1)absent fromPATHentirely: upgrade on run 1, trueno-op on run 2,
rc=0. The unbounded path works.A timeout kill yields no parse, which falls to reinstall and then
through the verification — confirmed, not assumed.
Parse matrix — re-run, plus seven adversarial rows
All thirteen of the previous reviewer's rows reproduce exactly as the
author reports. I added seven more looking specifically for a false
match:
2.12.2.1(superstring of the pin)x version notaversion y version 2.12.2versionNo input in twenty produced a false match. Every mis-parse falls to
reinstall, and every reinstall now falls through verification. The
i < NFbound is correct rather than an off-by-one. Dropping2>/dev/nulldid not move any row: the stderr-only case stillinstalls, and now the binary's own message is visible above the
absent or unparseableline.Definition of done
execution.
2.12.2appears atscript/bootstrap:21and nowhere else in the file. Repo-wide theonly other occurrences are
Dockerfile:2-3(N4, out of scope) andTODO.mdprose.git/make/goaudited and documented as deliberately unpinned —script/bootstrap:147-152, and the justification is honest.stubs, and the real script on this host (already at the pin) is a
0.118s no-op at
rc=0.make checkandmake dockergreen, localmake lintmatches theDockerfile lint stage — confirmed below.
Gates
make fmt-checkclean.make fmtproduces no diff.make check: green —0 issues.,ok sneak.berlin/go/sfdupes,88.3% coverage.
One caveat other reviewers should know about. My first
make checkrunreported ten findings whose paths pointed at
../sfdupes-rework-34/, a directory that does not exist on this host.That is a poisoned
golangci-lintcache left by a deleted worktreewith byte-identical content, not a property of this PR. Re-running with
a fresh
GOLANGCI_LINT_CACHEgives0 issues.This PR changes zero.gofiles (git diff --name-only 076d822..HEADis exactlyTODO.mdand
script/bootstrap), so any Go finding here is by constructionmain's finding, not this branch's.make docker: green, but this host's run proves nothing on its own.With
BUILDKIT_PROGRESS=plainI got 17CACHEDsteps — a totalcache hit in which
make fmt-check,make lint,make checkandmake buildwere allCACHEDand executed nothing, exit 0. That isissue #32, reconfirmed live for the third time. The author's "10
CACHEDwith all four gates running" was true only for the first buildof
9d06c13; it is not reproducible once a cache for this tree exists.So I forced a cold build (
docker build --no-cache,BUILDKIT_PROGRESS=plain), which is the real result:All four gates executed with real output and real wall-clock time.
CI, mergeability, scope, hygiene
CI
check / check (push)is success on9d06c13(1m31s).Merges into
mainat076d822with no conflicts;mainhas not moved.9e92472is still an ancestor of9d06c13— one added commit, noforce-push, no history rewrite.
Scope is exactly
TODO.mdandscript/bootstrap, 145 insertions, 5deletions. Nothing from #32 or #26. Commit titles are correct:
9e92472carries
(closes #24)and the rework commit correctly does not repeatit.
TODO.md's Completed Steps entry is accurate — I checked itsspecific claims (thirteen-input matrix, shadowed install exiting
non-zero, install destination not on
PATH,GOBINset, wedged binaryhitting the timeout) against my own runs and they hold.
No vendor, tooling-attribution or
Co-Authored-Bytrailer anywhere inthe commits, the diff or the PR body. Author and committer are
clawboton both commits. Inclusive-terminology scan clean. Naming isconsistent with the surrounding script and carries no stutter;
verify_golangci_lintandgolangci_lint_versionread correctlyagainst the existing
missing/pkg_install/detect_pkgmgrstyle.Non-blocking
M1.
script/bootstrap:128— a multi-elementGOPATHyields amalformed directory in the diagnostic.
go env GOPATHreturns thewhole colon-separated list, so with
GOPATH=/a:/bthe message reads:go installwrites to the FIRST element'sbin, so the advice names apath that does not exist. This is a diagnostic-quality defect only —
the run still exits 1 loudly, never a false pass — and multi-element
GOPATHis rare and effectively legacy. Acceptable would be taking thefirst colon-separated element. Not worth another round on its own.
M2. Without
timeout(1), a wedged binary hangs bootstrapindefinitely. Verified: with
timeoutabsent fromPATH, an outer8s bound returned 124. The script's own comment is honest about this
and calls it a deliberate degradation, which I accept — but note that
brewis in the package-manager detection list, so stock macOS is asupported host class and this is a hang risk that did not exist before
this PR (the old code never executed the binary).
TODO.md's phrasing"so a wedged binary cannot hang bootstrap" is qualified by "where that
exists" but reads stronger than the behaviour. A follow-up could fall
back to a background-kill idiom or
perl -e alarm.M3.
--versionexit status is ignored. A binary that printshas version 2.12.2on stdout and then exits non-zero is treated as amatch and produces a silent no-op (matrix row 8, unchanged from the
previous review). It is the one row where a demonstrably broken binary
yields a pass rather than a reinstall. Low severity —
make lintwiththat binary would fail loudly and immediately — and it is pre-existing
behaviour from
9e92472rather than something the rework introduced.M4. Worst case before the loud failure is 60s on a shadowed host
with a wedged shadow: the pre-install read and the verification read
each burn the full 30s. Bounded and correct, just slower than the
single pin suggests.
M5 (out of scope, as instructed). N4 (the pin also living in
Dockerfile:2-3) and N5 (@v2.12.2as a mutable tag againstREPO_POLICIES.md's no-exceptions hash-pinning rule) are untouched andare not held against this PR. Both predate it and this change only
re-derives the existing ref. They deserve their own issues.
M6. For the maintainer, not this PR. Two independent ways the local
gate can lie were observed on this host during review:
make dockerasa total cache hit (#32), and a
golangci-lintcache poisoned by adeleted worktree reporting phantom findings against paths that no
longer exist. The second is a new observation and is thematically the
same class of problem as #24 — a gate that reports something other than
the truth about the current tree. It may be worth an issue.
The rework does what it claims. The hole the previous review found is
closed, the fix is verified by execution rather than by inspection, it
degrades in the safe direction on every input I could construct, and
both gates are green on a build that actually ran. Merge it.
Manager note — merged as
b8ebe5f.Fresh re-review returned PASS with no blocking findings. Branch deleted;
origincarries onlymain.Verified on
mainafter the merge, with a cleanGOLANGCI_LINT_CACHEto avoid the cache-poisoning effect the reviewer found:That last line is the point of the whole issue. This host was running v2.10.1 against a v2.12.2 pin when the session started;
script/bootstrapupgraded it, and localmake lintnow runs the same linter as the Dockerfile lint stage for the first time.The loop earned its keep again. The first attempt compared versions correctly but never checked that the install took effect —
go installwrites to$GOPATH/binwhile the effective binary is whateverPATHresolves, so a wrong-version linter shadowing it left bootstrap printingbootstrap completeand exiting 0 having changed nothing. That is strictly worse than the bug it replaced: a silent problem converted into a confidently-wrong one, on precisely the host population #24 was written for. The reviewer reproduced it rather than reasoning about it, and noted this repo ships a live instance of the shadowing layout atDockerfile:29.The re-review rebuilt that reproduction from scratch across seven
PATHlayouts — wrong-version shadow,GOBINoffPATH,GOBINset with a competingGOPATH/bin, stale copy ahead, symlink to the correct version, non-executable shadow — and confirmed the first four fail loudly with both paths named while the last two correctly pass. It ran a 20-row version-parse matrix establishing that no input produces a false match: every mis-parse falls to reinstall. That is the property that matters, since a false match is the original bug returning.Confirmed along the way, and worth recording: v2.10.1 was hiding nothing on
main. At v2.12.2 bothmake lintandmake checkare0 issues.The sixgoconstfindings that hit PR #29 were in that PR's own code.Two gate-integrity observations from the review, both now tracked:
make dockerwas a 17-CACHEDtotal no-op — every gate cached, exit 0, nothing executed. Third independent confirmation of #32. The reviewer forced--no-cache(0 cached, all four gates real, green) rather than accepting it.make checkreported ten findings against paths under a deleted worktree, from a poisoned golangci-lint cache keyed on file content. Filed as #36, along with three small robustness gaps in the new bootstrap verification (multi-elementGOPATHin the diagnostic, ignored--versionexit status, unbounded hang withouttimeout(1)on macOS). None can produce a false pass.With #24 closed, one of the three ways this repo's gates could lie is fixed. #32 and #36 remain.