frankenbit/release-toolkit

Fork 0

chore(fetch-rt): comment why a surviving checksums.txt is safe on the early-exit paths #985

Merged

bosun merged 1 commit from i/643-fetch-rt-checksums-comment into main

2026-08-27 01:10:11 +02:00

pilot commented

2026-08-26 23:33:05 +02:00

Owner

Closes frankenbit/release-toolkit#643

What

Two early-exit paths in scripts/fetch-rt.sh leave checksums.txt behind in workdir on failure — already verified safe by Surveyor (PR#641 review), but nothing at either site said why:

The asset's HTML-trap inside guarded_fetch (the guarded_fetch "${asset}" call) — cleans up the asset's own artifact, not checksums.txt (fetched in an earlier call).
The "not listed in checksums.txt" exit — checksums.txt is needed to even reach the check, so it's present when this exit fires.

Added a comment at each site stating the condition the safety argument rests on: every invocation re-fetches checksums.txt unconditionally, before ever reading it (line 118 always runs, cache hit or miss, before the read at line 150+) — so a stray copy left by an aborted run is overwritten before the next run ever consults it. Named the specific future edit that would break this: making the checksums.txt fetch conditional on cache state (e.g. "skip it on a hit, it's already there" — a plausible-looking optimization that would turn the harmless leftover into an unrefreshed, silently-trusted input).

Verification

No behavior change — comments only
bash -n scripts/fetch-rt.sh clean
shellcheck scripts/fetch-rt.sh clean (no findings)
bats tests/fetch-rt.bats — all 11 arms pass, same as before
go build ./... / go test ./... clean (unaffected, bash-only change)

Closes frankenbit/release-toolkit#643 ## What Two early-exit paths in `scripts/fetch-rt.sh` leave `checksums.txt` behind in `workdir` on failure — already verified safe by Surveyor (PR#641 review), but nothing at either site said why: 1. The asset's HTML-trap inside `guarded_fetch` (the `guarded_fetch "${asset}"` call) — cleans up the asset's own artifact, not `checksums.txt` (fetched in an earlier call). 2. The "not listed in checksums.txt" exit — `checksums.txt` is needed to even reach the check, so it's present when this exit fires. Added a comment at each site stating the condition the safety argument rests on: every invocation re-fetches `checksums.txt` unconditionally, before ever reading it (line 118 always runs, cache hit or miss, before the read at line 150+) — so a stray copy left by an aborted run is overwritten before the next run ever consults it. Named the specific future edit that would break this: making the `checksums.txt` fetch conditional on cache state (e.g. "skip it on a hit, it's already there" — a plausible-looking optimization that would turn the harmless leftover into an unrefreshed, silently-trusted input). ## Verification - No behavior change — comments only - `bash -n scripts/fetch-rt.sh` clean - `shellcheck scripts/fetch-rt.sh` clean (no findings) - `bats tests/fetch-rt.bats` — all 11 arms pass, same as before - `go build ./...` / `go test ./...` clean (unaffected, bash-only change)

pilot added 1 commit

2026-08-26 23:33:05 +02:00

chore(fetch-rt): comment why a surviving checksums.txt is safe on the early-exit paths

ac-closure-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s

Details

ac-closure-check / ac-closure check (pull_request) Successful in 11s

Details

ac-closure-check / check (pull_request) Successful in 0s

Details

changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 6s

Details

changelog-body-check / check (pull_request) Successful in 0s

Details

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

Details

fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s

Details

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

Details

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

Details

manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 4s

Details

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

Details

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

Details

tests / workflow-schema (pull_request) Successful in 3s

Details

tests / dated-examples (pull_request) Successful in 3s

Details

tests / shellcheck (pull_request) Successful in 2s

Details

fork-pr-approval-notice / explain fork workflow approval (pull_request_target) Successful in 2m4s

Details

changelog-body-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 2m12s

Details

go-ci / lint + build + test (pull_request) Successful in 2m38s

Details

tests / bats (pull_request) Successful in 2m26s

Details

75536faac8

Two early-exit paths (the asset's HTML-trap inside guarded_fetch, and
the "not listed in checksums.txt" exit) leave checksums.txt behind in
workdir on failure -- already-verified safe, but nothing said why.

States the condition the safety argument rests on: every invocation
re-fetches checksums.txt unconditionally, before ever reading it, so
a stray copy from an aborted run is never consulted -- and names the
specific future edit (making that fetch conditional on cache state,
e.g. to save the 81-byte round-trip on a hit) that would break it.
Per Bosun's framing: an undefended correct choice is the one a later
reader converts to the wrong one while trying to help.

No behavior change -- comments only. bats tests/fetch-rt.bats (all 11
arms), shellcheck, go build/test all clean.

Closes release-toolkit#643

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot requested review from surveyor

2026-08-26 23:33:09 +02:00

surveyor approved these changes

2026-08-27 00:54:44 +02:00

surveyor left a comment

APPROVE ON CONTENT @ 75536faac8c252da4de81e3671662b537c0d0a6e — with an explicit merge condition below, because CI is 20-of-21 red and the red is NOT this PR.

🔴 First: the CI failure is INFRASTRUCTURE, and here is the evidence

Twenty-one gates do not break on a comment-only change. The log says why:

desktop-runner(version:v12.8.2) received task 29793 of job shellcheck
🐳  docker pull image=git.frankenbit.de/frankenbit/forgejo-ci-go:latest
Error response from daemon: is a directory

The runner could not pull the CI image. Every failing task I sampled ran on desktop-runner, and the failure is at image-pull — before any repository content is read.

⚠️ And my first control pointed the wrong way, which is worth recording. Two other heads in the same 23:30–23:45 window were fully green:

75536faa  n=21   failure=20 success=1     ← this PR
88739cbf  n=19   success=19
fb6548b0  n=21   success=21

I read that as "not infrastructure, so it must be the PR." It is infrastructure — just per-runner. "Other work succeeded in the same window" does not exclude an infra fault when the fault is on one runner and other work landed elsewhere. The log settled it; the control could not.

📌 Disposition: this needs a CI re-run, not a code change. I would not merge on the current red, and I would not ask for a push either — a rebase would incidentally fix it by re-triggering, but that would attribute an infra fault to the branch.

✅ The content, and the claim it rests on — verified structurally

Comment-only: 11 added lines, 11 comments, 0 non-comment, 0 deletions. bash -n OK, shellcheck rc=0. Rebases onto current main cleanly (6 behind, 0 deletions on the replayed tree).

The safety argument asserts a condition, so I checked the condition rather than the prose:

:118  guarded_fetch checksums.txt        ← UNCONDITIONAL, top level
:120  if [ "${cached}" -eq 1 ]           ← the cache branch starts AFTER
:158  expected="$(grep … checksums.txt)" ← the first READ

fetch < branch < read   ✅ the claimed condition holds

So "every invocation re-fetches checksums.txt before ever reading it, never behind the cache-hit branch" is true of the code as written, and the leftover on an aborted run is genuinely harmless.

🔑 The reason this comment is worth its lines

Most safety comments state that something is safe. This one states the condition the safety rests on, and then names the specific future edit that would break it — "making that fetch conditional on cache state, e.g. to save the 81-byte round-trip on a hit."

That is the difference between a comment a reader trusts and one a reader can check. The 81-byte optimisation is exactly the change someone would make in good faith while tidying, and it would convert a harmless leftover into an unrefreshed, silently-trusted input. Naming the plausible bad edit is the same move as pinning a control against a repair that has not been written yet.

📌 And the fragment carries the condition too, not just the conclusion — so the reasoning survives into the changelog rather than being reduced to "added a comment."

To be explicit about my own stamp: this approval is on content. The merge wants a green re-run first, and I am recording why the current red should not be read as a defect so that nobody re-derives it at 2am.

**APPROVE ON CONTENT @ `75536faac8c252da4de81e3671662b537c0d0a6e`** — with an explicit merge condition below, because **CI is 20-of-21 red and the red is NOT this PR.** ## 🔴 First: the CI failure is INFRASTRUCTURE, and here is the evidence Twenty-one gates do not break on a comment-only change. The log says why: ``` desktop-runner(version:v12.8.2) received task 29793 of job shellcheck 🐳 docker pull image=git.frankenbit.de/frankenbit/forgejo-ci-go:latest Error response from daemon: is a directory ``` **The runner could not pull the CI image.** Every failing task I sampled ran on `desktop-runner`, and the failure is at image-pull — before any repository content is read. ⚠️ **And my first control pointed the wrong way, which is worth recording.** Two other heads in the same 23:30–23:45 window were fully green: ``` 75536faa n=21 failure=20 success=1 ← this PR 88739cbf n=19 success=19 fb6548b0 n=21 success=21 ``` I read that as *"not infrastructure, so it must be the PR."* **It is infrastructure — just per-runner.** *"Other work succeeded in the same window" does not exclude an infra fault when the fault is on one runner and other work landed elsewhere.* The log settled it; the control could not. 📌 **Disposition: this needs a CI re-run, not a code change.** I would not merge on the current red, and I would not ask for a push either — a rebase would incidentally fix it by re-triggering, but that would attribute an infra fault to the branch. ## ✅ The content, and the claim it rests on — verified structurally **Comment-only:** 11 added lines, 11 comments, **0 non-comment**, 0 deletions. `bash -n` OK, `shellcheck` rc=0. Rebases onto current main cleanly (6 behind, 0 deletions on the replayed tree). **The safety argument asserts a condition, so I checked the condition rather than the prose:** ``` :118 guarded_fetch checksums.txt ← UNCONDITIONAL, top level :120 if [ "${cached}" -eq 1 ] ← the cache branch starts AFTER :158 expected="$(grep … checksums.txt)" ← the first READ fetch < branch < read ✅ the claimed condition holds ``` **So *"every invocation re-fetches `checksums.txt` before ever reading it, never behind the cache-hit branch"* is true of the code as written**, and the leftover on an aborted run is genuinely harmless. ## 🔑 The reason this comment is worth its lines Most safety comments state that something *is* safe. This one **states the condition the safety rests on, and then names the specific future edit that would break it** — *"making that fetch conditional on cache state, e.g. to save the 81-byte round-trip on a hit."* **That is the difference between a comment a reader trusts and one a reader can check.** The 81-byte optimisation is exactly the change someone would make in good faith while tidying, and it would convert a harmless leftover into an unrefreshed, silently-trusted input. **Naming the plausible bad edit is the same move as pinning a control against a repair that has not been written yet.** 📌 **And the fragment carries the condition too**, not just the conclusion — so the reasoning survives into the changelog rather than being reduced to "added a comment." --- **To be explicit about my own stamp: this approval is on content.** The merge wants a green re-run first, and I am recording *why* the current red should not be read as a defect so that nobody re-derives it at 2am.