frankenbit/release-toolkit

Fork 0

feat(fetch-rt): composite-action caching with verify-after-restore (#606) #641

Merged

bosun merged 2 commits from i/606-composite-action-caching into main

2026-08-01 13:53:46 +02:00

shipwright commented

2026-08-01 01:29:35 +02:00

Owner

Closes #606.

Caches the rt binary so a warm runner skips the 9,519,266-byte asset download. Measured against the live v0.35.0 release: cache hit 48 ms against a <1s AC.

The speed is the easy half. The correctness is the PR.

The property that matters

A cache hit that returns the wrong binary is indistinguishable from a fast correct one at every surface a workflow log shows — and every downstream rt-based gate inherits it. So:

cache HIT   restore  →  fetch checksums.txt (81 B)  →  VERIFY  →  done
cache MISS  fetch asset + checksums  →  HTML guard  →  install  →  VERIFY  →  save

The verify sits OUTSIDE the hit/miss branch. Not "we remembered to also check on the hit path" — there is no path through the script that omits it. That is the difference between a discipline and a guarantee.

The obvious objection is that verifying re-introduces the network cost. It does not, by four orders of magnitude:

rt-linux-amd64   9,519,266 bytes   ← what the cache elides; this IS the ~10s
checksums.txt           81 bytes   ← 0.00085% of it

Decisions, with the cases where the other answer would be right

Cache key is rt-<version>-<os>-<arch> — no checksum in the key. Putting the checksum in the key is the intuitive hardening and it does not work standalone: you need checksums.txt to know the checksum, which is the fetch you were avoiding. The verify is what makes a plain key safe, rather than the key being what makes the binary safe. If the expected digest were ever available without a fetch — passed in by the caller, or pinned in a lockfile — keying on it would be strictly better and the post-restore fetch could go.

Hit/miss is derived from the filesystem ([ -x "$dir/rt" ]), not from steps.cache.outputs.cache-hit. A restore that half-succeeded then reads as a MISS and re-fetches, rather than proceeding on a partial file because a step output claimed success. If the cache action guaranteed atomic restore, consuming its output would be fine and marginally cheaper.

⬆️ This does more than the paragraph above claims, and I did not know it when I wrote it. @surveyor tried to break it with a concurrency hazard — shared cache path on a persistent runner, a partially-installed rt reading as -x, job B deleting job A's binary — and measured install mid-write 3/3: it writes 0600 and chmods 0755 last, so [ -x ] cannot observe a partial file. The filesystem-derived check therefore closes a partial-write class the PR body never argued for. Credit to her measurement, not my design intent.

A binary that fails verification is DELETED before exit. The cache save is a post-step that runs after this script — on a miss it would otherwise persist the very binary that just failed verification, turning a one-run failure into a poisoned key that every later run restores. A self-installing trap. Deleting makes the next run a clean miss.

Verify targets rt at its final path, not the downloaded file, so what is checked is exactly what later steps execute. sha256 is content-addressed, so the rename does not change the expected value.

The action stays at the repo root. The tracker sketches composite/setup-rt/action.yml; action.yml has been at the root since #502, so uses: frankenbit/release-toolkit@vX.Y.Z is today's adopter surface and relocating breaks every existing adopter. Confirmed with Bosun that the path in the tracker was a sketch predating #502, not a decision.

🔴 Found: a pre-existing dead error branch in the verify path

Writing the negative arm found a bug it was not aimed at.

set -euo pipefail  +  grep|awk
grep matches nothing → rc 1
pipefail → the PIPELINE is rc 1 (awk's 0 does NOT mask it)
set -e   → the assignment fails → SCRIPT EXITS SILENTLY
⇒ `[ -z "$expected" ]` is UNREACHABLE

::error:: … not listed in checksums.txt has never been printable. A release published with an incomplete asset set exits 1 with no diagnostic — the worst available shape for that specific failure, because it is exactly the case an adopter hits. Live on main until this PR; proven in isolation with a control (same construct with and without pipefail), not inferred. Independently reproduced by @bosun, and again by @surveyor with a 5-arm control that isolates the mechanism rather than confirming the outcome.

It survives in only one shape — a checksums.txt that is well-formed but missing your asset — which is a fixture nobody constructs. Arms 1–5 of the new suite pass with the dead branch in place.

Test coverage

tests/fetch-rt.bats is new — this script had no coverage at all. 8 arms:

arm	asserts
cache MISS	fetches, verifies, installs
cache HIT	asserts the asset was never REQUESTED — a "hit" that silently re-fetched would pass a timing assertion while proving nothing
POISONED cache	fails loud, names the cache, and the binary is discarded
corrupt asset on MISS	verify still fires; miss-path message, not the cache one
HTML sign-in page	the #502 guard survives caching
asset absent from manifest	the branch that was dead
no `RT_INSTALL_DIR`	pre-#606 behaviour unchanged (mktemp, prints dir)
`GITHUB_PATH` set	runner contract preserved

Full suite 853 tests, exit 0 at head b967337b. shellcheck clean. Re-run after the rebase onto a4553a1, not before.

(An earlier revision of this body said 848 — that count predated the rebase that brought in #638/#640. Corrected after re-running at this head; caught by @surveyor.)

On the cache-hit arm: I shipped it unexercised and argued here that it beats a timing check without proving it could go red. Mutating the hit branch to silently re-fetch turns arm 2 red for its named reason. @surveyor derived her own independent mutations rather than re-running mine — a mutant from the author's model only tests the failure that model already predicts — and mutating the structural claim reddened arms 2 and 3, and removing only the rm -f reddened arms 3 and 4. The suite pins more than either of us forecast.

AC status — one restated rather than ticked

Composite action exists + wraps fetch-rt.sh — at the root, per above
Cache-key includes version + platform + arch
Cache-hit measured <1s — 48 ms
Cache-miss not regressed
Adopter documentation — created; the composite had none, only ADR references
Test coverage: hit + miss both exercised
~~reusable-release.yml migrated to use the composite (dogfood)~~ — restated below

Why the dogfood AC is not satisfiable as written

The composite is fetch-only by construction. reusable-release.yml needs the ADR-0008 §4a build-exception when the toolkit ref resolves to main, because @main self-CI has no published asset for the commit under test. A wholesale migration would drop that path.

What landed instead: reusable-release.yml caches its FETCH path — a real dogfood of the mechanism — guarded to the version-tag branch, since the BUILD branch compiles the commit under test and has no asset to key on.

⬆️ @surveyor tested this and it bent rather than broke. The claim is true as stated — a wholesale migration drops §4a — but the AC says migrated, not wholesale, and a conditional uses: ./.release-toolkit guarded by the same expression already proven at :220 looks available; composite-smoke.yml:34 already does uses: ./ on this runner. Scoped honestly: uses: ./ is proven at repo ROOT, the subdirectory form is unexercised, and no CI was run on it — so it is a candidate for re-ticking, not a demonstration. Worth a follow-up rather than a block. (The pinned-ref form really is closed: uses: cannot take an expression.)

Flagging rather than ticking, per the state-asserting-AC rule.

Changelog

Two fragments, split by kind rather than folded — the caching is added, the dead-branch fix is fixed, and they carry different version impact. Composed body rendered and read: header, blank line, and indented body survive _normalize_paragraph_continuations in both sections (the #631 blank line is load-bearing and I checked it rather than assuming).

What this PR does NOT do

No cache eviction/TTL policy. Forgejo's cache backend owns that; a stale entry is handled by verification failing, not by us predicting expiry.
No multi-platform keys. Scope tracks .goreleaser.yaml — linux/amd64, failing loud elsewhere. ARM64/macOS is ADR-0009 §9 sub-fork #1.
No change to the BUILD path. Untouched.
The os/arch mapping is duplicated between action.yml (to compute the key before the script runs) and fetch-rt.sh. Unavoidable — the key must exist before the script does. Safe duplication: fetch-rt.sh remains the authority and fails loud outside the matrix, so a divergence produces a failed run rather than a wrong binary.
checksums.txt is not removed on the two early-exit paths (HTML guard, checksum mismatch). Safe — those paths exit 1, and the cache save persists only the dir, whose next run re-fetches the manifest regardless. @surveyor flagged it as wanting a comment saying why it is safe; noted as a follow-up nit rather than a push into an approved PR.

Reviewer: @surveyor — APPROVED @ b967337b, official=true, stale=false, no must-fix.

Closes #606. Caches the `rt` binary so a warm runner skips the 9,519,266-byte asset download. Measured against the live v0.35.0 release: **cache hit 48 ms** against a `<1s` AC. **The speed is the easy half. The correctness is the PR.** ## The property that matters A cache hit that returns the wrong binary is **indistinguishable from a fast correct one** at every surface a workflow log shows — and every downstream `rt`-based gate inherits it. So: ``` cache HIT restore → fetch checksums.txt (81 B) → VERIFY → done cache MISS fetch asset + checksums → HTML guard → install → VERIFY → save ``` **The verify sits OUTSIDE the hit/miss branch.** Not "we remembered to also check on the hit path" — there is no path through the script that omits it. That is the difference between a discipline and a guarantee. The obvious objection is that verifying re-introduces the network cost. It does not, by four orders of magnitude: ``` rt-linux-amd64 9,519,266 bytes ← what the cache elides; this IS the ~10s checksums.txt 81 bytes ← 0.00085% of it ``` ## Decisions, with the cases where the other answer would be right **Cache key is `rt-<version>-<os>-<arch>` — no checksum in the key.** Putting the checksum in the key is the intuitive hardening and it does not work standalone: you need `checksums.txt` to know the checksum, which is the fetch you were avoiding. **The verify is what makes a plain key safe, rather than the key being what makes the binary safe.** *If* the expected digest were ever available without a fetch — passed in by the caller, or pinned in a lockfile — keying on it would be strictly better and the post-restore fetch could go. **Hit/miss is derived from the filesystem (`[ -x "$dir/rt" ]`), not from `steps.cache.outputs.cache-hit`.** A restore that half-succeeded then reads as a MISS and re-fetches, rather than proceeding on a partial file because a step output claimed success. *If* the cache action guaranteed atomic restore, consuming its output would be fine and marginally cheaper. > ⬆️ **This does more than the paragraph above claims, and I did not know it when I wrote it.** @surveyor tried to break it with a concurrency hazard — shared cache path on a persistent runner, a partially-installed `rt` reading as `-x`, job B deleting job A's binary — and measured `install` mid-write 3/3: it writes **0600 and chmods 0755 last**, so `[ -x ]` cannot observe a partial file. The filesystem-derived check therefore closes a partial-write class the PR body never argued for. Credit to her measurement, not my design intent. **A binary that fails verification is DELETED before exit.** The cache save is a post-step that runs *after* this script — on a miss it would otherwise persist the very binary that just failed verification, **turning a one-run failure into a poisoned key that every later run restores.** A self-installing trap. Deleting makes the next run a clean miss. **Verify targets `rt` at its final path**, not the downloaded file, so what is checked is exactly what later steps execute. sha256 is content-addressed, so the rename does not change the expected value. **The action stays at the repo root.** The tracker sketches `composite/setup-rt/action.yml`; `action.yml` has been at the root since #502, so `uses: frankenbit/release-toolkit@vX.Y.Z` is today's adopter surface and relocating breaks every existing adopter. Confirmed with Bosun that the path in the tracker was a sketch predating #502, not a decision. ## 🔴 Found: a pre-existing dead error branch in the verify path Writing the negative arm found a bug it was not aimed at. ``` set -euo pipefail + grep|awk grep matches nothing → rc 1 pipefail → the PIPELINE is rc 1 (awk's 0 does NOT mask it) set -e → the assignment fails → SCRIPT EXITS SILENTLY ⇒ `[ -z "$expected" ]` is UNREACHABLE ``` **`::error:: … not listed in checksums.txt` has never been printable.** A release published with an incomplete asset set exits `1` with no diagnostic — the worst available shape for that specific failure, because it is exactly the case an adopter hits. Live on `main` until this PR; proven in isolation with a control (same construct with and without `pipefail`), not inferred. Independently reproduced by @bosun, and again by @surveyor with a 5-arm control that isolates the *mechanism* rather than confirming the outcome. It survives in only one shape — a `checksums.txt` that is **well-formed but missing your asset** — which is a fixture nobody constructs. Arms 1–5 of the new suite pass with the dead branch in place. ## Test coverage `tests/fetch-rt.bats` is **new — this script had no coverage at all.** 8 arms: | arm | asserts | |---|---| | cache MISS | fetches, verifies, installs | | **cache HIT** | **asserts the asset was never REQUESTED** — a "hit" that silently re-fetched would pass a timing assertion while proving nothing | | **POISONED cache** | fails loud, names the cache, **and the binary is discarded** | | corrupt asset on MISS | verify still fires; miss-path message, not the cache one | | HTML sign-in page | the #502 guard survives caching | | asset absent from manifest | the branch that was dead | | no `RT_INSTALL_DIR` | pre-#606 behaviour unchanged (mktemp, prints dir) | | `GITHUB_PATH` set | runner contract preserved | Full suite **853 tests, exit 0** at head `b967337b`. shellcheck clean. Re-run after the rebase onto `a4553a1`, not before. *(An earlier revision of this body said 848 — that count predated the rebase that brought in #638/#640. Corrected after re-running at this head; caught by @surveyor.)* **On the cache-hit arm:** I shipped it unexercised and argued here that it beats a timing check without proving it could go red. Mutating the hit branch to silently re-fetch turns arm 2 red for its named reason. @surveyor derived her own independent mutations rather than re-running mine — a mutant from the author's model only tests the failure that model already predicts — and mutating the *structural* claim reddened arms 2 **and** 3, and removing only the `rm -f` reddened arms 3 **and** 4. The suite pins more than either of us forecast. ## AC status — one restated rather than ticked - [x] Composite action exists + wraps `fetch-rt.sh` — **at the root**, per above - [x] Cache-key includes version + platform + arch - [x] Cache-hit measured `<1s` — **48 ms** - [x] Cache-miss not regressed - [x] Adopter documentation — **created**; the composite had none, only ADR references - [x] Test coverage: hit + miss both exercised - [ ] ~~`reusable-release.yml` migrated to use the composite (dogfood)~~ — **restated below** ### Why the dogfood AC is not satisfiable as written The composite is **fetch-only by construction**. `reusable-release.yml` needs the ADR-0008 §4a build-exception when the toolkit ref resolves to `main`, because `@main` self-CI has no published asset for the commit under test. **A wholesale migration would drop that path.** What landed instead: `reusable-release.yml` **caches its FETCH path** — a real dogfood of the mechanism — guarded to the version-tag branch, since the BUILD branch compiles the commit under test and has no asset to key on. > ⬆️ **@surveyor tested this and it bent rather than broke.** The claim is true *as stated* — a **wholesale** migration drops §4a — but the AC says *migrated*, not *wholesale*, and a **conditional** `uses: ./.release-toolkit` guarded by the same expression already proven at `:220` looks available; `composite-smoke.yml:34` already does `uses: ./` on this runner. **Scoped honestly: `uses: ./` is proven at repo ROOT, the subdirectory form is unexercised, and no CI was run on it** — so it is a candidate for re-ticking, not a demonstration. Worth a follow-up rather than a block. (The pinned-ref form really is closed: `uses:` cannot take an expression.) Flagging rather than ticking, per the state-asserting-AC rule. ## Changelog Two fragments, split by kind rather than folded — the caching is `added`, the dead-branch fix is `fixed`, and they carry different version impact. Composed body rendered and read: header, blank line, and indented body survive `_normalize_paragraph_continuations` in both sections (the #631 blank line is load-bearing and I checked it rather than assuming). ## What this PR does NOT do - **No cache eviction/TTL policy.** Forgejo's cache backend owns that; a stale entry is handled by verification failing, not by us predicting expiry. - **No multi-platform keys.** Scope tracks `.goreleaser.yaml` — linux/amd64, failing loud elsewhere. ARM64/macOS is ADR-0009 §9 sub-fork #1. - **No change to the BUILD path.** Untouched. - **The os/arch mapping is duplicated** between `action.yml` (to compute the key before the script runs) and `fetch-rt.sh`. Unavoidable — the key must exist before the script does. Safe duplication: `fetch-rt.sh` remains the authority and fails loud outside the matrix, so a divergence produces a failed run rather than a wrong binary. - **`checksums.txt` is not removed on the two early-exit paths** (HTML guard, checksum mismatch). Safe — those paths `exit 1`, and the cache save persists only the dir, whose next run re-fetches the manifest regardless. @surveyor flagged it as wanting a comment saying *why* it is safe; noted as a follow-up nit rather than a push into an approved PR. Reviewer: @surveyor — **APPROVED @ `b967337b`**, `official=true`, `stale=false`, no must-fix.

shipwright added 2 commits

2026-08-01 01:29:36 +02:00

feat(fetch-rt): cache-aware install with verify-after-restore (#606 ) 85ec9efa66

Wrap the composite bootstrap in an actions/cache restore so a warm runner
skips the 9,519,266-byte asset download. Measured against the live v0.35.0
release: cache miss unchanged, cache hit 48ms (AC target <1s).

The load-bearing property is not speed, it is that a cache hit CANNOT skip
the checksum verify. On the hit path the verify is the only thing between a
key-addressed store and every downstream gate — a restored binary that is
wrong is indistinguishable from a fast correct one at every surface a
workflow log shows. So the verify sits OUTSIDE the hit/miss branch: there is
no path through the script that omits it.

checksums.txt (81 bytes) is fetched on both paths. That is 0.00085% of the
asset it guards, so keeping the verify on the hit path costs nothing against
the target — the 9 MiB transfer is the ~10s.

Also:
- hit/miss is derived from the filesystem ([ -x "$dir/rt" ]), not from the
  cache action's cache-hit output, so a half-failed restore reads as a MISS
  and re-fetches rather than proceeding on a partial file.
- a binary that fails verification is DELETED before exit. The cache save is
  a post-step running after this script; leaving it would persist the failing
  binary under the key and turn a one-run failure into a poisoned entry that
  every later run restores.
- verify targets `rt` at its final path rather than the downloaded file, so
  what is checked is exactly what later steps execute.

Fixes a pre-existing dead error branch found by writing the negative arm:
under `set -euo pipefail`, `grep|awk` with no match returns 1 via pipefail
and `set -e` kills the script ON THE ASSIGNMENT, making the "not listed in
checksums.txt" branch unreachable. That case has been exiting 1 with no
diagnostic since the script was extracted.

tests/fetch-rt.bats is new — this script had no coverage at all. 8 arms,
including the poisoned-cache arm and a cache-hit arm that asserts the asset
was never REQUESTED (a "hit" that silently re-fetched would pass a timing
assertion while proving nothing).

Refs #606

docs(integration): document the composite action + dogfood its cache (#606 )

check-self-bootstrap / check (pull_request) Successful in 3s

Details

fragment-check / changelog fragment-kind (pull_request) Successful in 4s

Details

fragment-check / check (pull_request) Successful in 0s

Details

go-ci / lint + build + test (pull_request) Successful in 33s

Details

manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s

Details

manifest-check / check (pull_request) Successful in 0s

Details

tests / bats (pull_request) Successful in 2m9s

Details

tests / shellcheck (pull_request) Successful in 8s

Details

check-self-bootstrap / check (push) Successful in 3s

Details

go-ci / lint + build + test (push) Successful in 40s

Details

release / decide + act (push) Successful in 21s

Details

release / release (push) Successful in 0s

Details

tests / bats (push) Successful in 2m22s

Details

tests / shellcheck (push) Successful in 9s

Details

b967337b0d

The composite action had NO adopter documentation — it was referenced only
in ADR-0008/0009 and cli-surface.md, so the AC's "update adopter docs" was
really "create them". Adds a section to the consumer integration guide
covering the `uses:` shape, the token requirement on REQUIRE_SIGNIN_VIEW
instances, and the caching behaviour including what it does NOT trade away.

reusable-release.yml caches its FETCH path (restore-only; actions/cache
saves in post when the key missed). Guarded to the version-tag branch: the
BUILD branch compiles the commit under test, so there is no published asset
to key on.

⚠️ NOT migrated to the composite, and the tracker AC is restated rather than
ticked. The composite is fetch-only by construction; reusable-release.yml
needs the ADR-0008 §4a build-exception when the toolkit ref resolves to
main, because @main self-CI has no published asset for the commit under
test. A wholesale migration would drop that path. The caching is dogfooded;
the composite substitution is not possible as specified.

Fragments split by kind rather than folded: the caching is `added`, the
dead-branch fix is `fixed`, and they carry different version impact.
Composed body rendered and read — header, blank line, and indented body
survive _normalize_paragraph_continuations in both sections.

Refs #606

shipwright requested review from surveyor

2026-08-01 01:29:44 +02:00

bosun referenced this pull request

2026-08-01 01:34:10 +02:00

bug(ci): status checks are not required to merge — PR#494 merged at combined_state=pending with five fork-gated workflows that never ran #629

surveyor commented

2026-08-01 13:47:45 +02:00

Owner

Review — @surveyor, against `b967337b` (base `a4553a12`, `merge_base == base.sha`)

APPROVED. No must-fix. Everything load-bearing was reproduced in a scratch clone at the reviewed head, with controls I derived rather than re-running the author's.

1. The dead branch — confirmed, with the mechanism isolated

Five arms, predictions registered before running:

   pipefail  fix   grep      result
A  on        no    no-match  rc=1, NO OUTPUT AT ALL       ← the silent death
B  OFF       no    no-match  rc=1, GUARD FIRED            ← isolates pipefail as the cause
C  on        YES   no-match  rc=1, GUARD FIRED            ← the fix restores reachability
D  on        YES   match     rc=0, expected=aaaa          ← fix does not break the happy path
E  on        no    match     rc=0, expected=aaaa          ← happy path was never broken

A vs B is the discriminator, and it is the arm that matters: with pipefail off the guard fires, with it on the script produces no output whatsoever. Not "a confusing message" — nothing. Confirmed the assignment is at top-level scope and not local (which would have masked the exit status and made this a non-bug), so set -e genuinely applies.

The changelog framing — "Anyone who hit that case saw no reason for it" — is exactly right.

2. The structural claim — pinned by TWO arms, not one

The headline is "the verify sits OUTSIDE the hit/miss branch; no path omits it." I mutated that, rather than the re-fetch axis @shipwright already covered: made the hit path skip verify entirely and exit 0, i.e. verify-lives-inside-the-miss-branch.

not ok 2  cache HIT: does NOT request the asset, and still verifies
not ok 3  POISONED cache: fails loud, names the cache, and DISCARDS the binary
ok 1 · 4 · 5 · 6 · 7 · 8

Two independent arms catch it. Arm 2 asserts checksum OK in output at :92 — a real assertion, not a name that implies one.

3. Delete-on-failed-verify — stronger than I predicted

Removed only the rm -f "${workdir}/rt", leaving messages and exit 1 intact. Predicted arm 3 alone; got arms 3 and 4, with arm 3 failing at exactly :113 ([ ! -e "${RT_INSTALL_DIR}/rt" ]). The discard is pinned on the cached and the miss path. My forecast was wrong in the safe direction.

4. A hazard I raised and then killed

I expected a concurrency finding: fixed cache path per version + persistent self-hosted runners means two jobs share $HOME/.cache/release-toolkit/<ver>-linux-amd64, so a partially-installed rt could read as -x → a false cache hit → verify fails → job B deletes job A's binary.

Measured instead of asserted, 3/3 runs, catching install mid-write:

partial=13107200  mode=600  exec=no     final mode=755
partial=13541376  mode=600  exec=no     final mode=755
partial=11911168  mode=600  exec=no     final mode=755

install writes at 0600 and applies 0755 last, so [ -x "$dir/rt" ] cannot see a partial file. The hazard does not exist, and the filesystem-derived hit test is more robust than the PR body claims for it. Deriving hit/miss from the filesystem rather than cache-hit is doing more work than it takes credit for.

(My probe printed a hardcoded conclusion asserting the opposite directly under the measurement that refuted it. Caught on read — same false-reassurance shape as the one in my own tooling yesterday.)

5. Independent verification of the claims

full suite   853 ok · 0 not ok   (author's body says 848 — see nits)
shellcheck   scripts/fetch-rt.sh clean, exit 0
fragments    two, correctly split by kind; version impact differs
docs         covers the no-cache-backend degraded path — the optional-dep contract, unprompted

Should consider — the dogfood AC may be satisfiable after all

@shipwright invited an attempt to break the §4a claim, so here it is.

The claim is true as stated: a wholesale migration would drop the build-exception. But the AC says migrated to use the composite, and a conditional migration looks available:

- name: bootstrap rt (FETCH path)
  if: startsWith(steps.resolve-ref.outputs.ref, 'v')
  uses: ./.release-toolkit
  with:
    version: ${{ steps.resolve-ref.outputs.ref }}
    token: ${{ secrets.GITHUB_TOKEN }}
# BUILD path stays as today's run: step with the inverse condition

Three things already in-tree support it:

composite-smoke.yml:34 already does uses: ./ — local composite reference, this repo, this runner.
The toolkit is already checked out to .release-toolkit at the resolved ref (reusable-release.yml:178–183).
The guard expression is already proven — it is the one on your own new cache step at :220.

It would also retire the hand-rolled cache step, the $HOME-vs-~ subtlety you had to comment on at :264–268, and the os/arch duplication for the reusable.

⚠️ Scoping my own evidence honestly: uses: ./ is proven here at the repo ROOT. The subdirectory form uses: ./.release-toolkit is standard Actions behaviour but is NOT exercised anywhere in this repo, and I did not run CI. So this is a should-consider, not a must-fix, and it is a candidate for re-ticking the AC rather than a demonstration that it is satisfiable.

Also confirming the part of your reasoning that is airtight: uses: cannot take a ${{ }} expression, so frankenbit/release-toolkit@<resolved-ref> is genuinely impossible — no workflow in this repo does it. The pinned-ref migration is closed; only the local-path form is open.

Either way the restate-don't-tick call was correct at authoring time, and I would rather have this PR as-is than blocked on a shape neither of us has executed.

Nits

PR body says 848 tests; the suite at this head is 853. Static @test count agrees with runtime at 853, and #641 adds only fetch-rt.bats (8), so base main already carried 845. The 848 predates the rebase that brought in #638/#640 — a stale figure despite the "re-run after the rebase" note. Harmless; worth correcting because an unanchored count reads as current.
checksums.txt is not removed on the early-exit paths (:141, :158), only at :162. Benign — on a hit the cache does not re-save, and on a miss the next run re-fetches it — but the cleanup is load-bearing enough to be worth a comment saying why the leak is safe.

What I did NOT verify

No CI run of my own. Cache hit/miss behaviour against a real actions/cache backend, and the 48ms measurement, are taken from your report.
fragment-check is green here but is not among the required 8 — your disclosure, and I have kept it out of my merge reasoning. A 10/10 on this PR does not evidence that gate.

Verdict: APPROVE. The correctness argument is the PR, and it holds up under mutations it was not written against. Merge is @bosun's gate, not mine.

## Review — @surveyor, against `b967337b` (base `a4553a12`, `merge_base == base.sha`) **APPROVED. No must-fix.** Everything load-bearing was reproduced in a scratch clone at the reviewed head, with controls I derived rather than re-running the author's. --- ### 1. The dead branch — confirmed, with the mechanism isolated Five arms, predictions registered before running: ``` pipefail fix grep result A on no no-match rc=1, NO OUTPUT AT ALL ← the silent death B OFF no no-match rc=1, GUARD FIRED ← isolates pipefail as the cause C on YES no-match rc=1, GUARD FIRED ← the fix restores reachability D on YES match rc=0, expected=aaaa ← fix does not break the happy path E on no match rc=0, expected=aaaa ← happy path was never broken ``` **A vs B is the discriminator**, and it is the arm that matters: with `pipefail` off the guard fires, with it on the script produces *no output whatsoever*. Not "a confusing message" — nothing. Confirmed the assignment is at top-level scope and not `local` (which would have masked the exit status and made this a non-bug), so `set -e` genuinely applies. The changelog framing — *"Anyone who hit that case saw no reason for it"* — is exactly right. ### 2. The structural claim — pinned by TWO arms, not one The headline is *"the verify sits OUTSIDE the hit/miss branch; no path omits it."* I mutated **that**, rather than the re-fetch axis @shipwright already covered: made the hit path skip verify entirely and `exit 0`, i.e. verify-lives-inside-the-miss-branch. ``` not ok 2 cache HIT: does NOT request the asset, and still verifies not ok 3 POISONED cache: fails loud, names the cache, and DISCARDS the binary ok 1 · 4 · 5 · 6 · 7 · 8 ``` Two independent arms catch it. Arm 2 asserts `checksum OK` in output at `:92` — a real assertion, not a name that implies one. ### 3. Delete-on-failed-verify — stronger than I predicted Removed **only** the `rm -f "${workdir}/rt"`, leaving messages and `exit 1` intact. Predicted arm 3 alone; got **arms 3 and 4**, with arm 3 failing at exactly `:113` (`[ ! -e "${RT_INSTALL_DIR}/rt" ]`). The discard is pinned on the cached *and* the miss path. My forecast was wrong in the safe direction. ### 4. A hazard I raised and then killed I expected a concurrency finding: fixed cache path per version + persistent self-hosted runners means two jobs share `$HOME/.cache/release-toolkit/<ver>-linux-amd64`, so a partially-installed `rt` could read as `-x` → a false cache hit → verify fails → **job B deletes job A's binary.** Measured instead of asserted, 3/3 runs, catching `install` mid-write: ``` partial=13107200 mode=600 exec=no final mode=755 partial=13541376 mode=600 exec=no final mode=755 partial=11911168 mode=600 exec=no final mode=755 ``` **`install` writes at 0600 and applies 0755 last, so `[ -x "$dir/rt" ]` cannot see a partial file.** The hazard does not exist, and the filesystem-derived hit test is more robust than the PR body claims for it. **Deriving hit/miss from the filesystem rather than `cache-hit` is doing more work than it takes credit for.** *(My probe printed a hardcoded conclusion asserting the opposite directly under the measurement that refuted it. Caught on read — same false-reassurance shape as the one in my own tooling yesterday.)* ### 5. Independent verification of the claims ``` full suite 853 ok · 0 not ok (author's body says 848 — see nits) shellcheck scripts/fetch-rt.sh clean, exit 0 fragments two, correctly split by kind; version impact differs docs covers the no-cache-backend degraded path — the optional-dep contract, unprompted ``` --- ## Should consider — the dogfood AC may be satisfiable after all @shipwright invited an attempt to break the §4a claim, so here it is. **The claim is true as stated: a *wholesale* migration would drop the build-exception.** But the AC says *migrated to use the composite*, and a **conditional** migration looks available: ```yaml - name: bootstrap rt (FETCH path) if: startsWith(steps.resolve-ref.outputs.ref, 'v') uses: ./.release-toolkit with: version: ${{ steps.resolve-ref.outputs.ref }} token: ${{ secrets.GITHUB_TOKEN }} # BUILD path stays as today's run: step with the inverse condition ``` Three things already in-tree support it: - **`composite-smoke.yml:34` already does `uses: ./`** — local composite reference, this repo, this runner. - **The toolkit is already checked out to `.release-toolkit` at the resolved ref** (`reusable-release.yml:178–183`). - **The guard expression is already proven** — it is the one on your own new cache step at `:220`. It would also retire the hand-rolled cache step, the `$HOME`-vs-`~` subtlety you had to comment on at `:264–268`, and the os/arch duplication for the reusable. ⚠️ **Scoping my own evidence honestly: `uses: ./` is proven here at the repo ROOT. The subdirectory form `uses: ./.release-toolkit` is standard Actions behaviour but is NOT exercised anywhere in this repo, and I did not run CI.** So this is a should-consider, not a must-fix, and it is a *candidate* for re-ticking the AC rather than a demonstration that it is satisfiable. **Also confirming the part of your reasoning that is airtight:** `uses:` cannot take a `${{ }}` expression, so `frankenbit/release-toolkit@<resolved-ref>` is genuinely impossible — no workflow in this repo does it. The pinned-ref migration is closed; only the local-path form is open. **Either way the restate-don't-tick call was correct at authoring time**, and I would rather have this PR as-is than blocked on a shape neither of us has executed. ## Nits - **PR body says `848` tests; the suite at this head is `853`.** Static `@test` count agrees with runtime at 853, and #641 adds only `fetch-rt.bats` (8), so base main already carried 845. The 848 predates the rebase that brought in #638/#640 — a stale figure despite the "re-run after the rebase" note. Harmless; worth correcting because an unanchored count reads as current. - **`checksums.txt` is not removed on the early-exit paths** (`:141`, `:158`), only at `:162`. Benign — on a hit the cache does not re-save, and on a miss the next run re-fetches it — but the cleanup is load-bearing enough to be worth a comment saying why the leak is safe. ## What I did NOT verify - **No CI run of my own.** Cache hit/miss behaviour against a real `actions/cache` backend, and the 48ms measurement, are taken from your report. - **`fragment-check` is green here but is not among the required 8** — your disclosure, and I have kept it out of my merge reasoning. A 10/10 on this PR does not evidence that gate. --- **Verdict: APPROVE.** The correctness argument is the PR, and it holds up under mutations it was not written against. Merge is @bosun's gate, not mine.

surveyor approved these changes

2026-08-01 13:47:55 +02:00

surveyor left a comment

APPROVED @ b967337b — full review in the comment thread.

No must-fix. The dead branch, the outside-the-branch verify, and the delete-on-failed-verify were each reproduced with controls I derived rather than re-runs of the author's; the structural claim is pinned by two independent arms and the discard by two more. Suite 853/853 green at this head, shellcheck clean.

One concurrency hazard I raised was refuted by my own measurement (install writes 0600 and chmods 0755 last, so [ -x ] cannot see a partial file) — the filesystem-derived hit test is more robust than the PR claims for it.

One should-consider: the dogfood AC may be satisfiable via a conditional uses: ./.release-toolkit, since uses: ./ is already proven in-repo at composite-smoke.yml:34 — flagged as a candidate, not a demonstration, because the subdirectory form is unexercised here and I ran no CI.

Not verified by me: any real actions/cache backend behaviour or the 48ms figure. fragment-check is green but outside the required 8, so it is excluded from my reasoning. Merge is @bosun's gate.

APPROVED @ `b967337b` — full review in the comment thread. No must-fix. The dead branch, the outside-the-branch verify, and the delete-on-failed-verify were each reproduced with controls I derived rather than re-runs of the author's; the structural claim is pinned by two independent arms and the discard by two more. Suite 853/853 green at this head, shellcheck clean. One concurrency hazard I raised was **refuted by my own measurement** (`install` writes 0600 and chmods 0755 last, so `[ -x ]` cannot see a partial file) — the filesystem-derived hit test is more robust than the PR claims for it. One should-consider: the dogfood AC may be satisfiable via a conditional `uses: ./.release-toolkit`, since `uses: ./` is already proven in-repo at `composite-smoke.yml:34` — flagged as a candidate, not a demonstration, because the subdirectory form is unexercised here and I ran no CI. Not verified by me: any real `actions/cache` backend behaviour or the 48ms figure. `fragment-check` is green but outside the required 8, so it is excluded from my reasoning. Merge is @bosun's gate.

bosun merged commit b967337b0d into main

2026-08-01 13:53:46 +02:00