sneak/lora.vegas
1
0
Fork 0
Code Issues 11 Pull Requests Actions Packages Projects Releases Wiki Activity

script/cibuild can report a green it did not earn: make check is served from Docker cache #23

New Issue
Closed
opened 2026-08-09 07:38:25 +02:00 by clawbot · 5 comments
clawbot commented 2026-08-09 07:38:25 +02:00
Collaborator
Copy Link
Copy Source

Problem

script/cibuild is docker build . with no cache control. The Dockerfile
ends with COPY . . then RUN make check. On an unchanged tree, Docker serves
the RUN make check layer from cache: the checks never execute and the build
still exits 0.

Reproduced in this repo just now. Second consecutive run against an unchanged
tree:

exit=0
CACHED layers: 5
#7 CACHED
#8 CACHED
#9 CACHED
#10 CACHED
#11 [6/6] RUN make check
#11 CACHED

No prettier output, no Hugo build output, no Total in ... line — because
nothing ran. The first run of the pair (cold cache) took real time and produced
real output. Both exit 0 and both look identical to anything that only checks
the exit code.

This is not theoretical here. During the review of PR #17 a reviewer reported
that its script/cibuild run was fully CACHED, and had to re-run it after
docker builder prune -af to get a real result. A gate that returns success
without running is worse than no gate, because it is trusted.

Reported fleet-wide; the same hole exists in the shared template, so this is
not a lora.vegas-specific defect. Tracked upstream in the prompts repo as its
issue #26.

Why this matters more than it looks

REPO_POLICIES.md requires main to always pass make check and requires the
Dockerfile to run make check so "the build fails if the branch is not
green". A cached layer satisfies neither while appearing to satisfy both.

It also interacts badly with how this repo just spent an outage: PR #17 shipped
a production break with a green script/cibuild. The cache hole was not the
cause there — docker build structurally cannot exercise the Actions runtime —
but it is the same category of defect. A green that proves nothing.

Fix

The upstream-proposed fix is a cache-busting build argument, e.g.:

ARG CHECK_EPOCH=0
RUN make check

with script/cibuild passing a changing value so the check layer is never
reused. Confirm the exact shape against the upstream prompts issue #26 fix
rather than inventing a local variant — this should stay identical across
repos, since script/cibuild is one of the scripts that is meant to be
byte-identical everywhere.

Whatever shape is adopted, --no-cache on the whole build is the wrong answer:
it would also discard the script/bootstrap layer, turning a ~10-second check
into a full toolchain reinstall every time and blowing the "Docker builds must
complete in under 5 minutes" budget for no benefit. Only the check layer needs
to be unconditional.

Definition of done

  1. script/cibuild cannot serve make check from cache. Demonstrate by
    running it twice in a row against an unchanged tree and showing the check
    output present in both runs — paste both, not just the second.
  2. The script/bootstrap layer is still cached between runs (verify the second
    run does not reinstall hugo/node), so build time stays reasonable.
  3. script/cibuild still exits non-zero when a check genuinely fails. Prove it:
    deliberately break formatting in a scratch commit, confirm the build fails,
    then revert.
  4. The fix matches the upstream prompts resolution so script/cibuild stays
    consistent across repos. If it must diverge, say why in the PR.
  5. make check passes and script/cibuild succeeds.
  6. TODO.md updated in the same commit.

Depends on

Upstream prompts #26 settling the canonical fix. If that is still open when
this is picked up, either wait or implement the canonical shape and note that
it may need syncing. Do not invent a lora.vegas-only mechanism.

Out of scope

  • Making warnings fatal, or any other change to what make check runs. This
    issue is solely about the gate actually executing.
  • The separate problem that docker build cannot exercise the Gitea Actions
    runtime at all. That is inherent, was the subject of #7, and no cache fix
    addresses it.
## Problem `script/cibuild` is `docker build .` with no cache control. The `Dockerfile` ends with `COPY . .` then `RUN make check`. On an unchanged tree, Docker serves the `RUN make check` layer from cache: **the checks never execute and the build still exits 0.** Reproduced in this repo just now. Second consecutive run against an unchanged tree: ``` exit=0 CACHED layers: 5 #7 CACHED #8 CACHED #9 CACHED #10 CACHED #11 [6/6] RUN make check #11 CACHED ``` No prettier output, no Hugo build output, no `Total in ...` line — because nothing ran. The first run of the pair (cold cache) took real time and produced real output. Both exit 0 and both look identical to anything that only checks the exit code. This is not theoretical here. During the review of PR #17 a reviewer reported that its `script/cibuild` run was fully `CACHED`, and had to re-run it after `docker builder prune -af` to get a real result. A gate that returns success without running is worse than no gate, because it is trusted. Reported fleet-wide; the same hole exists in the shared template, so this is not a lora.vegas-specific defect. Tracked upstream in the `prompts` repo as its issue #26. ## Why this matters more than it looks `REPO_POLICIES.md` requires `main` to always pass `make check` and requires the `Dockerfile` to run `make check` so "the build fails if the branch is not green". A cached layer satisfies neither while appearing to satisfy both. It also interacts badly with how this repo just spent an outage: PR #17 shipped a production break with a green `script/cibuild`. The cache hole was not the cause there — `docker build` structurally cannot exercise the Actions runtime — but it is the same category of defect. A green that proves nothing. ## Fix The upstream-proposed fix is a cache-busting build argument, e.g.: ```dockerfile ARG CHECK_EPOCH=0 RUN make check ``` with `script/cibuild` passing a changing value so the check layer is never reused. Confirm the exact shape against the upstream `prompts` issue #26 fix rather than inventing a local variant — this should stay identical across repos, since `script/cibuild` is one of the scripts that is meant to be byte-identical everywhere. Whatever shape is adopted, `--no-cache` on the whole build is the wrong answer: it would also discard the `script/bootstrap` layer, turning a ~10-second check into a full toolchain reinstall every time and blowing the "Docker builds must complete in under 5 minutes" budget for no benefit. Only the check layer needs to be unconditional. ## Definition of done 1. `script/cibuild` cannot serve `make check` from cache. Demonstrate by running it twice in a row against an unchanged tree and showing the check output present in **both** runs — paste both, not just the second. 2. The `script/bootstrap` layer is still cached between runs (verify the second run does not reinstall hugo/node), so build time stays reasonable. 3. `script/cibuild` still exits non-zero when a check genuinely fails. Prove it: deliberately break formatting in a scratch commit, confirm the build fails, then revert. 4. The fix matches the upstream `prompts` resolution so `script/cibuild` stays consistent across repos. If it must diverge, say why in the PR. 5. `make check` passes and `script/cibuild` succeeds. 6. `TODO.md` updated in the same commit. ## Depends on Upstream `prompts` #26 settling the canonical fix. If that is still open when this is picked up, either wait or implement the canonical shape and note that it may need syncing. Do not invent a lora.vegas-only mechanism. ## Out of scope - Making warnings fatal, or any other change to what `make check` runs. This issue is solely about the gate actually executing. - The separate problem that `docker build` cannot exercise the Gitea Actions runtime at all. That is inherent, was the subject of #7, and no cache fix addresses it.
clawbot commented 2026-08-09 07:42:56 +02:00
Author
Collaborator
Copy Link
Copy Source

Ordering dependency: land this issue before the .dockerignore changes in
#8, and re-verify this issue's behaviour afterwards.

A fleet-wide warning came in saying that repos with no .dockerignore are
accidentally protected from the cache hole: .git lands in the build context,
.git churns on nearly every git operation, so COPY . . is invalidated
constantly and the check layers are forced to re-run. Adopting the canonical
.dockerignore excludes .git, removes the churn, and converts an accidentally
safe repo into one that reliably reports unearned greens.

That specific mechanism does not apply to lora.vegas. Verified just now —
.dockerignore here already contains .git:

.git
public
resources
.hugo_build.lock

Which is exactly why the cache hole was reproducible in the first place. This
repo has never had the accidental protection, so there is nothing here to lose.

But the ordering conclusion still holds, by a different route. #8 adds
.claude/ to .dockerignore. .claude/ currently holds worktrees/, which
is created and destroyed constantly by tooling — so it is, right now, a live
source of build-context churn that intermittently invalidates COPY . . and
forces the check layers to re-run. It is the same accidental protection, just
via a different directory.

So excluding .claude/ will make caching stickier and more consistent,
which is desirable on its own terms but removes a source of accidental
re-execution. If #8 lands first, the gate gets quietly weaker and more reliably
so, with nothing in the output to signal it.

Required order:

  1. Land this issue (#23). Verify with two consecutive runs on an unchanged tree
    that the check layer executes in both, and that the script/bootstrap
    layer still caches.
  2. Then land the .dockerignore/.claude exclusion from #8.
  3. Re-verify #23's two-run check afterwards, because excluding .claude/
    changes the exact cache behaviour validated in step 1. A fix verified before
    the context changed is not evidence about the context after.

Step 3 is the one most likely to be skipped, so it is called out as its own
item rather than as a parenthetical. Note also that this is the same class of
mistake that caused the #7 outage: validating against one environment and
assuming the result transfers to a changed one.

Recorded upstream on prompts #27.

Ordering dependency: **land this issue before the `.dockerignore` changes in #8**, and re-verify this issue's behaviour afterwards. A fleet-wide warning came in saying that repos with **no** `.dockerignore` are accidentally protected from the cache hole: `.git` lands in the build context, `.git` churns on nearly every git operation, so `COPY . .` is invalidated constantly and the check layers are forced to re-run. Adopting the canonical `.dockerignore` excludes `.git`, removes the churn, and converts an accidentally safe repo into one that reliably reports unearned greens. **That specific mechanism does not apply to lora.vegas.** Verified just now — `.dockerignore` here already contains `.git`: ``` .git public resources .hugo_build.lock ``` Which is exactly why the cache hole was reproducible in the first place. This repo has never had the accidental protection, so there is nothing here to lose. **But the ordering conclusion still holds, by a different route.** #8 adds `.claude/` to `.dockerignore`. `.claude/` currently holds `worktrees/`, which is created and destroyed constantly by tooling — so it is, right now, a live source of build-context churn that intermittently invalidates `COPY . .` and forces the check layers to re-run. It is the same accidental protection, just via a different directory. So excluding `.claude/` will make caching **stickier and more consistent**, which is desirable on its own terms but removes a source of accidental re-execution. If #8 lands first, the gate gets quietly weaker and more reliably so, with nothing in the output to signal it. Required order: 1. Land this issue (#23). Verify with two consecutive runs on an unchanged tree that the check layer executes in **both**, and that the `script/bootstrap` layer still caches. 2. Then land the `.dockerignore`/`.claude` exclusion from #8. 3. **Re-verify #23's two-run check afterwards**, because excluding `.claude/` changes the exact cache behaviour validated in step 1. A fix verified before the context changed is not evidence about the context after. Step 3 is the one most likely to be skipped, so it is called out as its own item rather than as a parenthetical. Note also that this is the same class of mistake that caused the #7 outage: validating against one environment and assuming the result transfers to a changed one. Recorded upstream on `prompts` #27.
clawbot reopened this issue 2026-08-09 12:21:21 +02:00
clawbot commented 2026-08-09 12:21:53 +02:00
Author
Collaborator
Copy Link
Copy Source

Reopened

This issue was in closed state when picked up, but nothing on main (961ec71)
implements it: the Dockerfile still ends with a bare COPY . . / RUN make check
and script/cibuild is still a bare docker build .. No merged PR references #23 —
the close timestamp coincides to within two seconds with the merge of PR #24
(closes #9), so this looks like a mis-close rather than a decision. Reopened; it
will close via the PR.

Implementation plan

Upstream prompts #26 is still open, but its canonical shape has settled across
fifteen comments and four independent empirical confirmations (cattbox, rfscan, and
two separate dnswatcher probes). I am adopting that settled shape verbatim rather
than the ARG CHECK_EPOCH=0 sketch in this issue's body, which is superseded — a
default value is a stable cache key, which is precisely the defect.

Dockerfile

Immediately above the check step, replacing RUN make check:

ARG CHECK_EPOCH
RUN [ -n "$CHECK_EPOCH" ] || exit 1
RUN echo "check epoch: ${CHECK_EPOCH}" && make check

Three deliberate choices, each traceable to an upstream finding:

  • ARG placed after COPY . ., not before. Everything at or below the ARG
    is invalidated; everything above it — including COPY script/ script/ and
    RUN script/bootstrap — keeps caching. Placing it higher would destroy
    dependency caching and turn a ~10s check into a full toolchain reinstall.
  • No default value. ARG CHECK_EPOCH=0 would make 0 the value on every
    invocation, i.e. a constant, i.e. cached — the bug unchanged.
  • The value is expanded into the RUN command. The bare unreferenced-ARG
    form does work here (four independent measurements upstream agree, and the one
    contradicting claim was retracted), so expansion is hardening, not a fix: it makes
    the cache miss contractual instead of dependent on BuildKit's handling of an
    unreferenced ARG, and it prints the epoch into the build log so a reader can see
    the layer was keyed fresh.
  • The [ -n ... ] guard. An unset ARG is the empty string, and empty is a
    stable cache key — so without the guard a bare docker build . (the command
    REPO_POLICIES.md names verbatim) would still produce the false green. Failed
    steps are never cached, so this fails on every such invocation rather than once:
    the documented-but-unscripted path becomes a loud error instead of a quiet lie.

script/cibuild

epoch="$(date +%s%N)$$"
docker build --build-arg CHECK_EPOCH="$epoch" .
  • Assigned to a variable first rather than inlined as
    --build-arg CHECK_EPOCH="$(date +%s%N)". A command substitution that fails
    inside an argument does not trip set -e (confirmed upstream in dash), so
    the inline form would silently degrade to an empty constant and restore the false
    green. As a standalone assignment, set -e catches it.
  • %N rather than %s: this host runs ~18 concurrent sessions, and second
    granularity lets two concurrent invocations collide on an identical key.
  • The trailing $$ is not decoration. busybox date silently drops %N and
    exits 0 — verified upstream inside a pinned alpine image — so on a busybox host
    the epoch would degrade to seconds with no warning. The PID differs between
    concurrent invocations regardless, making the guarantee unconditional. Both %N
    and $$ are POSIX-safe in the sense that matters: %N degrades silently and $$
    covers the degradation.

script/docker

Gets the same --build-arg. This is a required consequence of the guard, not scope
creep: without it the guard would break make docker outright. Upstream independently
recommends it for its own reasons — local builds are almost always warm and nobody
watches make docker for a suspicious duration — and leaving the two entrypoints
divergent would have them silently disagree about whether the tree is green.

Not doing

  • No change to what make check runs (explicitly out of scope).
  • Not touching .dockerignore — #8 lands after this, per the ordering recorded above.
  • Not implementing the upstream suggestion that script/cibuild grep its own output
    for CACHED on the check layer. That did not make it into the settled canonical
    form, and inventing it here would be exactly the local variant this issue forbids.
    It belongs upstream if it belongs anywhere.

Verification I will run and paste

  1. Two consecutive script/cibuild runs on an unchanged tree, both pasted, with
    per-run CACHED step counts. Acceptance is that both show real check output (both
    Hugo builds and the prettier line) and that the script/bootstrap layer shows
    CACHED in run 2. That second half is the validity control, not just a performance
    check: on a shared host a pair that spanned a cache eviction would show bootstrap
    re-executing, and would have to be discarded rather than believed.
  2. A negative control: revert only script/cibuild to plain docker build .,
    keeping the Dockerfile change, and confirm the false green returns. Without this,
    a passing pair shows only that the build re-ran, not that CHECK_EPOCH is why.
    (The guard means this now surfaces as a loud failure rather than a cached green,
    which is a stronger result; I will report exactly what it produces.)
  3. A genuine failure: break markdown formatting so prettier fails, confirm non-zero
    exit, revert.
  4. make check green.

No docker builder prune in any form, and no --no-cache on the whole build. Any
scoped invalidation needed for diagnosis will use --no-cache-filter.

## Reopened This issue was in `closed` state when picked up, but nothing on `main` (`961ec71`) implements it: the `Dockerfile` still ends with a bare `COPY . .` / `RUN make check` and `script/cibuild` is still a bare `docker build .`. No merged PR references #23 — the close timestamp coincides to within two seconds with the merge of PR #24 (`closes #9`), so this looks like a mis-close rather than a decision. Reopened; it will close via the PR. ## Implementation plan Upstream `prompts` #26 is still open, but its canonical shape has settled across fifteen comments and four independent empirical confirmations (cattbox, rfscan, and two separate dnswatcher probes). I am adopting that settled shape verbatim rather than the `ARG CHECK_EPOCH=0` sketch in this issue's body, which is superseded — a default value is a stable cache key, which is precisely the defect. ### `Dockerfile` Immediately above the check step, replacing `RUN make check`: ``` ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make check ``` Three deliberate choices, each traceable to an upstream finding: - **`ARG` placed after `COPY . .`, not before.** Everything at or below the `ARG` is invalidated; everything above it — including `COPY script/ script/` and `RUN script/bootstrap` — keeps caching. Placing it higher would destroy dependency caching and turn a ~10s check into a full toolchain reinstall. - **No default value.** `ARG CHECK_EPOCH=0` would make `0` the value on every invocation, i.e. a constant, i.e. cached — the bug unchanged. - **The value is expanded into the `RUN` command.** The bare unreferenced-`ARG` form does work here (four independent measurements upstream agree, and the one contradicting claim was retracted), so expansion is hardening, not a fix: it makes the cache miss contractual instead of dependent on BuildKit's handling of an unreferenced `ARG`, and it prints the epoch into the build log so a reader can see the layer was keyed fresh. - **The `[ -n ... ]` guard.** An unset `ARG` is the empty string, and empty is a stable cache key — so without the guard a bare `docker build .` (the command `REPO_POLICIES.md` names verbatim) would still produce the false green. Failed steps are never cached, so this fails on every such invocation rather than once: the documented-but-unscripted path becomes a loud error instead of a quiet lie. ### `script/cibuild` ``` epoch="$(date +%s%N)$$" docker build --build-arg CHECK_EPOCH="$epoch" . ``` - Assigned to a variable first rather than inlined as `--build-arg CHECK_EPOCH="$(date +%s%N)"`. A command substitution that fails inside an argument does **not** trip `set -e` (confirmed upstream in `dash`), so the inline form would silently degrade to an empty constant and restore the false green. As a standalone assignment, `set -e` catches it. - `%N` rather than `%s`: this host runs ~18 concurrent sessions, and second granularity lets two concurrent invocations collide on an identical key. - The trailing `$$` is not decoration. busybox `date` **silently drops `%N`** and exits 0 — verified upstream inside a pinned alpine image — so on a busybox host the epoch would degrade to seconds with no warning. The PID differs between concurrent invocations regardless, making the guarantee unconditional. Both `%N` and `$$` are POSIX-safe in the sense that matters: `%N` degrades silently and `$$` covers the degradation. ### `script/docker` Gets the same `--build-arg`. This is a required consequence of the guard, not scope creep: without it the guard would break `make docker` outright. Upstream independently recommends it for its own reasons — local builds are almost always warm and nobody watches `make docker` for a suspicious duration — and leaving the two entrypoints divergent would have them silently disagree about whether the tree is green. ### Not doing - No change to what `make check` runs (explicitly out of scope). - Not touching `.dockerignore` — #8 lands after this, per the ordering recorded above. - Not implementing the upstream suggestion that `script/cibuild` grep its own output for `CACHED` on the check layer. That did not make it into the settled canonical form, and inventing it here would be exactly the local variant this issue forbids. It belongs upstream if it belongs anywhere. ### Verification I will run and paste 1. Two consecutive `script/cibuild` runs on an unchanged tree, **both** pasted, with per-run `CACHED` step counts. Acceptance is that both show real check output (both Hugo builds and the prettier line) **and** that the `script/bootstrap` layer shows `CACHED` in run 2. That second half is the validity control, not just a performance check: on a shared host a pair that spanned a cache eviction would show bootstrap re-executing, and would have to be discarded rather than believed. 2. A negative control: revert **only** `script/cibuild` to plain `docker build .`, keeping the `Dockerfile` change, and confirm the false green returns. Without this, a passing pair shows only that the build re-ran, not that `CHECK_EPOCH` is why. (The guard means this now surfaces as a loud failure rather than a cached green, which is a stronger result; I will report exactly what it produces.) 3. A genuine failure: break markdown formatting so prettier fails, confirm non-zero exit, revert. 4. `make check` green. No `docker builder prune` in any form, and no `--no-cache` on the whole build. Any scoped invalidation needed for diagnosis will use `--no-cache-filter`.
clawbot commented 2026-08-09 16:15:20 +02:00
Author
Collaborator
Copy Link
Copy Source

Status — implementation was started and stopped part-way when the
implementer hit an account capacity limit. No PR opened. Partial work is
preserved and pushed as branch fix/23-cibuild-check-epoch (commit b6e0be8,
titled WIP: and deliberately carrying refs #23, not a closing token).

Pushing it rather than discarding it because the analysis in it is worth
keeping, but it must not be merged as-is and the branch is not a candidate
for review.

What the partial work got right

The mechanism is sound and the reasoning behind each choice is documented in
the diff:

  • ARG CHECK_EPOCH declared with no default, on the grounds that a default
    would be a constant and a constant is a stable cache key — i.e. the defect
    unchanged. That is correct and is the trap most naive versions of this fix
    fall into.
  • The epoch is expanded into the RUN command rather than merely declared,
    so the cache miss does not depend on BuildKit's handling of a
    declared-but-unreferenced ARG, and the value shows up in the build log.
  • In script/cibuild, the value is assigned to a variable rather than
    substituted inline, because a failing command substitution inside an argument
    list does not trip set -e — the inline form would silently pass an empty
    string and restore the cached false green. As the whole of an assignment its
    exit status is the command's, so set -e catches it.
  • %N for sub-second distinctness with $$ appended, since busybox date
    silently drops %N and still exits 0.

Everything above COPY . . still caches, so the script/bootstrap toolchain
layer is preserved — which was the explicit constraint.

Why it is not mergeable

  1. make docker would break. The Dockerfile guard fails the build when
    CHECK_EPOCH is unset, and script/docker was never updated to pass it.
    Verified: script/docker still runs a bare docker build -t ... .. The
    Dockerfile header comment already claims both script/cibuild and
    script/docker pass the argument — that claim is currently false, which is
    exactly the class of stale-documentation defect #9 was about.
  2. It diverges from the canonical upstream shape tracked in prompts #26.
    The implementer's own last note was that it intended to simplify to match
    upstream rather than keep the bespoke guard. script/cibuild is meant to be
    byte-identical across repos, so a lora.vegas-only variant is the wrong
    outcome.
  3. Nothing was verified. Neither the two-consecutive-runs proof nor the
    deliberate-failure proof required by this issue's definition of done was
    run. Given this repo's history, an unverified cache fix is worth nothing —
    the whole point is that it is easy to believe a gate works when it does not.

For whoever picks this up

Treat the branch as notes, not as a starting point to polish. Decide first
whether the guard belongs at all: it is defensible (it makes a bare docker build . fail loudly rather than silently returning a false green) but it is an
addition beyond what upstream specifies, and it forces every entrypoint that
builds the image to pass the argument. If it is kept, script/docker must pass
CHECK_EPOCH too and the two scripts should generate the value identically.

The definition of done in the issue body is unchanged and still governs,
including that both consecutive runs must be pasted, that the bootstrap layer
must still cache, and that a deliberate check failure must be shown to fail the
build.

Also unchanged: the ordering dependency with #8. This lands before the
.dockerignore work, and #8's .dockerignore change requires re-verifying
this issue's two-run proof afterwards.

Status — implementation was started and **stopped part-way when the implementer hit an account capacity limit**. No PR opened. Partial work is preserved and pushed as branch `fix/23-cibuild-check-epoch` (commit `b6e0be8`, titled `WIP:` and deliberately carrying `refs #23`, not a closing token). Pushing it rather than discarding it because the analysis in it is worth keeping, but **it must not be merged as-is** and the branch is not a candidate for review. ## What the partial work got right The mechanism is sound and the reasoning behind each choice is documented in the diff: - `ARG CHECK_EPOCH` declared with **no default**, on the grounds that a default would be a constant and a constant is a stable cache key — i.e. the defect unchanged. That is correct and is the trap most naive versions of this fix fall into. - The epoch is **expanded into the `RUN` command** rather than merely declared, so the cache miss does not depend on BuildKit's handling of a declared-but-unreferenced `ARG`, and the value shows up in the build log. - In `script/cibuild`, the value is assigned to a variable rather than substituted inline, because a failing command substitution inside an argument list does not trip `set -e` — the inline form would silently pass an empty string and restore the cached false green. As the whole of an assignment its exit status is the command's, so `set -e` catches it. - `%N` for sub-second distinctness with `$$` appended, since busybox `date` silently drops `%N` and still exits 0. Everything above `COPY . .` still caches, so the `script/bootstrap` toolchain layer is preserved — which was the explicit constraint. ## Why it is not mergeable 1. **`make docker` would break.** The `Dockerfile` guard fails the build when `CHECK_EPOCH` is unset, and `script/docker` was never updated to pass it. Verified: `script/docker` still runs a bare `docker build -t ... .`. The `Dockerfile` header comment already claims both `script/cibuild` and `script/docker` pass the argument — that claim is currently false, which is exactly the class of stale-documentation defect #9 was about. 2. **It diverges from the canonical upstream shape** tracked in `prompts` #26. The implementer's own last note was that it intended to simplify to match upstream rather than keep the bespoke guard. `script/cibuild` is meant to be byte-identical across repos, so a lora.vegas-only variant is the wrong outcome. 3. **Nothing was verified.** Neither the two-consecutive-runs proof nor the deliberate-failure proof required by this issue's definition of done was run. Given this repo's history, an unverified cache fix is worth nothing — the whole point is that it is easy to believe a gate works when it does not. ## For whoever picks this up Treat the branch as notes, not as a starting point to polish. Decide first whether the guard belongs at all: it is defensible (it makes a bare `docker build .` fail loudly rather than silently returning a false green) but it is an addition beyond what upstream specifies, and it forces every entrypoint that builds the image to pass the argument. If it is kept, `script/docker` must pass `CHECK_EPOCH` too and the two scripts should generate the value identically. The definition of done in the issue body is unchanged and still governs, including that both consecutive runs must be pasted, that the bootstrap layer must still cache, and that a deliberate check failure must be shown to fail the build. Also unchanged: the ordering dependency with #8. This lands before the `.dockerignore` work, and #8's `.dockerignore` change requires re-verifying this issue's two-run proof afterwards.
clawbot commented 2026-08-09 17:48:25 +02:00
Author
Collaborator
Copy Link
Copy Source

Implementation plan (picking this up after the stalled partial work)

Branch: fix/23-cibuild-check-epoch-v2, from main at 8034fd8. The earlier
fix/23-cibuild-check-epoch branch is treated as notes only, per the status
comment above; it is not being polished forward.

Upstream status, checked first

sneak/prompts #26 has not landed on that repo's main — script/cibuild
there is still a bare docker build .. The canonical form has, however,
settled: it is implemented on that repo's next branch (commit 51c3945,
carried by its PR #34) and the #26 portion passed independent re-review. PR
#34 is still open only because a later commit on it, for a different issue,
needs rework.

So I am adopting the settled canonical four-element form verbatim rather than
inventing anything, and the PR will note that upstream has not merged yet and
may need re-syncing. Propagation to consuming repos is tracked upstream
separately as sneak/prompts #35.

Guard decision: keep it

The caller asked for a deliberate decision on whether the
[ -n "$CHECK_EPOCH" ] || exit 1 guard belongs. Keeping it, for the reason it
was added upstream rather than as a preference: an unset ARG is the empty
string, and empty is a stable cache key. Upstream measured a repo that had
"landed the fix" still producing the original false green through a bare
docker build . — checks ran on the cold run, then exit 0 in 0s with every
check layer CACHED on the warm one. Documentation-only mitigation was shown
insufficient. Without the guard the fix leaves the exact defect reachable
through the one command that a reviewer diagnosing a build is most likely to
type by hand, which in this repo has already burned three separate reviewers.

The cost is that every entrypoint building the image must pass the argument, so
script/docker gets it too and both scripts generate the value identically.
That was precisely what made the previous attempt unmergeable.

Dockerfile

Below COPY . ., replacing RUN make check:

ARG CHECK_EPOCH
RUN [ -n "$CHECK_EPOCH" ] || exit 1
RUN echo "check epoch: ${CHECK_EPOCH}" && make check

ARG is stage-scoped and must be redeclared in every stage running checks;
this image is single-stage, so one declaration is correct and complete. No
default value. The value is expanded into the RUN — this is hardening, not
the fix: the bare unreferenced-ARG form does work, but expansion makes the
cache miss contractual rather than dependent on BuildKit's handling of an
unreferenced ARG, and prints the epoch into the log. Both the guard and the
check RUN reference the value, so there are two independent invalidation
points, not one; both stay.

Placement below COPY . . is what preserves the script/bootstrap layer,
which on this repo compiles Hugo from source.

script/cibuild and script/docker

epoch="$(date +%s%N)$$"
docker build --build-arg CHECK_EPOCH="$epoch" .

Assigned on its own line, never inlined into the argument list: a failing
command substitution inside an argument does not trip set -e, so the inline
form would degrade to an empty string and restore the cached false green. %N
for sub-second distinctness on a host running many concurrent sessions, $$
because busybox date silently drops %N and still exits 0.

script/docker gets the identical two lines with its -t tag.

Docs

README.md's Entrypoints section claimed script/cibuild is docker build .;
corrected, plus a short note that the image must be built through the scripts.
This repo has no REPO_POLICIES.md or checklist files yet, so the upstream
prose sweep has no other targets here — git grep -nF 'docker build' now
returns only the two canonical --build-arg invocations and sites describing
the bare command as failing closed by design.

Not doing

  • No change to what make check runs.
  • Not touching .dockerignore; #8 lands after this, per the ordering above.
  • Not implementing the "grep the build output for CACHED" self-check. That
    was explicitly rejected upstream — the guard already turns the regression
    case into a hard failure, and grepping progress output is format-dependent.

Verification to be pasted

  1. Both consecutive script/cibuild runs on an unchanged tree, in full, with
    wall-clock times.
  2. Validity control: RUN script/bootstrap must show CACHED in run 2. This
    is not a performance note — it is what rules out the pair having spanned a
    cache eviction, and rules out an accidental whole-build --no-cache.
  3. Counterfactual: pass a constant CHECK_EPOCH twice and confirm the false
    green returns. Reverting script/cibuild to a bare docker build . is no
    longer a usable counterfactual once the guard exists, since that now fails.
  4. Planted defect: break markdown formatting so prettier fails, confirm
    non-zero exit, revert.
  5. make docker works; make check green.

No docker builder prune in any form, and no whole-build --no-cache.

## Implementation plan (picking this up after the stalled partial work) Branch: `fix/23-cibuild-check-epoch-v2`, from `main` at `8034fd8`. The earlier `fix/23-cibuild-check-epoch` branch is treated as notes only, per the status comment above; it is not being polished forward. ### Upstream status, checked first `sneak/prompts` #26 has **not** landed on that repo's `main` — `script/cibuild` there is still a bare `docker build .`. The canonical form has, however, settled: it is implemented on that repo's `next` branch (commit `51c3945`, carried by its PR #34) and the `#26` portion passed independent re-review. PR #34 is still open only because a later commit on it, for a different issue, needs rework. So I am adopting the settled canonical four-element form verbatim rather than inventing anything, and the PR will note that upstream has not merged yet and may need re-syncing. Propagation to consuming repos is tracked upstream separately as `sneak/prompts` #35. ### Guard decision: keep it The caller asked for a deliberate decision on whether the `[ -n "$CHECK_EPOCH" ] || exit 1` guard belongs. Keeping it, for the reason it was added upstream rather than as a preference: an unset `ARG` is the empty string, and empty is a *stable cache key*. Upstream measured a repo that had "landed the fix" still producing the original false green through a bare `docker build .` — checks ran on the cold run, then exit 0 in 0s with every check layer `CACHED` on the warm one. Documentation-only mitigation was shown insufficient. Without the guard the fix leaves the exact defect reachable through the one command that a reviewer diagnosing a build is most likely to type by hand, which in this repo has already burned three separate reviewers. The cost is that every entrypoint building the image must pass the argument, so `script/docker` gets it too and both scripts generate the value identically. That was precisely what made the previous attempt unmergeable. ### `Dockerfile` Below `COPY . .`, replacing `RUN make check`: ``` ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" && make check ``` `ARG` is stage-scoped and must be redeclared in every stage running checks; this image is single-stage, so one declaration is correct and complete. No default value. The value is expanded into the `RUN` — this is hardening, not the fix: the bare unreferenced-`ARG` form does work, but expansion makes the cache miss contractual rather than dependent on BuildKit's handling of an unreferenced `ARG`, and prints the epoch into the log. Both the guard and the check `RUN` reference the value, so there are two independent invalidation points, not one; both stay. Placement below `COPY . .` is what preserves the `script/bootstrap` layer, which on this repo compiles Hugo from source. ### `script/cibuild` and `script/docker` ``` epoch="$(date +%s%N)$$" docker build --build-arg CHECK_EPOCH="$epoch" . ``` Assigned on its own line, never inlined into the argument list: a failing command substitution inside an argument does not trip `set -e`, so the inline form would degrade to an empty string and restore the cached false green. `%N` for sub-second distinctness on a host running many concurrent sessions, `$$` because busybox `date` silently drops `%N` and still exits 0. `script/docker` gets the identical two lines with its `-t` tag. ### Docs `README.md`'s Entrypoints section claimed `script/cibuild` is `docker build .`; corrected, plus a short note that the image must be built through the scripts. This repo has no `REPO_POLICIES.md` or checklist files yet, so the upstream prose sweep has no other targets here — `git grep -nF 'docker build'` now returns only the two canonical `--build-arg` invocations and sites describing the bare command as failing closed by design. ### Not doing - No change to what `make check` runs. - Not touching `.dockerignore`; #8 lands after this, per the ordering above. - Not implementing the "grep the build output for `CACHED`" self-check. That was explicitly rejected upstream — the guard already turns the regression case into a hard failure, and grepping progress output is format-dependent. ### Verification to be pasted 1. Both consecutive `script/cibuild` runs on an unchanged tree, in full, with wall-clock times. 2. Validity control: `RUN script/bootstrap` must show `CACHED` in run 2. This is not a performance note — it is what rules out the pair having spanned a cache eviction, and rules out an accidental whole-build `--no-cache`. 3. Counterfactual: pass a *constant* `CHECK_EPOCH` twice and confirm the false green returns. Reverting `script/cibuild` to a bare `docker build .` is no longer a usable counterfactual once the guard exists, since that now fails. 4. Planted defect: break markdown formatting so prettier fails, confirm non-zero exit, revert. 5. `make docker` works; `make check` green. No `docker builder prune` in any form, and no whole-build `--no-cache`.
clawbot commented 2026-08-09 17:52:31 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in #30 (branch
fix/23-cibuild-check-epoch-v2, commit 223c520). Full evidence is pasted in
the PR body; summary against this issue's definition of done below.

1. script/cibuild cannot serve RUN make check from cache. The
Dockerfile declares ARG CHECK_EPOCH with no default immediately below
COPY . ., guards it with RUN [ -n "$CHECK_EPOCH" ] || exit 1, and expands
it into the check command. script/cibuild and script/docker both build
epoch="$(date +%s%N)$$" on its own line and pass it.

2. Two consecutive runs, both executing the checks. Yes — both pasted in
full in the PR. Both show two Hugo builds and
All matched files use Prettier code style!, with different epochs printed.
15s then 6s wall.

3. The script/bootstrap layer still caches. Run 2 shows
#8 [4/7] RUN script/bootstrap → CACHED. Hugo was not recompiled. COPY . .
is CACHED in run 2 as well, which is the mechanism working rather than a
problem: the content hash is unchanged — that is the defect — and the epoch
below it is what forces the checks to run anyway.

4. A genuine failure still fails. Appended badly-wrapped prose to
README.md; script/cibuild exited 1 on
[warn] Code style issues found in the above file. Reverted.

5. make docker works. Exit 0, bootstrap CACHED, check layer executed,
image tagged. This required updating script/docker to pass the argument,
which is the gap that made the earlier branch unmergeable.

6. make check passes. Green on the host.

7. TODO.md updated in the same commit.

Two things worth flagging

Guard decision: kept. Reasoning in the PR. Short version: an unset ARG is
the empty string and empty is a stable cache key, so without the guard the
original defect stays reachable through a bare docker build .. Upstream
measured a repo that had "landed the fix" and still produced the false green
that way. The accepted consequence is that a bare docker build . now fails by
design, exit 1, and keeps failing on repeat since failed steps are not cached.

A counterfactual was run, and it is the part I would look at first in
review. Two consecutive runs proving the checks ran does not by itself prove
CHECK_EPOCH is why — the tree had just changed. Passing a constant
CHECK_EPOCH twice reproduced the original false green on the fixed
Dockerfile: every layer CACHED, exit 0, no check output. So the
per-invocation value is the active ingredient, not the file edit.

Note that the counterfactual named in the earlier plan comment — reverting
script/cibuild to a bare docker build . — is no longer usable once the
guard exists, because that path now fails rather than producing a green. The
constant-epoch form replaces it and is the stronger test anyway.

Ordering

This lands before the .dockerignore work in
#8, per the ordering comment above.
The two-run proof here will need re-running after that change, since excluding
.claude/ removes a current source of build-context churn. Not something this
PR can pre-empt.

Upstream

sneak/prompts #26 has not merged to that repo's main yet — the shape has
settled and is implemented on its next branch, and that is what this adopts
verbatim. Flagged in the PR body that it may need re-syncing once upstream
merges.

Implemented in https://git.eeqj.de/sneak/lora.vegas/pulls/30 (branch `fix/23-cibuild-check-epoch-v2`, commit `223c520`). Full evidence is pasted in the PR body; summary against this issue's definition of done below. **1. `script/cibuild` cannot serve `RUN make check` from cache.** The `Dockerfile` declares `ARG CHECK_EPOCH` with no default immediately below `COPY . .`, guards it with `RUN [ -n "$CHECK_EPOCH" ] || exit 1`, and expands it into the check command. `script/cibuild` and `script/docker` both build `epoch="$(date +%s%N)$$"` on its own line and pass it. **2. Two consecutive runs, both executing the checks.** Yes — both pasted in full in the PR. Both show two Hugo builds and `All matched files use Prettier code style!`, with different epochs printed. 15s then 6s wall. **3. The `script/bootstrap` layer still caches.** Run 2 shows `#8 [4/7] RUN script/bootstrap` → `CACHED`. Hugo was not recompiled. `COPY . .` is `CACHED` in run 2 as well, which is the mechanism working rather than a problem: the content hash is unchanged — that is the defect — and the epoch below it is what forces the checks to run anyway. **4. A genuine failure still fails.** Appended badly-wrapped prose to `README.md`; `script/cibuild` exited 1 on `[warn] Code style issues found in the above file`. Reverted. **5. `make docker` works.** Exit 0, bootstrap `CACHED`, check layer executed, image tagged. This required updating `script/docker` to pass the argument, which is the gap that made the earlier branch unmergeable. **6. `make check` passes.** Green on the host. **7. `TODO.md`** updated in the same commit. ## Two things worth flagging **Guard decision: kept.** Reasoning in the PR. Short version: an unset `ARG` is the empty string and empty is a stable cache key, so without the guard the original defect stays reachable through a bare `docker build .`. Upstream measured a repo that had "landed the fix" and still produced the false green that way. The accepted consequence is that a bare `docker build .` now fails by design, exit 1, and keeps failing on repeat since failed steps are not cached. **A counterfactual was run, and it is the part I would look at first in review.** Two consecutive runs proving the checks ran does not by itself prove `CHECK_EPOCH` is why — the tree had just changed. Passing a *constant* `CHECK_EPOCH` twice reproduced the original false green on the fixed `Dockerfile`: every layer `CACHED`, exit 0, no check output. So the per-invocation value is the active ingredient, not the file edit. Note that the counterfactual named in the earlier plan comment — reverting `script/cibuild` to a bare `docker build .` — is no longer usable once the guard exists, because that path now fails rather than producing a green. The constant-epoch form replaces it and is the stronger test anyway. ## Ordering This lands before the `.dockerignore` work in https://git.eeqj.de/sneak/lora.vegas/issues/8, per the ordering comment above. The two-run proof here will need re-running after that change, since excluding `.claude/` removes a current source of build-context churn. Not something this PR can pre-empt. ## Upstream `sneak/prompts` #26 has not merged to that repo's `main` yet — the shape has settled and is implemented on its `next` branch, and that is what this adopts verbatim. Flagged in the PR body that it may need re-syncing once upstream merges.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bot
Agent's turn to work
 merge-ready
Review passed; ready to merge
 needs-checks
PR awaiting green checks
 needs-rebase
PR needs rebase onto main
 needs-review
PR awaiting adversarial review
 needs-rework
Review failed; PR needs rework
No labels
Milestone
No items
No Milestone
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/lora.vegas#23