Make script/cibuild unable to report an unearned green (closes #85) #89
Reference in New Issue
Block a user
Delete Branch "fix-cibuild-cache"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.