From 09dbe6f4c988995c98c3231737ea3d9e6ff4f84f Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 06:08:33 +0000 Subject: [PATCH] Make script/cibuild unable to report an unearned green (closes #85) 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. The fix is an `ARG CHECK_EPOCH` declared immediately above the check RUNs, with script/cibuild passing a fresh value 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. The epoch is assigned to its own variable rather than substituted inline into the --build-arg: epoch="$(date +%s)" docker build --build-arg CHECK_EPOCH="$epoch" . Under `set -eu` a command substitution that fails inside an argument does not abort the script. Inline, a failing `date` would leave CHECK_EPOCH an empty string; an empty string is a constant; and a constant CHECK_EPOCH is precisely the cached-check false green this commit exists to eliminate -- so the guard would have carried a silent path to the defect it guards against. As a bare assignment, `set -e` aborts before any build starts. The guarantee is conditional, and README.md and the Dockerfile now say so instead of claiming the check layers can never be cached. They are keyed on CHECK_EPOCH, so they re-run for any value not yet built against this tree -- but a build that omits --build-arg gets the empty default, and on an unchanged tree every build after the first then replays them, executes nothing, and exits 0. That state was produced by measurement rather than reasoned about. Issue #91 tracks the upstream hardening that would make the missing-arg case fail loudly, along with the expanded ARG form, a per-invocation epoch, and script/docker. The bare unreferenced ARG form is kept deliberately, not because it matches upstream -- upstream has since settled on expanding the value into the check command. A declared-but-unreferenced ARG does enter BuildKit's cache key, which is measured on this host rather than assumed, and upstream records that repos on the bare form need no rework. Moving to the expanded form is hardening, tracked in #91. The script/cibuild header comment claimed the Dockerfile runs script/check via make check. It does not: it runs make fmt-check and make lint in the lint stage and make test in the builder stage. Corrected. Measurements are recorded once, in the verification comment on PR #89: a back-to-back script/cibuild pair on an unchanged tree, and the counterfactual that withholds --build-arg and reproduces the original false green on its second run. They are deliberately not restated here or in TODO.md, so there is a single record that cannot disagree with itself. .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. --- Dockerfile | 35 +++++++++++++++++++++++++++++++++-- README.md | 22 ++++++++++++++++++---- TODO.md | 25 +++++++++++++++++++++++++ script/cibuild | 24 +++++++++++++++++++++--- 4 files changed, 97 insertions(+), 9 deletions(-) diff --git a/Dockerfile b/Dockerfile index 267f8ae..7d7bd16 100644 --- a/Dockerfile +++ b/Dockerfile @@ -19,7 +19,34 @@ RUN go mod download # Copy source code COPY . . -# Run formatting check and linter +# Run formatting check and linter. +# +# 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. +# +# 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. +# +# 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. +# +# 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 @@ -44,7 +71,11 @@ RUN go mod download # Copy source code COPY . . -# Run tests +# 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. +ARG CHECK_EPOCH RUN make test # Build (pure Go, no CGO required since we use modernc.org/sqlite) diff --git a/README.md b/README.md index 552d00c..71328c4 100644 --- a/README.md +++ b/README.md @@ -617,10 +617,24 @@ them. We provide: lint findings. * `script/docker` — build the Docker image tagged via `script/projectname` -* `script/cibuild` — CI entrypoint: `docker build .` (the Dockerfile - runs the checks). 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. +* `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. + + 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. * `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 16f67b1..60235dc 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,31 @@ or delete the branch. # Completed Steps +- 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: + the checks never executed and the build still exited 0. The fix is an + `ARG CHECK_EPOCH` declared immediately above the check `RUN`s in both + the lint stage and the builder stage (`ARG` scope is per-stage, so + each declares its own), with `script/cibuild` assigning + `epoch="$(date +%s)"` and passing `--build-arg CHECK_EPOCH="$epoch"`. + The assignment is separate on purpose: under `set -eu` a command + substitution that fails inside an argument does not abort the script, + which would leave an empty constant `CHECK_EPOCH` and restore the + very false green being fixed. Placement is the rest of the point — + the `ARG` sits below the `apk add`, `COPY go.mod go.sum`, and `go mod + download` layers, so only the checks are invalidated and the + dependency layers still cache. The guarantee is conditional on a + fresh value rather than absolute: a bare `docker build .` gets an + empty `CHECK_EPOCH` and can still serve the check layers from cache, + which `README.md` and the `Dockerfile` now say plainly, with issue + #91 tracking the upstream hardening (expanded `ARG` form, unset + guard, per-invocation epoch, `script/docker`) that would close it. + Verified by re-running the reproduction plus the withheld-`--build-arg` + counterfactual; the measurements are recorded once, in the PR #89 + verification comment, rather than restated here. `.golangci.yml`, the + lint-stage `FROM` line and its digest, `script/lint`, and + `.gitea/workflows/check.yml` are all untouched. - 2026-08-09: Corrected the `Vaultik.UI` doc comment (issue #84). It claimed the cli layer replaces the writer with a discarding one in `--cron` mode; the actual mechanism is `UI.SetQuiet(true)` in diff --git a/script/cibuild b/script/cibuild index 3da5857..44c62d0 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,6 +1,9 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile runs script/check -# (via make check), so a successful build implies all checks pass. +# script/cibuild: run the CI build. The Dockerfile does not run +# script/check; it runs `make fmt-check` and `make lint` in its lint +# stage and `make test` in its builder stage. A successful build +# implies those three passed, provided they actually ran -- which is +# what the CHECK_EPOCH below is for. # Generic: needs no adaptation. The Gitea workflow runs this on push. set -eu @@ -8,7 +11,22 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - docker build . + # The Dockerfile's check layers are keyed on CHECK_EPOCH, so a + # 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. + # + # Assign the epoch on its own line rather than inline in the + # argument. Under `set -eu` a command substitution that fails + # inside an argument does NOT abort the script: CHECK_EPOCH would + # become an empty string, an empty string is a constant, and a + # constant CHECK_EPOCH is exactly the cached-check false green this + # 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)" + docker build --build-arg CHECK_EPOCH="$epoch" . } main "$@" -- 2.49.1