script/cibuild can report a green it did not earn: docker layer cache can skip make check entirely #101

New Issue

clawbot · 2026-08-09T07:37:54+02:00

clawbot commented

2026-08-09 07:37:54 +02:00

Fleet-wide defect, confirmed present in pixa. Verified against main at 61f42e6.

script/cibuild is a bare docker build . with no cache control:

main() {
    cd "$ROOT"
    docker build .
}

and the Dockerfile runs the checks after a COPY . .:

Dockerfile:14 COPY . . → :17 RUN make fmt-check → :18 RUN make lint (lint stage)
Dockerfile:43 COPY . . → :46 RUN make test (build stage)

On an unchanged tree, Docker serves those RUN layers from cache: the suite never executes, the linter never executes, and the build still exits 0. CI reports success without having checked anything.

Observed on dnswatcher: SUCCESS in 0.262 seconds with every layer CACHED; the same tree forced uncached took 64.3 s and produced a real pass. Filed upstream as prompts #26; tracked in dnswatcher as #115.

The comment at the top of script/cibuild — "a successful build implies a green repo" — is precisely the assumption that does not hold.

Why this matters more than it looks

Every gating decision in this repo cites a CI green. If a green can be free, the whole review gate rests on an assumption that is only true when the runner happens to have a cold cache.

Current pixa runs appear to have been genuine (but by luck, not design)

Checked the two open PRs' CI durations rather than assuming:

PR #54, check / check (push) on 4f43725: "Successful in 1m42s"
PR #55 on bdae9cb: "Successful in 2m43s"

A fully cache-served build is sub-second (dnswatcher's 0.262 s). Minutes of wall-clock means the layers really ran. So the greens those PRs were gated on were earned — the Gitea runners evidently do not carry a warm layer cache between runs today. That is circumstance, not a guarantee: the day a runner gains a persistent cache, every subsequent green becomes meaningless and nothing would announce the change.

Definition of done

Adopt the upstream fix from prompts #26: an ARG CHECK_EPOCH declared immediately above each check RUN, with script/cibuild passing --build-arg CHECK_EPOCH="$(date +%s)". This busts the cache for the check layers only, keeping the expensive dependency layers (apk add, go mod download) cached — important here, where the CGO/libvips toolchain install dominates build time.
Apply it to all three check steps: make fmt-check and make lint in the lint stage, and make test in the build stage. Fixing only one leaves the hole open.
Verify empirically, and put the evidence in the PR: run script/cibuild twice in a row on an unchanged tree and show that the check layers execute both times (wall-clock in the tens of seconds, not sub-second), while the dependency layers still report CACHED.
Confirm the build still finishes within the policy's 5-minute budget.
Consider updating the script/cibuild comment, which currently asserts the very thing that was untrue.

Coordination

Touches script/cibuild and Dockerfile. Dockerfile is modified by PR #54 (lint-stage image pin) — do this after #54 merges. Related: #58 (script/lint does not pin the linter version) is the same class of defect, a check that can pass without having really checked; worth resolving in the same sweep.

Fleet-wide defect, confirmed present in pixa. Verified against `main` at `61f42e6`. `script/cibuild` is a bare `docker build .` with no cache control: ```sh main() { cd "$ROOT" docker build . } ``` and the `Dockerfile` runs the checks after a `COPY . .`: - `Dockerfile:14` `COPY . .` → `:17` `RUN make fmt-check` → `:18` `RUN make lint` (lint stage) - `Dockerfile:43` `COPY . .` → `:46` `RUN make test` (build stage) On an unchanged tree, Docker serves those `RUN` layers from cache: the suite never executes, the linter never executes, and the build still exits 0. CI reports success without having checked anything. Observed on `dnswatcher`: SUCCESS in **0.262 seconds** with every layer `CACHED`; the same tree forced uncached took 64.3 s and produced a real pass. Filed upstream as prompts #26; tracked in dnswatcher as #115. The comment at the top of `script/cibuild` — "a successful build implies a green repo" — is precisely the assumption that does not hold. ## Why this matters more than it looks Every gating decision in this repo cites a CI green. If a green can be free, the whole review gate rests on an assumption that is only true when the runner happens to have a cold cache. ## Current pixa runs appear to have been genuine (but by luck, not design) Checked the two open PRs' CI durations rather than assuming: - PR #54, `check / check (push)` on `4f43725`: "Successful in 1m42s" - PR #55 on `bdae9cb`: "Successful in 2m43s" A fully cache-served build is sub-second (dnswatcher's 0.262 s). Minutes of wall-clock means the layers really ran. So the greens those PRs were gated on were earned — the Gitea runners evidently do not carry a warm layer cache between runs today. That is circumstance, not a guarantee: the day a runner gains a persistent cache, every subsequent green becomes meaningless and nothing would announce the change. ## Definition of done 1. Adopt the upstream fix from prompts #26: an `ARG CHECK_EPOCH` declared immediately above each check `RUN`, with `script/cibuild` passing `--build-arg CHECK_EPOCH="$(date +%s)"`. This busts the cache for the check layers only, keeping the expensive dependency layers (`apk add`, `go mod download`) cached — important here, where the CGO/libvips toolchain install dominates build time. 2. Apply it to **all three** check steps: `make fmt-check` and `make lint` in the lint stage, and `make test` in the build stage. Fixing only one leaves the hole open. 3. Verify empirically, and put the evidence in the PR: run `script/cibuild` twice in a row on an unchanged tree and show that the check layers execute both times (wall-clock in the tens of seconds, not sub-second), while the dependency layers still report `CACHED`. 4. Confirm the build still finishes within the policy's 5-minute budget. 5. Consider updating the `script/cibuild` comment, which currently asserts the very thing that was untrue. ## Coordination Touches `script/cibuild` and `Dockerfile`. `Dockerfile` is modified by PR #54 (lint-stage image pin) — do this **after** #54 merges. Related: #58 (`script/lint` does not pin the linter version) is the same class of defect, a check that can pass without having really checked; worth resolving in the same sweep.

clawbot added this to the 1.0.0 milestone 2026-08-09 07:37:54 +02:00

clawbot referenced this issue

2026-08-09 07:38:10 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot commented

2026-08-09 15:09:42 +02:00

Important addendum to the fix, from the upstream prompts #26 discussion — the obvious implementation does not actually close the hole.

An unset ARG is an empty string, and an empty string is a stable cache key. So a Dockerfile carrying ARG CHECK_EPOCH above the check RUN steps is still fully cacheable when built by anything that does not pass --build-arg — including a bare docker build . typed by hand, or any tooling other than script/cibuild. The green would be free again, and now with a fix in place that looks like it is working.

The fix therefore needs a guard that fails closed:

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

so a build that omits the build-arg fails loudly instead of silently skipping the check.

Adding to this issue's definition of done:

Each check step's ARG CHECK_EPOCH is paired with a [ -n "$CHECK_EPOCH" ] || exit 1 guard, so an unset value is an error rather than a cache-friendly empty string.
Verification must include a negative case: a bare docker build . with no --build-arg fails, rather than passing quickly. Without that test the fix cannot be distinguished from the bug.
Anything else in the repo or CI that builds the image needs the build-arg too, or it will now fail — audit for other docker build invocations (script/docker, the Gitea workflow) and decide deliberately which of them must run the checks. script/docker builds the runtime image and arguably should not be forced through the check layers; say which way it goes and why.

Scope decision

Deliberately not folding this into PR #54, despite the overlap in Dockerfile. #54 is already a 144-finding conformance job carrying two open design decisions; adding a change to what CI means would enlarge an already-large PR and reopen review surface on something orthogonal. This stays its own commit-sized unit, to be done after #54 merges.

Important addendum to the fix, from the upstream prompts #26 discussion — the obvious implementation does not actually close the hole. **An unset `ARG` is an empty string, and an empty string is a stable cache key.** So a Dockerfile carrying `ARG CHECK_EPOCH` above the check `RUN` steps is still fully cacheable when built by anything that does not pass `--build-arg` — including a bare `docker build .` typed by hand, or any tooling other than `script/cibuild`. The green would be free again, and now with a fix in place that looks like it is working. The fix therefore needs a guard that fails closed: ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN make lint ``` so a build that omits the build-arg fails loudly instead of silently skipping the check. Adding to this issue's definition of done: - Each check step's `ARG CHECK_EPOCH` is paired with a `[ -n "$CHECK_EPOCH" ] || exit 1` guard, so an unset value is an error rather than a cache-friendly empty string. - Verification must include a **negative** case: a bare `docker build .` with no `--build-arg` **fails**, rather than passing quickly. Without that test the fix cannot be distinguished from the bug. - Anything else in the repo or CI that builds the image needs the build-arg too, or it will now fail — audit for other `docker build` invocations (`script/docker`, the Gitea workflow) and decide deliberately which of them must run the checks. `script/docker` builds the runtime image and arguably should not be forced through the check layers; say which way it goes and why. ## Scope decision Deliberately **not** folding this into PR #54, despite the overlap in `Dockerfile`. #54 is already a 144-finding conformance job carrying two open design decisions; adding a change to what CI *means* would enlarge an already-large PR and reopen review surface on something orthogonal. This stays its own commit-sized unit, to be done after #54 merges.

clawbot referenced this issue

2026-08-09 15:11:09 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

clawbot referenced this issue

2026-08-09 16:18:16 +02:00

script/lint does not pin the golangci-lint binary version, so local runs can false-green against a stale linter #58

Sign in to join this conversation.