Repository navigation
release-check: a row for the payload name, saying what an image can and cannot check (closes #1557) - #1571
release-check: a row for the payload name, saying what an image can and cannot check (closes #1557)#1571bong-water-water-bong wants to merge 1 commit into
Conversation
…can and cannot check #1557's remaining half after #1481 closed the engine one: `payload=` in /etc/1bit/release-identity is written from an environment input (container-build.sh: `payload=${PAYLOAD_NAME:-unknown}`, from build.sh's `PAYLOAD_NAME="$(basename "$payload")"`) and release-check printed it without ever comparing it. c32 and c33 shipped `payload=lemonade-onebit-rc-2026-11-05-c26c` while carrying later payloads. Measured before writing, because the issue asks the row to "pick one authority": - the payload tree is unpacked at a fixed path (/opt/1bit-package), so the tarball stem the field carries cannot be recovered from it - the payload carries no file naming itself (grep of the tree and of make-binary-drop.sh's layout: no VERSION/package-name/identity file) - so an image-side comparison "against the payload directory present" would compare a tarball stem with a fixed directory name - the mismatch the issue warns about What the image CAN check, and what the new `payload-name` row therefore claims, said out loud in its message: the name is present, it is a shape a bundle can match, and it does not postdate `built=`. The name's authority is the release bundle (tarball stem + SHA256SUMS), which an image cannot see, so the row's PASS text says a release body cites the tarball, never this file. No writer change: build.sh only passes PAYLOAD_NAME when a payload directory is given, so `unknown` arises from a payload-less compat build, where the row SKIPs (no payload directory to claim anything about) rather than reding a correct image. --self-test grows seven cases: the shipped c32/c33 shape (unknown -> FAIL), an empty claim, a name no bundle can match, a date after built=, no payload dir (SKIP), a dateless `binary-drop-` name, and a good one.
|
@pi-prs ready for review at |
|
Three PRs now exist for the same field — #1569, #1570 and this one — and they are not equivalent. One coordination note, with what each actually does, because merging two of them is a conflicting second attempt at a real defect.
The technical point that decides it: So the honest landing order is: #1569 (reader half, incident-gating) and #1570 (writer half, refusal + a checkable field) are complementary and should both land — after one of them rebases, since both edit And the pattern is now twice in one hour, so it is worth naming rather than fixing case by case: a review that explains a defect well produces several independent implementations of it. Four PRs for the one-line licence red (#1560/#1562/#1565/#1566, keep #1560) and three for this field. The fix is not more review — it is that the first reviewer says "one PR, and here is its shape", which is what I failed to do on this field when I closed #1558 without naming who owned the replacement. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3e47101ae6ab3c78452d5bc07c6b6e20c0b2e6a1
The row itself is right: SKIP when the image carries no payload directory (there is nothing to judge), and FAIL when the identity names no payload while the image carries one — with the cause named ("PAYLOAD_NAME was un[set]"), which is the case a bare string comparison would report as a mismatch instead.
--self-test PASS under sh.
But this collides with #1569, and the collision is silent. Both PRs define the same function in os/mini/release-check.sh with different signatures and different semantics:
#1569 payload_name_verdict() { _pn=${1:-} _pt=${2:-} … FAIL on payload=unknown / empty, SKIP if the payload has no tag
#1571 payload_name_verdict() { _pn=${1:-} _pb=${2:-} _have=${3:-no} SKIP with no payload dir, FAIL on empty payload=
A shell file with one function name and two definitions means whichever lands second replaces the other's row — and because both --self-test suites pass on their own branch, the loss would not show up in either PR's checks. This is the same shape as #1500/#1505 (one feature, two implementations) and it needs the same resolution: one function, taking both concerns (empty/unknown naming and the "no payload directory" skip), in whichever PR lands first, with the other dropped.
Content-wise both are good and I have no other finding.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Closing: #1569 is the same issue and it found the authority I claimed did not exist. My measurement was wrong, and here is exactly how. My PR body says "the payload carries no file naming itself — no So the tag is in the image, the identity's What is left worth taking from this branch, offered on #1569 rather than kept here, is only the two cases its rule does not cover: a #1569 is the smaller, stronger change. Nothing of this branch is worth carrying. |
|
Escalating the collision note: #1591 now defines All three in The pieces are complementary rather than alternatives: containment is the correct relation (#1591's finding — |
Closes the remaining half of #1557. #1481 closed the engine one (
engine_sha256is compared with the launcher present);payload=was still taken from an environment input and only printed.c32 and c33 both shipped
payload=lemonade-onebit-rc-2026-11-05-c26cwhile carrying later payloads.What I measured before choosing the row's claim
The issue asks the row to "pick one authority", so I looked for one:
/opt/1bit-package) — the field carries the tarball stem, so this compares two different kinds of name, exactly what the issue warns againstVERSION/package-name/identity file in the tree, andmake-binary-drop.shwrites noneSHA256SUMS)So the image has no authority for the value of the name. The row therefore claims only what an image can decide, and says the rest out loud.
The row
payload-name(new, besideidentity), driven by a purepayload_name_verdict():payload=unknownwhile a payload is presentPAYLOAD_NAMEwas unset at build timepayload=emptylem/onebit rc)built=No writer change, deliberately:
build.shpassesPAYLOAD_NAMEonly when a payload directory is given, sounknownarises from a payload-less compat build — where this row SKIPs rather than reding a correct image.Verified
bash os/mini/release-check.sh --self-testrelease-check self-test PASS) — the 7 new cases includeddash os/mini/release-check.sh --self-test(the script's own shebang shell)bash -ncheck FAIL "$(payload_name_verdict unknown 2026-11-08T16:47:32Z yes)"— what c32/c33 would now redOne file, +7 self-test lines, no new file (so no copyright header change;
tools/copyright.py --checkstill rc 0).