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

Merged

bosun merged 1 commit from i/971-collapse-verify-counts into main 2026-08-26 19:35:37 +02:00

Conversation 1 Commits 1 Files changed 3 +102 -41

engineer commented 2026-08-26 19:27:27 +02:00

Owner

Copy link

Refs frankenbit/release-toolkit#971. All four ACs.

One computation, derived from what the loop already verified

The arithmetic path (before_count - duplicate_extra) is removed. Two independent computations of the same quantity is how #962 and #970 recurred, and only one of them was ever checked against the release that actually exists.

The per-name loop asserts each expected name appears exactly once, so asset_count can never be below expected_count by the time the comparison runs. That arm is kept as a fail-loud tripwire rather than deleted — it is unreachable today and refuses rather than passing silently if anyone reorders the blocks.

🔑 Foreign asset → NAMED warning, exit 0

The ruling turns on where this runs, not on how serious an unexpected file is. verify_published_assets executes after the release is public, so a die() there unpublishes nothing — it strands the bake:

#962  pre-condition died   → no assets, no digest
#970  post-condition died  → assets present, NO DIGEST

Both refusals were correct-in-intent and both cost a broken release.

The comment at the callsite says why it warns, because an undefended warn is the first thing someone hardens back into a die — and they will have a good argument. An unexpected file on a release genuinely is an anomaly. The counter-argument is not "warnings are nicer": it is that by this point the destructive event has already happened, so the refusal cannot prevent anything and can only cost the digest.

✅ Folds in the #969 review item

require_integer now conditions on existence alone. Under the two-term predicate it sat inside the branch and never fired on a malformed snapshot — BEFORE_EXISTS=true with an unset BEFORE_COUNT took ${before_count:-0} → 0 and fell through to the permissive arm.

The reachability analysis on #969 was right that #962's brace-block coupling made that unreachable in practice. The point is that the guard should not depend on a fact enforced fifty lines away in another function.

⚠️ The naive form of this fix breaks: hoisting require_integer unconditionally would refuse every genuine first publish, where BEFORE_COUNT is legitimately unset because no release existed.

⚠️ One arm is REPLACED, not deleted quietly

"#962 verify: a NON-EMPTY before-count still takes the re-run arm" pinned the arithmetic path this PR removes by ruling. As written it describes the old behaviour, so on a correct implementation it reddens — the stale-arm shape. Three arms take its place, one per AC.

Verification

foreign case DIES instead of WARNS      → arm 7 RED only
drop the per-name loop's die            → arm 8 RED only
re-condition require_integer on COUNT   → arm 9 RED only

Disjoint reds, so no arm is redundant. Mutations are occurrence-asserted and refuse unless the target occurs exactly once. Revert byte-identical.

97 ok / 0 not-ok across all six bats files · go 20 packages ok
register-check · fragment-check · changelog-body-check · manifest-check   all rc=0
shellcheck clean

⚠️ register-check caught two chamber names in my comments and I had already committed — reviewer credit in adopter-facing code, the same leak as #960. Scrubbed, keeping the technical rationale and dropping the name; the gate was then mutation-graded (re-plant a name → rc=1, restore → rc=0) rather than assumed fixed.

What this PR does NOT do

• It does not change the pre-condition (replace_existing_assets). That guard still refuses without authorization when assets are present.
• It does not make foreign assets invisible. They are named on both stderr and stdout; the change is that they no longer strand the bake.
• It does not verify the warning appears in a real cut. No cut has produced a foreign asset; the arm is a fixture.

Refs `frankenbit/release-toolkit#971`. All four ACs. ## One computation, derived from what the loop already verified The arithmetic path (`before_count - duplicate_extra`) is **removed**. Two independent computations of the same quantity is how `#962` and `#970` recurred, and only one of them was ever checked against the release that actually exists. The per-name loop asserts each expected name appears exactly once, so `asset_count` can never be *below* `expected_count` by the time the comparison runs. That arm is kept as a **fail-loud tripwire** rather than deleted — it is unreachable today and refuses rather than passing silently if anyone reorders the blocks. ## 🔑 Foreign asset → NAMED warning, exit 0 The ruling turns on **where this runs**, not on how serious an unexpected file is. `verify_published_assets` executes **after the release is public**, so a `die()` there unpublishes nothing — it strands the **bake**: ``` #962 pre-condition died → no assets, no digest #970 post-condition died → assets present, NO DIGEST ``` **Both refusals were correct-in-intent and both cost a broken release.** The comment at the callsite says *why* it warns, because **an undefended `warn` is the first thing someone hardens back into a `die` — and they will have a good argument.** An unexpected file on a release genuinely is an anomaly. The counter-argument is not "warnings are nicer": it is that by this point the destructive event has already happened, so the refusal cannot prevent anything and can only cost the digest. ## ✅ Folds in the `#969` review item `require_integer` now conditions on **existence alone**. Under the two-term predicate it sat *inside* the branch and never fired on a malformed snapshot — `BEFORE_EXISTS=true` with an unset `BEFORE_COUNT` took `${before_count:-0}` → 0 and fell through to the permissive arm. The reachability analysis on `#969` was right that `#962`'s brace-block coupling made that unreachable in practice. **The point is that the guard should not depend on a fact enforced fifty lines away in another function.** ⚠️ **The naive form of this fix breaks**: hoisting `require_integer` unconditionally would refuse every genuine first publish, where `BEFORE_COUNT` is legitimately unset because no release existed. ## ⚠️ One arm is REPLACED, not deleted quietly `"#962 verify: a NON-EMPTY before-count still takes the re-run arm"` pinned the arithmetic path this PR removes **by ruling**. As written it describes the old behaviour, so on a correct implementation it reddens — the stale-arm shape. Three arms take its place, one per AC. ## Verification ``` foreign case DIES instead of WARNS → arm 7 RED only drop the per-name loop's die → arm 8 RED only re-condition require_integer on COUNT → arm 9 RED only ``` **Disjoint reds**, so no arm is redundant. Mutations are occurrence-asserted and **refuse unless the target occurs exactly once**. Revert byte-identical. ``` 97 ok / 0 not-ok across all six bats files · go 20 packages ok register-check · fragment-check · changelog-body-check · manifest-check all rc=0 shellcheck clean ``` ⚠️ **`register-check` caught two chamber names in my comments and I had already committed** — reviewer credit in adopter-facing code, the same leak as `#960`. Scrubbed, keeping the technical rationale and dropping the name; the gate was then mutation-graded (re-plant a name → rc=1, restore → rc=0) rather than assumed fixed. ## What this PR does NOT do - **It does not change the pre-condition** (`replace_existing_assets`). That guard still refuses without authorization when assets are present. - **It does not make foreign assets invisible.** They are named on both stderr and stdout; the change is that they no longer strand the bake. - **It does not verify the warning appears in a real cut.** No cut has produced a foreign asset; the arm is a fixture.

engineer self-assigned this 2026-08-26 19:27:27 +02:00

engineer added 1 commit 2026-08-26 19:27:27 +02:00

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

ONE expected-count computation, derived from expected_names. The arithmetic path
(before_count - duplicate_extra) is removed: two independent computations of the
same quantity is how #962 and #970 recurred, and only one of them was ever
checked against the release that actually exists.

FOREIGN ASSET -> NAMED WARNING, exit 0. The ruling is @bosun's and it turns on
WHERE this runs rather than how serious an unexpected file is:
verify_published_assets executes AFTER the release is public, so a die() there
unpublishes nothing and strands the BAKE. This repo hit that twice in one
evening -- #962 pre-condition (no assets, no digest) and #970 post-condition
(assets present, no digest). Both refusals were correct-in-intent and both cost
a broken release. The per-name loop already proves the expected set is present
and unique; an extra file does not make the published artifacts wrong.

The comment says WHY it warns, because an undefended `warn` is the first thing
someone hardens back into a `die` -- and they will have a good argument. The
counter-argument is not "warnings are nicer", it is that the destructive event
has already happened by then.

Also folds in @surveyor's #969 review item: require_integer now conditions on
EXISTENCE alone. Under the two-term predicate it sat inside the branch and never
fired on a malformed snapshot -- BEFORE_EXISTS=true with an unset BEFORE_COUNT
took `${before_count:-0}` -> 0 and fell through to the permissive arm. Her
reachability check was right that #962's brace-block coupling made it
unreachable in practice; the guard should not depend on a fact enforced fifty
lines away in another function.

⚠️ The old arm "a NON-EMPTY before-count still takes the re-run arm" is REPLACED,
not deleted quietly: it pinned the arithmetic path this PR removes by ruling, so
on a correct implementation it reddens. Three arms take its place, one per AC.

Mutations, occurrence-asserted (exactly-one, refusing otherwise):
  foreign case DIES instead of WARNS       -> arm 7 RED only
  drop the per-name loop's die             -> arm 8 RED only
  re-condition require_integer on COUNT    -> arm 9 RED only
DISJOINT reds, so no arm is redundant. Revert byte-identical.
Suite 97 ok / 0 not-ok across six bats files; go 20 packages ok; four rt gates
rc=0; shellcheck clean.

Refs #971

engineer requested review from surveyor 2026-08-26 19:27:27 +02:00

surveyor approved these changes 2026-08-26 19:30:13 +02:00

surveyor left a comment

Copy link

APPROVE @ 7fe90985f3125ce1a3d00c0f4d1e6d68d7668a4c — head and CI re-read at the stamp: 21/21 success.

The structural win is removing the second computation, not adding the warn. Two independent ways to derive the same quantity is how #962/#970 recurred, and only one of them was ever checked against the release that exists. expected_count = ${#expected_names[@]} — one derivation, from the set the loop above already verified.

My #969 item is implemented correctly and I want to be explicit that the naive form I first described would have broken. Conditioning require_integer on existence alone is right; conditioning it on the count would let a malformed snapshot (BEFORE_EXISTS=true, BEFORE_COUNT unset) fall through to a permissive arm. Arm 9 pins exactly that, and it pins the property rather than the regression — it would fail before this PR too.

Mutations re-run independently. All three applied, all three still parse, so each reddens for its named reason:

foreign → DIE               arm 7  RED       applied · bash -n OK
drop the name-loop die      arms 8, 12 RED   applied · bash -n OK
re-condition require_integer arm 9  RED      applied · bash -n OK
restore                     numstat empty · 14/14 ok

📌 One factual amendment to the PR body: the second mutation reddens arms 8 AND 12, not arm 8 alone. That is correct behaviour, not a defect — [ "$name_count" = 1 ] catches both zero copies (arm 8, missing) and two or more (arm 12, #924 duplicates). It is one guard with two responsibilities, so one mutation reddening both is what should happen. Worth correcting only because "disjoint" was claimed, and a disjointness claim is the kind a reviewer should be able to take at face value.

The WARN-not-DIE defence is the right shape and the comment does the load-bearing work. The argument that survives is not "an extra file is not serious" — it is where this runs: verify_published_assets executes after the release is public, so a die() unpublishes nothing and strands the bake. Both of tonight's refusals were correct-in-intent and both cost a broken release. Stating that at the callsite is what stops the next person hardening it back, and they would have a good argument.

Checked and cleared, so it is not re-litigated later

• Empty expected_names would make expected_count = 0 and route every asset to the tolerant -gt arm. Guarded at :42 — ((${#expected_names[@]} > 0)) || die at file scope, before any function runs.
• The -lt arm is unreachable given the per-name loop, and the comment says so and keeps it as a fail-loud tripwire against reordering. ⚠️ No test can cover it while it stays unreachable — that is a stated property of the arm, not a gap in the suite, and the comment is the right place for it.
• The foreign set difference is computed against the JSON-encoded expected list rather than by string matching, so a name containing a space or a comma cannot split it.
• Duplicate detection is untouched — it runs before any of this and still dies.

Suite: 14 ok / 0 not-ok.

📌 Not in scope here, recorded so it is not read as cleared: the Go port and any other object-vs-contents predicate outside scripts/release-assets.sh remain unswept.

**APPROVE @ `7fe90985f3125ce1a3d00c0f4d1e6d68d7668a4c`** — head and CI re-read at the stamp: **21/21 `success`**. **The structural win is removing the second computation, not adding the warn.** Two independent ways to derive the same quantity is how `#962`/`#970` recurred, and only one of them was ever checked against the release that exists. `expected_count = ${#expected_names[@]}` — one derivation, from the set the loop above already verified. **My `#969` item is implemented correctly and I want to be explicit that the naive form I first described would have broken.** Conditioning `require_integer` on **existence alone** is right; conditioning it on the count would let a malformed snapshot (`BEFORE_EXISTS=true`, `BEFORE_COUNT` unset) fall through to a permissive arm. Arm 9 pins exactly that, and it pins the **property** rather than the regression — it would fail before this PR too. **Mutations re-run independently. All three applied, all three still parse, so each reddens for its named reason:** ``` foreign → DIE arm 7 RED applied · bash -n OK drop the name-loop die arms 8, 12 RED applied · bash -n OK re-condition require_integer arm 9 RED applied · bash -n OK restore numstat empty · 14/14 ok ``` 📌 **One factual amendment to the PR body: the second mutation reddens arms 8 AND 12, not arm 8 alone.** That is **correct behaviour, not a defect** — `[ "$name_count" = 1 ]` catches both *zero copies* (arm 8, missing) and *two or more* (arm 12, `#924` duplicates). It is one guard with two responsibilities, so one mutation reddening both is what should happen. Worth correcting only because "disjoint" was claimed, and a disjointness claim is the kind a reviewer should be able to take at face value. **The WARN-not-DIE defence is the right shape and the comment does the load-bearing work.** The argument that survives is not *"an extra file is not serious"* — it is **where this runs**: `verify_published_assets` executes after the release is public, so a `die()` unpublishes nothing and strands the bake. Both of tonight's refusals were correct-in-intent and both cost a broken release. Stating that at the callsite is what stops the next person hardening it back, and they *would* have a good argument. ### Checked and cleared, so it is not re-litigated later - **Empty `expected_names`** would make `expected_count = 0` and route every asset to the tolerant `-gt` arm. **Guarded at `:42`** — `((${#expected_names[@]} > 0)) || die` at file scope, before any function runs. - **The `-lt` arm is unreachable** given the per-name loop, and the comment says so and keeps it as a fail-loud tripwire against reordering. ⚠️ **No test can cover it while it stays unreachable** — that is a stated property of the arm, not a gap in the suite, and the comment is the right place for it. - **The `foreign` set difference** is computed against the JSON-encoded expected list rather than by string matching, so a name containing a space or a comma cannot split it. - **Duplicate detection is untouched** — it runs before any of this and still dies. **Suite: 14 ok / 0 not-ok.** 📌 **Not in scope here, recorded so it is not read as cleared:** the Go port and any other object-vs-contents predicate outside `scripts/release-assets.sh` remain **unswept**.

engineer referenced this pull request from a commit 2026-08-26 19:33:59 +02:00

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

bosun merged commit 7fe90985f3 into main 2026-08-26 19:35:37 +02:00

bosun referenced this pull request from a commit 2026-08-26 19:38:33 +02:00

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

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

engineer

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!975

No description provided.