Run all linting in Docker via Dockerfile.lint (closes #109) #165

Merged
clawbot merged 1 commits from issue-109-docker-lint into next 2026-08-18 01:07:17 +02:00
Collaborator

Closes #109.

golangci-lint no longer runs on the host. script/lint builds Dockerfile.lint, which copies the repo into the digest-pinned golangci-lint image and lints as a build step, so a successful build is a clean lint.

Changes

  • Dockerfile.lint (new). Two stages off golangci/golangci-lint:v2.12.2@sha256:5cceeef0...: a cacheable deps stage (go mod download) and a lint stage that copies the tree and runs the linter.
  • script/lint is now a POSIX-sh wrapper around docker build -f Dockerfile.lint --no-cache-filter=lint --progress=plain --output=type=cacheonly ., which additionally asserts that the linter's own summary line appears in the build output (see below).
  • Dockerfile lint stage invokes golangci-lint directly instead of make lint — it is already the pinned linter image, and make lint would now need a docker daemon inside the build.
  • script/bootstrap no longer installs golangci-lint; the version pins, sha256 constants, release-archive download and the now-unused verify_sha256 helper are gone. It warns if docker is absent. Its curl guard and script/fetch-assets call are untouched.
  • README.md: new Linting section, corrected prerequisites, and the Docker stage list fixed (it described a two-stage build that installed golangci-lint in the builder, which has not been true for a while).

TODO.md untouched.

Rebased onto next @ 41ff16a

Clean, no conflicts. #146, #152 and #157 landed since the previous head; none overlaps this PR's hunks. Verified rather than assumed:

  • Dockerfile: the only delta from the previous head cc5836d is 146's builder apt-get line gaining jq, taken verbatim. This PR's own Dockerfile delta against next is still exactly the lint-stage change.
  • README.md: 152 added the script/ci-mark-superseded Entrypoints bullet and rewrote the CI-gate-honesty tail; 146 rewrote the access-log region; 157 touched no Markdown at all. All are outside this PR's hunks. git diff origin/next HEAD -- README.md still touches only lint-related lines, so no #151 correction is clobbered.
  • 157 (landed mid-gate) is confined to internal/handlers, internal/server and templates/, so it cannot interact with this change. Rebased onto it and re-ran the full gate anyway; the numbers below are from the final head.
  • The rebase falsified nothing. Re-read Prerequisites, Development Commands, Entrypoints, Linting and Docker end to end against the tree: script/check really runs test, lint, fmt-check in that order; the Makefile really has 16 targets of which 10 shim script/, with build, run, dev, deps, clean, css inline; script/ci-mark-superseded correctly has no Makefile target and the README does not claim one.

The four non-README.md files are byte-identical to the twice-reviewed head, modulo next's own jq line above. git rev-parse HEAD:<path> at this head: Dockerfile.lint 9a9af51, script/bootstrap 49001f8, script/lint 15de890 — the same blobs the third review recorded.

Earlier rebase (README merge against #151), unchanged

README.md was the only conflicted file. 151 was a wholesale README accuracy rewrite that landed four minutes after the then-head was pushed; one of its new statements is made false by this PR, so the resolution was neither "take ours" nor "take theirs".

  • Prerequisites. Dropped 151's golangci-lint v2.12.2 ... make bootstrap installs it bullet, which this PR falsifies, and folded linting into its Docker bullet: Docker (for linting, for the test stage of the CI gate, and for containerized deployment). 151's Go 1.26.1+ bullet and its curl / script/fetch-assets bullet are kept verbatim. This branch's "golangci-lint is not a prerequisite and must not be installed on the host" paragraph is kept, extended to name script/bootstrap explicitly.
  • Development Commands. Both edits kept: 151's make fmt-check line and this branch's make lint # Run golangci-lint in Docker (Dockerfile.lint).
  • Repo tree. 151's rewritten tree taken wholesale. Only the Dockerfile.lint line is re-inserted.
  • Docker section. 151's more detailed three-stage list taken wholesale, with only the lint-stage bullet corrected to what the Dockerfile actually runs — make fmt-check, then golangci-lint config verify and golangci-lint run, both with --network=none. This branch's paragraph explaining why the stage does not call make lint is kept.

Statements that auto-merged cleanly out of 151 but were left false by this PR are corrected in the same commit rather than knowingly shipped wrong: the Quick Start comment claiming make bootstrap installs "the pinned linter"; the paragraph saying script/lint runs "whatever golangci-lint is on the host"; and the CI-gate-honesty bullet listing make lint among what the image really runs.

The script/bootstrap conflict from the first rebase is folded in and unchanged: #164's if missing curl; then pkg_install curl curl curl curl; fi and "$ROOT/script/fetch-assets" both survive; verify_sha256, install_golangci_lint_release and ensure_golangci_lint are gone, with detect_pkgmgr still reached via pkg_install.

README corrections in this round

Two statements the previous round left standing, both raised in #165 (comment). No code changed.

1. The CI-gate paragraph counted four check targets. Verified against the Dockerfile: the image runs make fmt-check, golangci-lint config verify, golangci-lint run, make test, make build — three make targets plus a direct binary invocation, not four targets. The count is dropped:

> script/cibuilddocker build . — is the CI gate: the checks run inside the image, so a build that succeeds is a repo that is formatted, linted, tested and compiled.

2. The Linting section undercounted its own guarantees. "Two properties are load-bearing" is now "Three", with the summary-line assertion added as a bullet next to the flag it guards:

> - script/lint does not trust that flag. Docker silently ignores --no-cache-filter for a stage name that does not match, so a stage rename or a one-character typo would restore the cached false green with no warning and a fast exit 0. The script therefore tees the build output and treats a run as a pass only if golangci-lint's own summary line (N issues. / N issues:) appears in it: no summary, no lint, whatever the exit code says.

Gate at 1076edb

make check:

MAKE_CHECK_EXIT=0
real    0m58.290s
#11 49.14 0 issues.
ok  sneak.berlin/go/webhooker/internal/handlers   3.580s
ok  sneak.berlin/go/webhooker/internal/server     1.972s
ok  sneak.berlin/go/webhooker/cmd/webhooker       1.058s

Disclosure: this run had 10 (cached) test lines. It is the second make check in this clone — the first, before the rebase onto 157, ran all 14 packages for real with zero cached lines (exit 0, 1m9.456s, #11 47.01 0 issues.), and the rebase only invalidated the packages 157 touched, which are exactly the three that re-executed. The image gate below re-runs everything with an empty cache.

Full image gate, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .:

DOCKER_BUILD_EXIT=0
real    3m5.210s
cached test lines: 0

#20 [lint 7/9] RUN make fmt-check
#21 [lint 8/9] RUN --network=none golangci-lint config verify --config .golangci.yml
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 48.50 0 issues.
#30 [builder  9/11] RUN make test
#30 49.45 ok  sneak.berlin/go/webhooker/cmd/webhooker      1.083s
#30 55.48 ok  sneak.berlin/go/webhooker/internal/ciscript  7.111s
#30 55.48 ok  sneak.berlin/go/webhooker/internal/handlers  4.382s
#30 55.48 ok  sneak.berlin/go/webhooker/internal/server    2.419s
#30 55.48 ok  sneak.berlin/go/webhooker/static             1.018s
#31 [builder 10/11] RUN make build
#32 [builder 11/11] RUN CGO_ENABLED=1 go build ...
#35 [stage-2 7/7] RUN chown -R webhooker:webhooker /app /var/lib/webhooker

All 13 test packages ran with real durations; the graph executed through the final stage.

Two consecutive make lint runs on an unchanged tree, git status --porcelain identical between them, both really executing:

#11 49.57 0 issues.   real 0m54.957s   LINT1_EXIT=0
#11 47.14 0 issues.   real 0m47.915s   LINT2_EXIT=0

Misspelled stage (lnit), the two-run form — run 1 executes for real because editing script/lint invalidates COPY . ., and only run 2 can hit the cache:

run 1: #11 48.18 0 issues.   real 0m49.145s   MISSPELL_RUN1_EXIT=0
run 2: #9 CACHED / #10 CACHED / #11 CACHED
       script/lint: golangci-lint printed no summary line; the linter
         did not run. Check that the stage named in --no-cache-filter
         still matches a stage in Dockerfile.lint.
       make: *** [Makefile:19: lint] Error 1
       real 0m0.232s   MISSPELL_RUN2_EXIT=2

The false green reproduced and caught. script/lint restored afterwards and confirmed back to blob 15de890.

make bootstrap end to end in the fresh clone: exit 0, fetch-assets: installed static/js/alpine.min.js (3ed1eed2...), all assets in static/vendor.sha256 verified.

Nothing tagged, docker ps -a empty, no prune of any kind run.

Trap 1: a cached build lints nothing

Without the flag, on an unchanged tree:

#9 [lint 1/2] COPY . .
#9 CACHED
#10 [lint 2/2] RUN golangci-lint run --config .golangci.yml ./...
#10 CACHED

real    0m0.273s
exit=0

0.27s, exit 0, no linter. That is the false green. --no-cache-filter=lint forces the lint stage to re-execute while deps keeps its cache.

--output=type=cacheonly means no image is produced, so repeated local linting leaves nothing to clean up on a shared host.

Trap 1a: --no-cache-filter silently ignores an unmatched stage name

Raised in review. Docker does not validate the argument, so the whole anti-false-green guarantee rested on the literal string lint matching the stage name, with a one-character typo or a stage rename silently restoring the cached false green — and failing fast and green, the worst shape a failure can take.

script/lint no longer trusts the flag. It tees the build output and fails unless golangci-lint's own summary line (N issues. / N issues:) appears in it. The build's exit status travels via a file, because a pipeline's status is tee's and POSIX sh has no pipefail; output still streams live, so --progress=plain is not defeated. Re-verified at this head, above.

Negative control

Deliberate violation added to internal/globals/globals.go:

var DeliberateLintViolation = "negative control"

make lint:

#11 50.39 internal/globals/globals.go:17:5: exported: exported var DeliberateLintViolation should have comment or be unexported (revive)
#11 50.39 var DeliberateLintViolation = "negative control"
#11 50.39 1 issues:
#11 50.39 * revive: 1
#11 ERROR: process "/bin/sh -c golangci-lint run --config .golangci.yml ./..." did not complete successfully: exit code: 1
MAKE_LINT_EXIT=2

The finding reaches the terminal in full — the tee captures a copy, it does not swallow the stream. Reverted, run again: 0 issues., exit 0.

Note the reported path: internal/globals/globals.go, relative to the repo root inside the container. The container holds only this repo, so the ../other-worktree/... contamination from #106 is structurally impossible here rather than filtered after the fact.

Trap 2: config verify and its live schema fetch

I planned to drop config verify on hash-pinning grounds. Testing reversed that on both halves of the reasoning.

Its value is real. golangci-lint run silently ignores config keys it does not recognize — a bogus top-level key, and a bogus key nested under run:, both give 0 issues. and exit 0. config verify is the only thing that catches them:

jsonschema: "" does not validate with "/additionalProperties": additional properties 'bogus-top-level-key' not allowed
The command is terminated due to an error: the configuration contains invalid elements
rc=3

The cost is not real in this pinned image. The same detection works with the network fully removed (net=none, rc=3 on the bad config, rc=0 on the good one). v2.12.2 resolves the schema from inside the image when the config's version matches the binary, so it is content-addressed and hash-pinning holds.

Decision: keep config verify, and enforce the absence of a fetch rather than trust it. Both lint steps use RUN --network=none, which the built-in BuildKit frontend supports with no # syntax directive (so no extra external reference to pin). If a future image bump makes either step reach for the network, the build fails loudly instead of quietly acquiring an unpinned dependency. The reasoning is recorded in the Dockerfile.lint header so the step is not "helpfully" removed or the flag dropped.

This is a deliberate divergence from sneak/homoicon, which runs config verify without --network=none and builds without --no-cache-filter.

make check with no golangci-lint on PATH

The host has three copies, so an earlier run used a stripped environment that excludes all of them:

$ env -i PATH="/usr/local/go/bin:/usr/bin:/bin" HOME="$HOME" sh -c '...'
golangci-lint: NOT ON PATH

$ env -i PATH="/usr/local/go/bin:/usr/bin:/bin" HOME="$HOME" make check
MAKE_CHECK_EXIT=0
cached test lines: 0

with the lint stage really executing inside it (#11 51.69 0 issues.) and zero (cached) test lines.

Notes

  • This supersedes #106. Its option 2 is what landed here, and structurally rather than by cache isolation: there is no host binary and no shared cache left to contaminate. That issue can be closed as superseded once this merges.
  • #120 (JS linting) is unblocked by this — Dockerfile.lint is the place its linter stage goes. Not attempted here.
  • Surfaced but not acted on, since it is out of scope here: every lint run prints The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2. .golangci.yml is the standardized file that must only be updated from the canonical copy, so this belongs upstream rather than in this PR.
  • REPO_POLICIES.md still describes the lint stage as part of the main Dockerfile only. It is a synced cross-project document, so updating it is the canonical rollout's job, not this repo's.
Closes https://git.eeqj.de/sneak/webhooker/issues/109. golangci-lint no longer runs on the host. `script/lint` builds `Dockerfile.lint`, which copies the repo into the digest-pinned golangci-lint image and lints as a build step, so a successful build is a clean lint. ## Changes - **`Dockerfile.lint`** (new). Two stages off `golangci/golangci-lint:v2.12.2@sha256:5cceeef0...`: a cacheable `deps` stage (`go mod download`) and a `lint` stage that copies the tree and runs the linter. - **`script/lint`** is now a POSIX-sh wrapper around `docker build -f Dockerfile.lint --no-cache-filter=lint --progress=plain --output=type=cacheonly .`, which additionally asserts that the linter's own summary line appears in the build output (see below). - **`Dockerfile`** lint stage invokes `golangci-lint` directly instead of `make lint` — it is already the pinned linter image, and `make lint` would now need a docker daemon inside the build. - **`script/bootstrap`** no longer installs golangci-lint; the version pins, sha256 constants, release-archive download and the now-unused `verify_sha256` helper are gone. It warns if docker is absent. Its `curl` guard and `script/fetch-assets` call are untouched. - **`README.md`**: new Linting section, corrected prerequisites, and the Docker stage list fixed (it described a two-stage build that installed golangci-lint in the builder, which has not been true for a while). `TODO.md` untouched. ## Rebased onto `next` @ `41ff16a` Clean, no conflicts. https://git.eeqj.de/sneak/webhooker/issues/146, https://git.eeqj.de/sneak/webhooker/issues/152 and https://git.eeqj.de/sneak/webhooker/issues/157 landed since the previous head; none overlaps this PR's hunks. Verified rather than assumed: - `Dockerfile`: the only delta from the previous head `cc5836d` is 146's builder `apt-get` line gaining `jq`, taken verbatim. This PR's own `Dockerfile` delta against `next` is still exactly the lint-stage change. - `README.md`: 152 added the `script/ci-mark-superseded` Entrypoints bullet and rewrote the CI-gate-honesty tail; 146 rewrote the access-log region; 157 touched no Markdown at all. All are outside this PR's hunks. `git diff origin/next HEAD -- README.md` still touches only lint-related lines, so no https://git.eeqj.de/sneak/webhooker/issues/151 correction is clobbered. - 157 (landed mid-gate) is confined to `internal/handlers`, `internal/server` and `templates/`, so it cannot interact with this change. Rebased onto it and re-ran the full gate anyway; the numbers below are from the final head. - The rebase falsified nothing. Re-read Prerequisites, Development Commands, Entrypoints, Linting and Docker end to end against the tree: `script/check` really runs test, lint, fmt-check in that order; the Makefile really has 16 targets of which 10 shim `script/`, with `build`, `run`, `dev`, `deps`, `clean`, `css` inline; `script/ci-mark-superseded` correctly has no Makefile target and the README does not claim one. **The four non-`README.md` files are byte-identical to the twice-reviewed head**, modulo `next`'s own `jq` line above. `git rev-parse HEAD:<path>` at this head: `Dockerfile.lint` `9a9af51`, `script/bootstrap` `49001f8`, `script/lint` `15de890` — the same blobs the third review recorded. ### Earlier rebase (README merge against https://git.eeqj.de/sneak/webhooker/issues/151), unchanged `README.md` was the only conflicted file. 151 was a wholesale README accuracy rewrite that landed four minutes after the then-head was pushed; one of its new statements is made false by this PR, so the resolution was neither "take ours" nor "take theirs". - **Prerequisites.** Dropped 151's `golangci-lint v2.12.2 ... make bootstrap installs it` bullet, which this PR falsifies, and folded linting into its Docker bullet: `Docker (for linting, for the test stage of the CI gate, and for containerized deployment)`. 151's `Go 1.26.1+` bullet and its `curl` / `script/fetch-assets` bullet are kept verbatim. This branch's "golangci-lint is not a prerequisite and must not be installed on the host" paragraph is kept, extended to name `script/bootstrap` explicitly. - **Development Commands.** Both edits kept: 151's `make fmt-check` line and this branch's `make lint # Run golangci-lint in Docker (Dockerfile.lint)`. - **Repo tree.** 151's rewritten tree taken wholesale. Only the `Dockerfile.lint` line is re-inserted. - **Docker section.** 151's more detailed three-stage list taken wholesale, with only the lint-stage bullet corrected to what the `Dockerfile` actually runs — `make fmt-check`, then `golangci-lint config verify` and `golangci-lint run`, both with `--network=none`. This branch's paragraph explaining why the stage does not call `make lint` is kept. Statements that auto-merged cleanly out of 151 but were left false by this PR are corrected in the same commit rather than knowingly shipped wrong: the Quick Start comment claiming `make bootstrap` installs "the pinned linter"; the paragraph saying `script/lint` runs "whatever `golangci-lint` is on the host"; and the CI-gate-honesty bullet listing `make lint` among what the image really runs. The `script/bootstrap` conflict from the first rebase is folded in and unchanged: https://git.eeqj.de/sneak/webhooker/pulls/164's `if missing curl; then pkg_install curl curl curl curl; fi` and `"$ROOT/script/fetch-assets"` both survive; `verify_sha256`, `install_golangci_lint_release` and `ensure_golangci_lint` are gone, with `detect_pkgmgr` still reached via `pkg_install`. ## README corrections in this round Two statements the previous round left standing, both raised in https://git.eeqj.de/sneak/webhooker/pulls/165#issuecomment-62702. No code changed. **1. The CI-gate paragraph counted four check targets.** Verified against the `Dockerfile`: the image runs `make fmt-check`, `golangci-lint config verify`, `golangci-lint run`, `make test`, `make build` — three `make` targets plus a direct binary invocation, not four targets. The count is dropped: &gt; `script/cibuild` — `docker build .` — is the CI gate: the checks run inside the image, so a build that succeeds is a repo that is formatted, linted, tested and compiled. **2. The Linting section undercounted its own guarantees.** "Two properties are load-bearing" is now "Three", with the summary-line assertion added as a bullet next to the flag it guards: &gt; - `script/lint` does not trust that flag. Docker silently ignores `--no-cache-filter` for a stage name that does not match, so a stage rename or a one-character typo would restore the cached false green with no warning and a fast exit 0. The script therefore tees the build output and treats a run as a pass only if golangci-lint's own summary line (`N issues.` / `N issues:`) appears in it: no summary, no lint, whatever the exit code says. ## Gate at `1076edb` `make check`: ``` MAKE_CHECK_EXIT=0 real 0m58.290s #11 49.14 0 issues. ok sneak.berlin/go/webhooker/internal/handlers 3.580s ok sneak.berlin/go/webhooker/internal/server 1.972s ok sneak.berlin/go/webhooker/cmd/webhooker 1.058s ``` Disclosure: this run had **10 `(cached)` test lines**. It is the second `make check` in this clone — the first, before the rebase onto 157, ran all 14 packages for real with zero cached lines (exit 0, 1m9.456s, `#11 47.01 0 issues.`), and the rebase only invalidated the packages 157 touched, which are exactly the three that re-executed. The image gate below re-runs everything with an empty cache. Full image gate, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .`: ``` DOCKER_BUILD_EXIT=0 real 3m5.210s cached test lines: 0 #20 [lint 7/9] RUN make fmt-check #21 [lint 8/9] RUN --network=none golangci-lint config verify --config .golangci.yml #22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #22 48.50 0 issues. #30 [builder 9/11] RUN make test #30 49.45 ok sneak.berlin/go/webhooker/cmd/webhooker 1.083s #30 55.48 ok sneak.berlin/go/webhooker/internal/ciscript 7.111s #30 55.48 ok sneak.berlin/go/webhooker/internal/handlers 4.382s #30 55.48 ok sneak.berlin/go/webhooker/internal/server 2.419s #30 55.48 ok sneak.berlin/go/webhooker/static 1.018s #31 [builder 10/11] RUN make build #32 [builder 11/11] RUN CGO_ENABLED=1 go build ... #35 [stage-2 7/7] RUN chown -R webhooker:webhooker /app /var/lib/webhooker ``` All 13 test packages ran with real durations; the graph executed through the final stage. Two consecutive `make lint` runs on an unchanged tree, `git status --porcelain` identical between them, both really executing: ``` #11 49.57 0 issues. real 0m54.957s LINT1_EXIT=0 #11 47.14 0 issues. real 0m47.915s LINT2_EXIT=0 ``` Misspelled stage (`lnit`), the two-run form — run 1 executes for real because editing `script/lint` invalidates `COPY . .`, and only run 2 can hit the cache: ``` run 1: #11 48.18 0 issues. real 0m49.145s MISSPELL_RUN1_EXIT=0 run 2: #9 CACHED / #10 CACHED / #11 CACHED script/lint: golangci-lint printed no summary line; the linter did not run. Check that the stage named in --no-cache-filter still matches a stage in Dockerfile.lint. make: *** [Makefile:19: lint] Error 1 real 0m0.232s MISSPELL_RUN2_EXIT=2 ``` The false green reproduced and caught. `script/lint` restored afterwards and confirmed back to blob `15de890`. `make bootstrap` end to end in the fresh clone: exit 0, `fetch-assets: installed static/js/alpine.min.js (3ed1eed2...)`, `all assets in static/vendor.sha256 verified`. Nothing tagged, `docker ps -a` empty, no prune of any kind run. ## Trap 1: a cached build lints nothing Without the flag, on an unchanged tree: ``` #9 [lint 1/2] COPY . . #9 CACHED #10 [lint 2/2] RUN golangci-lint run --config .golangci.yml ./... #10 CACHED real 0m0.273s exit=0 ``` 0.27s, exit 0, no linter. That is the false green. `--no-cache-filter=lint` forces the lint stage to re-execute while `deps` keeps its cache. `--output=type=cacheonly` means no image is produced, so repeated local linting leaves nothing to clean up on a shared host. ## Trap 1a: `--no-cache-filter` silently ignores an unmatched stage name Raised in review. Docker does not validate the argument, so the whole anti-false-green guarantee rested on the literal string `lint` matching the stage name, with a one-character typo or a stage rename silently restoring the cached false green — and failing *fast* and *green*, the worst shape a failure can take. `script/lint` no longer trusts the flag. It tees the build output and fails unless golangci-lint's own summary line (`N issues.` / `N issues:`) appears in it. The build's exit status travels via a file, because a pipeline's status is `tee`'s and POSIX sh has no `pipefail`; output still streams live, so `--progress=plain` is not defeated. Re-verified at this head, above. ## Negative control Deliberate violation added to `internal/globals/globals.go`: ```go var DeliberateLintViolation = "negative control" ``` `make lint`: ``` #11 50.39 internal/globals/globals.go:17:5: exported: exported var DeliberateLintViolation should have comment or be unexported (revive) #11 50.39 var DeliberateLintViolation = "negative control" #11 50.39 1 issues: #11 50.39 * revive: 1 #11 ERROR: process "/bin/sh -c golangci-lint run --config .golangci.yml ./..." did not complete successfully: exit code: 1 MAKE_LINT_EXIT=2 ``` The finding reaches the terminal in full — the tee captures a copy, it does not swallow the stream. Reverted, run again: `0 issues.`, exit 0. Note the reported path: `internal/globals/globals.go`, relative to the repo root inside the container. The container holds only this repo, so the `../other-worktree/...` contamination from https://git.eeqj.de/sneak/webhooker/issues/106 is structurally impossible here rather than filtered after the fact. ## Trap 2: `config verify` and its live schema fetch I planned to drop `config verify` on hash-pinning grounds. Testing reversed that on both halves of the reasoning. **Its value is real.** `golangci-lint run` silently ignores config keys it does not recognize — a bogus top-level key, and a bogus key nested under `run:`, both give `0 issues.` and exit 0. `config verify` is the only thing that catches them: ``` jsonschema: "" does not validate with "/additionalProperties": additional properties 'bogus-top-level-key' not allowed The command is terminated due to an error: the configuration contains invalid elements rc=3 ``` **The cost is not real in this pinned image.** The same detection works with the network fully removed (`net=none`, `rc=3` on the bad config, `rc=0` on the good one). v2.12.2 resolves the schema from inside the image when the config's `version` matches the binary, so it is content-addressed and hash-pinning holds. **Decision: keep `config verify`, and enforce the absence of a fetch rather than trust it.** Both lint steps use `RUN --network=none`, which the built-in BuildKit frontend supports with no `# syntax` directive (so no extra external reference to pin). If a future image bump makes either step reach for the network, the build fails loudly instead of quietly acquiring an unpinned dependency. The reasoning is recorded in the `Dockerfile.lint` header so the step is not "helpfully" removed or the flag dropped. This is a deliberate divergence from `sneak/homoicon`, which runs `config verify` without `--network=none` and builds without `--no-cache-filter`. ## `make check` with no golangci-lint on `PATH` The host has three copies, so an earlier run used a stripped environment that excludes all of them: ``` $ env -i PATH="/usr/local/go/bin:/usr/bin:/bin" HOME="$HOME" sh -c '...' golangci-lint: NOT ON PATH $ env -i PATH="/usr/local/go/bin:/usr/bin:/bin" HOME="$HOME" make check MAKE_CHECK_EXIT=0 cached test lines: 0 ``` with the lint stage really executing inside it (`#11 51.69 0 issues.`) and zero `(cached)` test lines. ## Notes - This supersedes https://git.eeqj.de/sneak/webhooker/issues/106. Its option 2 is what landed here, and structurally rather than by cache isolation: there is no host binary and no shared cache left to contaminate. That issue can be closed as superseded once this merges. - https://git.eeqj.de/sneak/webhooker/issues/120 (JS linting) is unblocked by this — `Dockerfile.lint` is the place its linter stage goes. Not attempted here. - Surfaced but not acted on, since it is out of scope here: every lint run prints `The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2`. `.golangci.yml` is the standardized file that must only be updated from the canonical copy, so this belongs upstream rather than in this PR. - `REPO_POLICIES.md` still describes the lint stage as part of the main `Dockerfile` only. It is a synced cross-project document, so updating it is the canonical rollout's job, not this repo's.
clawbot added the needs-review label 2026-08-17 23:07:21 +02:00
clawbot added 1 commit 2026-08-17 23:07:21 +02:00
Run all linting in Docker via Dockerfile.lint (closes #109)
All checks were successful
check / check (push) Successful in 3m9s
46248a2838
golangci-lint no longer runs on the host. script/lint builds
Dockerfile.lint, which copies the repo into the digest-pinned
golangci-lint image and lints as a build step, so a successful build is
a clean lint. The host binary shared one cache and one lock with every
other checkout on the machine, which produced findings attributed to
unrelated worktrees as well as unearned passes.

Two properties the wrapper has to get right:

- --no-cache-filter=lint forces the lint stage to re-execute. Without
  it an unchanged tree replays the layer and the build exits 0 in under
  a second having linted nothing. The deps stage stays cacheable.
- Both lint steps use RUN --network=none. golangci-lint config verify
  is documented as fetching its JSON schema over HTTPS; the pinned
  image resolves it with no network, and --network=none enforces that
  rather than trusting it. Verify is kept because golangci-lint run
  silently ignores config keys it does not recognize.

The main Dockerfile's lint stage now invokes golangci-lint directly
instead of `make lint`, which would otherwise need a docker daemon
inside the build.

golangci-lint installation is removed from script/bootstrap.
clawbot self-assigned this 2026-08-17 23:07:29 +02:00
Author
Collaborator

FAIL — needs-rebase

Content review passes: the central claim is real and I reproduced every load-bearing item independently in my own clone at 46248a2. The only blocker is that the branch no longer merges into next.

Blocker

1. Conflicts with next @ c378690 (#164 squash-merged since this branch was cut). git merge-tree reports one conflict, script/bootstrap; Dockerfile and README.md auto-merge. Resolution is mechanical but has one trap:

  • Header comment: 164 appended a sentence about script/fetch-assets; this PR rewrote the adjacent golangci-lint sentence. Keep both edits.
  • main(): 164 added if missing curl; then pkg_install curl curl curl curl; fi + "$ROOT/script/fetch-assets" immediately after go mod download; this PR deleted the adjacent ensure_golangci_lint call. Keep 164's two lines and this PR's deletion.
  • Trap: the only other pkg_install curl in the pre-164 file lived inside install_golangci_lint_release, which this PR deletes. 164's own curl guard must survive the merge or script/fetch-assets loses its fetcher on a bare host.
  • This branch predates script/fetch-assets entirely (zero references in its base and its head), so it has not removed or broken the call — the merge must re-introduce it.

Should fix during the rework

2. --no-cache-filter silently ignores a stage name that does not match. The whole anti-false-green guarantee rests on the literal string lint in script/lint matching the stage name in Dockerfile.lint, and Docker does not validate it. Verified on this host:

docker build -f Dockerfile.lint --no-cache-filter=lnit --progress=plain --output=type=cacheonly .
#11 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./...
#11 CACHED
rc=0   (0.2s, no warning about the unknown stage)

A stage rename or a one-character typo therefore restores exactly the defect this PR exists to eliminate, with no error and a fast green. Dockerfile.lint's "Do not drop that flag" comment does not cover this case. Acceptable fix: make script/lint assert on evidence instead of trusting the flag — capture the build output (--progress=plain writes to stderr) and exit non-zero unless the linter's own summary line (issues.) appears in it. Three lines of POSIX sh, and it converts the guarantee from a convention into a check.

Note, not a defect

3. REPO_POLICIES.md still states the Dockerfile lint stage "runs make fmt-check and make lint". The main Dockerfile here now calls golangci-lint directly, which is correct (calling make lint would need a docker daemon inside the build), and the PR body discloses it. It does leave this repo textually out of step with the synced policy doc until the canonical rollout lands — track under sneak/prompts#40.

Gate evidence (my clone, PR head 46248a2)

  • False green reproduced, both halves: without --no-cache-filter, real 0m0.262s, exit 0, lint layers CACHED. With it, two consecutive runs on an unchanged tree: 62.7s and 52.7s, 0 issues. printed both times, deps/go mod download staying CACHED.
  • Negative control, extended to a second linter class: a scratch file with an uncommented exported var and an unchecked os.Setenv produced errcheck and revive findings, 3 issues:, exit 1, paths relative to the repo root inside the container. Reverted, clean. --output=type=cacheonly does not mask the failure.
  • Claim that golangci-lint run silently ignores unknown config keys: confirmed. Bogus top-level key and bogus key nested under run: both give 0 issues. / exit 0, while config verify rejects both by jsonschema. Keeping verify is justified.
  • RUN --network=none works on the built-in BuildKit frontend with no # syntax directive (docker 29.7.2 / buildx 0.36.1), so no new unpinned reference. Enforcement proven: same image, getent hosts deb.debian.org resolves under the default network and fails under --network=none.
  • Digest sha256:5cceeef0... resolves to golangci-lint has version 2.12.2. No other external reference introduced.
  • make check exit 0 with golangci-lint: NOT ON PATH under env -i PATH="/usr/local/go/bin:/usr/bin:/bin"; zero (cached) test lines; lint executed in-container.
  • docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0 in 2m57s, zero (cached) test lines, lint stage genuinely re-executed.
  • make bootstrap exit 0; no dead code left behind (verify_sha256, install_golangci_lint_release, ensure_golangci_lint all removed, detect_pkgmgr still reached via pkg_install).
  • CI green on 46248a2 (check / check, 3m9s). One commit, title ends (closes #109), base next, TODO.md untouched, no Claude/Anthropic references or attribution trailers, script/lint is POSIX sh with set -eu and the canonical root idiom, make fmt-check clean, inclusive terminology clean, #120 correctly not attempted.

Interaction check for the rebase worker, so nobody "helpfully" tightens it: 164's RUN script/fetch-assets sits in the builder stage, after COPY . . and before make test, with no network restriction. It is not in any stage this PR marks --network=none, and it must not be — that step needs the network. Dockerfile.lint needs no change for 164: static/static.go embeds directories (//go:embed css js), not the individual file, so the package still type-checks with static/js/alpine.min.js absent from the context, and static/vendor_test.go only needs the asset at run time, in the builder stage.

Containers and images from this review: none left behind.

FAIL — needs-rebase Content review passes: the central claim is real and I reproduced every load-bearing item independently in my own clone at `46248a2`. The only blocker is that the branch no longer merges into `next`. ## Blocker **1. Conflicts with `next` @ `c378690`** (https://git.eeqj.de/sneak/webhooker/pulls/164 squash-merged since this branch was cut). `git merge-tree` reports one conflict, `script/bootstrap`; `Dockerfile` and `README.md` auto-merge. Resolution is mechanical but has one trap: - Header comment: 164 appended a sentence about `script/fetch-assets`; this PR rewrote the adjacent golangci-lint sentence. Keep both edits. - `main()`: 164 added `if missing curl; then pkg_install curl curl curl curl; fi` + `"$ROOT/script/fetch-assets"` immediately after `go mod download`; this PR deleted the adjacent `ensure_golangci_lint` call. Keep 164's two lines and this PR's deletion. - Trap: the only other `pkg_install curl` in the pre-164 file lived inside `install_golangci_lint_release`, which this PR deletes. 164's own curl guard must survive the merge or `script/fetch-assets` loses its fetcher on a bare host. - This branch predates `script/fetch-assets` entirely (zero references in its base and its head), so it has not removed or broken the call — the merge must re-introduce it. ## Should fix during the rework **2. `--no-cache-filter` silently ignores a stage name that does not match.** The whole anti-false-green guarantee rests on the literal string `lint` in `script/lint` matching the stage name in `Dockerfile.lint`, and Docker does not validate it. Verified on this host: ``` docker build -f Dockerfile.lint --no-cache-filter=lnit --progress=plain --output=type=cacheonly . #11 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./... #11 CACHED rc=0 (0.2s, no warning about the unknown stage) ``` A stage rename or a one-character typo therefore restores exactly the defect this PR exists to eliminate, with no error and a fast green. `Dockerfile.lint`'s "Do not drop that flag" comment does not cover this case. Acceptable fix: make `script/lint` assert on evidence instead of trusting the flag — capture the build output (`--progress=plain` writes to stderr) and exit non-zero unless the linter's own summary line (`issues.`) appears in it. Three lines of POSIX sh, and it converts the guarantee from a convention into a check. ## Note, not a defect **3.** `REPO_POLICIES.md` still states the Dockerfile lint stage "runs `make fmt-check` and `make lint`". The main `Dockerfile` here now calls `golangci-lint` directly, which is correct (calling `make lint` would need a docker daemon inside the build), and the PR body discloses it. It does leave this repo textually out of step with the synced policy doc until the canonical rollout lands — track under https://git.eeqj.de/sneak/prompts/issues/40. ## Gate evidence (my clone, PR head `46248a2`) - False green reproduced, both halves: without `--no-cache-filter`, `real 0m0.262s`, exit 0, lint layers `CACHED`. With it, two consecutive runs on an unchanged tree: 62.7s and 52.7s, `0 issues.` printed both times, `deps`/`go mod download` staying `CACHED`. - Negative control, extended to a second linter class: a scratch file with an uncommented exported var and an unchecked `os.Setenv` produced `errcheck` and `revive` findings, `3 issues:`, exit 1, paths relative to the repo root inside the container. Reverted, clean. `--output=type=cacheonly` does not mask the failure. - Claim that `golangci-lint run` silently ignores unknown config keys: confirmed. Bogus top-level key and bogus key nested under `run:` both give `0 issues.` / exit 0, while `config verify` rejects both by jsonschema. Keeping `verify` is justified. - `RUN --network=none` works on the built-in BuildKit frontend with no `# syntax` directive (docker 29.7.2 / buildx 0.36.1), so no new unpinned reference. Enforcement proven: same image, `getent hosts deb.debian.org` resolves under the default network and fails under `--network=none`. - Digest `sha256:5cceeef0...` resolves to `golangci-lint has version 2.12.2`. No other external reference introduced. - `make check` exit 0 with `golangci-lint: NOT ON PATH` under `env -i PATH="/usr/local/go/bin:/usr/bin:/bin"`; zero `(cached)` test lines; lint executed in-container. - `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0 in 2m57s, zero `(cached)` test lines, lint stage genuinely re-executed. - `make bootstrap` exit 0; no dead code left behind (`verify_sha256`, `install_golangci_lint_release`, `ensure_golangci_lint` all removed, `detect_pkgmgr` still reached via `pkg_install`). - CI green on `46248a2` (`check / check`, 3m9s). One commit, title ends ` (closes #109)`, base `next`, `TODO.md` untouched, no Claude/Anthropic references or attribution trailers, `script/lint` is POSIX sh with `set -eu` and the canonical root idiom, `make fmt-check` clean, inclusive terminology clean, https://git.eeqj.de/sneak/webhooker/issues/120 correctly not attempted. Interaction check for the rebase worker, so nobody "helpfully" tightens it: 164's `RUN script/fetch-assets` sits in the **builder** stage, after `COPY . .` and before `make test`, with no network restriction. It is not in any stage this PR marks `--network=none`, and it must not be — that step needs the network. `Dockerfile.lint` needs no change for 164: `static/static.go` embeds directories (`//go:embed css js`), not the individual file, so the package still type-checks with `static/js/alpine.min.js` absent from the context, and `static/vendor_test.go` only needs the asset at run time, in the builder stage. Containers and images from this review: none left behind.
clawbot added needs-rebase and removed needs-review labels 2026-08-17 23:24:30 +02:00
clawbot force-pushed issue-109-docker-lint from 46248a2838 to 93be4d2846 2026-08-17 23:40:18 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-08-17 23:43:33 +02:00
Author
Collaborator

FAIL — needs-rebase

Scoped confirmation pass on the rework at 93be4d2, not a full re-review. Both reworked items pass in my own fresh clone. I relied on the prior review (#165 (comment)) for the digest resolving to v2.12.2, the --network=none enforcement proof, make check with no host golangci-lint on PATH, and the config verify reversal (that golangci-lint run silently ignores unrecognized config keys), and did not redo those.

Blocker

Conflicts with current next @ 39064a3. #151 landed on next at 23:44:59, four minutes after this head was pushed. git merge-tree HEAD origin/next reports a conflict in README.md — four hunks, and this one is semantic rather than mechanical:

  • Prerequisites (README line ~14): next now says "golangci-lint v2.12.2 (the version pinned in script/bootstrap and in the Dockerfile's lint stage; make bootstrap installs it)". That statement is made false by this PR and must be replaced by this branch's version, while next's two other new bullets (Go 1.26.1+ wording, the curl / script/fetch-assets bullet) must be kept.
  • Makefile target list (~line 62): keep next's new make fmt-check line and this branch's make lint # Run golangci-lint in Docker (Dockerfile.lint).
  • Repo tree (~line 1182): take next's rewritten tree wholesale and re-insert only the Dockerfile.lint line. Note next already renamed the Dockerfile comment to "Three stages: lint, test+build, Alpine runtime".
  • Fourth hunk at ~line 1363 (the Docker section) — same shape: next rewrote it, this branch rewrote it, both edits are wanted.

Nothing else conflicts. No content change is required.

Item 1 — script/bootstrap conflict resolution: correct

if missing curl; then pkg_install curl curl curl curl; fi and "$ROOT/script/fetch-assets" both survive at lines 74-75. make bootstrap in a bare clone: exit 0, Alpine.js fetched and verified against static/vendor.sha256. verify_sha256, install_golangci_lint_release and ensure_golangci_lint are all gone; detect_pkgmgr is still reached via pkg_install. dash -n clean on both script/bootstrap and script/lint.

Item 2 — script/lint summary assertion: correct, and I could not fool it

  • Misspelled stage (lnit): FAILS loudly both runs — 0.31s / 0.24s, make exit 2, full diagnostic. Note it failed on the first run too, not only the second: the author's "first run is a cache miss" trap did not reproduce for me because BuildKit's cache is content-addressed and host-wide, so the author's own earlier lnit edit had already primed an identical-content context entry. I separately confirmed COPY . . is genuinely content-sensitive (appending a comment to script/lint invalidated it and the linter ran, 57s).
  • Pass case: two consecutive runs on an unchanged tree, 69.2s and 65.0s, 0 issues. both times, exit 0.
  • Fail case: scratch file with an uncommented exported var and an unchecked os.Setenv4 issues: across errcheck/revive/gochecknoglobals, exit 2, every finding line reaching the terminal in full. The tee copies, it does not swallow.
  • False-PASS attempt: the summary check is reached only after [ "$rc" -eq 0 ], so a findings-bearing run (which does echo source text) can never satisfy the grep. Dockerfile.lint contains exactly one linter invocation, config verify is silent on success, and BuildKit prints only CACHED — no other line in the build output can match [0-9]+ issues[.:]. Could not construct one.
  • False-FAILURE attempt: forced a non-default output format (output.formats.json) into .golangci.yml, the one realistic way the summary could vanish under a canonical-config sync. golangci-lint v2.12.2 still prints 0 issues. to stderr alongside the JSON, and the guard passed. Reverted, config byte-identical.
  • Status-via-file is sound under set -eu: the && echo 0 || echo $? list suppresses set -e, $? in the || arm is the failed docker build's, and the compound always succeeds so tee always runs. Worst case (empty rcfile from a kill) still exits non-zero.

Untouched as required

Dockerfile.lint and both RUN --network=none steps unchanged; RUN script/fetch-assets still in the builder stage with no network restriction (it fetched over the network during the gate); digest sha256:5cceeef0... intact; #120 not attempted.

Gate (my clone, 93be4d2)

  • make check exit 0 in 1m15s, zero (cached) test lines, real per-package durations, lint executed in-container (#11 59.45 0 issues.).
  • docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly . exit 0 in 3m3s; zero (cached) test lines; whole graph executed through stage-2 7/7. --output=type=cacheonly does not weaken this — BuildKit still runs every step, it only skips the image export, and I verified the failure path is still reported under it (the negative control failed the build with cacheonly in effect).
  • CI green on 93be4d2 (check / check, 2m43s).
  • One commit; title ends (closes #109); base next; TODO.md untouched; author clawbot; no Claude/Anthropic references or attribution trailers; make fmt-check clean; inclusive terminology clean.

Notes, not defects

  • Commit committer is sneak@sneak.berlin while the author is clawbot@eeqj.de, from the shared host git config. Squash-merge rewrites it anyway.
  • mktemp -t is not POSIX and behaves differently on BSD/macOS (template treated as a prefix), but it creates a usable file on both. --output=type=cacheonly and --no-cache-filter both require BuildKit; on a pre-BuildKit docker they error loudly rather than silently pass.
  • #106 is superseded for this repo by this change, but its definition of done is about the canonical shared script/lint propagated everywhere. Close it as superseded here and let sneak/prompts#40 carry the cross-repo half.

No containers or images left behind; my clone is byte-identical to 93be4d2. No prune of any kind was run.

FAIL — needs-rebase Scoped confirmation pass on the rework at `93be4d2`, not a full re-review. Both reworked items pass in my own fresh clone. I relied on the prior review (https://git.eeqj.de/sneak/webhooker/pulls/165#issuecomment-62467) for the digest resolving to v2.12.2, the `--network=none` enforcement proof, `make check` with no host golangci-lint on `PATH`, and the `config verify` reversal (that `golangci-lint run` silently ignores unrecognized config keys), and did not redo those. ## Blocker **Conflicts with current `next` @ `39064a3`.** https://git.eeqj.de/sneak/webhooker/issues/151 landed on `next` at 23:44:59, four minutes after this head was pushed. `git merge-tree HEAD origin/next` reports a conflict in `README.md` — four hunks, and this one is semantic rather than mechanical: - Prerequisites (README line ~14): `next` now says "golangci-lint v2.12.2 (the version pinned in `script/bootstrap` and in the `Dockerfile`'s lint stage; `make bootstrap` installs it)". That statement is made false by this PR and must be replaced by this branch's version, while `next`'s two other new bullets (Go 1.26.1+ wording, the `curl` / `script/fetch-assets` bullet) must be kept. - Makefile target list (~line 62): keep `next`'s new `make fmt-check` line and this branch's `make lint # Run golangci-lint in Docker (Dockerfile.lint)`. - Repo tree (~line 1182): take `next`'s rewritten tree wholesale and re-insert only the `Dockerfile.lint` line. Note `next` already renamed the `Dockerfile` comment to "Three stages: lint, test+build, Alpine runtime". - Fourth hunk at ~line 1363 (the Docker section) — same shape: `next` rewrote it, this branch rewrote it, both edits are wanted. Nothing else conflicts. No content change is required. ## Item 1 — `script/bootstrap` conflict resolution: correct `if missing curl; then pkg_install curl curl curl curl; fi` and `"$ROOT/script/fetch-assets"` both survive at lines 74-75. `make bootstrap` in a bare clone: exit 0, Alpine.js fetched and verified against `static/vendor.sha256`. `verify_sha256`, `install_golangci_lint_release` and `ensure_golangci_lint` are all gone; `detect_pkgmgr` is still reached via `pkg_install`. `dash -n` clean on both `script/bootstrap` and `script/lint`. ## Item 2 — `script/lint` summary assertion: correct, and I could not fool it - Misspelled stage (`lnit`): FAILS loudly both runs — 0.31s / 0.24s, `make` exit 2, full diagnostic. Note it failed on the *first* run too, not only the second: the author's "first run is a cache miss" trap did not reproduce for me because BuildKit's cache is content-addressed and host-wide, so the author's own earlier `lnit` edit had already primed an identical-content context entry. I separately confirmed `COPY . .` is genuinely content-sensitive (appending a comment to `script/lint` invalidated it and the linter ran, 57s). - Pass case: two consecutive runs on an unchanged tree, 69.2s and 65.0s, `0 issues.` both times, exit 0. - Fail case: scratch file with an uncommented exported var and an unchecked `os.Setenv` — `4 issues:` across `errcheck`/`revive`/`gochecknoglobals`, exit 2, every finding line reaching the terminal in full. The tee copies, it does not swallow. - False-PASS attempt: the summary check is reached only after `[ "$rc" -eq 0 ]`, so a findings-bearing run (which does echo source text) can never satisfy the grep. `Dockerfile.lint` contains exactly one linter invocation, `config verify` is silent on success, and BuildKit prints only `CACHED` — no other line in the build output can match `[0-9]+ issues[.:]`. Could not construct one. - False-FAILURE attempt: forced a non-default output format (`output.formats.json`) into `.golangci.yml`, the one realistic way the summary could vanish under a canonical-config sync. golangci-lint v2.12.2 still prints `0 issues.` to stderr alongside the JSON, and the guard passed. Reverted, config byte-identical. - Status-via-file is sound under `set -eu`: the `&& echo 0 || echo $?` list suppresses `set -e`, `$?` in the `||` arm is the failed `docker build`'s, and the compound always succeeds so `tee` always runs. Worst case (empty rcfile from a kill) still exits non-zero. ## Untouched as required `Dockerfile.lint` and both `RUN --network=none` steps unchanged; `RUN script/fetch-assets` still in the **builder** stage with no network restriction (it fetched over the network during the gate); digest `sha256:5cceeef0...` intact; https://git.eeqj.de/sneak/webhooker/issues/120 not attempted. ## Gate (my clone, `93be4d2`) - `make check` exit 0 in 1m15s, zero `(cached)` test lines, real per-package durations, lint executed in-container (`#11 59.45 0 issues.`). - `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .` exit 0 in 3m3s; zero `(cached)` test lines; whole graph executed through `stage-2 7/7`. `--output=type=cacheonly` does not weaken this — BuildKit still runs every step, it only skips the image export, and I verified the failure path is still reported under it (the negative control failed the build with `cacheonly` in effect). - CI green on `93be4d2` (`check / check`, 2m43s). - One commit; title ends ` (closes #109)`; base `next`; `TODO.md` untouched; author `clawbot`; no Claude/Anthropic references or attribution trailers; `make fmt-check` clean; inclusive terminology clean. ## Notes, not defects - Commit *committer* is `sneak@sneak.berlin` while the author is `clawbot@eeqj.de`, from the shared host git config. Squash-merge rewrites it anyway. - `mktemp -t` is not POSIX and behaves differently on BSD/macOS (template treated as a prefix), but it creates a usable file on both. `--output=type=cacheonly` and `--no-cache-filter` both require BuildKit; on a pre-BuildKit docker they error loudly rather than silently pass. - https://git.eeqj.de/sneak/webhooker/issues/106 is superseded *for this repo* by this change, but its definition of done is about the canonical shared `script/lint` propagated everywhere. Close it as superseded here and let https://git.eeqj.de/sneak/prompts/issues/40 carry the cross-repo half. No containers or images left behind; my clone is byte-identical to `93be4d2`. No prune of any kind was run.
clawbot added needs-rebase and removed needs-review labels 2026-08-18 00:00:13 +02:00
clawbot force-pushed issue-109-docker-lint from 93be4d2846 to c568441dc6 2026-08-18 00:11:57 +02:00 Compare
clawbot force-pushed issue-109-docker-lint from c568441dc6 to cc5836d1f9 2026-08-18 00:20:08 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-08-18 00:21:57 +02:00
Author
Collaborator

FAIL — needs-rework

Scoped confirmation pass on the README.md merge at cc5836d, not a full re-review. I relied on #165 (comment) and #165 (comment) for the digest resolving to v2.12.2, the --network=none enforcement proof, the config verify reversal, the script/bootstrap conflict resolution, and make check with no host golangci-lint on PATH, and did not redo those.

The four non-README.md files are unchanged from the twice-reviewed head. 93be4d2 was force-pushed out of ref advertisement so I could not fetch it into my clone; I compared git blob SHAs and file modes served by the Gitea API at 93be4d2 against git rev-parse HEAD:&lt;path&gt; in my clone — identical for all four, content and mode:

7e10b0d05bdd35ea331101ddfbd0bc3e917a7522 100644 Dockerfile
9a9af5124653ab851830ad15b1787fc6be8f9fbb 100644 Dockerfile.lint
49001f81b488bb8ca87454284c3958fc1b32743a 100755 script/bootstrap
15de89001768de17c19e748a2c240809d39b566d 100755 script/lint

A blob SHA is the content hash, so this is byte-identity. Prior coverage holds; no escalation to full review.

Blocker

README.md lines 1447-1449 — a statement this PR falsified, missed by the merge.

`script/cibuild` — `docker build .` — is the CI gate: the four check
targets run inside the image, so a build that succeeds is a repo that
is formatted, linted, tested and compiled.

Before this PR the image ran exactly four make targets: fmt-check, lint, test, build. This PR's Dockerfile change replaces make lint with a direct golangci-lint config verify + golangci-lint run, so the image now runs three make targets plus a direct binary invocation. There are no longer four check targets.

This is the same falsehood the commit already corrects fifteen lines below, in the CI-gate-honesty bullet — make fmt-check, make lint, make test, make build was changed to make fmt-check, golangci-lint, make test, make build. The prose sentence counts the identical four and was left standing, in a paragraph whose second half this commit rewrote. Given #151's premise that the README contains no false statements, and given the author's own applied standard on the sibling bullet, this one has to go too.

Acceptable: drop the target count, e.g. "the checks run inside the image", or enumerate as the bullet below now does.

Secondary, non-blocking — fix in the same pass

README.md line 1403, Linting section: "Two properties are load-bearing:" then lists --no-cache-filter=lint and RUN --network=none. There is a third, added at the previous review's demand: script/lint does not trust --no-cache-filter and fails unless golangci-lint's own summary line appears in the build output. As written the section presents the flag as the guarantee, when the code deliberately treats it as untrusted — which is the more interesting half. The script/lint header comment covers it; the README does not.

Gate — my clone, cc5836d, all via make/script/

  • make check: exit 0, 63s, zero (cached) test lines, all 12 packages with real durations, lint executed in-container (#11 52.60 0 issues.).
  • docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .: exit 0, 186s, zero (cached) lines, lint stage genuinely re-executed (#19 56.73 0 issues.), both RUN --network=none steps ran, all 12 packages real in [builder 9/11] RUN make test, graph executed through builder 11/11.
  • Two consecutive make lint on an unchanged tree: 53s / 53s, 0 issues. both, exit 0, git status --porcelain empty between them.
  • Misspelled stage (lnit), the two-run form: run 1 executed for real (60s, 0 issues., exit 0 — my edit invalidated COPY . .); run 2 hit the cache, 0s, and the guard fired — every lint layer CACHED, make exit 2, full diagnostic. That is the false green reproduced and caught. Restored; script/lint blob back to 15de890.
  • Disclosure assessed: the author's 7 host (cached) test lines are moot — my make check ran in a fresh clone with an empty Go test cache, zero cached lines, and passed.
  • CI green on cc5836d (check / check, 2m53s).
  • next has advanced two commits to 5888d14 since the PR base. git merge-tree is clean and the new changes (Dockerfile builder apt line, README.md Entrypoints/access-log/CI-honesty regions) do not overlap this PR's hunks. No rebase needed.

Verified clean

No #151 correction clobbered — git diff origin/next HEAD -- README.md touches only lint-related lines, so every other statement 151 landed survives verbatim by construction. ### Shutdown then ### Linting then ### Docker reads coherently; the file has no TOC and the one internal anchor, (#docker) at line 375, still resolves. Docker is genuinely required for make lint and therefore make check locally, and the Prerequisites text says so without claiming Docker is needed for tests. One commit; title ends (closes #109); base next; TODO.md untouched; no Claude/Anthropic references or attribution trailers; #120 not attempted; Makefile shim count (10 of 16) still accurate.

No containers or images left behind; docker ps -a empty. No prune of any kind was run.

FAIL — needs-rework Scoped confirmation pass on the `README.md` merge at `cc5836d`, not a full re-review. I relied on https://git.eeqj.de/sneak/webhooker/pulls/165#issuecomment-62467 and https://git.eeqj.de/sneak/webhooker/pulls/165#issuecomment-62616 for the digest resolving to v2.12.2, the `--network=none` enforcement proof, the `config verify` reversal, the `script/bootstrap` conflict resolution, and `make check` with no host golangci-lint on `PATH`, and did not redo those. **The four non-`README.md` files are unchanged from the twice-reviewed head.** `93be4d2` was force-pushed out of ref advertisement so I could not fetch it into my clone; I compared git blob SHAs and file modes served by the Gitea API at `93be4d2` against `git rev-parse HEAD:&lt;path&gt;` in my clone — identical for all four, content and mode: ``` 7e10b0d05bdd35ea331101ddfbd0bc3e917a7522 100644 Dockerfile 9a9af5124653ab851830ad15b1787fc6be8f9fbb 100644 Dockerfile.lint 49001f81b488bb8ca87454284c3958fc1b32743a 100755 script/bootstrap 15de89001768de17c19e748a2c240809d39b566d 100755 script/lint ``` A blob SHA is the content hash, so this is byte-identity. Prior coverage holds; no escalation to full review. ## Blocker **`README.md` lines 1447-1449 — a statement this PR falsified, missed by the merge.** ``` `script/cibuild` — `docker build .` — is the CI gate: the four check targets run inside the image, so a build that succeeds is a repo that is formatted, linted, tested and compiled. ``` Before this PR the image ran exactly four `make` targets: `fmt-check`, `lint`, `test`, `build`. This PR's `Dockerfile` change replaces `make lint` with a direct `golangci-lint config verify` + `golangci-lint run`, so the image now runs three `make` targets plus a direct binary invocation. There are no longer four check targets. This is the same falsehood the commit already corrects fifteen lines below, in the CI-gate-honesty bullet — `make fmt-check`, `make lint`, `make test`, `make build` was changed to `make fmt-check`, `golangci-lint`, `make test`, `make build`. The prose sentence counts the identical four and was left standing, in a paragraph whose second half this commit rewrote. Given https://git.eeqj.de/sneak/webhooker/issues/151's premise that the README contains no false statements, and given the author's own applied standard on the sibling bullet, this one has to go too. Acceptable: drop the target count, e.g. "the checks run inside the image", or enumerate as the bullet below now does. ## Secondary, non-blocking — fix in the same pass `README.md` line 1403, Linting section: "Two properties are load-bearing:" then lists `--no-cache-filter=lint` and `RUN --network=none`. There is a third, added at the previous review's demand: `script/lint` does not trust `--no-cache-filter` and fails unless golangci-lint's own summary line appears in the build output. As written the section presents the flag as the guarantee, when the code deliberately treats it as untrusted — which is the more interesting half. The `script/lint` header comment covers it; the README does not. ## Gate — my clone, `cc5836d`, all via `make`/`script/` - `make check`: exit 0, 63s, **zero `(cached)` test lines**, all 12 packages with real durations, lint executed in-container (`#11 52.60 0 issues.`). - `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .`: exit 0, 186s, zero `(cached)` lines, lint stage genuinely re-executed (`#19 56.73 0 issues.`), both `RUN --network=none` steps ran, all 12 packages real in `[builder 9/11] RUN make test`, graph executed through `builder 11/11`. - Two consecutive `make lint` on an unchanged tree: 53s / 53s, `0 issues.` both, exit 0, `git status --porcelain` empty between them. - Misspelled stage (`lnit`), the two-run form: run 1 executed for real (60s, `0 issues.`, exit 0 — my edit invalidated `COPY . .`); **run 2 hit the cache, 0s, and the guard fired** — every lint layer `CACHED`, `make` exit 2, full diagnostic. That is the false green reproduced and caught. Restored; `script/lint` blob back to `15de890`. - Disclosure assessed: the author's 7 host `(cached)` test lines are moot — my `make check` ran in a fresh clone with an empty Go test cache, zero cached lines, and passed. - CI green on `cc5836d` (`check / check`, 2m53s). - `next` has advanced two commits to `5888d14` since the PR base. `git merge-tree` is clean and the new changes (`Dockerfile` builder apt line, `README.md` Entrypoints/access-log/CI-honesty regions) do not overlap this PR's hunks. No rebase needed. ## Verified clean No https://git.eeqj.de/sneak/webhooker/issues/151 correction clobbered — `git diff origin/next HEAD -- README.md` touches only lint-related lines, so every other statement 151 landed survives verbatim by construction. `### Shutdown` then `### Linting` then `### Docker` reads coherently; the file has no TOC and the one internal anchor, `(#docker)` at line 375, still resolves. Docker is genuinely required for `make lint` and therefore `make check` locally, and the Prerequisites text says so without claiming Docker is needed for tests. One commit; title ends ` (closes #109)`; base `next`; `TODO.md` untouched; no Claude/Anthropic references or attribution trailers; https://git.eeqj.de/sneak/webhooker/issues/120 not attempted; Makefile shim count (10 of 16) still accurate. No containers or images left behind; `docker ps -a` empty. No prune of any kind was run.
clawbot added needs-rework and removed needs-review labels 2026-08-18 00:35:00 +02:00
clawbot force-pushed issue-109-docker-lint from cc5836d1f9 to 376ec2de92 2026-08-18 00:46:34 +02:00 Compare
clawbot force-pushed issue-109-docker-lint from 376ec2de92 to 1076edb2e8 2026-08-18 00:52:17 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 00:54:20 +02:00
Author
Collaborator

PASS

Round-four scoped confirmation pass on the two README passages only. Behaviour of the change itself is relied on from the three prior reviews (#issuecomment-62467, #issuecomment-62616, #issuecomment-62702): false-green reproduction, digest pin, config verify's value, --network=none, the negative control, the script/bootstrap resolution, and the #151 merge.

Scope confirmed README-only. Fresh clone at 1076edb: git rev-parse HEAD:&lt;path&gt; gives Dockerfile.lint 9a9af51, script/bootstrap 49001f8, script/lint 15de890 — the blobs the third review recorded. git diff origin/next HEAD -- Dockerfile is still only the lint-stage change; the jq line is next's.

Passage 1 (CI gate) is true against the Dockerfile: lint stage runs make fmt-check, golangci-lint config verify, golangci-lint run; builder runs make test and make build, and depends on lint via COPY --from=lint. All four claimed properties (formatted, linted, tested, compiled) hold, and the dropped count is no longer asserted.

Passage 2: exactly three bullets, all accurate. The unmatched-stage claim verified empirically — --no-cache-filter=lnit, run 1 executed for real (54.4s, 0 issues., exit 0, no warning from docker), run 2 replayed COPY . . and both lint steps as CACHED in 0.33s and the summary-line guard fired with exit 2. script/lint restored, blob back to 15de890.

No sixth false statement found. Read end to end against the tree: Prerequisites, Quick Start, Development Commands, Entrypoints (14 bullets vs 14 files in script/; script/ci-mark-superseded bullet present and correctly claims no Makefile target), repo tree, Linting, Docker, CI gate honesty. Checked the countable claims rather than the prose: Makefile has 16 targets, 10 shimming script/ and build/run/dev/deps/clean/css inline; .dockerignore really excludes *.md, LICENSE, .editorconfig and really does not exclude .ci-fingerprint; go.mod is go 1.26.1; both Dockerfile and Dockerfile.lint carry the same sha256:5cceeef0... digest, so "the same pinned linter version the gate does" holds; the #ci-gate-honesty anchor resolves and the "see Linting below" / "see Linting above" cross-references point the right way. No stale claim that make bootstrap installs the linter survives anywhere.

Gate re-run here, not taken on report:

  • make check exit 0 after make bootstrap in the fresh clone; lint stage executed (49.3s, 0 issues.).
  • docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly . exit 0 in 2m43s; 0 (cached) test lines; 13 packages with real durations; graph ran through builder 11/11.
  • Two consecutive make lint on an unchanged tree: 53.0s and 48.9s, both exit 0 (a cached replay is 0.33s), tree clean between them — #109 definition-of-done item 2.
  • make fmt writes nothing; git status clean. CI success on 1076edb. Mergeable, clean against both next and main. One commit, title ends (closes #109), base next, TODO.md untouched. No Claude/Anthropic reference or attribution trailer anywhere in the diff or the tree.

Notes, none blocking:

  • "only script/test and script/fmt-check run on the host" is true of make check's components, which is its subject, but reads as a global claim about script/; script/fmt, script/precommit, script/bootstrap and others also run on the host. Wording only.
  • The Prerequisites bullet says Docker is needed "for the test stage of the CI gate". Docker is needed for the whole gate; understated rather than false.
  • Disclosure: REPO_POLICIES.md line 105 still specifies a lint stage running make fmt-check and make lint. This Dockerfile runs golangci-lint directly, which is necessary (make lint would need a docker daemon inside the build) and is disclosed in the file header and the PR body. Correctly left to the canonical rollout.
  • Out of scope, pre-existing, untouched here: .gitea/workflows/check.yml names its build step "Build Docker image (runs make check)"; the image has never run make check.
  • Disclosure: the misspelled-stage check required a temporary edit to script/lint in my own clone, reverted and verified by blob hash. Nothing committed or pushed. My own second make test showed 13 (cached) lines because it followed make check in the same clone; the authoritative figure is the image gate's 0.

No containers started or left behind, nothing tagged, no prune of any kind.

PASS Round-four scoped confirmation pass on the two README passages only. Behaviour of the change itself is relied on from the three prior reviews (#issuecomment-62467, #issuecomment-62616, #issuecomment-62702): false-green reproduction, digest pin, `config verify`'s value, `--network=none`, the negative control, the `script/bootstrap` resolution, and the https://git.eeqj.de/sneak/webhooker/issues/151 merge. Scope confirmed README-only. Fresh clone at `1076edb`: `git rev-parse HEAD:&lt;path&gt;` gives `Dockerfile.lint` `9a9af51`, `script/bootstrap` `49001f8`, `script/lint` `15de890` — the blobs the third review recorded. `git diff origin/next HEAD -- Dockerfile` is still only the lint-stage change; the `jq` line is `next`'s. Passage 1 (CI gate) is true against the `Dockerfile`: lint stage runs `make fmt-check`, `golangci-lint config verify`, `golangci-lint run`; builder runs `make test` and `make build`, and depends on lint via `COPY --from=lint`. All four claimed properties (formatted, linted, tested, compiled) hold, and the dropped count is no longer asserted. Passage 2: exactly three bullets, all accurate. The unmatched-stage claim verified empirically — `--no-cache-filter=lnit`, run 1 executed for real (54.4s, `0 issues.`, exit 0, no warning from docker), run 2 replayed `COPY . .` and both lint steps as `CACHED` in 0.33s and the summary-line guard fired with exit 2. `script/lint` restored, blob back to `15de890`. No sixth false statement found. Read end to end against the tree: Prerequisites, Quick Start, Development Commands, Entrypoints (14 bullets vs 14 files in `script/`; `script/ci-mark-superseded` bullet present and correctly claims no Makefile target), repo tree, Linting, Docker, CI gate honesty. Checked the countable claims rather than the prose: Makefile has 16 targets, 10 shimming `script/` and `build`/`run`/`dev`/`deps`/`clean`/`css` inline; `.dockerignore` really excludes `*.md`, `LICENSE`, `.editorconfig` and really does not exclude `.ci-fingerprint`; `go.mod` is `go 1.26.1`; both `Dockerfile` and `Dockerfile.lint` carry the same `sha256:5cceeef0...` digest, so "the same pinned linter version the gate does" holds; the `#ci-gate-honesty` anchor resolves and the "see Linting below" / "see Linting above" cross-references point the right way. No stale claim that `make bootstrap` installs the linter survives anywhere. Gate re-run here, not taken on report: - `make check` exit 0 after `make bootstrap` in the fresh clone; lint stage executed (49.3s, `0 issues.`). - `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain --output=type=cacheonly .` exit 0 in 2m43s; 0 `(cached)` test lines; 13 packages with real durations; graph ran through `builder 11/11`. - Two consecutive `make lint` on an unchanged tree: 53.0s and 48.9s, both exit 0 (a cached replay is 0.33s), tree clean between them — https://git.eeqj.de/sneak/webhooker/issues/109 definition-of-done item 2. - `make fmt` writes nothing; `git status` clean. CI `success` on `1076edb`. Mergeable, clean against both `next` and `main`. One commit, title ends ` (closes #109)`, base `next`, `TODO.md` untouched. No Claude/Anthropic reference or attribution trailer anywhere in the diff or the tree. Notes, none blocking: - "only `script/test` and `script/fmt-check` run on the host" is true of `make check`'s components, which is its subject, but reads as a global claim about `script/`; `script/fmt`, `script/precommit`, `script/bootstrap` and others also run on the host. Wording only. - The Prerequisites bullet says Docker is needed "for the test stage of the CI gate". Docker is needed for the whole gate; understated rather than false. - Disclosure: `REPO_POLICIES.md` line 105 still specifies a lint stage running `make fmt-check` and `make lint`. This `Dockerfile` runs `golangci-lint` directly, which is necessary (`make lint` would need a docker daemon inside the build) and is disclosed in the file header and the PR body. Correctly left to the canonical rollout. - Out of scope, pre-existing, untouched here: `.gitea/workflows/check.yml` names its build step "Build Docker image (runs make check)"; the image has never run `make check`. - Disclosure: the misspelled-stage check required a temporary edit to `script/lint` in my own clone, reverted and verified by blob hash. Nothing committed or pushed. My own second `make test` showed 13 `(cached)` lines because it followed `make check` in the same clone; the authoritative figure is the image gate's 0. No containers started or left behind, nothing tagged, no prune of any kind.
clawbot merged commit 992b3c68f5 into next 2026-08-18 01:07:17 +02:00
clawbot deleted branch issue-109-docker-lint 2026-08-18 01:07:17 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#165