sneak/vaultik
1
0
Fork 0
Code Issues 19 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Adopt the remaining upstream CHECK_EPOCH hardening (expanded form, unset guard, unique epoch, script/docker) #91

New Issue
Closed
opened 2026-08-09 08:45:51 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 08:45:51 +02:00
Collaborator
Copy Link

Follow-up to #85 / PR #89. That PR closes the false-green hole in
script/cibuild and its fix is verified working on this host. Upstream
sneak/prompts #26 has since accumulated four further hardening items
that PR #89 deliberately did not adopt, because its rework was
explicitly scoped to script/cibuild and to prose, with the
Dockerfile ARG CHECK_EPOCH placement frozen after independent
measurement confirmed it correct.

None of these is a defect in the landed fix. Each is a case where the
guard, if it ever breaks, breaks green — so they are worth closing
deliberately rather than leaving implicit.

1. Expanded form, so the cache miss is contractual

sneak/prompts #26 comment 48122 settled on:

ARG CHECK_EPOCH
RUN echo "check epoch: ${CHECK_EPOCH}" && make lint

The bare, unreferenced ARG this repo ships is not broken: four
independent upstream measurements plus this repo's own back-to-back
runs and withheld---build-arg counterfactual all confirm a
declared-but-unreferenced ARG enters BuildKit's cache key. Upstream
says bare-form repos "need no urgent rework". The argument for
expanding is that it is correct under either reading of BuildKit's
behavior, so it survives that behavior changing, and the epoch appears
in the build log where a reader can see the layer was keyed fresh.

2. Fail closed when --build-arg is withheld

sneak/prompts #26 comment 48284. An unset ARG is an empty string,
which is a stable cache key, so a bare docker build . — the command
REPO_POLICIES.md names verbatim — still produces the original false
green on the second consecutive run. Measured on PR #89's own branch:
274ms, 0 ok lines, all three check layers CACHED, exit 0.

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

Failed steps are never cached, so this fails on every invocation rather
than once, turning a quiet lie into a loud error. Documentation-only
mitigation is known insufficient: upstream shipped exactly that and
someone still ran the documented command and got the false green.

3. Make the epoch unique per invocation, not per second

sneak/prompts #26 comments 48237 and 48458. 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. Not
currently reachable here — the warm floor is well over two minutes — but
concurrency is the norm on this host. date +%s%N is not sufficient
alone: busybox silently drops %N, exits 0, and gives back second
granularity with no warning. The portable form is:

epoch="$(date +%s%N)$$"

$$ differs between concurrent invocations regardless of whether %N
is honored.

4. script/docker has the identical hole

sneak/prompts #26 comments 47571 and 48076. script/docker is a bare
docker build -t ... . with no cache control. It is not the gate, but
once script/cibuild is fixed the two entrypoints silently disagree
about whether the tree is green, and local builds are almost always
warm.

Definition of done

  • Each item above is either adopted or explicitly rejected in writing,
    with the reason recorded in the repo rather than only in review.
  • Verification is by negative control, not inspection: after the
    change, a bare docker build . run twice back to back must fail
    loudly on both runs rather than reporting a cached green on the
    second. The two-run requirement is not optional — an empty
    CHECK_EPOCH is itself a novel cache key, so the first run executes
    and a single run would wrongly suggest there is no defect.
  • Dependency layers (apk add, COPY go.mod go.sum, go mod download) must still report CACHED on an untouched-tree run. If
    the build lands near cold-build wall time, the ARG was moved too
    high and the change should be rejected.
  • .golangci.yml must stay at sha256
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
    and the lint-stage FROM line and its digest are not to be touched
    (#78).
Follow-up to #85 / PR #89. That PR closes the false-green hole in `script/cibuild` and its fix is verified working on this host. Upstream `sneak/prompts` #26 has since accumulated four further hardening items that PR #89 deliberately did **not** adopt, because its rework was explicitly scoped to `script/cibuild` and to prose, with the `Dockerfile` `ARG CHECK_EPOCH` placement frozen after independent measurement confirmed it correct. None of these is a defect in the landed fix. Each is a case where the guard, if it ever breaks, breaks *green* — so they are worth closing deliberately rather than leaving implicit. ## 1. Expanded form, so the cache miss is contractual `sneak/prompts` #26 comment 48122 settled on: ```dockerfile ARG CHECK_EPOCH RUN echo "check epoch: ${CHECK_EPOCH}" && make lint ``` The bare, unreferenced `ARG` this repo ships is **not** broken: four independent upstream measurements plus this repo's own back-to-back runs and withheld-`--build-arg` counterfactual all confirm a declared-but-unreferenced `ARG` enters BuildKit's cache key. Upstream says bare-form repos "need no urgent rework". The argument for expanding is that it is correct under either reading of BuildKit's behavior, so it survives that behavior changing, and the epoch appears in the build log where a reader can see the layer was keyed fresh. ## 2. Fail closed when `--build-arg` is withheld `sneak/prompts` #26 comment 48284. An unset `ARG` is an empty string, which is a stable cache key, so a bare `docker build .` — the command `REPO_POLICIES.md` names verbatim — still produces the original false green on the second consecutive run. Measured on PR #89's own branch: 274ms, 0 `ok` lines, all three check layers `CACHED`, exit 0. ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 ``` Failed steps are never cached, so this fails on every invocation rather than once, turning a quiet lie into a loud error. Documentation-only mitigation is known insufficient: upstream shipped exactly that and someone still ran the documented command and got the false green. ## 3. Make the epoch unique per invocation, not per second `sneak/prompts` #26 comments 48237 and 48458. `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. Not currently reachable here — the warm floor is well over two minutes — but concurrency is the norm on this host. `date +%s%N` is not sufficient alone: busybox silently drops `%N`, exits 0, and gives back second granularity with no warning. The portable form is: ```sh epoch="$(date +%s%N)$$" ``` `$$` differs between concurrent invocations regardless of whether `%N` is honored. ## 4. `script/docker` has the identical hole `sneak/prompts` #26 comments 47571 and 48076. `script/docker` is a bare `docker build -t ... .` with no cache control. It is not the gate, but once `script/cibuild` is fixed the two entrypoints silently disagree about whether the tree is green, and local builds are almost always warm. ## Definition of done - Each item above is either adopted or explicitly rejected in writing, with the reason recorded in the repo rather than only in review. - Verification is by negative control, not inspection: after the change, a bare `docker build .` run **twice** back to back must fail loudly on both runs rather than reporting a cached green on the second. The two-run requirement is not optional — an empty `CHECK_EPOCH` is itself a novel cache key, so the first run executes and a single run would wrongly suggest there is no defect. - Dependency layers (`apk add`, `COPY go.mod go.sum`, `go mod download`) must still report `CACHED` on an untouched-tree run. If the build lands near cold-build wall time, the `ARG` was moved too high and the change should be rejected. - `.golangci.yml` must stay at sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, and the lint-stage `FROM` line and its digest are not to be touched (#78).
clawbot added this to the 1.0.0 milestone 2026-08-09 09:03:00 +02:00
clawbot commented 2026-08-09 09:38:27 +02:00
Author
Collaborator
Copy Link

Manager note — dispatching this now that #85 has merged (main is past
PR #89). Decisions on the four items, so the implementer is not
re-litigating them.

Adopt all four. Ordering by value:

Item 2 (fail-closed guard) is the reason this issue is not optional.
A bare docker build . still replays the check layers from the second
consecutive run onward — independently reproduced twice on PR #89's
branch at 400ms and 356ms, zero ok lines, all three check layers
CACHED, exit 0. That matters more than a normal residual gap because
REPO_POLICIES.md names docker build . verbatim as a thing that
must be green. So the documented command is precisely the one that lies.
Documentation-only mitigation is already known insufficient: upstream
shipped exactly that and someone still ran the documented command and got
the false green.

Note the mechanism, because it is unintuitive: an unset ARG is an empty
string, and an empty string is a perfectly stable cache key. Failed steps
are never cached, so RUN [ -n "$CHECK_EPOCH" ] || exit 1 fails on
every invocation rather than once — which is what converts a quiet lie
into a loud error.

Item 1 (expanded form) — adopt. The bare unreferenced ARG is not
broken; it was verified working here across five consecutive untouched-tree
runs. But it works because of BuildKit's current cache-key behavior, which
is an implementation detail we happen to depend on. Referencing the value
in the RUN makes the cache miss contractual rather than incidental, and
puts the epoch in the build log where a reader can see the layer was keyed
fresh.

Item 3 (unique epoch) — adopt epoch="$(date +%s%N)$$". Not currently
reachable (warm floor is well over two minutes), but concurrency is normal
on this host and second-granularity is a latent collision. Heed the
busybox caveat: %N is silently dropped, exits 0, and yields second
granularity with no warning — $$ is what makes it correct regardless.

Item 4 (script/docker) — adopt. It is not the gate, but once
script/cibuild is honest and script/docker is not, two entrypoints
disagree about whether the tree is green, and local builds are almost
always warm.

Environmental warning for whoever picks this up

The shared BuildKit cache on this host was destroyed earlier
(docker builder prune -af from another session, ~41 GB). Consequences:

  • CACHED: 0 is temporarily uninformative as evidence of a forced
    run — everything is cold, so that condition is trivially satisfied. The
    ok line count still carries the signal, since a cached build yields
    zero ok lines regardless of why.
  • Cold-cache timeout flakes will look like real failures. script/test
    uses -race -timeout 30s per package (see #69). Retry before treating a
    timeout as a defect.
  • Never run docker builder prune. Scope invalidation to your own
    build: docker build --no-cache, or --no-cache-filter=<stage>. It is
    shared mutable state on a shared host and the blast radius is every
    other session. This is worth stating explicitly because "prove nothing
    was cached" makes pruning look like the most rigorous move available; it
    is the most destructive one.
Manager note — dispatching this now that #85 has merged (`main` is past PR #89). Decisions on the four items, so the implementer is not re-litigating them. **Adopt all four.** Ordering by value: **Item 2 (fail-closed guard) is the reason this issue is not optional.** A bare `docker build .` still replays the check layers from the second consecutive run onward — independently reproduced twice on PR #89's branch at 400ms and 356ms, zero `ok` lines, all three check layers `CACHED`, exit 0. That matters more than a normal residual gap because `REPO_POLICIES.md` names `docker build .` **verbatim** as a thing that must be green. So the documented command is precisely the one that lies. Documentation-only mitigation is already known insufficient: upstream shipped exactly that and someone still ran the documented command and got the false green. Note the mechanism, because it is unintuitive: an unset `ARG` is an empty string, and an empty string is a perfectly stable cache key. Failed steps are never cached, so `RUN [ -n "$CHECK_EPOCH" ] || exit 1` fails on *every* invocation rather than once — which is what converts a quiet lie into a loud error. **Item 1 (expanded form)** — adopt. The bare unreferenced `ARG` is not broken; it was verified working here across five consecutive untouched-tree runs. But it works because of BuildKit's current cache-key behavior, which is an implementation detail we happen to depend on. Referencing the value in the `RUN` makes the cache miss contractual rather than incidental, and puts the epoch in the build log where a reader can see the layer was keyed fresh. **Item 3 (unique epoch)** — adopt `epoch="$(date +%s%N)$$"`. Not currently reachable (warm floor is well over two minutes), but concurrency is normal on this host and second-granularity is a latent collision. Heed the busybox caveat: `%N` is silently dropped, exits 0, and yields second granularity with no warning — `$$` is what makes it correct regardless. **Item 4 (`script/docker`)** — adopt. It is not the gate, but once `script/cibuild` is honest and `script/docker` is not, two entrypoints disagree about whether the tree is green, and local builds are almost always warm. ## Environmental warning for whoever picks this up The shared BuildKit cache on this host was destroyed earlier (`docker builder prune -af` from another session, ~41 GB). Consequences: - **`CACHED: 0` is temporarily uninformative** as evidence of a forced run — everything is cold, so that condition is trivially satisfied. The `ok` line count still carries the signal, since a cached build yields zero `ok` lines regardless of why. - **Cold-cache timeout flakes will look like real failures.** `script/test` uses `-race -timeout 30s` per package (see #69). Retry before treating a timeout as a defect. - **Never run `docker builder prune`.** Scope invalidation to your own build: `docker build --no-cache`, or `--no-cache-filter=<stage>`. It is shared mutable state on a shared host and the blast radius is every other session. This is worth stating explicitly because "prove nothing was cached" makes pruning look like the most rigorous move available; it is the most destructive one.
clawbot commented 2026-08-09 09:40:05 +02:00
Author
Collaborator
Copy Link

Implementation plan, branch fix-check-epoch-hardening off main at
c3bb3b5. All four items adopted as decided in the manager comment;
nothing here re-opens them.

Dockerfile — items 1 and 2, both check stages

ARG is per-stage and this repo has two stages carrying gate steps
(make fmt-check + make lint in lint, make test in builder),
so each gets the full treatment, with the ARG staying exactly where
it is today — below apk add, COPY go.mod go.sum and go mod download, so dependency layers keep caching.

Lint stage:

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

Builder stage:

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

The guard is its own RUN rather than folded into each check line so
that the missing-arg case fails on the cheapest possible step, before
any check starts, and reads as one thing rather than three.

The existing block comment is rewritten: it currently documents the
unset-ARG hole as a live condition to be worked around and points at
this issue as the tracker. That text becomes false with this change, so
it is replaced with the mechanism (unset ARG is an empty string, an
empty string is a stable cache key, failed steps are never cached, so
the guard fires on every invocation rather than once) and the one-line
upstream justification for the expanded form: expand the value into the
command so the cache miss does not depend on BuildKit's
unreferenced-ARG handling. The lint-stage FROM line and its digest
are untouched (#78), as is .golangci.yml.

script/cibuild — item 3

epoch="$(date +%s)" becomes epoch="$(date +%s%N)$$". Bare
assignment form is kept deliberately — inlining the substitution into
the --build-arg would not abort under set -eu and would silently
yield an empty constant epoch, which is the exact false green this
exists to prevent. The comment gains the busybox caveat: %N is
silently dropped with exit 0, giving second granularity with no
warning, and $$ is what makes the per-invocation guarantee hold
regardless.

script/docker — item 4

Gets the identical epoch assignment and --build-arg CHECK_EPOCH, so
the two entrypoints cannot disagree about whether the tree is green.
With the guard in place a bare docker build now fails, so passing the
arg here is required, not optional.

Docs

README.md's script/cibuild entry currently describes the bare
docker build . false green as reachable and names this issue as the
tracker; that paragraph is rewritten to say the case now fails closed.
TODO.md is updated in the same commit per its Workflow section.

REPO_POLICIES.md is org-canonical and stays untouched. Its line 171
still asserts "a successful build implies all checks pass" — with this
change that is true for script/cibuild, script/docker and a bare
docker build . alike (the last by failing rather than by passing),
but the sentence is only accurate by accident of this repo having
adopted the hardening. It will be noted in the PR, not edited.

Verification — negative controls, not inspection

Recorded once, in the PR:

  1. Bare docker build ., no --build-arg, run twice back to back.
    Both must fail non-zero. One run is not sufficient: an empty
    CHECK_EPOCH is itself a novel cache key, so pre-fix the first run
    executes and only the second fakes.
  2. script/cibuild back to back on an unchanged tree: second run must
    still execute all three check layers, with real wall time and real
    ok lines carrying differing durations. Exit codes captured
    immediately.
  3. Dependency layers (apk add, COPY go.mod go.sum, go mod download) must still report CACHED from the second run onward.
    This doubles as the validity control for the pair: a cache wipe
    landing mid-pair would make them re-execute and the claim would fail
    loudly instead of passing silently.
  4. Same two-run treatment for script/docker.
  5. Host-side GOFLAGS=-count=1 make check as a cross-check.

BUILDKIT_PROGRESS=plain throughout. The shared BuildKit cache on this
host was destroyed earlier by another session, so CACHED: 0 is
uninformative as evidence right now and ok line count is the primary
signal; dependency caching is judged from the second run onward. No
docker builder prune of any kind will be run — invalidation is scoped
with --no-cache-filter if it is needed at all.

Implementation plan, branch `fix-check-epoch-hardening` off `main` at `c3bb3b5`. All four items adopted as decided in the manager comment; nothing here re-opens them. ## Dockerfile — items 1 and 2, both check stages `ARG` is per-stage and this repo has two stages carrying gate steps (`make fmt-check` + `make lint` in `lint`, `make test` in `builder`), so each gets the full treatment, with the `ARG` staying exactly where it is today — below `apk add`, `COPY go.mod go.sum` and `go mod download`, so dependency layers keep caching. Lint stage: ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" &amp;&amp; make fmt-check RUN echo "check epoch: ${CHECK_EPOCH}" &amp;&amp; make lint ``` Builder stage: ```dockerfile ARG CHECK_EPOCH RUN [ -n "$CHECK_EPOCH" ] || exit 1 RUN echo "check epoch: ${CHECK_EPOCH}" &amp;&amp; make test ``` The guard is its own `RUN` rather than folded into each check line so that the missing-arg case fails on the cheapest possible step, before any check starts, and reads as one thing rather than three. The existing block comment is rewritten: it currently documents the unset-`ARG` hole as a live condition to be worked around and points at this issue as the tracker. That text becomes false with this change, so it is replaced with the mechanism (unset `ARG` is an empty string, an empty string is a stable cache key, failed steps are never cached, so the guard fires on every invocation rather than once) and the one-line upstream justification for the expanded form: expand the value into the command so the cache miss does not depend on BuildKit's unreferenced-`ARG` handling. The lint-stage `FROM` line and its digest are untouched (#78), as is `.golangci.yml`. ## script/cibuild — item 3 `epoch="$(date +%s)"` becomes `epoch="$(date +%s%N)$$"`. Bare assignment form is kept deliberately — inlining the substitution into the `--build-arg` would not abort under `set -eu` and would silently yield an empty constant epoch, which is the exact false green this exists to prevent. The comment gains the busybox caveat: `%N` is silently dropped with exit 0, giving second granularity with no warning, and `$$` is what makes the per-invocation guarantee hold regardless. ## script/docker — item 4 Gets the identical epoch assignment and `--build-arg CHECK_EPOCH`, so the two entrypoints cannot disagree about whether the tree is green. With the guard in place a bare `docker build` now fails, so passing the arg here is required, not optional. ## Docs `README.md`'s `script/cibuild` entry currently describes the bare `docker build .` false green as reachable and names this issue as the tracker; that paragraph is rewritten to say the case now fails closed. `TODO.md` is updated in the same commit per its Workflow section. `REPO_POLICIES.md` is org-canonical and stays untouched. Its line 171 still asserts "a successful build implies all checks pass" — with this change that is true for `script/cibuild`, `script/docker` and a bare `docker build .` alike (the last by failing rather than by passing), but the sentence is only accurate by accident of this repo having adopted the hardening. It will be noted in the PR, not edited. ## Verification — negative controls, not inspection Recorded once, in the PR: 1. Bare `docker build .`, no `--build-arg`, run **twice** back to back. Both must fail non-zero. One run is not sufficient: an empty `CHECK_EPOCH` is itself a novel cache key, so pre-fix the first run executes and only the second fakes. 2. `script/cibuild` back to back on an unchanged tree: second run must still execute all three check layers, with real wall time and real `ok` lines carrying differing durations. Exit codes captured immediately. 3. Dependency layers (`apk add`, `COPY go.mod go.sum`, `go mod download`) must still report `CACHED` from the second run onward. This doubles as the validity control for the pair: a cache wipe landing mid-pair would make them re-execute and the claim would fail loudly instead of passing silently. 4. Same two-run treatment for `script/docker`. 5. Host-side `GOFLAGS=-count=1 make check` as a cross-check. `BUILDKIT_PROGRESS=plain` throughout. The shared BuildKit cache on this host was destroyed earlier by another session, so `CACHED: 0` is uninformative as evidence right now and `ok` line count is the primary signal; dependency caching is judged from the second run onward. No `docker builder prune` of any kind will be run — invalidation is scoped with `--no-cache-filter` if it is needed at all.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#91
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?