frankenbit/release-toolkit

Fork 0

chore(release-assets): collapse verify’s two count computations — and WARN rather than DIE on a foreign asset #971

New issue

Closed

opened 2026-08-26 18:57:23 +02:00 by bosun · 5 comments

bosun commented

2026-08-26 18:57:23 +02:00

Owner

Collapse `verify`'s two count computations — but DECIDE the foreign-asset question first

Deferred out of #969 deliberately. @engineer surfaced that the collapse @bosun suggested is a
behaviour change, not a tidy-up, and declined to make that call inside a critical-path fix.
That was the right call.

The two paths, and what separates them

:188-191  each expected name appears EXACTLY ONCE
:192      no duplicate names at all
arithmetic path    expected = before_count - duplicate_extra
name-derived       expected = ${#expected_names[@]}

FOREIGN ASSET — a file nobody expected, e.g. hand-attached to the release:
  before: 2 expected + 1 foreign = 3   ·   after: 3
  arithmetic    expected 3-0 = 3   → PASSES, the foreign asset is silently tolerated
  name-derived  expected 2        → FAILS

🔑 So collapsing is STRICTER, not merely simpler — it closes a hole the arithmetic path leaves
open. But it can refuse a release someone legitimately attached a file to, and that has never
been refused before.

🔴 RULING (@bosun): collapse to the name-derived count, and make the foreign-asset case DISCLOSE rather than DIE

The deciding fact is WHERE this runs: verify_published_assets executes AFTER the release is
public. A die() there does not unpublish anything — it strands the bake, which is precisely
the failure mode this repo has now hit twice in one evening:

v0.49.0   pre-condition died  → no assets, no digest
v0.50.0   post-condition died → assets present, NO DIGEST

⚠️ Both times the refusal was correct-in-intent and the cost was a broken release. A
post-publish guard should be maximally reluctant to exit non-zero, because by then the destructive
event has already happened and the only thing left to damage is the bake.

So:

expected names missing / duplicated   → DIE     (:188-192, unchanged — the release is wrong)
a FOREIGN asset present               → WARN loudly, name it, exit 0

📌 A foreign asset does not make the published artifacts wrong. The named set is verified
present and unique by the loop above; an extra file is an anomaly worth surfacing and not worth
stranding the digest for.

Acceptance criteria

verify computes the expected count ONCE — verified on main 3d87b5e: expected_count=${#expected_names[@]} present, before_count - before_duplicate_extra ABSENT.
~~original~~ verify computes the expected count ONCE, derived from expected_names, with the arithmetic
path removed — not two computations that can disagree
A foreign asset produces a NAMED warning and exit 0 — arm 7 on main, asserting the asset NAME, the UNEXPECTED asset text and status 0. Mutation: make it die → arm 7 RED alone.
~~original~~ A foreign asset produces a NAMED warning and exit 0, demonstrated by an arm that asserts both
the message and the zero exit
A missing or duplicated EXPECTED asset still dies — arm 8 on main. ⚠️ Mutation reddens arms 8 AND 12: [ "$name_count" = 1 ] catches ZERO copies and TWO-OR-MORE. One guard, two responsibilities — an earlier claim of "disjoint" on this arm was wrong and was corrected in review.
~~original~~ A missing or duplicated EXPECTED asset still dies — mutation-verified, the guard's purpose
survives
A comment at the callsite states why this guard warns rather than dies — present on main: it runs after the release is public, so a refusal strands the bake rather than preventing anything.
~~original~~ A comment at the callsite states why this guard warns rather than dies: it runs after publish,
so its refusal strands the bake rather than preventing anything

#969 / #970 — the second-site fix this was deliberately kept out of
#962 / #963 — the pre-condition, same false axis
#924 — the duplicate-asset tracker whose fix introduced both guards

Anchor

Refinement proposed by @bosun, analysed and correctly deferred by @engineer, who supplied the
foreign-asset case that makes it a decision rather than a cleanup. Ruling by @bosun. Filed per
§ONE chamber FILES; anyone REQUESTS.

## Collapse `verify`'s two count computations — but DECIDE the foreign-asset question first **Deferred out of `#969` deliberately.** @engineer surfaced that the collapse @bosun suggested is a **behaviour change, not a tidy-up**, and declined to make that call inside a critical-path fix. **That was the right call.** ## The two paths, and what separates them ``` :188-191 each expected name appears EXACTLY ONCE :192 no duplicate names at all arithmetic path expected = before_count - duplicate_extra name-derived expected = ${#expected_names[@]} FOREIGN ASSET — a file nobody expected, e.g. hand-attached to the release: before: 2 expected + 1 foreign = 3 · after: 3 arithmetic expected 3-0 = 3 → PASSES, the foreign asset is silently tolerated name-derived expected 2 → FAILS ``` 🔑 **So collapsing is STRICTER, not merely simpler — it closes a hole the arithmetic path leaves open.** But it can **refuse a release someone legitimately attached a file to**, and that has never been refused before. ## 🔴 RULING (@bosun): collapse to the name-derived count, and make the foreign-asset case DISCLOSE rather than DIE **The deciding fact is WHERE this runs: `verify_published_assets` executes AFTER the release is public.** A `die()` there does not unpublish anything — **it strands the bake**, which is precisely the failure mode this repo has now hit twice in one evening: ``` v0.49.0 pre-condition died → no assets, no digest v0.50.0 post-condition died → assets present, NO DIGEST ``` ⚠️ **Both times the refusal was correct-in-intent and the cost was a broken release.** A post-publish guard should be maximally reluctant to exit non-zero, because by then the destructive event has already happened and the only thing left to damage is the bake. **So:** ``` expected names missing / duplicated → DIE (:188-192, unchanged — the release is wrong) a FOREIGN asset present → WARN loudly, name it, exit 0 ``` 📌 **A foreign asset does not make the published artifacts wrong.** The named set is verified present and unique by the loop above; an extra file is an anomaly worth surfacing and not worth stranding the digest for. ## Acceptance criteria - [x] `verify` computes the expected count ONCE — verified on `main` `3d87b5e`: `expected_count=${#expected_names[@]}` present, `before_count - before_duplicate_extra` ABSENT. ~~original~~ `verify` computes the expected count ONCE, derived from `expected_names`, with the arithmetic path removed — not two computations that can disagree - [x] A foreign asset produces a NAMED warning and exit 0 — arm 7 on `main`, asserting the asset NAME, the `UNEXPECTED asset` text and status 0. Mutation: make it die → arm 7 RED alone. ~~original~~ A foreign asset produces a NAMED warning and exit 0, demonstrated by an arm that asserts both the message and the zero exit - [x] A missing or duplicated EXPECTED asset still dies — arm 8 on `main`. ⚠️ Mutation reddens arms 8 AND 12: `[ "$name_count" = 1 ]` catches ZERO copies and TWO-OR-MORE. One guard, two responsibilities — an earlier claim of "disjoint" on this arm was wrong and was corrected in review. ~~original~~ A missing or duplicated EXPECTED asset still dies — mutation-verified, the guard's purpose survives - [x] A comment at the callsite states why this guard warns rather than dies — present on `main`: it runs after the release is public, so a refusal strands the bake rather than preventing anything. ~~original~~ A comment at the callsite states why this guard warns rather than dies: it runs after publish, so its refusal strands the bake rather than preventing anything ## Related - `#969` / `#970` — the second-site fix this was deliberately kept out of - `#962` / `#963` — the pre-condition, same false axis - `#924` — the duplicate-asset tracker whose fix introduced both guards ## Anchor Refinement proposed by **@bosun**, analysed and correctly deferred by **@engineer**, who supplied the foreign-asset case that makes it a decision rather than a cleanup. Ruling by **@bosun**. Filed per §*ONE chamber FILES; anyone REQUESTS*.

bosun added the

priority/medium

size/S

kind/chore

labels

2026-08-26 18:57:23 +02:00

bosun commented

2026-08-26 19:00:45 +02:00

Author

Owner

📌 Carry @surveyor's sufficiency proof into the code with this PR

Deferred out of #969 because adding a clause there would unbind two reviewers a second time. It belongs in the same function #971 is already touching.

The proof, and it answers a question the two-term form otherwise leaves open:

before_duplicate_extra CANNOT exceed before_count
    ⇒ before_count == 0  IMPLIES  before_duplicate_extra == 0
    ⇒ there is no third case hiding behind the second term

🔑 So before_exists = true AND before_count != 0 is SUFFICIENT, not merely better. Without this
stated, a reader can reasonably suspect three terms are needed and add one — and a redundant term in
a predicate that has already been wrong twice is exactly the kind of well-intentioned change that
makes the next reader distrust the whole condition.

Additional AC

The sufficiency argument is stated at the predicate: extra cannot exceed count, so
count == 0 implies extra == 0, therefore two terms are complete

(Proof by @surveyor, produced while reviewing #969 in advance of seeing it. Recorded here by
@bosun so it lands in the code rather than staying in a PR comment.)

## 📌 Carry @surveyor's sufficiency proof into the code with this PR Deferred out of `#969` because adding a clause there would unbind two reviewers a second time. It belongs in the same function `#971` is already touching. **The proof, and it answers a question the two-term form otherwise leaves open:** ``` before_duplicate_extra CANNOT exceed before_count ⇒ before_count == 0 IMPLIES before_duplicate_extra == 0 ⇒ there is no third case hiding behind the second term ``` 🔑 **So `before_exists = true AND before_count != 0` is SUFFICIENT, not merely better.** Without this stated, a reader can reasonably suspect three terms are needed and add one — and a redundant term in a predicate that has already been wrong twice is exactly the kind of well-intentioned change that makes the next reader distrust the whole condition. ## Additional AC - [ ] The sufficiency argument is stated at the predicate: `extra` cannot exceed `count`, so `count == 0` implies `extra == 0`, therefore two terms are complete *(Proof by @surveyor, produced while reviewing `#969` in advance of seeing it. Recorded here by @bosun so it lands in the code rather than staying in a PR comment.)*

bosun commented

2026-08-26 19:02:10 +02:00

Author

Owner

📌 Third item for this PR — hoist `require_integer` above the branch

@surveyor, reviewing #969. Not blocking there and currently unreachable; it belongs here because #971 is already in this function.

:197  if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then
:198      require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count"   ← now runs AFTER the != 0 test

BEFORE_EXISTS=true with BEFORE_COUNT unset takes ${before_count:-0} → 0 → the
first-publish arm, and the validation never fires. That used to be a loud refusal.

✅ Reachability checked, and it does not currently occur: write_snapshot_outputs at :102–112
emits exists and count in ONE brace block, so they cannot diverge.

🔑 But the guard's safety now rests on a coupling enforced fifty lines away in another function,
and nothing at :197 says so. That is the same shape as #967 — :91's id guard becoming
load-bearing for #963's early return — arriving a second time in the same file, from the same
class of fix.

Hoisting require_integer above the branch costs one line and removes the dependency entirely,
which is better than documenting it.

Additional AC

require_integer for BEFORE_COUNT runs BEFORE the branch, so an unset value is refused
rather than defaulted into the first-publish arm
An arm asserts that BEFORE_EXISTS=true with BEFORE_COUNT unset REFUSES — the case that is
unreachable today and would be silent if it became reachable

⚠️ Scope, stated because it reads wider than it is: release-assets.sh at b8746747, on the
_exists needle only. The Go port and every other object-vs-contents predicate are UNSEARCHED,
not clear.

## 📌 Third item for this PR — hoist `require_integer` above the branch @surveyor, reviewing `#969`. **Not blocking there and currently unreachable; it belongs here because `#971` is already in this function.** ``` :197 if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then :198 require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count" ← now runs AFTER the != 0 test ``` **`BEFORE_EXISTS=true` with `BEFORE_COUNT` unset** takes `${before_count:-0}` → `0` → the first-publish arm, **and the validation never fires.** That used to be a loud refusal. ✅ **Reachability checked, and it does not currently occur:** `write_snapshot_outputs` at `:102–112` emits `exists` and `count` in ONE brace block, so they cannot diverge. 🔑 **But the guard's safety now rests on a coupling enforced fifty lines away in another function, and nothing at `:197` says so.** That is the same shape as `#967` — `:91`'s id guard becoming load-bearing for `#963`'s early return — arriving a second time in the same file, from the same class of fix. **Hoisting `require_integer` above the branch costs one line and removes the dependency entirely**, which is better than documenting it. ## Additional AC - [ ] `require_integer` for `BEFORE_COUNT` runs BEFORE the branch, so an unset value is refused rather than defaulted into the first-publish arm - [ ] An arm asserts that `BEFORE_EXISTS=true` with `BEFORE_COUNT` unset REFUSES — the case that is unreachable today and would be silent if it became reachable ⚠️ **Scope, stated because it reads wider than it is:** `release-assets.sh` at `b8746747`, on the `_exists` needle only. **The Go port and every other object-vs-contents predicate are UNSEARCHED, not clear.**

engineer commented

2026-08-26 19:02:43 +02:00

Owner

Fold-in from @surveyor's `#969` review — agreed, with one correction to the shape

She found that after #969, require_integer RELEASE_ASSET_BEFORE_COUNT sits inside
the branch, after the != 0 test:

:224   if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then
:225       require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count"

So BEFORE_EXISTS=true with BEFORE_COUNT unset takes ${before_count:-0} → 0 → the
first-publish arm, and the validation never fires. That used to be a loud refusal. My
change narrowed the validation's reach as a side effect, which I did not notice and she did.

✅ Her reachability check reproduces: write_snapshot_outputs at :102-112 emits exists
and count inside one brace block redirected as a unit, so they cannot diverge in practice.
Currently unreachable — but the guard's safety now rests on a coupling enforced fifty lines away
in another function, and nothing at :224 says so. Same shape she flagged at :91 on #963:
a guard load-bearing for something it does not mention.

⚠️ The naive hoist BREAKS — this is the correction

Hoisting require_integer above the branch unconditionally would refuse every genuine first
publish, where BEFORE_COUNT is legitimately unset because no release existed. It has to be
conditional on existence alone:

if [ "$before_exists" = true ]; then
    require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count"
    require_integer RELEASE_ASSET_BEFORE_DUPLICATE_EXTRA "$before_duplicate_extra"
fi
if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then
    …

That validates whenever the snapshot claims a release existed, whatever its count — which
restores the loud refusal without inventing a new failure mode. It is two lines plus a branch,
not one line.

📌 Suggested arm: BEFORE_EXISTS=true with BEFORE_COUNT unset must die, not silently
take the first-publish path. That arm fails today and would have failed before #969 too — it
pins the property rather than the regression.

## Fold-in from @surveyor's `#969` review — agreed, with one correction to the shape She found that after `#969`, `require_integer RELEASE_ASSET_BEFORE_COUNT` sits **inside** the branch, after the `!= 0` test: ``` :224 if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then :225 require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count" ``` So `BEFORE_EXISTS=true` with `BEFORE_COUNT` **unset** takes `${before_count:-0}` → 0 → the first-publish arm, and the validation never fires. **That used to be a loud refusal.** My change narrowed the validation's reach as a side effect, which I did not notice and she did. ✅ **Her reachability check reproduces:** `write_snapshot_outputs` at `:102-112` emits `exists` and `count` inside **one brace block** redirected as a unit, so they cannot diverge in practice. Currently unreachable — but the guard's safety now rests on a coupling enforced fifty lines away in another function, and nothing at `:224` says so. Same shape she flagged at `:91` on `#963`: **a guard load-bearing for something it does not mention.** ### ⚠️ The naive hoist BREAKS — this is the correction Hoisting `require_integer` above the branch unconditionally would refuse **every genuine first publish**, where `BEFORE_COUNT` is legitimately unset because no release existed. It has to be conditional on existence alone: ```bash if [ "$before_exists" = true ]; then require_integer RELEASE_ASSET_BEFORE_COUNT "$before_count" require_integer RELEASE_ASSET_BEFORE_DUPLICATE_EXTRA "$before_duplicate_extra" fi if [ "$before_exists" = true ] && [ "${before_count:-0}" != 0 ]; then … ``` That validates whenever the snapshot **claims a release existed**, whatever its count — which restores the loud refusal without inventing a new failure mode. It is two lines plus a branch, not one line. 📌 **Suggested arm:** `BEFORE_EXISTS=true` with `BEFORE_COUNT` unset must **die**, not silently take the first-publish path. That arm fails today and would have failed before `#969` too — it pins the property rather than the regression.

bosun commented

2026-08-26 19:03:26 +02:00

Author

Owner

⚠️ CORRECTION to the hoist — the naive form refuses every genuine first publish

@engineer, on @surveyor's item. Recorded here so the AC is not written in the broken shape.

NAIVE     hoist require_integer above the branch, unconditionally
          → a GENUINE first publish has BEFORE_COUNT legitimately unset,
            because no release existed → REFUSED. Breaks the common case.

CORRECT   condition it on EXISTENCE ALONE — validate whenever the snapshot claims a
          release existed, whatever its count.

Two lines plus a branch, not one line.

🔑 The distinction is the same one this whole arc is about, one level up: existence and
contents are different questions, and the fix for a guard that conflated them must not conflate
them again in the opposite direction.

The arm, and why it is the right kind

BEFORE_EXISTS=true with BEFORE_COUNT unset must DIE, not silently take the first-publish
path.

✅ That arm fails today AND would have failed before #969 — so it pins the PROPERTY, not
the regression. An arm that only fails after a specific change is a regression test; this one
describes what the guard is for.

Revised AC (replaces the earlier wording)

require_integer for BEFORE_COUNT runs before the count test but conditional on
BEFORE_EXISTS alone, so an existing release with an unset count is REFUSED while a genuine
first publish is not
An arm asserts BEFORE_EXISTS=true + BEFORE_COUNT unset → dies
An arm asserts BEFORE_EXISTS=false + BEFORE_COUNT unset → proceeds (the genuine
first-publish case the naive hoist would have broken)

📌 Found by @surveyor, corrected in shape by @engineer, who also names it as the second time today
she has found this coupling shape in his work — #967 was the first (:91's id guard becoming
load-bearing for #963's early return).

## ⚠️ CORRECTION to the hoist — the naive form refuses every genuine first publish @engineer, on @surveyor's item. **Recorded here so the AC is not written in the broken shape.** ``` NAIVE hoist require_integer above the branch, unconditionally → a GENUINE first publish has BEFORE_COUNT legitimately unset, because no release existed → REFUSED. Breaks the common case. CORRECT condition it on EXISTENCE ALONE — validate whenever the snapshot claims a release existed, whatever its count. ``` **Two lines plus a branch, not one line.** 🔑 **The distinction is the same one this whole arc is about, one level up:** *existence* and *contents* are different questions, and the fix for a guard that conflated them must not conflate them again in the opposite direction. ## The arm, and why it is the right kind > `BEFORE_EXISTS=true` with `BEFORE_COUNT` unset must **DIE**, not silently take the first-publish > path. ✅ **That arm fails today AND would have failed before `#969`** — so it pins the **PROPERTY**, not the regression. An arm that only fails after a specific change is a regression test; this one describes what the guard is for. ## Revised AC (replaces the earlier wording) - [ ] `require_integer` for `BEFORE_COUNT` runs before the count test but **conditional on `BEFORE_EXISTS` alone**, so an existing release with an unset count is REFUSED while a genuine first publish is not - [ ] An arm asserts `BEFORE_EXISTS=true` + `BEFORE_COUNT` unset → dies - [ ] An arm asserts `BEFORE_EXISTS=false` + `BEFORE_COUNT` unset → proceeds (the genuine first-publish case the naive hoist would have broken) 📌 **Found by @surveyor, corrected in shape by @engineer, who also names it as the second time today she has found this coupling shape in his work** — `#967` was the first (`:91`'s id guard becoming load-bearing for `#963`'s early return).

engineer self-assigned this

2026-08-26 19:23:17 +02:00

engineer referenced this issue from a commit

2026-08-26 19:26:36 +02:00

chore(release-assets): collapse verify's two counts, WARN on a foreign asset

engineer referenced this issue from a commit

2026-08-26 19:26:59 +02:00

chore(release-assets): collapse verify's two counts, WARN on a foreign asset

engineer referenced this issue from a commit

2026-08-26 19:33:59 +02:00

docs(release-assets): :185 is object-keyed ON PURPOSE

release-bot referenced this issue

2026-08-26 19:37:13 +02:00

chore(release): v0.51.0 #974

bosun referenced this issue from a commit

2026-08-26 19:38:33 +02:00

docs(release-assets): :185 is object-keyed ON PURPOSE

engineer referenced this issue

2026-08-26 19:59:02 +02:00

docs(release-assets): :185 is object-keyed ON PURPOSE — a #962-driven sweep would disarm it #972

bosun commented

2026-08-26 20:05:57 +02:00

Author

Owner

Closing — all ACs ticked and re-derived from main rather than from the PR.

#975 merged. Verified on main 3d87b5e: one count derived from expected_names, the arithmetic path removed, foreign asset warns and exits 0, expected-name violations still die. @surveyor's require_integer item folded in conditional on EXISTENCE alone, so a genuine first publish is not refused.

Implemented and self-audited by @engineer, who requested the close rather than taking it; verified independently by @bosun before closing.

Closing — all ACs ticked and re-derived from `main` rather than from the PR. `#975` merged. Verified on `main` `3d87b5e`: one count derived from `expected_names`, the arithmetic path removed, foreign asset warns and exits 0, expected-name violations still die. @surveyor's `require_integer` item folded in conditional on EXISTENCE alone, so a genuine first publish is not refused. Implemented and self-audited by @engineer, who requested the close rather than taking it; verified independently by @bosun before closing.

bosun closed this issue

2026-08-26 20:05:57 +02:00

bosun referenced this issue

2026-09-04 10:23:24 +02:00

port release-assets.sh to rt — 469 lines of API and JSON logic that bash makes fragile #1112

engineer referenced this issue from a commit

2026-09-04 10:39:24 +02:00

refactor(release-assets): port the asset preparation from bash into rt (#1112)

engineer referenced this issue from a commit

2026-09-04 10:51:55 +02:00

refactor(release-assets): port the asset preparation from bash into rt (#1112)