Make a missing CHECK_EPOCH fail the build instead of faking it (closes #91) #92

clawbot · 2026-08-09T09:53:28+02:00

clawbot commented

2026-08-09 09:53:28 +02:00

Adopts all four remaining upstream CHECK_EPOCH hardening items from
sneak/prompts #26, as decided in the manager comment on #91. Closes
the gap PR #89 left open deliberately.

What changed

1. Fail closed on a missing value (Dockerfile, both check stages).

ARG CHECK_EPOCH
RUN [ -n "$CHECK_EPOCH" ] || exit 1

An unset ARG is an empty string, and an empty string is a perfectly
stable cache key — so the second and every later bare docker build .
on an unchanged tree replayed all three check layers, executed nothing,
and still exited 0. Failed steps are never cached, so the guard fires on
every invocation rather than once, converting a quiet lie into a loud
error. The guard is its own RUN so the missing-arg case fails on the
cheapest possible step, before any check starts.

2. Expanded form. Each check RUN now references the value:

RUN echo "check epoch: ${CHECK_EPOCH}" &amp;&amp; make lint

The bare form was not broken — it was measured working on this host in
PR #89. Expanding makes the cache miss contractual rather than dependent
on BuildKit's unreferenced-ARG handling staying as it is, and puts the
epoch in the build log where a reader can see the layer was keyed fresh.

3. Unique epoch per invocation (script/cibuild).
epoch="$(date +%s)" becomes epoch="$(date +%s%N)$$". %N alone is
insufficient: busybox drops it silently, exits 0, and hands back second
granularity with no warning. $$ differs between concurrent invocations
regardless. The bare-assignment form is kept deliberately — inlined into
the --build-arg, a failing substitution does not abort under set -eu
and would silently yield an empty constant epoch, restoring the exact
false green this exists to prevent.

4. script/docker passes the same fresh arg. Not the gate, but
local builds are almost always warm, and two entrypoints disagreeing
about whether the tree is green is worse than either being wrong alone.
With the guard in place, passing it is now required rather than optional.

ARG scope is per-stage and this repo has gate steps in two stages
(make fmt-check + make lint in lint, make test in builder), so
both get their own declaration, guard and expansion. Placement is
unchanged from #85 — below apk add, COPY go.mod go.sum and go mod download.

Docs: the Dockerfile block comment and the README.md script/cibuild
entry both described the bare-docker build . false green as a live
condition to work around, with #91 named as the tracker. Both are
rewritten to state that the case now fails closed. TODO.md updated in
the same commit.

Verification

All numbers are recorded here and only here.

Environment note: the shared BuildKit cache on this host was
destroyed earlier by another session (docker builder prune -af, ~41
GB). No prune of any kind was run from this session — invalidation was
never needed, since the pre-fix baseline below found the cache already
warm for this tree. CACHED: 0 would be uninformative right now, so
ok line count is the primary evidence and dependency caching is judged
from the second run of each pair onward.

1. Negative control, bare `docker build .`, two runs

Pre-fix, on the unmodified tree — the defect reproduced on both
runs, because the cache already held an empty-CHECK_EPOCH entry for
this tree from an earlier session:

run	wall	exit	`CACHED`	`ok` lines
1	0.428s	0	19	0
2	0.285s	0	19	0

Both are the false green: all three check layers CACHED, no check
executed, exit 0.

Post-fix, same command, two runs back to back:

run	wall	exit	`ok` lines
1	0.866s	1	0
2	0.427s	1	0

Both fail, with BuildKit naming the guard:

ERROR: failed to build: failed to solve: process
"/bin/sh -c [ -n \"$CHECK_EPOCH\" ] || exit 1"
did not complete successfully: exit code: 1

The second run failing is the load-bearing observation — it is what
demonstrates that a failed step is never cached, so the error is not a
one-shot.

2. `script/cibuild`, back to back, unchanged tree

run	wall	exit	`ok` lines	epoch
1	2m20.584s	0	14	`1786261355146279441420340`
2	2m6.103s	0	14	`1786261495731625237576490`

Distinct epochs, both echoed in the build log by the expanded form.
Three independent proofs the checks genuinely re-executed in run 2:

14 ok lines, which a replayed layer cannot produce;
per-package durations differ between the runs — internal/database
6.216s vs 5.785s, internal/vaultik 6.445s vs 6.235s. A cached layer
reproduces its recorded output byte for byte, so differing timings
inside the check step are proof of real execution independent of any
reasoning about cache state;
wall time is 2m+, far above the sub-second cached signature.

3. Dependency layers still cache (also the pair-validity control)

Run 2 of the script/cibuild pair, per-step:

step	status
`[lint 2/9] RUN apk add --no-cache make build-base`	`CACHED`
`[lint 4/9] COPY go.mod go.sum ./`	`CACHED`
`[lint 5/9] RUN go mod download`	`CACHED`
`[builder 3/10] RUN apk add --no-cache make build-base sqlite`	`CACHED`
`[builder 5/10] COPY go.mod go.sum ./`	`CACHED`
`[builder 6/10] RUN go mod download`	`CACHED`

So the ARG did not move too high: the checks bust, the dependencies
hold. This doubles as the validity control for the pair — had a cache
wipe landed between the two runs, these layers would have re-executed
and the claim would have failed loudly rather than passing silently.

4. `script/docker`, back to back

run	wall	exit	`ok` lines	epoch
1	2m9.433s	0	14	`1786261640246162821697228`
2	2m11.101s	0	14	`1786261769681695670832854`

Dependency layers CACHED in both, image tagged
docker.io/library/vaultik:latest. script/docker and script/cibuild
now agree.

5. Host-side cross-check

GOFLAGS=-count=1 make check — exit 0, 0 issues. from the linter.
Not a void run per #88: output contains no parallel golangci-lint is running, and cites no path outside this worktree. The gomodguard
deprecation warning is present and left alone (#90, reassigned
upstream).

Notes for the reviewer

The builder-stage guard is not directly exercised by the negative
control. The lint stage fails first, so docker build . never
reaches the builder stage's RUN [ -n "$CHECK_EPOCH" ] || exit 1. It
is there because ARG is stage-scoped and a stage carrying a gate step
without its own guard would be a silent hole if the stage ordering ever
changed. I did not construct an artificial build to fire it in
isolation; stating this rather than implying the measurement covers it.
REPO_POLICIES.md is untouched, per its org-canonical status. Its
lines 170-172 assert "a successful build implies all checks pass". With
this change that statement is now true for every path into the
Dockerfile — script/cibuild, script/docker, and a bare docker build . (the last by failing rather than by passing). But it is true
by virtue of this repo having adopted the hardening, not by anything
the policy text itself guarantees, so a repo that has not adopted it
still reads a false promise there. Flagging rather than editing.
.golangci.yml is unmodified, sha256
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
verified before pushing. The lint-stage FROM line and its digest
(#78), script/lint (#78/#80/#88), and .gitea/workflows/check.yml
are untouched — git diff against those paths is empty.

Single commit: b101b4e.

Adopts all four remaining upstream `CHECK_EPOCH` hardening items from `sneak/prompts` #26, as decided in the manager comment on #91. Closes the gap PR #89 left open deliberately. ## What changed **1. Fail closed on a missing value (`Dockerfile`, both check stages).** ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 ``` An unset `ARG` is an empty string, and an empty string is a perfectly stable cache key — so the second and every later bare `docker build .` on an unchanged tree replayed all three check layers, executed nothing, and still exited 0. Failed steps are never cached, so the guard fires on every invocation rather than once, converting a quiet lie into a loud error. The guard is its own `RUN` so the missing-arg case fails on the cheapest possible step, before any check starts. **2. Expanded form.** Each check `RUN` now references the value: ```dockerfile RUN echo "check epoch: ${CHECK_EPOCH}" && make lint ``` The bare form was not broken — it was measured working on this host in PR #89. Expanding makes the cache miss contractual rather than dependent on BuildKit's unreferenced-`ARG` handling staying as it is, and puts the epoch in the build log where a reader can see the layer was keyed fresh. **3. Unique epoch per invocation (`script/cibuild`).** `epoch="$(date +%s)"` becomes `epoch="$(date +%s%N)$$"`. `%N` alone is insufficient: busybox drops it silently, exits 0, and hands back second granularity with no warning. `$$` differs between concurrent invocations regardless. The bare-assignment form is kept deliberately — inlined into the `--build-arg`, a failing substitution does not abort under `set -eu` and would silently yield an empty constant epoch, restoring the exact false green this exists to prevent. **4. `script/docker`** passes the same fresh arg. Not the gate, but local builds are almost always warm, and two entrypoints disagreeing about whether the tree is green is worse than either being wrong alone. With the guard in place, passing it is now required rather than optional. `ARG` scope is per-stage and this repo has gate steps in two stages (`make fmt-check` + `make lint` in `lint`, `make test` in `builder`), so both get their own declaration, guard and expansion. Placement is unchanged from #85 — below `apk add`, `COPY go.mod go.sum` and `go mod download`. Docs: the `Dockerfile` block comment and the `README.md` `script/cibuild` entry both described the bare-`docker build .` false green as a live condition to work around, with #91 named as the tracker. Both are rewritten to state that the case now fails closed. `TODO.md` updated in the same commit. ## Verification All numbers are recorded here and only here. **Environment note:** the shared BuildKit cache on this host was destroyed earlier by another session (`docker builder prune -af`, ~41 GB). No prune of any kind was run from this session — invalidation was never needed, since the pre-fix baseline below found the cache already warm for this tree. `CACHED: 0` would be uninformative right now, so `ok` line count is the primary evidence and dependency caching is judged from the second run of each pair onward. ### 1. Negative control, bare `docker build .`, two runs Pre-fix, on the unmodified tree — the defect reproduced on **both** runs, because the cache already held an empty-`CHECK_EPOCH` entry for this tree from an earlier session: | run | wall | exit | `CACHED` | `ok` lines | |---|---|---|---|---| | 1 | **0.428s** | **0** | 19 | **0** | | 2 | **0.285s** | **0** | 19 | **0** | Both are the false green: all three check layers `CACHED`, no check executed, exit 0. Post-fix, same command, two runs back to back: | run | wall | exit | `ok` lines | |---|---|---|---| | 1 | 0.866s | **1** | 0 | | 2 | 0.427s | **1** | 0 | Both fail, with BuildKit naming the guard: ``` ERROR: failed to build: failed to solve: process "/bin/sh -c [ -n \"$CHECK_EPOCH\" ] || exit 1" did not complete successfully: exit code: 1 ``` The second run failing is the load-bearing observation — it is what demonstrates that a failed step is never cached, so the error is not a one-shot. ### 2. `script/cibuild`, back to back, unchanged tree | run | wall | exit | `ok` lines | epoch | |---|---|---|---|---| | 1 | 2m20.584s | 0 | 14 | `1786261355146279441420340` | | 2 | 2m6.103s | 0 | 14 | `1786261495731625237576490` | Distinct epochs, both echoed in the build log by the expanded form. Three independent proofs the checks genuinely re-executed in run 2: - 14 `ok` lines, which a replayed layer cannot produce; - per-package durations **differ** between the runs — `internal/database` 6.216s vs 5.785s, `internal/vaultik` 6.445s vs 6.235s. A cached layer reproduces its recorded output byte for byte, so differing timings inside the check step are proof of real execution independent of any reasoning about cache state; - wall time is 2m+, far above the sub-second cached signature. ### 3. Dependency layers still cache (also the pair-validity control) Run 2 of the `script/cibuild` pair, per-step: | step | status | |---|---| | `[lint 2/9] RUN apk add --no-cache make build-base` | `CACHED` | | `[lint 4/9] COPY go.mod go.sum ./` | `CACHED` | | `[lint 5/9] RUN go mod download` | `CACHED` | | `[builder 3/10] RUN apk add --no-cache make build-base sqlite` | `CACHED` | | `[builder 5/10] COPY go.mod go.sum ./` | `CACHED` | | `[builder 6/10] RUN go mod download` | `CACHED` | So the `ARG` did not move too high: the checks bust, the dependencies hold. This doubles as the validity control for the pair — had a cache wipe landed between the two runs, these layers would have re-executed and the claim would have failed loudly rather than passing silently. ### 4. `script/docker`, back to back | run | wall | exit | `ok` lines | epoch | |---|---|---|---|---| | 1 | 2m9.433s | 0 | 14 | `1786261640246162821697228` | | 2 | 2m11.101s | 0 | 14 | `1786261769681695670832854` | Dependency layers `CACHED` in both, image tagged `docker.io/library/vaultik:latest`. `script/docker` and `script/cibuild` now agree. ### 5. Host-side cross-check `GOFLAGS=-count=1 make check` — exit **0**, `0 issues.` from the linter. Not a void run per #88: output contains no `parallel golangci-lint is running`, and cites no path outside this worktree. The `gomodguard` deprecation warning is present and left alone (#90, reassigned upstream). ## Notes for the reviewer - **The builder-stage guard is not directly exercised by the negative control.** The lint stage fails first, so `docker build .` never reaches the builder stage's `RUN [ -n "$CHECK_EPOCH" ] || exit 1`. It is there because `ARG` is stage-scoped and a stage carrying a gate step without its own guard would be a silent hole if the stage ordering ever changed. I did not construct an artificial build to fire it in isolation; stating this rather than implying the measurement covers it. - **`REPO_POLICIES.md` is untouched**, per its org-canonical status. Its lines 170-172 assert "a successful build implies all checks pass". With this change that statement is now true for every path into the `Dockerfile` — `script/cibuild`, `script/docker`, and a bare `docker build .` (the last by failing rather than by passing). But it is true by virtue of this repo having adopted the hardening, not by anything the policy text itself guarantees, so a repo that has not adopted it still reads a false promise there. Flagging rather than editing. - `.golangci.yml` is unmodified, sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, verified before pushing. The lint-stage `FROM` line and its digest (#78), `script/lint` (#78/#80/#88), and `.gitea/workflows/check.yml` are untouched — `git diff` against those paths is empty. Single commit: `b101b4e`.

clawbot added 1 commit 2026-08-09 09:53:28 +02:00

Make a missing CHECK_EPOCH fail the build instead of faking it (closes #91 )

check / check (pull_request) Successful in 2m13s

Details

b101b4e21c

Adopt the four remaining upstream CHECK_EPOCH hardening items from
sneak/prompts #26, closing the gap #85 left open deliberately.

Fail closed on a missing value. Each check stage now asserts
`[ -n "$CHECK_EPOCH" ] || exit 1` before running anything. An unset ARG
is an empty string and an empty string is a stable cache key, so the
second and every later bare `docker build .` on an unchanged tree
replayed all three check layers, executed nothing, and still exited 0 --
and `docker build .` is the command REPO_POLICIES.md names verbatim as a
thing that must be green, so the documented command was precisely the
one that lied. Failed steps are never cached, which is what makes the
guard fire on every invocation rather than once.

Expand the epoch into each check command rather than leaving it a bare
declaration, so the cache miss does not depend on BuildKit's
unreferenced-ARG handling staying as it is, and so the value appears in
the build log where a reader can see the layer was keyed fresh.

Make the epoch unique per invocation rather than per second:
`epoch="$(date +%s%N)$$"`. `%N` alone is not enough, since busybox drops
it silently and exits 0, handing back second granularity with no
warning; `$$` differs between concurrent invocations regardless. The
bare-assignment form is kept on purpose -- inlined into an argument, a
failing substitution does not abort under `set -eu` and would yield an
empty constant epoch, restoring the exact false green this prevents.

Pass the same fresh value from script/docker. It is not the CI gate, but
local builds are almost always warm, so it was the likelier fooling in
practice, and two entrypoints disagreeing about whether the tree is
green is worse than either being wrong alone.

The ARG placement from #85 is unchanged, below apk add, COPY go.mod
go.sum and go mod download, so dependency layers still cache and the
build is not cold. Verified by negative control rather than inspection;
measurements are recorded once, in the PR verification comment.

.golangci.yml, the lint-stage FROM line and its digest, script/lint,
REPO_POLICIES.md and .gitea/workflows/check.yml are untouched.

clawbot added the needs-review label 2026-08-09 09:54:18 +02:00

clawbot commented

2026-08-09 10:08:34 +02:00

Review of PR #92 — VERDICT: PASS

Independent adversarial review of b101b4e. Every number below is my own
measurement on this host, not a restatement of the PR's. No worktree of
this repo was modified, nothing was committed, and no docker builder prune of any kind was run.

1. Negative control — the core claim. CONFIRMED

Bare docker build . (no --build-arg) at b101b4e, twice back to back:

run	exit	wall	`ok` lines
1	1	1.354s	0
2	1	0.547s	0

Both name the guard:

#15 [lint 7/9] RUN [ -n "$CHECK_EPOCH" ] || exit 1
ERROR: failed to build: failed to solve: process
"/bin/sh -c [ -n \"$CHECK_EPOCH\" ] || exit 1"
did not complete successfully: exit code: 1

The second failure is the load-bearing one and it is present. A failed
step is not cached, so this is a standing error, not a one-shot.

2. Pre-fix control — the change is not a no-op. CONFIRMED

I re-ran the identical command against origin/main (c3bb3b5) in a
separate worktree on the same host, twice:

run	exit	wall	`ok` lines
1	0	268ms	0
2	0	251ms	0

The false green is live on main right now and reproduces on both runs.
This PR converts it into a hard failure on both runs. The defect is real
and currently reachable, and the fix closes it.

3. `script/cibuild` still works. CONFIRMED

Unchanged tree, back to back:

run	exit	wall	`ok` lines	epoch
1	0	124s	14	`17862621647309109461004227`
2	0	124s	14	`17862622893839236821136808`

Distinct epochs, both echoed by the expanded form. All three check layers
executed in run 2 (#15/#16/#17 in lint, #24/#25/#26 in
builder). BuildKit prints the substituted value in the step name — e.g.
#15 [lint 7/9] RUN [ -n "17862622893839236821136808" ] || exit 1 —
which is direct evidence the ARG is in that layer's cache key rather
than an inference about it.

4. Not a cold build — dependency layers still cache. CONFIRMED

Run 2 of the script/cibuild pair, mapped from the CACHED step
numbers:

step	status
`#12 [lint 2/9] RUN apk add --no-cache make build-base`	`CACHED`
`#10 [lint 4/9] COPY go.mod go.sum ./`	`CACHED`
`#13 [lint 5/9] RUN go mod download`	`CACHED`
`#18 [builder 3/10] RUN apk add --no-cache make build-base sqlite`	`CACHED`
`#21 [builder 5/10] COPY go.mod go.sum ./`	`CACHED`
`#22 [builder 6/10] RUN go mod download`	`CACHED`

Cached in both check stages. The ARG did not creep upward. Same
pattern independently reproduced in the script/docker pair below.

5. `script/docker`. CONFIRMED

run	exit	wall	`ok` lines	epoch
1	0	133s	14	`17862624513552986991294222`
2	0	120s	14	`17862625848241255761427259`

Dependency layers CACHED in both stages on run 2; both check stages
executed. Image builds and works: docker run --rm vaultik:latest version returns the version banner. The two entrypoints now agree.

6. Builder-stage guard — self-flagged gap, now closed by measurement

The author correctly stated that the negative control does not reach the
builder-stage guard because the lint stage fails first, and did not imply
otherwise. I constructed the targeted probe they declined to build.

Method: a probe Dockerfile outside the repo (-f /tmp/builderprobe.Dockerfile, repo tree untouched and unmodified),
byte-identical to the PR's Dockerfile except that the single lint-stage
line RUN [ -n "$CHECK_EPOCH" ] || exit 1 is removed, built with
--target builder and no --build-arg. That lets an empty epoch survive
the lint stage and reach the builder stage.

Result, twice:

#21 [builder 8/9] RUN [ -n "$CHECK_EPOCH" ] || exit 1
ERROR: ... did not complete successfully: exit code: 1

exit 1 on both runs. The builder-stage guard is live, correct, and
uncached on repeat. That gap is now measured, not merely argued.

7. The differing-durations primitive — ENDORSED, with one correction and one caveat

Sound, and it is a better primitive than CACHED: 0 (which is
uninformative on this host right now). My own two script/cibuild runs:

package	run 1	run 2
`internal/database`	5.664s	5.447s
`internal/vaultik`	6.302s	6.140s
`internal/blob`	1.154s	1.180s
`internal/pidlock`	1.016s	1.016s

Correction to the stated mechanism. Under BUILDKIT_PROGRESS=plain a
replayed layer prints CACHED and no stdout at all — it does not
"reproduce its recorded output byte for byte". So durations are not what
rules out layer replay; zero ok lines already does that. The conclusion
holds a fortiori, but the reason given is not the operative one.

Where it genuinely adds value is one level below layer cache:
script/test runs go test without -count=1, so Go's own test-result
cache could in principle emit ok pkg (cached) lines. Those would count
as ok lines while executing nothing. Real per-package durations are
exactly what distinguishes them. I verified 0 occurrences of (cached)
in both runs. That makes the primitive strictly stronger than ok-line
count, not merely a restatement of it.

Caveat on use. Apply it to the whole duration vector, not to a single
package. As the table shows, internal/pidlock was identical to the
millisecond across two genuinely-executed runs. A single matching package
is not evidence of replay; the vector matching exactly would be.

8. Guard failure modes — probed, none found

No default anywhere. grep -rn CHECK_EPOCH over the whole tree:
both ARG CHECK_EPOCH declarations are bare, with no =. There is no
docker-compose file in the repo, .gitea/workflows/check.yml invokes
only script/cibuild, .goreleaser.yaml has no docker section, and no
--build-arg CHECK_EPOCH appears without a value.
The only two docker build invocations in the repo are
script/cibuild:39 and script/docker:20, both passing
--build-arg CHECK_EPOCH="$epoch". make docker shims to
script/docker. No unguarded path remains.
Bare-assignment form preserved in both scripts: script/cibuild:38
and script/docker:19 are epoch="$(date +%s%N)$$" on their own line,
never inlined into the argument. The PR #89 bug is not reintroduced.
The Dockerfile comment explicitly forbids adding a default, and no
default exists.

9. Nothing weakened. CONFIRMED

.golangci.yml sha256 =
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
git diff c3bb3b5..b101b4e -- REPO_POLICIES.md .gitea/ script/lint .golangci.yml is empty.
No FROM line or digest changed (#78).
No Go files touched at all, so no deleted tests and no weakened
assertions.

10. CI, mergeability, hygiene

CI green on b101b4e: check / check (pull_request) succeeded in
2m13s. The duration is consistent with real execution, not a replay.
Mergeable: origin/main is still c3bb3b5 and is an ancestor of
b101b4e; merge-tree reports zero conflicts. Fast-forwardable.
No Claude/Anthropic references in the diff, commit message, or PR
body. Commit trailer list is empty — no attribution trailers.
Commit subject ends with (closes #91). Single commit.
TODO.md updated in the same commit and it correctly defers all
measurements to the PR rather than restating them, so there are no
divergent numbers across places.
git diff --check clean; changed prose wraps at 80 columns; no
non-inclusive terminology; containerised make fmt-check and make lint passed cleanly across five in-container runs (structurally immune
to #88; no parallel golangci-lint is running, no out-of-worktree
paths).
No scope creep: five files, all of them the ones the issue names.

11. Honesty of the self-flagged items

Both handled correctly. The builder-guard limitation was stated plainly
rather than papered over — the PR says "stating this rather than implying
the measurement covers it", which is exactly right, and I closed it above.
REPO_POLICIES.md was correctly flagged and correctly left unedited given
its org-canonical status.

I also cross-checked the author's own reported figures against their run
logs still present on this host: 14 ok lines, epoch
1786261495731625237576490, internal/database 5.785s and
internal/vaultik 6.235s — matching the PR table exactly. The reported
measurements are genuine.

Nits (non-blocking, no rework required)

Commit subject is 75 characters, over the conventional 72 and
longer than all twelve most recent commits on main (max 68).
Cosmetic; no written rule in REPO_POLICIES.md covers it.
The guard emits no diagnostic. An operator running a bare
docker build . sees only the raw failing command text. A message
such as || { echo "CHECK_EPOCH must be set; use script/cibuild" >&2; exit 1; } would read better — but the bare form is the decided
upstream/org-canonical text, so diverging from it unilaterally here
would be worse than the nit. Explicitly not requesting a change.
TODO.md "Next Step" was not advanced — still "Triage the stale
remote branches (issue #71)", unchanged from main, while the file's
own Workflow section says to do the work in Next Step and rotate it.
Pre-existing pattern (PR #89 behaved identically) and this repo is
issue-driven; not this PR's to fix.
script/fmt-check covers gofmt only, so markdown formatting is
ungated by the gate this PR hardens. Not introduced here, and the
changed prose is correctly wrapped anyway.

Verdict

PASS. Blocking findings: none.

This is genuinely clean. The change does exactly what the issue's
definition of done requires, the two-run negative control passes, the
pre-fix defect is confirmed live on main so the change is not a no-op,
dependency caching is intact in both stages, both entrypoints agree, CI
is green, it is fast-forwardable onto main, and the one gap the author
self-flagged has now been closed by direct measurement rather than
argument. Label merge-ready and assign to sneak.

## Review of PR #92 — VERDICT: PASS Independent adversarial review of `b101b4e`. Every number below is my own measurement on this host, not a restatement of the PR's. No worktree of this repo was modified, nothing was committed, and no `docker builder prune` of any kind was run. --- ### 1. Negative control — the core claim. CONFIRMED Bare `docker build .` (no `--build-arg`) at `b101b4e`, twice back to back: | run | exit | wall | `ok` lines | |---|---|---|---| | 1 | **1** | 1.354s | 0 | | 2 | **1** | 0.547s | 0 | Both name the guard: ``` #15 [lint 7/9] RUN [ -n "$CHECK_EPOCH" ] || exit 1 ERROR: failed to build: failed to solve: process "/bin/sh -c [ -n \"$CHECK_EPOCH\" ] || exit 1" did not complete successfully: exit code: 1 ``` The second failure is the load-bearing one and it is present. A failed step is not cached, so this is a standing error, not a one-shot. ### 2. Pre-fix control — the change is not a no-op. CONFIRMED I re-ran the identical command against `origin/main` (`c3bb3b5`) in a separate worktree on the same host, twice: | run | exit | wall | `ok` lines | |---|---|---|---| | 1 | **0** | 268ms | **0** | | 2 | **0** | 251ms | **0** | The false green is live on `main` right now and reproduces on both runs. This PR converts it into a hard failure on both runs. The defect is real and currently reachable, and the fix closes it. ### 3. `script/cibuild` still works. CONFIRMED Unchanged tree, back to back: | run | exit | wall | `ok` lines | epoch | |---|---|---|---|---| | 1 | 0 | 124s | **14** | `17862621647309109461004227` | | 2 | 0 | 124s | **14** | `17862622893839236821136808` | Distinct epochs, both echoed by the expanded form. All three check layers executed in run 2 (`#15`/`#16`/`#17` in lint, `#24`/`#25`/`#26` in builder). BuildKit prints the substituted value in the step name — e.g. `#15 [lint 7/9] RUN [ -n "17862622893839236821136808" ] || exit 1` — which is direct evidence the `ARG` is in that layer's cache key rather than an inference about it. ### 4. Not a cold build — dependency layers still cache. CONFIRMED Run 2 of the `script/cibuild` pair, mapped from the `CACHED` step numbers: | step | status | |---|---| | `#12 [lint 2/9] RUN apk add --no-cache make build-base` | `CACHED` | | `#10 [lint 4/9] COPY go.mod go.sum ./` | `CACHED` | | `#13 [lint 5/9] RUN go mod download` | `CACHED` | | `#18 [builder 3/10] RUN apk add --no-cache make build-base sqlite` | `CACHED` | | `#21 [builder 5/10] COPY go.mod go.sum ./` | `CACHED` | | `#22 [builder 6/10] RUN go mod download` | `CACHED` | Cached in **both** check stages. The `ARG` did not creep upward. Same pattern independently reproduced in the `script/docker` pair below. ### 5. `script/docker`. CONFIRMED | run | exit | wall | `ok` lines | epoch | |---|---|---|---|---| | 1 | 0 | 133s | 14 | `17862624513552986991294222` | | 2 | 0 | 120s | 14 | `17862625848241255761427259` | Dependency layers `CACHED` in both stages on run 2; both check stages executed. Image builds and works: `docker run --rm vaultik:latest version` returns the version banner. The two entrypoints now agree. ### 6. Builder-stage guard — self-flagged gap, now closed by measurement The author correctly stated that the negative control does not reach the builder-stage guard because the lint stage fails first, and did not imply otherwise. I constructed the targeted probe they declined to build. Method: a probe `Dockerfile` **outside** the repo (`-f /tmp/builderprobe.Dockerfile`, repo tree untouched and unmodified), byte-identical to the PR's `Dockerfile` except that the single lint-stage line `RUN [ -n "$CHECK_EPOCH" ] || exit 1` is removed, built with `--target builder` and no `--build-arg`. That lets an empty epoch survive the lint stage and reach the builder stage. Result, twice: ``` #21 [builder 8/9] RUN [ -n "$CHECK_EPOCH" ] || exit 1 ERROR: ... did not complete successfully: exit code: 1 ``` exit 1 on both runs. The builder-stage guard is live, correct, and uncached on repeat. That gap is now measured, not merely argued. ### 7. The differing-durations primitive — ENDORSED, with one correction and one caveat Sound, and it is a better primitive than `CACHED: 0` (which is uninformative on this host right now). My own two `script/cibuild` runs: | package | run 1 | run 2 | |---|---|---| | `internal/database` | 5.664s | 5.447s | | `internal/vaultik` | 6.302s | 6.140s | | `internal/blob` | 1.154s | 1.180s | | `internal/pidlock` | 1.016s | 1.016s | **Correction to the stated mechanism.** Under `BUILDKIT_PROGRESS=plain` a replayed layer prints `CACHED` and *no stdout at all* — it does not "reproduce its recorded output byte for byte". So durations are not what rules out layer replay; zero `ok` lines already does that. The conclusion holds a fortiori, but the reason given is not the operative one. **Where it genuinely adds value** is one level below layer cache: `script/test` runs `go test` without `-count=1`, so Go's own test-result cache could in principle emit `ok pkg (cached)` lines. Those would count as `ok` lines while executing nothing. Real per-package durations are exactly what distinguishes them. I verified `0` occurrences of `(cached)` in both runs. That makes the primitive strictly stronger than `ok`-line count, not merely a restatement of it. **Caveat on use.** Apply it to the whole duration vector, not to a single package. As the table shows, `internal/pidlock` was identical to the millisecond across two genuinely-executed runs. A single matching package is not evidence of replay; the vector matching exactly would be. ### 8. Guard failure modes — probed, none found - **No default anywhere.** `grep -rn CHECK_EPOCH` over the whole tree: both `ARG CHECK_EPOCH` declarations are bare, with no `=`. There is no `docker-compose` file in the repo, `.gitea/workflows/check.yml` invokes only `script/cibuild`, `.goreleaser.yaml` has no docker section, and no `--build-arg CHECK_EPOCH` appears without a value. - **The only two `docker build` invocations in the repo** are `script/cibuild:39` and `script/docker:20`, both passing `--build-arg CHECK_EPOCH="$epoch"`. `make docker` shims to `script/docker`. No unguarded path remains. - **Bare-assignment form preserved** in both scripts: `script/cibuild:38` and `script/docker:19` are `epoch="$(date +%s%N)$$"` on their own line, never inlined into the argument. The PR #89 bug is not reintroduced. - The `Dockerfile` comment explicitly forbids adding a default, and no default exists. ### 9. Nothing weakened. CONFIRMED - `.golangci.yml` sha256 = `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. - `git diff c3bb3b5..b101b4e -- REPO_POLICIES.md .gitea/ script/lint .golangci.yml` is **empty**. - No `FROM` line or digest changed (#78). - No Go files touched at all, so no deleted tests and no weakened assertions. ### 10. CI, mergeability, hygiene - **CI green** on `b101b4e`: `check / check (pull_request)` succeeded in 2m13s. The duration is consistent with real execution, not a replay. - **Mergeable**: `origin/main` is still `c3bb3b5` and is an ancestor of `b101b4e`; merge-tree reports zero conflicts. Fast-forwardable. - **No Claude/Anthropic references** in the diff, commit message, or PR body. Commit trailer list is empty — no attribution trailers. - Commit subject ends with ` (closes #91)`. Single commit. - `TODO.md` updated in the same commit and it correctly *defers* all measurements to the PR rather than restating them, so there are no divergent numbers across places. - `git diff --check` clean; changed prose wraps at 80 columns; no non-inclusive terminology; containerised `make fmt-check` and `make lint` passed cleanly across five in-container runs (structurally immune to #88; no `parallel golangci-lint is running`, no out-of-worktree paths). - No scope creep: five files, all of them the ones the issue names. ### 11. Honesty of the self-flagged items Both handled correctly. The builder-guard limitation was stated plainly rather than papered over — the PR says "stating this rather than implying the measurement covers it", which is exactly right, and I closed it above. `REPO_POLICIES.md` was correctly flagged and correctly left unedited given its org-canonical status. I also cross-checked the author's own reported figures against their run logs still present on this host: 14 `ok` lines, epoch `1786261495731625237576490`, `internal/database` 5.785s and `internal/vaultik` 6.235s — matching the PR table exactly. The reported measurements are genuine. --- ## Nits (non-blocking, no rework required) 1. **Commit subject is 75 characters**, over the conventional 72 and longer than all twelve most recent commits on `main` (max 68). Cosmetic; no written rule in `REPO_POLICIES.md` covers it. 2. **The guard emits no diagnostic.** An operator running a bare `docker build .` sees only the raw failing command text. A message such as `|| { echo "CHECK_EPOCH must be set; use script/cibuild" >&2; exit 1; }` would read better — but the bare form is the decided upstream/org-canonical text, so diverging from it unilaterally here would be worse than the nit. Explicitly **not** requesting a change. 3. **`TODO.md` "Next Step" was not advanced** — still "Triage the stale remote branches (issue #71)", unchanged from `main`, while the file's own Workflow section says to do the work in Next Step and rotate it. Pre-existing pattern (PR #89 behaved identically) and this repo is issue-driven; not this PR's to fix. 4. **`script/fmt-check` covers `gofmt` only**, so markdown formatting is ungated by the gate this PR hardens. Not introduced here, and the changed prose is correctly wrapped anyway. --- ## Verdict **PASS.** Blocking findings: **none**. This is genuinely clean. The change does exactly what the issue's definition of done requires, the two-run negative control passes, the pre-fix defect is confirmed live on `main` so the change is not a no-op, dependency caching is intact in both stages, both entrypoints agree, CI is green, it is fast-forwardable onto `main`, and the one gap the author self-flagged has now been closed by direct measurement rather than argument. Label `merge-ready` and assign to `sneak`.

clawbot added merge-ready and removed needs-review labels 2026-08-09 10:09:12 +02:00

clawbot merged commit 50816b7415 into main

2026-08-09 10:09:27 +02:00

clawbot deleted branch fix-check-epoch-hardening

2026-08-09 10:09:27 +02:00

clawbot referenced this pull request

2026-08-09 10:09:55 +02:00

script/test omits -count=1, so Go's test cache can satisfy the gate without running tests #93

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#92