sneak/pixa
1
0
Fork 0
Code Issues 11 Pull Requests 4 Actions 11 Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 07:37:54 +02:00 by clawbot · 4 comments
clawbot commented 2026-08-09 07:37:54 +02:00
Collaborator
Copy Link
Copy Source

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

  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.

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 commented 2026-08-09 15:09:42 +02:00
Author
Collaborator
Copy Link
Copy Source

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 commented 2026-10-03 14:28:15 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan, checked against current next:

  • Dockerfile: ARG CHECK_EPOCH in the lint stage directly above RUN make fmt-check, and in the build stage directly above RUN make test. Each check step names the value in its command (RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check, the same for make lint and make test), so a new value always reruns all three.
  • script/cibuild and script/docker set epoch="$(date +%s)$$" on a line of its own and pass --build-arg CHECK_EPOCH="$epoch". The header comment of script/cibuild stops claiming that any successful build implies a green repo, and says what does.
  • No step that fails the build when CHECK_EPOCH is unset, unlike the fleet's canonical form from sneak/prompts#26. A plain docker build . must keep working: upaas builds the image that way, and sneak asked for exactly that build to stamp the right version (#166). Such a build can reuse the check steps only for a build context identical byte for byte, .git included, which is a replay of checks that ran on the same bytes.
  • script/test and REPO_POLICIES.md are not touched.
  • Shown to the reviewer, not posted: two script/cibuild runs in a row on an unchanged tree both run all three checks while script/bootstrap stays cached; a plain docker build . still succeeds.

Model: opus-5-5

Plan, checked against current `next`: - `Dockerfile`: `ARG CHECK_EPOCH` in the lint stage directly above `RUN make fmt-check`, and in the build stage directly above `RUN make test`. Each check step names the value in its command (`RUN echo "check epoch: ${CHECK_EPOCH}" && make fmt-check`, the same for `make lint` and `make test`), so a new value always reruns all three. - `script/cibuild` and `script/docker` set `epoch="$(date +%s)$$"` on a line of its own and pass `--build-arg CHECK_EPOCH="$epoch"`. The header comment of `script/cibuild` stops claiming that any successful build implies a green repo, and says what does. - No step that fails the build when `CHECK_EPOCH` is unset, unlike the fleet's canonical form from https://git.eeqj.de/sneak/prompts/issues/26. A plain `docker build .` must keep working: upaas builds the image that way, and sneak asked for exactly that build to stamp the right version (https://git.eeqj.de/sneak/pixa/issues/166). Such a build can reuse the check steps only for a build context identical byte for byte, `.git` included, which is a replay of checks that ran on the same bytes. - `script/test` and `REPO_POLICIES.md` are not touched. - Shown to the reviewer, not posted: two `script/cibuild` runs in a row on an unchanged tree both run all three checks while `script/bootstrap` stays cached; a plain `docker build .` still succeeds. Model: opus-5-5
clawbot commented 2026-10-03 18:24:21 +02:00
Author
Collaborator
Copy Link
Copy Source

State, 16:35 UTC: built per the plan above as 0cef03d on issue-101-cibuild-cache, on top of next at 021516e; no PR yet. Its worker stopped at its account's weekly limit while verifying, before opening the PR. Left: a worker confirms script/cibuild and the checks the plan names on 0cef03d rebased onto next, opens the PR to next with needs-review; then an independent reviewer.

Model: opus-5-5

State, 16:35 UTC: built per the plan above as `0cef03d` on `issue-101-cibuild-cache`, on top of `next` at `021516e`; no PR yet. Its worker stopped at its account's weekly limit while verifying, before opening the PR. Left: a worker confirms `script/cibuild` and the checks the plan names on `0cef03d` rebased onto `next`, opens the PR to `next` with `needs-review`; then an independent reviewer. Model: opus-5-5
clawbot commented 2026-10-04 02:19:37 +02:00
Author
Collaborator
Copy Link
Copy Source

Built as planned above in #170, rebased onto current next; awaiting an independent review.

Model: opus-5-5

Built as planned above in https://git.eeqj.de/sneak/pixa/pulls/170, rebased onto current `next`; awaiting an independent review. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#101