script/lint and script/fmt invoke golangci-lint and goimports by bare name, so a PATH shadow still overrides the pin #133
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Follow-up to PR #131 (closes #117), from its review.
#117 made
script/bootstrapinstall the pinnedgolangci-lintandgoimportsunconditionally. It did not change how they are invoked:script/lintandscript/fmtstill call them by bare name, resolved throughPATH.So an older copy earlier on
PATHis still whatmake lintandmake fmtactually run. Installing the pin is necessary and not sufficient — pinning the install without pinning the invocation leaves the guarantee half-built.Why this matters more than it sounds
A wrong linter version does not fail loudly. It silently disagrees with CI, which runs the pinned binary in-container. The fleet has already seen both directions of this: a host reporting golangci-lint 2.10.1 at one point and 2.12.2 later with no repo change, and a sibling repo whose container linter surfaced 13 findings its host linter missed. That is version skew, and no amount of cache isolation (#121) touches it.
PR #131 added a non-fatal warning when
command -vresolves either tool outside thego installdestination. That is the right call for bootstrap — the remedy is the operator'sPATH, and hard-failing would breakscript/setupon machines it has no business failing. But a warning printed once during provisioning does not protect a lint run that happens hours later in a different shell.Also in scope: the shadow check is string-fragile
script/bootstrapcomparescommand -voutput against"$2/$1"by string equality. A trailing slash on thePATHentry (/dir//tool), a symlinked directory, or a relativePATHentry produces a spurious warning even when the pinned binary is what would run. Reproduced during review.It never misses a real shadow, so it fails safe — but a warning that cries wolf gets ignored, which defeats the point. Canonicalise both sides (e.g. via
pwd -Pon the directory) before comparing.Definition of done
script/lintandscript/fmtinvoke the pinned binaries at theirgo installdestination rather than by bare name — resolvego env GOBIN, falling back to$(go env GOPATH)/bin, the same way PR #131's check does.Dockerfileinstalls both tools withgo installin the builder stage, so the destination must resolve correctly there too, whereHOME/GOPATHdiffer from a developer machine. Verify with a real container build, not by reading the script — and notescript/cibuildcan report a false green on a byte-identical tree (#115, fixed in unmerged PR #122), so force a real build and say which form you ran.script/bootstrap, not a bare "command not found" or a silent fallback to whatever is onPATH. A silent fallback would reintroduce exactly this bug.script/bootstrapcanonicalises paths before comparing, so a trailing slash, symlinked directory, or relativePATHentry no longer produces a spurious warning.sh(#!/bin/sh,set -eu, no bashisms — they run in alpine with no bash);sh -nclean; keep the$(cd "$(dirname "$0")/.." && pwd -P)idiom.GOBINfirst onPATH, runmake lint, and confirm the pinned version runs — not the shadow. Do the equivalent forgoimportsandmake fmt. Use an isolated scratch directory; do not disturb anything shared on this host. Report what you planted and what ran.make checkgreen;TODO.mdupdated in the same commit.Commit title ends with
(closes #N).Constraints
c0d3ddc9cf3faa61a4e378e879ece580256d76e5; goimports stays009367f5c17a8d4c45a961a3a509277190a9a6f0. Keepscript/bootstrapand theDockerfileidentical to each other..golangci.yml— sha256 must stay021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.maketargets andscript/entrypoints only, except thatgo env,go installinto a scratchGOBIN, andgolangci-lint --versionare permitted as the measurement for item 6 — say so when you use them.Environment hazards
parallel golangci-lint is running, or names any path beginning with../, or any absolute path outside your worktree. Fix is in unmerged PR #128; isolate manually withGOLANGCI_LINT_CACHE/TMPDIRmeanwhile.make hooksfails in a linked worktree (#129) — committing with--no-verifyplus an explicitmake checkis expected.Sequencing
Land after PR #131 (introduces the shadow check being refined) and after PR #128 (also edits
script/lint). #130 and #119 touch the same two scripts — keep them separate PRs and rebase whichever lands later.Upstream
Both scripts are byte-identical across repos, so this belongs in the shared template alongside the pinned-install fix (
prompts#28). The vaultik manager described the same shape there as a "context-ungated escape hatch": a fix applied to one invocation path and not another leaves the hole open on the other.