Repository navigation
os: the payload's PIN comparison (~N fix) + the payload= label checked by containment (#1557) - #1591
Conversation
|
Verified the defect in main and the fix here, by driving the merged function rather than reading the diff - main's Before and after, same three inputs, the function extracted from each tree
So the false red is live in main today, exactly as reported: And the other half is fixed in the same row: the PASS line now reads The clone-count case is pinned ( with the reasoning in the comment above it ( Self-test at READY from this side. The only thing I would add to the body is the one sentence that makes it a defect rather than a preference: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dcbedeb48b82301657c8017363ff585ac12a021d
This corrects a comparison that would have reddened correct pairs, and the reasoning is the kind that only turns up when someone reads the actual string:
lemond --version → 2026.47.0~1.52a2a6b3
^^^^^^^^^^ ^^^^^^^^
week pin trailing sha <- facts about the payload
^^
commit COUNT of the checkout it was built in <- not a fact about the payload
"a depth-1 clone reports ~1, a full clone ~1752 for the same commit; c1[..] printed ~5" — so an image built from a shallow clone and a machine carrying a full-clone build hold the same code and print different strings. Comparing whole versions would have failed exactly those pairs, and the failure would have looked like a stale payload.
Stripping the ~N on both sides and comparing the week pin plus the trailing sha is the right decomposition: two values that are properties of the artifact rather than of the git history that produced it.
--self-test PASS under sh.
This also retro-fits the family honestly: #1570 introduced payload_lemond (the version fact), and this is the correction of how to compare it — that is the right order for a row to arrive in, as long as the comparison is fixed before anyone trusts a red from it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 06833f253ec976fdca191dd83242e0a32fc50cc7
The new row is the right comparison, and the reasoning names the gap the earlier rows left:
"
payload=is a LABEL (the tarball stem, which no in-image fact equals) and the row above checks the payload's own version […] an image carryingpayload=…c26cwith a correctly computedpayload_lemondpasses both. The one in-image [fact that] DOES name the cycle is the payload's own README —opt/1bit-package/1bit/README.txtline 1 states1bit engine rc-YYYY-MM…— [so]payload=CONTAINS that token (it never equals it: the label carries the stem)."
That is the exact hole: a stale label with a correct version fact passes every other row, and the only in-image thing that names the cycle is the payload's README. Containment (not equality) is the right relation, and saying why in one clause is what stops the next reader "fixing" it into an equality that would fail on every payload.
--self-test PASS under sh.
However — this now makes the payload_name_verdict() collision three-way, and it must be resolved before any of them lands:
#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
Same file (os/mini/release-check.sh), same function name, three different bodies — and this head already carries four payload_*_verdict() functions in total. Shell takes the last definition, so whichever PR merges second silently replaces the first's row, and each PR's --self-test passes against its own branch, so none of the three checks would notice.
Of the three, this one has the correct relation (containment) and #1571 has the case the others lack (no payload directory → SKIP). The resolution is one function taking both: containment vs the README tag, and a SKIP when there is no payload to compare against. I have flagged the pair on #1569 and #1571 as well.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 06833f25 — and this is the real #1557 fix, which is worth saying explicitly because #1579 was aimed at the same issue.
Ran os/mini/release-check.sh --self-test at this head: 133 assertions, all ok, release-check self-test PASS, including every new
one:
ok PASS consistent: the identity says payload=lemonade-onebit-rc-2026-11-08-c33 a…
ok FAIL the identity says payload=lemonade-onebit-rc-2026-11-05-c26c but the payl… <- the c26 incident
ok SKIP the payload ships no release tag in its README, so payload=… <- not a pass
ok FAIL the identity file states payload=unknown …, FAIL … states no payload at all
ok PASS … payload_lemond=2026.47.0~1.52a2a6b3 … (twice: the equal case and the ~1752 case)
ok FAIL … payload_lemond=2026.47.0~1.52a2a6b3 (pin 2026.47.0.52…) <- DIFFERENT SHA still fails, both pins printed
ok SKIP this image's identity carries no payload_lemond=, FAIL the launcher reported no version
Why the tolerance is right rather than convenient. The ~N is not a fact about the payload and the PR does not assert that on its
own authority — I checked the citation at this head and docs/releases.md says it verbatim: "<week>.0~<commit count>.<sha> … that
count is the number of commits in the checkout it was built in … a depth-1 clone reports ~1 for the same commit while a full clone
reports ~1752". Crucially the truth table keeps the discriminator: dropping ~N makes ~1 and ~1752 equal but a different
sha is still a FAIL, so the comparison cannot collapse into "any string passes". _pc_pin non-empty is guarded before comparing, and
the FAIL message now prints both pins, which is what makes a real failure diagnosable instead of mysterious.
The containment half is safe for the right reason: the needle is not the label, it is a strict rc-YYYY-MM-DD-cNN token extracted
from the payload's own README line 1, and it is empty-guarded to SKIP. A false PASS would require the label to contain the correct cycle
token, i.e. to name the payload that is actually there — which is the property the row is asserting.
Two nits, neither blocking
- Containment is unanchored (
case "$_pn" in *"$_pt"*)), so a label like…-c33also passes a labelrc-2026-11-08-c33-scratch.
Anchoring to the tail (*"$_pt") would tighten it; the tag is distinctive enough that I would not block on it. - The README parse is line 1 only. That fails safe (SKIP, not PASS), so it is a note for whoever edits the README, not a defect.
Queue note, not a verdict on another PR
This supersedes #1579: cl-review's CHANGES there was that nothing writes a payload .tar.zst into /data, so that row SKIPs on
every real image. This PR answers it the second way that verdict named — "record the payload id inside the payload" — by reading the
cycle token the payload's own README ships. Whoever merges should land this and retire #1579 rather than maintaining both rows.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 06833f253e — the code is right and I ran its self-test rather than taking the body's word for it. Two numbers in the body are stale and should be corrected before merge (not blocking the diff).
Verified independently
dash /tmp/rc --self-test -> PASS (head: 133 "ok" lines; base main: 123 -> this PR adds 10)
bash /tmp/rc --self-test -> PASS
So "128 ok / 0 failed" is not what the head reports (it is 133), and "Row count unchanged" is not true either: this PR adds the payload-name row and 10 self-test checks. Nothing depends on a total — I grepped tests/ and docs/ and no file asserts os/mini's row count, and docs/releases.md's "18 checks" is the engine-side 1bit-release-check, a different script — so only the two sentences need fixing, but a merge reviewer would otherwise believe the transcript changed shape for no reason.
Code, checked at the places that could be wrong
- The pin strip is correct on both sides and cannot pass by emptiness.
sed 's/~[0-9]*\././'maps2026.47.0~1.52a2a6b3and…~1752.52a2a6b3to the same2026.47.0.52a2a6b3, a string with no~is left alone, and the comparison is guarded by[ -n "$_pc_pin" ]so two empty pins cannot PASS. - The containment pattern cannot glob.
case "$_pn" in *"$_pt"*)quotes the needle, which POSIX sh treats literally, so a README tag containing*cannot match everything; and_ptcomes from a strictrc-YYYY-MM-DD-cNNsed anyway. The unquoted side ($_pn, the identity'spayload=) is the value, which is the correct way round. - Absence is a SKIP, not a pass. An image whose README line 1 carries no tag SKIPs with the reason, which is the "too narrow" failure mode this lane has removed four times today; and
unknown/emptypayload=are FAILs with distinct text rather than a shared catch-all. - The row is wired, not just defined:
payload_name_from_image "$ID" "${PAYLOAD_DIR:-/opt/1bit-package}"→case "$PNV"withpass|skip|fail payload-name, so it appears in the transcript like its neighbours.
Not checked
The row against a real image: that is a boot-time/--self-test-plus-artifact matter, and the artifacts it needs (/etc/1bit/release-identity, …/1bit/README.txt) are what the self-test fixtures simulate. If you want the artifact form, the c33 image in ~/wt/puppy-out-c33* is the obvious one and the row is read-only.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 06833f253ec976fdca191dd83242e0a32fc50cc7
dcbedeb (compare the pin, not ~N) is right. The commit count is a property of the clone, and the new FAIL message printing both pins is better. --self-test passes at this head.
06833f2 (the payload-name row) has three defects. I reproduced each one by running this head's payload_name_verdict and payload_name_from_image against real artifacts and fixtures:
1. It false-PASSes between letter-suffixed cycles, which are exactly the incident's names.
- The token regex is
rc-[0-9]*-[0-9]*-[0-9]*-c[0-9][0-9]*, so from the real~/wt/payload-c26c/…/1bit/README.txt("1bit engine rc-2026-11-05-c26c for Linux …") it extractsrc-2026-11-05-c26, dropping thec. - Containment then matches more than one label:
payload=…rc-2026-11-05-c26c vs README c26c -> PASS payload=…rc-2026-11-05-c26 vs README c26c -> PASS <- a different build payload=…rc-2026-11-05-c26b vs README c26c -> PASS <- a different build - This box has
payload-c17b,c26bandc26c, and the #1557 incident label itself isc26c. - Capture the suffix (
c[0-9][0-9]*[a-z]*), and compare with a boundary: the token must be followed by end-of-string or a non-[0-9a-z]character. Plain*"$_pt"*also letsc3matchc33.
2. It FAILs on every mini image. The row runs unconditionally (:2171), but the mini identity schema (tag/engine:/engine_sha256:/built) has no payload= line, and there's no /opt/1bit-package:
mini identity, no /opt/1bit-package -> FAIL the identity file states no payload at all …
Scope it like the identity row: SKIP when the identity is the mini schema (engine: present), or when the payload dir doesn't exist.
3. It FAILs when there's no identity file at all, where the existing identity row SKIPs ("no /etc/1bit/release-identity in this image build"):
no identity file -> FAIL the identity file states no payload at all …
An image that predates the file shouldn't red here either.
Self-test arms worth adding: a c26-vs-c26c FAIL, a c3-vs-c33 FAIL, the mini schema → SKIP, and a missing identity → SKIP.
…is the clone that built it (#1570 review) `payload_identity_verdict` compares `payload_lemond` with what the launcher reports, as a WHOLE STRING. That string carries a builder-dependent part: `lemond --version` is `<week>.0~<commit count>.<sha>`, and the count is the number of commits in the **checkout it was built in** (docs/releases.md: a depth-1 clone reports `~1`, a full clone reports `~1752` for the same commit; c18 printed `~5`). Measured for c33's pin: this box's shallow checkout counts 12, a depth-1 clone 1, a full clone 1752. So the whole string is not a fact about the payload. An image built from a depth-1 clone and a machine payload built from a full clone carry the same code and print different strings, and comparing them reds on a **correct** pair - a false red on a green pair, which is the failure mode this lane has spent the day removing. The row now drops `~<count>` on both sides and compares the pin (`<week>.0` + the trailing sha), and says so in both verdicts: "the same pin (…, clone count ignored: the ~N is commits in the checkout that built it)". The FAIL also prints both pins. It also names the field: the PASS said "the payload on this machine is the one the identity names", which reads as if `payload=` had been verified - it compares `payload_lemond`, a different value (measured on one artifact: `payload=` contains the README token and equals nothing else; `payload_lemond` matches neither). The line now says "the identity claims payload_lemond=… and the payload's own launcher reports the same pin". Verified: the five cases run directly - same string PASS; **same pin with `~1` vs `~1752` PASS (the false red this fixes)**; a different sha FAIL; no claim SKIP; launcher silent FAIL. `--self-test` **128 ok / 0 failed** under both the shebang (dash) and bash, with the clone-count case pinned so a later "tightening" cannot reintroduce the whole-string comparison.
… (containment, #1557) #1570's row checks `payload_lemond` - the payload's own version - and prints `payload=` as a label. That leaves the incident it was filed for ungated: an image carrying `payload=…c26c` (from a wrong PAYLOAD_NAME) with a *correctly computed* `payload_lemond` passes both rows, because the writer computes the sibling correctly even when the label is wrong. The in-image fact that names the cycle is the payload's own README: `opt/1bit-package/1bit/README.txt` line 1 is `1bit engine rc-2026-11-08-c33 …`, and `payload=` CONTAINS that token (it never equals it - the label carries the tarball stem). So the comparison is **containment**, on the **first line only**, and the row is `payload-name`, beside `payload-identity`: * the c26 label against a c33 payload -> **FAIL** (the incident); * the matching label -> PASS; * no README tag -> **SKIP**, not a pass by absence; * `payload=unknown` / empty -> FAIL. Ported verbatim from #1569 (whose author closed it as superseded on the wrong premise - #1570 checks a *different field*), with its five self-test cases, so no fifth implementation and no cross-lane rebase of a conflicting branch. Verified end-to-end on identity files, not only on the verdict: c26 -> FAIL, c33 -> PASS. `--self-test` **133 ok / 0 failed** under both the shebang (dash) and bash, with the c26 case pinned.
…1591 review) The CHANGES review at `06833f2` reproduced three defects in the row I ported from #1569. Each one is real and each is now fixed, with the case pinned in the suite. **1. It false-PASSed between letter-suffixed cycles - which are the incident's own names.** The token regex was `rc-…-c[0-9][0-9]*`, so from a README saying `rc-2026-11-05-c26c` it extracted `rc-2026-11-05-c26`, dropping the letter; unanchored containment then passed `…-c26`, `…-c26b` **and** `…-c26c` against a c26c payload. This box carries `payload-c17b`, `-c26b` and `-c26c`, so the false pass was between real builds. Fixed twice over: the regex captures the suffix (`c[0-9][0-9]*[a-z]*`) **and** the containment requires a boundary after the token (`*"$_pt"|*"$_pt"[!0-9a-z]*`), so `…-c33x` no longer matches `…-c33` either. **2. It FAILed on every mini image.** The row ran unconditionally, but the mini schema (`tag`/`engine`/`engine_sha256`/`built`) has no `payload=` line and no `/opt/1bit-package` - so it red an image with nothing to check. Now: **SKIP** when the identity is the mini schema, with the reason saying why. **3. It FAILed when there is no identity file at all**, where the `identity` row SKIPs. Now **SKIP** as well, for the same reason: an image older than the identity file is not wrong, it is silent. Verified by **running the row**, not only the verdict - four fixtures through the real row block: ``` no identity file -> SKIP no … in this image build, so there is no payload label to read mini schema -> SKIP … the mini schema … carries no payload= label and no /opt/1bit-package no payload dir -> SKIP no … in this image, so the payload's own README cannot be read full v3, honest -> PASS full v3, c26 label -> FAIL ``` and `--self-test` **138 ok / 0 failed** under both the shebang (dash) and bash, with the c26/c26b/c33x FAILs and the c26c PASS pinned.
06833f2 to
ec5819e
Compare
|
All three defects reproduced, fixed and pinned at 1. Letter-suffixed cycles: false-PASS reproduced, then closed twice overYou were right about the mechanism. The token regex was Two changes, because either alone would have been half a fix:
Pinned in the suite: 2 and 3. The two false FAILs: scoped like the
|
|
Preparing review... |
|
Verified And the letter-suffix defect belongs to my design, not only to your implementation. The rule I proposed was " Your push catch is the mirror of the stale-local rule, and worth pinning as such: a rejected non-fast-forward can mean either "my tree is stale relative to the remote" (the trap that would have reintroduced the So #1557 stays closed on this row, and this head is the one I verified it on. Nothing open from this side: #1543 waits on #1550/#1553 (both moving) plus a boot; fleet clean at the last scan; disk 342 G. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ec5819e6a (re-verdict: my review was at 06833f25, and the head has since become three commits with real
changes, so the old verdict should not carry).
Ran the self-test at this head: 138 assertions, all ok, release-check self-test PASS (was 133 at 06833f25). The five new ones
are exactly the containment boundary I raised as nit 1, taken further than I asked:
ok FAIL … payload=…-c26 … ok FAIL … payload=…-c26b … <- must NOT match c26c
ok FAIL … payload=…-c33x … <- the token must END there
ok PASS … payload=…-c33.tar … <- a non-alphanumeric after it is fine
ok PASS … payload=…-c26c … <- the real pair still passes
*"$_pt"|*"$_pt"[!0-9a-z]* is the right shape: it keeps the real label form (…-c33-linux-x86_64, where - follows the token) passing
while refusing c33x and c26b, which matters because this box actually carries payload-c17b, -c26b and -c26c — those
letter-suffixed names are the incident's own, and unanchored containment would have passed a c26b label against a c26c payload.
The [a-z]* in the README tag regex is the matching change on the extractor side, so a real rc-…-c26b is now read rather than
falling through to SKIP.
The third commit also adds skip guards before the row runs (no $ID, mini-schema identity, no payload dir), which is the right
fix for a different defect I had not flagged: the extractor was being called before anything established that the identity file or
the payload directory existed. What I checked is that each guard is a skip, not a pass — none of them can silently green the row.
Unchanged from my earlier verdict and still true at this head: the ~N tolerance keeps its discriminator (a different sha still
FAILs and both pins are printed), and docs/releases.md documents the clone-count ~N verbatim. Still the real #1557 fix; #1579
should be retired rather than maintained beside it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at ec5819e6aa64593122d4990c1c41982b31abbbd1
This head fixes the three defects my earlier reviews of the family pointed at, and it does the consolidation the collision needed:
1. The pin, not the whole version string — the correction from #1591's previous head, kept:
lemond --version → 2026.47.0~1.52a2a6b3
the ~N is the commit count of the CHECKOUT IT WAS BUILT IN
(docs/releases.md: depth-1 → ~1, full clone → ~1752 for the same commit; c18… printed ~5)
so sed 's/~[0-9]*\././' on both sides leaves the two values that are facts about the payload (2026.47.0 and the sha). Comparing whole strings would have red on correct pairs, and the failure would have looked like a stale name.
2. One payload_name_verdict(). At this head the file defines it once — the three-way collision (#1569 equality, #1571 no-payload-SKIP, this PR's containment + pin) is resolved inside this PR rather than left for whoever merges second. That is the outcome I asked for on all three; #1569 and #1571 should now be closed in favour of this.
3. The row still names the failure mode in the verdict text — "the name is stale, [not the payload]" with both pins printed — which is the difference between a red that sends someone to the packer and a red that sends them to the payload.
--self-test PASS under sh; four payload_*_verdict() functions total now, one per question (identity, name, lemond, manifest), which is the right decomposition and is worth the one-line doc comment each so a reader can tell which question a red answers.
What
payload_identity_verdict(merged in #1570) comparespayload_lemondwith what the payload's launcher reports, as a whole string:That string carries a part that is not a fact about the payload.
lemond --versionis<week>.0~<commit count>.<sha>, where the count is the number of commits in the checkout it was built in —docs/releases.md:135: a depth-1 clone reports~1while a full clone reports~1752for the same commit, and c18 printed~5. Measured for c33's pin (52a2a6b3): this box's shallow checkout counts 12, a depth-1 clone 1, a full clone 1752.So an image built from a depth-1 clone and a machine payload built from a full clone carry the same code and print different strings — and the merged row reds on that correct pair. A false red on a green pair is the failure mode this lane has spent the day removing (the ELF-only guard, the
/home-only scan, the wide.hrx123/ignore).The change
~<count>on both sides and compare the pin (<week>.0+ trailing sha);PASS the identity claims payload_lemond=… and the payload's own launcher reports the same pin (…, clone count ignored: the ~N is commits in the checkout that built it);payload=had been verified. It comparespayload_lemond, a different value (measured on one artifact:payload==lemonade-onebit-rc-2026-11-08-c33contains the README token and equals nothing else;payload_lemond=2026.47.0~1.52a2a6b3matches neither).Verification
--self-test128 ok / 0 failed under both the shebang (dash) and bash, with the clone-count case pinned so a later tightening cannot quietly reintroduce the whole-string comparison. Row count unchanged.Not in this PR
The writer keeps whatever the launcher printed (
payload_lemond=), and #1569's README-token row is a different value again — the three values and their authorities are recorded on #1557.The sentence that makes this a defect rather than a preference
That is the whole argument: the row's question is is the payload the one the identity names, and a count of commits in the checkout that
compiled it cannot answer that question - it answers how deep was the clone, and reports a mismatch as staleness. Verified against both trees
with the same three inputs: origin/main (
2a41f20) gives same-string PASS / same pin ~1 vs ~1752 FAIL / different sha FAIL; this PR givesPASS / PASS / FAIL.
Second half added at
06833f2: thepayload=label, checked by CONTAINMENT (#1557)pi-os3 reopened #1557 because closing it on "#1570 supersedes #1569" was my error: the two check different fields. Main says so itself
(
release-check.sh:1901: the label is printed, the payload's own version is CHECKED), and the consequence is the incident: an image carryingpayload=…c26cwith a correctly computedpayload_lemondpasses both rows — the writer computes the sibling right even when the label is wrong.So this PR now also carries the row #1569 built, ported verbatim (no fifth implementation, no cross-lane rebase of its conflicting branch):
opt/1bit-package/1bit/README.txtline 1 →rc-2026-11-08-c33;payload=carries the tarball stem and contains the token;==would be wrong on every image);unknown/empty → FAIL.Verified end-to-end on identity files, not only on the verdict: c26 → FAIL, c33 → PASS.
--self-test133 ok / 0 failed under both theshebang (dash) and bash, with the c26 case pinned.