diff --git a/Dockerfile b/Dockerfile index 7d7bd16..013babe 100644 --- a/Dockerfile +++ b/Dockerfile @@ -23,32 +23,32 @@ COPY . . # # CHECK_EPOCH must stay immediately above these RUNs. These layers are # keyed on its value, so they are cache-eligible only for a value -# already built against this same tree. script/cibuild passes a fresh -# value on every invocation, which is what makes its green mean the -# checks really executed. +# already built against this same tree. script/cibuild and script/docker +# each pass a fresh value on every invocation, which is what makes their +# green mean the checks really executed. # -# The guarantee is conditional on that fresh value, not absolute. A -# build that omits --build-arg -- a bare `docker build .` -- gets an -# empty CHECK_EPOCH, and an empty string is a constant: the first such -# build runs the checks, and every one after it on an unchanged tree -# replays these layers from cache, never executing a check and still -# exiting 0, a green nothing earned. Gate through script/cibuild. -# Making the missing-arg case fail loudly instead is tracked in #91. +# The value is expanded into each check command rather than left to a +# bare declaration, so the cache miss does not depend on BuildKit's +# unreferenced-ARG handling staying as it is. It also puts the epoch in +# the build log, where a reader can see the layer was keyed fresh. # -# CHECK_EPOCH is deliberately not referenced by the commands below: a -# declared-but-unreferenced ARG does enter BuildKit's cache key, which -# is measured on this host rather than assumed (PR #89). Upstream -# sneak/prompts #26 prefers expanding the value into the command so -# that the miss is contractual rather than dependent on that behavior -# staying as it is; adopting that here is tracked in #91. Do not delete -# this ARG as dead code -- the gate depends on it. +# The guard is what makes a build that omits --build-arg fail instead of +# lie. An unset ARG is an empty string, and an empty string is a +# perfectly stable cache key: without the guard the first such build +# runs the checks and every one after it on an unchanged tree replays +# these layers from cache, executes nothing, and still exits 0. Failed +# steps are never cached, so the guard fails on EVERY invocation rather +# than once -- a bare `docker build .` is now a loud error, not a quiet +# green. Do not give CHECK_EPOCH a default value; a default would +# satisfy the guard with a constant and restore the hole. # # ARG scope is per-stage, so the builder stage declares its own. # Everything above this line (apk, go.mod, `go mod download`) is # deliberately outside the busted range and keeps caching. ARG CHECK_EPOCH -RUN make fmt-check -RUN make lint +RUN [ -n "$CHECK_EPOCH" ] || exit 1 +RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check +RUN echo "check epoch: ${CHECK_EPOCH}" && make lint # Build stage # golang:1.26.1-alpine, 2026-03-17 @@ -71,12 +71,13 @@ RUN go mod download # Copy source code COPY . . -# Run tests. See the CHECK_EPOCH comment in the lint stage, including -# the conditions the guarantee depends on; ARG scope is per-stage, so -# this stage needs its own declaration, and it must stay immediately -# above the check RUN. +# Run tests. See the CHECK_EPOCH comment in the lint stage for the +# mechanism; ARG scope is per-stage, so this stage needs its own +# declaration, its own guard, and its own expansion, and they must stay +# immediately above the check RUN. ARG CHECK_EPOCH -RUN make test +RUN [ -n "$CHECK_EPOCH" ] || exit 1 +RUN echo "check epoch: ${CHECK_EPOCH}" && make test # Build (pure Go, no CGO required since we use modernc.org/sqlite) RUN CGO_ENABLED=0 go build -ldflags "-X 'sneak.berlin/go/vaultik/internal/globals.Version=${VERSION}' -X 'sneak.berlin/go/vaultik/internal/globals.Commit=$(git rev-parse HEAD 2>/dev/null || echo unknown)' -X 'sneak.berlin/go/vaultik/internal/globals.CommitDate=$(git show -s --format=%cs HEAD 2>/dev/null || echo unknown)'" -o /vaultik ./cmd/vaultik diff --git a/README.md b/README.md index 71328c4..7ebb4db 100644 --- a/README.md +++ b/README.md @@ -616,25 +616,31 @@ them. We provide: the pinned linter: a local `make check` and CI cannot disagree about lint findings. * `script/docker` — build the Docker image tagged via - `script/projectname` + `script/projectname`. Passes a fresh `--build-arg CHECK_EPOCH` for the + same reason `script/cibuild` does, so a local image build cannot be + green on checks it replayed from cache. * `script/cibuild` — CI entrypoint: `docker build` (the `Dockerfile` runs `make fmt-check` and `make lint` in its lint stage and `make test` in its builder stage). This is the full CI-equivalent gate — it runs the checks in the same containers CI does, from a clean copy of the tree, so it also catches anything that depends on host state. It - passes a fresh `--build-arg CHECK_EPOCH`, which the `Dockerfile` - declares immediately above the check `RUN`s in both stages. Those - layers are keyed on that value, so a new value re-runs them even on a - byte-identical tree, and a green from this script means the checks - executed. Dependency and module layers sit above the `ARG` and still - cache, so a build is not cold. + passes a fresh `--build-arg CHECK_EPOCH`, unique per invocation, which + the `Dockerfile` declares immediately above the check `RUN`s in both + stages and expands into each check command. Those layers are keyed on + that value, so a new value re-runs them even on a byte-identical tree, + and a green from this script means the checks executed. Dependency and + module layers sit above the `ARG` and still cache, so a build is not + cold. - That guarantee is conditional on the fresh value, so **run the gate - through `script/cibuild`, not by invoking `docker build` yourself**. A - bare `docker build .` supplies no `CHECK_EPOCH`; the empty default is - a constant, so the second and every later build on an unchanged tree - serves all three check layers from cache, executes nothing, and still - exits 0. Issue #91 tracks making that case fail loudly instead. + A build that supplies no `CHECK_EPOCH` — a bare `docker build .` — + fails rather than lying. An unset `ARG` is an empty string and an + empty string is a stable cache key, so without a guard such a build + would serve all three check layers from cache, execute nothing, and + still exit 0. Each check stage therefore asserts the value is + non-empty before running anything, and because failed steps are never + cached that assertion fires on every invocation rather than once. Use + `script/cibuild` (or `script/docker`, which passes the same arg); a + bare `docker build .` is now a loud error. * `script/precommit` — pre-commit gate: `go mod tidy` + `go fmt` (must not change files), then `script/check` * `script/install-precommit` — install the git pre-commit hook that diff --git a/TODO.md b/TODO.md index 60235dc..21ae2eb 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,42 @@ or delete the branch. # Completed Steps +- 2026-08-09: Adopted the remaining upstream `CHECK_EPOCH` hardening + (issue #91), closing the gap #85 knowingly left open. Four changes, + all four decided as adopt upstream in `sneak/prompts` #26. (1) Each + check stage now asserts `[ -n "$CHECK_EPOCH" ] || exit 1` before + running anything, so a build that supplies no `--build-arg` fails + instead of lying. This is the item that mattered: 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 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. (2) The epoch is now + expanded into each check command rather than left as a bare + declaration, so the cache miss no longer depends on BuildKit's + unreferenced-`ARG` handling staying as it is, and the value appears + in the build log. (3) `script/cibuild` uses + `epoch="$(date +%s%N)$$"`, unique per invocation rather than per + second; `%N` alone is insufficient because busybox drops it silently + and exits 0, and `$$` is what makes the guarantee hold regardless. + The bare-assignment form is kept deliberately — inlined in an + argument, a failing substitution does not abort under `set -eu` and + would yield an empty constant epoch, restoring the exact false green + being fixed. (4) `script/docker` passes the same fresh arg, so the + two entrypoints cannot disagree about whether the tree is green; + local builds are almost always warm, which made it the likelier + fooling in practice. 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 — a bare `docker build .` run twice + back to back, plus back-to-back pairs of both scripts and a host-side + `make check`; the measurements are recorded once, in the PR + verification comment, rather than restated here. `.golangci.yml`, the + lint-stage `FROM` line and its digest, `script/lint`, + `REPO_POLICIES.md` and `.gitea/workflows/check.yml` are all + untouched. - 2026-08-09: Stopped `script/cibuild` from reporting a green it did not earn (issue #85). A bare `docker build .` let Docker serve the check layers from the layer cache whenever the tree had not changed: diff --git a/script/cibuild b/script/cibuild index 44c62d0..371fe59 100755 --- a/script/cibuild +++ b/script/cibuild @@ -15,7 +15,17 @@ main() { # fresh value here is what forces them to re-run: without it an # unchanged tree replays them from cache, the checks never execute, # and the build still exits 0. The ARG sits immediately above the - # check RUNs, so dependency and module layers still cache. + # check RUNs, so dependency and module layers still cache. The + # Dockerfile also refuses to build at all when CHECK_EPOCH is empty, + # so a missing value fails loudly here rather than passing quietly. + # + # The value must be unique per invocation, not per second. `date +%s` + # is second-granular, so two concurrent invocations in the same + # second get identical epochs and the later one can be served from + # cache -- the original defect in miniature. `%N` alone does not fix + # it: busybox silently drops %N, exits 0, and hands back second + # granularity with no warning. `$$` is what makes this correct + # regardless, since concurrent invocations have different pids. # # Assign the epoch on its own line rather than inline in the # argument. Under `set -eu` a command substitution that fails @@ -25,7 +35,7 @@ main() { # script exists to prevent -- so the guard would disarm itself and # still exit 0. As a bare assignment, `set -e` catches a failing # `date` and no build starts. - epoch="$(date +%s)" + epoch="$(date +%s%N)$$" docker build --build-arg CHECK_EPOCH="$epoch" . } diff --git a/script/docker b/script/docker index 2884e41..f592630 100755 --- a/script/docker +++ b/script/docker @@ -9,7 +9,16 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" - docker build -t "$("$SCRIPT_DIR/projectname")" . + # Same CHECK_EPOCH contract as script/cibuild, for the same reason + # and with the same bare-assignment and `$$` requirements -- see the + # comments there. This script is not the CI gate, but a local build + # is almost always warm, so without this it would report a green the + # tree had not earned and the two entrypoints would disagree about + # whether the tree is clean. The Dockerfile now refuses to build + # without a non-empty value, so this is required, not optional. + epoch="$(date +%s%N)$$" + docker build --build-arg CHECK_EPOCH="$epoch" \ + -t "$("$SCRIPT_DIR/projectname")" . } main "$@"