script/cibuild was plain docker build .. The Dockerfile does COPY . . and then RUN make check, and Docker invalidates COPY . . only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed a successful build implies all checks pass — false whenever the cache was warm.
Reproduced on this branch's parent (9347a28) before changing anything: a second consecutive script/cibuild returned exit 0 in 283 ms, with #13 [builder 9/10] RUN make check reported CACHED and every other layer CACHED too.
Fix
ARG CHECK_EPOCH immediately above the check step, expanded into the command; script/cibuild passes a fresh $(date +%s%N) per invocation.
ARG CHECK_EPOCHRUNecho"check epoch: ${CHECK_EPOCH}"&& make check
Why the value is expanded inside the RUN. A build argument's value participates in the cache key of later instructions in the same stage even when those instructions never reference it — this was tested directly on this builder (Docker 29.7.2 / buildx 0.36.1 / BuildKit v0.32.2) by reverting to a bare RUN make check with ARG CHECK_EPOCH declared but unused: changing only --build-arg re-executed the layer, and repeating a previously-used value came back CACHED, proving the cache was live and the miss genuine. So a fresh value busts this layer either way. Expanding it into the command is nonetheless the deliberate choice: it makes the invalidation a property of the command string itself rather than of how a given builder version treats unreferenced args, and it surfaces the epoch in the build log as a diagnostic showing which run a layer belongs to.
(An earlier revision of this PR asserted the opposite — that BuildKit keys on the expanded command string only, so a bare ARG would still hit the cache. That claim was empirically false on this builder and has been removed from the Dockerfile comment, the commit message and this description. The code was always correct; only the justification was wrong.)
The epoch is nanosecond-granular so that two concurrent invocations starting within the same wall-clock second cannot produce an identical value and have the second served from the first's layer. Sequential runs could not collide (a build is ~45 s), but concurrent ones could.
Placing the ARG here and no earlier keeps the pinned toolchain installs and go mod download above the invalidation line. A plain docker build without the argument caches exactly as before; only the CI entrypoint changes behaviour.
Verification — by experiment, not inspection
Per the refinement in the issue comment: timings and a negative control, neither alone.
1. Twice in a row on an unchanged tree
run
result
wall clock
baseline (before fix), 2nd consecutive run
exit 0, check layer CACHED
0.283 s
after fix, run 1
exit 0, check executed
55.2 s
after fix, run 2 (byte-identical tree)
exit 0, check executed
42.2 s
Re-run after the date +%s%N change, on an unchanged tree, both exit 0 and both executing the check step:
run
wall clock
check layer
epoch
1
51.20 s
executed 42.7 s
1786256586666772611
2
46.32 s
executed (not CACHED)
1786256640154832161
Distinct epochs, nothing sub-second, no CACHED on the check layer, and apk add, both pinned go install steps, go mod download, COPY go.mod go.sum and COPY . . all still CACHED.
funcTestNegativeControlIssue115(t*testing.T){t.Fatal("NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer")}
script/cibuild failed in 24.7 s with exit 1. The build printed exactly the predicted message:
#16 20.91 zz_negative_control_test.go:6: NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer
#16 20.91 --- FAIL: TestNegativeControlIssue115 (0.00s)
#16 20.91 FAIL sneak.berlin/go/dnswatcher/internal/config 0.088s
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
A cached layer cannot produce a failure predicted in advance, so the suite demonstrably ran. The file was then deleted, git status showed only the intended modifications and no untracked leftovers, and the tree built green again in 48.1 s. An independent reviewer reproduced this with their own planted sentinel. The date +%s%N change does not alter the shape of the command string (the epoch is expanded by the builder, not the script), so the control was not re-run for it.
3. Dependency layers stayed cached
In every post-fix run, all of these report CACHED: apk add --no-cache git make gcc musl-dev binutils-gold, both pinned go install steps (golangci-lint, goimports), WORKDIR /src, COPY go.mod go.sum ./, RUN go mod download, and COPY . .. Only the check step and the steps after it re-run. No module re-download, no toolchain reinstall.
4. Build time vs the ceiling
42-55 s against the policy's 5-minute ceiling — roughly 15-18% of budget.
5. make check and script sanity
Green, exit 0, 0 issues. (run with an isolated GOLANGCI_LINT_CACHE and GOFLAGS=-count=1; not void — no parallel golangci-lint is running, no paths outside the worktree). sh -n script/cibuild clean; the script stays POSIX sh with set -eu and no bashisms.
date +%s%N was verified to work in this environment (uutils coreutils 0.8.0) and returns a full nanosecond value, not a literal %N. It is a GNU/uutils extension rather than POSIX; if a future CI host lacks it, the fallback would be appending $$ to a POSIX date +%s.
Constraints honoured
No pin touched.golang/alpinesha256 digests, golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0 all unchanged — the Dockerfile diff adds only the comment block, the ARG, and the echo prefix.
.golangci.yml untouched; still sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
Gate not weakened.make check still runs in full. No -short, no skip flags, no narrowing to lint-only.
No test behaviour touched. DNS is not mocked; no test file is modified by this PR (the negative-control file was created and deleted, and is not in the commit).
script/cibuild stays POSIX sh and keeps the ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" idiom.
TODO.md updated in the same commit. One commit; files staged by name.
Scope notes for the reviewer
Kept minimal for #109. That issue restructures the same Dockerfile for a fail-fast lint stage and ARG VERSION. This PR adds only the ARG and the echo prefix at the existing check step, so whichever lands second rebases trivially.
README.md Entrypoints entry updated: it described script/cibuild as "plain docker build .", which this change falsifies. One line, no other README edits.
Deferred, not fixed here: unbounded builder-cache growth; script/docker and plain docker build . still serving a cached check (filed as #124); and REPO_POLICIES.md lines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream with prompts#26 since that file is the org-canonical copy.
Closes #115.
`script/cibuild` was plain `docker build .`. The Dockerfile does `COPY . .` and then `RUN make check`, and Docker invalidates `COPY . .` only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed a successful build implies all checks pass — false whenever the cache was warm.
**Reproduced on this branch's parent (`9347a28`) before changing anything:** a second consecutive `script/cibuild` returned exit 0 in **283 ms**, with `#13 [builder 9/10] RUN make check` reported `CACHED` and every other layer `CACHED` too.
## Fix
`ARG CHECK_EPOCH` immediately above the check step, expanded into the command; `script/cibuild` passes a fresh `$(date +%s%N)` per invocation.
```dockerfile
ARG CHECK_EPOCH
RUN echo "check epoch: ${CHECK_EPOCH}" && make check
```
```sh
docker build --build-arg CHECK_EPOCH="$(date +%s%N)" .
```
**Why the value is expanded inside the `RUN`.** A build argument's value participates in the cache key of later instructions in the same stage **even when those instructions never reference it** — this was tested directly on this builder (Docker 29.7.2 / buildx 0.36.1 / BuildKit v0.32.2) by reverting to a bare `RUN make check` with `ARG CHECK_EPOCH` declared but unused: changing only `--build-arg` re-executed the layer, and repeating a previously-used value came back `CACHED`, proving the cache was live and the miss genuine. So a fresh value busts this layer either way. Expanding it into the command is nonetheless the deliberate choice: it makes the invalidation a property of the command string itself rather than of how a given builder version treats unreferenced args, and it surfaces the epoch in the build log as a diagnostic showing which run a layer belongs to.
*(An earlier revision of this PR asserted the opposite — that BuildKit keys on the expanded command string only, so a bare `ARG` would still hit the cache. That claim was empirically false on this builder and has been removed from the `Dockerfile` comment, the commit message and this description. The code was always correct; only the justification was wrong.)*
The epoch is nanosecond-granular so that two **concurrent** invocations starting within the same wall-clock second cannot produce an identical value and have the second served from the first's layer. Sequential runs could not collide (a build is ~45 s), but concurrent ones could.
Placing the `ARG` here and no earlier keeps the pinned toolchain installs and `go mod download` above the invalidation line. A plain `docker build` without the argument caches exactly as before; only the CI entrypoint changes behaviour.
## Verification — by experiment, not inspection
Per the refinement in the issue comment: timings **and** a negative control, neither alone.
### 1. Twice in a row on an unchanged tree
| run | result | wall clock |
| --- | --- | --- |
| baseline (before fix), 2nd consecutive run | exit 0, check layer `CACHED` | **0.283 s** |
| after fix, run 1 | exit 0, check executed | **55.2 s** |
| after fix, run 2 (byte-identical tree) | exit 0, check executed | **42.2 s** |
Re-run after the `date +%s%N` change, on an unchanged tree, both exit 0 and both executing the check step:
| run | wall clock | check layer | epoch |
| --- | --- | --- | --- |
| 1 | **51.20 s** | executed **42.7 s** | `1786256586666772611` |
| 2 | **46.32 s** | executed (not `CACHED`) | `1786256640154832161` |
Distinct epochs, nothing sub-second, no `CACHED` on the check layer, and `apk add`, both pinned `go install` steps, `go mod download`, `COPY go.mod go.sum` and `COPY . .` all still `CACHED`.
### 2. Negative control (the conclusive evidence)
Planted `internal/config/zz_negative_control_test.go`:
```go
func TestNegativeControlIssue115(t *testing.T) {
t.Fatal("NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer")
}
```
`script/cibuild` failed in 24.7 s with exit 1. The build printed exactly the predicted message:
```
#16 20.91 zz_negative_control_test.go:6: NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer
#16 20.91 --- FAIL: TestNegativeControlIssue115 (0.00s)
#16 20.91 FAIL sneak.berlin/go/dnswatcher/internal/config 0.088s
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
```
A cached layer cannot produce a failure predicted in advance, so the suite demonstrably ran. The file was then deleted, `git status` showed only the intended modifications and no untracked leftovers, and the tree built green again in **48.1 s**. An independent reviewer reproduced this with their own planted sentinel. The `date +%s%N` change does not alter the shape of the command string (the epoch is expanded by the builder, not the script), so the control was not re-run for it.
### 3. Dependency layers stayed cached
In every post-fix run, all of these report `CACHED`: `apk add --no-cache git make gcc musl-dev binutils-gold`, both pinned `go install` steps (golangci-lint, goimports), `WORKDIR /src`, `COPY go.mod go.sum ./`, `RUN go mod download`, and `COPY . .`. Only the check step and the steps after it re-run. No module re-download, no toolchain reinstall.
### 4. Build time vs the ceiling
**42-55 s** against the policy's **5-minute** ceiling — roughly 15-18% of budget.
### 5. `make check` and script sanity
Green, exit 0, `0 issues.` (run with an isolated `GOLANGCI_LINT_CACHE` and `GOFLAGS=-count=1`; not void — no `parallel golangci-lint is running`, no paths outside the worktree). `sh -n script/cibuild` clean; the script stays POSIX `sh` with `set -eu` and no bashisms.
`date +%s%N` was verified to work in this environment (uutils coreutils 0.8.0) and returns a full nanosecond value, not a literal `%N`. It is a GNU/uutils extension rather than POSIX; if a future CI host lacks it, the fallback would be appending `$$` to a POSIX `date +%s`.
## Constraints honoured
- **No pin touched.** `golang`/`alpine` `sha256` digests, golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0` all unchanged — the `Dockerfile` diff adds only the comment block, the `ARG`, and the `echo` prefix.
- **`.golangci.yml` untouched**; still `sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`.
- **Gate not weakened.** `make check` still runs in full. No `-short`, no skip flags, no narrowing to lint-only.
- **No test behaviour touched.** DNS is not mocked; no test file is modified by this PR (the negative-control file was created and deleted, and is not in the commit).
- `script/cibuild` stays POSIX `sh` and keeps the `ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"` idiom.
- `TODO.md` updated in the same commit. One commit; files staged by name.
## Scope notes for the reviewer
- **Kept minimal for #109.** That issue restructures the same `Dockerfile` for a fail-fast `lint` stage and `ARG VERSION`. This PR adds only the `ARG` and the `echo` prefix at the existing check step, so whichever lands second rebases trivially.
- **`README.md`** Entrypoints entry updated: it described `script/cibuild` as "plain `docker build .`", which this change falsifies. One line, no other README edits.
- **Deferred, not fixed here:** unbounded builder-cache growth; `script/docker` and plain `docker build .` still serving a cached check (filed as #124); and `REPO_POLICIES.md` lines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream with `prompts` #26 since that file is the org-canonical copy.
[implementer] Summary and how to reproduce the evidence yourself.
One commit, four files, +38/-6. Dockerfile gets a comment block, ARG CHECK_EPOCH, and an echo prefix on the existing check step; script/cibuild gains --build-arg CHECK_EPOCH="$(date +%s)" and a corrected header comment; README.md gets one Entrypoints line; TODO.md gets a Completed Steps entry in the same commit.
The single thing worth checking closely is why ${CHECK_EPOCH} is expanded inside the RUN rather than just declared above it. Under BuildKit — the builder here, Docker 29.7.2 — a bare ARG is not a layer and does not change the cache key of the instruction below it. Writing ARG CHECK_EPOCH above an unmodified RUN make check would look like the recommended fix from the issue while leaving the layer CACHED and the bug fully intact. That failure mode is invisible to code review, which is exactly why the negative control matters here rather than being a formality.
Reproducing the before state: check out 9347a28, run script/cibuild twice. The second returns exit 0 in about a quarter second with RUN make check marked CACHED. I measured 283 ms.
Reproducing the after state: on this branch, run script/cibuild twice. Both execute the suite (55.2 s and 42.2 s for me), print a different check epoch: value each time, and show 216 passing tests — while apk add, both pinned go install steps, go mod download and both COPY layers all stay CACHED. No modules re-downloaded, no toolchain reinstalled.
Reproducing the negative control, which is the part I would re-run if I were reviewing this: drop a file in internal/config whose only test calls t.Fatal with a string you choose, run script/cibuild, and confirm the build dies printing your string. Mine failed in 24.7 s with exit 1, surfacing NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer and --- FAIL: TestNegativeControlIssue115, with the check step exiting 2. Deleting the file returned the tree to clean and the build to green in 48.1 s. The planted file is not in the commit.
Build time lands at 42-55 s against the 5-minute ceiling. make check is green with 0 issues.
Two disclosures the reviewer should weigh:
I ran docker build only through script/cibuild; there was no direct docker build invocation, including for the negative control.
Per coordinator guidance about the shared golangci-lint cache on this host, host-side checks ran with GOLANGCI_LINT_CACHE="$(mktemp -d)" in front of the make target. One run was void with Error: parallel golangci-lint is running — a lock collision, not a lint result. Worth noting for whoever owns that guidance: the isolated cache directory did not prevent the collision, so the lock evidently does not live in GOLANGCI_LINT_CACHE. The retry was clean under both validity conditions.
Two things I found and deliberately did not fix, to keep this confined to the issue and separately reviewable from #109:
REPO_POLICIES.md now describes script/cibuild inaccurately in two places. It is the org-canonical copy, so the correction belongs upstream with prompts#26.
script/docker has the identical caching hole. It is not the CI gate and it is byte-identical across repos, so it also wants a template fix rather than a local one. Say the word and I will file it.
Nothing in the definition of done went unsatisfied.
**[implementer] Summary and how to reproduce the evidence yourself.**
One commit, four files, +38/-6. `Dockerfile` gets a comment block, `ARG CHECK_EPOCH`, and an `echo` prefix on the existing check step; `script/cibuild` gains `--build-arg CHECK_EPOCH="$(date +%s)"` and a corrected header comment; `README.md` gets one Entrypoints line; `TODO.md` gets a Completed Steps entry in the same commit.
**The single thing worth checking closely** is why `${CHECK_EPOCH}` is expanded *inside* the `RUN` rather than just declared above it. Under BuildKit — the builder here, Docker 29.7.2 — a bare `ARG` is not a layer and does not change the cache key of the instruction below it. Writing `ARG CHECK_EPOCH` above an unmodified `RUN make check` would look like the recommended fix from the issue while leaving the layer `CACHED` and the bug fully intact. That failure mode is invisible to code review, which is exactly why the negative control matters here rather than being a formality.
**Reproducing the before state:** check out `9347a28`, run `script/cibuild` twice. The second returns exit 0 in about a quarter second with `RUN make check` marked `CACHED`. I measured 283 ms.
**Reproducing the after state:** on this branch, run `script/cibuild` twice. Both execute the suite (55.2 s and 42.2 s for me), print a different `check epoch:` value each time, and show 216 passing tests — while `apk add`, both pinned `go install` steps, `go mod download` and both `COPY` layers all stay `CACHED`. No modules re-downloaded, no toolchain reinstalled.
**Reproducing the negative control**, which is the part I would re-run if I were reviewing this: drop a file in `internal/config` whose only test calls `t.Fatal` with a string you choose, run `script/cibuild`, and confirm the build dies printing *your* string. Mine failed in 24.7 s with exit 1, surfacing `NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer` and `--- FAIL: TestNegativeControlIssue115`, with the check step exiting 2. Deleting the file returned the tree to clean and the build to green in 48.1 s. The planted file is not in the commit.
Build time lands at 42-55 s against the 5-minute ceiling. `make check` is green with `0 issues.`
Two disclosures the reviewer should weigh:
- I ran `docker build` only through `script/cibuild`; there was no direct `docker build` invocation, including for the negative control.
- Per coordinator guidance about the shared golangci-lint cache on this host, host-side checks ran with `GOLANGCI_LINT_CACHE="$(mktemp -d)"` in front of the make target. One run was void with `Error: parallel golangci-lint is running` — a lock collision, not a lint result. Worth noting for whoever owns that guidance: the isolated cache directory did **not** prevent the collision, so the lock evidently does not live in `GOLANGCI_LINT_CACHE`. The retry was clean under both validity conditions.
Two things I found and deliberately did **not** fix, to keep this confined to the issue and separately reviewable from #109:
- `REPO_POLICIES.md` now describes `script/cibuild` inaccurately in two places. It is the org-canonical copy, so the correction belongs upstream with `prompts` #26.
- `script/docker` has the identical caching hole. It is not the CI gate and it is byte-identical across repos, so it also wants a template fix rather than a local one. Say the word and I will file it.
Nothing in the definition of done went unsatisfied.
[reviewer] Independent adversarial review of #122 @ f71ed12 (base main @ 9347a28)
Verdict: FAIL — needs-rework
The fix works. I reproduced the cache defeat and confirmed it with my own negative control. One blocking finding, and it is not the code: the PR's headline technical justification — repeated verbatim in a permanent Dockerfile comment, the commit message, the PR body and the issue plan — is empirically false on this host. I tested it as instructed and it does not hold.
Blocking
B1. Dockerfile lines 23-26: the stated BuildKit mechanism is wrong
# command itself, not merely declared: BuildKit derives each
# instruction's cache key from the command string after expansion, so a
# bare ARG above an unchanged RUN would still hit the cache.
This says a bare ARG CHECK_EPOCH above an unchanged RUN make check would still be served CACHED. It would not. On this builder (Docker 29.7.2 / buildx 0.36.1) the build-arg value participates in the cache key of subsequent instructions in the stage even when the instruction never references it.
Experiment: I temporarily replaced line 33 with a bare RUN make check, leaving ARG CHECK_EPOCH in place, and ran four direct docker build invocations against a byte-identical tree, varying only --build-arg:
#
--build-arg CHECK_EPOCH
RUN make check result
A
1111111111
executed, 34.4 s
B
2222222222
executed, 40.4 s (not CACHED)
C
2222222222 (repeat of B)
CACHED
D
1111111111 (repeat of A)
CACHED
B is the decisive cell: with the same tree, the same Dockerfile and an unreferenced ARG, changing only the value busted the layer. C and D prove the cache was live and working the whole time, so B was a genuine key miss and not a cold cache. The recommended-in-#115 bare-ARG form would therefore have worked on its own.
Why this is blocking rather than a nit:
#115 exists because a comment asserted a guarantee the tooling did not provide. Landing a new comment that asserts builder behaviour the builder does not exhibit is the same species of defect, in the same PR that fixes it.
The comment is written as load-bearing rationale ("not merely declared"), so a future maintainer will preserve the echo for a reason that is fictitious and may reason from the false premise elsewhere. #109 is about to restructure this exact file.
The PR body elevates it to "the one part of the change that is easy to get subtly wrong", so it is the claim most likely to be quoted forward.
Acceptable looks like: keep the code exactly as it is — expanding the epoch into the RUN is fine and has a real, honest benefit (it prints the epoch, making it visible in build output which run a layer belongs to). Reword lines 18-31 so the comment states what is true: script/cibuild passes a fresh CHECK_EPOCH per invocation; the build-arg value participates in this instruction's cache key, so the layer is re-executed every run; the value is echoed so the epoch is visible in the log. Drop the "BuildKit expands before keying, so a bare ARG would still hit the cache" claim. Correct the commit message and PR body to match. No functional change is needed.
Non-blocking
N1. script/cibuild line 15: date +%s is second-granular
Two invocations starting inside the same wall-clock second on an unchanged tree produce an identical CHECK_EPOCH, and the second can be served from the first's layer. Sequential runs cannot collide (a build is ~45 s), so this is not the reported bug, but concurrent invocations (two CI jobs, a wrapper firing twice) can. date +%s%N, or appending $$, closes it at zero cost. Note the fix is only as strong as the freshness of this value — worth a comment either way.
N2. Unbounded builder-cache growth
Every script/cibuild run mints a new check layer plus the RUN make build layer beneath it, none of which is ever reused. Correct by design, but the local builder cache now grows monotonically with cibuild invocations. Operational note only.
N3. Plain docker build . and script/docker still serve a cached check
Verified: two consecutive plain docker build . runs on the unchanged tree — the second reports #14 [builder 9/10] RUN echo "check epoch: ${CHECK_EPOCH}" && make check as CACHED, with an empty check epoch: line on the first. This matches what the PR claims, and #115 scoped the fix to the CI entrypoint, so it is in-bounds and honestly documented. For the record: Docker emits no warning for the unset ARG, so the only signal is a bare check epoch: with nothing after it. make docker / script/docker therefore still produce a green that did not run the suite. The PR flags this rather than fixing it, which is the right call for scope; it should become an issue.
N4. REPO_POLICIES.md now contradicts the code
Lines 62 and 170-172 still describe script/cibuild as running docker build . and assert "a successful build implies all checks pass" — the exact false statement #115 was filed about. Deliberately deferred upstream by the author; defensible, but the repo carries a false claim in the meantime and it should be tracked, not just mentioned in a PR body.
N5. Pre-existing, not this PR
level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0)" surfaces on every check run. Already accepted in TODO.md.
Verification performed
All builds via ./script/cibuild except the bare-ARG experiment (B1) and the no-build-arg experiment (N3), which used direct docker build invocations — stated explicitly. No go tool run directly. Nothing was committed or pushed; working tree confirmed clean before and after every experiment.
1. Twice in a row, unchanged tree
Four consecutive runs total, all on a byte-identical tree, all exit 0, all executing the check step:
run
wall clock
check layer
epoch
1
not instrumented
executed 30.3 s
1786255666
2
not instrumented
executed 41.7 s
1786255703
3
48.36 s
executed 41.3 s
1786255782
4
53.36 s
executed 44.0 s
1786255832
Distinct epoch every run, no CACHED on the check layer, nothing close to sub-second. 216 test results across all eight packages, 0 issues. from lint inside the container.
2. My own negative control (not the author's)
Planted internal/config/zz_reviewer_negctl_test.go with a single t.Fatal carrying a string I chose in advance, then ran script/cibuild. Exit 1, verbatim:
#16 26.37 zz_reviewer_negctl_test.go:6: REVIEWER-NEGCTL-122: planted failure, layer was not served from cache
#16 26.37 --- FAIL: TestReviewerNegativeControl122 (0.00s)
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
A cached layer cannot emit a failure predicted in advance. The suite demonstrably ran. File deleted, git status clean, tree green again on the next run. Definition-of-done item 1 is satisfied.
3. Dependency layers stayed cached
Confirmed CACHED on every post-fix run: apk add --no-cache git make gcc musl-dev binutils-gold, go install .../golangci-lint@c0d3ddc9..., go install .../goimports@009367f5..., WORKDIR /src, COPY go.mod go.sum ./, RUN go mod download, COPY . .. No module re-download, no toolchain reinstall. The ARG placement is correct. Item 2 satisfied.
4. Build time
48-53 s against the 5-minute ceiling in REPO_POLICIES.md line 231. Roughly 16-18% of budget. Item 6 satisfied.
5. Other checks
CI: check / check (push) = success in 58 s on f71ed12. Green. The workflow runs script/cibuild, so CI now exercises the fixed path.
Mergeable: yes; f71ed12 is a descendant of origin/main @ 9347a28. No rebase needed.
Host make check: exit 0, 0 issues., run with an isolated GOLANGCI_LINT_CACHE. Not void — zero occurrences of parallel golangci-lint is running, zero file paths outside this worktree. One valid run, no voids hit.
Pins: golang@sha256:f6751d82..., alpine@sha256:c3f8e73f..., golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0 — all unchanged in Dockerfile and script/bootstrap.
Gate not weakened: make check runs in full via script/check (test, then lint, then fmt-check). No -short, no skip flags, no lint-only narrowing. Strictly stricter.
DNS: no test file changed anywhere. git diff --name-status 9347a28..f71ed12 = 4 files, internal/ untouched. No negative-control leftovers in the commit (mine or the author's).
POSIX: #!/bin/sh, set -eu, sh -n clean, ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" idiom preserved, $(date +%s) is POSIX, no bashisms.
Header comment: now states a guarantee the script actually provides. Item 4 satisfied.
Scope: no Dockerfile restructuring; only the comment block, ARG CHECK_EPOCH, and the echo prefix. #109 rebases trivially. README.md and TODO.md edits are in-scope and correct.
Commit hygiene: one commit, title ends (closes #115), TODO.md in the same commit, markdown wrapped consistently with surrounding prose. No forbidden attribution trailers or vendor references anywhere in the diff or commit message. Terminology clean.
Summary: definition-of-done items 1-7 are all functionally met and independently verified. The single reason this is needs-rework rather than merge-ready is B1 — a factually incorrect explanation of builder behaviour, committed as a permanent comment and as the PR's central argument, in a repo where exactly that class of false claim is what produced this issue. Correcting prose is all that is required; the code should not change.
**[reviewer] Independent adversarial review of #122 @ `f71ed12` (base `main` @ `9347a28`)**
## Verdict: FAIL — `needs-rework`
The fix **works**. I reproduced the cache defeat and confirmed it with my own negative control. One blocking finding, and it is not the code: the PR's headline technical justification — repeated verbatim in a permanent `Dockerfile` comment, the commit message, the PR body and the issue plan — is **empirically false on this host**. I tested it as instructed and it does not hold.
---
## Blocking
### B1. `Dockerfile` lines 23-26: the stated BuildKit mechanism is wrong
```
# command itself, not merely declared: BuildKit derives each
# instruction's cache key from the command string after expansion, so a
# bare ARG above an unchanged RUN would still hit the cache.
```
This says a bare `ARG CHECK_EPOCH` above an unchanged `RUN make check` would still be served `CACHED`. **It would not.** On this builder (Docker 29.7.2 / buildx 0.36.1) the build-arg *value* participates in the cache key of subsequent instructions in the stage **even when the instruction never references it**.
Experiment: I temporarily replaced line 33 with a bare `RUN make check`, leaving `ARG CHECK_EPOCH` in place, and ran four direct `docker build` invocations against a byte-identical tree, varying only `--build-arg`:
| # | `--build-arg CHECK_EPOCH` | `RUN make check` result |
| --- | --- | --- |
| A | `1111111111` | executed, 34.4 s |
| B | `2222222222` | **executed, 40.4 s** (not `CACHED`) |
| C | `2222222222` (repeat of B) | `CACHED` |
| D | `1111111111` (repeat of A) | `CACHED` |
B is the decisive cell: with the same tree, the same Dockerfile and an unreferenced `ARG`, changing only the value **busted the layer**. C and D prove the cache was live and working the whole time, so B was a genuine key miss and not a cold cache. The recommended-in-#115 bare-`ARG` form would therefore have worked on its own.
Why this is blocking rather than a nit:
- #115 exists **because a comment asserted a guarantee the tooling did not provide**. Landing a new comment that asserts builder behaviour the builder does not exhibit is the same species of defect, in the same PR that fixes it.
- The comment is written as load-bearing rationale (*"not merely declared"*), so a future maintainer will preserve the `echo` for a reason that is fictitious and may reason from the false premise elsewhere. #109 is about to restructure this exact file.
- The PR body elevates it to *"the one part of the change that is easy to get subtly wrong"*, so it is the claim most likely to be quoted forward.
**Acceptable looks like:** keep the code exactly as it is — expanding the epoch into the `RUN` is fine and has a real, honest benefit (it prints the epoch, making it visible in build output which run a layer belongs to). Reword lines 18-31 so the comment states what is true: `script/cibuild` passes a fresh `CHECK_EPOCH` per invocation; the build-arg value participates in this instruction's cache key, so the layer is re-executed every run; the value is echoed so the epoch is visible in the log. Drop the "BuildKit expands before keying, so a bare ARG would still hit the cache" claim. Correct the commit message and PR body to match. No functional change is needed.
---
## Non-blocking
### N1. `script/cibuild` line 15: `date +%s` is second-granular
Two invocations starting inside the same wall-clock second on an unchanged tree produce an identical `CHECK_EPOCH`, and the second can be served from the first's layer. Sequential runs cannot collide (a build is ~45 s), so this is not the reported bug, but concurrent invocations (two CI jobs, a wrapper firing twice) can. `date +%s%N`, or appending `$$`, closes it at zero cost. Note the fix is *only* as strong as the freshness of this value — worth a comment either way.
### N2. Unbounded builder-cache growth
Every `script/cibuild` run mints a new check layer plus the `RUN make build` layer beneath it, none of which is ever reused. Correct by design, but the local builder cache now grows monotonically with cibuild invocations. Operational note only.
### N3. Plain `docker build .` and `script/docker` still serve a cached check
Verified: two consecutive plain `docker build .` runs on the unchanged tree — the second reports `#14 [builder 9/10] RUN echo "check epoch: ${CHECK_EPOCH}" && make check` as `CACHED`, with an empty `check epoch:` line on the first. This matches what the PR claims, and #115 scoped the fix to the CI entrypoint, so it is in-bounds and honestly documented. For the record: Docker emits **no** warning for the unset `ARG`, so the only signal is a bare `check epoch:` with nothing after it. `make docker` / `script/docker` therefore still produce a green that did not run the suite. The PR flags this rather than fixing it, which is the right call for scope; it should become an issue.
### N4. `REPO_POLICIES.md` now contradicts the code
Lines 62 and 170-172 still describe `script/cibuild` as running `docker build .` and assert *"a successful build implies all checks pass"* — the exact false statement #115 was filed about. Deliberately deferred upstream by the author; defensible, but the repo carries a false claim in the meantime and it should be tracked, not just mentioned in a PR body.
### N5. Pre-existing, not this PR
`level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0)"` surfaces on every check run. Already accepted in `TODO.md`.
---
## Verification performed
All builds via `./script/cibuild` except the bare-`ARG` experiment (B1) and the no-build-arg experiment (N3), which used direct `docker build` invocations — stated explicitly. No `go` tool run directly. Nothing was committed or pushed; working tree confirmed clean before and after every experiment.
### 1. Twice in a row, unchanged tree
Four consecutive runs total, all on a byte-identical tree, all exit 0, all executing the check step:
| run | wall clock | check layer | epoch |
| --- | --- | --- | --- |
| 1 | not instrumented | executed 30.3 s | 1786255666 |
| 2 | not instrumented | executed 41.7 s | 1786255703 |
| **3** | **48.36 s** | **executed 41.3 s** | 1786255782 |
| **4** | **53.36 s** | **executed 44.0 s** | 1786255832 |
Distinct epoch every run, no `CACHED` on the check layer, nothing close to sub-second. 216 test results across all eight packages, `0 issues.` from lint inside the container.
### 2. My own negative control (not the author's)
Planted `internal/config/zz_reviewer_negctl_test.go` with a single `t.Fatal` carrying a string I chose in advance, then ran `script/cibuild`. Exit **1**, verbatim:
```
#16 26.37 zz_reviewer_negctl_test.go:6: REVIEWER-NEGCTL-122: planted failure, layer was not served from cache
#16 26.37 --- FAIL: TestReviewerNegativeControl122 (0.00s)
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
```
A cached layer cannot emit a failure predicted in advance. The suite demonstrably ran. File deleted, `git status` clean, tree green again on the next run. **Definition-of-done item 1 is satisfied.**
### 3. Dependency layers stayed cached
Confirmed `CACHED` on every post-fix run: `apk add --no-cache git make gcc musl-dev binutils-gold`, `go install .../golangci-lint@c0d3ddc9...`, `go install .../goimports@009367f5...`, `WORKDIR /src`, `COPY go.mod go.sum ./`, `RUN go mod download`, `COPY . .`. No module re-download, no toolchain reinstall. The `ARG` placement is correct. **Item 2 satisfied.**
### 4. Build time
**48-53 s** against the 5-minute ceiling in `REPO_POLICIES.md` line 231. Roughly 16-18% of budget. **Item 6 satisfied.**
### 5. Other checks
- **CI**: `check / check (push)` = **success in 58 s** on `f71ed12`. Green. The workflow runs `script/cibuild`, so CI now exercises the fixed path.
- **Mergeable**: yes; `f71ed12` is a descendant of `origin/main` @ `9347a28`. No rebase needed.
- **Host `make check`**: exit 0, `0 issues.`, run with an isolated `GOLANGCI_LINT_CACHE`. **Not void** — zero occurrences of `parallel golangci-lint is running`, zero file paths outside this worktree. One valid run, no voids hit.
- **Pins**: `golang@sha256:f6751d82...`, `alpine@sha256:c3f8e73f...`, golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0` — all unchanged in `Dockerfile` and `script/bootstrap`.
- **`.golangci.yml`**: `sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. Unmodified.
- **Gate not weakened**: `make check` runs in full via `script/check` (test, then lint, then fmt-check). No `-short`, no skip flags, no lint-only narrowing. Strictly stricter.
- **DNS**: no test file changed anywhere. `git diff --name-status 9347a28..f71ed12` = 4 files, `internal/` untouched. No negative-control leftovers in the commit (mine or the author's).
- **POSIX**: `#!/bin/sh`, `set -eu`, `sh -n` clean, `ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"` idiom preserved, `$(date +%s)` is POSIX, no bashisms.
- **Header comment**: now states a guarantee the script actually provides. **Item 4 satisfied.**
- **Scope**: no Dockerfile restructuring; only the comment block, `ARG CHECK_EPOCH`, and the `echo` prefix. #109 rebases trivially. `README.md` and `TODO.md` edits are in-scope and correct.
- **Commit hygiene**: one commit, title ends ` (closes #115)`, `TODO.md` in the same commit, markdown wrapped consistently with surrounding prose. No forbidden attribution trailers or vendor references anywhere in the diff or commit message. Terminology clean.
---
**Summary:** definition-of-done items 1-7 are all functionally met and independently verified. The single reason this is `needs-rework` rather than `merge-ready` is B1 — a factually incorrect explanation of builder behaviour, committed as a permanent comment and as the PR's central argument, in a repo where exactly that class of false claim is what produced this issue. Correcting prose is all that is required; the code should not change.
[manager] Independent adversarial review returned FAIL. Relabeled needs-rework, staying assigned to clawbot. Dispatching a prose-only rework.
The fix is correct and stays. The explanation committed alongside it is false and must go.
Two independent experiments agree, and both contradict the PR
The reviewer reverted the RUN to the bare form (leaving ARG CHECK_EPOCH declared but unreferenced) and ran four builds on a byte-identical tree, varying only --build-arg:
#
value
result
A
1111111111
executed, 34.4s
B
2222222222
executed, 40.4s — not CACHED
C
2222222222
CACHED
D
1111111111
CACHED
B is the decisive one; C and D prove the cache was live throughout, so B was a genuine key miss rather than an absent cache.
I ran my own probe before the review returned, on a minimal two-variant Dockerfile using this repo's pinned alpine digest. Same result: a declared-but-unreferenced ARGdoes enter the cache key, and a changed value forces re-execution (DONE 0.3s, not CACHED).
So the comment at Dockerfile lines 23-26 — that BuildKit "derives each instruction's cache key from the command string after expansion, so a bare ARG above an unchanged RUN would still hit the cache" — is empirically false on this builder (Docker 29.7.2, buildx v0.36.1, BuildKit v0.32.2, DOCKER_BUILDKIT unset, no # syntax= line). The bare form recommended in #115 would have worked on its own.
Why this blocks, when the fix demonstrably works
Because the false claim is committed as a permanent Dockerfile comment, and because this PR is the reference implementation other repos are copying. Two sibling repos independently measured the opposite behaviour, and this claim was the sole reason propagation of the shared-template fix was halted while three repos waited.
A correct fix carrying an incorrect justification is more dangerous than a plain error: the next person to touch this will reason from the comment. Merging it would embed a falsehood at the exact point where someone might later "simplify" the expanded form back to bare — and be misled about why they shouldn't.
A methodological note worth keeping
My first probe attempt returned "both CACHED" and would have confirmed the PR's claim. It was wrong — an earlier run of my own probe had already populated the cache for those exact command strings. I only caught it because the result was too convenient. The corrected probe embeds a unique per-run nonce so no prior entry can match.
That is the same trap as the original bug, one level up: a cache experiment can itself be served from cache. Anyone re-testing this without a nonce is measuring their own history.
What the author got right, and it is most of it
The negative control was rigorous and real, and the reviewer reproduced it independently with their own planted sentinel (REVIEWER-NEGCTL-122, build exit 1, exact predicted string in the output, tree clean afterwards). Definition-of-done item 1 is satisfied. Four consecutive script/cibuild runs on an unchanged tree all executed the check step with distinct epochs — 48.36s and 53.36s wall on runs 3 and 4, nothing sub-second, no CACHED on the check layer. Dependency layers all stayed CACHED, so the ARG placement is right. 48-53s against the 5-minute ceiling.
The failure was narrow and specific: the author asserted the mechanism as reasoning while proving only the conclusion. Their negative control tested the expanded form; it never tested the bare form.
Rework scope — prose only
Reword the Dockerfile comment block, the commit message, and the PR body to state that the build-arg value participates in the cache key of subsequent instructions even when they do not reference it, and that the expanded form is chosen deliberately — because it is correct regardless of how BuildKit treats unreferenced args, and because it surfaces the epoch in build logs as a diagnostic. No code change; the echo stays.
Also worth fixing while in there: date +%s is second-granular, so two concurrent invocations in the same second produce identical epochs and the later one could be served from cache — the original bug, narrowly. Builds take 40s+ so sequential runs cannot collide, but concurrent ones can. date +%s%N removes the concern for one character.
Deferred to follow-ups, not this PR: unbounded builder-cache growth; script/docker and plain docker build . still serving a cached check (already filed as #124); and REPO_POLICIES.md lines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream with prompts#26 since that file is the org-canonical copy.
A fresh reviewer will re-review the amended head.
**[manager]** Independent adversarial review returned **FAIL**. Relabeled `needs-rework`, staying assigned to `clawbot`. Dispatching a prose-only rework.
**The fix is correct and stays. The explanation committed alongside it is false and must go.**
## Two independent experiments agree, and both contradict the PR
The reviewer reverted the `RUN` to the bare form (leaving `ARG CHECK_EPOCH` declared but unreferenced) and ran four builds on a byte-identical tree, varying only `--build-arg`:
| # | value | result |
|---|---|---|
| A | `1111111111` | executed, 34.4s |
| B | `2222222222` | **executed, 40.4s — not `CACHED`** |
| C | `2222222222` | `CACHED` |
| D | `1111111111` | `CACHED` |
B is the decisive one; C and D prove the cache was live throughout, so B was a genuine key miss rather than an absent cache.
I ran my own probe before the review returned, on a minimal two-variant Dockerfile using this repo's pinned alpine digest. Same result: a declared-but-unreferenced `ARG` **does** enter the cache key, and a changed value forces re-execution (`DONE 0.3s`, not `CACHED`).
So the comment at `Dockerfile` lines 23-26 — that BuildKit "derives each instruction's cache key from the command string after expansion, so a bare ARG above an unchanged RUN would still hit the cache" — is **empirically false on this builder** (Docker 29.7.2, buildx v0.36.1, BuildKit v0.32.2, `DOCKER_BUILDKIT` unset, no `# syntax=` line). The bare form recommended in #115 would have worked on its own.
## Why this blocks, when the fix demonstrably works
Because the false claim is committed as a permanent `Dockerfile` comment, and because this PR is the reference implementation other repos are copying. Two sibling repos independently measured the opposite behaviour, and this claim was the sole reason propagation of the shared-template fix was halted while three repos waited.
A correct fix carrying an incorrect justification is more dangerous than a plain error: the next person to touch this will reason *from* the comment. Merging it would embed a falsehood at the exact point where someone might later "simplify" the expanded form back to bare — and be misled about why they shouldn't.
## A methodological note worth keeping
My first probe attempt returned "both `CACHED`" and would have *confirmed* the PR's claim. It was wrong — an earlier run of my own probe had already populated the cache for those exact command strings. I only caught it because the result was too convenient. The corrected probe embeds a unique per-run nonce so no prior entry can match.
That is the same trap as the original bug, one level up: **a cache experiment can itself be served from cache.** Anyone re-testing this without a nonce is measuring their own history.
## What the author got right, and it is most of it
The negative control was rigorous and real, and the reviewer reproduced it independently with their own planted sentinel (`REVIEWER-NEGCTL-122`, build exit 1, exact predicted string in the output, tree clean afterwards). Definition-of-done item 1 is satisfied. Four consecutive `script/cibuild` runs on an unchanged tree all executed the check step with distinct epochs — 48.36s and 53.36s wall on runs 3 and 4, nothing sub-second, no `CACHED` on the check layer. Dependency layers all stayed `CACHED`, so the `ARG` placement is right. 48-53s against the 5-minute ceiling.
The failure was narrow and specific: the author asserted the *mechanism* as reasoning while proving only the *conclusion*. Their negative control tested the expanded form; it never tested the bare form.
## Rework scope — prose only
Reword the `Dockerfile` comment block, the commit message, and the PR body to state that **the build-arg value participates in the cache key of subsequent instructions even when they do not reference it**, and that the expanded form is chosen deliberately — because it is correct regardless of how BuildKit treats unreferenced args, and because it surfaces the epoch in build logs as a diagnostic. **No code change**; the `echo` stays.
Also worth fixing while in there: `date +%s` is second-granular, so two concurrent invocations in the same second produce identical epochs and the later one could be served from cache — the original bug, narrowly. Builds take 40s+ so sequential runs cannot collide, but concurrent ones can. `date +%s%N` removes the concern for one character.
Deferred to follow-ups, not this PR: unbounded builder-cache growth; `script/docker` and plain `docker build .` still serving a cached check (already filed as #124); and `REPO_POLICIES.md` lines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream with `prompts` #26 since that file is the org-canonical copy.
A fresh reviewer will re-review the amended head.
[implementer] Rework of f71ed12 -> ff66ecc. Prose plus one character. No functional change to the mechanism; the ARG placement and the echo are untouched.
B1 (blocking) — false BuildKit claim: fixed, and the finding is accepted without reservation
The reviewer is right and my original justification was wrong. I asserted a mechanism while my negative control only ever established the conclusion — it exercised the expanded form and never once tested the bare form, so it could not have detected the error. Two independent experiments (the reviewer's four-build A/B/C/D matrix, and the manager's nonce-bearing minimal probe) both show that a declared-but-unreferenced ARGdoes enter the cache key of later instructions in the stage, and that changing its value forces re-execution. Cells C and D settle it: the cache was live throughout, so B was a genuine key miss.
Dockerfile lines 23-26 now read (replacing the four lines that carried the false claim):
# build reports a green it did not earn. A build argument's value
# participates in the cache key of later instructions in the stage even
# when they do not reference it, so a fresh value busts this layer
# either way. It is expanded into the command deliberately: that makes
# the invalidation a property of the command string itself rather than
# of how a given builder treats unreferenced args, and it surfaces the
# epoch in the build log as a diagnostic.
The surrounding comment (the CHECK_EPOCH purpose sentence above, the ARG-placement paragraph below) is unchanged. The commit message carries the identical correction in its Fix: paragraph, and the PR body has been rewritten — including an explicit note that the earlier revision asserted the opposite and why that was wrong, so the record does not just quietly change.
N1 (non-blocking) — date +%s granularity: fixed
script/cibuild line 15 now passes $(date +%s%N). Sequential runs never could collide at ~45 s per build; concurrent invocations within one wall-clock second could, and that is the original bug in miniature.
Portability, since %N is a GNU/uutils extension and not POSIX: verified working here — date (uutils coreutils) 0.8.0 returns a full 19-digit value, and it behaves identically under /bin/sh, not just the interactive shell. No literal %N anywhere. Had it not worked, the fallback would have been $(date +%s)$$, which is POSIX and still collision-free for concurrent invocations. sh -n script/cibuild is clean; still #!/bin/sh, set -eu, no bashisms, ROOT idiom preserved.
The change does not alter the shape of the command string — the epoch is substituted by the builder, not the script — so per the rework brief I did not repeat the negative control. Two independent ones already passed against this exact RUN.
Verification of the reworked head
script/cibuild twice in a row, unchanged tree, after the %N change:
run
wall clock
check layer
epoch
1
51.20 s
executed, 42.7 s
1786256586666772611
2
46.32 s
executed, not CACHED
1786256640154832161
Distinct nanosecond epochs, both printed in the build log, nothing remotely sub-second, no CACHED on the check layer either time. Layers 9-14 (apk add, both pinned go install steps, WORKDIR, COPY go.mod go.sum, go mod download) plus COPY . . all CACHED on both runs — no toolchain reinstall, no module re-download. Gate unweakened: full make check inside the container.
Host make check: exit 0, 0 issues., run as GOLANGCI_LINT_CACHE=<isolated dir> GOFLAGS=-count=1 make check. Not void — no parallel golangci-lint is running, and no reported path outside this worktree. I hit zero void runs during this rework, one clean run on the first attempt. The gomodguard deprecation warning (N5) still surfaces; already tracked, untouched.
Untouched, as required: no pin moved (golang/alpine digests, golangci-lint c0d3ddc9..., goimports 009367f5...); .golangci.yml still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; no internal/ or test file changed; no negative-control leftovers; DNS still never mocked. git status clean after both builds. Files staged by name — Dockerfile, TODO.md, script/cibuild. TODO.md sits in the same commit and its Completed Steps entry was updated to say $(date +%s%N).
The commit was amended rather than followed up, so the head is still a single commit whose title ends (closes #115), force-pushed to fix/115-cibuild-cache. No new PR, no new branch. Diff against base is now 4 files, +41/-6.
N2, N3, N4 — acknowledged, deliberately not addressed here
Builder-cache growth is an operational note; script/docker and plain docker build . still serving a cached check is filed as #124; REPO_POLICIES.md lines 62 and 170-172 belong upstream with prompts#26 since that file is the org-canonical copy. All three are out of #115's scope and none is silently skipped.
Label left needs-rework, assignee left clawbot, for a fresh reviewer to re-review ff66ecc.
**[implementer] Rework of `f71ed12` -> `ff66ecc`.** Prose plus one character. No functional change to the mechanism; the `ARG` placement and the `echo` are untouched.
## B1 (blocking) — false BuildKit claim: fixed, and the finding is accepted without reservation
The reviewer is right and my original justification was wrong. I asserted a mechanism while my negative control only ever established the conclusion — it exercised the expanded form and never once tested the bare form, so it could not have detected the error. Two independent experiments (the reviewer's four-build A/B/C/D matrix, and the manager's nonce-bearing minimal probe) both show that a declared-but-unreferenced `ARG` **does** enter the cache key of later instructions in the stage, and that changing its value forces re-execution. Cells C and D settle it: the cache was live throughout, so B was a genuine key miss.
`Dockerfile` lines 23-26 now read (replacing the four lines that carried the false claim):
```
# build reports a green it did not earn. A build argument's value
# participates in the cache key of later instructions in the stage even
# when they do not reference it, so a fresh value busts this layer
# either way. It is expanded into the command deliberately: that makes
# the invalidation a property of the command string itself rather than
# of how a given builder treats unreferenced args, and it surfaces the
# epoch in the build log as a diagnostic.
```
The surrounding comment (the `CHECK_EPOCH` purpose sentence above, the `ARG`-placement paragraph below) is unchanged. The commit message carries the identical correction in its `Fix:` paragraph, and the PR body has been rewritten — including an explicit note that the earlier revision asserted the opposite and why that was wrong, so the record does not just quietly change.
## N1 (non-blocking) — `date +%s` granularity: fixed
`script/cibuild` line 15 now passes `$(date +%s%N)`. Sequential runs never could collide at ~45 s per build; concurrent invocations within one wall-clock second could, and that is the original bug in miniature.
**Portability, since `%N` is a GNU/uutils extension and not POSIX:** verified working here — `date (uutils coreutils) 0.8.0` returns a full 19-digit value, and it behaves identically under `/bin/sh`, not just the interactive shell. No literal `%N` anywhere. Had it not worked, the fallback would have been `$(date +%s)$$`, which is POSIX and still collision-free for concurrent invocations. `sh -n script/cibuild` is clean; still `#!/bin/sh`, `set -eu`, no bashisms, `ROOT` idiom preserved.
The change does not alter the shape of the command string — the epoch is substituted by the builder, not the script — so per the rework brief I did not repeat the negative control. Two independent ones already passed against this exact `RUN`.
## Verification of the reworked head
**`script/cibuild` twice in a row, unchanged tree**, after the `%N` change:
| run | wall clock | check layer | epoch |
| --- | --- | --- | --- |
| 1 | **51.20 s** | executed, **42.7 s** | `1786256586666772611` |
| 2 | **46.32 s** | executed, not `CACHED` | `1786256640154832161` |
Distinct nanosecond epochs, both printed in the build log, nothing remotely sub-second, no `CACHED` on the check layer either time. Layers 9-14 (`apk add`, both pinned `go install` steps, `WORKDIR`, `COPY go.mod go.sum`, `go mod download`) plus `COPY . .` all `CACHED` on both runs — no toolchain reinstall, no module re-download. Gate unweakened: full `make check` inside the container.
**Host `make check`**: exit 0, `0 issues.`, run as `GOLANGCI_LINT_CACHE=<isolated dir> GOFLAGS=-count=1 make check`. **Not void** — no `parallel golangci-lint is running`, and no reported path outside this worktree. **I hit zero void runs during this rework**, one clean run on the first attempt. The `gomodguard` deprecation warning (N5) still surfaces; already tracked, untouched.
**Untouched, as required**: no pin moved (`golang`/`alpine` digests, golangci-lint `c0d3ddc9...`, goimports `009367f5...`); `.golangci.yml` still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`; no `internal/` or test file changed; no negative-control leftovers; DNS still never mocked. `git status` clean after both builds. Files staged by name — `Dockerfile`, `TODO.md`, `script/cibuild`. `TODO.md` sits in the same commit and its Completed Steps entry was updated to say `$(date +%s%N)`.
The commit was amended rather than followed up, so the head is still a single commit whose title ends ` (closes #115)`, force-pushed to `fix/115-cibuild-cache`. No new PR, no new branch. Diff against base is now 4 files, +41/-6.
## N2, N3, N4 — acknowledged, deliberately not addressed here
Builder-cache growth is an operational note; `script/docker` and plain `docker build .` still serving a cached check is filed as #124; `REPO_POLICIES.md` lines 62 and 170-172 belong upstream with `prompts` #26 since that file is the org-canonical copy. All three are out of #115's scope and none is silently skipped.
Label left `needs-rework`, assignee left `clawbot`, for a fresh reviewer to re-review `ff66ecc`.
[reviewer] Independent adversarial re-review of #122 @ ff66ecc (base main @ 9347a28; prior head f71ed12)
Verdict: PASS
I did not author or rework this change and did not see it before this review. I re-tested the corrected claim from scratch rather than accepting the earlier experiments, using a per-run nonce so no result could be served from my own probe history. The new wording is true on this builder. No blocking findings. Four non-blocking items below, one of which I considered blocking and explain why I did not.
1. The corrected Dockerfile claim — verified, twice, with liveness controls
The claim under test (Dockerfile lines 23-26): "A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way."
Builder: Docker 29.7.2, buildx v0.36.1. Both experiments used direct docker build — stated explicitly, as required. Nonce for this review: ba7900b69c439a99, never used by any earlier run.
Experiment A — minimal context, nonce embedded in every command string (so no pre-existing cache entry can match). Dockerfile: pinned alpine digest, a base RUN, then ARG CHECK_EPOCH, then a final RUN that never references the arg.
#
--build-arg CHECK_EPOCH
parent layer
final unreferenced RUN
A
...-a (fresh)
executed
executed, 3.6 s
B
...-b (fresh)
CACHED
executed, 0.2 s — not CACHED
C
...-b (repeat)
CACHED
CACHED
D
...-a (repeat)
CACHED
CACHED
Experiment B — the repo's real Dockerfile, with line 36 temporarily reverted to a bare RUN make check and ARG CHECK_EPOCH left declared-but-unreferenced, byte-identical tree across all four builds:
#
--build-arg CHECK_EPOCH
COPY . .
RUN make check
A
...-A (fresh)
executed
executed
B
...-B (fresh)
CACHED
executed, 31.4 s — not CACHED
C
...-B (repeat)
CACHED
CACHED
D
...-A (repeat)
CACHED
CACHED
Cell B is decisive in both: identical tree, identical command string, identical cached parent, only the unreferenced arg value changed, and the layer re-executed. Cells C and D are the liveness proof — repeating a previously-seen value comes back CACHED, so the cache was live throughout and B was a genuine key miss, not an empty cache. In Experiment B the COPY . . above it reports CACHED in the same build that RUN make check executes, which isolates the arg as the only variable.
Conclusion: the reworked wording is correct, and the earlier wording it replaced was false. The second sentence — that the expanded form is chosen so invalidation is a property of the command string rather than of how a builder treats unreferenced args — is a hedge, not a mechanism claim, and is sound. B1 is resolved.
Experiment reverted; git status clean, HEAD still ff66ecc, Dockerfile restored byte-for-byte (git diff HEAD empty).
2. The corrected claim propagated everywhere
grep for bare ARG, after expansion, would still hit the cache across the tree: no hits. The Fix: paragraph of the commit message (git log 9347a28..ff66ecc --format=%B) carries the corrected claim verbatim. The PR body carries it plus an explicit erratum recording that the earlier revision asserted the opposite. The old wording survives nowhere.
3. script/cibuild twice in a row on an unchanged tree
run
wall clock
check layer
epoch
1
2 m 12.2 s
executed
1786257099551809952
2
33.9 s
executed, DONE 31.6s, not CACHED
1786257240813048311
3 (post-negative-control)
43.7 s
executed
1786257361506593276
Distinct epoch every run, printed in the build log, nothing remotely sub-second, never CACHED.
Run 1's 2 m 12 s is my doing, not the PR's: my Experiment B had left the builder holding a different Dockerfile content, so COPY . . and everything below it rebuilt. Runs 2 and 3 are the representative figures. Steady-state build time 34-44 s against the 5-minute ceiling (REPO_POLICIES.md line 231) — roughly 11-15% of budget.
4. My own negative control (third independent one)
Planted internal/config/zz_rereviewer_negctl_test.go with a single t.Fatal carrying a sentinel I chose in advance. script/cibuild output, verbatim:
#16 [builder 9/10] RUN echo "check epoch: 1786257286631785140" && make check
#16 0.351 check epoch: 1786257286631785140
#16 26.58 zz_rereviewer_negctl_test.go:6: REREVIEW-NEGCTL-122-ba7900b6: planted failure, the check layer was not served from cache
#16 26.58 --- FAIL: TestRereviewNegativeControl122 (0.00s)
#16 26.58 FAIL sneak.berlin/go/dnswatcher/internal/config 0.069s
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
Separately confirmed script/cibuild exit=1. File deleted, git status clean, tree green again on run 3. A cached layer cannot emit a failure predicted in advance — definition-of-done item 1 is satisfied.
5. Dependency layers stayed cached
On run 2, all CACHED: apk add --no-cache git make gcc musl-dev binutils-gold, go install ...golangci-lint@c0d3ddc9..., go install ...goimports@009367f5..., WORKDIR /src, COPY go.mod go.sum ./, RUN go mod download, COPY . ., plus all four runtime-stage layers. No toolchain reinstall, no module re-download. The ARG placement is right. Item 2 satisfied.
6. Scope discipline
git diff f71ed12..ff66ecc is exactly three hunks: the Dockerfile comment block (4 lines out, 7 in), the TODO.md sentence that quoted $(date +%s), and one script/cibuild line. NoARG relocation, no removal of the echo, no restructuring, no internal/ or test-file change, no README.md change. Prose plus one character, as scoped.
7. Hard constraints
Pins: golang@sha256:f6751d82..., alpine@sha256:c3f8e73f..., golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0 — unchanged in both Dockerfile and script/bootstrap; neither file's pin lines appear in the diff.
No vendor/attribution references: case-insensitive grep over the full diff, the commit message, and author/committer identity — zero hits. No Co-Authored-By, no session trailers.
DNS never mocked: git diff --name-status 9347a28..ff66ecc = 4 files (Dockerfile, README.md, TODO.md, script/cibuild). No test file, no internal/ file. No negative-control leftovers committed by anyone; my own planted file is deleted and the tree is clean.
POSIX sh: #!/bin/sh, set -eu, sh -n script/cibuild clean, ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" idiom preserved, no bashisms.
Gate strength: make check runs in full inside the container (test, lint, fmt-check). No -short, no skip flags, no lint-only narrowing. script/test, script/check, script/lint, script/fmt-check are untouched by this PR.
Commit hygiene: one commit, title ends (closes #115), TODO.md updated in the same commit and consistent with the surrounding entry style.
CI: check / check (push) = success on ff66ecc. The workflow (.gitea/workflows/check.yml) is a single run: script/cibuild, so CI exercises the fixed path.
Mergeable: ff66ecc is a descendant of origin/main @ 9347a28 (re-fetched at end of review; main has not moved). No rebase needed.
Host make check: exit 0, 0 issues., make fmt-check silent. Not void — grep of the lint output for parallel golangci-lint is running, any ../ path, and any absolute path outside this worktree returned nothing. Zero void runs hit during this review.
Non-blocking
N1. %N is not POSIX, and busybox silently drops it — the "never cached" guarantee is host-conditional
I tested three implementations rather than one:
host, date (uutils coreutils) 0.8.0, under /bin/sh (dash): 19-digit nanosecond value. Works.
the repo's pinned alpine image, busybox date: date +%s%N prints 1786257437 — %N is silently dropped, exit 0. Degrades to second granularity.
So on a busybox/POSIX-only host the epoch is second-granular and the concurrency guarantee the rework was meant to add does not hold there. That is no worse than f71ed12, which is why it is not blocking. The catastrophic case — a constant value — requires date to exit non-zero, and no implementation I could find fails on an unknown strftime conversion; they copy it through. Worth knowing that if it ever did fail, set -eu would not save you: I verified that in dash a failing command substitution inside a command's arguments does not trip set -e (sh -c 'set -eu; echo "[$(false)]"; echo REACHED' prints [], then REACHED, exit 0). The build would then run with CHECK_EPOCH= constant-empty and serve a cached green, silently. Low likelihood, maximum blast radius.
$(date +%s%N)$$ costs one more token, keeps nanoseconds where available, and makes concurrent-invocation uniqueness unconditional via the PID even where %N is dropped or a substitution returns empty. Recommended, not required.
Related: script/cibuild's header comment and the new README.md line both state the guarantee unconditionally ("never served from the Docker layer cache", "always means the checks ran on this invocation"). Those are true on this host and on the ubuntu-latest runner, and the PR body honestly discloses the %N caveat, so this does not reach the bar that failed f71ed12 — that claim was false on this builder. Flagging it so it is a deliberate acceptance rather than an oversight.
N2. An unset CHECK_EPOCH defaults silently
RUN echo "check epoch: ${CHECK_EPOCH}" && make check with no --build-arg prints an empty value, emits no builder warning, and serves the cached check — the repo's known-rejected "silent defaulting" shape. Failing loudly on an empty value would fix script/docker and plain docker build . at the same time, but it would change behaviour the issue explicitly scoped out ("a plain docker build without the argument caches exactly as before") and is already tracked as #124. Correct call to leave it; noting it belongs in #124's brief rather than being lost.
N3. Could not corroborate CI-side behaviour from logs
The commit status on ff66ecc is green and the workflow does invoke script/cibuild, but the job log the API returned for that run is stale and unrelated (dated 2026-02-21, showing a native go build and pre-#93 DNS skips rather than a Docker build). So there is no log-level confirmation that date +%s%N expanded correctly on the runner or that the check layer executed there. Not attributable to this PR, and the green status plus a workflow that is one line is sufficient. Recorded so it is not mistaken for verified.
N4. Deferred items, correctly deferred
REPO_POLICIES.md lines 62 and 170-172 still describe script/cibuild as docker build . and still assert "a successful build implies all checks pass" — the exact statement #115 falsified. Confirmed still present. Deferred upstream to prompts#26 as the org-canonical copy. Also unbounded builder-cache growth, and #124. All three are out of #115's scope and none is silently skipped.
N5. Nit
The commit subject is 95 characters and becomes ~102 after the squash-merge (#122) suffix. No repo policy sets a limit, so this is preference only.
Summary: the rework does exactly what it was scoped to do and nothing else. The claim that failed f71ed12 has been replaced with one I independently verified true under a nonce-protected design with liveness controls, and the correction reached the Dockerfile, the commit message and the PR body alike. Definition-of-done items 1-7 all satisfied and independently re-verified, including a third negative control. Constraints intact, CI green, mergeable, no void lint runs.
**[reviewer] Independent adversarial re-review of #122 @ `ff66ecc`** (base `main` @ `9347a28`; prior head `f71ed12`)
## Verdict: PASS
I did not author or rework this change and did not see it before this review. I re-tested the corrected claim from scratch rather than accepting the earlier experiments, using a per-run nonce so no result could be served from my own probe history. **The new wording is true on this builder.** No blocking findings. Four non-blocking items below, one of which I considered blocking and explain why I did not.
---
## 1. The corrected `Dockerfile` claim — verified, twice, with liveness controls
The claim under test (`Dockerfile` lines 23-26): *"A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way."*
Builder: Docker **29.7.2**, buildx **v0.36.1**. Both experiments used direct `docker build` — stated explicitly, as required. Nonce for this review: `ba7900b69c439a99`, never used by any earlier run.
**Experiment A — minimal context, nonce embedded in every command string** (so no pre-existing cache entry can match). Dockerfile: pinned `alpine` digest, a base `RUN`, then `ARG CHECK_EPOCH`, then a final `RUN` that **never references** the arg.
| # | `--build-arg CHECK_EPOCH` | parent layer | final unreferenced `RUN` |
| --- | --- | --- | --- |
| A | `...-a` (fresh) | executed | executed, 3.6 s |
| B | `...-b` (fresh) | **CACHED** | **executed, 0.2 s — not `CACHED`** |
| C | `...-b` (repeat) | CACHED | **CACHED** |
| D | `...-a` (repeat) | CACHED | **CACHED** |
**Experiment B — the repo's real `Dockerfile`**, with line 36 temporarily reverted to a bare `RUN make check` and `ARG CHECK_EPOCH` left declared-but-unreferenced, byte-identical tree across all four builds:
| # | `--build-arg CHECK_EPOCH` | `COPY . .` | `RUN make check` |
| --- | --- | --- | --- |
| A | `...-A` (fresh) | executed | executed |
| B | `...-B` (fresh) | **CACHED** | **executed, 31.4 s — not `CACHED`** |
| C | `...-B` (repeat) | CACHED | **CACHED** |
| D | `...-A` (repeat) | CACHED | **CACHED** |
Cell B is decisive in both: identical tree, identical command string, identical cached parent, only the unreferenced arg value changed, and the layer re-executed. Cells C and D are the liveness proof — repeating a previously-seen value comes back `CACHED`, so the cache was live throughout and B was a genuine key miss, not an empty cache. In Experiment B the `COPY . .` above it reports `CACHED` in the same build that `RUN make check` executes, which isolates the arg as the only variable.
**Conclusion: the reworked wording is correct, and the earlier wording it replaced was false.** The second sentence — that the expanded form is chosen so invalidation is a property of the command string rather than of how a builder treats unreferenced args — is a hedge, not a mechanism claim, and is sound. B1 is resolved.
Experiment reverted; `git status` clean, `HEAD` still `ff66ecc`, `Dockerfile` restored byte-for-byte (`git diff HEAD` empty).
## 2. The corrected claim propagated everywhere
`grep` for `bare ARG`, `after expansion`, `would still hit the cache` across the tree: **no hits**. The `Fix:` paragraph of the commit message (`git log 9347a28..ff66ecc --format=%B`) carries the corrected claim verbatim. The PR body carries it plus an explicit erratum recording that the earlier revision asserted the opposite. The old wording survives nowhere.
## 3. `script/cibuild` twice in a row on an unchanged tree
| run | wall clock | check layer | epoch |
| --- | --- | --- | --- |
| 1 | **2 m 12.2 s** | executed | `1786257099551809952` |
| 2 | **33.9 s** | executed, `DONE 31.6s`, not `CACHED` | `1786257240813048311` |
| 3 (post-negative-control) | **43.7 s** | executed | `1786257361506593276` |
Distinct epoch every run, printed in the build log, nothing remotely sub-second, never `CACHED`.
**Run 1's 2 m 12 s is my doing, not the PR's**: my Experiment B had left the builder holding a different `Dockerfile` content, so `COPY . .` and everything below it rebuilt. Runs 2 and 3 are the representative figures. **Steady-state build time 34-44 s against the 5-minute ceiling** (`REPO_POLICIES.md` line 231) — roughly 11-15% of budget.
## 4. My own negative control (third independent one)
Planted `internal/config/zz_rereviewer_negctl_test.go` with a single `t.Fatal` carrying a sentinel I chose in advance. `script/cibuild` output, verbatim:
```
#16 [builder 9/10] RUN echo "check epoch: 1786257286631785140" && make check
#16 0.351 check epoch: 1786257286631785140
#16 26.58 zz_rereviewer_negctl_test.go:6: REREVIEW-NEGCTL-122-ba7900b6: planted failure, the check layer was not served from cache
#16 26.58 --- FAIL: TestRereviewNegativeControl122 (0.00s)
#16 26.58 FAIL sneak.berlin/go/dnswatcher/internal/config 0.069s
#16 ERROR: process "/bin/sh -c echo \"check epoch: ${CHECK_EPOCH}\" && make check" did not complete successfully: exit code: 2
```
Separately confirmed `script/cibuild exit=1`. File deleted, `git status` clean, tree green again on run 3. A cached layer cannot emit a failure predicted in advance — **definition-of-done item 1 is satisfied.**
## 5. Dependency layers stayed cached
On run 2, all `CACHED`: `apk add --no-cache git make gcc musl-dev binutils-gold`, `go install ...golangci-lint@c0d3ddc9...`, `go install ...goimports@009367f5...`, `WORKDIR /src`, `COPY go.mod go.sum ./`, `RUN go mod download`, `COPY . .`, plus all four runtime-stage layers. No toolchain reinstall, no module re-download. The `ARG` placement is right. **Item 2 satisfied.**
## 6. Scope discipline
`git diff f71ed12..ff66ecc` is exactly three hunks: the `Dockerfile` comment block (4 lines out, 7 in), the `TODO.md` sentence that quoted `$(date +%s)`, and one `script/cibuild` line. **No** `ARG` relocation, **no** removal of the `echo`, no restructuring, no `internal/` or test-file change, no `README.md` change. Prose plus one character, as scoped.
## 7. Hard constraints
- **Pins**: `golang@sha256:f6751d82...`, `alpine@sha256:c3f8e73f...`, golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0` — unchanged in both `Dockerfile` and `script/bootstrap`; neither file's pin lines appear in the diff.
- **`.golangci.yml`**: `sha256sum` = `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. Untouched.
- **No vendor/attribution references**: case-insensitive grep over the full diff, the commit message, and author/committer identity — zero hits. No `Co-Authored-By`, no session trailers.
- **DNS never mocked**: `git diff --name-status 9347a28..ff66ecc` = 4 files (`Dockerfile`, `README.md`, `TODO.md`, `script/cibuild`). No test file, no `internal/` file. No negative-control leftovers committed by anyone; my own planted file is deleted and the tree is clean.
- **POSIX `sh`**: `#!/bin/sh`, `set -eu`, `sh -n script/cibuild` clean, `ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"` idiom preserved, no bashisms.
- **Gate strength**: `make check` runs in full inside the container (test, lint, fmt-check). No `-short`, no skip flags, no lint-only narrowing. `script/test`, `script/check`, `script/lint`, `script/fmt-check` are untouched by this PR.
- **Commit hygiene**: one commit, title ends ` (closes #115)`, `TODO.md` updated in the same commit and consistent with the surrounding entry style.
- **CI**: `check / check (push)` = **success** on `ff66ecc`. The workflow (`.gitea/workflows/check.yml`) is a single `run: script/cibuild`, so CI exercises the fixed path.
- **Mergeable**: `ff66ecc` is a descendant of `origin/main` @ `9347a28` (re-fetched at end of review; `main` has not moved). No rebase needed.
- **Host `make check`**: exit 0, `0 issues.`, `make fmt-check` silent. **Not void** — grep of the lint output for `parallel golangci-lint is running`, any `../` path, and any absolute path outside this worktree returned nothing. **Zero void runs hit during this review.**
---
## Non-blocking
### N1. `%N` is not POSIX, and busybox silently drops it — the "never cached" guarantee is host-conditional
I tested three implementations rather than one:
- host, `date (uutils coreutils) 0.8.0`, under `/bin/sh` (dash): 19-digit nanosecond value. Works.
- the repo's pinned `alpine` image, busybox `date`: `date +%s%N` prints `1786257437` — `%N` is **silently dropped**, exit 0. Degrades to second granularity.
So on a busybox/POSIX-only host the epoch is second-granular and the concurrency guarantee the rework was meant to add does not hold there. That is **no worse than `f71ed12`**, which is why it is not blocking. The catastrophic case — a *constant* value — requires `date` to exit non-zero, and no implementation I could find fails on an unknown `strftime` conversion; they copy it through. Worth knowing that if it ever did fail, `set -eu` would **not** save you: I verified that in dash a failing command substitution inside a command's *arguments* does not trip `set -e` (`sh -c 'set -eu; echo "[$(false)]"; echo REACHED'` prints `[]`, then `REACHED`, exit 0). The build would then run with `CHECK_EPOCH=` constant-empty and serve a cached green, silently. Low likelihood, maximum blast radius.
`$(date +%s%N)$$` costs one more token, keeps nanoseconds where available, and makes concurrent-invocation uniqueness unconditional via the PID even where `%N` is dropped or a substitution returns empty. Recommended, not required.
Related: `script/cibuild`'s header comment and the new `README.md` line both state the guarantee unconditionally ("never served from the Docker layer cache", "always means the checks ran on this invocation"). Those are true on this host and on the `ubuntu-latest` runner, and the PR body honestly discloses the `%N` caveat, so this does not reach the bar that failed `f71ed12` — that claim was false *on this builder*. Flagging it so it is a deliberate acceptance rather than an oversight.
### N2. An unset `CHECK_EPOCH` defaults silently
`RUN echo "check epoch: ${CHECK_EPOCH}" && make check` with no `--build-arg` prints an empty value, emits no builder warning, and serves the cached check — the repo's known-rejected "silent defaulting" shape. Failing loudly on an empty value would fix `script/docker` and plain `docker build .` at the same time, but it would change behaviour the issue explicitly scoped out (*"a plain docker build without the argument caches exactly as before"*) and is already tracked as #124. Correct call to leave it; noting it belongs in #124's brief rather than being lost.
### N3. Could not corroborate CI-side behaviour from logs
The commit status on `ff66ecc` is green and the workflow does invoke `script/cibuild`, but the job log the API returned for that run is stale and unrelated (dated 2026-02-21, showing a native `go build` and pre-#93 DNS skips rather than a Docker build). So there is no log-level confirmation that `date +%s%N` expanded correctly on the runner or that the check layer executed there. Not attributable to this PR, and the green status plus a workflow that is one line is sufficient. Recorded so it is not mistaken for verified.
### N4. Deferred items, correctly deferred
`REPO_POLICIES.md` lines 62 and 170-172 still describe `script/cibuild` as `docker build .` and still assert *"a successful build implies all checks pass"* — the exact statement #115 falsified. Confirmed still present. Deferred upstream to `prompts` #26 as the org-canonical copy. Also unbounded builder-cache growth, and #124. All three are out of #115's scope and none is silently skipped.
### N5. Nit
The commit subject is 95 characters and becomes ~102 after the squash-merge `(#122)` suffix. No repo policy sets a limit, so this is preference only.
---
**Summary:** the rework does exactly what it was scoped to do and nothing else. The claim that failed `f71ed12` has been replaced with one I independently verified true under a nonce-protected design with liveness controls, and the correction reached the `Dockerfile`, the commit message and the PR body alike. Definition-of-done items 1-7 all satisfied and independently re-verified, including a third negative control. Constraints intact, CI green, mergeable, no void lint runs.
[manager] Fresh re-review — by a reviewer who had neither seen this PR before nor performed the rework — returned PASS, no blocking findings. Labeling merge-ready and assigning to @sneak.
The corrected claim is now verified, not merely asserted
This PR failed the first time because its explanation was asserted as reasoning while only its conclusion was proven. The rework could have repeated that mistake in the opposite direction, so the reviewer was asked to test the new wording experimentally. They did, twice, with a fresh nonce (ba7900b6) never used by any prior probe:
On a minimal context — ARG CHECK_EPOCH declared, final RUN never referencing it:
#
value
result
A
fresh
executed 3.6s
B
fresh
executed 0.2s — not CACHED
C
repeat of B
CACHED
D
repeat of A
CACHED
On this repo's real Dockerfile, line 36 temporarily reverted to bare RUN make check, byte-identical tree throughout:
#
value
COPY . .
RUN make check
A
fresh
executed
executed
B
fresh
CACHED
executed 31.4s — not CACHED
C
repeat of B
CACHED
CACHED
D
repeat of A
CACHED
CACHED
Row B of the second table is the strongest single piece of evidence produced in this whole investigation: COPY . . reports CACHED in the very same build where the check executes, which isolates the build argument as the only variable. C and D are the liveness proof that a miss was a genuine key miss rather than an empty cache. Experiment reverted, git diff HEAD empty, head still ff66ecc.
A third independent negative control also passed, with the reviewer's own predicted sentinel appearing verbatim in a failing build (exit 1), then a clean tree and a green build afterwards.
Grep confirms the old false wording is gone tree-wide — no hits for bare ARG, after expansion, or would still hit the cache in the Dockerfile, commit message, or PR body.
Two findings that outlive this PR
1. %N silently degrades on busybox. The reviewer checked the repo's own pinned alpine image: date +%s%N there prints 1786257437 — it drops %N silently and exits 0, degrading to second granularity. Not blocking, because that is exactly where f71ed12 already was and no worse. But it means the nanosecond guarantee is host-conditional, while script/cibuild's header comment and the README describe it unconditionally.
They also chased the catastrophic path properly rather than assuming: a constant CHECK_EPOCH would require date to exit non-zero, which no strftime implementation does for an unknown conversion — and they verified in dash that even if it did, set -eu would not catch a failing command substitution inside a command's arguments, so the build would silently get CHECK_EPOCH= empty and constant. That is the false green returning by a different door. $(date +%s%N)$$ makes the freshness unconditional for four characters. Filed as #125.
2. The CI log for this commit does not correspond to this commit. The reviewer could not corroborate CI-side behaviour: the job log Gitea's API returns for the green run on ff66ecc is dated 2026-02-21 and shows a native go build with pre-#93 DNS skips — a build that predates this branch entirely. So the green check mark on this PR is not evidence about this code.
That matters well beyond #122. "CI green" has been cited as corroborating evidence on every PR in this milestone. It now appears to be a fifth check that looks authoritative and is not — after script/cibuild (#115), script/bootstrap (#117), script/lint (#121), and script/docker (#124). Filed as #126; it needs your access, since the Actions API returns 403 for this account.
None of the merge-ready decisions in this milestone rested on CI. Every one was made on locally-reproduced evidence — negative controls, mutation tests, and repeated cache-bypassed runs — which is fortunate, and in hindsight was the right instinct rather than good luck.
Verified clean
All pins unchanged; .golangci.yml sha256 still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; no test or internal/ file touched; git diff f71ed12..ff66ecc is exactly prose plus one character; sh -n script/cibuild clean and still POSIX; title ends (closes #115); TODO.md in the same commit; no vendor references or attribution trailers; fast-forwards from origin/main9347a28.
Zero void lint runs — 0 issues., no parallel golangci-lint is running, no ../ or foreign absolute paths.
**[manager]** Fresh re-review — by a reviewer who had neither seen this PR before nor performed the rework — returned **PASS**, no blocking findings. Labeling `merge-ready` and assigning to @sneak.
## The corrected claim is now verified, not merely asserted
This PR failed the first time because its explanation was asserted as reasoning while only its conclusion was proven. The rework could have repeated that mistake in the opposite direction, so the reviewer was asked to test the *new* wording experimentally. They did, twice, with a fresh nonce (`ba7900b6`) never used by any prior probe:
**On a minimal context** — `ARG CHECK_EPOCH` declared, final `RUN` never referencing it:
| # | value | result |
|---|---|---|
| A | fresh | executed 3.6s |
| B | fresh | **executed 0.2s — not `CACHED`** |
| C | repeat of B | `CACHED` |
| D | repeat of A | `CACHED` |
**On this repo's real `Dockerfile`**, line 36 temporarily reverted to bare `RUN make check`, byte-identical tree throughout:
| # | value | `COPY . .` | `RUN make check` |
|---|---|---|---|
| A | fresh | executed | executed |
| B | fresh | **`CACHED`** | **executed 31.4s — not `CACHED`** |
| C | repeat of B | `CACHED` | `CACHED` |
| D | repeat of A | `CACHED` | `CACHED` |
Row B of the second table is the strongest single piece of evidence produced in this whole investigation: `COPY . .` reports `CACHED` in the very same build where the check executes, which isolates the build argument as the only variable. C and D are the liveness proof that a miss was a genuine key miss rather than an empty cache. Experiment reverted, `git diff HEAD` empty, head still `ff66ecc`.
A third independent negative control also passed, with the reviewer's own predicted sentinel appearing verbatim in a failing build (exit 1), then a clean tree and a green build afterwards.
Grep confirms the old false wording is gone tree-wide — no hits for `bare ARG`, `after expansion`, or `would still hit the cache` in the `Dockerfile`, commit message, or PR body.
## Two findings that outlive this PR
**1. `%N` silently degrades on busybox.** The reviewer checked the repo's own pinned alpine image: `date +%s%N` there prints `1786257437` — **it drops `%N` silently and exits 0**, degrading to second granularity. Not blocking, because that is exactly where `f71ed12` already was and no worse. But it means the nanosecond guarantee is host-conditional, while `script/cibuild`'s header comment and the README describe it unconditionally.
They also chased the catastrophic path properly rather than assuming: a constant `CHECK_EPOCH` would require `date` to exit non-zero, which no strftime implementation does for an unknown conversion — and they verified in dash that even if it did, `set -eu` would **not** catch a failing command substitution inside a command's arguments, so the build would silently get `CHECK_EPOCH=` empty and constant. That is the false green returning by a different door. `$(date +%s%N)$$` makes the freshness unconditional for four characters. Filed as **#125**.
**2. The CI log for this commit does not correspond to this commit.** The reviewer could not corroborate CI-side behaviour: the job log Gitea's API returns for the green run on `ff66ecc` is **dated 2026-02-21 and shows a native `go build` with pre-#93 DNS skips** — a build that predates this branch entirely. So the green check mark on this PR is not evidence about this code.
That matters well beyond #122. "CI green" has been cited as corroborating evidence on every PR in this milestone. It now appears to be a *fifth* check that looks authoritative and is not — after `script/cibuild` (#115), `script/bootstrap` (#117), `script/lint` (#121), and `script/docker` (#124). Filed as **#126**; it needs your access, since the Actions API returns 403 for this account.
None of the merge-ready decisions in this milestone rested on CI. Every one was made on locally-reproduced evidence — negative controls, mutation tests, and repeated cache-bypassed runs — which is fortunate, and in hindsight was the right instinct rather than good luck.
## Verified clean
All pins unchanged; `.golangci.yml` sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`; no test or `internal/` file touched; `git diff f71ed12..ff66ecc` is exactly prose plus one character; `sh -n script/cibuild` clean and still POSIX; title ends ` (closes #115)`; `TODO.md` in the same commit; no vendor references or attribution trailers; fast-forwards from `origin/main` `9347a28`.
Zero void lint runs — `0 issues.`, no `parallel golangci-lint is running`, no `../` or foreign absolute paths.
`script/cibuild` was plain `docker build .`. The Dockerfile does
`COPY . .` and then `RUN make check`, and Docker invalidates `COPY . .`
only on a content change, so on a byte-identical tree the check layer
was reused and the suite never ran. The script's header comment claimed
that a successful build implies all checks pass, which was false
whenever the cache was warm. Reproduced on this branch's parent: a
second consecutive run returned success in 283 ms with
`#13 [builder 9/10] RUN make check` reported `CACHED`.
That matters more here than in a typical repo. DNS is never mocked in
this repository, so the suite queries live DNS and its outcome varies
with real-world conditions; caching the verdict of a non-deterministic
check replays a stale result in exactly the case where re-running is
most valuable. It is also the gate every PR is verified through.
Fix: declare `ARG CHECK_EPOCH` immediately above the check step and
expand it into the command, with `script/cibuild` passing a fresh
`$(date +%s%N)` per invocation. A build argument's value participates in
the cache key of later instructions in the stage even when they do not
reference it, so a fresh value busts this layer either way; the value is
expanded into the command deliberately, which makes the invalidation a
property of the command string itself rather than of how a given builder
treats unreferenced args, and surfaces the epoch in the build log as a
diagnostic. Placing the ARG here and no earlier keeps the pinned
toolchain installs and `go mod download` above the invalidation line, so
only the check and the steps after it re-run. The epoch is nanosecond
granular so that two concurrent invocations starting in the same second
cannot share a value.
A plain `docker build` without the argument caches as before; nothing
outside the CI entrypoint changes behaviour.
Verified by experiment, not inspection:
- Two consecutive runs on an unchanged tree: 55.2 s and 42.2 s, both
exit 0, with distinct epochs. The second run shows
`RUN echo "check epoch: ..." && make check` executing for 36.0 s and
216 passing tests across all eight packages, while `apk add`, both
pinned `go install` steps, `go mod download`, `COPY go.mod go.sum` and
`COPY . .` all report `CACHED`.
- Negative control: planted `internal/config/zz_negative_control_test.go`
calling `t.Fatal("NEGATIVE-CONTROL-115: planted failure, cache did not
serve this layer")`. The build failed in 24.7 s with exit 1, printing
that exact message and `--- FAIL: TestNegativeControlIssue115`, and the
check step exited with code 2. A cached layer cannot produce a failure
predicted in advance, so this establishes the suite ran. The file was
then removed, `git status` confirmed clean, and the tree built green
again in 48.1 s.
- Total build time 42-55 s against the policy's 5-minute ceiling.
- `make check` green. No pin touched: the `golang` and `alpine` sha256
digests, golangci-lint `c0d3ddc9`, and goimports `009367f5` are
unchanged, and `.golangci.yml` still hashes to `021cc83f4e6f...`.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #115.
script/cibuildwas plaindocker build .. The Dockerfile doesCOPY . .and thenRUN make check, and Docker invalidatesCOPY . .only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed a successful build implies all checks pass — false whenever the cache was warm.Reproduced on this branch's parent (
9347a28) before changing anything: a second consecutivescript/cibuildreturned exit 0 in 283 ms, with#13 [builder 9/10] RUN make checkreportedCACHEDand every other layerCACHEDtoo.Fix
ARG CHECK_EPOCHimmediately above the check step, expanded into the command;script/cibuildpasses a fresh$(date +%s%N)per invocation.Why the value is expanded inside the
RUN. A build argument's value participates in the cache key of later instructions in the same stage even when those instructions never reference it — this was tested directly on this builder (Docker 29.7.2 / buildx 0.36.1 / BuildKit v0.32.2) by reverting to a bareRUN make checkwithARG CHECK_EPOCHdeclared but unused: changing only--build-argre-executed the layer, and repeating a previously-used value came backCACHED, proving the cache was live and the miss genuine. So a fresh value busts this layer either way. Expanding it into the command is nonetheless the deliberate choice: it makes the invalidation a property of the command string itself rather than of how a given builder version treats unreferenced args, and it surfaces the epoch in the build log as a diagnostic showing which run a layer belongs to.(An earlier revision of this PR asserted the opposite — that BuildKit keys on the expanded command string only, so a bare
ARGwould still hit the cache. That claim was empirically false on this builder and has been removed from theDockerfilecomment, the commit message and this description. The code was always correct; only the justification was wrong.)The epoch is nanosecond-granular so that two concurrent invocations starting within the same wall-clock second cannot produce an identical value and have the second served from the first's layer. Sequential runs could not collide (a build is ~45 s), but concurrent ones could.
Placing the
ARGhere and no earlier keeps the pinned toolchain installs andgo mod downloadabove the invalidation line. A plaindocker buildwithout the argument caches exactly as before; only the CI entrypoint changes behaviour.Verification — by experiment, not inspection
Per the refinement in the issue comment: timings and a negative control, neither alone.
1. Twice in a row on an unchanged tree
CACHEDRe-run after the
date +%s%Nchange, on an unchanged tree, both exit 0 and both executing the check step:1786256586666772611CACHED)1786256640154832161Distinct epochs, nothing sub-second, no
CACHEDon the check layer, andapk add, both pinnedgo installsteps,go mod download,COPY go.mod go.sumandCOPY . .all stillCACHED.2. Negative control (the conclusive evidence)
Planted
internal/config/zz_negative_control_test.go:script/cibuildfailed in 24.7 s with exit 1. The build printed exactly the predicted message:A cached layer cannot produce a failure predicted in advance, so the suite demonstrably ran. The file was then deleted,
git statusshowed only the intended modifications and no untracked leftovers, and the tree built green again in 48.1 s. An independent reviewer reproduced this with their own planted sentinel. Thedate +%s%Nchange does not alter the shape of the command string (the epoch is expanded by the builder, not the script), so the control was not re-run for it.3. Dependency layers stayed cached
In every post-fix run, all of these report
CACHED:apk add --no-cache git make gcc musl-dev binutils-gold, both pinnedgo installsteps (golangci-lint, goimports),WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download, andCOPY . .. Only the check step and the steps after it re-run. No module re-download, no toolchain reinstall.4. Build time vs the ceiling
42-55 s against the policy's 5-minute ceiling — roughly 15-18% of budget.
5.
make checkand script sanityGreen, exit 0,
0 issues.(run with an isolatedGOLANGCI_LINT_CACHEandGOFLAGS=-count=1; not void — noparallel golangci-lint is running, no paths outside the worktree).sh -n script/cibuildclean; the script stays POSIXshwithset -euand no bashisms.date +%s%Nwas verified to work in this environment (uutils coreutils 0.8.0) and returns a full nanosecond value, not a literal%N. It is a GNU/uutils extension rather than POSIX; if a future CI host lacks it, the fallback would be appending$$to a POSIXdate +%s.Constraints honoured
golang/alpinesha256digests, golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0all unchanged — theDockerfilediff adds only the comment block, theARG, and theechoprefix..golangci.ymluntouched; stillsha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.make checkstill runs in full. No-short, no skip flags, no narrowing to lint-only.script/cibuildstays POSIXshand keeps theROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom.TODO.mdupdated in the same commit. One commit; files staged by name.Scope notes for the reviewer
Dockerfilefor a fail-fastlintstage andARG VERSION. This PR adds only theARGand theechoprefix at the existing check step, so whichever lands second rebases trivially.README.mdEntrypoints entry updated: it describedscript/cibuildas "plaindocker build .", which this change falsifies. One line, no other README edits.script/dockerand plaindocker build .still serving a cached check (filed as #124); andREPO_POLICIES.mdlines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream withprompts#26 since that file is the org-canonical copy.[implementer] Summary and how to reproduce the evidence yourself.
One commit, four files, +38/-6.
Dockerfilegets a comment block,ARG CHECK_EPOCH, and anechoprefix on the existing check step;script/cibuildgains--build-arg CHECK_EPOCH="$(date +%s)"and a corrected header comment;README.mdgets one Entrypoints line;TODO.mdgets a Completed Steps entry in the same commit.The single thing worth checking closely is why
${CHECK_EPOCH}is expanded inside theRUNrather than just declared above it. Under BuildKit — the builder here, Docker 29.7.2 — a bareARGis not a layer and does not change the cache key of the instruction below it. WritingARG CHECK_EPOCHabove an unmodifiedRUN make checkwould look like the recommended fix from the issue while leaving the layerCACHEDand the bug fully intact. That failure mode is invisible to code review, which is exactly why the negative control matters here rather than being a formality.Reproducing the before state: check out
9347a28, runscript/cibuildtwice. The second returns exit 0 in about a quarter second withRUN make checkmarkedCACHED. I measured 283 ms.Reproducing the after state: on this branch, run
script/cibuildtwice. Both execute the suite (55.2 s and 42.2 s for me), print a differentcheck epoch:value each time, and show 216 passing tests — whileapk add, both pinnedgo installsteps,go mod downloadand bothCOPYlayers all stayCACHED. No modules re-downloaded, no toolchain reinstalled.Reproducing the negative control, which is the part I would re-run if I were reviewing this: drop a file in
internal/configwhose only test callst.Fatalwith a string you choose, runscript/cibuild, and confirm the build dies printing your string. Mine failed in 24.7 s with exit 1, surfacingNEGATIVE-CONTROL-115: planted failure, cache did not serve this layerand--- FAIL: TestNegativeControlIssue115, with the check step exiting 2. Deleting the file returned the tree to clean and the build to green in 48.1 s. The planted file is not in the commit.Build time lands at 42-55 s against the 5-minute ceiling.
make checkis green with0 issues.Two disclosures the reviewer should weigh:
docker buildonly throughscript/cibuild; there was no directdocker buildinvocation, including for the negative control.GOLANGCI_LINT_CACHE="$(mktemp -d)"in front of the make target. One run was void withError: parallel golangci-lint is running— a lock collision, not a lint result. Worth noting for whoever owns that guidance: the isolated cache directory did not prevent the collision, so the lock evidently does not live inGOLANGCI_LINT_CACHE. The retry was clean under both validity conditions.Two things I found and deliberately did not fix, to keep this confined to the issue and separately reviewable from #109:
REPO_POLICIES.mdnow describesscript/cibuildinaccurately in two places. It is the org-canonical copy, so the correction belongs upstream withprompts#26.script/dockerhas the identical caching hole. It is not the CI gate and it is byte-identical across repos, so it also wants a template fix rather than a local one. Say the word and I will file it.Nothing in the definition of done went unsatisfied.
[reviewer] Independent adversarial review of #122 @
f71ed12(basemain@9347a28)Verdict: FAIL —
needs-reworkThe fix works. I reproduced the cache defeat and confirmed it with my own negative control. One blocking finding, and it is not the code: the PR's headline technical justification — repeated verbatim in a permanent
Dockerfilecomment, the commit message, the PR body and the issue plan — is empirically false on this host. I tested it as instructed and it does not hold.Blocking
B1.
Dockerfilelines 23-26: the stated BuildKit mechanism is wrongThis says a bare
ARG CHECK_EPOCHabove an unchangedRUN make checkwould still be servedCACHED. It would not. On this builder (Docker 29.7.2 / buildx 0.36.1) the build-arg value participates in the cache key of subsequent instructions in the stage even when the instruction never references it.Experiment: I temporarily replaced line 33 with a bare
RUN make check, leavingARG CHECK_EPOCHin place, and ran four directdocker buildinvocations against a byte-identical tree, varying only--build-arg:--build-arg CHECK_EPOCHRUN make checkresult11111111112222222222CACHED)2222222222(repeat of B)CACHED1111111111(repeat of A)CACHEDB is the decisive cell: with the same tree, the same Dockerfile and an unreferenced
ARG, changing only the value busted the layer. C and D prove the cache was live and working the whole time, so B was a genuine key miss and not a cold cache. The recommended-in-#115 bare-ARGform would therefore have worked on its own.Why this is blocking rather than a nit:
echofor a reason that is fictitious and may reason from the false premise elsewhere. #109 is about to restructure this exact file.Acceptable looks like: keep the code exactly as it is — expanding the epoch into the
RUNis fine and has a real, honest benefit (it prints the epoch, making it visible in build output which run a layer belongs to). Reword lines 18-31 so the comment states what is true:script/cibuildpasses a freshCHECK_EPOCHper invocation; the build-arg value participates in this instruction's cache key, so the layer is re-executed every run; the value is echoed so the epoch is visible in the log. Drop the "BuildKit expands before keying, so a bare ARG would still hit the cache" claim. Correct the commit message and PR body to match. No functional change is needed.Non-blocking
N1.
script/cibuildline 15:date +%sis second-granularTwo invocations starting inside the same wall-clock second on an unchanged tree produce an identical
CHECK_EPOCH, and the second can be served from the first's layer. Sequential runs cannot collide (a build is ~45 s), so this is not the reported bug, but concurrent invocations (two CI jobs, a wrapper firing twice) can.date +%s%N, or appending$$, closes it at zero cost. Note the fix is only as strong as the freshness of this value — worth a comment either way.N2. Unbounded builder-cache growth
Every
script/cibuildrun mints a new check layer plus theRUN make buildlayer beneath it, none of which is ever reused. Correct by design, but the local builder cache now grows monotonically with cibuild invocations. Operational note only.N3. Plain
docker build .andscript/dockerstill serve a cached checkVerified: two consecutive plain
docker build .runs on the unchanged tree — the second reports#14 [builder 9/10] RUN echo "check epoch: ${CHECK_EPOCH}" && make checkasCACHED, with an emptycheck epoch:line on the first. This matches what the PR claims, and #115 scoped the fix to the CI entrypoint, so it is in-bounds and honestly documented. For the record: Docker emits no warning for the unsetARG, so the only signal is a barecheck epoch:with nothing after it.make docker/script/dockertherefore still produce a green that did not run the suite. The PR flags this rather than fixing it, which is the right call for scope; it should become an issue.N4.
REPO_POLICIES.mdnow contradicts the codeLines 62 and 170-172 still describe
script/cibuildas runningdocker build .and assert "a successful build implies all checks pass" — the exact false statement #115 was filed about. Deliberately deferred upstream by the author; defensible, but the repo carries a false claim in the meantime and it should be tracked, not just mentioned in a PR body.N5. Pre-existing, not this PR
level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0)"surfaces on every check run. Already accepted inTODO.md.Verification performed
All builds via
./script/cibuildexcept the bare-ARGexperiment (B1) and the no-build-arg experiment (N3), which used directdocker buildinvocations — stated explicitly. Nogotool run directly. Nothing was committed or pushed; working tree confirmed clean before and after every experiment.1. Twice in a row, unchanged tree
Four consecutive runs total, all on a byte-identical tree, all exit 0, all executing the check step:
Distinct epoch every run, no
CACHEDon the check layer, nothing close to sub-second. 216 test results across all eight packages,0 issues.from lint inside the container.2. My own negative control (not the author's)
Planted
internal/config/zz_reviewer_negctl_test.gowith a singlet.Fatalcarrying a string I chose in advance, then ranscript/cibuild. Exit 1, verbatim:A cached layer cannot emit a failure predicted in advance. The suite demonstrably ran. File deleted,
git statusclean, tree green again on the next run. Definition-of-done item 1 is satisfied.3. Dependency layers stayed cached
Confirmed
CACHEDon every post-fix run:apk add --no-cache git make gcc musl-dev binutils-gold,go install .../golangci-lint@c0d3ddc9...,go install .../goimports@009367f5...,WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download,COPY . .. No module re-download, no toolchain reinstall. TheARGplacement is correct. Item 2 satisfied.4. Build time
48-53 s against the 5-minute ceiling in
REPO_POLICIES.mdline 231. Roughly 16-18% of budget. Item 6 satisfied.5. Other checks
check / check (push)= success in 58 s onf71ed12. Green. The workflow runsscript/cibuild, so CI now exercises the fixed path.f71ed12is a descendant oforigin/main@9347a28. No rebase needed.make check: exit 0,0 issues., run with an isolatedGOLANGCI_LINT_CACHE. Not void — zero occurrences ofparallel golangci-lint is running, zero file paths outside this worktree. One valid run, no voids hit.golang@sha256:f6751d82...,alpine@sha256:c3f8e73f..., golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0— all unchanged inDockerfileandscript/bootstrap..golangci.yml:sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. Unmodified.make checkruns in full viascript/check(test, then lint, then fmt-check). No-short, no skip flags, no lint-only narrowing. Strictly stricter.git diff --name-status 9347a28..f71ed12= 4 files,internal/untouched. No negative-control leftovers in the commit (mine or the author's).#!/bin/sh,set -eu,sh -nclean,ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom preserved,$(date +%s)is POSIX, no bashisms.ARG CHECK_EPOCH, and theechoprefix. #109 rebases trivially.README.mdandTODO.mdedits are in-scope and correct.(closes #115),TODO.mdin the same commit, markdown wrapped consistently with surrounding prose. No forbidden attribution trailers or vendor references anywhere in the diff or commit message. Terminology clean.Summary: definition-of-done items 1-7 are all functionally met and independently verified. The single reason this is
needs-reworkrather thanmerge-readyis B1 — a factually incorrect explanation of builder behaviour, committed as a permanent comment and as the PR's central argument, in a repo where exactly that class of false claim is what produced this issue. Correcting prose is all that is required; the code should not change.[manager] Independent adversarial review returned FAIL. Relabeled
needs-rework, staying assigned toclawbot. Dispatching a prose-only rework.The fix is correct and stays. The explanation committed alongside it is false and must go.
Two independent experiments agree, and both contradict the PR
The reviewer reverted the
RUNto the bare form (leavingARG CHECK_EPOCHdeclared but unreferenced) and ran four builds on a byte-identical tree, varying only--build-arg:11111111112222222222CACHED2222222222CACHED1111111111CACHEDB is the decisive one; C and D prove the cache was live throughout, so B was a genuine key miss rather than an absent cache.
I ran my own probe before the review returned, on a minimal two-variant Dockerfile using this repo's pinned alpine digest. Same result: a declared-but-unreferenced
ARGdoes enter the cache key, and a changed value forces re-execution (DONE 0.3s, notCACHED).So the comment at
Dockerfilelines 23-26 — that BuildKit "derives each instruction's cache key from the command string after expansion, so a bare ARG above an unchanged RUN would still hit the cache" — is empirically false on this builder (Docker 29.7.2, buildx v0.36.1, BuildKit v0.32.2,DOCKER_BUILDKITunset, no# syntax=line). The bare form recommended in #115 would have worked on its own.Why this blocks, when the fix demonstrably works
Because the false claim is committed as a permanent
Dockerfilecomment, and because this PR is the reference implementation other repos are copying. Two sibling repos independently measured the opposite behaviour, and this claim was the sole reason propagation of the shared-template fix was halted while three repos waited.A correct fix carrying an incorrect justification is more dangerous than a plain error: the next person to touch this will reason from the comment. Merging it would embed a falsehood at the exact point where someone might later "simplify" the expanded form back to bare — and be misled about why they shouldn't.
A methodological note worth keeping
My first probe attempt returned "both
CACHED" and would have confirmed the PR's claim. It was wrong — an earlier run of my own probe had already populated the cache for those exact command strings. I only caught it because the result was too convenient. The corrected probe embeds a unique per-run nonce so no prior entry can match.That is the same trap as the original bug, one level up: a cache experiment can itself be served from cache. Anyone re-testing this without a nonce is measuring their own history.
What the author got right, and it is most of it
The negative control was rigorous and real, and the reviewer reproduced it independently with their own planted sentinel (
REVIEWER-NEGCTL-122, build exit 1, exact predicted string in the output, tree clean afterwards). Definition-of-done item 1 is satisfied. Four consecutivescript/cibuildruns on an unchanged tree all executed the check step with distinct epochs — 48.36s and 53.36s wall on runs 3 and 4, nothing sub-second, noCACHEDon the check layer. Dependency layers all stayedCACHED, so theARGplacement is right. 48-53s against the 5-minute ceiling.The failure was narrow and specific: the author asserted the mechanism as reasoning while proving only the conclusion. Their negative control tested the expanded form; it never tested the bare form.
Rework scope — prose only
Reword the
Dockerfilecomment block, the commit message, and the PR body to state that the build-arg value participates in the cache key of subsequent instructions even when they do not reference it, and that the expanded form is chosen deliberately — because it is correct regardless of how BuildKit treats unreferenced args, and because it surfaces the epoch in build logs as a diagnostic. No code change; theechostays.Also worth fixing while in there:
date +%sis second-granular, so two concurrent invocations in the same second produce identical epochs and the later one could be served from cache — the original bug, narrowly. Builds take 40s+ so sequential runs cannot collide, but concurrent ones can.date +%s%Nremoves the concern for one character.Deferred to follow-ups, not this PR: unbounded builder-cache growth;
script/dockerand plaindocker build .still serving a cached check (already filed as #124); andREPO_POLICIES.mdlines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream withprompts#26 since that file is the org-canonical copy.A fresh reviewer will re-review the amended head.
[implementer] Rework of
f71ed12->ff66ecc. Prose plus one character. No functional change to the mechanism; theARGplacement and theechoare untouched.B1 (blocking) — false BuildKit claim: fixed, and the finding is accepted without reservation
The reviewer is right and my original justification was wrong. I asserted a mechanism while my negative control only ever established the conclusion — it exercised the expanded form and never once tested the bare form, so it could not have detected the error. Two independent experiments (the reviewer's four-build A/B/C/D matrix, and the manager's nonce-bearing minimal probe) both show that a declared-but-unreferenced
ARGdoes enter the cache key of later instructions in the stage, and that changing its value forces re-execution. Cells C and D settle it: the cache was live throughout, so B was a genuine key miss.Dockerfilelines 23-26 now read (replacing the four lines that carried the false claim):The surrounding comment (the
CHECK_EPOCHpurpose sentence above, theARG-placement paragraph below) is unchanged. The commit message carries the identical correction in itsFix:paragraph, and the PR body has been rewritten — including an explicit note that the earlier revision asserted the opposite and why that was wrong, so the record does not just quietly change.N1 (non-blocking) —
date +%sgranularity: fixedscript/cibuildline 15 now passes$(date +%s%N). Sequential runs never could collide at ~45 s per build; concurrent invocations within one wall-clock second could, and that is the original bug in miniature.Portability, since
%Nis a GNU/uutils extension and not POSIX: verified working here —date (uutils coreutils) 0.8.0returns a full 19-digit value, and it behaves identically under/bin/sh, not just the interactive shell. No literal%Nanywhere. Had it not worked, the fallback would have been$(date +%s)$$, which is POSIX and still collision-free for concurrent invocations.sh -n script/cibuildis clean; still#!/bin/sh,set -eu, no bashisms,ROOTidiom preserved.The change does not alter the shape of the command string — the epoch is substituted by the builder, not the script — so per the rework brief I did not repeat the negative control. Two independent ones already passed against this exact
RUN.Verification of the reworked head
script/cibuildtwice in a row, unchanged tree, after the%Nchange:1786256586666772611CACHED1786256640154832161Distinct nanosecond epochs, both printed in the build log, nothing remotely sub-second, no
CACHEDon the check layer either time. Layers 9-14 (apk add, both pinnedgo installsteps,WORKDIR,COPY go.mod go.sum,go mod download) plusCOPY . .allCACHEDon both runs — no toolchain reinstall, no module re-download. Gate unweakened: fullmake checkinside the container.Host
make check: exit 0,0 issues., run asGOLANGCI_LINT_CACHE=<isolated dir> GOFLAGS=-count=1 make check. Not void — noparallel golangci-lint is running, and no reported path outside this worktree. I hit zero void runs during this rework, one clean run on the first attempt. Thegomodguarddeprecation warning (N5) still surfaces; already tracked, untouched.Untouched, as required: no pin moved (
golang/alpinedigests, golangci-lintc0d3ddc9..., goimports009367f5...);.golangci.ymlstill021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; nointernal/or test file changed; no negative-control leftovers; DNS still never mocked.git statusclean after both builds. Files staged by name —Dockerfile,TODO.md,script/cibuild.TODO.mdsits in the same commit and its Completed Steps entry was updated to say$(date +%s%N).The commit was amended rather than followed up, so the head is still a single commit whose title ends
(closes #115), force-pushed tofix/115-cibuild-cache. No new PR, no new branch. Diff against base is now 4 files, +41/-6.N2, N3, N4 — acknowledged, deliberately not addressed here
Builder-cache growth is an operational note;
script/dockerand plaindocker build .still serving a cached check is filed as #124;REPO_POLICIES.mdlines 62 and 170-172 belong upstream withprompts#26 since that file is the org-canonical copy. All three are out of #115's scope and none is silently skipped.Label left
needs-rework, assignee leftclawbot, for a fresh reviewer to re-reviewff66ecc.[reviewer] Independent adversarial re-review of #122 @
ff66ecc(basemain@9347a28; prior headf71ed12)Verdict: PASS
I did not author or rework this change and did not see it before this review. I re-tested the corrected claim from scratch rather than accepting the earlier experiments, using a per-run nonce so no result could be served from my own probe history. The new wording is true on this builder. No blocking findings. Four non-blocking items below, one of which I considered blocking and explain why I did not.
1. The corrected
Dockerfileclaim — verified, twice, with liveness controlsThe claim under test (
Dockerfilelines 23-26): "A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way."Builder: Docker 29.7.2, buildx v0.36.1. Both experiments used direct
docker build— stated explicitly, as required. Nonce for this review:ba7900b69c439a99, never used by any earlier run.Experiment A — minimal context, nonce embedded in every command string (so no pre-existing cache entry can match). Dockerfile: pinned
alpinedigest, a baseRUN, thenARG CHECK_EPOCH, then a finalRUNthat never references the arg.--build-arg CHECK_EPOCHRUN...-a(fresh)...-b(fresh)CACHED...-b(repeat)...-a(repeat)Experiment B — the repo's real
Dockerfile, with line 36 temporarily reverted to a bareRUN make checkandARG CHECK_EPOCHleft declared-but-unreferenced, byte-identical tree across all four builds:--build-arg CHECK_EPOCHCOPY . .RUN make check...-A(fresh)...-B(fresh)CACHED...-B(repeat)...-A(repeat)Cell B is decisive in both: identical tree, identical command string, identical cached parent, only the unreferenced arg value changed, and the layer re-executed. Cells C and D are the liveness proof — repeating a previously-seen value comes back
CACHED, so the cache was live throughout and B was a genuine key miss, not an empty cache. In Experiment B theCOPY . .above it reportsCACHEDin the same build thatRUN make checkexecutes, which isolates the arg as the only variable.Conclusion: the reworked wording is correct, and the earlier wording it replaced was false. The second sentence — that the expanded form is chosen so invalidation is a property of the command string rather than of how a builder treats unreferenced args — is a hedge, not a mechanism claim, and is sound. B1 is resolved.
Experiment reverted;
git statusclean,HEADstillff66ecc,Dockerfilerestored byte-for-byte (git diff HEADempty).2. The corrected claim propagated everywhere
grepforbare ARG,after expansion,would still hit the cacheacross the tree: no hits. TheFix:paragraph of the commit message (git log 9347a28..ff66ecc --format=%B) carries the corrected claim verbatim. The PR body carries it plus an explicit erratum recording that the earlier revision asserted the opposite. The old wording survives nowhere.3.
script/cibuildtwice in a row on an unchanged tree1786257099551809952DONE 31.6s, notCACHED17862572408130483111786257361506593276Distinct epoch every run, printed in the build log, nothing remotely sub-second, never
CACHED.Run 1's 2 m 12 s is my doing, not the PR's: my Experiment B had left the builder holding a different
Dockerfilecontent, soCOPY . .and everything below it rebuilt. Runs 2 and 3 are the representative figures. Steady-state build time 34-44 s against the 5-minute ceiling (REPO_POLICIES.mdline 231) — roughly 11-15% of budget.4. My own negative control (third independent one)
Planted
internal/config/zz_rereviewer_negctl_test.gowith a singlet.Fatalcarrying a sentinel I chose in advance.script/cibuildoutput, verbatim:Separately confirmed
script/cibuild exit=1. File deleted,git statusclean, tree green again on run 3. A cached layer cannot emit a failure predicted in advance — definition-of-done item 1 is satisfied.5. Dependency layers stayed cached
On run 2, all
CACHED:apk add --no-cache git make gcc musl-dev binutils-gold,go install ...golangci-lint@c0d3ddc9...,go install ...goimports@009367f5...,WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download,COPY . ., plus all four runtime-stage layers. No toolchain reinstall, no module re-download. TheARGplacement is right. Item 2 satisfied.6. Scope discipline
git diff f71ed12..ff66eccis exactly three hunks: theDockerfilecomment block (4 lines out, 7 in), theTODO.mdsentence that quoted$(date +%s), and onescript/cibuildline. NoARGrelocation, no removal of theecho, no restructuring, nointernal/or test-file change, noREADME.mdchange. Prose plus one character, as scoped.7. Hard constraints
golang@sha256:f6751d82...,alpine@sha256:c3f8e73f..., golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0— unchanged in bothDockerfileandscript/bootstrap; neither file's pin lines appear in the diff..golangci.yml:sha256sum=021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. Untouched.Co-Authored-By, no session trailers.git diff --name-status 9347a28..ff66ecc= 4 files (Dockerfile,README.md,TODO.md,script/cibuild). No test file, nointernal/file. No negative-control leftovers committed by anyone; my own planted file is deleted and the tree is clean.sh:#!/bin/sh,set -eu,sh -n script/cibuildclean,ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom preserved, no bashisms.make checkruns in full inside the container (test, lint, fmt-check). No-short, no skip flags, no lint-only narrowing.script/test,script/check,script/lint,script/fmt-checkare untouched by this PR.(closes #115),TODO.mdupdated in the same commit and consistent with the surrounding entry style.check / check (push)= success onff66ecc. The workflow (.gitea/workflows/check.yml) is a singlerun: script/cibuild, so CI exercises the fixed path.ff66eccis a descendant oforigin/main@9347a28(re-fetched at end of review;mainhas not moved). No rebase needed.make check: exit 0,0 issues.,make fmt-checksilent. Not void — grep of the lint output forparallel golangci-lint is running, any../path, and any absolute path outside this worktree returned nothing. Zero void runs hit during this review.Non-blocking
N1.
%Nis not POSIX, and busybox silently drops it — the "never cached" guarantee is host-conditionalI tested three implementations rather than one:
date (uutils coreutils) 0.8.0, under/bin/sh(dash): 19-digit nanosecond value. Works.alpineimage, busyboxdate:date +%s%Nprints1786257437—%Nis silently dropped, exit 0. Degrades to second granularity.So on a busybox/POSIX-only host the epoch is second-granular and the concurrency guarantee the rework was meant to add does not hold there. That is no worse than
f71ed12, which is why it is not blocking. The catastrophic case — a constant value — requiresdateto exit non-zero, and no implementation I could find fails on an unknownstrftimeconversion; they copy it through. Worth knowing that if it ever did fail,set -euwould not save you: I verified that in dash a failing command substitution inside a command's arguments does not tripset -e(sh -c 'set -eu; echo "[$(false)]"; echo REACHED'prints[], thenREACHED, exit 0). The build would then run withCHECK_EPOCH=constant-empty and serve a cached green, silently. Low likelihood, maximum blast radius.$(date +%s%N)$$costs one more token, keeps nanoseconds where available, and makes concurrent-invocation uniqueness unconditional via the PID even where%Nis dropped or a substitution returns empty. Recommended, not required.Related:
script/cibuild's header comment and the newREADME.mdline both state the guarantee unconditionally ("never served from the Docker layer cache", "always means the checks ran on this invocation"). Those are true on this host and on theubuntu-latestrunner, and the PR body honestly discloses the%Ncaveat, so this does not reach the bar that failedf71ed12— that claim was false on this builder. Flagging it so it is a deliberate acceptance rather than an oversight.N2. An unset
CHECK_EPOCHdefaults silentlyRUN echo "check epoch: ${CHECK_EPOCH}" && make checkwith no--build-argprints an empty value, emits no builder warning, and serves the cached check — the repo's known-rejected "silent defaulting" shape. Failing loudly on an empty value would fixscript/dockerand plaindocker build .at the same time, but it would change behaviour the issue explicitly scoped out ("a plain docker build without the argument caches exactly as before") and is already tracked as #124. Correct call to leave it; noting it belongs in #124's brief rather than being lost.N3. Could not corroborate CI-side behaviour from logs
The commit status on
ff66eccis green and the workflow does invokescript/cibuild, but the job log the API returned for that run is stale and unrelated (dated 2026-02-21, showing a nativego buildand pre-#93 DNS skips rather than a Docker build). So there is no log-level confirmation thatdate +%s%Nexpanded correctly on the runner or that the check layer executed there. Not attributable to this PR, and the green status plus a workflow that is one line is sufficient. Recorded so it is not mistaken for verified.N4. Deferred items, correctly deferred
REPO_POLICIES.mdlines 62 and 170-172 still describescript/cibuildasdocker build .and still assert "a successful build implies all checks pass" — the exact statement #115 falsified. Confirmed still present. Deferred upstream toprompts#26 as the org-canonical copy. Also unbounded builder-cache growth, and #124. All three are out of #115's scope and none is silently skipped.N5. Nit
The commit subject is 95 characters and becomes ~102 after the squash-merge
(#122)suffix. No repo policy sets a limit, so this is preference only.Summary: the rework does exactly what it was scoped to do and nothing else. The claim that failed
f71ed12has been replaced with one I independently verified true under a nonce-protected design with liveness controls, and the correction reached theDockerfile, the commit message and the PR body alike. Definition-of-done items 1-7 all satisfied and independently re-verified, including a third negative control. Constraints intact, CI green, mergeable, no void lint runs.[manager] Fresh re-review — by a reviewer who had neither seen this PR before nor performed the rework — returned PASS, no blocking findings. Labeling
merge-readyand assigning to @sneak.The corrected claim is now verified, not merely asserted
This PR failed the first time because its explanation was asserted as reasoning while only its conclusion was proven. The rework could have repeated that mistake in the opposite direction, so the reviewer was asked to test the new wording experimentally. They did, twice, with a fresh nonce (
ba7900b6) never used by any prior probe:On a minimal context —
ARG CHECK_EPOCHdeclared, finalRUNnever referencing it:CACHEDCACHEDCACHEDOn this repo's real
Dockerfile, line 36 temporarily reverted to bareRUN make check, byte-identical tree throughout:COPY . .RUN make checkCACHEDCACHEDCACHEDCACHEDCACHEDCACHEDRow B of the second table is the strongest single piece of evidence produced in this whole investigation:
COPY . .reportsCACHEDin the very same build where the check executes, which isolates the build argument as the only variable. C and D are the liveness proof that a miss was a genuine key miss rather than an empty cache. Experiment reverted,git diff HEADempty, head stillff66ecc.A third independent negative control also passed, with the reviewer's own predicted sentinel appearing verbatim in a failing build (exit 1), then a clean tree and a green build afterwards.
Grep confirms the old false wording is gone tree-wide — no hits for
bare ARG,after expansion, orwould still hit the cachein theDockerfile, commit message, or PR body.Two findings that outlive this PR
1.
%Nsilently degrades on busybox. The reviewer checked the repo's own pinned alpine image:date +%s%Nthere prints1786257437— it drops%Nsilently and exits 0, degrading to second granularity. Not blocking, because that is exactly wheref71ed12already was and no worse. But it means the nanosecond guarantee is host-conditional, whilescript/cibuild's header comment and the README describe it unconditionally.They also chased the catastrophic path properly rather than assuming: a constant
CHECK_EPOCHwould requiredateto exit non-zero, which no strftime implementation does for an unknown conversion — and they verified in dash that even if it did,set -euwould not catch a failing command substitution inside a command's arguments, so the build would silently getCHECK_EPOCH=empty and constant. That is the false green returning by a different door.$(date +%s%N)$$makes the freshness unconditional for four characters. Filed as #125.2. The CI log for this commit does not correspond to this commit. The reviewer could not corroborate CI-side behaviour: the job log Gitea's API returns for the green run on
ff66eccis dated 2026-02-21 and shows a nativego buildwith pre-#93 DNS skips — a build that predates this branch entirely. So the green check mark on this PR is not evidence about this code.That matters well beyond #122. "CI green" has been cited as corroborating evidence on every PR in this milestone. It now appears to be a fifth check that looks authoritative and is not — after
script/cibuild(#115),script/bootstrap(#117),script/lint(#121), andscript/docker(#124). Filed as #126; it needs your access, since the Actions API returns 403 for this account.None of the merge-ready decisions in this milestone rested on CI. Every one was made on locally-reproduced evidence — negative controls, mutation tests, and repeated cache-bypassed runs — which is fortunate, and in hindsight was the right instinct rather than good luck.
Verified clean
All pins unchanged;
.golangci.ymlsha256 still021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; no test orinternal/file touched;git diff f71ed12..ff66eccis exactly prose plus one character;sh -n script/cibuildclean and still POSIX; title ends(closes #115);TODO.mdin the same commit; no vendor references or attribution trailers; fast-forwards fromorigin/main9347a28.Zero void lint runs —
0 issues., noparallel golangci-lint is running, no../or foreign absolute paths.`script/cibuild` was plain `docker build .`. The Dockerfile does `COPY . .` and then `RUN make check`, and Docker invalidates `COPY . .` only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed that a successful build implies all checks pass, which was false whenever the cache was warm. Reproduced on this branch's parent: a second consecutive run returned success in 283 ms with `#13 [builder 9/10] RUN make check` reported `CACHED`. That matters more here than in a typical repo. DNS is never mocked in this repository, so the suite queries live DNS and its outcome varies with real-world conditions; caching the verdict of a non-deterministic check replays a stale result in exactly the case where re-running is most valuable. It is also the gate every PR is verified through. Fix: declare `ARG CHECK_EPOCH` immediately above the check step and expand it into the command, with `script/cibuild` passing a fresh `$(date +%s%N)` per invocation. A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way; the value is expanded into the command deliberately, which makes the invalidation a property of the command string itself rather than of how a given builder treats unreferenced args, and surfaces the epoch in the build log as a diagnostic. Placing the ARG here and no earlier keeps the pinned toolchain installs and `go mod download` above the invalidation line, so only the check and the steps after it re-run. The epoch is nanosecond granular so that two concurrent invocations starting in the same second cannot share a value. A plain `docker build` without the argument caches as before; nothing outside the CI entrypoint changes behaviour. Verified by experiment, not inspection: - Two consecutive runs on an unchanged tree: 55.2 s and 42.2 s, both exit 0, with distinct epochs. The second run shows `RUN echo "check epoch: ..." && make check` executing for 36.0 s and 216 passing tests across all eight packages, while `apk add`, both pinned `go install` steps, `go mod download`, `COPY go.mod go.sum` and `COPY . .` all report `CACHED`. - Negative control: planted `internal/config/zz_negative_control_test.go` calling `t.Fatal("NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer")`. The build failed in 24.7 s with exit 1, printing that exact message and `--- FAIL: TestNegativeControlIssue115`, and the check step exited with code 2. A cached layer cannot produce a failure predicted in advance, so this establishes the suite ran. The file was then removed, `git status` confirmed clean, and the tree built green again in 48.1 s. - Total build time 42-55 s against the policy's 5-minute ceiling. - `make check` green. No pin touched: the `golang` and `alpine` sha256 digests, golangci-lint `c0d3ddc9`, and goimports `009367f5` are unchanged, and `.golangci.yml` still hashes to `021cc83f4e6f...`.clawbot referenced this pull request2026-09-04 00:10:40 +02:00
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.