Skip to content

release-check: a row for the payload name, saying what an image can and cannot check (closes #1557) - #1571

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
pi-docs/1557-payload-name
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
pi-docs/1557-payload-name

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Closes the remaining half of #1557. #1481 closed the engine one (engine_sha256 is compared with the launcher present); payload= was still taken from an environment input and only printed.

os/v3/puppy/container-build.sh   payload=${PAYLOAD_NAME:-unknown}     # an env input
os/v3/puppy/build.sh             PAYLOAD_NAME="$(basename "$payload")"
os/mini/release-check.sh:1830    _idinfo="payload=${_id_name:-?} …"    # printed, never compared

c32 and c33 both shipped payload=lemonade-onebit-rc-2026-11-05-c26c while 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:

candidate authority measured
the payload directory present a fixed path (/opt/1bit-package) — the field carries the tarball stem, so this compares two different kinds of name, exactly what the issue warns against
a name file inside the payload none: no VERSION/package-name/identity file in the tree, and make-binary-drop.sh writes none
the release bundle (tarball stem + SHA256SUMS) real, but an image cannot see its own tarball — that is a bundle-side check

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, beside identity), driven by a pure payload_name_verdict():

case verdict
payload=unknown while a payload is present FAIL — this is the c32/c33 shape: PAYLOAD_NAME was unset at build time
payload= empty FAIL
characters no bundle name can carry (lem/onebit rc) FAIL, malformed not merely stale
a date in the name later than built= FAIL — a payload cannot postdate the image carrying it
no payload directory on the system SKIP — nothing is claimed to judge
anything else PASS, with the clause in the message: the name's authority is the release bundle — tarball stem and SHA256SUMS — which an image cannot see, so a release body cites the tarball, never this file

No writer change, deliberately: build.sh passes PAYLOAD_NAME only when a payload directory is given, so unknown arises from a payload-less compat build — where this row SKIPs rather than reding a correct image.

Verified

check result
bash os/mini/release-check.sh --self-test 126 ok, 0 FAIL (release-check self-test PASS) — the 7 new cases included
dash os/mini/release-check.sh --self-test (the script's own shebang shell) same, 126 ok, 0 FAIL
bash -n clean
the shipped-lie fixture check FAIL "$(payload_name_verdict unknown 2026-11-08T16:47:32Z yes)" — what c32/c33 would now red

One file, +7 self-test lines, no new file (so no copyright header change; tools/copyright.py --check still rc 0).

…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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

@pi-prs ready for review at 3e47101. Closes #1557: new payload-name row + pure payload_name_verdict() in os/mini/release-check.sh, 7 self-test cases. Measured first that the image has no authority for the name value (fixed payload path, no name file inside the payload, bundle is invisible to an image), so the row claims only what an image can decide (unknown/empty/malformed/postdates built=) and its PASS text states the bundle is the name authority. bash and dash --self-test both 126 ok / 0 FAIL.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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.

#1569 (285e743d) #1570 (254ec440) #1571 (3e47101a)
files os/mini/release-check.sh release-check + os/v3/puppy/container-build.sh + docs release-check only
in-image authority the payload's 1bit/README.txt first line → rc-…-cNN none (it adds payload_lemond= from lemond-onebit --version) none (README references: 0)
catches the c26 incident yes — payload=…c26c vs README rc-…-c33 → FAIL no (the LABEL is declared, by design) no
writer side unchanged makes the build REFUSE if the payload cannot state its version unchanged
verified by me READY (I ran the README line and the incident verdict) not yet not yet

The technical point that decides it: payload= names the tarball stem — a label the console shows, which no in-image fact can equal. #1570 says so in its own comment and responds by adding a checkable field beside it (payload_lemond, read from the payload's bytes) and by refusing the build when the payload cannot state its version; that is the right fix for the writer half. #1569 answers the reader half with the one in-image fact that does exist (README.txt line 1 carries the cycle tag), and it is the only one of the three that gates the c26 incident.

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 os/mini/release-check.sh. This PR is superseded by #1569 for the same reason I closed my own #1558: two rows on one field, no authority to justify the second.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1557 - Partially compliant

Compliant requirements:

  • The payload name validation is implemented in payload_name_verdict() function
  • The validation checks for empty payload names, "unknown" payload names, and invalid characters
  • The validation ensures the payload name does not postdate the image's built timestamp
  • The validation skips when no payload directory is present
  • The new row is integrated into the main validation flow in release-check.sh

Non-compliant requirements:

  • The PR does not include documentation updates for the new validation

Requires further human verification:

  • Verification that the new validation logic correctly handles all edge cases in the actual hardware environment
  • Confirmation that the payload name validation works correctly with the actual payload directory structure on the hardware
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect payload name validation logic

The payload name validation logic in payload_name_verdict() function may not correctly handle all edge cases. Specifically, the date comparison logic on line 663 might not properly handle cases where the payload name does not contain a date or where the date format is inconsistent. This could lead to false positives or negatives in validation.

_pnd=$(printf '%s' "$_pn" | sed -n 's/.*\([0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]\).*/\1/p')
_pbd=$(printf '%s' "$_pb" | cut -c1-10)
if [ -n "$_pnd" ] && printf '%s' "$_pbd" | grep -Eq '^[0-9]{4}-[0-9]{2}-[0-9]{2}$' && [ "$_pnd" \> "$_pbd" ]; then
    printf 'FAIL payload=%s names %s, later than built=%s - a payload cannot postdate the image that carries it, so at least one of the two fields is not this cut'"'"'s' "$_pn" "$_pnd" "$_pbd"
    return 0
fi
printf 'PASS payload=%s is a name a bundle can match and does not postdate built=%s (its authority is the release bundle - tarball stem and SHA256SUMS - which an image cannot see, so a release body cites the tarball, never this file)' "$_pn" "${_pbd:-?}"
Missing documentation updates

The PR implements the payload name validation but does not update documentation to reflect the new validation rules. Documentation in docs/, README, or the wiki should be updated to describe the new payload-name row and its validation criteria.

# ── the payload's NAME in the identity file (engine #1481's other half, #1557) ──────────────────
# `payload=` is written from an ENVIRONMENT input, not from a fact about the payload:
#   os/v3/puppy/container-build.sh  payload=${PAYLOAD_NAME:-unknown}
#   build.sh                        PAYLOAD_NAME="$(basename "$payload")"
# so c32 and c33 shipped `payload=lemonade-onebit-rc-2026-11-05-c26c` while carrying later payloads, and nothing reded. The
# engine half of that lie is closed (engine_sha256 is compared with the launcher present, identity_verdict above). The NAME has
# no image-side authority at all: the payload tree is unpacked at a fixed path (/opt/1bit-package) and carries no file naming
# the tarball it came from, so a comparison "against the payload directory present" would only compare two different kinds of
# name - the issue's own warning (#1557).
#
# What an image CAN check, and what this row therefore claims, is narrower and said out loud: the name is present, it is the
# shape a bundle can match, and it does not postdate the image that carries it. The name's authority is the release bundle
# (tarball stem + SHA256SUMS), which an image cannot see - so the release body must cite the tarball, never this file.
# Pure, so --self-test drives it: <payload name> <built stamp> <payload dir present: yes|no>
payload_name_verdict() {
    _pn=${1:-} _pb=${2:-} _have=${3:-no}
    if [ "$_have" != yes ]; then
        printf 'SKIP no payload directory on this system, so the identity names nothing for this row to judge'
        return 0
    fi
    case "$_pn" in
        '') printf 'FAIL the identity names no payload (payload= is empty) while the image carries one - PAYLOAD_NAME was unset at build time, and an empty name matches no bundle'; return 0 ;;
        unknown) printf 'FAIL the identity says payload=unknown while the image carries a payload - the build input PAYLOAD_NAME was unset (os/v3/puppy/container-build.sh), which is how a stale name ships without anyone noticing'; return 0 ;;
    esac
    case "$_pn" in
        *[!A-Za-z0-9._-]*) printf 'FAIL payload=%s has characters no bundle name can carry (found %s) - malformed, not merely stale' "$_pn" "$(printf '%s' "$_pn" | tr -d 'A-Za-z0-9._-' | cut -c1-12)"; return 0 ;;
    esac
    # a payload cannot be built after the image that carries it: the date in the name must not postdate `built=`
    _pnd=$(printf '%s' "$_pn" | sed -n 's/.*\([0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]\).*/\1/p')
    _pbd=$(printf '%s' "$_pb" | cut -c1-10)
    if [ -n "$_pnd" ] && printf '%s' "$_pbd" | grep -Eq '^[0-9]{4}-[0-9]{2}-[0-9]{2}$' && [ "$_pnd" \> "$_pbd" ]; then
        printf 'FAIL payload=%s names %s, later than built=%s - a payload cannot postdate the image that carries it, so at least one of the two fields is not this cut'"'"'s' "$_pn" "$_pnd" "$_pbd"
        return 0
    fi
    printf 'PASS payload=%s is a name a bundle can match and does not postdate built=%s (its authority is the release bundle - tarball stem and SHA256SUMS - which an image cannot see, so a release body cites the tarball, never this file)' "$_pn" "${_pbd:-?}"
}

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 VERSION/package-name/identity file in the tree". That is false. I looked for those three filenames and concluded "no authority" without looking at the payload's own README. Measured now on this box, in the real c26c payload:

~/wt/payload-c26c/lemonade-onebit-rc-2026-11-05-c26c/1bit/README.txt
line 1: 1bit engine rc-2026-11-05-c26c for Linux x86_64 (Strix Halo / Ryzen AI): https://1bit.gg/

So the tag is in the image, the identity's payload=lemonade-onebit-rc-2026-11-05-c26c contains it, and #1569's rule (compare the identity's value against the tag the payload's README states) is the comparison #1557 asked for. My earlier "no 1bit/README.txt" in this thread was the same mistake one layer down: the payload nests one directory deeper than the path I probed.

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 payload= whose value carries characters no bundle name can have, and one whose embedded date is later than built= (a payload cannot postdate the image carrying it). Both are independent of the README and both are one case each.

#1569 is the smaller, stronger change. Nothing of this branch is worth carrying.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Escalating the collision note: #1591 now defines payload_name_verdict() too — that makes it three PRs, one function name, three bodies:

#1569  payload_name_verdict() { _pn=${1:-} _pt=${2:-}                 equality vs the README tag
#1571  payload_name_verdict() { _pn=${1:-} _pb=${2:-} _have=${3:-no}  skip when no payload dir
#1591  payload_name_verdict() { _pn=${1:-} _pt=${2:-}                 CONTAINMENT vs the README tag

All three in os/mini/release-check.sh; shell takes the last definition, so whichever merges second silently drops another's row, and each PR's --self-test passes against its own branch — none of the three checks can see the loss.

The pieces are complementary rather than alternatives: containment is the correct relation (#1591's finding — payload= never equals the README tag, it carries the stem), and the "no payload directory → SKIP" branch is a case the others lack (#1571). One function should take both. I have flagged this on all three.

@bong-water-water-bong
bong-water-water-bong deleted the pi-docs/1557-payload-name branch November 10, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant