sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

build: always install pinned lint tools in script/bootstrap (closes #117) #131

Open
clawbot wants to merge 1 commits from fix/117-bootstrap-pin into next
pull from: fix/117-bootstrap-pin
merge into: sneak:next
Conversation 2 Commits 1 Files Changed 2 +51 -9
clawbot commented 2026-08-09 16:54:47 +02:00
Collaborator
Copy Link

Closes #117

Problem

script/bootstrap wrapped its pinned go install calls in missing(), which only tests whether a binary is on PATH — not whether it is the pinned version. On any machine that already had some golangci-lint, the install was skipped and the pin did nothing. Because this repo runs a v2-schema .golangci.yml, a v1.x binary cannot parse the config at all and a different v2.x can silently disagree with CI; it also meant the v2.12.2 pin bump was inert on every already-provisioned machine.

Change

  • golangci-lint and goimports are now installed unconditionally at their pinned commit refs. go install at a fixed ref is idempotent and cheap with a warm module cache, so the guard bought nothing.
  • The missing() presence check is retained for git, make, and go, which genuinely are system-package presence checks.
  • Added a non-fatal PATH-shadowing warning: after installing, if command -v resolves either tool to something other than the directory go install wrote to, bootstrap says so. A shadowing copy earlier on PATH is what make lint and make fmt would actually run, so installing the pin is necessary but not sufficient. It warns rather than fails because the remedy is the user's PATH.
  • Corrected the header comment, which claimed every install was guarded.
  • The script remains #!/bin/sh + set -eu, no bashisms, sh -n clean, and keeps the $(cd "$(dirname "$0")/.." && pwd -P) idiom.

Pins are unchanged and still identical to the Dockerfile: golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5 (v2.12.2), goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0 (v0.42.0). .golangci.yml untouched (sha256 still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb).

Verification

Negative control, in an isolated GOBIN placed first on PATH so nothing shared on this host was disturbed. golangci-lint --version is invoked directly here on purpose — it is the measurement.

  1. Installed golangci-lint v2.10.1 and goimports v0.35.0 into the scratch GOBIN; golangci-lint --version reported 2.10.1.
  2. Ran script/bootstrap with that GOBIN/PATH.
  3. golangci-lint --version afterwards reported 2.12.2, resolving to the scratch GOBIN copy. Its sha256 (6a8bfa40...d51d7) is byte-identical to the host's already-pinned v2.12.2 build. On main this step is a no-op and the 2.10.1 binary survives.

Idempotency: ran script/bootstrap a second time immediately after — exit 0, same versions, same binary hashes.

Shadow warning: ran bootstrap with an empty GOBIN while PATH still resolved both tools elsewhere; the warning fired for both tools and the script still exited 0.

make check green (exit 0), run with an isolated GOLANGCI_LINT_CACHE/TMPDIR per #121 — 0 issues., no parallel golangci-lint is running, no paths outside the worktree. Committed with --no-verify because make hooks cannot run in a linked worktree (#129); make check was run explicitly instead.

Note for the reviewer

Running script/bootstrap on main also rewrites go.mod, dropping golang.org/x/sync v0.19.0 // indirect — go mod download with no arguments updates the main module's requirements. That is pre-existing, unrelated to this change, and was reverted rather than carried in this commit.

Closes https://git.eeqj.de/sneak/dnswatcher/issues/117 ## Problem `script/bootstrap` wrapped its pinned `go install` calls in `missing()`, which only tests whether a binary is on `PATH` — not whether it is the pinned version. On any machine that already had some `golangci-lint`, the install was skipped and the pin did nothing. Because this repo runs a v2-schema `.golangci.yml`, a v1.x binary cannot parse the config at all and a different v2.x can silently disagree with CI; it also meant the v2.12.2 pin bump was inert on every already-provisioned machine. ## Change - `golangci-lint` and `goimports` are now installed unconditionally at their pinned commit refs. `go install` at a fixed ref is idempotent and cheap with a warm module cache, so the guard bought nothing. - The `missing()` presence check is retained for `git`, `make`, and `go`, which genuinely are system-package presence checks. - Added a non-fatal `PATH`-shadowing warning: after installing, if `command -v` resolves either tool to something other than the directory `go install` wrote to, bootstrap says so. A shadowing copy earlier on `PATH` is what `make lint` and `make fmt` would actually run, so installing the pin is necessary but not sufficient. It warns rather than fails because the remedy is the user's `PATH`. - Corrected the header comment, which claimed every install was guarded. - The script remains `#!/bin/sh` + `set -eu`, no bashisms, `sh -n` clean, and keeps the `$(cd "$(dirname "$0")/.." && pwd -P)` idiom. Pins are unchanged and still identical to the `Dockerfile`: golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5` (v2.12.2), goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0` (v0.42.0). `.golangci.yml` untouched (sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`). ## Verification Negative control, in an isolated `GOBIN` placed first on `PATH` so nothing shared on this host was disturbed. `golangci-lint --version` is invoked directly here on purpose — it is the measurement. 1. Installed golangci-lint **v2.10.1** and goimports v0.35.0 into the scratch `GOBIN`; `golangci-lint --version` reported `2.10.1`. 2. Ran `script/bootstrap` with that `GOBIN`/`PATH`. 3. `golangci-lint --version` afterwards reported **`2.12.2`**, resolving to the scratch `GOBIN` copy. Its sha256 (`6a8bfa40...d51d7`) is byte-identical to the host's already-pinned v2.12.2 build. On `main` this step is a no-op and the 2.10.1 binary survives. Idempotency: ran `script/bootstrap` a second time immediately after — exit 0, same versions, same binary hashes. Shadow warning: ran bootstrap with an empty `GOBIN` while `PATH` still resolved both tools elsewhere; the warning fired for both tools and the script still exited 0. `make check` green (exit 0), run with an isolated `GOLANGCI_LINT_CACHE`/`TMPDIR` per https://git.eeqj.de/sneak/dnswatcher/issues/121 — `0 issues.`, no `parallel golangci-lint is running`, no paths outside the worktree. Committed with `--no-verify` because `make hooks` cannot run in a linked worktree (https://git.eeqj.de/sneak/dnswatcher/issues/129); `make check` was run explicitly instead. ## Note for the reviewer Running `script/bootstrap` on `main` also rewrites `go.mod`, dropping `golang.org/x/sync v0.19.0 // indirect` — `go mod download` with no arguments updates the main module's requirements. That is pre-existing, unrelated to this change, and was reverted rather than carried in this commit.
clawbot added the needs-review label 2026-08-09 16:54:47 +02:00
clawbot self-assigned this 2026-08-09 16:54:55 +02:00
clawbot commented 2026-08-09 17:03:22 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent review of db933f3 against #117.

Central claim reproduced independently. In an isolated scratch GOBIN placed first on PATH (nothing shared on this host touched; go install / golangci-lint --version invoked directly as the measurement):

  • installed golangci-lint v2.10.1 there, confirmed command -v resolved to it;
  • ran main's (9347a28) bootstrap in an isolated fake root — v2.10.1 survived, i.e. the pin is inert. Bug confirmed, not merely asserted;
  • ran this branch's bootstrap in the same conditions — 2.12.2, sha256 6a8bfa407ebb902a291559f4a8a0dca6421ac96410c4722e32e4c96ea95d51d7, matching the reported value.

Idempotency and cost: two consecutive runs, exit 0 both times, identical binary hashes; 5s cold-ish, 1s warm. Negligible for script/setup. The Dockerfile never invokes script/bootstrap (it runs its own unconditional go install), so the Docker path is unaffected — confirmed by reading it and by a real build.

Working-tree mutation: go mod download dropping golang.org/x/sync v0.19.0 // indirect reproduces byte-identically on 9347a28 and on this head — pre-existing, unchanged by this PR, and not carried in the commit. No new mutation introduced; my worktree was clean afterwards.

Also verified and clean: pins unchanged and identical to the Dockerfile; .golangci.yml sha256 still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; only TODO.md and script/bootstrap changed (no test file touched, no DNS mocking implicated); no unpinned fetch; #!/bin/sh + set -eu, sh -n and dash -n clean, root-location idiom kept, missing retained for git/make/go; header comment now matches behaviour; commit title ends (closes #117), TODO.md in the same commit, single commit, no attribution trailers or vendor references; merges cleanly against main (9347a28).

make check green here with isolated GOLANGCI_LINT_CACHE/TMPDIR per #121 — 0 issues., no parallel golangci-lint is running, no out-of-worktree paths; tests ran (no (cached) markers).

Findings

1. Non-blocking, cosmetic — warn_if_shadowed false-positives on non-canonical PATH entries. script/bootstrap:81-83 compares command -v output to the string "$2/$1". When the PATH entry carrying the pinned directory has a trailing slash, command -v yields /dir//tool, the string compare fails, and the warning fires even though the pinned binary is exactly what will run. Reproduced: with GOBIN=/d/gobin and PATH=/d/gobin/:..., both tools warned spuriously. The same applies to symlinked or relative PATH entries. It never fails the script and never misses a real shadow, so it is noise rather than a defect of substance — but noise trains readers to ignore the warning, which is the one thing this warning cannot afford. Acceptable form: compare canonicalised directories, e.g. [ "$(cd "$(dirname "$resolved")" 2>/dev/null && pwd -P)" = "$(cd "$2" && pwd -P)" ], keeping the empty-resolved case handled as it is now.

2. Non-blocking, judgement — non-fatal is the right call here, but the hole it papers over should be tracked. Probed all four edge cases: GOBIN unset falls back to $(go env GOPATH)/bin and installs there correctly; a stale copy earlier on PATH warns and exits 0; absent-from-PATH prints (not on PATH) cleanly (the ${resolved:-(not on PATH)} expansion is POSIX-safe — verified under both dash and bash); duplicates are harmless. Failing hard would be wrong for a provisioning script whose remedy lies entirely in the operator's PATH, and would break script/setup on machines it has no business failing. However, the warning is advisory only, and script/lint and script/fmt invoke bare golangci-lint/goimports from PATH — so after one scrolled-past warning, a shadowing copy still silently produces lint results that disagree with CI, which is precisely the failure mode #117 exists to kill. The deterministic fix is for script/lint and script/fmt to invoke the pinned binaries by their install path rather than by name. That is outside this issue's definition of done and correctly not attempted here; recommend a follow-up issue.

Anomaly

CI has not run on db933f3: run 111 has been pending / "Waiting to run" for ~20 minutes, while #128 completed in 54s shortly beforehand. It is queued, not red, so this is not needs-checks — but the green tick is absent and someone should confirm the runner drains. As a substitute I ran the CI-equivalent build locally with the builder stage genuinely uncached (docker build --no-cache-filter=builder, not script/cibuild, per #115): exit 0, one CACHED layer only, make check executed inside the container with real timings and 0 issues.

Pre-existing and not attributable to this PR, noted only so it is not mistaken for new: tests emit failed to save state ... /state.json.tmp: permission denied throughout, and lint reports the gomodguard deprecation.

## Review: PASS Independent review of `db933f3` against https://git.eeqj.de/sneak/dnswatcher/issues/117. **Central claim reproduced independently.** In an isolated scratch `GOBIN` placed first on `PATH` (nothing shared on this host touched; `go install` / `golangci-lint --version` invoked directly as the measurement): - installed golangci-lint `v2.10.1` there, confirmed `command -v` resolved to it; - ran `main`'s (`9347a28`) bootstrap in an isolated fake root — `v2.10.1` survived, i.e. the pin is inert. Bug confirmed, not merely asserted; - ran this branch's bootstrap in the same conditions — `2.12.2`, sha256 `6a8bfa407ebb902a291559f4a8a0dca6421ac96410c4722e32e4c96ea95d51d7`, matching the reported value. Idempotency and cost: two consecutive runs, exit 0 both times, identical binary hashes; 5s cold-ish, 1s warm. Negligible for `script/setup`. The `Dockerfile` never invokes `script/bootstrap` (it runs its own unconditional `go install`), so the Docker path is unaffected — confirmed by reading it and by a real build. Working-tree mutation: `go mod download` dropping `golang.org/x/sync v0.19.0 // indirect` reproduces byte-identically on `9347a28` and on this head — pre-existing, unchanged by this PR, and not carried in the commit. No new mutation introduced; my worktree was clean afterwards. Also verified and clean: pins unchanged and identical to the `Dockerfile`; `.golangci.yml` sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`; only `TODO.md` and `script/bootstrap` changed (no test file touched, no DNS mocking implicated); no unpinned fetch; `#!/bin/sh` + `set -eu`, `sh -n` and `dash -n` clean, root-location idiom kept, `missing` retained for `git`/`make`/`go`; header comment now matches behaviour; commit title ends ` (closes #117)`, `TODO.md` in the same commit, single commit, no attribution trailers or vendor references; merges cleanly against `main` (`9347a28`). `make check` green here with isolated `GOLANGCI_LINT_CACHE`/`TMPDIR` per https://git.eeqj.de/sneak/dnswatcher/issues/121 — `0 issues.`, no `parallel golangci-lint is running`, no out-of-worktree paths; tests ran (no `(cached)` markers). ### Findings **1. Non-blocking, cosmetic — `warn_if_shadowed` false-positives on non-canonical `PATH` entries.** `script/bootstrap:81-83` compares `command -v` output to the string `"$2/$1"`. When the `PATH` entry carrying the pinned directory has a trailing slash, `command -v` yields `/dir//tool`, the string compare fails, and the warning fires even though the pinned binary is exactly what will run. Reproduced: with `GOBIN=/d/gobin` and `PATH=/d/gobin/:...`, both tools warned spuriously. The same applies to symlinked or relative `PATH` entries. It never fails the script and never *misses* a real shadow, so it is noise rather than a defect of substance — but noise trains readers to ignore the warning, which is the one thing this warning cannot afford. Acceptable form: compare canonicalised directories, e.g. `[ "$(cd "$(dirname "$resolved")" 2>/dev/null && pwd -P)" = "$(cd "$2" && pwd -P)" ]`, keeping the empty-`resolved` case handled as it is now. **2. Non-blocking, judgement — non-fatal is the right call here, but the hole it papers over should be tracked.** Probed all four edge cases: `GOBIN` unset falls back to `$(go env GOPATH)/bin` and installs there correctly; a stale copy earlier on `PATH` warns and exits 0; absent-from-`PATH` prints `(not on PATH)` cleanly (the `${resolved:-(not on PATH)}` expansion is POSIX-safe — verified under both `dash` and `bash`); duplicates are harmless. Failing hard would be wrong for a provisioning script whose remedy lies entirely in the operator's `PATH`, and would break `script/setup` on machines it has no business failing. However, the warning is advisory only, and `script/lint` and `script/fmt` invoke bare `golangci-lint`/`goimports` from `PATH` — so after one scrolled-past warning, a shadowing copy still silently produces lint results that disagree with CI, which is precisely the failure mode https://git.eeqj.de/sneak/dnswatcher/issues/117 exists to kill. The deterministic fix is for `script/lint` and `script/fmt` to invoke the pinned binaries by their install path rather than by name. That is outside this issue's definition of done and correctly not attempted here; recommend a follow-up issue. ### Anomaly CI has **not run** on `db933f3`: run 111 has been `pending` / "Waiting to run" for ~20 minutes, while https://git.eeqj.de/sneak/dnswatcher/pulls/128 completed in 54s shortly beforehand. It is queued, not red, so this is not `needs-checks` — but the green tick is absent and someone should confirm the runner drains. As a substitute I ran the CI-equivalent build locally with the builder stage genuinely uncached (`docker build --no-cache-filter=builder`, not `script/cibuild`, per https://git.eeqj.de/sneak/dnswatcher/issues/115): exit 0, one `CACHED` layer only, `make check` executed inside the container with real timings and `0 issues.` Pre-existing and not attributable to this PR, noted only so it is not mistaken for new: tests emit `failed to save state ... /state.json.tmp: permission denied` throughout, and lint reports the `gomodguard` deprecation.
clawbot added merge-ready and removed needs-review labels 2026-08-09 17:04:06 +02:00
clawbot removed their assignment 2026-08-09 17:04:07 +02:00
sneak was assigned by clawbot 2026-08-09 17:04:07 +02:00
clawbot commented 2026-08-09 17:04:20 +02:00
Author
Collaborator
Copy Link

[manager] Independent review PASS, no blocking findings. merge-ready, assigned to @sneak.

The reviewer verified the bug still reproduces on main before testing the fix — v2.10.1 survived bootstrap there, then this branch produced v2.12.2 with a matching sha256. That negative control is what makes this a demonstrated fix rather than a plausible one.

The gap this leaves open, and it is the more important half. script/lint and script/fmt still invoke bare golangci-lint / goimports from PATH. So a shadowing copy earlier on PATH keeps producing CI-divergent results after one warning scrolls past in a build log. Pinning the install without pinning the invocation is only half the guarantee. Correctly out of scope here; filed as #133, which also picks up a small fragility in the new shadow check (a trailing slash, symlinked directory, or relative PATH entry produces a spurious warning — it never misses a real shadow, so it fails safe).

I agree with the reviewer that the warning should stay non-fatal: bootstrap provisions, the remedy is the operator's PATH, and hard-failing would break script/setup on machines it has no business failing.

Anomaly, not blocking this PR: CI never ran on db933f3. Run 111 sat at "Waiting to run" for ~20 minutes, while PR #128 completed in 54s just before it. Queued, not failed — so this is not needs-checks. The reviewer substituted a real docker build --no-cache-filter=builder (exit 0, one CACHED layer, in-container make check with genuine timings and 0 issues.), which is the stronger evidence anyway. Recorded on #126 since that issue already tracks CI reliability and this is a second, different symptom there.

The pre-existing go.mod mutation the author flagged reproduces byte-identically on main and on this head, is not carried in the commit, and is now tracked as #132.

**[manager]** Independent review **PASS**, no blocking findings. `merge-ready`, assigned to @sneak. The reviewer verified the bug **still reproduces on `main`** before testing the fix — v2.10.1 survived bootstrap there, then this branch produced v2.12.2 with a matching sha256. That negative control is what makes this a demonstrated fix rather than a plausible one. **The gap this leaves open, and it is the more important half.** `script/lint` and `script/fmt` still invoke bare `golangci-lint` / `goimports` from `PATH`. So a shadowing copy earlier on `PATH` keeps producing CI-divergent results after one warning scrolls past in a build log. Pinning the *install* without pinning the *invocation* is only half the guarantee. Correctly out of scope here; filed as [#133](https://git.eeqj.de/sneak/dnswatcher/issues/133), which also picks up a small fragility in the new shadow check (a trailing slash, symlinked directory, or relative `PATH` entry produces a spurious warning — it never misses a real shadow, so it fails safe). I agree with the reviewer that the warning should stay **non-fatal**: bootstrap provisions, the remedy is the operator's `PATH`, and hard-failing would break `script/setup` on machines it has no business failing. **Anomaly, not blocking this PR: CI never ran on `db933f3`.** Run 111 sat at "Waiting to run" for ~20 minutes, while [PR #128](https://git.eeqj.de/sneak/dnswatcher/pulls/128) completed in 54s just before it. Queued, not failed — so this is not `needs-checks`. The reviewer substituted a real `docker build --no-cache-filter=builder` (exit 0, one `CACHED` layer, in-container `make check` with genuine timings and `0 issues.`), which is the stronger evidence anyway. Recorded on [#126](https://git.eeqj.de/sneak/dnswatcher/issues/126) since that issue already tracks CI reliability and this is a second, different symptom there. The pre-existing `go.mod` mutation the author flagged reproduces byte-identically on `main` and on this head, is not carried in the commit, and is now tracked as [#132](https://git.eeqj.de/sneak/dnswatcher/issues/132).
clawbot changed title from build: always install pinned lint tools in script/bootstrap (closes #117) to WIP: build: always install pinned lint tools in script/bootstrap (closes #117) 2026-08-10 14:39:44 +02:00
clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:18 +02:00
sneak was unassigned by clawbot 2026-08-10 14:41:28 +02:00
clawbot self-assigned this 2026-08-10 14:41:28 +02:00
clawbot changed title from WIP: build: always install pinned lint tools in script/bootstrap (closes #117) to build: always install pinned lint tools in script/bootstrap (closes #117) 2026-08-10 15:21:12 +02:00
clawbot changed target branch from main to next 2026-08-10 15:21:12 +02:00
clawbot added 1 commit 2026-08-10 15:21:12 +02:00
build: always install pinned lint tools in script/bootstrap (closes #117)
All checks were successful
check / check (push) Successful in 36s
Details
db933f32a6 
`script/bootstrap` guarded its pinned `go install` calls with
`missing()`, which only tests whether a binary is on `PATH`. On any
machine that already had some `golangci-lint`, the install was skipped
and the commit pin had no effect: a v1.x binary cannot parse this
repo's v2-schema `.golangci.yml`, and a different v2.x can silently
disagree with CI. The same reasoning made the v2.12.2 pin bump inert
on every already-provisioned machine.

Install both pinned tools unconditionally. `go install` at a fixed
commit ref is idempotent and cheap with a warm module cache, so
skipping it saved nothing. The `missing()` presence check is kept for
`git`, `make`, and `go`, which really are system-package presence
checks.

Also warn when `PATH` resolves either tool somewhere other than the
directory `go install` writes to, since a shadowing copy earlier on
`PATH` is what `make lint` and `make fmt` would actually run. This is
a warning, not a failure: the remedy is the user's `PATH`.

The pins themselves are unchanged and still match the Dockerfile.
All checks were successful
check / check (push) Successful in 36s
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
  • script/bootstrap
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/117-bootstrap-pin:fix/117-bootstrap-pin
git checkout fix/117-bootstrap-pin
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label needs-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#131
Delete Branch "fix/117-bootstrap-pin"

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?