script/docker has the same layer-cache hole as script/cibuild, so make docker can hand a developer an unearned green #124

New Issue

clawbot · 2026-08-09T08:07:37+02:00

clawbot commented

2026-08-09 08:07:37 +02:00

Flagged by the implementer of PR #122 while fixing #115, and correctly left out of scope there.

script/docker builds the image with no cache control:

docker build -t "$(script/projectname)" .

The Dockerfile does COPY . . then RUN make check, so on a byte-identical tree the check layer is served from the Docker layer cache and the suite never runs — exactly the defect #115 fixed in script/cibuild.

Why this is worth fixing even though it is not the CI gate

script/cibuild is what CI runs, and #115 has closed that hole. But make docker is what a developer or agent runs locally to convince themselves the image builds and the checks pass. It is arguably the more dangerous of the two:

CI runs on a fresh checkout, where the cache is more often cold. Local builds are almost always warm, so the false-green rate is higher here.
Nobody watches make docker for a suspicious duration the way they now watch script/cibuild.
After #115 lands, the two entrypoints will disagree: script/cibuild genuinely re-runs the suite while script/docker may not. Two commands that look interchangeable and are not is a trap, and the person who falls into it will have no reason to suspect anything.

The severity is lower than #115 only because this is not the gate of record. The mechanism is identical.

Definition of done

script/docker cannot report a successful build without actually executing make check, even on a byte-identical tree with a warm cache.
Reuse the mechanism #115 established rather than inventing a second one. PR #122 adds ARG CHECK_EPOCH immediately above the check step and passes --build-arg CHECK_EPOCH="$(date +%s)". Note the load-bearing detail that PR uncovered: the value must be expanded into the RUN command, not merely declared above it. BuildKit does not treat ARG as a layer and keys each instruction on the command string after expansion, so a bare ARG above an unchanged RUN make check leaves the instruction byte-identical and the layer still comes back CACHED — the bug reintroduced in a form that passes code review.
Dependency layers stay cached — the pinned toolchain installs and go mod download must remain above the invalidation line. Do not reach for a blanket --no-cache.
The image tag still comes from script/projectname, so this script stays byte-identical across repos apart from the cache fix.
script/docker remains POSIX sh (#!/bin/sh, set -eu, no bashisms) and keeps the $(cd "$(dirname "$0")/.." && pwd -P) root-location idiom. Verify with sh -n.
Verify by experiment, with a negative control. Run script/docker twice in a row on an unchanged tree and confirm the second run still executes the suite (not CACHED, not sub-second). Then plant a deliberate always-failing test, run script/docker, and confirm the build fails with that specific predicted message. Delete it, confirm git status is clean and no leftover is committed, and confirm the tree builds green again. Report both timings and the negative control verbatim. A cached layer cannot produce a failure predicted in advance — that is the only conclusive proof, and a code-reading argument is explicitly not sufficient here.
Build stays inside the policy's 5-minute ceiling; report the total.
make check green; TODO.md updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Hard constraints

Every external reference stays pinned by hash. Do not touch the golang or alpine sha256 digests, golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5, or goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0.
Do not weaken the gate. Removing make check from the Dockerfile, narrowing it to lint-only, or adding -short/skip flags would delete the guarantee rather than fix it. TESTING.md forbids -short and skip flags by name.
Do not modify .golangci.yml — sha256 must remain 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
DNS is never mocked in this repository; nothing here touches test behaviour. No test file should change, and no negative-control leftover may be committed.
Lint results on this host may be void (#121): treat a run as void if it says parallel golangci-lint is running or names paths outside your worktree, and retry. Isolating GOLANGCI_LINT_CACHE prevents cross-contamination but does not prevent lock collisions.

Sequencing

Land after PR #122, which introduces the ARG CHECK_EPOCH line this builds on. Starting before it merges means guessing at the mechanism and conflicting on the Dockerfile.

Also note #109 restructures the same Dockerfile for a fail-fast lint stage and an ARG VERSION. Keep this PR minimal; whichever lands later rebases.

Upstream

script/docker is byte-identical across repos, so this belongs in the shared template alongside the script/cibuild fix (prompts #26). Fixing it here does not depend on the upstream change landing, but the two should end up consistent.

Flagged by the implementer of [PR #122](https://git.eeqj.de/sneak/dnswatcher/pulls/122) while fixing #115, and correctly left out of scope there. `script/docker` builds the image with no cache control: ```sh docker build -t "$(script/projectname)" . ``` The Dockerfile does `COPY . .` then `RUN make check`, so on a byte-identical tree the check layer is served from the Docker layer cache and the suite never runs — exactly the defect #115 fixed in `script/cibuild`. ## Why this is worth fixing even though it is not the CI gate `script/cibuild` is what CI runs, and #115 has closed that hole. But `make docker` is what a **developer or agent runs locally** to convince themselves the image builds and the checks pass. It is arguably the more dangerous of the two: - CI runs on a fresh checkout, where the cache is more often cold. Local builds are almost always warm, so the false-green rate is *higher* here. - Nobody watches `make docker` for a suspicious duration the way they now watch `script/cibuild`. - After #115 lands, the two entrypoints will **disagree**: `script/cibuild` genuinely re-runs the suite while `script/docker` may not. Two commands that look interchangeable and are not is a trap, and the person who falls into it will have no reason to suspect anything. The severity is lower than #115 only because this is not the gate of record. The mechanism is identical. ## Definition of done 1. `script/docker` cannot report a successful build without actually executing `make check`, even on a byte-identical tree with a warm cache. 2. **Reuse the mechanism #115 established** rather than inventing a second one. PR #122 adds `ARG CHECK_EPOCH` immediately above the check step and passes `--build-arg CHECK_EPOCH="$(date +%s)"`. Note the load-bearing detail that PR uncovered: **the value must be expanded into the `RUN` command**, not merely declared above it. BuildKit does not treat `ARG` as a layer and keys each instruction on the command string *after* expansion, so a bare `ARG` above an unchanged `RUN make check` leaves the instruction byte-identical and the layer still comes back `CACHED` — the bug reintroduced in a form that passes code review. 3. Dependency layers stay cached — the pinned toolchain installs and `go mod download` must remain above the invalidation line. Do not reach for a blanket `--no-cache`. 4. The image tag still comes from `script/projectname`, so this script stays byte-identical across repos apart from the cache fix. 5. `script/docker` remains POSIX `sh` (`#!/bin/sh`, `set -eu`, no bashisms) and keeps the `$(cd "$(dirname "$0")/.." && pwd -P)` root-location idiom. Verify with `sh -n`. 6. **Verify by experiment, with a negative control.** Run `script/docker` twice in a row on an unchanged tree and confirm the second run still executes the suite (not `CACHED`, not sub-second). Then plant a deliberate always-failing test, run `script/docker`, and confirm the build fails **with that specific predicted message**. Delete it, confirm `git status` is clean and no leftover is committed, and confirm the tree builds green again. Report both timings and the negative control verbatim. A cached layer cannot produce a failure predicted in advance — that is the only conclusive proof, and a code-reading argument is explicitly not sufficient here. 7. Build stays inside the policy's 5-minute ceiling; report the total. 8. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Hard constraints - **Every external reference stays pinned by hash.** Do not touch the `golang` or `alpine` `sha256` digests, golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, or goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0`. - **Do not weaken the gate.** Removing `make check` from the Dockerfile, narrowing it to lint-only, or adding `-short`/skip flags would delete the guarantee rather than fix it. `TESTING.md` forbids `-short` and skip flags by name. - **Do not modify `.golangci.yml`** — sha256 must remain `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. - DNS is never mocked in this repository; nothing here touches test behaviour. No test file should change, and no negative-control leftover may be committed. - **Lint results on this host may be void** (#121): treat a run as void if it says `parallel golangci-lint is running` or names paths outside your worktree, and retry. Isolating `GOLANGCI_LINT_CACHE` prevents cross-contamination but does **not** prevent lock collisions. ## Sequencing **Land after PR #122**, which introduces the `ARG CHECK_EPOCH` line this builds on. Starting before it merges means guessing at the mechanism and conflicting on the `Dockerfile`. Also note **#109** restructures the same `Dockerfile` for a fail-fast `lint` stage and an `ARG VERSION`. Keep this PR minimal; whichever lands later rebases. ## Upstream `script/docker` is byte-identical across repos, so this belongs in the shared template alongside the `script/cibuild` fix (`prompts` #26). Fixing it here does not depend on the upstream change landing, but the two should end up consistent.

clawbot added this to the 1.0 milestone 2026-08-09 08:07:37 +02:00

clawbot referenced this issue

2026-08-09 08:20:16 +02:00

ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115) #122

clawbot referenced this issue

2026-08-09 08:25:30 +02:00

ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115) #122

clawbot referenced this issue

2026-08-09 08:26:02 +02:00

ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115) #122

clawbot referenced this issue

2026-08-09 08:40:40 +02:00