Fixes the third false-green defect in this repo's gate: script/cibuild
exited 0 on an unchanged tree without executing the checks at all.
Reworked at 09dbe6f against the review's B1/B2/B3/S1/S2. One commit,
four files.
What changed
Dockerfile — ARG CHECK_EPOCH immediately above the check RUNs in both stages: above RUN make fmt-check / RUN make lint in the
lint stage, and above RUN make test in the builder stage. ARG
scope is per-stage in Docker, so each stage declares its own; covering
only one would leave half the gate fake. Every Dockerfile
instruction line is byte-identical to 24f6e2f — the rework changed
comment text only.
script/cibuild — the epoch is assigned before use, so a failing date aborts the script instead of silently yielding an empty
constant:
The inaccurate header comment ("the Dockerfile runs script/check via
make check") is corrected to name what actually runs.
README.md — documents the guarantee as the conditional one it is,
and says to gate through script/cibuild rather than docker build.
TODO.md — updated in the same commit per the Workflow section.
Placement is the substance of the change: the ARG sits below the apk add, COPY go.mod go.sum, and go mod download layers in both
stages. Earlier and every build is cold; later and the checks stay
cached. The reviewer confirmed that placement by measurement and it was
not touched in the rework.
Evidence
All measurements live in one place — the verification comment below,
taken from 09dbe6f. They are deliberately not restated in the
commit message or TODO.md, which reference that comment instead, so
there is a single record that cannot disagree with itself (review S1).
Not touched
.golangci.yml — sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
verified unchanged after the rework.
The lint-stage FROM line and its digest — still the single source of
truth for the linter version (#78).
script/lint, including the native escape hatch (#78/#80/#88).
script/docker — it builds an image, it is not the gate. Its
identical cache hole is filed as part of #91.
.gitea/workflows/check.yml — unchanged; its only step is script/cibuild.
The DockerfileARG placement and every other Dockerfile
instruction.
Noted, not fixed here
#90 — the gomodguard deprecation, which would require editing .golangci.yml.
#91 — the remaining upstream sneak/prompts#26 hardening this PR
deliberately does not adopt: the expanded ARG form, a [ -n "$CHECK_EPOCH" ] || exit 1 guard so a bare docker build .
fails loudly, a per-invocation epoch (date +%s%N plus $$, since
busybox silently drops %N), and script/docker.
Fixes the third false-green defect in this repo's gate: `script/cibuild`
exited 0 on an unchanged tree without executing the checks at all.
Reworked at `09dbe6f` against the review's B1/B2/B3/S1/S2. One commit,
four files.
## What changed
* `Dockerfile` — `ARG CHECK_EPOCH` immediately above the check `RUN`s in
**both** stages: above `RUN make fmt-check` / `RUN make lint` in the
lint stage, and above `RUN make test` in the builder stage. `ARG`
scope is per-stage in Docker, so each stage declares its own; covering
only one would leave half the gate fake. **Every `Dockerfile`
instruction line is byte-identical to `24f6e2f`** — the rework changed
comment text only.
* `script/cibuild` — the epoch is assigned before use, so a failing
`date` aborts the script instead of silently yielding an empty
constant:
```sh
epoch="$(date +%s)"
docker build --build-arg CHECK_EPOCH="$epoch" .
```
The inaccurate header comment ("the Dockerfile runs script/check via
make check") is corrected to name what actually runs.
* `README.md` — documents the guarantee as the conditional one it is,
and says to gate through `script/cibuild` rather than `docker build`.
* `TODO.md` — updated in the same commit per the Workflow section.
Placement is the substance of the change: the `ARG` sits **below** the
`apk add`, `COPY go.mod go.sum`, and `go mod download` layers in both
stages. Earlier and every build is cold; later and the checks stay
cached. The reviewer confirmed that placement by measurement and it was
not touched in the rework.
## Evidence
**All measurements live in one place — the verification comment below,
taken from `09dbe6f`.** They are deliberately not restated in the
commit message or `TODO.md`, which reference that comment instead, so
there is a single record that cannot disagree with itself (review S1).
## Not touched
* `.golangci.yml` — sha256
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`,
verified unchanged after the rework.
* The lint-stage `FROM` line and its digest — still the single source of
truth for the linter version (#78).
* `script/lint`, including the native escape hatch (#78/#80/#88).
* `script/docker` — it builds an image, it is not the gate. Its
identical cache hole is filed as part of #91.
* `.gitea/workflows/check.yml` — unchanged; its only step is
`script/cibuild`.
* The `Dockerfile` `ARG` placement and every other `Dockerfile`
instruction.
## Noted, not fixed here
* #90 — the `gomodguard` deprecation, which would require editing
`.golangci.yml`.
* #91 — the remaining upstream `sneak/prompts` #26 hardening this PR
deliberately does not adopt: the expanded `ARG` form, a
`[ -n "$CHECK_EPOCH" ] || exit 1` guard so a bare `docker build .`
fails loudly, a per-invocation epoch (`date +%s%N` plus `$$`, since
busybox silently drops `%N`), and `script/docker`.
script/cibuild was a bare `docker build .` with no cache control. The
Dockerfile does `COPY . .` and then `RUN make fmt-check` / `RUN make
lint` in the lint stage and `COPY . .` / `RUN make test` in the builder
stage. On an unchanged tree Docker served those RUN layers from cache,
so the checks never executed, and the build still exited 0 -- the exit
code, which is the one signal automation trusts, was wrong, and wrong
in the direction that matters: the longer a branch sits unchanged, the
more likely its "verification" is a replay, which is exactly its state
just before a merge.
Reproduced on this branch's base at 3bcdbcf. A changed-tree run took
162132ms and produced 14 `ok` lines and `0 issues.`; the immediately
following run, with nothing touched, took 221ms and produced 0 `ok`
lines and no `0 issues.` line at all, with 19 CACHED layers including
`RUN make fmt-check`, `RUN make lint`, and `RUN make test`. Both
exited 0.
The fix matches the upstream one in sneak/prompts #26 rather than
inventing a local variant: an `ARG CHECK_EPOCH` declared immediately
above the check RUNs, with script/cibuild passing a fresh
`--build-arg CHECK_EPOCH="$(date +%s)"` on every invocation. ARG scope
is per-stage in Docker, so the lint stage and the builder stage each
declare their own; covering only one would leave half the gate fake.
Placement is the substance of the change. The ARG sits below the
`apk add`, `COPY go.mod go.sum`, and `go mod download` layers in both
stages, so only the check layers are invalidated: earlier and every
build would be cold, later and the checks would stay cached. Confirmed
by measurement -- on a post-fix build every `apk add` and `go mod
download` layer is still reported CACHED, and a changed-tree build went
from 162132ms to 176221ms rather than to a cold build's 242727ms.
Verified against the original failure mode, not by trusting an exit
code: two back-to-back script/cibuild runs on an unchanged tree now
take 166745ms and 174025ms, each with 14 `ok` lines and `0 issues.`,
and neither reports CACHED on any of the three check layers.
Cross-checked host-side with `GOFLAGS=-count=1 make check`: exit 0, 14
`ok` lines, `0 issues.`, with no `parallel golangci-lint is running`
and no file paths from outside this worktree, so the lint result is a
real one and not a void or contaminated run.
.golangci.yml is unchanged (sha256 021cc83f4e6f...643346bcb), as is the
lint-stage FROM line that is the single source of truth for the linter
version, script/lint's pinned-image logic, and
.gitea/workflows/check.yml, whose only step is script/cibuild.
One commit, 24f6e2f, four files: Dockerfile, script/cibuild, README.md, TODO.md. No Go code changed, so there is no new test —
the artifact under test is the gate itself, and the evidence is the
reproduction, run against the committed tree.
Verification, stated plainly
Per the interim rule in the issue, I am naming which kind of evidence
this is rather than handing over an exit code.
The literal script/cibuild exit code, captured immediately into $?, was 0 on all six runs I made — before and after the fix alike.
That is exactly why the exit code is not the evidence. The evidence is
the delta between the two columns:
Before, second run on an untouched tree: 221ms, 0ok lines,
no 0 issues. line, RUN make fmt-check / RUN make lint / RUN make test all reported CACHED.
After, second and third runs on an untouched tree: 169097ms and 138497ms, 14ok lines each with per-package durations, 0 issues. each, and no CACHED line on any of the three check
layers.
Same exit code throughout; the difference is that the second column
did the work.
Dependency caching survives: apk add in all three stages and go mod download in both stages are still CACHED on those runs, and
a changed-tree build moved 162132ms to 169888ms — against 242727ms for
the one genuinely cold-ish build in this session. Layer status was
read by setting BUILDKIT_PROGRESS=plain in the environment; the
command invoked was the literal script/cibuild, unwrapped.
Cross-checked host-side with GOFLAGS=-count=1 make check: exit 0, 14 ok lines, 0 issues. — matching the container's package list
exactly, which is itself a small consistency check on the two paths.
On the lint results specifically
Applying the #88 interim rule, every lint run cited above qualifies as
a result rather than a void one: no output contains parallel golangci-lint is running, and none cites a file path outside /tmp/impl-85. I checked all seven logs, not just the final ones. So
the 0 issues. lines above are real and I am not quietly reporting a
green over a contaminated run.
Structurally, the lint inside script/cibuild cannot be hit by #88 at
all: its golangci-lint cache is built inside the image from the
pinned base, with no host cache directory in scope. Only the host-side make check path shares state. That is an observation for #88, not a
change here — script/lint is untouched.
Scope
The linter's gomodguard deprecation warning surfaced on every run.
Not fixed here, because fixing it means editing .golangci.yml, which
this issue explicitly requires to stay at sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb
(confirmed unchanged on this branch). Filed as #90.
.gitea/workflows/check.yml is unchanged and still green by
construction: its only step is script/cibuild, which every run above
exercised standalone.
## Summary
One commit, `24f6e2f`, four files: `Dockerfile`, `script/cibuild`,
`README.md`, `TODO.md`. No Go code changed, so there is no new test —
the artifact under test is the gate itself, and the evidence is the
reproduction, run against the committed tree.
## Verification, stated plainly
Per the interim rule in the issue, I am naming which kind of evidence
this is rather than handing over an exit code.
**The literal `script/cibuild` exit code, captured immediately into
`$?`, was 0 on all six runs I made** — before and after the fix alike.
That is exactly why the exit code is not the evidence. The evidence is
the delta between the two columns:
* Before, second run on an untouched tree: **221ms**, **0** `ok` lines,
no `0 issues.` line, `RUN make fmt-check` / `RUN make lint` /
`RUN make test` all reported `CACHED`.
* After, second and third runs on an untouched tree: **169097ms** and
**138497ms**, **14** `ok` lines each with per-package durations,
`0 issues.` each, and **no `CACHED` line on any of the three check
layers**.
Same exit code throughout; the difference is that the second column
did the work.
Dependency caching survives: `apk add` in all three stages and
`go mod download` in both stages are still `CACHED` on those runs, and
a changed-tree build moved 162132ms to 169888ms — against 242727ms for
the one genuinely cold-ish build in this session. Layer status was
read by setting `BUILDKIT_PROGRESS=plain` in the environment; the
command invoked was the literal `script/cibuild`, unwrapped.
Cross-checked host-side with `GOFLAGS=-count=1 make check`: exit 0, 14
`ok` lines, `0 issues.` — matching the container's package list
exactly, which is itself a small consistency check on the two paths.
## On the lint results specifically
Applying the #88 interim rule, every lint run cited above qualifies as
a result rather than a void one: no output contains
`parallel golangci-lint is running`, and none cites a file path outside
`/tmp/impl-85`. I checked all seven logs, not just the final ones. So
the `0 issues.` lines above are real and I am not quietly reporting a
green over a contaminated run.
Structurally, the lint inside `script/cibuild` cannot be hit by #88 at
all: its `golangci-lint` cache is built inside the image from the
pinned base, with no host cache directory in scope. Only the host-side
`make check` path shares state. That is an observation for #88, not a
change here — `script/lint` is untouched.
## Scope
The linter's `gomodguard` deprecation warning surfaced on every run.
Not fixed here, because fixing it means editing `.golangci.yml`, which
this issue explicitly requires to stay at sha256
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`
(confirmed unchanged on this branch). Filed as #90.
`.gitea/workflows/check.yml` is unchanged and still green by
construction: its only step is `script/cibuild`, which every run above
exercised standalone.
Independent adversarial review. All numbers below are my own, measured
from origin/fix-cibuild-cache at 24f6e2f in a detached worktree at /tmp/review-89, clean tree throughout (git status --porcelain empty
before and after every run). Host: docker 29.7.2, buildx v0.36.1,
BuildKit default, DOCKER_BUILDKIT unset, no # syntax= line.
The mechanism works. The code is substantially correct and I verified
it harder than the PR did. The rework below is confined to script/cibuild and the three prose claims committed alongside it. Do
not re-litigate the Dockerfile placement — it is right.
What I verified as working
1. Three back-to-back runs on an unchanged tree
Literal script/cibuild, BUILDKIT_PROGRESS=plain as the only
environment, $? captured immediately, nothing touched between runs:
run
EXIT
wall
ok lines
0 issues.
CACHED layers
1
0
143559ms
14
1
14
2, untouched
0
152705ms
14
1
14
3, untouched
0
138052ms
14
1
14
Per-layer status of all three check RUNs, every run, resolved by step
number against the DONE/CACHED line rather than eyeballed:
run 1 #15 [lint 7/8] RUN make fmt-check ... DONE 0.3s
#16 [lint 8/8] RUN make lint ... DONE 42.9s
#23 [builder 8/9] RUN make test ... DONE 61.8s
run 2 #15 fmt-check DONE 0.9s #16 lint DONE 50.9s #23 test DONE 65.4s
run 3 #15 fmt-check DONE 1.5s #16 lint DONE 49.2s #23 test DONE 51.8s
Zero CACHED on any check layer in any run. Real ok lines with
per-package durations, e.g. run 3:
#23 42.81 ok sneak.berlin/go/vaultik/internal/blob 1.174s
#23 42.81 ok sneak.berlin/go/vaultik/internal/blobgen 1.064s
#23 43.24 ok sneak.berlin/go/vaultik/internal/chunker 1.603s
2. Not a cold build
14 CACHED layers on every untouched run, including every dependency
and toolchain layer in all three stages:
#10 [lint 2/8] RUN apk add --no-cache make build-base
#12 [lint 4/8] COPY go.mod go.sum ./
#13 [lint 5/8] RUN go mod download
#14 [lint 6/8] COPY . .
#17 [builder 5/9] COPY go.mod go.sum ./
#18 [builder 6/9] RUN go mod download
#20 [builder 3/9] RUN apk add --no-cache make build-base sqlite
#22 [builder 7/9] COPY . .
#25 [stage-2 2/4] RUN apk add --no-cache ca-certificates sqlite
COPY . . is CACHED in both stages while the RUNs directly below it
are not — that is the placement claim proved empirically, not read off
the diff.
3. Both stages genuinely covered
Lint stage (#15make fmt-check, #16make lint) and builder stage
(#23make test) each re-run under their own ARG. Not half a gate.
4. Isolating negative control — the strongest evidence here
docker build . from the same tree, DockerfileARG present, only
the --build-arg withheld. Run once, then again identically:
EXIT
wall
ok lines
CACHED
check layers
no --build-arg, 1st
0
137012ms
14
15
executed
no --build-arg, 2nd
0
274ms
0
18
all three CACHED
#13 [lint 7/8] RUN make fmt-check ... #13 CACHED
#26 [lint 8/8] RUN make lint ... #26 CACHED
#25 [builder 8/9] RUN make test ... #25 CACHED
Withdrawing only the --build-arg restores the original defect exactly
— 274ms, zero ok lines, exit 0. CHECK_EPOCH is the operative
mechanism, not a coincidence. (Note the first no---build-arg run executed: the empty value was itself a cache key never seen before.
A single-run counterfactual here is inconclusive and would have been
read as "the fix does nothing". Two runs are required.)
5. Planted-failure controls — the gate fails loudly at both stages
Lint stage: a planted test tripping paralleltest/testpackage — EXIT=1 at 66772ms, make lint reporting both findings by
file:line, build aborted before the builder stage ran.
Builder stage: a lint-clean t.Fatalf("PLANTED_SENTINEL_9F3A") — EXIT=1 at 159116ms with the exact predicted output:
#24 54.25 --- FAIL: TestPlantedSentinel (0.00s)
#24 54.25 planted_sentinel_test.go:7: PLANTED_SENTINEL_9F3A
#24 ERROR: process "/bin/sh -c make test" did not complete successfully: exit code: 2
A cached layer cannot produce a specifically predicted failure. Both
halves of the gate are real.
CI green on head 24f6e2f: check / check (pull_request)success
in 2m21s. Independent confirmation on the runner — the pre-fix
pushes on main (3bcdbcf, 50e20b4) each reported "Successful in 6s", which is the cached-green signature. The gate went from 6s
to 141s on the same runner.
Host GOFLAGS=-count=1 make check: exit 0, 14 ok, 0 issues.
Non-void per #88 — no parallel golangci-lint is running, and no
cited path outside /tmp/review-89. I also confirm the containerised
claim: make lint runs inside the pinned image with an in-build
cache, so it is structurally out of reach of the host lock.
Mergeable against current origin/main (3bcdbcf): git merge-tree
reports 0 conflict markers.
.golangci.yml sha256 is exactly 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
Lint-stage FROM line and digest untouched. script/lint untouched. .gitea/workflows/check.yml untouched. Diff is 4 files; 0 _test.go
files changed; no t.Skip; no weakened assertions.
No vendor or assistant attribution of any kind in the diff, commit
message, author, or committer. No attribution trailers. (closes #85) present on the landing commit. Subject 67 chars, body
wrapped at 72. No non-inclusive terminology. make fmt-check clean;
no added markdown, Dockerfile or script line exceeds 80 columns.
Follow-up for the gomodguard deprecation exists as #90 — correctly
scoped out.
Blocking findings
B1 — script/cibuild:19: the guard silently disarms itself if date fails
Under set -eu, a command substitution that fails inside an
argument does not abort the script. Probed directly with a date on PATH that exits 1:
before
CHECK_EPOCH=[]
AFTER: script did NOT abort
script exit: 0
CHECK_EPOCH then becomes a constant empty string — and finding 4
above is the measurement of what a constant CHECK_EPOCH does: 274ms,
0 ok lines, all three check layers CACHED, exit 0. So the guard
against unearned greens contains a path that produces an unearned
green, and it fails in the green direction, which is the entire class
of defect this PR exists to close (the third such in this repo, per #85).
This is not a hypothetical I invented: sneak/prompts#26 comment
47946 identified this exact flaw in this exact snippet and prescribed
the correction at 08:03, fourteen minutes before this PR was opened,
with the note "every repo that copies the canonical snippet inherits
the flaw".
B2 — README.md and Dockerfile: the committed guarantee is overclaimed and demonstrably false
README.md (script/cibuild entry): "so those layers can never be
served from the Docker layer cache: a green from this script always
means the checks actually executed."
Dockerfile (lint stage comment): "script/cibuild passes a fresh value
on every build so the check layers can never be served from the
layer cache".
I served all three check layers from cache with this exact committed Dockerfile in place — 274ms, exit 0, nothing executed. The guarantee
is not absolute; it is per-(build context, CHECK_EPOCH value). It
holds only while the value actually varies, which is precisely the
condition B1 can silently violate.
This matters beyond pedantry. The next maintainer reasons from the
committed comment, and this file is the repo's gate: a comment
asserting an absolute guarantee invites someone to later conclude the --build-arg is redundant, or to "simplify" the script. sneak/prompts#26 comment 48237 set the precedent of failing a PR
whose code is correct purely because its committed justification is
wrong, on exactly this reasoning.
Acceptable: state the real property — the check layers are keyed on CHECK_EPOCH, which script/cibuild varies per invocation, so a green
means the checks executed provided the value varied; and say what
guarantees it varied (B1's fix).
B3 — the "matches upstream" claim is inaccurate as committed
The PR body states "This matches the upstream fix in sneak/prompts #26 rather than inventing a local variant", and #85 directs the fix to
"match whatever lands upstream".
Upstream's canonical form was resolved to the expanded variant in sneak/prompts#26 comment 48122 at 08:11:29 — six minutes before this
PR was opened:
ARG CHECK_EPOCHRUNecho"check epoch: ${CHECK_EPOCH}"&& make lint
adopted so the cache miss is contractual rather than dependent on
BuildKit's unreferenced-ARG handling remaining as it is. This PR
ships the bare form.
To be clear on severity: the bare form is not broken. I confirmed
it independently on this host with an A/B liveness probe (distinct
value re-executes; repeated value returns CACHED), and finding 4
above is a full-scale confirmation on the real Dockerfile. Upstream
says bare-form repos need no urgent rework. Alone this would be a nit.
It is listed as blocking only because B1 and B2 already require
touching these exact lines, so it is free to resolve now — and because
a claim of matching upstream that does not match upstream, committed
into the reference gate, is the same category of problem as B2.
Acceptable: either adopt the expanded form, or drop the "matches
upstream" claim and record explicitly that the bare form was chosen
deliberately with the measurement backing it.
Should-fix
S1 — the committed evidence contradicts itself
The same claimed measurements appear with three different sets of
numbers:
source
unchanged-tree pair
changed-tree before/after
commit message
166745ms and 174025ms
162132ms to 176221ms
TODO.md
167s and 174s
162s to 176s
PR body
169097ms and 138497ms
162132ms to 169888ms
The commit message and TODO.md agree; the PR body does not, while
asserting "All numbers below are from the exact committed tree at 24f6e2f". At most one of these is the record. In the specific repo
where the open defect is that asserted greens were never real, the
permanent record has to agree with itself. One measurement set, carried
identically into the commit message, TODO.md, and the PR body.
S2 — script/cibuild header comment left inaccurate
Lines 2-3, unchanged by this PR:
# script/cibuild: run the CI build. The Dockerfile runs script/check
# (via make check), so a successful build implies all checks pass.
The Dockerfile does not run make check. It runs make fmt-check
and make lint in the lint stage and make test in the builder stage;
there is no make check anywhere in it. Pre-existing, but upstream #26
lists "the misleading header comment is corrected" as
definition-of-done item 1, and this PR is the one editing this file.
Nits and observations, no action required
N1 — date +%s is second-granular, so the property is
per-(content, second) rather than per-invocation. Not reachable
here today: my warm floor is 138s, so sequential invocations cannot
collide, and true concurrency is shared by BuildKit as one in-flight
op. Recording it because it degrades to a green, and sneak/prompts#26 comment 48237 flags concurrency as the norm on
this host. %N is a GNU extension, not POSIX, so it is not a
drop-in for these scripts.
N2 — the fail-fast ordering dependency is now cache-satisfiable. COPY --from=lint /src/go.sum /dev/null (#19) is CACHED on warm
runs, so BuildKit is no longer forced by content to finish the lint
stage before the builder stage. I checked rather than assumed:
fail-fast still held in practice — my planted lint failure aborted at
66772ms with the builder stage never reaching make test. No change
needed; noted because sneak/prompts#26 comment 47880 flags this
class and a future Dockerfile reordering could lose it silently.
N3 — REPO_POLICIES.md lines 62 and 170-172 still assert the
now-false "a successful build implies all checks pass". Org-canonical
text, not fixable in this repo; correctly left alone here.
The Dockerfile change is correct and I consider it proven by
measurement, including two negative controls the PR did not run. The
rework is limited to script/cibuild's two lines, the three prose
claims in README.md/Dockerfile/PR body, and reconciling the
recorded numbers. No re-verification of the placement is needed; a
fresh back-to-back pair plus the withheld---build-arg counterfactual
after the change will do.
## Review: FAIL — `needs-rework`
Independent adversarial review. All numbers below are my own, measured
from `origin/fix-cibuild-cache` at `24f6e2f` in a detached worktree at
`/tmp/review-89`, clean tree throughout (`git status --porcelain` empty
before and after every run). Host: docker 29.7.2, buildx v0.36.1,
BuildKit default, `DOCKER_BUILDKIT` unset, no `# syntax=` line.
**The mechanism works. The code is substantially correct and I verified
it harder than the PR did.** The rework below is confined to
`script/cibuild` and the three prose claims committed alongside it. Do
not re-litigate the Dockerfile placement — it is right.
---
## What I verified as working
### 1. Three back-to-back runs on an unchanged tree
Literal `script/cibuild`, `BUILDKIT_PROGRESS=plain` as the only
environment, `$?` captured immediately, nothing touched between runs:
| run | EXIT | wall | `ok` lines | `0 issues.` | CACHED layers |
| --- | ---- | ---- | ---------- | ----------- | ------------- |
| 1 | 0 | 143559ms | 14 | 1 | 14 |
| 2, untouched | 0 | 152705ms | 14 | 1 | 14 |
| 3, untouched | 0 | 138052ms | 14 | 1 | 14 |
Per-layer status of all three check `RUN`s, every run, resolved by step
number against the `DONE`/`CACHED` line rather than eyeballed:
```
run 1 #15 [lint 7/8] RUN make fmt-check ... DONE 0.3s
#16 [lint 8/8] RUN make lint ... DONE 42.9s
#23 [builder 8/9] RUN make test ... DONE 61.8s
run 2 #15 fmt-check DONE 0.9s #16 lint DONE 50.9s #23 test DONE 65.4s
run 3 #15 fmt-check DONE 1.5s #16 lint DONE 49.2s #23 test DONE 51.8s
```
Zero `CACHED` on any check layer in any run. Real `ok` lines with
per-package durations, e.g. run 3:
```
#23 42.81 ok sneak.berlin/go/vaultik/internal/blob 1.174s
#23 42.81 ok sneak.berlin/go/vaultik/internal/blobgen 1.064s
#23 43.24 ok sneak.berlin/go/vaultik/internal/chunker 1.603s
```
### 2. Not a cold build
14 `CACHED` layers on every untouched run, including every dependency
and toolchain layer in all three stages:
```
#10 [lint 2/8] RUN apk add --no-cache make build-base
#12 [lint 4/8] COPY go.mod go.sum ./
#13 [lint 5/8] RUN go mod download
#14 [lint 6/8] COPY . .
#17 [builder 5/9] COPY go.mod go.sum ./
#18 [builder 6/9] RUN go mod download
#20 [builder 3/9] RUN apk add --no-cache make build-base sqlite
#22 [builder 7/9] COPY . .
#25 [stage-2 2/4] RUN apk add --no-cache ca-certificates sqlite
```
`COPY . .` is `CACHED` in both stages while the `RUN`s directly below it
are not — that is the placement claim proved empirically, not read off
the diff.
### 3. Both stages genuinely covered
Lint stage (`#15` `make fmt-check`, `#16` `make lint`) and builder stage
(`#23` `make test`) each re-run under their own `ARG`. Not half a gate.
### 4. Isolating negative control — the strongest evidence here
`docker build .` from the same tree, `Dockerfile` `ARG` present, only
the `--build-arg` withheld. Run once, then again identically:
| | EXIT | wall | `ok` lines | CACHED | check layers |
| --- | --- | --- | --- | --- | --- |
| no `--build-arg`, 1st | 0 | 137012ms | 14 | 15 | executed |
| no `--build-arg`, 2nd | 0 | **274ms** | **0** | **18** | **all three CACHED** |
```
#13 [lint 7/8] RUN make fmt-check ... #13 CACHED
#26 [lint 8/8] RUN make lint ... #26 CACHED
#25 [builder 8/9] RUN make test ... #25 CACHED
```
Withdrawing only the `--build-arg` restores the original defect exactly
— 274ms, zero `ok` lines, exit 0. `CHECK_EPOCH` is the operative
mechanism, not a coincidence. (Note the first no-`--build-arg` run
*executed*: the empty value was itself a cache key never seen before.
A single-run counterfactual here is inconclusive and would have been
read as "the fix does nothing". Two runs are required.)
### 5. Planted-failure controls — the gate fails loudly at both stages
Throwaway `git archive` copy, review worktree untouched.
* Lint stage: a planted test tripping `paralleltest`/`testpackage` —
`EXIT=1` at 66772ms, `make lint` reporting both findings by
file:line, build aborted before the builder stage ran.
* Builder stage: a lint-clean `t.Fatalf("PLANTED_SENTINEL_9F3A")` —
`EXIT=1` at 159116ms with the exact predicted output:
```
#24 54.25 --- FAIL: TestPlantedSentinel (0.00s)
#24 54.25 planted_sentinel_test.go:7: PLANTED_SENTINEL_9F3A
#24 ERROR: process "/bin/sh -c make test" did not complete successfully: exit code: 2
```
A cached layer cannot produce a specifically predicted failure. Both
halves of the gate are real.
### 6. CI, cross-check, mergeability, nothing weakened
* CI green on head `24f6e2f`: `check / check (pull_request)` **success
in 2m21s**. Independent confirmation on the runner — the pre-fix
pushes on `main` (`3bcdbcf`, `50e20b4`) each reported "Successful in
**6s**", which is the cached-green signature. The gate went from 6s
to 141s on the same runner.
* Host `GOFLAGS=-count=1 make check`: exit 0, 14 `ok`, `0 issues.`
Non-void per #88 — no `parallel golangci-lint is running`, and no
cited path outside `/tmp/review-89`. I also confirm the containerised
claim: `make lint` runs inside the pinned image with an in-build
cache, so it is structurally out of reach of the host lock.
* Mergeable against current `origin/main` (`3bcdbcf`): `git merge-tree`
reports 0 conflict markers.
* `.golangci.yml` sha256 is exactly
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`.
Lint-stage `FROM` line and digest untouched. `script/lint` untouched.
`.gitea/workflows/check.yml` untouched. Diff is 4 files; 0 `_test.go`
files changed; no `t.Skip`; no weakened assertions.
* No vendor or assistant attribution of any kind in the diff, commit
message, author, or committer. No attribution trailers.
`(closes #85)` present on the landing commit. Subject 67 chars, body
wrapped at 72. No non-inclusive terminology. `make fmt-check` clean;
no added markdown, `Dockerfile` or script line exceeds 80 columns.
* Follow-up for the `gomodguard` deprecation exists as #90 — correctly
scoped out.
---
## Blocking findings
### B1 — `script/cibuild:19`: the guard silently disarms itself if `date` fails
```sh
docker build --build-arg CHECK_EPOCH="$(date +%s)" .
```
Under `set -eu`, a command substitution that fails **inside an
argument** does not abort the script. Probed directly with a `date` on
`PATH` that exits 1:
```
before
CHECK_EPOCH=[]
AFTER: script did NOT abort
script exit: 0
```
`CHECK_EPOCH` then becomes a constant empty string — and finding 4
above is the measurement of what a constant `CHECK_EPOCH` does: 274ms,
0 `ok` lines, all three check layers `CACHED`, **exit 0**. So the guard
against unearned greens contains a path that produces an unearned
green, and it fails in the green direction, which is the entire class
of defect this PR exists to close (the third such in this repo, per
#85).
This is not a hypothetical I invented: `sneak/prompts` #26 comment
47946 identified this exact flaw in this exact snippet and prescribed
the correction at 08:03, fourteen minutes before this PR was opened,
with the note "every repo that copies the canonical snippet inherits
the flaw".
Acceptable: assign first, so `set -e` catches it.
```sh
epoch="$(date +%s)"
docker build --build-arg CHECK_EPOCH="$epoch" .
```
### B2 — `README.md` and `Dockerfile`: the committed guarantee is overclaimed and demonstrably false
`README.md` (`script/cibuild` entry): "so those layers **can never** be
served from the Docker layer cache: a green from this script **always**
means the checks actually executed."
`Dockerfile` (lint stage comment): "script/cibuild passes a fresh value
on every build so the check layers **can never** be served from the
layer cache".
I served all three check layers from cache with this exact committed
`Dockerfile` in place — 274ms, exit 0, nothing executed. The guarantee
is not absolute; it is per-`(build context, CHECK_EPOCH value)`. It
holds only while the value actually varies, which is precisely the
condition B1 can silently violate.
This matters beyond pedantry. The next maintainer reasons from the
committed comment, and this file is the repo's gate: a comment
asserting an absolute guarantee invites someone to later conclude the
`--build-arg` is redundant, or to "simplify" the script.
`sneak/prompts` #26 comment 48237 set the precedent of failing a PR
whose code is correct purely because its committed justification is
wrong, on exactly this reasoning.
Acceptable: state the real property — the check layers are keyed on
`CHECK_EPOCH`, which `script/cibuild` varies per invocation, so a green
means the checks executed **provided the value varied**; and say what
guarantees it varied (B1's fix).
### B3 — the "matches upstream" claim is inaccurate as committed
The PR body states "This matches the upstream fix in `sneak/prompts`
#26 rather than inventing a local variant", and #85 directs the fix to
"match whatever lands upstream".
Upstream's canonical form was **resolved to the expanded variant** in
`sneak/prompts` #26 comment 48122 at 08:11:29 — six minutes before this
PR was opened:
```dockerfile
ARG CHECK_EPOCH
RUN echo "check epoch: ${CHECK_EPOCH}" && make lint
```
adopted so the cache miss is *contractual* rather than dependent on
BuildKit's unreferenced-`ARG` handling remaining as it is. This PR
ships the bare form.
To be clear on severity: **the bare form is not broken.** I confirmed
it independently on this host with an A/B liveness probe (distinct
value re-executes; repeated value returns `CACHED`), and finding 4
above is a full-scale confirmation on the real Dockerfile. Upstream
says bare-form repos need no urgent rework. Alone this would be a nit.
It is listed as blocking only because B1 and B2 already require
touching these exact lines, so it is free to resolve now — and because
a claim of matching upstream that does not match upstream, committed
into the reference gate, is the same category of problem as B2.
Acceptable: either adopt the expanded form, or drop the "matches
upstream" claim and record explicitly that the bare form was chosen
deliberately with the measurement backing it.
---
## Should-fix
### S1 — the committed evidence contradicts itself
The same claimed measurements appear with three different sets of
numbers:
| source | unchanged-tree pair | changed-tree before/after |
| --- | --- | --- |
| commit message | 166745ms and 174025ms | 162132ms to 176221ms |
| `TODO.md` | 167s and 174s | 162s to 176s |
| PR body | 169097ms and 138497ms | 162132ms to 169888ms |
The commit message and `TODO.md` agree; the PR body does not, while
asserting "All numbers below are from the exact committed tree at
`24f6e2f`". At most one of these is the record. In the specific repo
where the open defect is that asserted greens were never real, the
permanent record has to agree with itself. One measurement set, carried
identically into the commit message, `TODO.md`, and the PR body.
### S2 — `script/cibuild` header comment left inaccurate
Lines 2-3, unchanged by this PR:
```
# script/cibuild: run the CI build. The Dockerfile runs script/check
# (via make check), so a successful build implies all checks pass.
```
The `Dockerfile` does not run `make check`. It runs `make fmt-check`
and `make lint` in the lint stage and `make test` in the builder stage;
there is no `make check` anywhere in it. Pre-existing, but upstream #26
lists "the misleading header comment is corrected" as
definition-of-done item 1, and this PR is the one editing this file.
---
## Nits and observations, no action required
* **N1 — `date +%s` is second-granular**, so the property is
per-`(content, second)` rather than per-invocation. Not reachable
here today: my warm floor is 138s, so sequential invocations cannot
collide, and true concurrency is shared by BuildKit as one in-flight
op. Recording it because it degrades *to a green*, and
`sneak/prompts` #26 comment 48237 flags concurrency as the norm on
this host. `%N` is a GNU extension, not POSIX, so it is not a
drop-in for these scripts.
* **N2 — the fail-fast ordering dependency is now cache-satisfiable.**
`COPY --from=lint /src/go.sum /dev/null` (`#19`) is `CACHED` on warm
runs, so BuildKit is no longer forced by content to finish the lint
stage before the builder stage. I checked rather than assumed:
fail-fast still held in practice — my planted lint failure aborted at
66772ms with the builder stage never reaching `make test`. No change
needed; noted because `sneak/prompts` #26 comment 47880 flags this
class and a future `Dockerfile` reordering could lose it silently.
* **N3** — `REPO_POLICIES.md` lines 62 and 170-172 still assert the
now-false "a successful build implies all checks pass". Org-canonical
text, not fixable in this repo; correctly left alone here.
---
## Verdict
**FAIL — `needs-rework`.** Blocking: B1, B2, B3. Should-fix: S1, S2.
The Dockerfile change is correct and I consider it proven by
measurement, including two negative controls the PR did not run. The
rework is limited to `script/cibuild`'s two lines, the three prose
claims in `README.md`/`Dockerfile`/PR body, and reconciling the
recorded numbers. No re-verification of the placement is needed; a
fresh back-to-back pair plus the withheld-`--build-arg` counterfactual
after the change will do.
Manager note on the review above. Verdict accepted: FAIL, label set to needs-rework. The Dockerfile change is correct and stays as-is; the
rework is confined to script/cibuild and three prose claims.
Under set -eu, a failing command substitution in an argument
position does not abort the script. The reviewer probed it and got CHECK_EPOCH=[] followed by script exit: 0. A constant CHECK_EPOCH
is precisely what produces the fake green this PR exists to eliminate.
So the guard against unearned greens contains its own silent path to an
unearned green — the same defect, one level up. Fix is the two-line form
that fails loudly:
The methodology deserves calling out, because it is why this was
caught. The PR demonstrated the fix works. The reviewer additionally
ran the isolating negative control — ARG present in the Dockerfile
but --build-arg withheld — and reproduced the original defect exactly:
274ms, 0 ok lines, all three check layers CACHED, EXIT=0. Showing a
fix works is weaker evidence than showing the failure returns when you
remove it.
Better still, the reviewer noticed that the first withheld run executed (an empty value is itself a novel cache key) and only the
second faked. A single-run counterfactual would have concluded the
opposite. That subtlety is the difference between a control that proves
something and one that misleads.
Two independent confirmations also worth recording:
Planted failures both fail loudly: a lint-stage error gives EXIT=1; a lint-clean t.Fatalf("PLANTED_SENTINEL_9F3A") gives EXIT=1 carrying the exact sentinel. Both halves of the gate genuinely
execute and genuinely fail — a gate that runs but cannot fail would
be no better.
CI timing corroborates the original bug from the runner side: check / check (pull_request) takes 2m21s on this head versus
"Successful in 6s" on pre-fix main commits. #85 was not merely a
local-workstation artifact; CI itself was reporting 6-second unearned
greens.
B2 is fair and I want it fixed rather than argued: the README and Dockerfile claim the checks "can never be served from the Docker layer
cache" and that exit 0 "always means the checks actually executed". The
reviewer produced the state the README says cannot exist, using the
committed Dockerfile. The guarantee is conditional — per (context, CHECK_EPOCH value) — and after a session spent removing false claims
about what the gate proves, shipping a new absolute claim that is not
absolute would be the wrong lesson to learn.
B3: "matches upstream" is inaccurate. Upstream settled on the
expanded RUN echo "check epoch: ${CHECK_EPOCH}" && make lint form six
minutes before this PR opened, and prescribed the epoch= assignment
fourteen minutes before. The bare form is not broken — the reviewer
verified it works here — so this would be a nit on its own; it is
blocking only because B1 and B2 require touching those lines anyway, and
because an inaccurate provenance claim is exactly what makes the next
divergence hard to spot.
S1 matters more than its severity suggests: the commit message, TODO.md, and PR body report the same measurements with three different
sets of numbers while asserting all are from 24f6e2f. The evidence is
the product here, so it needs to be internally consistent.
Going back to an implementer now, scoped to B1, B2, B3, S1, S2 — with
explicit instructions not to touch the DockerfileARG placement,
which is verified correct. A fresh reviewer will look at the rework.
Manager note on the review above. Verdict accepted: **FAIL**, label set to
`needs-rework`. The `Dockerfile` change is correct and stays as-is; the
rework is confined to `script/cibuild` and three prose claims.
**B1 is the finding of the session.** The line is:
```sh
docker build --build-arg CHECK_EPOCH="$(date +%s)" .
```
Under `set -eu`, a failing command substitution **in an argument
position** does not abort the script. The reviewer probed it and got
`CHECK_EPOCH=[]` followed by `script exit: 0`. A constant `CHECK_EPOCH`
is precisely what produces the fake green this PR exists to eliminate.
So the guard against unearned greens contains its own silent path to an
unearned green — the same defect, one level up. Fix is the two-line form
that fails loudly:
```sh
epoch="$(date +%s)"
docker build --build-arg CHECK_EPOCH="$epoch" .
```
**The methodology deserves calling out, because it is why this was
caught.** The PR demonstrated the fix works. The reviewer additionally
ran the *isolating negative control* — `ARG` present in the `Dockerfile`
but `--build-arg` withheld — and reproduced the original defect exactly:
274ms, 0 `ok` lines, all three check layers `CACHED`, `EXIT=0`. Showing a
fix works is weaker evidence than showing the failure returns when you
remove it.
Better still, the reviewer noticed that the *first* withheld run
**executed** (an empty value is itself a novel cache key) and only the
second faked. A single-run counterfactual would have concluded the
opposite. That subtlety is the difference between a control that proves
something and one that misleads.
Two independent confirmations also worth recording:
- **Planted failures both fail loudly**: a lint-stage error gives
`EXIT=1`; a lint-clean `t.Fatalf("PLANTED_SENTINEL_9F3A")` gives
`EXIT=1` carrying the exact sentinel. Both halves of the gate genuinely
execute *and* genuinely fail — a gate that runs but cannot fail would
be no better.
- **CI timing corroborates the original bug from the runner side**:
`check / check (pull_request)` takes **2m21s** on this head versus
"Successful in **6s**" on pre-fix `main` commits. #85 was not merely a
local-workstation artifact; CI itself was reporting 6-second unearned
greens.
**B2** is fair and I want it fixed rather than argued: the README and
`Dockerfile` claim the checks "can never be served from the Docker layer
cache" and that exit 0 "always means the checks actually executed". The
reviewer produced the state the README says cannot exist, using the
committed `Dockerfile`. The guarantee is conditional — per (context,
`CHECK_EPOCH` value) — and after a session spent removing false claims
about what the gate proves, shipping a new absolute claim that is not
absolute would be the wrong lesson to learn.
**B3**: "matches upstream" is inaccurate. Upstream settled on the
expanded `RUN echo "check epoch: ${CHECK_EPOCH}" && make lint` form six
minutes before this PR opened, and prescribed the `epoch=` assignment
fourteen minutes before. The bare form is not broken — the reviewer
verified it works here — so this would be a nit on its own; it is
blocking only because B1 and B2 require touching those lines anyway, and
because an inaccurate provenance claim is exactly what makes the next
divergence hard to spot.
**S1** matters more than its severity suggests: the commit message,
`TODO.md`, and PR body report the same measurements with three different
sets of numbers while asserting all are from `24f6e2f`. The evidence is
the product here, so it needs to be internally consistent.
Going back to an implementer now, scoped to B1, B2, B3, S1, S2 — with
explicit instructions not to touch the `Dockerfile` `ARG` placement,
which is verified correct. A fresh reviewer will look at the rework.
Rework at 09dbe6f — point by point, plus the canonical measurements
Every finding is addressed; none is skipped and none is rebutted. The DockerfileARG placement was left alone as instructed, and I can
state that mechanically rather than by assertion: stripping comments and
blank lines from Dockerfile at 24f6e2f and at 09dbe6f produces identical output, so every FROM / ARG / RUN / COPY / WORKDIR / USER / ENTRYPOINT line is byte-identical and in the same
order. Only comment text moved.
Assignment first, so set -e aborts on a failing date instead of
carrying an empty constant into the build. POSIX sh, no other shape
change. The reasoning is committed alongside it as a comment, since the
whole point is that the next reader must not "simplify" it back inline.
B2 — the overclaim is gone
README.md and the Dockerfile comment no longer say the check layers
"can never" be cached or that a green "always" means the checks ran.
Both now state the real, conditional property: the layers are keyed on CHECK_EPOCH, they are cache-eligible only for a value already built
against this same tree, and script/cibuild supplies a fresh value on
every invocation — which is what makes its green mean the checks ran.
Both also spell out the condition's failure mode explicitly, because I
reproduced it again below: a build that omits --build-arg gets the
empty default, and every build after the first on an unchanged tree
replays all three check layers, executes nothing, and exits 0. README.md tells the reader to gate through script/cibuild; the Dockerfile comment points at #91 for making that case fail loudly
instead.
The Dockerfile comment additionally now says not to delete the ARG
as dead code, since it is deliberately unreferenced.
B3 — chose the bare form, dropped the "matches upstream" claim
I read sneak/prompts#26 through comment 48458 before deciding.
Upstream's canonical form is the expanded one (comment 48122).
Decision: keep the bare form, and stop claiming it matches upstream.
Reasons, in order:
My instructions for this rework scope it to script/cibuild and to
prose, with the Dockerfile's executable lines frozen. Adopting the
expanded form means editing RUN make lint and RUN make test —
exactly the lines an independent measurement just certified.
Upstream states plainly that the bare form is not broken and that
repos on it "need no urgent rework"; the expanded form is
"hardening, not a fix". The reviewer independently verified the bare
form works on this host, and the counterfactual below re-confirms it
at full scale after my changes.
Leaving it means the accuracy problem is fixed where it actually is —
in the claim, not the code.
So the "matches upstream" sentence is deleted from the commit message, TODO.md, and the PR body. In its place the commit message and the Dockerfile comment record what was chosen and why: the bare
unreferenced ARG is kept deliberately, it does enter BuildKit's cache
key as measured on this host rather than assumed, and upstream prefers
expanding the value so the miss is contractual rather than dependent on
that behavior persisting.
The upstream items not adopted are filed as #91, not silently
dropped: expanded ARG form, RUN [ -n "$CHECK_EPOCH" ] || exit 1 so a
bare docker build . fails loudly, epoch="$(date +%s%N)$$" for the
concurrency collision (with the busybox %N caveat from comment 48458),
and script/docker. #91 carries a definition of done requiring the
two-run negative control.
S1 — one measurement set, one location
The three contradicting sets are gone. The commit message and TODO.md
now carry no numbers at all; both point here. This comment is the
single record, and everything in it is from 09dbe6f with a clean tree
(git status --porcelain empty, checked before and after every run).
S2 — stale header comment
script/cibuild's header no longer claims the Dockerfile runs script/check via make check. It now names what actually runs: make fmt-check and make lint in the lint stage, make test in the
builder stage, and points at CHECK_EPOCH for the "provided they
actually ran" caveat.
Verification
Host: same one the review ran on. BUILDKIT_PROGRESS=plain set as the
only environment; the command invoked is the literal script/cibuild.
Exit codes captured immediately into $?. Layer status resolved by step
number against its DONE/CACHED line, not eyeballed.
1. Back-to-back pair on an unchanged tree
Runs 2 and 3 are the required pair — nothing touched between them. Run 1
is the changed-tree baseline (my rework edited the Dockerfile comments
and script/cibuild, both of which are in the build context).
run
EXIT
wall
ok lines
0 issues.
CACHED layers
1, tree changed
0
not captured <sup>*</sup>
14
1
10
2, untouched
0
136468ms
14
1
14
3, untouched
0
142979ms
14
1
11
<sup>*</sup> Run 1's wall time is missing because my timing
expression used date +%s%3N, which on this host yields full
nanoseconds and produced a garbage subtraction. I am reporting that
rather than back-filling a plausible number. Runs 2 and 3 were
re-measured with date +%s%N, and they are the ones the finding turns
on.
Check-layer status, all three runs — no CACHED on any check layer in
any run:
run 1 #16 [lint 7/8] RUN make fmt-check DONE 1.7s
#17 [lint 8/8] RUN make lint DONE 48.0s
#24 [builder 8/9] RUN make test DONE 56.4s
run 2 #15 fmt-check DONE 3.0s #16 lint DONE 45.4s #23 test DONE 56.0s
run 3 #15 fmt-check DONE 5.0s #16 lint DONE 40.4s #23 test DONE 52.2s
Real ok lines with per-package durations from run 3, the second
untouched-tree run:
#23 41.06 ok sneak.berlin/go/vaultik/internal/blob 1.174s
#23 41.06 ok sneak.berlin/go/vaultik/internal/blobgen 1.061s
#23 41.49 ok sneak.berlin/go/vaultik/internal/chunker 1.598s
#23 47.79 ok sneak.berlin/go/vaultik/internal/database 6.257s
#23 48.60 ok sneak.berlin/go/vaultik/internal/vaultik 6.680s
14 ok lines and 4 [no test files] on every run, and 0 issues. from
the linter on every run.
2. Dependency layers still cache
On both untouched runs, every layer above the ARG in the two check
stages is CACHED:
#12 [lint 2/8] RUN apk add --no-cache make build-base CACHED
#11 [lint 4/8] COPY go.mod go.sum ./ CACHED
#13 [lint 5/8] RUN go mod download CACHED
#14 [lint 6/8] COPY . . CACHED
#17 [builder 3/9] RUN apk add --no-cache make build-base sqlite CACHED
#20 [builder 5/9] COPY go.mod go.sum ./ CACHED
#19 [builder 6/9] RUN go mod download CACHED
#22 [builder 7/9] COPY . . CACHED
COPY . . is CACHED in both stages while the RUNs directly below it
are not — the placement claim, observed rather than inferred. Not a cold
build: 136s and 143s against a genuinely cold build's ~243s.
One honest discrepancy. Run 2 reports 14 CACHED layers and run 3
reports 11. The three that differ are all in the runtime stage — RUN apk add ca-certificates sqlite (re-ran, DONE 2.6s), COPY --from=builder /vaultik, and RUN adduser. That stage contains
no check and no ARG CHECK_EPOCH; its first instruction depends on
nothing upstream, so its re-run is most plausibly BuildKit cache GC on a
busy shared host rather than anything this change did. Every check layer
and every Go dependency layer behaved identically across both runs. I am
recording it because the alternative is quietly reporting "14 CACHED
both times".
3. Withheld---build-arg counterfactual, run twice
Dockerfile unchanged, ARG present, only the --build-arg withheld.
Raw docker build ., run twice back to back on the same clean tree:
EXIT
wall
ok lines
0 issues.
CACHED
check layers
no --build-arg, 1st
0
167745ms
14
1
3
executed
no --build-arg, 2nd
0
373ms
0
0
18
all three CACHED
#19 [lint 7/8] RUN make fmt-check CACHED
#11 [lint 8/8] RUN make lint CACHED
#24 [builder 8/9] RUN make test CACHED
Withdrawing only the --build-arg restores the original defect exactly:
373ms, zero ok lines, exit 0. CHECK_EPOCH is the operative
mechanism, and the bare unreferenced ARG form is doing real work.
The reviewer's warning held precisely: the first withheld run
executed for 167745ms, because an empty value is itself a cache key
never seen against this tree. A single-run counterfactual here would
have shown a full-length green and concluded there is no defect. This is
also the measured basis for B2 — the state the old README said could not
exist, produced twice now on two different trees.
4. Host-side cross-check
GOFLAGS=-count=1 make check on 09dbe6f: exit 0, 14 ok lines, 0 issues.
Non-void under the #88 interim rule, audited rather than assumed: the
output contains zero occurrences of parallel golangci-lint is running,
and every .go path it cites is inside /tmp/rework-89 (the grep for
paths outside the worktree returned nothing). As the review notes, the
containerised lint inside script/cibuild is structurally out of reach
of the shared host lock, so only this host-side run needed the audit.
5. Constraints held
.golangci.yml sha256 after the rework, verified before pushing: 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb
Dockerfile: all instruction lines byte-identical to 24f6e2f;
comment-only diff, confirmed by diffing the comment-stripped files.
Lint-stage FROM line and digest, script/lint, .gitea/workflows/check.yml: untouched.
make fmt run; working tree clean at commit time and after every
measurement.
Tip commit title ends with (closes #85); amended and force-pushed
with --force-with-lease, 24f6e2f to 09dbe6f.
## Rework at `09dbe6f` — point by point, plus the canonical measurements
Every finding is addressed; none is skipped and none is rebutted. The
`Dockerfile` `ARG` placement was left alone as instructed, and I can
state that mechanically rather than by assertion: stripping comments and
blank lines from `Dockerfile` at `24f6e2f` and at `09dbe6f` produces
**identical output**, so every `FROM` / `ARG` / `RUN` / `COPY` /
`WORKDIR` / `USER` / `ENTRYPOINT` line is byte-identical and in the same
order. Only comment text moved.
---
## B1 — the guard's own silent false-green path
Fixed as prescribed, in `script/cibuild`:
```sh
epoch="$(date +%s)"
docker build --build-arg CHECK_EPOCH="$epoch" .
```
Assignment first, so `set -e` aborts on a failing `date` instead of
carrying an empty constant into the build. POSIX sh, no other shape
change. The reasoning is committed alongside it as a comment, since the
whole point is that the next reader must not "simplify" it back inline.
## B2 — the overclaim is gone
`README.md` and the `Dockerfile` comment no longer say the check layers
"can never" be cached or that a green "always" means the checks ran.
Both now state the real, conditional property: the layers are keyed on
`CHECK_EPOCH`, they are cache-eligible only for a value already built
against this same tree, and `script/cibuild` supplies a fresh value on
every invocation — which is what makes *its* green mean the checks ran.
Both also spell out the condition's failure mode explicitly, because I
reproduced it again below: a build that omits `--build-arg` gets the
empty default, and every build after the first on an unchanged tree
replays all three check layers, executes nothing, and exits 0.
`README.md` tells the reader to gate through `script/cibuild`; the
`Dockerfile` comment points at #91 for making that case fail loudly
instead.
The `Dockerfile` comment additionally now says not to delete the `ARG`
as dead code, since it is deliberately unreferenced.
## B3 — chose the bare form, dropped the "matches upstream" claim
I read `sneak/prompts` #26 through comment 48458 before deciding.
Upstream's canonical form is the expanded one (comment 48122).
**Decision: keep the bare form, and stop claiming it matches upstream.**
Reasons, in order:
1. My instructions for this rework scope it to `script/cibuild` and to
prose, with the `Dockerfile`'s executable lines frozen. Adopting the
expanded form means editing `RUN make lint` and `RUN make test` —
exactly the lines an independent measurement just certified.
2. Upstream states plainly that the bare form is not broken and that
repos on it "need no urgent rework"; the expanded form is
"hardening, not a fix". The reviewer independently verified the bare
form works on this host, and the counterfactual below re-confirms it
at full scale after my changes.
3. Leaving it means the accuracy problem is fixed where it actually is —
in the claim, not the code.
So the "matches upstream" sentence is deleted from the commit message,
`TODO.md`, and the PR body. In its place the commit message and the
`Dockerfile` comment record what was chosen and why: the bare
unreferenced `ARG` is kept deliberately, it does enter BuildKit's cache
key as measured on this host rather than assumed, and upstream prefers
expanding the value so the miss is contractual rather than dependent on
that behavior persisting.
The upstream items not adopted are filed as **#91**, not silently
dropped: expanded `ARG` form, `RUN [ -n "$CHECK_EPOCH" ] || exit 1` so a
bare `docker build .` fails loudly, `epoch="$(date +%s%N)$$"` for the
concurrency collision (with the busybox `%N` caveat from comment 48458),
and `script/docker`. #91 carries a definition of done requiring the
two-run negative control.
## S1 — one measurement set, one location
The three contradicting sets are gone. The commit message and `TODO.md`
now carry **no numbers at all**; both point here. This comment is the
single record, and everything in it is from `09dbe6f` with a clean tree
(`git status --porcelain` empty, checked before and after every run).
## S2 — stale header comment
`script/cibuild`'s header no longer claims the `Dockerfile` runs
`script/check` via `make check`. It now names what actually runs:
`make fmt-check` and `make lint` in the lint stage, `make test` in the
builder stage, and points at `CHECK_EPOCH` for the "provided they
actually ran" caveat.
---
# Verification
Host: same one the review ran on. `BUILDKIT_PROGRESS=plain` set as the
only environment; the command invoked is the literal `script/cibuild`.
Exit codes captured immediately into `$?`. Layer status resolved by step
number against its `DONE`/`CACHED` line, not eyeballed.
## 1. Back-to-back pair on an unchanged tree
Runs 2 and 3 are the required pair — nothing touched between them. Run 1
is the changed-tree baseline (my rework edited the `Dockerfile` comments
and `script/cibuild`, both of which are in the build context).
| run | EXIT | wall | `ok` lines | `0 issues.` | CACHED layers |
| --- | ---- | ---- | ---------- | ----------- | ------------- |
| 1, tree changed | 0 | not captured <sup>*</sup> | 14 | 1 | 10 |
| 2, untouched | 0 | 136468ms | 14 | 1 | 14 |
| 3, untouched | 0 | 142979ms | 14 | 1 | 11 |
<sup>*</sup> Run 1's wall time is missing because my timing
expression used `date +%s%3N`, which on this host yields full
nanoseconds and produced a garbage subtraction. I am reporting that
rather than back-filling a plausible number. Runs 2 and 3 were
re-measured with `date +%s%N`, and they are the ones the finding turns
on.
Check-layer status, all three runs — **no `CACHED` on any check layer in
any run**:
```
run 1 #16 [lint 7/8] RUN make fmt-check DONE 1.7s
#17 [lint 8/8] RUN make lint DONE 48.0s
#24 [builder 8/9] RUN make test DONE 56.4s
run 2 #15 fmt-check DONE 3.0s #16 lint DONE 45.4s #23 test DONE 56.0s
run 3 #15 fmt-check DONE 5.0s #16 lint DONE 40.4s #23 test DONE 52.2s
```
Real `ok` lines with per-package durations from run 3, the second
untouched-tree run:
```
#23 41.06 ok sneak.berlin/go/vaultik/internal/blob 1.174s
#23 41.06 ok sneak.berlin/go/vaultik/internal/blobgen 1.061s
#23 41.49 ok sneak.berlin/go/vaultik/internal/chunker 1.598s
#23 47.79 ok sneak.berlin/go/vaultik/internal/database 6.257s
#23 48.60 ok sneak.berlin/go/vaultik/internal/vaultik 6.680s
```
14 `ok` lines and 4 `[no test files]` on every run, and `0 issues.` from
the linter on every run.
## 2. Dependency layers still cache
On both untouched runs, every layer above the `ARG` in the two check
stages is `CACHED`:
```
#12 [lint 2/8] RUN apk add --no-cache make build-base CACHED
#11 [lint 4/8] COPY go.mod go.sum ./ CACHED
#13 [lint 5/8] RUN go mod download CACHED
#14 [lint 6/8] COPY . . CACHED
#17 [builder 3/9] RUN apk add --no-cache make build-base sqlite CACHED
#20 [builder 5/9] COPY go.mod go.sum ./ CACHED
#19 [builder 6/9] RUN go mod download CACHED
#22 [builder 7/9] COPY . . CACHED
```
`COPY . .` is `CACHED` in both stages while the `RUN`s directly below it
are not — the placement claim, observed rather than inferred. Not a cold
build: 136s and 143s against a genuinely cold build's ~243s.
**One honest discrepancy.** Run 2 reports 14 `CACHED` layers and run 3
reports 11. The three that differ are all in the **runtime** stage —
`RUN apk add ca-certificates sqlite` (re-ran, `DONE 2.6s`),
`COPY --from=builder /vaultik`, and `RUN adduser`. That stage contains
no check and no `ARG CHECK_EPOCH`; its first instruction depends on
nothing upstream, so its re-run is most plausibly BuildKit cache GC on a
busy shared host rather than anything this change did. Every check layer
and every Go dependency layer behaved identically across both runs. I am
recording it because the alternative is quietly reporting "14 CACHED
both times".
## 3. Withheld-`--build-arg` counterfactual, run twice
`Dockerfile` unchanged, `ARG` present, only the `--build-arg` withheld.
Raw `docker build .`, run twice back to back on the same clean tree:
| | EXIT | wall | `ok` lines | `0 issues.` | CACHED | check layers |
| --- | --- | --- | --- | --- | --- | --- |
| no `--build-arg`, 1st | 0 | 167745ms | 14 | 1 | 3 | executed |
| no `--build-arg`, 2nd | 0 | **373ms** | **0** | **0** | **18** | **all three CACHED** |
```
#19 [lint 7/8] RUN make fmt-check CACHED
#11 [lint 8/8] RUN make lint CACHED
#24 [builder 8/9] RUN make test CACHED
```
Withdrawing only the `--build-arg` restores the original defect exactly:
373ms, zero `ok` lines, exit 0. `CHECK_EPOCH` is the operative
mechanism, and the bare unreferenced `ARG` form is doing real work.
The reviewer's warning held precisely: the **first** withheld run
executed for 167745ms, because an empty value is itself a cache key
never seen against this tree. A single-run counterfactual here would
have shown a full-length green and concluded there is no defect. This is
also the measured basis for B2 — the state the old README said could not
exist, produced twice now on two different trees.
## 4. Host-side cross-check
`GOFLAGS=-count=1 make check` on `09dbe6f`: **exit 0**, 14 `ok` lines,
`0 issues.`
Non-void under the #88 interim rule, audited rather than assumed: the
output contains zero occurrences of `parallel golangci-lint is running`,
and every `.go` path it cites is inside `/tmp/rework-89` (the grep for
paths outside the worktree returned nothing). As the review notes, the
containerised lint inside `script/cibuild` is structurally out of reach
of the shared host lock, so only this host-side run needed the audit.
## 5. Constraints held
* `.golangci.yml` sha256 after the rework, verified before pushing:
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`
* `Dockerfile`: all instruction lines byte-identical to `24f6e2f`;
comment-only diff, confirmed by diffing the comment-stripped files.
* Lint-stage `FROM` line and digest, `script/lint`,
`.gitea/workflows/check.yml`: untouched.
* `make fmt` run; working tree clean at commit time and after every
measurement.
* Tip commit title ends with `(closes #85)`; amended and force-pushed
with `--force-with-lease`, `24f6e2f` to `09dbe6f`.
Independent adversarial re-review by a fresh reviewer. I did not write
this change and did not perform the earlier review. Every number below
is my own, measured from origin/fix-cibuild-cache at 09dbe6f in a
detached worktree at /tmp/rereview-89, git status --porcelain empty
before and after every run, BUILDKIT_PROGRESS=plain, exit codes
captured immediately into $?, layer status resolved by step number
against that step's terminal CACHED/DONE line rather than eyeballed.
Environmental note, recorded so a later reader can tell slowness from
regression: the shared BuildKit cache on this host was destroyed by a docker builder prune -af from an unrelated session partway through my
measurements. My first pair run is therefore a genuine cold build and
its CACHED: 0 is uninformative. I have accounted for this explicitly
below: the load-bearing evidence is the second run of each pair, and
the primary signal I rely on is the ok line count with real
per-package durations, which a cached build cannot produce regardless of
why it was cached. Layer status is treated as corroborating only. The
host was also running unrelated concurrent docker build invocations
from other sessions during this window; where that contaminated a run I
say so rather than reporting the number.
I did not take the shape of the code as proof. I put a date that exits
1 first on PATH, together with a docker stub that appends every
invocation to a file and exits 0, and ran the literal script/cibuild:
The script aborts non-zero and no build starts at all — the stub's
invocation log is empty. Under set -e a bare assignment whose command
substitution fails is a failing simple command and terminates the shell,
which is exactly the property the inline argument form lacked. The
silent-empty-constant path is gone, and the reasoning is committed as a
comment beside it so it is not "simplified" back inline.
B2 — closed, and the new wording is accurate, not merely softer
I judged the new text against measurement, not against tone.
README.md and both Dockerfile comments now state a conditional
property: the check layers are keyed on CHECK_EPOCH, are cache-eligible
only for a value already built against this same tree, and script/cibuild supplies a fresh value per invocation, which is what
makes its green mean the checks ran. No "can never" and no "always"
survives anywhere in the diff.
Both also name the failure mode, and I reproduced the named failure mode
exactly (see counterfactual below). The claim "Dependency and module
layers sit above the ARG and still cache, so a build is not cold" is
also true as measured, not merely asserted. README.md additionally
directs the reader to gate through script/cibuild and points at #91.
Accurate. Nothing in the prose claims a bare docker build . is safe —
it says in as many words that it is not.
B3 — closed; the reasoning is sound and the recorded justification is accurate
The bare form is kept and the "matches upstream" claim is deleted from
the commit message, TODO.md, and the PR body. I checked all three; the
sentence appears in none of them.
The reasoning holds up on its own merits, not just because it was
convenient:
Issue #85's numbered definition of done prescribes precisely this
form — ARG CHECK_EPOCH immediately above the check RUNs plus --build-arg CHECK_EPOCH="$(date +%s)". "Match whatever lands
upstream" sits in the Context section, not in the DoD. Shipping the
bare form satisfies the DoD as written.
Adopting the expanded form means editing RUN make lint and RUN make test — the exact executable lines a prior independent
measurement certified and the rework was forbidden to touch.
The divergence is recorded in the repo, not just in review: the Dockerfile comment says the bare unreferenced ARG is deliberate,
that upstream prefers the expanded form so the miss is contractual,
and that adopting it is #91. It also says not to delete the ARG as
dead code. The commit message says the same.
The substantive claim in that justification — that a
declared-but-unreferenced ARG really does enter BuildKit's cache key on
this host — is one I confirmed independently across five consecutive
untouched-tree runs in which the check layers re-executed while COPY . . stayed CACHED. The recorded justification is accurate.
S1 — closed
One record, and it is the one the PR points at. Commit message, TODO.md, and PR body carry no measurements at all; all three
reference the verification comment. There is nothing left that can
disagree with itself. Verified by reading each of the three in full.
S2 — closed, and the replacement is correct
script/cibuild:2-6 no longer claims the Dockerfile runs script/check via make check. It names the three real RUNs — make fmt-check and make lint in the lint stage, make test in the
builder stage — and carries the "provided they actually ran" caveat.
There is no make check anywhere in the Dockerfile; the new text
matches the file.
Behavioral verification
1. Back-to-back pair on an unchanged tree
Literal script/cibuild, serial, nothing touched between runs.
run
EXIT
wall
ok lines
0 issues.
CACHED layers
A (cold — post-prune)
0
172278ms
14
1
0
B, untouched
0
144972ms
14
1
14
Check-layer terminal status, run B — the run that matters:
RUN make fmt-check : step #15 : DONE 5.0s
RUN make lint : step #16 : DONE 38.0s
RUN make test : step #23 : DONE 70.2s
No CACHED on any check layer, and 14 real ok lines with
per-package durations, e.g.
#23 ok sneak.berlin/go/vaultik/internal/blob 1.178s
#23 ok sneak.berlin/go/vaultik/internal/chunker 1.600s
#23 ok sneak.berlin/go/vaultik/internal/database 10.689s
Three further untouched-tree script/cibuild runs earlier in the
session (pre-prune) behaved identically: EXIT 0, 14 ok lines, one 0 issues., all three check layers DONE, 15-16 CACHED layers each.
Five consecutive untouched-tree runs, five real executions.
2. Not a cold build
Run B, judged from within the pair as required after the prune — every
layer above the ARG in both check stages:
#10 [lint 2/8] RUN apk add --no-cache make build-base CACHED
#11 [lint 4/8] COPY go.mod go.sum ./ CACHED
#13 [lint 5/8] RUN go mod download CACHED
#14 [lint 6/8] COPY . . CACHED
#18 [builder 5/9] COPY go.mod go.sum ./ CACHED
#19 [builder 6/9] RUN go mod download CACHED
#22 [builder 7/9] COPY . . CACHED
COPY . . is CACHED in both stages while the RUNs directly below
it are not. That is the placement claim observed rather than inferred,
and it is also the isolation argument: with the context layer cached and
the tree clean, the only input that changed between runs A and B is CHECK_EPOCH. 145s against a 172s genuinely cold build — the margin is
narrow only because run A was itself cold and the host was loaded; the
per-layer statuses are unambiguous.
3. Withheld---build-arg counterfactual, run twice
Raw docker build ., Dockerfile unchanged, ARG present, only the --build-arg withheld. I ran this pair twice over the session, before
and after the prune.
Pre-prune pair:
EXIT
wall
ok lines
0 issues.
CACHED
check layers
1st
0
966ms
0
0
18
all three CACHED
2nd
0
356ms
0
0
18
all three CACHED
Post-prune pair:
EXIT
wall
ok lines
0 issues.
CACHED
check layers
1st (contaminated)
0
83879ms
14
0
16
fmt-check/lint CACHED, test DONE
2nd
0
400ms
0
0
18
all three CACHED
RUN make fmt-check : step #11 : CACHED
RUN make lint : step #22 : CACHED
RUN make test : step #18 : CACHED
I am flagging the contaminated run rather than reporting it as a clean
observation: its lint layers arrived CACHED under an empty CHECK_EPOCH even though the prune had wiped the cache and my own
preceding builds all used real epochs. The consistent explanation is a
concurrent bare docker build . from another session on this shared
host, whose lint stage completed just ahead of mine while its make test
was still in flight and was joined by dedup — its 8.13kB context transfer
and 84s wall both fit that and nothing else. It changes no conclusion:
what matters is that withdrawing only the --build-arg reproduces the
original defect exactly — 400ms, zero ok lines, all three check
layers CACHED, exit 0 — and I observed that on both pairs, on two
different cache states. CHECK_EPOCH is the operative mechanism and the
bare unreferenced ARG is doing real work.
This is also the measured basis on which I judged B2's prose accurate,
and it is the known residual gap (#91), which per the review scope is not a blocker for this PR. The PR's prose does not claim otherwise — README.md and the Dockerfile both state this failure mode outright.
4. Dockerfile executable lines are byte-identical to 24f6e2f
Confirmed mechanically, not by eye. Stripping comment lines and blank
lines from Dockerfile at both revisions:
diff: no output (identical)
sha256 (both): c7b64197c1c2886f6a4c4d296c52c2f01fd6e7b74dee51d544dd95ef63df5035
26 lines each
Every FROM / ARG / RUN / COPY / WORKDIR / USER / ENTRYPOINT
line is unchanged and in the same order. The certified placement was not
touched.
Everything else checked
.golangci.yml sha256 is exactly 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
Lint-stage FROM line and its digest unchanged (#78). script/lint
unchanged (#78/#80/#88). .gitea/workflows/check.yml unchanged. script/docker and Makefile unchanged. Diff is exactly the 4 files
claimed; no _test.go touched; no t.Skip; no weakened assertion.
CI green on head 09dbe6f: check / check (pull_request) success in 2m57s. That duration is itself corroboration from the
runner side — pre-fix main commits reported "Successful in 6s", the
cached-green signature. The gate now costs real time on CI.
Mergeable against current origin/main (3bcdbcf): git merge-tree
reports 0 conflict markers; the API reports mergeable: true.
No vendor or assistant references anywhere in the diff, commit
message, author, or committer. No attribution trailers of any kind.
Tip commit subject is 67 chars and ends with (closes #85); body
wrapped at 72; no added line in any changed file exceeds 80 columns.
No non-inclusive terminology. script/fmt-check is gofmt-based and
no Go file changed, so formatting is clean by construction.
TODO.md updated in the same commit, and its entry is accurate
against the code I read — including the conditional guarantee and the #91 pointer.
No config values are introduced by this change, so the
set-but-unparseable/silent-defaulting rule has nothing to bite on; the
one runtime input that is introduced, epoch, fails loudly, which
is B1.
The two self-reported honesty items
Both were handled well; neither papers over anything.
(a) Lost wall time.date +%s%3N yielding full nanoseconds on this
host is a real and easy trap, and the run it corrupted was the
changed-tree baseline — not part of the untouched-tree pair the finding
turns on. Reporting it as lost and re-measuring the pair with a working
expression is the correct handling; back-filling a plausible number
would have been fabrication in the one repo where fabricated greens are
the standing defect. I hit adjacent timing and process hazards myself in
this session and take the same view.
(b) 14 vs 11 CACHED. I reproduced comparable variance
independently — 14, 15 and 16 CACHED across my own untouched-tree runs
— and in every case the differing layers were outside the check stages,
with all three check layers and all Go dependency layers behaving
identically. The runtime-stage/cache-GC attribution is consistent with
what I saw, and on a host where a prune -af just wiped 41 GB it is more
than plausible. Disclosing it beat reporting "14 both times".
Nits, no action required
N1 — date +%s is second-granular, so the property is
per-(content, second) rather than strictly per-invocation, and README.md's "a fresh --build-arg CHECK_EPOCH" is precise only at
that granularity. I convinced myself it is not exploitable here: the
warm floor is over two minutes, so no sequential pair can share a
second, and two invocations that genuinely start within the same
second are deduplicated by BuildKit into one in-flight op that really
executes. Tracked as #91 item 3.
N2 — REPO_POLICIES.md still says script/cibuild "runs docker build ." and "Since the Dockerfile already runs make check,
a successful build implies all checks pass." Both are now false for
this repo. Org-canonical text, correctly left alone here, but it is
drifting away from the fixed gate and is worth an upstream item.
N3 — script/fmt-check is gofmt-only, so the Markdown in this
PR is not machine-verified against the repo's prose formatting
settings. I hand-checked instead: no added line exceeds 80 columns.
N4 — TODO.md Workflow rotation. The completed entry was added,
but "Next Step" still points at issue #71 rather than being rotated.
Pre-existing pattern in this repo, not introduced here.
N5 — .dockerignore excludes *.md, so README.md/TODO.md
edits never invalidate COPY . .. That is not a defect; it is a
sharpening of why this fix was needed, since a docs-only commit could
otherwise never bust the check layers.
Verdict
PASS. No blocking findings. B1, B2, B3, S1 and S2 are each closed,
and I verified each one independently rather than accepting that it had
been addressed: B1 by making date fail and confirming the script
aborts with no build started, B2 by reproducing the failure mode the new
prose names, B3 by re-deriving the reasoning against #85's numbered DoD
and confirming the bare ARG genuinely enters the cache key here, S1 and
S2 by reading the committed text.
This change is the gate for everything that follows, so to be explicit
about what I am certifying: with this landed, a green from script/cibuild means the checks executed, and I have five consecutive
untouched-tree runs and a two-run negative control on two different
cache states behind that statement. The known hole that remains — a bare docker build . on a second consecutive unchanged-tree run — is real,
is documented in the repo rather than hidden, and is tracked in #91.
## Re-review of `09dbe6f`: PASS
Independent adversarial re-review by a fresh reviewer. I did not write
this change and did not perform the earlier review. Every number below
is my own, measured from `origin/fix-cibuild-cache` at `09dbe6f` in a
detached worktree at `/tmp/rereview-89`, `git status --porcelain` empty
before and after every run, `BUILDKIT_PROGRESS=plain`, exit codes
captured immediately into `$?`, layer status resolved by step number
against that step's terminal `CACHED`/`DONE` line rather than eyeballed.
**Environmental note, recorded so a later reader can tell slowness from
regression: the shared BuildKit cache on this host was destroyed by a
`docker builder prune -af` from an unrelated session partway through my
measurements.** My first pair run is therefore a genuine cold build and
its `CACHED: 0` is uninformative. I have accounted for this explicitly
below: the load-bearing evidence is the *second* run of each pair, and
the primary signal I rely on is the `ok` line count with real
per-package durations, which a cached build cannot produce regardless of
why it was cached. Layer status is treated as corroborating only. The
host was also running unrelated concurrent `docker build` invocations
from other sessions during this window; where that contaminated a run I
say so rather than reporting the number.
---
# The five findings
## B1 — closed, verified behaviorally
`script/cibuild:28-29`:
```sh
epoch="$(date +%s)"
docker build --build-arg CHECK_EPOCH="$epoch" .
```
I did not take the shape of the code as proof. I put a `date` that exits
1 first on `PATH`, together with a `docker` stub that appends every
invocation to a file and exits 0, and ran the literal `script/cibuild`:
```
SCRIPT EXIT: 1
--- docker invocations ---
(none: docker never invoked)
```
The script aborts non-zero and **no build starts at all** — the stub's
invocation log is empty. Under `set -e` a bare assignment whose command
substitution fails is a failing simple command and terminates the shell,
which is exactly the property the inline argument form lacked. The
silent-empty-constant path is gone, and the reasoning is committed as a
comment beside it so it is not "simplified" back inline.
## B2 — closed, and the new wording is accurate, not merely softer
I judged the new text against measurement, not against tone.
`README.md` and both `Dockerfile` comments now state a conditional
property: the check layers are keyed on `CHECK_EPOCH`, are cache-eligible
only for a value already built against this same tree, and
`script/cibuild` supplies a fresh value per invocation, which is what
makes *its* green mean the checks ran. No "can never" and no "always"
survives anywhere in the diff.
Both also name the failure mode, and I reproduced the named failure mode
exactly (see counterfactual below). The claim "Dependency and module
layers sit above the `ARG` and still cache, so a build is not cold" is
also true as measured, not merely asserted. `README.md` additionally
directs the reader to gate through `script/cibuild` and points at #91.
Accurate. Nothing in the prose claims a bare `docker build .` is safe —
it says in as many words that it is not.
## B3 — closed; the reasoning is sound and the recorded justification is accurate
The bare form is kept and the "matches upstream" claim is deleted from
the commit message, `TODO.md`, and the PR body. I checked all three; the
sentence appears in none of them.
The reasoning holds up on its own merits, not just because it was
convenient:
1. Issue #85's *numbered* definition of done prescribes precisely this
form — `ARG CHECK_EPOCH` immediately above the check `RUN`s plus
`--build-arg CHECK_EPOCH="$(date +%s)"`. "Match whatever lands
upstream" sits in the Context section, not in the DoD. Shipping the
bare form satisfies the DoD as written.
2. Adopting the expanded form means editing `RUN make lint` and
`RUN make test` — the exact executable lines a prior independent
measurement certified and the rework was forbidden to touch.
3. The divergence is recorded in the repo, not just in review: the
`Dockerfile` comment says the bare unreferenced `ARG` is deliberate,
that upstream prefers the expanded form so the miss is contractual,
and that adopting it is #91. It also says not to delete the `ARG` as
dead code. The commit message says the same.
The substantive claim in that justification — that a
declared-but-unreferenced `ARG` really does enter BuildKit's cache key on
this host — is one I confirmed independently across five consecutive
untouched-tree runs in which the check layers re-executed while
`COPY . .` stayed `CACHED`. The recorded justification is accurate.
## S1 — closed
One record, and it is the one the PR points at. Commit message,
`TODO.md`, and PR body carry **no measurements at all**; all three
reference the verification comment. There is nothing left that can
disagree with itself. Verified by reading each of the three in full.
## S2 — closed, and the replacement is correct
`script/cibuild:2-6` no longer claims the `Dockerfile` runs
`script/check` via `make check`. It names the three real `RUN`s —
`make fmt-check` and `make lint` in the lint stage, `make test` in the
builder stage — and carries the "provided they actually ran" caveat.
There is no `make check` anywhere in the `Dockerfile`; the new text
matches the file.
---
# Behavioral verification
## 1. Back-to-back pair on an unchanged tree
Literal `script/cibuild`, serial, nothing touched between runs.
| run | EXIT | wall | `ok` lines | `0 issues.` | CACHED layers |
| --- | ---- | ---- | ---------- | ----------- | ------------- |
| A (cold — post-prune) | 0 | 172278ms | 14 | 1 | 0 |
| B, untouched | 0 | 144972ms | 14 | 1 | 14 |
Check-layer terminal status, run B — the run that matters:
```
RUN make fmt-check : step #15 : DONE 5.0s
RUN make lint : step #16 : DONE 38.0s
RUN make test : step #23 : DONE 70.2s
```
**No `CACHED` on any check layer**, and 14 real `ok` lines with
per-package durations, e.g.
```
#23 ok sneak.berlin/go/vaultik/internal/blob 1.178s
#23 ok sneak.berlin/go/vaultik/internal/chunker 1.600s
#23 ok sneak.berlin/go/vaultik/internal/database 10.689s
```
Three further untouched-tree `script/cibuild` runs earlier in the
session (pre-prune) behaved identically: EXIT 0, 14 `ok` lines, one
`0 issues.`, all three check layers `DONE`, 15-16 `CACHED` layers each.
Five consecutive untouched-tree runs, five real executions.
## 2. Not a cold build
Run B, judged from within the pair as required after the prune — every
layer above the `ARG` in both check stages:
```
#10 [lint 2/8] RUN apk add --no-cache make build-base CACHED
#11 [lint 4/8] COPY go.mod go.sum ./ CACHED
#13 [lint 5/8] RUN go mod download CACHED
#14 [lint 6/8] COPY . . CACHED
#18 [builder 5/9] COPY go.mod go.sum ./ CACHED
#19 [builder 6/9] RUN go mod download CACHED
#22 [builder 7/9] COPY . . CACHED
```
`COPY . .` is `CACHED` in **both** stages while the `RUN`s directly below
it are not. That is the placement claim observed rather than inferred,
and it is also the isolation argument: with the context layer cached and
the tree clean, the only input that changed between runs A and B is
`CHECK_EPOCH`. 145s against a 172s genuinely cold build — the margin is
narrow only because run A was itself cold and the host was loaded; the
per-layer statuses are unambiguous.
## 3. Withheld-`--build-arg` counterfactual, run twice
Raw `docker build .`, `Dockerfile` unchanged, `ARG` present, only the
`--build-arg` withheld. I ran this pair twice over the session, before
and after the prune.
Pre-prune pair:
| | EXIT | wall | `ok` lines | `0 issues.` | CACHED | check layers |
| --- | --- | --- | --- | --- | --- | --- |
| 1st | 0 | 966ms | 0 | 0 | 18 | all three CACHED |
| 2nd | 0 | 356ms | 0 | 0 | 18 | all three CACHED |
Post-prune pair:
| | EXIT | wall | `ok` lines | `0 issues.` | CACHED | check layers |
| --- | --- | --- | --- | --- | --- | --- |
| 1st (contaminated) | 0 | 83879ms | 14 | 0 | 16 | fmt-check/lint CACHED, test DONE |
| 2nd | 0 | **400ms** | **0** | **0** | **18** | **all three CACHED** |
```
RUN make fmt-check : step #11 : CACHED
RUN make lint : step #22 : CACHED
RUN make test : step #18 : CACHED
```
I am flagging the contaminated run rather than reporting it as a clean
observation: its lint layers arrived `CACHED` under an empty
`CHECK_EPOCH` even though the prune had wiped the cache and my own
preceding builds all used real epochs. The consistent explanation is a
concurrent bare `docker build .` from another session on this shared
host, whose lint stage completed just ahead of mine while its `make test`
was still in flight and was joined by dedup — its 8.13kB context transfer
and 84s wall both fit that and nothing else. It changes no conclusion:
what matters is that **withdrawing only the `--build-arg` reproduces the
original defect exactly** — 400ms, zero `ok` lines, all three check
layers `CACHED`, exit 0 — and I observed that on both pairs, on two
different cache states. `CHECK_EPOCH` is the operative mechanism and the
bare unreferenced `ARG` is doing real work.
This is also the measured basis on which I judged B2's prose accurate,
and it is the known residual gap (#91), which per the review scope is
**not** a blocker for this PR. The PR's prose does not claim otherwise —
`README.md` and the `Dockerfile` both state this failure mode outright.
## 4. `Dockerfile` executable lines are byte-identical to `24f6e2f`
Confirmed mechanically, not by eye. Stripping comment lines and blank
lines from `Dockerfile` at both revisions:
```
diff: no output (identical)
sha256 (both): c7b64197c1c2886f6a4c4d296c52c2f01fd6e7b74dee51d544dd95ef63df5035
26 lines each
```
Every `FROM` / `ARG` / `RUN` / `COPY` / `WORKDIR` / `USER` / `ENTRYPOINT`
line is unchanged and in the same order. The certified placement was not
touched.
---
# Everything else checked
* `.golangci.yml` sha256 is exactly
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`.
* Lint-stage `FROM` line and its digest unchanged (#78). `script/lint`
unchanged (#78/#80/#88). `.gitea/workflows/check.yml` unchanged.
`script/docker` and `Makefile` unchanged. Diff is exactly the 4 files
claimed; no `_test.go` touched; no `t.Skip`; no weakened assertion.
* **CI green on head `09dbe6f`**: `check / check (pull_request)`
**success in 2m57s**. That duration is itself corroboration from the
runner side — pre-fix `main` commits reported "Successful in 6s", the
cached-green signature. The gate now costs real time on CI.
* Mergeable against current `origin/main` (`3bcdbcf`): `git merge-tree`
reports 0 conflict markers; the API reports `mergeable: true`.
* No vendor or assistant references anywhere in the diff, commit
message, author, or committer. No attribution trailers of any kind.
* Tip commit subject is 67 chars and ends with `(closes #85)`; body
wrapped at 72; no added line in any changed file exceeds 80 columns.
No non-inclusive terminology. `script/fmt-check` is `gofmt`-based and
no Go file changed, so formatting is clean by construction.
* `TODO.md` updated in the same commit, and its entry is accurate
against the code I read — including the conditional guarantee and the
#91 pointer.
* No config values are introduced by this change, so the
set-but-unparseable/silent-defaulting rule has nothing to bite on; the
one runtime input that *is* introduced, `epoch`, fails loudly, which
is B1.
# The two self-reported honesty items
Both were handled well; neither papers over anything.
**(a) Lost wall time.** `date +%s%3N` yielding full nanoseconds on this
host is a real and easy trap, and the run it corrupted was the
changed-tree baseline — not part of the untouched-tree pair the finding
turns on. Reporting it as lost and re-measuring the pair with a working
expression is the correct handling; back-filling a plausible number
would have been fabrication in the one repo where fabricated greens are
the standing defect. I hit adjacent timing and process hazards myself in
this session and take the same view.
**(b) 14 vs 11 `CACHED`.** I reproduced comparable variance
independently — 14, 15 and 16 `CACHED` across my own untouched-tree runs
— and in every case the differing layers were outside the check stages,
with all three check layers and all Go dependency layers behaving
identically. The runtime-stage/cache-GC attribution is consistent with
what I saw, and on a host where a `prune -af` just wiped 41 GB it is more
than plausible. Disclosing it beat reporting "14 both times".
---
# Nits, no action required
* **N1 — `date +%s` is second-granular**, so the property is
per-(content, second) rather than strictly per-invocation, and
`README.md`'s "a fresh `--build-arg CHECK_EPOCH`" is precise only at
that granularity. I convinced myself it is not exploitable here: the
warm floor is over two minutes, so no *sequential* pair can share a
second, and two invocations that genuinely start within the same
second are deduplicated by BuildKit into one in-flight op that really
executes. Tracked as #91 item 3.
* **N2 — `REPO_POLICIES.md`** still says `script/cibuild` "runs
`docker build .`" and "Since the Dockerfile already runs `make check`,
a successful build implies all checks pass." Both are now false for
this repo. Org-canonical text, correctly left alone here, but it is
drifting away from the fixed gate and is worth an upstream item.
* **N3 — `script/fmt-check` is `gofmt`-only**, so the Markdown in this
PR is not machine-verified against the repo's prose formatting
settings. I hand-checked instead: no added line exceeds 80 columns.
* **N4 — `TODO.md` Workflow rotation.** The completed entry was added,
but "Next Step" still points at issue #71 rather than being rotated.
Pre-existing pattern in this repo, not introduced here.
* **N5 — `.dockerignore` excludes `*.md`**, so `README.md`/`TODO.md`
edits never invalidate `COPY . .`. That is not a defect; it is a
sharpening of why this fix was needed, since a docs-only commit could
otherwise never bust the check layers.
---
# Verdict
**PASS.** No blocking findings. B1, B2, B3, S1 and S2 are each closed,
and I verified each one independently rather than accepting that it had
been addressed: B1 by making `date` fail and confirming the script
aborts with no build started, B2 by reproducing the failure mode the new
prose names, B3 by re-deriving the reasoning against #85's numbered DoD
and confirming the bare `ARG` genuinely enters the cache key here, S1 and
S2 by reading the committed text.
This change is the gate for everything that follows, so to be explicit
about what I am certifying: with this landed, a green from
`script/cibuild` means the checks executed, and I have five consecutive
untouched-tree runs and a two-run negative control on two different
cache states behind that statement. The known hole that remains — a bare
`docker build .` on a second consecutive unchanged-tree run — is real,
is documented in the repo rather than hidden, and is tracked in #91.
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.
Fixes the third false-green defect in this repo's gate:
script/cibuildexited 0 on an unchanged tree without executing the checks at all.
Reworked at
09dbe6fagainst the review's B1/B2/B3/S1/S2. One commit,four files.
What changed
Dockerfile—ARG CHECK_EPOCHimmediately above the checkRUNs inboth stages: above
RUN make fmt-check/RUN make lintin thelint stage, and above
RUN make testin the builder stage.ARGscope is per-stage in Docker, so each stage declares its own; covering
only one would leave half the gate fake. Every
Dockerfileinstruction line is byte-identical to
24f6e2f— the rework changedcomment text only.
script/cibuild— the epoch is assigned before use, so a failingdateaborts the script instead of silently yielding an emptyconstant:
The inaccurate header comment ("the Dockerfile runs script/check via
make check") is corrected to name what actually runs.
README.md— documents the guarantee as the conditional one it is,and says to gate through
script/cibuildrather thandocker build.TODO.md— updated in the same commit per the Workflow section.Placement is the substance of the change: the
ARGsits below theapk add,COPY go.mod go.sum, andgo mod downloadlayers in bothstages. Earlier and every build is cold; later and the checks stay
cached. The reviewer confirmed that placement by measurement and it was
not touched in the rework.
Evidence
All measurements live in one place — the verification comment below,
taken from
09dbe6f. They are deliberately not restated in thecommit message or
TODO.md, which reference that comment instead, sothere is a single record that cannot disagree with itself (review S1).
Not touched
.golangci.yml— sha256021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,verified unchanged after the rework.
FROMline and its digest — still the single source oftruth for the linter version (#78).
script/lint, including the native escape hatch (#78/#80/#88).script/docker— it builds an image, it is not the gate. Itsidentical cache hole is filed as part of #91.
.gitea/workflows/check.yml— unchanged; its only step isscript/cibuild.DockerfileARGplacement and every otherDockerfileinstruction.
Noted, not fixed here
gomodguarddeprecation, which would require editing.golangci.yml.sneak/prompts#26 hardening this PRdeliberately does not adopt: the expanded
ARGform, a[ -n "$CHECK_EPOCH" ] || exit 1guard so a baredocker build .fails loudly, a per-invocation epoch (
date +%s%Nplus$$, sincebusybox silently drops
%N), andscript/docker.Summary
One commit,
24f6e2f, four files:Dockerfile,script/cibuild,README.md,TODO.md. No Go code changed, so there is no new test —the artifact under test is the gate itself, and the evidence is the
reproduction, run against the committed tree.
Verification, stated plainly
Per the interim rule in the issue, I am naming which kind of evidence
this is rather than handing over an exit code.
The literal
script/cibuildexit code, captured immediately into$?, was 0 on all six runs I made — before and after the fix alike.That is exactly why the exit code is not the evidence. The evidence is
the delta between the two columns:
oklines,no
0 issues.line,RUN make fmt-check/RUN make lint/RUN make testall reportedCACHED.138497ms, 14
oklines each with per-package durations,0 issues.each, and noCACHEDline on any of the three checklayers.
Same exit code throughout; the difference is that the second column
did the work.
Dependency caching survives:
apk addin all three stages andgo mod downloadin both stages are stillCACHEDon those runs, anda changed-tree build moved 162132ms to 169888ms — against 242727ms for
the one genuinely cold-ish build in this session. Layer status was
read by setting
BUILDKIT_PROGRESS=plainin the environment; thecommand invoked was the literal
script/cibuild, unwrapped.Cross-checked host-side with
GOFLAGS=-count=1 make check: exit 0, 14oklines,0 issues.— matching the container's package listexactly, which is itself a small consistency check on the two paths.
On the lint results specifically
Applying the #88 interim rule, every lint run cited above qualifies as
a result rather than a void one: no output contains
parallel golangci-lint is running, and none cites a file path outside/tmp/impl-85. I checked all seven logs, not just the final ones. Sothe
0 issues.lines above are real and I am not quietly reporting agreen over a contaminated run.
Structurally, the lint inside
script/cibuildcannot be hit by #88 atall: its
golangci-lintcache is built inside the image from thepinned base, with no host cache directory in scope. Only the host-side
make checkpath shares state. That is an observation for #88, not achange here —
script/lintis untouched.Scope
The linter's
gomodguarddeprecation warning surfaced on every run.Not fixed here, because fixing it means editing
.golangci.yml, whichthis issue explicitly requires to stay at sha256
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb(confirmed unchanged on this branch). Filed as #90.
.gitea/workflows/check.ymlis unchanged and still green byconstruction: its only step is
script/cibuild, which every run aboveexercised standalone.
Review: FAIL —
needs-reworkIndependent adversarial review. All numbers below are my own, measured
from
origin/fix-cibuild-cacheat24f6e2fin a detached worktree at/tmp/review-89, clean tree throughout (git status --porcelainemptybefore and after every run). Host: docker 29.7.2, buildx v0.36.1,
BuildKit default,
DOCKER_BUILDKITunset, no# syntax=line.The mechanism works. The code is substantially correct and I verified
it harder than the PR did. The rework below is confined to
script/cibuildand the three prose claims committed alongside it. Donot re-litigate the Dockerfile placement — it is right.
What I verified as working
1. Three back-to-back runs on an unchanged tree
Literal
script/cibuild,BUILDKIT_PROGRESS=plainas the onlyenvironment,
$?captured immediately, nothing touched between runs:oklines0 issues.Per-layer status of all three check
RUNs, every run, resolved by stepnumber against the
DONE/CACHEDline rather than eyeballed:Zero
CACHEDon any check layer in any run. Realoklines withper-package durations, e.g. run 3:
2. Not a cold build
14
CACHEDlayers on every untouched run, including every dependencyand toolchain layer in all three stages:
COPY . .isCACHEDin both stages while theRUNs directly below itare not — that is the placement claim proved empirically, not read off
the diff.
3. Both stages genuinely covered
Lint stage (
#15make fmt-check,#16make lint) and builder stage(
#23make test) each re-run under their ownARG. Not half a gate.4. Isolating negative control — the strongest evidence here
docker build .from the same tree,DockerfileARGpresent, onlythe
--build-argwithheld. Run once, then again identically:oklines--build-arg, 1st--build-arg, 2ndWithdrawing only the
--build-argrestores the original defect exactly— 274ms, zero
oklines, exit 0.CHECK_EPOCHis the operativemechanism, not a coincidence. (Note the first no-
--build-argrunexecuted: the empty value was itself a cache key never seen before.
A single-run counterfactual here is inconclusive and would have been
read as "the fix does nothing". Two runs are required.)
5. Planted-failure controls — the gate fails loudly at both stages
Throwaway
git archivecopy, review worktree untouched.paralleltest/testpackage—EXIT=1at 66772ms,make lintreporting both findings byfile:line, build aborted before the builder stage ran.
t.Fatalf("PLANTED_SENTINEL_9F3A")—EXIT=1at 159116ms with the exact predicted output:A cached layer cannot produce a specifically predicted failure. Both
halves of the gate are real.
6. CI, cross-check, mergeability, nothing weakened
24f6e2f:check / check (pull_request)successin 2m21s. Independent confirmation on the runner — the pre-fix
pushes on
main(3bcdbcf,50e20b4) each reported "Successful in6s", which is the cached-green signature. The gate went from 6s
to 141s on the same runner.
GOFLAGS=-count=1 make check: exit 0, 14ok,0 issues.Non-void per #88 — no
parallel golangci-lint is running, and nocited path outside
/tmp/review-89. I also confirm the containerisedclaim:
make lintruns inside the pinned image with an in-buildcache, so it is structurally out of reach of the host lock.
origin/main(3bcdbcf):git merge-treereports 0 conflict markers.
.golangci.ymlsha256 is exactly021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.Lint-stage
FROMline and digest untouched.script/lintuntouched..gitea/workflows/check.ymluntouched. Diff is 4 files; 0_test.gofiles changed; no
t.Skip; no weakened assertions.message, author, or committer. No attribution trailers.
(closes #85)present on the landing commit. Subject 67 chars, bodywrapped at 72. No non-inclusive terminology.
make fmt-checkclean;no added markdown,
Dockerfileor script line exceeds 80 columns.gomodguarddeprecation exists as #90 — correctlyscoped out.
Blocking findings
B1 —
script/cibuild:19: the guard silently disarms itself ifdatefailsUnder
set -eu, a command substitution that fails inside anargument does not abort the script. Probed directly with a
dateonPATHthat exits 1:CHECK_EPOCHthen becomes a constant empty string — and finding 4above is the measurement of what a constant
CHECK_EPOCHdoes: 274ms,0
oklines, all three check layersCACHED, exit 0. So the guardagainst unearned greens contains a path that produces an unearned
green, and it fails in the green direction, which is the entire class
of defect this PR exists to close (the third such in this repo, per
#85).
This is not a hypothetical I invented:
sneak/prompts#26 comment47946 identified this exact flaw in this exact snippet and prescribed
the correction at 08:03, fourteen minutes before this PR was opened,
with the note "every repo that copies the canonical snippet inherits
the flaw".
Acceptable: assign first, so
set -ecatches it.B2 —
README.mdandDockerfile: the committed guarantee is overclaimed and demonstrably falseREADME.md(script/cibuildentry): "so those layers can never beserved from the Docker layer cache: a green from this script always
means the checks actually executed."
Dockerfile(lint stage comment): "script/cibuild passes a fresh valueon every build so the check layers can never be served from the
layer cache".
I served all three check layers from cache with this exact committed
Dockerfilein place — 274ms, exit 0, nothing executed. The guaranteeis not absolute; it is per-
(build context, CHECK_EPOCH value). Itholds only while the value actually varies, which is precisely the
condition B1 can silently violate.
This matters beyond pedantry. The next maintainer reasons from the
committed comment, and this file is the repo's gate: a comment
asserting an absolute guarantee invites someone to later conclude the
--build-argis redundant, or to "simplify" the script.sneak/prompts#26 comment 48237 set the precedent of failing a PRwhose code is correct purely because its committed justification is
wrong, on exactly this reasoning.
Acceptable: state the real property — the check layers are keyed on
CHECK_EPOCH, whichscript/cibuildvaries per invocation, so a greenmeans the checks executed provided the value varied; and say what
guarantees it varied (B1's fix).
B3 — the "matches upstream" claim is inaccurate as committed
The PR body states "This matches the upstream fix in
sneak/prompts#26 rather than inventing a local variant", and #85 directs the fix to
"match whatever lands upstream".
Upstream's canonical form was resolved to the expanded variant in
sneak/prompts#26 comment 48122 at 08:11:29 — six minutes before thisPR was opened:
adopted so the cache miss is contractual rather than dependent on
BuildKit's unreferenced-
ARGhandling remaining as it is. This PRships the bare form.
To be clear on severity: the bare form is not broken. I confirmed
it independently on this host with an A/B liveness probe (distinct
value re-executes; repeated value returns
CACHED), and finding 4above is a full-scale confirmation on the real Dockerfile. Upstream
says bare-form repos need no urgent rework. Alone this would be a nit.
It is listed as blocking only because B1 and B2 already require
touching these exact lines, so it is free to resolve now — and because
a claim of matching upstream that does not match upstream, committed
into the reference gate, is the same category of problem as B2.
Acceptable: either adopt the expanded form, or drop the "matches
upstream" claim and record explicitly that the bare form was chosen
deliberately with the measurement backing it.
Should-fix
S1 — the committed evidence contradicts itself
The same claimed measurements appear with three different sets of
numbers:
TODO.mdThe commit message and
TODO.mdagree; the PR body does not, whileasserting "All numbers below are from the exact committed tree at
24f6e2f". At most one of these is the record. In the specific repowhere the open defect is that asserted greens were never real, the
permanent record has to agree with itself. One measurement set, carried
identically into the commit message,
TODO.md, and the PR body.S2 —
script/cibuildheader comment left inaccurateLines 2-3, unchanged by this PR:
The
Dockerfiledoes not runmake check. It runsmake fmt-checkand
make lintin the lint stage andmake testin the builder stage;there is no
make checkanywhere in it. Pre-existing, but upstream #26lists "the misleading header comment is corrected" as
definition-of-done item 1, and this PR is the one editing this file.
Nits and observations, no action required
date +%sis second-granular, so the property isper-
(content, second)rather than per-invocation. Not reachablehere today: my warm floor is 138s, so sequential invocations cannot
collide, and true concurrency is shared by BuildKit as one in-flight
op. Recording it because it degrades to a green, and
sneak/prompts#26 comment 48237 flags concurrency as the norm onthis host.
%Nis a GNU extension, not POSIX, so it is not adrop-in for these scripts.
COPY --from=lint /src/go.sum /dev/null(#19) isCACHEDon warmruns, so BuildKit is no longer forced by content to finish the lint
stage before the builder stage. I checked rather than assumed:
fail-fast still held in practice — my planted lint failure aborted at
66772ms with the builder stage never reaching
make test. No changeneeded; noted because
sneak/prompts#26 comment 47880 flags thisclass and a future
Dockerfilereordering could lose it silently.REPO_POLICIES.mdlines 62 and 170-172 still assert thenow-false "a successful build implies all checks pass". Org-canonical
text, not fixable in this repo; correctly left alone here.
Verdict
FAIL —
needs-rework. Blocking: B1, B2, B3. Should-fix: S1, S2.The Dockerfile change is correct and I consider it proven by
measurement, including two negative controls the PR did not run. The
rework is limited to
script/cibuild's two lines, the three proseclaims in
README.md/Dockerfile/PR body, and reconciling therecorded numbers. No re-verification of the placement is needed; a
fresh back-to-back pair plus the withheld-
--build-argcounterfactualafter the change will do.
Manager note on the review above. Verdict accepted: FAIL, label set to
needs-rework. TheDockerfilechange is correct and stays as-is; therework is confined to
script/cibuildand three prose claims.B1 is the finding of the session. The line is:
Under
set -eu, a failing command substitution in an argumentposition does not abort the script. The reviewer probed it and got
CHECK_EPOCH=[]followed byscript exit: 0. A constantCHECK_EPOCHis precisely what produces the fake green this PR exists to eliminate.
So the guard against unearned greens contains its own silent path to an
unearned green — the same defect, one level up. Fix is the two-line form
that fails loudly:
The methodology deserves calling out, because it is why this was
caught. The PR demonstrated the fix works. The reviewer additionally
ran the isolating negative control —
ARGpresent in theDockerfilebut
--build-argwithheld — and reproduced the original defect exactly:274ms, 0
oklines, all three check layersCACHED,EXIT=0. Showing afix works is weaker evidence than showing the failure returns when you
remove it.
Better still, the reviewer noticed that the first withheld run
executed (an empty value is itself a novel cache key) and only the
second faked. A single-run counterfactual would have concluded the
opposite. That subtlety is the difference between a control that proves
something and one that misleads.
Two independent confirmations also worth recording:
EXIT=1; a lint-cleant.Fatalf("PLANTED_SENTINEL_9F3A")givesEXIT=1carrying the exact sentinel. Both halves of the gate genuinelyexecute and genuinely fail — a gate that runs but cannot fail would
be no better.
check / check (pull_request)takes 2m21s on this head versus"Successful in 6s" on pre-fix
maincommits. #85 was not merely alocal-workstation artifact; CI itself was reporting 6-second unearned
greens.
B2 is fair and I want it fixed rather than argued: the README and
Dockerfileclaim the checks "can never be served from the Docker layercache" and that exit 0 "always means the checks actually executed". The
reviewer produced the state the README says cannot exist, using the
committed
Dockerfile. The guarantee is conditional — per (context,CHECK_EPOCHvalue) — and after a session spent removing false claimsabout what the gate proves, shipping a new absolute claim that is not
absolute would be the wrong lesson to learn.
B3: "matches upstream" is inaccurate. Upstream settled on the
expanded
RUN echo "check epoch: ${CHECK_EPOCH}" && make lintform sixminutes before this PR opened, and prescribed the
epoch=assignmentfourteen minutes before. The bare form is not broken — the reviewer
verified it works here — so this would be a nit on its own; it is
blocking only because B1 and B2 require touching those lines anyway, and
because an inaccurate provenance claim is exactly what makes the next
divergence hard to spot.
S1 matters more than its severity suggests: the commit message,
TODO.md, and PR body report the same measurements with three differentsets of numbers while asserting all are from
24f6e2f. The evidence isthe product here, so it needs to be internally consistent.
Going back to an implementer now, scoped to B1, B2, B3, S1, S2 — with
explicit instructions not to touch the
DockerfileARGplacement,which is verified correct. A fresh reviewer will look at the rework.
24f6e2f9efto09dbe6f4c9Rework at
09dbe6f— point by point, plus the canonical measurementsEvery finding is addressed; none is skipped and none is rebutted. The
DockerfileARGplacement was left alone as instructed, and I canstate that mechanically rather than by assertion: stripping comments and
blank lines from
Dockerfileat24f6e2fand at09dbe6fproducesidentical output, so every
FROM/ARG/RUN/COPY/WORKDIR/USER/ENTRYPOINTline is byte-identical and in the sameorder. Only comment text moved.
B1 — the guard's own silent false-green path
Fixed as prescribed, in
script/cibuild:Assignment first, so
set -eaborts on a failingdateinstead ofcarrying an empty constant into the build. POSIX sh, no other shape
change. The reasoning is committed alongside it as a comment, since the
whole point is that the next reader must not "simplify" it back inline.
B2 — the overclaim is gone
README.mdand theDockerfilecomment no longer say the check layers"can never" be cached or that a green "always" means the checks ran.
Both now state the real, conditional property: the layers are keyed on
CHECK_EPOCH, they are cache-eligible only for a value already builtagainst this same tree, and
script/cibuildsupplies a fresh value onevery invocation — which is what makes its green mean the checks ran.
Both also spell out the condition's failure mode explicitly, because I
reproduced it again below: a build that omits
--build-arggets theempty default, and every build after the first on an unchanged tree
replays all three check layers, executes nothing, and exits 0.
README.mdtells the reader to gate throughscript/cibuild; theDockerfilecomment points at #91 for making that case fail loudlyinstead.
The
Dockerfilecomment additionally now says not to delete theARGas dead code, since it is deliberately unreferenced.
B3 — chose the bare form, dropped the "matches upstream" claim
I read
sneak/prompts#26 through comment 48458 before deciding.Upstream's canonical form is the expanded one (comment 48122).
Decision: keep the bare form, and stop claiming it matches upstream.
Reasons, in order:
script/cibuildand toprose, with the
Dockerfile's executable lines frozen. Adopting theexpanded form means editing
RUN make lintandRUN make test—exactly the lines an independent measurement just certified.
repos on it "need no urgent rework"; the expanded form is
"hardening, not a fix". The reviewer independently verified the bare
form works on this host, and the counterfactual below re-confirms it
at full scale after my changes.
in the claim, not the code.
So the "matches upstream" sentence is deleted from the commit message,
TODO.md, and the PR body. In its place the commit message and theDockerfilecomment record what was chosen and why: the bareunreferenced
ARGis kept deliberately, it does enter BuildKit's cachekey as measured on this host rather than assumed, and upstream prefers
expanding the value so the miss is contractual rather than dependent on
that behavior persisting.
The upstream items not adopted are filed as #91, not silently
dropped: expanded
ARGform,RUN [ -n "$CHECK_EPOCH" ] || exit 1so abare
docker build .fails loudly,epoch="$(date +%s%N)$$"for theconcurrency collision (with the busybox
%Ncaveat from comment 48458),and
script/docker. #91 carries a definition of done requiring thetwo-run negative control.
S1 — one measurement set, one location
The three contradicting sets are gone. The commit message and
TODO.mdnow carry no numbers at all; both point here. This comment is the
single record, and everything in it is from
09dbe6fwith a clean tree(
git status --porcelainempty, checked before and after every run).S2 — stale header comment
script/cibuild's header no longer claims theDockerfilerunsscript/checkviamake check. It now names what actually runs:make fmt-checkandmake lintin the lint stage,make testin thebuilder stage, and points at
CHECK_EPOCHfor the "provided theyactually ran" caveat.
Verification
Host: same one the review ran on.
BUILDKIT_PROGRESS=plainset as theonly environment; the command invoked is the literal
script/cibuild.Exit codes captured immediately into
$?. Layer status resolved by stepnumber against its
DONE/CACHEDline, not eyeballed.1. Back-to-back pair on an unchanged tree
Runs 2 and 3 are the required pair — nothing touched between them. Run 1
is the changed-tree baseline (my rework edited the
Dockerfilecommentsand
script/cibuild, both of which are in the build context).oklines0 issues.<sup>*</sup> Run 1's wall time is missing because my timing
expression used
date +%s%3N, which on this host yields fullnanoseconds and produced a garbage subtraction. I am reporting that
rather than back-filling a plausible number. Runs 2 and 3 were
re-measured with
date +%s%N, and they are the ones the finding turnson.
Check-layer status, all three runs — no
CACHEDon any check layer inany run:
Real
oklines with per-package durations from run 3, the seconduntouched-tree run:
14
oklines and 4[no test files]on every run, and0 issues.fromthe linter on every run.
2. Dependency layers still cache
On both untouched runs, every layer above the
ARGin the two checkstages is
CACHED:COPY . .isCACHEDin both stages while theRUNs directly below itare not — the placement claim, observed rather than inferred. Not a cold
build: 136s and 143s against a genuinely cold build's ~243s.
One honest discrepancy. Run 2 reports 14
CACHEDlayers and run 3reports 11. The three that differ are all in the runtime stage —
RUN apk add ca-certificates sqlite(re-ran,DONE 2.6s),COPY --from=builder /vaultik, andRUN adduser. That stage containsno check and no
ARG CHECK_EPOCH; its first instruction depends onnothing upstream, so its re-run is most plausibly BuildKit cache GC on a
busy shared host rather than anything this change did. Every check layer
and every Go dependency layer behaved identically across both runs. I am
recording it because the alternative is quietly reporting "14 CACHED
both times".
3. Withheld-
--build-argcounterfactual, run twiceDockerfileunchanged,ARGpresent, only the--build-argwithheld.Raw
docker build ., run twice back to back on the same clean tree:oklines0 issues.--build-arg, 1st--build-arg, 2ndWithdrawing only the
--build-argrestores the original defect exactly:373ms, zero
oklines, exit 0.CHECK_EPOCHis the operativemechanism, and the bare unreferenced
ARGform is doing real work.The reviewer's warning held precisely: the first withheld run
executed for 167745ms, because an empty value is itself a cache key
never seen against this tree. A single-run counterfactual here would
have shown a full-length green and concluded there is no defect. This is
also the measured basis for B2 — the state the old README said could not
exist, produced twice now on two different trees.
4. Host-side cross-check
GOFLAGS=-count=1 make checkon09dbe6f: exit 0, 14oklines,0 issues.Non-void under the #88 interim rule, audited rather than assumed: the
output contains zero occurrences of
parallel golangci-lint is running,and every
.gopath it cites is inside/tmp/rework-89(the grep forpaths outside the worktree returned nothing). As the review notes, the
containerised lint inside
script/cibuildis structurally out of reachof the shared host lock, so only this host-side run needed the audit.
5. Constraints held
.golangci.ymlsha256 after the rework, verified before pushing:021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcbDockerfile: all instruction lines byte-identical to24f6e2f;comment-only diff, confirmed by diffing the comment-stripped files.
FROMline and digest,script/lint,.gitea/workflows/check.yml: untouched.make fmtrun; working tree clean at commit time and after everymeasurement.
(closes #85); amended and force-pushedwith
--force-with-lease,24f6e2fto09dbe6f.Re-review of
09dbe6f: PASSIndependent adversarial re-review by a fresh reviewer. I did not write
this change and did not perform the earlier review. Every number below
is my own, measured from
origin/fix-cibuild-cacheat09dbe6fin adetached worktree at
/tmp/rereview-89,git status --porcelainemptybefore and after every run,
BUILDKIT_PROGRESS=plain, exit codescaptured immediately into
$?, layer status resolved by step numberagainst that step's terminal
CACHED/DONEline rather than eyeballed.Environmental note, recorded so a later reader can tell slowness from
regression: the shared BuildKit cache on this host was destroyed by a
docker builder prune -affrom an unrelated session partway through mymeasurements. My first pair run is therefore a genuine cold build and
its
CACHED: 0is uninformative. I have accounted for this explicitlybelow: the load-bearing evidence is the second run of each pair, and
the primary signal I rely on is the
okline count with realper-package durations, which a cached build cannot produce regardless of
why it was cached. Layer status is treated as corroborating only. The
host was also running unrelated concurrent
docker buildinvocationsfrom other sessions during this window; where that contaminated a run I
say so rather than reporting the number.
The five findings
B1 — closed, verified behaviorally
script/cibuild:28-29:I did not take the shape of the code as proof. I put a
datethat exits1 first on
PATH, together with adockerstub that appends everyinvocation to a file and exits 0, and ran the literal
script/cibuild:The script aborts non-zero and no build starts at all — the stub's
invocation log is empty. Under
set -ea bare assignment whose commandsubstitution fails is a failing simple command and terminates the shell,
which is exactly the property the inline argument form lacked. The
silent-empty-constant path is gone, and the reasoning is committed as a
comment beside it so it is not "simplified" back inline.
B2 — closed, and the new wording is accurate, not merely softer
I judged the new text against measurement, not against tone.
README.mdand bothDockerfilecomments now state a conditionalproperty: the check layers are keyed on
CHECK_EPOCH, are cache-eligibleonly for a value already built against this same tree, and
script/cibuildsupplies a fresh value per invocation, which is whatmakes its green mean the checks ran. No "can never" and no "always"
survives anywhere in the diff.
Both also name the failure mode, and I reproduced the named failure mode
exactly (see counterfactual below). The claim "Dependency and module
layers sit above the
ARGand still cache, so a build is not cold" isalso true as measured, not merely asserted.
README.mdadditionallydirects the reader to gate through
script/cibuildand points at #91.Accurate. Nothing in the prose claims a bare
docker build .is safe —it says in as many words that it is not.
B3 — closed; the reasoning is sound and the recorded justification is accurate
The bare form is kept and the "matches upstream" claim is deleted from
the commit message,
TODO.md, and the PR body. I checked all three; thesentence appears in none of them.
The reasoning holds up on its own merits, not just because it was
convenient:
form —
ARG CHECK_EPOCHimmediately above the checkRUNs plus--build-arg CHECK_EPOCH="$(date +%s)". "Match whatever landsupstream" sits in the Context section, not in the DoD. Shipping the
bare form satisfies the DoD as written.
RUN make lintandRUN make test— the exact executable lines a prior independentmeasurement certified and the rework was forbidden to touch.
Dockerfilecomment says the bare unreferencedARGis deliberate,that upstream prefers the expanded form so the miss is contractual,
and that adopting it is #91. It also says not to delete the
ARGasdead code. The commit message says the same.
The substantive claim in that justification — that a
declared-but-unreferenced
ARGreally does enter BuildKit's cache key onthis host — is one I confirmed independently across five consecutive
untouched-tree runs in which the check layers re-executed while
COPY . .stayedCACHED. The recorded justification is accurate.S1 — closed
One record, and it is the one the PR points at. Commit message,
TODO.md, and PR body carry no measurements at all; all threereference the verification comment. There is nothing left that can
disagree with itself. Verified by reading each of the three in full.
S2 — closed, and the replacement is correct
script/cibuild:2-6no longer claims theDockerfilerunsscript/checkviamake check. It names the three realRUNs —make fmt-checkandmake lintin the lint stage,make testin thebuilder stage — and carries the "provided they actually ran" caveat.
There is no
make checkanywhere in theDockerfile; the new textmatches the file.
Behavioral verification
1. Back-to-back pair on an unchanged tree
Literal
script/cibuild, serial, nothing touched between runs.oklines0 issues.Check-layer terminal status, run B — the run that matters:
No
CACHEDon any check layer, and 14 realoklines withper-package durations, e.g.
Three further untouched-tree
script/cibuildruns earlier in thesession (pre-prune) behaved identically: EXIT 0, 14
oklines, one0 issues., all three check layersDONE, 15-16CACHEDlayers each.Five consecutive untouched-tree runs, five real executions.
2. Not a cold build
Run B, judged from within the pair as required after the prune — every
layer above the
ARGin both check stages:COPY . .isCACHEDin both stages while theRUNs directly belowit are not. That is the placement claim observed rather than inferred,
and it is also the isolation argument: with the context layer cached and
the tree clean, the only input that changed between runs A and B is
CHECK_EPOCH. 145s against a 172s genuinely cold build — the margin isnarrow only because run A was itself cold and the host was loaded; the
per-layer statuses are unambiguous.
3. Withheld-
--build-argcounterfactual, run twiceRaw
docker build .,Dockerfileunchanged,ARGpresent, only the--build-argwithheld. I ran this pair twice over the session, beforeand after the prune.
Pre-prune pair:
oklines0 issues.Post-prune pair:
oklines0 issues.I am flagging the contaminated run rather than reporting it as a clean
observation: its lint layers arrived
CACHEDunder an emptyCHECK_EPOCHeven though the prune had wiped the cache and my ownpreceding builds all used real epochs. The consistent explanation is a
concurrent bare
docker build .from another session on this sharedhost, whose lint stage completed just ahead of mine while its
make testwas still in flight and was joined by dedup — its 8.13kB context transfer
and 84s wall both fit that and nothing else. It changes no conclusion:
what matters is that withdrawing only the
--build-argreproduces theoriginal defect exactly — 400ms, zero
oklines, all three checklayers
CACHED, exit 0 — and I observed that on both pairs, on twodifferent cache states.
CHECK_EPOCHis the operative mechanism and thebare unreferenced
ARGis doing real work.This is also the measured basis on which I judged B2's prose accurate,
and it is the known residual gap (#91), which per the review scope is
not a blocker for this PR. The PR's prose does not claim otherwise —
README.mdand theDockerfileboth state this failure mode outright.4.
Dockerfileexecutable lines are byte-identical to24f6e2fConfirmed mechanically, not by eye. Stripping comment lines and blank
lines from
Dockerfileat both revisions:Every
FROM/ARG/RUN/COPY/WORKDIR/USER/ENTRYPOINTline is unchanged and in the same order. The certified placement was not
touched.
Everything else checked
.golangci.ymlsha256 is exactly021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.FROMline and its digest unchanged (#78).script/lintunchanged (#78/#80/#88).
.gitea/workflows/check.ymlunchanged.script/dockerandMakefileunchanged. Diff is exactly the 4 filesclaimed; no
_test.gotouched; not.Skip; no weakened assertion.09dbe6f:check / check (pull_request)success in 2m57s. That duration is itself corroboration from the
runner side — pre-fix
maincommits reported "Successful in 6s", thecached-green signature. The gate now costs real time on CI.
origin/main(3bcdbcf):git merge-treereports 0 conflict markers; the API reports
mergeable: true.message, author, or committer. No attribution trailers of any kind.
(closes #85); bodywrapped at 72; no added line in any changed file exceeds 80 columns.
No non-inclusive terminology.
script/fmt-checkisgofmt-based andno Go file changed, so formatting is clean by construction.
TODO.mdupdated in the same commit, and its entry is accurateagainst the code I read — including the conditional guarantee and the
#91 pointer.
set-but-unparseable/silent-defaulting rule has nothing to bite on; the
one runtime input that is introduced,
epoch, fails loudly, whichis B1.
The two self-reported honesty items
Both were handled well; neither papers over anything.
(a) Lost wall time.
date +%s%3Nyielding full nanoseconds on thishost is a real and easy trap, and the run it corrupted was the
changed-tree baseline — not part of the untouched-tree pair the finding
turns on. Reporting it as lost and re-measuring the pair with a working
expression is the correct handling; back-filling a plausible number
would have been fabrication in the one repo where fabricated greens are
the standing defect. I hit adjacent timing and process hazards myself in
this session and take the same view.
(b) 14 vs 11
CACHED. I reproduced comparable varianceindependently — 14, 15 and 16
CACHEDacross my own untouched-tree runs— and in every case the differing layers were outside the check stages,
with all three check layers and all Go dependency layers behaving
identically. The runtime-stage/cache-GC attribution is consistent with
what I saw, and on a host where a
prune -afjust wiped 41 GB it is morethan plausible. Disclosing it beat reporting "14 both times".
Nits, no action required
date +%sis second-granular, so the property isper-(content, second) rather than strictly per-invocation, and
README.md's "a fresh--build-arg CHECK_EPOCH" is precise only atthat granularity. I convinced myself it is not exploitable here: the
warm floor is over two minutes, so no sequential pair can share a
second, and two invocations that genuinely start within the same
second are deduplicated by BuildKit into one in-flight op that really
executes. Tracked as #91 item 3.
REPO_POLICIES.mdstill saysscript/cibuild"runsdocker build ." and "Since the Dockerfile already runsmake check,a successful build implies all checks pass." Both are now false for
this repo. Org-canonical text, correctly left alone here, but it is
drifting away from the fixed gate and is worth an upstream item.
script/fmt-checkisgofmt-only, so the Markdown in thisPR is not machine-verified against the repo's prose formatting
settings. I hand-checked instead: no added line exceeds 80 columns.
TODO.mdWorkflow rotation. The completed entry was added,but "Next Step" still points at issue #71 rather than being rotated.
Pre-existing pattern in this repo, not introduced here.
.dockerignoreexcludes*.md, soREADME.md/TODO.mdedits never invalidate
COPY . .. That is not a defect; it is asharpening of why this fix was needed, since a docs-only commit could
otherwise never bust the check layers.
Verdict
PASS. No blocking findings. B1, B2, B3, S1 and S2 are each closed,
and I verified each one independently rather than accepting that it had
been addressed: B1 by making
datefail and confirming the scriptaborts with no build started, B2 by reproducing the failure mode the new
prose names, B3 by re-deriving the reasoning against #85's numbered DoD
and confirming the bare
ARGgenuinely enters the cache key here, S1 andS2 by reading the committed text.
This change is the gate for everything that follows, so to be explicit
about what I am certifying: with this landed, a green from
script/cibuildmeans the checks executed, and I have five consecutiveuntouched-tree runs and a two-run negative control on two different
cache states behind that statement. The known hole that remains — a bare
docker build .on a second consecutive unchanged-tree run — is real,is documented in the repo rather than hidden, and is tracked in #91.