script/bootstrap installed the pinned linter only when the command was
absent (if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi), so on any host that already had somegolangci-lint the pin was
never 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 check
green, make docker rejecting the same commit.
What changed
The linter version is now a single value, GOLANGCI_LINT_VERSION
(2.12.2, no leading v, the way the binary reports it), with GOLANGCI_LINT_MODULE and GOLANGCI_LINT_REF derived from it. A
future bump edits one string; the install ref and the comparison value
cannot drift apart (DoD 2).
New golangci_lint_version() helper: returns early with no output if command -v golangci-lint fails, otherwise parses golangci-lint --version with awk, taking the field after the literal word version and stripping an optional leading v. That covers both
output 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-commit
line.
The presence test became a version test (DoD 1): any version that is
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.
Audit of the other tools (DoD 3): git, make and go keep their
presence-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.mod governs the language version, and
reproducible builds are the digest-pinned Docker images' job. Nothing
was newly pinned.
POSIX sh throughout; no bashisms (verified under dash, see below).
Verification
1. Wrong-version host upgrades. Before: golangci-lint has version 2.10.1 built with go1.26.5 .... Running script/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 lint now uses the pinned version.golangci-lint --version reports 2.12.2, the same version the Dockerfile lint stage
pins (golangci/golangci-lint:v2.12.2@sha256:5cceeef0...). make lint: 0 issues. (DoD 5).
4. make check green with the correct linter:
ok sneak.berlin/go/sfdupes 1.067s coverage: 88.5% of statements
0 issues.
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 docker green, and the build really ran. Guarding against
the cached-no-op failure mode of #32: the build log (captured with BUILDKIT_PROGRESS=plain) contains exactly three CACHED steps, all of
them WORKDIR /src. Every gate step executed with real output and real
wall-clock time:
#17 [lint 6/7] RUN make fmt-check DONE 0.9s
#18 [lint 7/7] RUN make lint 0 issues. DONE 54.1s
#26 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.177s
coverage: 88.5% of statements
0 issues. DONE 41.9s
#27 [builder 11/11] RUN make build DONE 9.3s
The COPY . . layer changed with this commit, which is what invalidated
the 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 stub golangci-lint and go binaries on PATH, asserting on which module ref the stub go was asked to
install:
installed
result
absent
installs ...@v2.12.2
has version 2.10.1 built with ... (unknown) (host style)
installs
has version 2.12.2 built with ... from 1a2b3c4 on ... (image style)
no-op
has version v2.12.2 ... (leading v)
no-op
has version 2.99.0 ... (newer than pin)
installs
binary exits non-zero
installs
sh -n and dash -n both parse the script clean.
Out of scope, noted
golangci-lint v2.12.2 emits a deprecation warning on every run: the gomodguard linter is deprecated since v2.12.0 in favour of gomodguard_v2. .golangci.yml is the canonical shared config and must
not be edited in this repo, so this is filed separately rather than
touched here.
Closes #24.
`script/bootstrap` installed the pinned linter only when the command was
absent (`if missing golangci-lint; then go install "$GOLANGCI_LINT_REF";
fi`), so on any host that already had *some* `golangci-lint` the pin was
never 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 check`
green, `make docker` rejecting the same commit.
## What changed
- The linter version is now a single value, `GOLANGCI_LINT_VERSION`
(`2.12.2`, no leading `v`, the way the binary reports it), with
`GOLANGCI_LINT_MODULE` and `GOLANGCI_LINT_REF` derived from it. A
future bump edits one string; the install ref and the comparison value
cannot drift apart (DoD 2).
- New `golangci_lint_version()` helper: returns early with no output if
`command -v golangci-lint` fails, otherwise parses `golangci-lint
--version` with `awk`, taking the field after the literal word
`version` and stripping an optional leading `v`. That covers both
output 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-commit
line.
- The presence test became a version test (DoD 1): any version that is
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.
- Audit of the other tools (DoD 3): `git`, `make` and `go` keep their
presence-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.mod` governs the language version, and
reproducible builds are the digest-pinned Docker images' job. Nothing
was newly pinned.
POSIX `sh` throughout; no bashisms (verified under `dash`, see below).
## Verification
**1. Wrong-version host upgrades.** Before: `golangci-lint has version
2.10.1 built with go1.26.5 ...`. Running `script/bootstrap`:
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
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 lint` now uses the pinned version.** `golangci-lint
--version` reports 2.12.2, the same version the Dockerfile lint stage
pins (`golangci/golangci-lint:v2.12.2@sha256:5cceeef0...`). `make lint`:
`0 issues.` (DoD 5).
**4. `make check` green with the correct linter:**
ok sneak.berlin/go/sfdupes 1.067s coverage: 88.5% of statements
0 issues.
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 docker` green, and the build really ran.** Guarding against
the cached-no-op failure mode of #32: the build log (captured with
`BUILDKIT_PROGRESS=plain`) contains exactly three `CACHED` steps, all of
them `WORKDIR /src`. Every gate step executed with real output and real
wall-clock time:
#17 [lint 6/7] RUN make fmt-check DONE 0.9s
#18 [lint 7/7] RUN make lint 0 issues. DONE 54.1s
#26 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.177s
coverage: 88.5% of statements
0 issues. DONE 41.9s
#27 [builder 11/11] RUN make build DONE 9.3s
The `COPY . .` layer changed with this commit, which is what invalidated
the 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 stub `golangci-lint` and `go` binaries on
`PATH`, asserting on which module ref the stub `go` was asked to
install:
| installed | result |
| --- | --- |
| absent | installs `...@v2.12.2` |
| `has version 2.10.1 built with ... (unknown)` (host style) | installs |
| `has version 2.12.2 built with ... from 1a2b3c4 on ...` (image style) | no-op |
| `has version v2.12.2 ...` (leading `v`) | no-op |
| `has version 2.99.0 ...` (newer than pin) | installs |
| binary exits non-zero | installs |
`sh -n` and `dash -n` both parse the script clean.
## Out of scope, noted
`golangci-lint` v2.12.2 emits a deprecation warning on every run: the
`gomodguard` linter is deprecated since v2.12.0 in favour of
`gomodguard_v2`. `.golangci.yml` is the canonical shared config and must
not be edited in this repo, so this is filed separately rather than
touched here.
script/bootstrap installed the pinned linter only when the command was
absent, so on any host that already had some golangci-lint the pin was
never consulted and a version bump was inert forever. That is how a
host running v2.10.1 against a v2.12.2 pin got a green `make check`
while `make docker` rejected the same commit: the local gate was
linting with a different ruleset than CI, and the disagreement only
surfaced after a push.
The version is now a single value, GOLANGCI_LINT_VERSION, with the
`go install` module ref derived from it, so a future bump cannot
half-apply. A golangci_lint_version helper parses the installed
version out of `golangci-lint --version` (the field after the word
"version", with an optional leading "v" stripped, since the module ref
carries one and the binary's output does not) and yields the empty
string when the tool is absent or unreadable. Any version that is not
the pin -- older, newer, absent or unparseable -- is reinstalled, so a
first run upgrades and a second is a no-op.
git, make and go keep their presence-only checks: they come from the
host package manager, the repo pins no system toolchain versions, and
go.mod governs the language version. That is now stated in a comment
next to them rather than left ambiguous beside a tool that is
version-checked.
clawbot
self-assigned this 2026-08-09 07:59:42 +02:00
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/bootstrap now compares the installed golangci-lint against the pin instead of testing only for presence.
The version is a single value, GOLANGCI_LINT_VERSION, from which the go install module ref is derived, so a bump cannot half-apply; a golangci_lint_version helper parses the number out of golangci-lint --version and tolerates the leading v the module ref carries and the
binary's output does not. git, make and go stay presence-checked
and now carry a comment saying that is deliberate for host
package-manager tools. TODO.md got its Completed Steps entry in the
same commit.
How it was verified:
first script/bootstrap run printed bootstrap: golangci-lint 2.10.1, want 2.12.2; installing and left the host on 2.12.2; the second run
printed only bootstrap complete and changed nothing
make lint at 2.12.2: 0 issues. — the pin now matches the
Dockerfile lint stage, and v2.12.2 surfaced nothing that v2.10.1 was
hiding on main
make 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 three CACHED steps, all WORKDIR /src, while make 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 output
stub-binary runs of the real script under dash covering absent,
older, newer, image-style, leading-v and broken-binary cases; sh -n and dash -n clean
One thing found and not fixed here: v2.12.2 warns that gomodguard is
deprecated in favour of gomodguard_v2. .golangci.yml is the
canonical shared config and is not editable in-repo, so that is filed
as #35 rather than touched in this PR. #32 (cached make docker that
proves nothing) is likewise untouched.
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/bootstrap` now compares the installed
`golangci-lint` against the pin instead of testing only for presence.
The version is a single value, `GOLANGCI_LINT_VERSION`, from which the
`go install` module ref is derived, so a bump cannot half-apply; a
`golangci_lint_version` helper parses the number out of `golangci-lint
--version` and tolerates the leading `v` the module ref carries and the
binary's output does not. `git`, `make` and `go` stay presence-checked
and now carry a comment saying that is deliberate for host
package-manager tools. `TODO.md` got its Completed Steps entry in the
same commit.
How it was verified:
- first `script/bootstrap` run printed `bootstrap: golangci-lint 2.10.1,
want 2.12.2; installing` and left the host on 2.12.2; the second run
printed only `bootstrap complete` and changed nothing
- `make lint` at 2.12.2: `0 issues.` — the pin now matches the
Dockerfile lint stage, and v2.12.2 surfaced nothing that v2.10.1 was
hiding on `main`
- `make 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 three
`CACHED` steps, all `WORKDIR /src`, while `make 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 output
- stub-binary runs of the real script under `dash` covering absent,
older, newer, image-style, leading-`v` and broken-binary cases;
`sh -n` and `dash -n` clean
One thing found and not fixed here: v2.12.2 warns that `gomodguard` is
deprecated in favour of `gomodguard_v2`. `.golangci.yml` is the
canonical shared config and is not editable in-repo, so that is filed
as #35 rather than touched in this PR. #32 (cached `make docker` that
proves nothing) is likewise untouched.
One blocking defect. Everything else below is non-blocking or a
confirmation. Reviewed at head 9e92472 against main at 076d822.
Blocking
B1. script/bootstrap:113-118 — the install is never verified, so
bootstrap can report success while the wrong linter is still what runs.
installed="$(golangci_lint_version)"
if [ "$installed" != "$GOLANGCI_LINT_VERSION" ]; then
echo "bootstrap: golangci-lint ${installed:-absent or unparseable}," \
"want $GOLANGCI_LINT_VERSION; installing"
go install "$GOLANGCI_LINT_REF"
fi
go install writes to $(go env GOPATH)/bin, but the version that was
read — and the version make lint will subsequently run — is whatever command -v golangci-lint resolves. Those are the same file only when $(go env GOPATH)/bin wins the PATH search. When a wrong-version golangci-lint sits earlier on PATH (nix profile, apt, brew, apk, a
hand-unpacked tarball in /usr/local/bin — i.e. exactly the population
issue #24 targets, "any machine that already has somegolangci-lint
on PATH"), the install lands behind the shadowing binary, nothing the
gate uses changes, and the script prints bootstrap complete and exits 0.
Reproduced against the real script under dash, with a 2.10.1 binary
ahead of the install target on PATH:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0
--- run 2 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0
effective linter still: golangci-lint has version 2.10.1 built with go1.26.5
Why it matters, on three counts:
DoD 4 is not met on that host class. "Running script/bootstrap
on a host with a wrong-version golangci-lint upgrades it, and a
second run is a no-op." It does neither: the effective version is
unchanged and every subsequent run re-installs, permanently.
It reintroduces #24's actual failure mode, one level in. The
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 0 having failed to
move the effective version produces precisely that state, now with
an affirmative "bootstrap complete" behind it.
This repo contains a live instance of the shadowing layout. Dockerfile:29 installs the linter to /usr/local/bin/golangci-lint
in the builder stage, which precedes $GOPATH/bin (/home/builder/go/bin, Dockerfile:20) in any normal PATH.
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 and
fail loudly if the pin is still not what resolves — do not exit 0.
Something in the shape of
go install "$GOLANGCI_LINT_REF"
installed="$(golangci_lint_version)"
if [ "$installed" != "$GOLANGCI_LINT_VERSION" ]; then
echo "bootstrap: installed golangci-lint $GOLANGCI_LINT_VERSION but" \
"$(command -v golangci-lint) still reports" \
"${installed:-nothing}; put $(go env GOPATH)/bin ahead of it" \
"on PATH"
exit 1
fi
with that message redirected to stderr. Printing the resolved path and $(go env GOPATH)/bin is the part that makes it diagnosable; a bare
non-zero exit would leave the user guessing.
Non-blocking
N1. script/bootstrap:76 — 2>/dev/null on golangci-lint --version
discards 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 unparseable and runs go install on every run
forever, 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.
# golangci-lint has version 2.12.2 built with go1.26.5 from abc1234 ...
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_VERSION and does not notice the
comment. 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 -v and never ran golangci-lint. There is no
timeout on golangci-lint --version, so a wedged binary now hangs
bootstrap 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) and Dockerfile:2-3 (v2.12.2 plus digest) must be bumped
together 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.2 is a mutable version tag, not a content
hash.REPO_POLICIES.md's hash-pinning rule is written without
exceptions. 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 -n and bash -n all parse the
script clean (/bin/sh is dash on this host). The set -e concern is
genuinely not a problem: command -v golangci-lint ... || return 0
executed under dash with set -eu returns the empty string and
continues — verified by execution, not inspection. No unquoted
expansions, no unset-variable hazards (installed is always assigned
before ${installed:-...} reads it), no bashisms. The pre-existing [ -n "$PKGMGR" ] && return 0 AND-list guard in detect_pkgmgr is also
safe under dashset -e, checked separately.
Version parsing. Thirteen inputs run end-to-end against the real
script under dash with stub golangci-lint and go binaries,
asserting on the module ref the stub go was asked to install:
input
outcome
absent from PATH
installs
host style has version 2.10.1 built with ... (unknown)
installs
host style has version 2.12.2 ...
no-op
image style has version 2.12.2 ... from 1a2b3c4d on ...
no-op
leading v: has version v2.12.2 ...
no-op
newer than pin, 2.99.0
installs
exits 1, no output
installs
exits 1 but prints 2.12.2
no-op
version on stderr only
installs
prints nothing, exits 0
installs
impostor on PATH: git version 2.43.0
installs
dev build: has version (devel) ...
installs
word version as the final field
installs
Every mis-parse falls to reinstall. No input produced a false match, so
the original bug does not return through the parser. The i < NF bound
is correct rather than an off-by-one: a trailing version has no
following field to read. go install failing propagates — rc 1, and bootstrap complete is not printed.
DoD 5, independently confirmed. All .go files at 9e92472 are
byte-identical to main (the diff is TODO.md and script/bootstrap
only), so a run here is main's verdict. make check at 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 fmt is clean — no diff produced.
make docker — and a caveat on the author's numbers.make docker
on this host was a complete cache hit: 18 CACHED steps including RUN make fmt-check, RUN make lint, RUN make check and RUN make build, exiting 0 in seconds having executed no gate. That is #32, reconfirmed live, and it means the author's "exactly three CACHED
steps" 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 green
and real:
5 CACHED (base-image resolve and WORKDIR /src metadata only)
#19 [lint 6/7] RUN make fmt-check DONE 1.1s
#20 [lint 7/7] RUN make lint 0 issues. DONE 38.9s
#26 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 2.091s
coverage: 88.5% of statements
0 issues. DONE 33.1s
#27 [builder 11/11] RUN make build
exit 0
CI.check / check (push) is success on 9e92472 (1m48s). It
was still pending when the PR was opened; it has since gone green.
Mergeable. Merges into main at 076d822 with no conflicts.
Scope. Two files, 72 insertions, 5 deletions. Nothing newly pinned or
unpinned beyond the linter; git/make/go keep presence-only checks
and the comment justifying that is honest and correct — they are host
package-manager tools, the repo pins no system toolchain, go.mod
governs 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. Dockerfile
untouched, non-root quirk untouched. #32 and #35 are correctly absent.
Hygiene. No vendor, tooling-attribution or Co-Authored-By trailer
anywhere in the commit message, the diff or the PR body. Commit title and
PR title both end with (closes #24). TODO.md updated in the same
commit; its claim that make docker once rejected a commit the local
gate passed is corroborated by #32's record of PR #29's six goconst
findings, 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.
## Review of PR #34 — verdict: FAIL (`needs-rework`)
One blocking defect. Everything else below is non-blocking or a
confirmation. Reviewed at head `9e92472` against `main` at `076d822`.
---
### Blocking
**B1. `script/bootstrap:113-118` — the install is never verified, so
bootstrap can report success while the wrong linter is still what runs.**
installed="$(golangci_lint_version)"
if [ "$installed" != "$GOLANGCI_LINT_VERSION" ]; then
echo "bootstrap: golangci-lint ${installed:-absent or unparseable}," \
"want $GOLANGCI_LINT_VERSION; installing"
go install "$GOLANGCI_LINT_REF"
fi
`go install` writes to `$(go env GOPATH)/bin`, but the version that was
read — and the version `make lint` will subsequently run — is whatever
`command -v golangci-lint` resolves. Those are the same file only when
`$(go env GOPATH)/bin` wins the `PATH` search. When a wrong-version
`golangci-lint` sits earlier on `PATH` (nix profile, apt, brew, apk, a
hand-unpacked tarball in `/usr/local/bin` — i.e. exactly the population
issue #24 targets, "any machine that already has *some* `golangci-lint`
on `PATH`"), the install lands behind the shadowing binary, nothing the
gate uses changes, and the script prints `bootstrap complete` and exits
`0`.
Reproduced against the real script under `dash`, with a 2.10.1 binary
ahead of the install target on `PATH`:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0
--- run 2 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0
effective linter still: golangci-lint has version 2.10.1 built with go1.26.5
Why it matters, on three counts:
1. **DoD 4 is not met on that host class.** "Running `script/bootstrap`
on a host with a wrong-version `golangci-lint` upgrades it, and a
second run is a no-op." It does neither: the effective version is
unchanged and every subsequent run re-installs, permanently.
2. **It reintroduces #24's actual failure mode, one level in.** The
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 `0` having failed to
move the effective version produces precisely that state, now with
an affirmative "bootstrap complete" behind it.
3. **This repo contains a live instance of the shadowing layout.**
`Dockerfile:29` installs the linter to `/usr/local/bin/golangci-lint`
in the builder stage, which precedes `$GOPATH/bin` (`/home/builder/go/bin`,
`Dockerfile:20`) in any normal `PATH`.
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 and
fail loudly if the pin is still not what resolves — do not exit `0`.
Something in the shape of
go install "$GOLANGCI_LINT_REF"
installed="$(golangci_lint_version)"
if [ "$installed" != "$GOLANGCI_LINT_VERSION" ]; then
echo "bootstrap: installed golangci-lint $GOLANGCI_LINT_VERSION but" \
"$(command -v golangci-lint) still reports" \
"${installed:-nothing}; put $(go env GOPATH)/bin ahead of it" \
"on PATH"
exit 1
fi
with that message redirected to stderr. Printing the resolved path and
`$(go env GOPATH)/bin` is the part that makes it diagnosable; a bare
non-zero exit would leave the user guessing.
---
### Non-blocking
**N1. `script/bootstrap:76` — `2>/dev/null` on `golangci-lint --version`
discards 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 unparseable` and runs `go install` on *every* run
forever, 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.**
# golangci-lint has version 2.12.2 built with go1.26.5 from abc1234 ...
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_VERSION` and does not notice the
comment. 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 -v` and never ran `golangci-lint`. There is no
timeout on `golangci-lint --version`, so a wedged binary now hangs
bootstrap 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`) and `Dockerfile:2-3` (`v2.12.2` plus digest) must be bumped
together 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.2` is a mutable version tag, not a content
hash.** `REPO_POLICIES.md`'s hash-pinning rule is written without
exceptions. 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 -n` and `bash -n` all parse the
script clean (`/bin/sh` is `dash` on this host). The `set -e` concern is
genuinely not a problem: `command -v golangci-lint ... || return 0`
executed under `dash` with `set -eu` returns the empty string and
continues — verified by execution, not inspection. No unquoted
expansions, no unset-variable hazards (`installed` is always assigned
before `${installed:-...}` reads it), no bashisms. The pre-existing
`[ -n "$PKGMGR" ] && return 0` AND-list guard in `detect_pkgmgr` is also
safe under `dash` `set -e`, checked separately.
**Version parsing.** Thirteen inputs run end-to-end against the real
script under `dash` with stub `golangci-lint` and `go` binaries,
asserting on the module ref the stub `go` was asked to install:
| input | outcome |
| --- | --- |
| absent from `PATH` | installs |
| host style `has version 2.10.1 built with ... (unknown)` | installs |
| host style `has version 2.12.2 ...` | no-op |
| image style `has version 2.12.2 ... from 1a2b3c4d on ...` | no-op |
| leading `v`: `has version v2.12.2 ...` | no-op |
| newer than pin, `2.99.0` | installs |
| exits 1, no output | installs |
| exits 1 but prints `2.12.2` | no-op |
| version on **stderr** only | installs |
| prints nothing, exits 0 | installs |
| impostor on `PATH`: `git version 2.43.0` | installs |
| dev build: `has version (devel) ...` | installs |
| word `version` as the final field | installs |
Every mis-parse falls to reinstall. No input produced a false match, so
the original bug does not return through the parser. The `i < NF` bound
is correct rather than an off-by-one: a trailing `version` has no
following field to read. `go install` failing propagates — rc 1, and
`bootstrap complete` is not printed.
**DoD 5, independently confirmed.** All `.go` files at `9e92472` are
byte-identical to `main` (the diff is `TODO.md` and `script/bootstrap`
only), so a run here is `main`'s verdict. `make check` at 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 fmt` is clean** — no diff produced.
**`make docker` — and a caveat on the author's numbers.** `make docker`
on this host was a **complete cache hit**: 18 `CACHED` steps including
`RUN make fmt-check`, `RUN make lint`, `RUN make check` and
`RUN make build`, exiting 0 in seconds having executed no gate. That is
#32, reconfirmed live, and it means the author's "exactly three `CACHED`
steps" 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 green
and real:
5 CACHED (base-image resolve and WORKDIR /src metadata only)
#19 [lint 6/7] RUN make fmt-check DONE 1.1s
#20 [lint 7/7] RUN make lint 0 issues. DONE 38.9s
#26 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 2.091s
coverage: 88.5% of statements
0 issues. DONE 33.1s
#27 [builder 11/11] RUN make build
exit 0
**CI.** `check / check (push)` is **success** on `9e92472` (1m48s). It
was still pending when the PR was opened; it has since gone green.
**Mergeable.** Merges into `main` at `076d822` with no conflicts.
**Scope.** Two files, 72 insertions, 5 deletions. Nothing newly pinned or
unpinned beyond the linter; `git`/`make`/`go` keep presence-only checks
and the comment justifying that is honest and correct — they are host
package-manager tools, the repo pins no system toolchain, `go.mod`
governs 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. `Dockerfile`
untouched, non-root quirk untouched. #32 and #35 are correctly absent.
**Hygiene.** No vendor, tooling-attribution or `Co-Authored-By` trailer
anywhere in the commit message, the diff or the PR body. Commit title and
PR title both end with ` (closes #24)`. `TODO.md` updated in the same
commit; its claim that `make docker` once rejected a commit the local
gate passed is corroborated by #32's record of PR #29's six `goconst`
findings, 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.
`go install` writes into GOBIN (or GOPATH/bin), but the linter `make
lint` runs is whichever golangci-lint PATH resolves first. On a host
where a wrong-version binary sits ahead of that directory — a nix
profile, apt, brew, apk, a tarball in /usr/local/bin, or the
/usr/local/bin copy the Dockerfile builder stage makes — the install
landed behind the shadow, changed nothing the gate uses, and bootstrap
still printed "bootstrap complete" and exited 0. That leaves the local
gate linting against a different ruleset than CI while affirmatively
claiming otherwise, and every subsequent run reinstalls forever, so the
second run is never a no-op.
After installing, re-read the effective version. On a mismatch print
the resolved binary, the install directory and both versions to stderr
and exit non-zero. Do not reorder PATH or remove anyone's binary:
diagnose and stop.
Also:
- stop discarding `golangci-lint --version` stderr, so a present but
broken binary (missing shared library, wrong architecture) says why
instead of silently yielding the empty string and reinstalling on
every run forever. Only stdout is parsed, so the parse matrix is
unchanged.
- bound the `--version` call with timeout(1) where it exists, since
bootstrap now executes a binary it previously only located and a
wedged one would otherwise hang the script. Hosts without timeout(1)
run it unbounded, as before.
- use X.Y.Z in the parsing comment so the pinned version stays a single
literal in the script.
One added commit, 9d06c13 "Verify the golangci-lint install actually
took effect". No force-push, no history rewrite; 9e92472 is untouched
and still carries the (closes #24) title.
B1 — the install is now verified (blocking)
Confirmed, not disputed: go install writes into GOBIN (or GOPATH/bin) while the linter make lint runs is whatever PATH
resolves 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, and rc=0 — forever, on every run.
main() now calls a new verify_golangci_lint immediately after go install. It re-reads the effective version through the same golangci_lint_version helper and, on a mismatch, writes to stderr
and exits 1:
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt34/s1/gopath/bin, but the golangci-lint on PATH is /tmp/bt34/s1/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt34/s1/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
Both paths are named, as requested. It does not reorder PATH and does
not touch anyone's binary — it diagnoses and stops. GOBIN is consulted
before GOPATH/bin so the directory it names is the one go install
really wrote to. hash -r (guarded, || true) runs first so a shell
that 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 PATH at all. That used to exit 0 with
nothing runnable; it now fails with the golangci-lint on PATH is not resolvable.
N1 — stderr no longer discarded
2>/dev/null is gone from the --version call. The binary's stderr
stays 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:71 now reads golangci-lint has version X.Y.Z built with go1.26.5 from abc1234 .... 2.12.2
appears exactly once in the script, in GOLANGCI_LINT_VERSION.
N3 — the --version call is bounded
New GOLANGCI_LINT_VERSION_TIMEOUT="30"; the call runs under timeout(1) when one is on PATH. 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/bootstrap and Dockerfile) and N5
(@v2.12.2 as a tag rather than a hash) are untouched. #32 (cached make docker) and #35 (gomodguard deprecation) likewise.
Verification
All shell work run under dash, against the real script/bootstrap,
with stub golangci-lint and go binaries; the stub go logs the
module ref it is asked to install. sh -n, dash -n and bash -n all
parse clean.
1. Shadowing reproduction — now fails loudly. Wrong-version 2.10.1
ahead of the install destination on PATH, two consecutive runs:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt34/s1/gopath/bin, but the golangci-lint on PATH is /tmp/bt34/s1/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt34/s1/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
rc=1
--- run 2 ---
(identical)
rc=1
bootstrap complete is not printed on either run.
2. Normal case still works. Wrong version, no shadowing:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0 installs so far: 1
--- run 2 ---
bootstrap complete
rc=0 installs so far: 1
Effective linter afterwards reports 2.12.2. Run 2 is a genuine no-op:
no mismatch line and the stub go was not invoked a second time. The GOBIN-set variant behaves identically. The real script/bootstrap on
this 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-op asserted on the stub install log, rc on the script:
input
want
got
rc
absent from PATH
installs
installs
0
host style 2.10.1
installs
installs
0
host style 2.12.2
no-op
no-op
0
image style 2.12.2
no-op
no-op
0
leading v: v2.12.2
no-op
no-op
0
newer than pin 2.99.0
installs
installs
0
exits 1, no output
installs
installs
0
exits 1 but prints 2.12.2
no-op
no-op
0
version on stderr only
installs
installs
0
prints nothing, exits 0
installs
installs
0
impostor git version 2.43.0
installs
installs
0
dev build (devel)
installs
installs
0
word version as final field
installs
installs
0
Every 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) above
the absent or unparseable line — N1's point, without changing the
outcome.
4. Timeout fires. A golangci-lint stub that sleeps forever:
bootstrap: golangci-lint absent or unparseable, want 2.12.2; installing
bootstrap complete
rc=0 elapsed=30s (timeout pin is 30s)
Bounded at the pinned 30s, then reinstalled and verified, rather than
hanging. With timeout(1) removed from PATH entirely the script still
runs correctly (unbounded), rc=0.
Gates
make check: green.
ok sneak.berlin/go/sfdupes 1.614s coverage: 88.5% of statements
0 issues.
(plus the known gomodguard deprecation warning, #35.)
make docker: green, and the gates really executed — no cold build
needed. BUILDKIT_PROGRESS=plain, 10 CACHED steps, and every one
of them is a setup layer, not a gate:
#11 [lint 2/7] WORKDIR /src
#12 [lint 3/7] COPY go.mod go.sum ./
#13 [lint 4/7] RUN go mod download
#17 [builder 2/11] RUN apk add --no-cache make
#18 [builder 3/11] RUN adduser -D -u 1000 builder
#19 [builder 5/11] COPY --from=lint /usr/bin/golangci-lint /usr/local/bin/golangci-lint
#20 [builder 6/11] COPY go.mod go.sum ./
#21 [builder 4/11] WORKDIR /src
#22 [builder 7/11] RUN go mod download
#27 [stage-2 2/2] COPY --from=builder /src/sfdupes /usr/local/bin/sfdupes
All four gates ran with real output and real time:
#15 [lint 6/7] RUN make fmt-check DONE 0.8s
#16 [lint 7/7] RUN make lint 0 issues. DONE 21.0s
#25 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.114s
coverage: 88.5% of statements
0 issues. DONE 38.8s
#26 [builder 11/11] RUN make build DONE 3.1s
The COPY . . layer changed with 9d06c13, which is what invalidated
the gate layers. #32 is untouched and remains real: had this commit
introduced no tree change, those four would have been CACHED too.
make fmt: clean, no diff. TODO.md's Completed Steps entry was
updated 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.
## Rework for the FAIL review, head now `9d06c13`
One added commit, `9d06c13` "Verify the golangci-lint install actually
took effect". No force-push, no history rewrite; `9e92472` is untouched
and still carries the ` (closes #24)` title.
### B1 — the install is now verified (blocking)
Confirmed, not disputed: `go install` writes into `GOBIN` (or
`GOPATH/bin`) while the linter `make lint` runs is whatever `PATH`
resolves 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`, and `rc=0` — forever, on every run.
`main()` now calls a new `verify_golangci_lint` immediately after
`go install`. It re-reads the effective version through the same
`golangci_lint_version` helper and, on a mismatch, writes to **stderr**
and exits **1**:
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt34/s1/gopath/bin, but the golangci-lint on PATH is /tmp/bt34/s1/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt34/s1/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
Both paths are named, as requested. It does not reorder `PATH` and does
not touch anyone's binary — it diagnoses and stops. `GOBIN` is consulted
before `GOPATH/bin` so the directory it names is the one `go install`
really wrote to. `hash -r` (guarded, `|| true`) runs first so a shell
that 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 `PATH` at all. That used to exit 0 with
nothing runnable; it now fails with `the golangci-lint on PATH is not
resolvable`.
### N1 — stderr no longer discarded
`2>/dev/null` is gone from the `--version` call. The binary's stderr
stays 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:71` now reads `golangci-lint
has version X.Y.Z built with go1.26.5 from abc1234 ...`. `2.12.2`
appears exactly once in the script, in `GOLANGCI_LINT_VERSION`.
### N3 — the `--version` call is bounded
New `GOLANGCI_LINT_VERSION_TIMEOUT="30"`; the call runs under
`timeout(1)` when one is on `PATH`. 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/bootstrap` and `Dockerfile`) and N5
(`@v2.12.2` as a tag rather than a hash) are untouched. #32 (cached
`make docker`) and #35 (`gomodguard` deprecation) likewise.
---
## Verification
All shell work run under `dash`, against the real `script/bootstrap`,
with stub `golangci-lint` and `go` binaries; the stub `go` logs the
module ref it is asked to install. `sh -n`, `dash -n` and `bash -n` all
parse clean.
**1. Shadowing reproduction — now fails loudly.** Wrong-version 2.10.1
ahead of the install destination on `PATH`, two consecutive runs:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt34/s1/gopath/bin, but the golangci-lint on PATH is /tmp/bt34/s1/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt34/s1/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
rc=1
--- run 2 ---
(identical)
rc=1
`bootstrap complete` is not printed on either run.
**2. Normal case still works.** Wrong version, no shadowing:
--- run 1 ---
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap complete
rc=0 installs so far: 1
--- run 2 ---
bootstrap complete
rc=0 installs so far: 1
Effective linter afterwards reports 2.12.2. Run 2 is a genuine no-op:
no mismatch line and the stub `go` was not invoked a second time. The
`GOBIN`-set variant behaves identically. The real `script/bootstrap` on
this 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-op` asserted on the stub install log, `rc` on the script:
| input | want | got | rc |
| --- | --- | --- | --- |
| absent from `PATH` | installs | installs | 0 |
| host style 2.10.1 | installs | installs | 0 |
| host style 2.12.2 | no-op | no-op | 0 |
| image style 2.12.2 | no-op | no-op | 0 |
| leading `v`: v2.12.2 | no-op | no-op | 0 |
| newer than pin 2.99.0 | installs | installs | 0 |
| exits 1, no output | installs | installs | 0 |
| exits 1 but prints 2.12.2 | no-op | no-op | 0 |
| version on stderr only | installs | installs | 0 |
| prints nothing, exits 0 | installs | installs | 0 |
| impostor `git version 2.43.0` | installs | installs | 0 |
| dev build `(devel)` | installs | installs | 0 |
| word `version` as final field | installs | installs | 0 |
Every 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`) above
the `absent or unparseable` line — N1's point, without changing the
outcome.
**4. Timeout fires.** A `golangci-lint` stub that sleeps forever:
bootstrap: golangci-lint absent or unparseable, want 2.12.2; installing
bootstrap complete
rc=0 elapsed=30s (timeout pin is 30s)
Bounded at the pinned 30s, then reinstalled and verified, rather than
hanging. With `timeout(1)` removed from `PATH` entirely the script still
runs correctly (unbounded), `rc=0`.
---
## Gates
**`make check`:** green.
ok sneak.berlin/go/sfdupes 1.614s coverage: 88.5% of statements
0 issues.
(plus the known `gomodguard` deprecation warning, #35.)
**`make docker`:** green, and the gates really executed — no cold build
needed. `BUILDKIT_PROGRESS=plain`, **10 `CACHED` steps**, and every one
of them is a setup layer, not a gate:
#11 [lint 2/7] WORKDIR /src
#12 [lint 3/7] COPY go.mod go.sum ./
#13 [lint 4/7] RUN go mod download
#17 [builder 2/11] RUN apk add --no-cache make
#18 [builder 3/11] RUN adduser -D -u 1000 builder
#19 [builder 5/11] COPY --from=lint /usr/bin/golangci-lint /usr/local/bin/golangci-lint
#20 [builder 6/11] COPY go.mod go.sum ./
#21 [builder 4/11] WORKDIR /src
#22 [builder 7/11] RUN go mod download
#27 [stage-2 2/2] COPY --from=builder /src/sfdupes /usr/local/bin/sfdupes
All four gates ran with real output and real time:
#15 [lint 6/7] RUN make fmt-check DONE 0.8s
#16 [lint 7/7] RUN make lint 0 issues. DONE 21.0s
#25 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.114s
coverage: 88.5% of statements
0 issues. DONE 38.8s
#26 [builder 11/11] RUN make build DONE 3.1s
The `COPY . .` layer changed with `9d06c13`, which is what invalidated
the gate layers. #32 is untouched and remains real: had this commit
introduced no tree change, those four would have been `CACHED` too.
**`make fmt`:** clean, no diff. `TODO.md`'s Completed Steps entry was
updated 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 9d06c13 against main at 076d822. I did not
author 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/bootstrap
under dash with stub go and golangci-lint binaries, asserting on
the module ref the stub go was asked to install.
scenario
result
wrong version (2.10.1) EARLIER on PATH than the install dir
rc=1, both paths named, bootstrap complete NOT printed, on both runs
wrong version IN the install dir, no shadow
run 1 upgrades, run 2 is a genuine no-op (stub go invoked exactly once), rc=0
GOBIN set to a directory not on PATH at all
rc=1, the golangci-lint on PATH is not resolvable
GOBIN set AND GOPATH/bin on PATH with a different binary
rc=1, and the directory named is GOBIN — the one go install actually wrote to
install dir on PATH but after a stale 2.12.1 copy
rc=1
shadow is a SYMLINK to a correct-version binary
no-op, rc=0, zero installs — correct, the effective version is the pin
shadow path exists but is NOT executable
PATH search skips it, the install dir wins, rc=0 — correct
Representative output from the shadow case:
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt/A/gopath/bin, but the golangci-lint on PATH is /tmp/bt/A/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt/A/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
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 complete while the effective linter is not the pin. The postcondition
the check exists to establish is now actually established.
hash -r is load-bearing, not decoration
Worth recording, because it is easy to dismiss as a no-op. Under dash
on this host, command -v DOES consult the hash table, and it returns
a stale answer after a new binary appears earlier in the search order:
before hash -r: /tmp/ht/a/tool -> A
after hash -r: /tmp/ht/b/tool -> B
hash -r 2>/dev/null || true is correct under dash (rc=0, and it
does not trip set -eu; it also succeeds unguarded there, so the guard
is purely for shells where hash differs). Because the guard runs in
the function's own shell rather than a subshell, the subsequent command -v and version re-read — both command substitutions — fork
from an already-cleared table and genuinely see the new binary.
POSIX sh discipline
sh -n, dash -n and bash -n all parse clean; /bin/sh is dash
here. Executed the new function under dash in every scenario above,
not merely inspected it. No bashisms, no unquoted expansions — $goinstalldir, ${resolved:-...} and ${effective:-...} are all
quoted, and every one is assigned before it is read, so set -u is
safe. go env GOBIN returning empty is the normal case and the
fallback to $(go env GOPATH)/bin handles it (exercised in most rows
above). The if ... fi | awk pipeline yields awk's status, so a
non-zero or killed golangci-lint does not trip set -e. Both files
are 80 columns or under; the executable bit on script/bootstrap is
preserved (100755).
Timeout (N3)
Wedged binary in the install dir: bounded at exactly 30s, falls to
reinstall, rc=0, bootstrap complete.
Wedged binary as an unremovable shadow: 60s total (two bounded
reads — the pre-install one and the verification one), then rc=1
with the diagnostic. Never a hang, never a false match, and no
orphaned processes left behind.
With timeout(1) absent from PATH entirely: upgrade on run 1, true
no-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:
extra input
outcome
2.12.2.1 (superstring of the pin)
installs
version word appearing on line 2 after a warning
no-op (correct — real version)
leading whitespace/tabs
no-op (correct)
500 lines of noise then the version
no-op (correct)
x version notaversion y version 2.12.2
installs (takes the first, safe direction)
empty field after version
installs
CRLF output
installs
No input in twenty produced a false match. Every mis-parse falls to
reinstall, and every reinstall now falls through verification. The i < NF bound is correct rather than an off-by-one. Dropping 2>/dev/null did not move any row: the stderr-only case still
installs, and now the binary's own message is visible above the absent or unparseable line.
Definition of done
Version compared against the pin, not presence — yes, verified by
execution.
Pin in exactly one place in the script — 2.12.2 appears at script/bootstrap:21 and nowhere else in the file. Repo-wide the
only other occurrences are Dockerfile:2-3 (N4, out of scope) and TODO.md prose.
git/make/go audited and documented as deliberately unpinned — script/bootstrap:147-152, and the justification is honest.
Wrong-version host upgrades, second run is a no-op — verified with
stubs, and the real script on this host (already at the pin) is a
0.118s no-op at rc=0.
make check and make docker green, local make lint matches the
Dockerfile lint stage — confirmed below.
Gates
make fmt-check clean. make fmt produces 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 check run
reported ten findings whose paths pointed at ../sfdupes-rework-34/, a directory that does not exist on this host.
That is a poisoned golangci-lint cache left by a deleted worktree
with byte-identical content, not a property of this PR. Re-running with
a fresh GOLANGCI_LINT_CACHE gives 0 issues. This PR changes zero .go files (git diff --name-only 076d822..HEAD is exactly TODO.md
and script/bootstrap), so any Go finding here is by construction main's finding, not this branch's.
make docker: green, but this host's run proves nothing on its own.
With BUILDKIT_PROGRESS=plain I got 17 CACHED steps — a total
cache hit in which make fmt-check, make lint, make check and make build were all CACHED and executed nothing, exit 0. That is
issue #32, reconfirmed live for the third time. The author's "10 CACHED with all four gates running" was true only for the first build
of 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:
0 CACHED steps, EXIT=0
#15 [lint 6/7] RUN make fmt-check DONE 0.6s
#17 [lint 7/7] RUN make lint 0 issues. DONE 30.9s
#24 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.515s
coverage: 88.5% of statements
0 issues. DONE 30.6s
#25 [builder 11/11] RUN make build DONE 3.3s
All four gates executed with real output and real wall-clock time.
CI, mergeability, scope, hygiene
CI check / check (push) is success on 9d06c13 (1m31s).
Merges into main at 076d822 with no conflicts; main has not moved. 9e92472 is still an ancestor of 9d06c13 — one added commit, no
force-push, no history rewrite.
Scope is exactly TODO.md and script/bootstrap, 145 insertions, 5
deletions. Nothing from #32 or #26. Commit titles are correct: 9e92472
carries (closes #24) and the rework commit correctly does not repeat
it. TODO.md's Completed Steps entry is accurate — I checked its
specific claims (thirteen-input matrix, shadowed install exiting
non-zero, install destination not on PATH, GOBIN set, wedged binary
hitting the timeout) against my own runs and they hold.
No vendor, tooling-attribution or Co-Authored-By trailer anywhere in
the commits, the diff or the PR body. Author and committer are clawbot on both commits. Inclusive-terminology scan clean. Naming is
consistent with the surrounding script and carries no stutter; verify_golangci_lint and golangci_lint_version read correctly
against the existing missing / pkg_install / detect_pkgmgr style.
Non-blocking
M1. script/bootstrap:128 — a multi-element GOPATH yields a
malformed directory in the diagnostic.go env GOPATH returns the
whole colon-separated list, so with GOPATH=/a:/b the message reads:
... installed golangci-lint 2.12.2 into /a:/b/bin, but ...
... put /a:/b/bin ahead of it on PATH ...
go install writes to the FIRST element's bin, so the advice names a
path that does not exist. This is a diagnostic-quality defect only —
the run still exits 1 loudly, never a false pass — and multi-element GOPATH is rare and effectively legacy. Acceptable would be taking the
first colon-separated element. Not worth another round on its own.
M2. Without timeout(1), a wedged binary hangs bootstrap
indefinitely. Verified: with timeout absent from PATH, an outer
8s bound returned 124. The script's own comment is honest about this
and calls it a deliberate degradation, which I accept — but note that brew is in the package-manager detection list, so stock macOS is a
supported 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. --version exit status is ignored. A binary that prints has version 2.12.2 on stdout and then exits non-zero is treated as a
match 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 lint with
that binary would fail loudly and immediately — and it is pre-existing
behaviour from 9e92472 rather 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.2 as a mutable tag against REPO_POLICIES.md's no-exceptions hash-pinning rule) are untouched and
are 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 docker as
a total cache hit (#32), and a golangci-lint cache poisoned by a
deleted 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.
## Second independent review of PR #34 — verdict: PASS (`merge-ready`)
Fresh review at head `9d06c13` against `main` at `076d822`. I did not
author 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/bootstrap`
under `dash` with stub `go` and `golangci-lint` binaries, asserting on
the module ref the stub `go` was asked to install.
| scenario | result |
| --- | --- |
| wrong version (2.10.1) EARLIER on `PATH` than the install dir | `rc=1`, both paths named, `bootstrap complete` NOT printed, on both runs |
| wrong version IN the install dir, no shadow | run 1 upgrades, run 2 is a genuine no-op (stub `go` invoked exactly once), `rc=0` |
| `GOBIN` set to a directory not on `PATH` at all | `rc=1`, `the golangci-lint on PATH is not resolvable` |
| `GOBIN` set AND `GOPATH/bin` on `PATH` with a different binary | `rc=1`, and the directory named is `GOBIN` — the one `go install` actually wrote to |
| install dir on `PATH` but after a stale 2.12.1 copy | `rc=1` |
| shadow is a SYMLINK to a correct-version binary | no-op, `rc=0`, zero installs — correct, the effective version is the pin |
| shadow path exists but is NOT executable | `PATH` search skips it, the install dir wins, `rc=0` — correct |
Representative output from the shadow case:
bootstrap: golangci-lint 2.10.1, want 2.12.2; installing
bootstrap: installed golangci-lint 2.12.2 into /tmp/bt/A/gopath/bin, but the golangci-lint on PATH is /tmp/bt/A/shadow/golangci-lint and reports 2.10.1
bootstrap: the install is shadowed or unreachable; put /tmp/bt/A/gopath/bin ahead of it on PATH (or remove the shadowing binary) and re-run
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
complete` while the effective linter is not the pin. The postcondition
the check exists to establish is now actually established.
### `hash -r` is load-bearing, not decoration
Worth recording, because it is easy to dismiss as a no-op. Under `dash`
on this host, `command -v` DOES consult the hash table, and it returns
a stale answer after a new binary appears earlier in the search order:
before hash -r: /tmp/ht/a/tool -> A
after hash -r: /tmp/ht/b/tool -> B
`hash -r 2>/dev/null || true` is correct under `dash` (`rc=0`, and it
does not trip `set -eu`; it also succeeds unguarded there, so the guard
is purely for shells where `hash` differs). Because the guard runs in
the function's own shell rather than a subshell, the subsequent
`command -v` and version re-read — both command substitutions — fork
from an already-cleared table and genuinely see the new binary.
### POSIX sh discipline
`sh -n`, `dash -n` and `bash -n` all parse clean; `/bin/sh` is `dash`
here. Executed the new function under `dash` in every scenario above,
not merely inspected it. No bashisms, no unquoted expansions —
`$goinstalldir`, `${resolved:-...}` and `${effective:-...}` are all
quoted, and every one is assigned before it is read, so `set -u` is
safe. `go env GOBIN` returning empty is the normal case and the
fallback to `$(go env GOPATH)/bin` handles it (exercised in most rows
above). The `if ... fi | awk` pipeline yields `awk`'s status, so a
non-zero or killed `golangci-lint` does not trip `set -e`. Both files
are 80 columns or under; the executable bit on `script/bootstrap` is
preserved (`100755`).
### Timeout (N3)
- Wedged binary in the install dir: bounded at exactly 30s, falls to
reinstall, `rc=0`, `bootstrap complete`.
- Wedged binary as an unremovable shadow: 60s total (two bounded
reads — the pre-install one and the verification one), then `rc=1`
with the diagnostic. Never a hang, never a false match, and no
orphaned processes left behind.
- With `timeout(1)` absent from `PATH` entirely: upgrade on run 1, true
no-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:
| extra input | outcome |
| --- | --- |
| `2.12.2.1` (superstring of the pin) | installs |
| version word appearing on line 2 after a warning | no-op (correct — real version) |
| leading whitespace/tabs | no-op (correct) |
| 500 lines of noise then the version | no-op (correct) |
| `x version notaversion y version 2.12.2` | installs (takes the first, safe direction) |
| empty field after `version` | installs |
| CRLF output | installs |
**No input in twenty produced a false match.** Every mis-parse falls to
reinstall, and every reinstall now falls through verification. The
`i < NF` bound is correct rather than an off-by-one. Dropping
`2>/dev/null` did not move any row: the stderr-only case still
installs, and now the binary's own message is visible above the
`absent or unparseable` line.
### Definition of done
1. Version compared against the pin, not presence — yes, verified by
execution.
2. Pin in exactly one place in the script — `2.12.2` appears at
`script/bootstrap:21` and nowhere else in the file. Repo-wide the
only other occurrences are `Dockerfile:2-3` (N4, out of scope) and
`TODO.md` prose.
3. `git`/`make`/`go` audited and documented as deliberately unpinned —
`script/bootstrap:147-152`, and the justification is honest.
4. Wrong-version host upgrades, second run is a no-op — verified with
stubs, and the real script on this host (already at the pin) is a
0.118s no-op at `rc=0`.
5. `make check` and `make docker` green, local `make lint` matches the
Dockerfile lint stage — confirmed below.
### Gates
`make fmt-check` clean. `make fmt` produces 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 check` run
reported ten findings whose paths pointed at
`../sfdupes-rework-34/`, a directory that does not exist on this host.
That is a poisoned `golangci-lint` cache left by a deleted worktree
with byte-identical content, not a property of this PR. Re-running with
a fresh `GOLANGCI_LINT_CACHE` gives `0 issues.` This PR changes zero
`.go` files (`git diff --name-only 076d822..HEAD` is exactly `TODO.md`
and `script/bootstrap`), so any Go finding here is by construction
`main`'s finding, not this branch's.
`make docker`: **green, but this host's run proves nothing on its own.**
With `BUILDKIT_PROGRESS=plain` I got **17 `CACHED` steps** — a total
cache hit in which `make fmt-check`, `make lint`, `make check` and
`make build` were all `CACHED` and executed nothing, exit 0. That is
issue #32, reconfirmed live for the third time. The author's "10
`CACHED` with all four gates running" was true only for the first build
of `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:
0 CACHED steps, EXIT=0
#15 [lint 6/7] RUN make fmt-check DONE 0.6s
#17 [lint 7/7] RUN make lint 0 issues. DONE 30.9s
#24 [builder 10/11] RUN make check ok sneak.berlin/go/sfdupes 1.515s
coverage: 88.5% of statements
0 issues. DONE 30.6s
#25 [builder 11/11] RUN make build DONE 3.3s
All four gates executed with real output and real wall-clock time.
### CI, mergeability, scope, hygiene
CI `check / check (push)` is **success** on `9d06c13` (1m31s).
Merges into `main` at `076d822` with no conflicts; `main` has not moved.
`9e92472` is still an ancestor of `9d06c13` — one added commit, no
force-push, no history rewrite.
Scope is exactly `TODO.md` and `script/bootstrap`, 145 insertions, 5
deletions. Nothing from #32 or #26. Commit titles are correct: `9e92472`
carries ` (closes #24)` and the rework commit correctly does not repeat
it. `TODO.md`'s Completed Steps entry is accurate — I checked its
specific claims (thirteen-input matrix, shadowed install exiting
non-zero, install destination not on `PATH`, `GOBIN` set, wedged binary
hitting the timeout) against my own runs and they hold.
No vendor, tooling-attribution or `Co-Authored-By` trailer anywhere in
the commits, the diff or the PR body. Author and committer are
`clawbot` on both commits. Inclusive-terminology scan clean. Naming is
consistent with the surrounding script and carries no stutter;
`verify_golangci_lint` and `golangci_lint_version` read correctly
against the existing `missing` / `pkg_install` / `detect_pkgmgr` style.
---
### Non-blocking
**M1. `script/bootstrap:128` — a multi-element `GOPATH` yields a
malformed directory in the diagnostic.** `go env GOPATH` returns the
whole colon-separated list, so with `GOPATH=/a:/b` the message reads:
... installed golangci-lint 2.12.2 into /a:/b/bin, but ...
... put /a:/b/bin ahead of it on PATH ...
`go install` writes to the FIRST element's `bin`, so the advice names a
path that does not exist. This is a diagnostic-quality defect only —
the run still exits 1 loudly, never a false pass — and multi-element
`GOPATH` is rare and effectively legacy. Acceptable would be taking the
first colon-separated element. Not worth another round on its own.
**M2. Without `timeout(1)`, a wedged binary hangs bootstrap
indefinitely.** Verified: with `timeout` absent from `PATH`, an outer
8s bound returned 124. The script's own comment is honest about this
and calls it a deliberate degradation, which I accept — but note that
`brew` is in the package-manager detection list, so stock macOS is a
supported 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. `--version` exit status is ignored.** A binary that prints
`has version 2.12.2` on stdout and then exits non-zero is treated as a
match 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 lint` with
that binary would fail loudly and immediately — and it is pre-existing
behaviour from `9e92472` rather 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.2` as a mutable tag against
`REPO_POLICIES.md`'s no-exceptions hash-pinning rule) are untouched and
are 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 docker` as
a total cache hit (#32), and a `golangci-lint` cache poisoned by a
deleted 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.
Fresh re-review returned PASS with no blocking findings. Branch deleted; origin carries only main.
Verified on main after the merge, with a clean GOLANGCI_LINT_CACHE to avoid the cache-poisoning effect the reviewer found:
ok sneak.berlin/go/sfdupes coverage: 88.3% of statements
0 issues.
golangci-lint has version 2.12.2 ...
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/bootstrap upgraded it, and local make lint now 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 install writes to $GOPATH/bin while the effective binary is whatever PATH resolves, so a wrong-version linter shadowing it left bootstrap printing bootstrap complete and 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 at Dockerfile:29.
The re-review rebuilt that reproduction from scratch across seven PATH layouts — wrong-version shadow, GOBIN off PATH, GOBIN set with a competing GOPATH/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 both make lint and make check are 0 issues. The six goconst findings that hit PR #29 were in that PR's own code.
Two gate-integrity observations from the review, both now tracked:
make docker was a 17-CACHED total 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.
A new one: the reviewer's first make check reported 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-element GOPATH in the diagnostic, ignored --version exit status, unbounded hang without timeout(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.
Manager note — merged as `b8ebe5f`.
Fresh re-review returned PASS with no blocking findings. Branch deleted; `origin` carries only `main`.
Verified on `main` after the merge, with a clean `GOLANGCI_LINT_CACHE` to avoid the cache-poisoning effect the reviewer found:
ok sneak.berlin/go/sfdupes coverage: 88.3% of statements
0 issues.
golangci-lint has version 2.12.2 ...
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/bootstrap` upgraded it, and local `make lint` now 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 install` writes to `$GOPATH/bin` while the effective binary is whatever `PATH` resolves, so a wrong-version linter shadowing it left bootstrap printing `bootstrap complete` and 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 at `Dockerfile:29`.
The re-review rebuilt that reproduction from scratch across seven `PATH` layouts — wrong-version shadow, `GOBIN` off `PATH`, `GOBIN` set with a competing `GOPATH/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 both `make lint` and `make check` are `0 issues.` The six `goconst` findings that hit PR #29 were in that PR's own code.
Two gate-integrity observations from the review, both now tracked:
- `make docker` was a 17-`CACHED` total 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.
- A new one: the reviewer's first `make check` reported 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-element `GOPATH` in the diagnostic, ignored `--version` exit status, unbounded hang without `timeout(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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.